How about adding KDFs? Here is a starting pointer for the most common function, PKCS #5 PBKDF2. Review appreciated.
/Simon
Simon Josefsson simon@josefsson.org writes:
How about adding KDFs? Here is a starting pointer for the most common function, PKCS #5 PBKDF2. Review appreciated.
I don't have time to review it at the moment, but I hope to be able to do that within a few days. You may also want to have a look at
http://git.lysator.liu.se/lsh/lsh/blobs/master/src/pkcs5.c
which I wrote a long time ago. Probably not very useful for Nettle as is, but I'll happily relicense it as LGPL if anything is derived from it.
One immediate comment: "PBKDF2" is an awful name :-/
Are there any other key derivation methods which are important?
Regards, /Niels
/Simon
From 2fe9180d4b0afb7acb77622d541d5be21830fc7b Mon Sep 17 00:00:00 2001 From: Simon Josefsson simon@josefsson.org Date: Wed, 12 Sep 2012 22:05:30 +0200 Subject: [PATCH] New PKCS #5 PBKDF2 function.
ChangeLog | 14 ++++++ Makefile.in | 2 + NEWS | 6 +++ nettle-internal.h | 1 + nettle.texinfo | 38 +++++++++++++++- pbkdf2.c | 103 ++++++++++++++++++++++++++++++++++++++++++++ pbkdf2.h | 49 +++++++++++++++++++++ testsuite/.gitignore | 1 + testsuite/.test-rules.make | 3 ++ testsuite/Makefile.in | 2 +- testsuite/meta-hash-test.c | 3 ++ testsuite/pbkdf2-test.c | 51 ++++++++++++++++++++++ 12 files changed, 270 insertions(+), 3 deletions(-) create mode 100644 pbkdf2.c create mode 100644 pbkdf2.h create mode 100644 testsuite/pbkdf2-test.c
diff --git a/ChangeLog b/ChangeLog index fe61ad9..a35f747 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,17 @@ +2012-09-12 Simon Josefsson simon@josefsson.org
- NEWS: Mention addition of PBKDF2.
- pbkdf2.c (pbkdf2_hmac): New file and function.
- pbkdf2.h: Declare it.
- Makefile.in (nettle_SOURCES): Add pbkdf2.c.
- (HEADERS): Add pbkdf2.h.
- testsuite/pbkdf2-test.c: New test case.
- nettle-internal.h (NETTLE_MAX_HASH_CONTEXT_SIZE): New constant.
- testsuite/meta-hash-test.c (test_main): Validate NETTLE_MAX_HASH_CONTEXT_SIZE.
- nettle.texinfo (Key derivation functions): New section.
- testsuite/Makefile.in (TS_NETTLE_SOURCES): Add pbkdf2-test.c.
- testsuite/.test-rules.make (pbkdf2-test): New target.
2012-09-10 Niels Möller nisse@lysator.liu.se
- examples/eratosthenes.c (main): Explicitly deallocate storage
diff --git a/Makefile.in b/Makefile.in index cf93593..7c6cf33 100644 --- a/Makefile.in +++ b/Makefile.in @@ -77,6 +77,7 @@ nettle_SOURCES = aes-decrypt-internal.c aes-decrypt.c \ des3.c des-compat.c \ hmac.c hmac-md5.c hmac-ripemd160.c hmac-sha1.c \ hmac-sha224.c hmac-sha256.c hmac-sha384.c hmac-sha512.c \
knuth-lfib.c \ md2.c md2-meta.c md4.c md4-meta.c \ md5.c md5-compress.c md5-compat.c md5-meta.c \pbkdf2.c \
@@ -123,6 +124,7 @@ HEADERS = aes.h arcfour.h arctwo.h asn1.h bignum.h blowfish.h \ cbc.h ctr.h gcm.h \ des.h des-compat.h dsa.h \ hmac.h \
knuth-lfib.h \ macros.h \ md2.h md4.h \pbkdf2.h \
diff --git a/NEWS b/NEWS index 4957f80..ea846a7 100644 --- a/NEWS +++ b/NEWS @@ -1,3 +1,9 @@
- New features:
- Support for PKCS #5 PBKDF2. Contributed by Simon Josefsson.
Specification in RFC 2898 and test vectors in RFC 6070.
NEWS for the 2.5 release
This release includes important portability fixes for Windows diff --git a/nettle-internal.h b/nettle-internal.h index e85e3c5..a7a37f8 100644 --- a/nettle-internal.h +++ b/nettle-internal.h @@ -48,6 +48,7 @@ do { if (size > (sizeof(name) / sizeof(name[0]))) abort(); } while (0) #define NETTLE_MAX_BIGNUM_SIZE ((NETTLE_MAX_BIGNUM_BITS + 7)/8) #define NETTLE_MAX_HASH_BLOCK_SIZE 128 #define NETTLE_MAX_HASH_DIGEST_SIZE 64 +#define NETTLE_MAX_HASH_CONTEXT_SIZE 2 #define NETTLE_MAX_SEXP_ASSOC 17 #define NETTLE_MAX_CIPHER_BLOCK_SIZE 32
diff --git a/nettle.texinfo b/nettle.texinfo index 4904d91..d887ed9 100644 --- a/nettle.texinfo +++ b/nettle.texinfo @@ -70,6 +70,7 @@ Reference
- Cipher functions::
- Cipher modes::
- Keyed hash functions::
+* Key derivation functions::
- Public-key algorithms::
- Randomness::
- Ascii encoding::
@@ -316,6 +317,7 @@ This chapter describes all the Nettle functions, grouped by family.
- Cipher functions::
- Cipher modes::
- Keyed hash functions::
+* Key derivation functions::
- Public-key algorithms::
- Randomness::
- Ascii encoding::
@@ -1852,7 +1854,7 @@ only the first @var{length} octets of the digest are written.
-@node Keyed hash functions, Public-key algorithms, Cipher modes, Reference +@node Keyed hash functions, Key derivation functions, Cipher modes, Reference @comment node-name, next, previous, up @section Keyed Hash Functions
@@ -2102,7 +2104,39 @@ This function also resets the context for processing new messages, with the same key. @end deftypefun
-@node Public-key algorithms, Randomness, Keyed hash functions, Reference +@node Key derivation functions, Public-key algorithms, Keyed hash functions, Reference +@comment node-name, next, previous, up +@section Key derivation Functions
+@cindex Key Derivation Function +@cindex Password Based Key Derivation Function +@cindex PKCS #5 +@cindex KDF +@cindex PBKDF
+A @dfn{key derivation function} (@acronym{KDF}) is a function that from +a given symmetric key derives other symmetric keys. A sub-class of KDFs +is the @dfn{password-based key derivation functions} (@acronym{PBKDFs}), +which take as input a password or passphrase, and its purpose is +typically to strengthen it and protect against certain pre-computation +attacks by using salting and expensive computation. The most well known +PBKDF is the @code{PKCS #5 PBKDF2} described in @cite{RFC 2898} which +uses a pseudorandom function such as @acronym{HMAC-SHA1}.
+Nettle's @acronym{PBKDF2} function is defined in @file{<nettle/pbkdf2.h>}. +It contains a function:
+@deftypefun void pbkdf2_hmac (unsigned @var{Plen}, const uint8_t *@var{P}, unsigned @var{Slen}, const uint8_t *@var{S}, const struct nettle_hash *@var{hash}, unsigned int @var{c}, unsigned @var{dkLen}, uint8_t *@var{DK})
+Derive symmetric key from a password according to PKCS #5 PBKDF2. The +PRF is the HMAC familly with @var{hash} indicating the underlying hash +function. Inputs are the password @var{P} of length @var{Plen}, the +salt @var{S} of length @var{Slen}, the iteration counter @var{C} (> 0), +and the desired derived output length @var{dkLen}. The output buffer is +@var{DK} which must have room for at least @var{dkLen} octets. +@end deftypefun
+@node Public-key algorithms, Randomness, Key derivation functions, Reference @comment node-name, next, previous, up @section Public-key algorithms
diff --git a/pbkdf2.c b/pbkdf2.c new file mode 100644 index 0000000..eab8f0a --- /dev/null +++ b/pbkdf2.c @@ -0,0 +1,103 @@ +/* pbkdf2.c
- PKCS #5 password-based key derivation function PBKDF2, see RFC 2898.
- */
+/* nettle, low-level cryptographics library
- Copyright (C) 2012 Simon Josefsson
- The nettle library is free software; you can redistribute it and/or modify
- it under the terms of the GNU Lesser General Public License as published by
- the Free Software Foundation; either version 2.1 of the License, or (at your
- option) any later version.
- The nettle library is distributed in the hope that it will be useful, but
- WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
- or FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public
- License for more details.
- You should have received a copy of the GNU Lesser General Public License
- along with the nettle library; see the file COPYING.LIB. If not, write to
- the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
- MA 02111-1301, USA.
- */
+#if HAVE_CONFIG_H +# include "config.h" +#endif
+#include <assert.h> +#include <stdlib.h> +#include <string.h>
+#include "pbkdf2.h"
+#include "hmac.h" +#include "nettle-internal.h"
+void +pbkdf2_hmac (unsigned Plen, const uint8_t * P,
unsigned Slen, const uint8_t * S,
const struct nettle_hash *hash,
unsigned int c, unsigned dkLen, uint8_t * DK)
+{
- unsigned int hLen = hash->digest_size;
- char U[NETTLE_MAX_HASH_DIGEST_SIZE];
- char T[NETTLE_MAX_HASH_DIGEST_SIZE];
- unsigned int u;
- unsigned int l;
- unsigned int r;
- unsigned int i;
- unsigned int k;
- char tmp[4];
- TMP_DECL (inner, uint8_t, NETTLE_MAX_HASH_CONTEXT_SIZE);
- TMP_DECL (outer, uint8_t, NETTLE_MAX_HASH_CONTEXT_SIZE);
- TMP_DECL (state, uint8_t, NETTLE_MAX_HASH_CONTEXT_SIZE);
- if (c == 0)
- return;
- if (dkLen == 0)
- return;
- l = ((dkLen - 1) / hLen) + 1;
- r = dkLen - (l - 1) * hLen;
- TMP_ALLOC (inner, hash->context_size);
- TMP_ALLOC (outer, hash->context_size);
- TMP_ALLOC (state, hash->context_size);
- for (i = 1; i <= l; i++)
- {
memset (T, 0, hLen);
for (u = 1; u <= c; u++)
- {
hmac_set_key (outer, inner, state, hash, Plen, P);
if (u == 1)
{
tmp[0] = (i & 0xff000000) >> 24;
tmp[1] = (i & 0x00ff0000) >> 16;
tmp[2] = (i & 0x0000ff00) >> 8;
tmp[3] = (i & 0x000000ff) >> 0;
hmac_update (state, hash, Slen, S);
hmac_update (state, hash, 4, tmp);
}
else
{
hmac_set_key (outer, inner, state, hash, Plen, P);
hmac_update (state, hash, hLen, U);
}
hmac_digest (outer, inner, state, hash, hLen, U);
for (k = 0; k < hLen; k++)
T[k] ^= U[k];
- }
memcpy (DK + (i - 1) * hLen, T, i == l ? r : hLen);
- }
+} diff --git a/pbkdf2.h b/pbkdf2.h new file mode 100644 index 0000000..f2f42c5 --- /dev/null +++ b/pbkdf2.h @@ -0,0 +1,49 @@ +/* pbkdf2.c
- PKCS #5 password-based key derivation function PBKDF2, see RFC 2898.
- */
+/* nettle, low-level cryptographics library
- Copyright (C) 2012 Simon Josefsson
- The nettle library is free software; you can redistribute it and/or modify
- it under the terms of the GNU Lesser General Public License as published by
- the Free Software Foundation; either version 2.1 of the License, or (at your
- option) any later version.
- The nettle library is distributed in the hope that it will be useful, but
- WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
- or FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public
- License for more details.
- You should have received a copy of the GNU Lesser General Public License
- along with the nettle library; see the file COPYING.LIB. If not, write to
- the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
- MA 02111-1301, USA.
- */
+#ifndef NETTLE_PBKDF2_H_INCLUDED +#define NETTLE_PBKDF2_H_INCLUDED
+#include "nettle-meta.h"
+#ifdef __cplusplus +extern "C" +{ +#endif
+/* Namespace mangling */ +#define pbkdf2_hmac nettle_pbkdf2_hmac
+void +pbkdf2_hmac (unsigned Plen, const uint8_t * P,
unsigned Slen, const uint8_t * S,
const struct nettle_hash *hash,
unsigned int c, unsigned dkLen, uint8_t * DK);
+#ifdef __cplusplus +} +#endif
+#endif /* NETTLE_PBKDF2_H_INCLUDED */ diff --git a/testsuite/.gitignore b/testsuite/.gitignore index c9f4698..b7ddd79 100644 --- a/testsuite/.gitignore +++ b/testsuite/.gitignore @@ -20,6 +20,7 @@ /dsa-test /gcm-test /hmac-test +/pbkdf2-test /knuth-lfib-test /md2-test /md4-test diff --git a/testsuite/.test-rules.make b/testsuite/.test-rules.make index 10c993f..d8f8f23 100644 --- a/testsuite/.test-rules.make +++ b/testsuite/.test-rules.make @@ -88,6 +88,9 @@ gcm-test$(EXEEXT): gcm-test.$(OBJEXT) hmac-test$(EXEEXT): hmac-test.$(OBJEXT) $(LINK) hmac-test.$(OBJEXT) $(TEST_OBJS) -o hmac-test$(EXEEXT)
+pbkdf2-test$(EXEEXT): pbkdf2-test.$(OBJEXT)
- $(LINK) pbkdf2-test.$(OBJEXT) $(TEST_OBJS) -o pbkdf2-test$(EXEEXT)
meta-hash-test$(EXEEXT): meta-hash-test.$(OBJEXT) $(LINK) meta-hash-test.$(OBJEXT) $(TEST_OBJS) -o meta-hash-test$(EXEEXT)
diff --git a/testsuite/Makefile.in b/testsuite/Makefile.in index 206a76e..86f365a 100644 --- a/testsuite/Makefile.in +++ b/testsuite/Makefile.in @@ -25,7 +25,7 @@ TS_NETTLE_SOURCES = aes-test.c arcfour-test.c arctwo-test.c \ knuth-lfib-test.c \ cbc-test.c ctr-test.c gcm-test.c hmac-test.c \ meta-hash-test.c meta-cipher-test.c meta-armor-test.c \
buffer-test.c yarrow-test.c
buffer-test.c yarrow-test.c pbkdf2-test.c
TS_HOGWEED_SOURCES = sexp-test.c sexp-format-test.c \ rsa2sexp-test.c sexp2rsa-test.c \ diff --git a/testsuite/meta-hash-test.c b/testsuite/meta-hash-test.c index 6db279b..0a8c61c 100644 --- a/testsuite/meta-hash-test.c +++ b/testsuite/meta-hash-test.c @@ -1,4 +1,5 @@ #include "testutils.h" +#include "nettle-internal.h" #include "nettle-meta.h"
const char* hashes[] = { @@ -29,6 +30,8 @@ test_main(void) while (NULL != nettle_hashes[j]) j++; ASSERT(j == count); /* we are not missing testing any hashes */
- for (j = 0; NULL != nettle_hashes[j]; j++)
- ASSERT(nettle_hashes[j]->context_size <= NETTLE_MAX_HASH_CONTEXT_SIZE); SUCCESS();
}
diff --git a/testsuite/pbkdf2-test.c b/testsuite/pbkdf2-test.c new file mode 100644 index 0000000..45d11a7 --- /dev/null +++ b/testsuite/pbkdf2-test.c @@ -0,0 +1,51 @@ +#include "testutils.h" +#include "pbkdf2.h"
+#define PBKDF2_HMAC_TEST(alg, plen, p, slen, s, c, dklen, expect) do { \
- dk[dklen] = 17; \
- pbkdf2_hmac (plen, p, slen, s, alg, c, dklen, dk); \
- ASSERT(MEMEQ (dklen, dk, expect)); \
- ASSERT(dk[dklen] == 17); \
- } while (0)
+#define MAX_DKLEN 25
+int +test_main (void) +{
- uint8_t dk[MAX_DKLEN + 1];
- /* Test vectors for PBKDF2 from RFC 6070. */
- PBKDF2_HMAC_TEST (&nettle_sha1, 8, "password", 4, "salt", 1, 20,
H("0c60c80f961f0e71f3a9b524af6012062fe037a6"));
- PBKDF2_HMAC_TEST (&nettle_sha1, 8, "password", 4, "salt", 2, 20,
H("ea6c014dc72d6f8ccd1ed92ace1d41f0d8de8957"));
- PBKDF2_HMAC_TEST (&nettle_sha1, 8, "password", 4, "salt", 4096, 20,
H("4b007901b765489abead49d926f721d065a429c1"));
+#if 0 /* too slow */
- PBKDF2_HMAC_TEST (&nettle_sha1, 8, "password", 4, "salt", 16777216, 20,
H("eefe3d61cd4da4e4e9945b3d6ba2158c2634e984"));
+#endif
- PBKDF2_HMAC_TEST (&nettle_sha1, 24, "passwordPASSWORDpassword",
36, "saltSALTsaltSALTsaltSALTsaltSALTsalt", 4096, 25,
H("3d2eec4fe41c849b80c8d83662c0e44a8b291a964cf2f07038"));
- PBKDF2_HMAC_TEST (&nettle_sha1, 9, "pass\0word", 5, "sa\0lt", 4096, 16,
H("56fa6aa75548099dcc37d7f03425e0c3"));
- /* PBKDF2-HMAC-SHA-256 test vectors confirmed with another
implementation. */
- PBKDF2_HMAC_TEST (&nettle_sha256, 6, "passwd", 4, "salt", 1, 16,
H("55ac046e56e3089fec1691c22544b605"));
- PBKDF2_HMAC_TEST (&nettle_sha256, 8, "Password", 4, "NaCl", 80000, 16,
H("4ddcd8f60b98be21830cee5ef22701f9"));
- SUCCESS ();
+}
nisse@lysator.liu.se (Niels Möller) writes:
Simon Josefsson simon@josefsson.org writes:
How about adding KDFs? Here is a starting pointer for the most common function, PKCS #5 PBKDF2. Review appreciated.
I don't have time to review it at the moment, but I hope to be able to do that within a few days. You may also want to have a look at
http://git.lysator.liu.se/lsh/lsh/blobs/master/src/pkcs5.c
which I wrote a long time ago. Probably not very useful for Nettle as is, but I'll happily relicense it as LGPL if anything is derived from it.
Possibly the block_count stuff could be used, although I'm not sure how it works: if you have a large dkLen wouldn't more than the final octet be needed?
One immediate comment: "PBKDF2" is an awful name :-/
Yes...
Are there any other key derivation methods which are important?
Widely used KDF's include the OpenPGP s2k, TLS (different variants), Kerberos. More recent KDF's include HKDF and scrypt. I'm sure there are more.
However, I'm not certain 1) it makes sense to implement several KDFs in nettle, 2) whether an generic interface can be found. Re 2) it seems KDFs are highly parametrized for its specialized purpose, and thus require special parameters.
/Simon
Simon Josefsson simon@josefsson.org writes:
Possibly the block_count stuff could be used, although I'm not sure how it works: if you have a large dkLen wouldn't more than the final octet be needed?
In general, one could have something larger. In lsh I use it only for encrypted private keys, and then I don't need anything larger than, say 256 bits key and 128 bits iv.
However, I'm not certain 1) it makes sense to implement several KDFs in nettle,
I imagine main use would be to decrypt alien password-encrypted data. E.g., gpg keyrings and private keys for any other tools (what is openssh using?) So any method which is used in more than one non-obscure application would make some sense to support.
2) whether an generic interface can be found.
I don't think we need to worry about a generic interface now. I was more thinking about proper naming for the non-generic interface. E.g, can we say "pkcs5" rather than "pbdwhatever", or are there other methods defined in pkcs#5 (or likely to be in the future)? Naming should be reasonable now, and still not break down if we add other methods later.
Regards, /Niels
nisse@lysator.liu.se (Niels Möller) writes:
Simon Josefsson simon@josefsson.org writes:
Possibly the block_count stuff could be used, although I'm not sure how it works: if you have a large dkLen wouldn't more than the final octet be needed?
In general, one could have something larger. In lsh I use it only for encrypted private keys, and then I don't need anything larger than, say 256 bits key and 128 bits iv.
Ah. I have some uses that require long outputs, and a generic implementation should support that.
However, I'm not certain 1) it makes sense to implement several KDFs in nettle,
I imagine main use would be to decrypt alien password-encrypted data. E.g., gpg keyrings and private keys for any other tools (what is openssh using?) So any method which is used in more than one non-obscure application would make some sense to support.
Not all KDFs are password-based, though, but I think it is the password-based KDFs that are most useful to support in the near term. HKDF is a recent strong candidate though.
- whether an generic interface can be found.
I don't think we need to worry about a generic interface now. I was more thinking about proper naming for the non-generic interface. E.g, can we say "pkcs5" rather than "pbdwhatever", or are there other methods defined in pkcs#5 (or likely to be in the future)? Naming should be reasonable now, and still not break down if we add other methods later.
PKCS#5 specify an obsolete PBKDF1 as well, and several other algorithms, so PKCS#5 as a name is not that unique. Possibly pkcs5_pbkdf2_hmac would be more precise, although it is longer.
/Simon
Simon Josefsson simon@josefsson.org writes:
- whether an generic interface can be found.
I don't think we need to worry about a generic interface now. I was more thinking about proper naming for the non-generic interface. E.g, can we say "pkcs5" rather than "pbdwhatever", or are there other methods defined in pkcs#5 (or likely to be in the future)? Naming should be reasonable now, and still not break down if we add other methods later.
PKCS#5 specify an obsolete PBKDF1 as well, and several other algorithms, so PKCS#5 as a name is not that unique. Possibly pkcs5_pbkdf2_hmac would be more precise, although it is longer.
I noticed this recent document that register the name "PBKDF2" and never mentions PKCS5 so I think PBKDF2 is the best name we can use.
Is there any other open issue? I could update the patch to work with the new testsuite interface, if that would help review.
/Simon
Simon Josefsson simon@josefsson.org writes:
Is there any other open issue? I could update the patch to work with the new testsuite interface, if that would help review.
Preparing an updated patch was easy, here it is and should apply to git master. This improves on a few minor issues compared to the old patch.
/Simon
Simon Josefsson simon@josefsson.org writes:
Preparing an updated patch was easy, here it is and should apply to git master. This improves on a few minor issues compared to the old patch.
Looks solid to me.
I was at first considering if one could do it without the struct nettle_hash abstraction, but since that is used for the hmac functions (I think I tried without, and that turned out to be too inconvenient), I think it makes sense to use it here as well.
--- a/nettle-internal.h +++ b/nettle-internal.h @@ -48,6 +48,7 @@ do { if (size > (sizeof(name) / sizeof(name[0]))) abort(); } while (0) #define NETTLE_MAX_BIGNUM_SIZE ((NETTLE_MAX_BIGNUM_BITS + 7)/8) #define NETTLE_MAX_HASH_BLOCK_SIZE 128 #define NETTLE_MAX_HASH_DIGEST_SIZE 64 +#define NETTLE_MAX_HASH_CONTEXT_SIZE 216 #define NETTLE_MAX_SEXP_ASSOC 17 #define NETTLE_MAX_CIPHER_BLOCK_SIZE 32
I'm a bit uncomfortable with that magic number. If sha512_ctx is the largest one, writing sizeof(struct sha512_ctx) is clearer. Or one could even go for sizeof(union { struct struct sha512_ctx sha512; struct foo_ctx foo; ... }).
+Derive symmetric key from a password according to PKCS #5 PBKDF2. The +PRF is the HMAC familly with @var{hash} indicating the underlying hash
s/familly/family/
+void +pbkdf2_hmac (unsigned Plen, const uint8_t * P,
unsigned Slen, const uint8_t * S,
const struct nettle_hash *hash,
unsigned int c, unsigned dkLen, uint8_t * DK)
+{
- unsigned int hLen = hash->digest_size;
- char U[NETTLE_MAX_HASH_DIGEST_SIZE];
- char T[NETTLE_MAX_HASH_DIGEST_SIZE];
[...]
- TMP_DECL (inner, uint8_t, NETTLE_MAX_HASH_CONTEXT_SIZE);
- TMP_DECL (outer, uint8_t, NETTLE_MAX_HASH_CONTEXT_SIZE);
- TMP_DECL (state, uint8_t, NETTLE_MAX_HASH_CONTEXT_SIZE);
Any good reason to allocate the digests and the contexts in different ways? (One issue with the current hash and hmac interfaces is that all three of inner, outer and state include a buffer, while we really need only one).
Regards, /Niels
nisse@lysator.liu.se (Niels Möller) writes:
Simon Josefsson simon@josefsson.org writes:
Preparing an updated patch was easy, here it is and should apply to git master. This improves on a few minor issues compared to the old patch.
Looks solid to me.
I was at first considering if one could do it without the struct nettle_hash abstraction, but since that is used for the hmac functions (I think I tried without, and that turned out to be too inconvenient), I think it makes sense to use it here as well.
Yes, this was the trickiest part to resolve when I implemented it. However, it mimics the hmac interface, so at least there is some consistency.
--- a/nettle-internal.h +++ b/nettle-internal.h @@ -48,6 +48,7 @@ do { if (size > (sizeof(name) / sizeof(name[0]))) abort(); } while (0) #define NETTLE_MAX_BIGNUM_SIZE ((NETTLE_MAX_BIGNUM_BITS + 7)/8) #define NETTLE_MAX_HASH_BLOCK_SIZE 128 #define NETTLE_MAX_HASH_DIGEST_SIZE 64 +#define NETTLE_MAX_HASH_CONTEXT_SIZE 216 #define NETTLE_MAX_SEXP_ASSOC 17 #define NETTLE_MAX_CIPHER_BLOCK_SIZE 32
I'm a bit uncomfortable with that magic number. If sha512_ctx is the largest one, writing sizeof(struct sha512_ctx) is clearer. Or one could even go for sizeof(union { struct struct sha512_ctx sha512; struct foo_ctx foo; ... }).
Did you notice that testsuite/meta-hash-test.c was modified as well to make sure the magic number is OK? A sizeof or sizeof-union could work too, but then nettle-internal.h would need more #include's.
+Derive symmetric key from a password according to PKCS #5 PBKDF2. The +PRF is the HMAC familly with @var{hash} indicating the underlying hash
s/familly/family/
Right.
+void +pbkdf2_hmac (unsigned Plen, const uint8_t * P,
unsigned Slen, const uint8_t * S,
const struct nettle_hash *hash,
unsigned int c, unsigned dkLen, uint8_t * DK)
+{
- unsigned int hLen = hash->digest_size;
- char U[NETTLE_MAX_HASH_DIGEST_SIZE];
- char T[NETTLE_MAX_HASH_DIGEST_SIZE];
[...]
- TMP_DECL (inner, uint8_t, NETTLE_MAX_HASH_CONTEXT_SIZE);
- TMP_DECL (outer, uint8_t, NETTLE_MAX_HASH_CONTEXT_SIZE);
- TMP_DECL (state, uint8_t, NETTLE_MAX_HASH_CONTEXT_SIZE);
Any good reason to allocate the digests and the contexts in different ways? (One issue with the current hash and hmac interfaces is that all three of inner, outer and state include a buffer, while we really need only one).
Ah, no reason really. I wrote the inner/outer/state part later, after settling on the nettle_hash abstraction, so this was code inspired by hmac.c. I found the hmac interface a bit odd here, so there may be better ways to do this.
/Simon
Simon Josefsson simon@josefsson.org writes:
Did you notice that testsuite/meta-hash-test.c was modified as well to make sure the magic number is OK?
Yes, that's good, but I'd still prefer to have it defined in terms of sizeof.
A sizeof or sizeof-union could work too, but then nettle-internal.h would need more #include's.
Since it's for internal use only, I don't think that's a problem. And including sha.h should be sufficient I guess? (I still haven't checked, but I would think sha512_ctx is the largest one).
- TMP_DECL (inner, uint8_t, NETTLE_MAX_HASH_CONTEXT_SIZE);
- TMP_DECL (outer, uint8_t, NETTLE_MAX_HASH_CONTEXT_SIZE);
- TMP_DECL (state, uint8_t, NETTLE_MAX_HASH_CONTEXT_SIZE);
[...]
Ah, no reason really. I wrote the inner/outer/state part later, after settling on the nettle_hash abstraction, so this was code inspired by hmac.c. I found the hmac interface a bit odd here, so there may be better ways to do this.
Hmm. I don't think using TMP_DECL like that is right. If HAVE_ALLOCA, then it's going to be a plain alloca, which is what we really want. The problem is the fallback case, when we don't use allloca. Then it expands to
uint8_t inner[NETTLE_MAX_HASH_CONTEXT_SIZE];
which may not be properly aligned.
We shouldn't use malloc here, so if we can't think up something completely different, I think nettle-internal.h needs to define some union type which makes the C compiler to provide sufficient space and proper alignment. Somewhat like sockaddr_storage.
And the hmac code uses TMP_DECL/TMP_ALLOC for input blocks and digests, not for the context structs.
(Maybe it should be designed differently? I'll send a separate reply on that).
Regards, /Niels
nisse@lysator.liu.se (Niels Möller) writes:
Simon Josefsson simon@josefsson.org writes:
Did you notice that testsuite/meta-hash-test.c was modified as well to make sure the magic number is OK?
Yes, that's good, but I'd still prefer to have it defined in terms of sizeof.
A sizeof or sizeof-union could work too, but then nettle-internal.h would need more #include's.
Since it's for internal use only, I don't think that's a problem. And including sha.h should be sufficient I guess? (I still haven't checked, but I would think sha512_ctx is the largest one).
The sizes are:
md2: 84 md4: 92 md5: 92 ripemd160: 96 sha1: 96 sha224: 108 sha256: 108 sha384: 216 sha512: 216
Do you want me to use this approach in any updated patch?
- TMP_DECL (inner, uint8_t, NETTLE_MAX_HASH_CONTEXT_SIZE);
- TMP_DECL (outer, uint8_t, NETTLE_MAX_HASH_CONTEXT_SIZE);
- TMP_DECL (state, uint8_t, NETTLE_MAX_HASH_CONTEXT_SIZE);
[...]
Ah, no reason really. I wrote the inner/outer/state part later, after settling on the nettle_hash abstraction, so this was code inspired by hmac.c. I found the hmac interface a bit odd here, so there may be better ways to do this.
Hmm. I don't think using TMP_DECL like that is right. If HAVE_ALLOCA, then it's going to be a plain alloca, which is what we really want. The problem is the fallback case, when we don't use allloca. Then it expands to
uint8_t inner[NETTLE_MAX_HASH_CONTEXT_SIZE];
which may not be properly aligned.
We shouldn't use malloc here, so if we can't think up something completely different, I think nettle-internal.h needs to define some union type which makes the C compiler to provide sufficient space and proper alignment. Somewhat like sockaddr_storage.
And the hmac code uses TMP_DECL/TMP_ALLOC for input blocks and digests, not for the context structs.
(Maybe it should be designed differently? I'll send a separate reply on that).
I'm hoping this issue goes away with your new proposed interface, I think it does.
/Simon
Simon Josefsson simon@josefsson.org writes:
+void +pbkdf2_hmac (unsigned Plen, const uint8_t * P,
unsigned Slen, const uint8_t * S,
const struct nettle_hash *hash,
unsigned int c, unsigned dkLen, uint8_t * DK);
Maybe it would make more sense for pbkdf2 to use an arbitrary mac? The caller would provide the mac an dinitialize it with the password. And then the pbkdf2 functions takes the mac, the salt, count, and generates the key. Like
void pbkdf2 (void *mac_ctx, unsigned digest_size, nettle_hash_update_func *update, nettle_hash_digest_func *digest, unsigned length, uint8_t *dst, unsigned iterations, unsigned salt_length, const uint8_t *salt);
Example usage:
hmac_sha1_ctx ctx; uint8_t key[57];
hmac_sha1_set_key (&ctx, 8, "password"); pbkdf2 (&ctx, SHA1_DIGEST_SIZE, hmac_sha1_update, hmac_sha1_digest, sizeof(key), key, 4711, 6, "pepper");
Would that make sense? I guess one may also want some convenience macros/functions for using hmac-sha1 etc.
I think that design would even make the implementation more natural.
Regards, /Niels
nisse@lysator.liu.se (Niels Möller) writes:
Simon Josefsson simon@josefsson.org writes:
+void +pbkdf2_hmac (unsigned Plen, const uint8_t * P,
unsigned Slen, const uint8_t * S,
const struct nettle_hash *hash,
unsigned int c, unsigned dkLen, uint8_t * DK);
Maybe it would make more sense for pbkdf2 to use an arbitrary mac?
Yes, PBKDF2 is actually defined to apply to any PRF generally.
The caller would provide the mac an dinitialize it with the password. And then the pbkdf2 functions takes the mac, the salt, count, and generates the key. Like
void pbkdf2 (void *mac_ctx, unsigned digest_size, nettle_hash_update_func *update, nettle_hash_digest_func *digest, unsigned length, uint8_t *dst, unsigned iterations, unsigned salt_length, const uint8_t *salt);
This feels a bit inconsistent with the hmac interface, but it is slightly more general. In practice, I'm not aware of anyone using PBKDF2 with anything non-HMAC though.
Example usage:
hmac_sha1_ctx ctx; uint8_t key[57];
hmac_sha1_set_key (&ctx, 8, "password"); pbkdf2 (&ctx, SHA1_DIGEST_SIZE, hmac_sha1_update, hmac_sha1_digest, sizeof(key), key, 4711, 6, "pepper");
Would that make sense? I guess one may also want some convenience macros/functions for using hmac-sha1 etc.
I suppose this would work. PBKDF2 invokes the PRF many times with different inputs (however always with the same key). It seems hmac_sha1_digest reset the context for new use.
Do you want me to submit an updated patch?
/Simon
Simon Josefsson simon@josefsson.org writes:
This feels a bit inconsistent with the hmac interface,
Anything in particular which you think is inconsistent?
HMAC is a bit special both in theory and practice. In that it isn't defined on top of any arbitrary hash function, it's defined only for hash functions in the Merkle-Damgård family (if I get the terminology right), and needs to know the underlying blocksize, which is usually considered an internal property of the hash function.
And then nettle's hmac_update and hmac_digest are a bit peculiar, since they avoid using types from HMAC_CTX, and use const for the two contexts which depend on the key only.
PBDKF2 is a more regular construction, which doesn't depend on the internals of the underlying mac/prf.
Do you want me to submit an updated patch?
It would be great if you could try out the proposed interface. And if it works out well, submit a new patch.
Regards, /Niels
nisse@lysator.liu.se (Niels Möller) writes:
Simon Josefsson simon@josefsson.org writes:
This feels a bit inconsistent with the hmac interface,
Anything in particular which you think is inconsistent?
That the HMAC interface takes this parameter:
const struct nettle_hash *hash
but the new PBKDF2 interface would take these:
void *mac_ctx, unsigned digest_size, nettle_hash_update_func *update, nettle_hash_digest_func *digest
which feels low-level, especially considering that those parameters are captured through the nettle_hash abstraction together with hard-coding PBKDF2 for HMAC.
However, I suppose this complexity could be hidden with a utility function 'pbkdf2_hmac' that is similar to my original function signature. I believe HMAC-based PBKDF2's are important enough to warrant a utility function for easy use. Then the flexibility is there in Nettle if someone wants to use PBKDF2 for non-HMACs in the future.
Do you want me to submit an updated patch?
It would be great if you could try out the proposed interface. And if it works out well, submit a new patch.
See patch below (only pbkdf2.h, pbkdf2.c, and testsuite/pbkdf2-test.c update, the rest of the patch is not updated). It works, however I get warnings because there is a type conflict:
pbkdf2-test.c:27:3: warning: passing argument 3 of 'nettle_pbkdf2' from incompatible pointer type [enabled by default] ../pbkdf2.h:40:1: note: expected 'void (*)(void *, unsigned int, const uint8_t *)' but argument is of type 'void (*)(struct hmac_sha1_ctx *, unsigned int, const uint8_t *)'
The reason is that pbkdf2 has this signature:
void pbkdf2 (void *mac_ctx, unsigned digest_size, nettle_hash_update_func *update, nettle_hash_digest_func *digest, unsigned length, uint8_t *dst, unsigned iterations, unsigned salt_length, const uint8_t *salt)
and nettle_hash_update_func looks like this:
typedef void nettle_hash_update_func(void *ctx, unsigned length, const uint8_t *src);
which is not compatible with any instantiation like this one:
void sha1_update(struct sha1_ctx *ctx, unsigned length, const uint8_t *data);
Casting the parameter would solve this. Is there any other way to resolve the warning? Do you think casting is an acceptable solution here? The problem seems to be that the casting needs to happen in the application not in the library.
/Simon
Simon Josefsson simon@josefsson.org writes:
nisse@lysator.liu.se (Niels Möller) writes:
That the HMAC interface takes this parameter:
const struct nettle_hash *hash
but the new PBKDF2 interface would take these:
void *mac_ctx, unsigned digest_size, nettle_hash_update_func *update, nettle_hash_digest_func *digest
which feels low-level, especially considering that those parameters are captured through the nettle_hash abstraction together with hard-coding PBKDF2 for HMAC.
The "low-level" interface is the preferred style in Nettle. See cbc_encrypt/cbc_decrypt for other examples. It's intended that the nettle_hash abstraction should be optional, and not used internally. But then it's used with hmac anyway, iirc that's because the alternative interface got *too* clumsy.
However, I suppose this complexity could be hidden with a utility function 'pbkdf2_hmac' that is similar to my original function signature.
Do you think a "half-general" pbkdf2_hmac function really is useful? I think specific wrapper functions, pbkdf2_hmac_sha256, etc, would be more useful.
pbkdf2-test.c:27:3: warning: passing argument 3 of 'nettle_pbkdf2' from incompatible pointer type [enabled by default] ../pbkdf2.h:40:1: note: expected 'void (*)(void *, unsigned int, const uint8_t *)' but argument is of type 'void (*)(struct hmac_sha1_ctx *, unsigned int, const uint8_t *)'
The reason is that pbkdf2 has this signature: typedef void nettle_hash_update_func(void *ctx, unsigned length, const uint8_t *src);
which is not compatible with any instantiation like this one:
void sha1_update(struct sha1_ctx *ctx, unsigned length, const uint8_t *data);
This is a problem shared with almost every function in nettle accepting function pointer arguments. I think we have to live with casts one way or the other; I don't see any good solution.
Casting the parameter would solve this. Is there any other way to resolve the warning? Do you think casting is an acceptable solution here? The problem seems to be that the casting needs to happen in the application not in the library.
At least specific functions like pbkdf2_hmac_sha256 functions will not expose the problem to users. It's also possible to write some macros to do the cast but preserve some type checking, something like
#define PBKDF2(ctx, update, digest, ...) \ (0 ? (update(ctx, 0, NULL), digest(ctx, 0, NULL)) \ : pbkdf(ctx, (nettle_hash_update_func *) update, (nettle_hash_digest_func *) digest, ..,))
The CBC_ENCRYPT and CBC_DECRYPT macros do this, and it seems to work fine, but it's not particularly pretty.
Any other ideas?
Thanks for the implementation, I'll try to get it integrated soon. Some minor comments (which I can fix):
--- a/nettle-internal.h +++ b/nettle-internal.h @@ -48,6 +48,7 @@ do { if (size > (sizeof(name) / sizeof(name[0]))) abort(); } while (0) #define NETTLE_MAX_BIGNUM_SIZE ((NETTLE_MAX_BIGNUM_BITS + 7)/8) #define NETTLE_MAX_HASH_BLOCK_SIZE 128 #define NETTLE_MAX_HASH_DIGEST_SIZE 64 +#define NETTLE_MAX_HASH_CONTEXT_SIZE 216 #define NETTLE_MAX_SEXP_ASSOC 17 #define NETTLE_MAX_CIPHER_BLOCK_SIZE 32
This is no longer needed, right?
+void +pbkdf2 (void *mac_ctx, unsigned digest_size,
- nettle_hash_update_func *update,
- nettle_hash_digest_func *digest,
- unsigned length, uint8_t *dst,
- unsigned iterations,
- unsigned salt_length, const uint8_t *salt)
+{
- char U[NETTLE_MAX_HASH_DIGEST_SIZE];
- char T[NETTLE_MAX_HASH_DIGEST_SIZE];
It would make sense to use TMP_ALLOC for those. And I think I'd use uint8_t rather than char (unless there's some reason for char I'm missing?).
tmp[0] = (i & 0xff000000) >> 24;
tmp[1] = (i & 0x00ff0000) >> 16;
tmp[2] = (i & 0x0000ff00) >> 8;
tmp[3] = (i & 0x000000ff) >> 0;
There's WRITE_UINT32 for this.
Regards, /Niels
nisse@lysator.liu.se (Niels Möller) writes:
However, I suppose this complexity could be hidden with a utility function 'pbkdf2_hmac' that is similar to my original function signature.
Do you think a "half-general" pbkdf2_hmac function really is useful? I think specific wrapper functions, pbkdf2_hmac_sha256, etc, would be more useful.
I agree. PBKDF2-HMAC-SHA1 is the most important. If there is also support for PBKDF2-HMAC-SHA256 then all usual cases are handled. I would actually hesistate to add convenience functions for the rest since it may lead people to use them without thinking that there are costs in using a new variant. I feel even PBKDF2-HMAC-SHA256 is wasteful, there is nothing cryptographically wrong with PBKDF2-HMAC-SHA1, but that is an uphill battle to fight...
This is a problem shared with almost every function in nettle accepting function pointer arguments. I think we have to live with casts one way or the other; I don't see any good solution.
Aha. I hadn't realized that.
Casting the parameter would solve this. Is there any other way to resolve the warning? Do you think casting is an acceptable solution here? The problem seems to be that the casting needs to happen in the application not in the library.
At least specific functions like pbkdf2_hmac_sha256 functions will not expose the problem to users. It's also possible to write some macros to do the cast but preserve some type checking, something like
#define PBKDF2(ctx, update, digest, ...) \ (0 ? (update(ctx, 0, NULL), digest(ctx, 0, NULL)) \ : pbkdf(ctx, (nettle_hash_update_func *) update, (nettle_hash_digest_func *) digest, ..,))
The CBC_ENCRYPT and CBC_DECRYPT macros do this, and it seems to work fine, but it's not particularly pretty.
Any other ideas?
This approach seems fine to me.
Thanks for the implementation, I'll try to get it integrated soon. Some minor comments (which I can fix):
I can submit an updated patch if that would speed things up. Recall that the ChangeLog, manual etc was not updated for the new interface in my most recent patch, so there is some cleaning up to do.
--- a/nettle-internal.h +++ b/nettle-internal.h @@ -48,6 +48,7 @@ do { if (size > (sizeof(name) / sizeof(name[0]))) abort(); } while (0) #define NETTLE_MAX_BIGNUM_SIZE ((NETTLE_MAX_BIGNUM_BITS + 7)/8) #define NETTLE_MAX_HASH_BLOCK_SIZE 128 #define NETTLE_MAX_HASH_DIGEST_SIZE 64 +#define NETTLE_MAX_HASH_CONTEXT_SIZE 216 #define NETTLE_MAX_SEXP_ASSOC 17 #define NETTLE_MAX_CIPHER_BLOCK_SIZE 32
This is no longer needed, right?
Once the next issue is fixed, agreed.
+void +pbkdf2 (void *mac_ctx, unsigned digest_size,
- nettle_hash_update_func *update,
- nettle_hash_digest_func *digest,
- unsigned length, uint8_t *dst,
- unsigned iterations,
- unsigned salt_length, const uint8_t *salt)
+{
- char U[NETTLE_MAX_HASH_DIGEST_SIZE];
- char T[NETTLE_MAX_HASH_DIGEST_SIZE];
It would make sense to use TMP_ALLOC for those. And I think I'd use uint8_t rather than char (unless there's some reason for char I'm missing?).
Agreed. It was copy'n'paste from my old implementation.
tmp[0] = (i & 0xff000000) >> 24;
tmp[1] = (i & 0x00ff0000) >> 16;
tmp[2] = (i & 0x0000ff00) >> 8;
tmp[3] = (i & 0x000000ff) >> 0;
There's WRITE_UINT32 for this.
Great. Same reason here.
/Simon
Simon Josefsson simon@josefsson.org writes:
I can submit an updated patch if that would speed things up. Recall that the ChangeLog, manual etc was not updated for the new interface in my most recent patch, so there is some cleaning up to do.
If you can make an updated patch for manual and ChangeLog, that'd be great. I'll take care of the code.
Regards, /Niels
nisse@lysator.liu.se (Niels Möller) writes:
Simon Josefsson simon@josefsson.org writes:
I can submit an updated patch if that would speed things up. Recall that the ChangeLog, manual etc was not updated for the new interface in my most recent patch, so there is some cleaning up to do.
If you can make an updated patch for manual and ChangeLog, that'd be great. I'll take care of the code.
Here it is.
Thanks, /Simon
Simon Josefsson simon@josefsson.org writes:
nisse@lysator.liu.se (Niels Möller) writes:
If you can make an updated patch for manual and ChangeLog, that'd be great. I'll take care of the code.
Here it is.
Thanks. Checked in now. Hope I got all the pieces. I also added a PBKDF2 macro with the casting tricks.
And a pbkdf2_hmac_sha1 function can now be implemented as follows:
void pbkdf2_hmac_sha1 (unsigned key_length, const uint8_t *key, unsigned length, uint8_t *dst, unsigned iterations, unsigned salt_length, const uint8_t *salt) { struct hmac_sha1_ctx ctx; hmac_sha1_set_key (&ctx, key_length, key); PBKDF2 (&ctx, SHA1_DIGEST_SIZE, hmac_sha1_update, hmac_sha1_digest, length, dst, iterations, salt_length, salt); }
Any final interface tweaks? Is the order of the arguments sensible and consistent with other nettle interfaces? I.e., currently
void pbkdf2 (void *mac_ctx, unsigned digest_size, nettle_hash_update_func *update, nettle_hash_digest_func *digest, unsigned length, uint8_t *dst, unsigned iterations, unsigned salt_length, const uint8_t *salt);
Hmm, looking at cbc_encrypt/cbc_decrypt, it might be more consistent to (1) put digest_size after the function pointers, and (2) put the length, dst arguments last (this depends a bit on whether or not one wants to think of the salt as an src input or as some auxillary "parameter". At east the iteration count should go earlier in the list. So maybe
void pbkdf2 (void *mac_ctx, nettle_hash_update_func *update, nettle_hash_digest_func *digest, unsigned digest_size, unsigned iterations, unsigned salt_length, const uint8_t *salt, unsigned length, uint8_t *dst);
What do you think? This *is* nit-picking, but interface consistency is important and this is the right time to tweak it.
Regards, /Niels
nisse@lysator.liu.se (Niels Möller) writes:
Simon Josefsson simon@josefsson.org writes:
nisse@lysator.liu.se (Niels Möller) writes:
If you can make an updated patch for manual and ChangeLog, that'd be great. I'll take care of the code.
Here it is.
Thanks. Checked in now. Hope I got all the pieces. I also added a PBKDF2 macro with the casting tricks.
Thank you!
And a pbkdf2_hmac_sha1 function can now be implemented as follows:
void pbkdf2_hmac_sha1 (unsigned key_length, const uint8_t *key, unsigned length, uint8_t *dst, unsigned iterations, unsigned salt_length, const uint8_t *salt) { struct hmac_sha1_ctx ctx; hmac_sha1_set_key (&ctx, key_length, key); PBKDF2 (&ctx, SHA1_DIGEST_SIZE, hmac_sha1_update, hmac_sha1_digest, length, dst, iterations, salt_length, salt); }
Could you add that, or should I submit a patch? I think having pbkdf2_hmac_sha256 as well would be good, but no others.
Any final interface tweaks? Is the order of the arguments sensible and consistent with other nettle interfaces? I.e., currently
void pbkdf2 (void *mac_ctx, unsigned digest_size, nettle_hash_update_func *update, nettle_hash_digest_func *digest, unsigned length, uint8_t *dst, unsigned iterations, unsigned salt_length, const uint8_t *salt);
Hmm, looking at cbc_encrypt/cbc_decrypt, it might be more consistent to (1) put digest_size after the function pointers, and (2) put the length, dst arguments last (this depends a bit on whether or not one wants to think of the salt as an src input or as some auxillary "parameter". At east the iteration count should go earlier in the list. So maybe
void pbkdf2 (void *mac_ctx, nettle_hash_update_func *update, nettle_hash_digest_func *digest, unsigned digest_size, unsigned iterations, unsigned salt_length, const uint8_t *salt, unsigned length, uint8_t *dst);
What do you think? This *is* nit-picking, but interface consistency is important and this is the right time to tweak it.
I prefer this new variant. The pbkdf2_hmac_* functions should follow this pattern too, so it would be:
void pbkdf2_hmac_sha1 (unsigned key_length, const uint8_t *key, unsigned iterations, unsigned salt_length, const uint8_t *salt, unsigned length, uint8_t *dst)
/Simon
Simon Josefsson simon@josefsson.org writes:
nisse@lysator.liu.se (Niels Möller) writes:
Thanks. Checked in now. Hope I got all the pieces.
Turned out I forgot to commit your test case. Fixed now.
I've also done the suggested reordering of the arguments (including the prototype in the manual).
Could you add that, or should I submit a patch?
A patch including tests and documentation would be very nice. Your prototype looks right to me,
void pbkdf2_hmac_sha1 (unsigned key_length, const uint8_t *key, unsigned iterations, unsigned salt_length, const uint8_t *salt, unsigned length, uint8_t *dst)
The declarations can go i pbkdf2.h, with implementation in separate source files pkbdf2-hmac-sha1.c and -sha256.c.
And I can now almost write "pbkdf2" correctly without thinking. Scary... ;-)
Regards, /Niels
nisse@lysator.liu.se (Niels Möller) writes:
Simon Josefsson simon@josefsson.org writes:
nisse@lysator.liu.se (Niels Möller) writes:
Thanks. Checked in now. Hope I got all the pieces.
Turned out I forgot to commit your test case. Fixed now.
I've also done the suggested reordering of the arguments (including the prototype in the manual).
Thank you.
Could you add that, or should I submit a patch?
A patch including tests and documentation would be very nice. Your prototype looks right to me,
void pbkdf2_hmac_sha1 (unsigned key_length, const uint8_t *key, unsigned iterations, unsigned salt_length, const uint8_t *salt, unsigned length, uint8_t *dst)
The declarations can go i pbkdf2.h, with implementation in separate source files pkbdf2-hmac-sha1.c and -sha256.c.
See patch below. I also improved the manual a bit.
/Simon
Simon Josefsson simon@josefsson.org writes:
See patch below. I also improved the manual a bit.
Thanks! Committed now.
--- a/pbkdf2.h +++ b/pbkdf2.h @@ -35,6 +35,8 @@ extern "C"
/* Namespace mangling */ #define pbkdf2 nettle_pbkdf2 +#define pbkdf2_hmac_sha1 nettle_pbkdf2_sha1 +#define pbkdf2_hmac_sha256 nettle_pbkdf2_sha256
I changed the symbols to nettle_pbkdf2_hmac_sha1 and nettle_pbkdf2_hmac_sha256. I take it the abbreviation was unintentional?
Regards, /Niels
nisse@lysator.liu.se (Niels Möller) writes:
--- a/pbkdf2.h +++ b/pbkdf2.h @@ -35,6 +35,8 @@ extern "C"
/* Namespace mangling */ #define pbkdf2 nettle_pbkdf2 +#define pbkdf2_hmac_sha1 nettle_pbkdf2_sha1 +#define pbkdf2_hmac_sha256 nettle_pbkdf2_sha256
I changed the symbols to nettle_pbkdf2_hmac_sha1 and nettle_pbkdf2_hmac_sha256. I take it the abbreviation was unintentional?
Right, sorry about that. Thank you!
I'll proceed cleaning up the Salsa code next, to reach my goal of having scrypt in here as well (I have it working on https://www.gitorious.org/scrypt/nettle-scrypt).
/Simon
Simon Josefsson simon@josefsson.org writes:
I'll proceed cleaning up the Salsa code next, to reach my goal of having scrypt in here as well (I have it working on https://www.gitorious.org/scrypt/nettle-scrypt).
That's the algorihtm described in https://tools.ietf.org/html/draft-josefsson-scrypt-kdf-00, right?
What features do you need for the salsa interface? After a quick look, it seems you will not be processing independent blocks with salsa20, so you will not be able to take any advantage of parallelism there (which I imagine is an intended feature of scrypt).
It's straight forward to add a round parameter to salsa20. But it's still not clear to me what's the best way to support hashing only. It would have been a bit easier if you could replace
X = Salsa(X xor B[i])
by
X = Salsa(X) xor B[i]
since the latter is closer to the standard encryption operation. I think the iteration *can* be rewritten in that form by some change of variables, but you'd need an extra xor at the end to really get Y[i] rather than Y[i] xor B[i]. The algorithm specification is already set in stone?
Regards, /Niels
nisse@lysator.liu.se (Niels Möller) writes:
Simon Josefsson simon@josefsson.org writes:
I'll proceed cleaning up the Salsa code next, to reach my goal of having scrypt in here as well (I have it working on https://www.gitorious.org/scrypt/nettle-scrypt).
That's the algorihtm described in https://tools.ietf.org/html/draft-josefsson-scrypt-kdf-00, right?
Yes.
What features do you need for the salsa interface? After a quick look, it seems you will not be processing independent blocks with salsa20, so you will not be able to take any advantage of parallelism there (which I imagine is an intended feature of scrypt).
Right. See the patch in the last e-mail for the feature I need.
It's straight forward to add a round parameter to salsa20. But it's still not clear to me what's the best way to support hashing only. It would have been a bit easier if you could replace
X = Salsa(X xor B[i])
by
X = Salsa(X) xor B[i]
since the latter is closer to the standard encryption operation. I think the iteration *can* be rewritten in that form by some change of variables, but you'd need an extra xor at the end to really get Y[i] rather than Y[i] xor B[i]. The algorithm specification is already set in stone?
Setting it stone is my goal. Scrypt has been in use for around 3 years and is implemented in several languages. Admittedly, several people (including myself) have ideas about various tweaks to the algorithm that may improve it, but I believe it is better for those ideas to be described separately under a different name. It would be extremely confusing for "scrypt" to mean anything but the algorithm described in the original paper.
/Simon
And a couple of comments on the implementation.
Simon Josefsson simon@josefsson.org writes:
- for (i = 1; i <= l; i++)
- {
memset (T, 0, hLen);
for (u = 1; u <= c; u++)
- {
hmac_set_key (outer, inner, state, hash, Plen, P);
if (u == 1)
{
tmp[0] = (i & 0xff000000) >> 24;
tmp[1] = (i & 0x00ff0000) >> 16;
tmp[2] = (i & 0x0000ff00) >> 8;
tmp[3] = (i & 0x000000ff) >> 0;
hmac_update (state, hash, Slen, S);
hmac_update (state, hash, 4, tmp);
}
else
{
hmac_set_key (outer, inner, state, hash, Plen, P);
hmac_update (state, hash, hLen, U);
}
hmac_digest (outer, inner, state, hash, hLen, U);
There's no need for all those hmac_set_key. You can set it once, and compute several macs usign the same key, each with a sequence of update, update, ..., update, digest.
If documentation or implementation doesn't agree, patches are appreciated.
for (k = 0; k < hLen; k++)
T[k] ^= U[k];
- }
And that's what memxor is for ;-)
Regards, /Niels
nettle-bugs@lists.lysator.liu.se