Re: [PATCH] DH support: add KDF handling support

[Stephan Mueller]

Am Mittwoch, 27. Juli 2016, 08:55:31 CEST schrieb David Howells:

Hi David,

> Mat Martineau  wrote:
> > > Though, shall I stuff the wrapper code back into the existing dh_compute
> > > functions or can I leave them as separate functions?
> > 
> > I'm not sure. In the existing code there's one keyctl wrapper per keyctl
> > command. A combined wrapper would need some extra logic to decide whether
> > kdfparams is passed in or not, which is different from existing code.
> 
> You shouldn't change the existing keyctl wrappers.  Feel free to add another
> one with extra arguments.

I created dh_compute_kdf and dh_compute_kdf_oi where the latter takes the 
other information from STDIN.
> 
> David



Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] DH support: add KDF handling support

[David Howells]

Mat Martineau  wrote:

> > Though, shall I stuff the wrapper code back into the existing dh_compute
> > functions or can I leave them as separate functions?
> 
> I'm not sure. In the existing code there's one keyctl wrapper per keyctl
> command. A combined wrapper would need some extra logic to decide whether
> kdfparams is passed in or not, which is different from existing code.

You shouldn't change the existing keyctl wrappers.  Feel free to add another
one with extra arguments.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] DH support: add KDF handling support

[Mat Martineau]

On Thu, 14 Jul 2016, Stephan Mueller wrote:


Am Mittwoch, 13. Juli 2016, 16:17:12 schrieb Mat Martineau:

Hi Mat,



---8<

Add the interface logic to support DH with KDF handling support.

The dh_compute code now allows the following options:

- no KDF support / output of raw DH shared secret:
 dh_compute   

- KDF support without "other information" string:
 dh_compute 


Why is  needed? Can it be determined from ?


The KDF can be considered as a random number generator that is seeded by the
shared secret. I.e. it can produce arbitrary string lengths. There is no
default string length for any given KDF.


Makes sense, thanks for the explanation.


Note, as shared secrets potentially post-processed by a KDF usually are again
used as key or data encryption keys, they need to be truncated/expanded to a
specific length anyway. A KDF inherently provides the truncation support to
any arbitrary length. Thus, I would think that the caller needs to provide
that length but does not need to truncate the output itself.



- KDF support with "other information string:
 dh_compute  

The test to verify the code is based on a test vector used for the CAVS
testing of SP800-56A.

Signed-off-by: Stephan Mueller 
---
keyctl.c | 14 +-
keyutils.c   | 48 ++
keyutils.h   | 13 +
tests/keyctl/dh_compute/valid/runtest.sh | 83
 4 files changed, 156 insertions(+), 2
deletions(-)

diff --git a/keyctl.c b/keyctl.c
index edb03de..32478b3 100644
--- a/keyctl.c
+++ b/keyctl.c
@@ -1638,14 +1638,24 @@ static void act_keyctl_dh_compute(int argc, char
*argv[])>
char *p;
int ret, sep, col;

-   if (argc != 4)
+   if (argc != 4 && argc != 6 && argc != 7)

format();

private = get_key_id(argv[1]);
prime = get_key_id(argv[2]);
base = get_key_id(argv[3]);

-   ret = keyctl_dh_compute_alloc(private, prime, base, );
+   if (argc == 4)
+   ret = keyctl_dh_compute_alloc(private, prime, base, );
+   else if (argc == 6)
+   ret = keyctl_dh_compute_kdf(private, prime, base, argv[4],
+   argv[5], NULL, );
+   else if (argc == 7)
+   ret = keyctl_dh_compute_kdf(private, prime, base, argv[4],
+   argv[5], argv[6], );
+   else
+   error("dh_compute: unknown number of arguments");
+

if (ret < 0)

error("keyctl_dh_compute_alloc");

diff --git a/keyutils.c b/keyutils.c
index 2a69304..ffdd622 100644
--- a/keyutils.c
+++ b/keyutils.c
@@ -386,6 +386,54 @@ int keyctl_dh_compute_alloc(key_serial_t private,
key_serial_t prime, }

/*
+ * fetch DH computation results processed by a KDF into an
+ * allocated buffer
+ * - resulting buffer has an extra NUL added to the end
+ * - returns count (not including extraneous NUL)
+ */
+int keyctl_dh_compute_kdf(key_serial_t private, key_serial_t prime,
+ key_serial_t base, char *len, char *kdfname,
+ char *otherinfo, void **_buffer)


All of the other keyctl_* wrappers (without the _alloc) just do the
syscall, with some packing/unpacking of structs where needed. The
allocation behavior below is only found in the *_alloc functions (in the
"utilities" section of keyutils.h) - I think it's worthwhile to have both
keyctl_dh_compute_kdf() (for pre-allocated buffers) and
keyctl_dh_compute_kdf_alloc().


Thank you for the note. I will change the code accordingly.

Though, shall I stuff the wrapper code back into the existing dh_compute
functions or can I leave them as separate functions?


I'm not sure. In the existing code there's one keyctl wrapper per keyctl 
command. A combined wrapper would need some extra logic to decide 
whether kdfparams is passed in or not, which is different from existing 
code.





+{
+   char *buf;
+   unsigned long buflen;
+   int ret;
+   struct keyctl_dh_params params = { .private = private,
+  .prime = prime,
+  .base = base };
+   struct keyctl_kdf_params kdfparams;
+
+   buflen = strtoul(len, NULL, 10);


The string to integer conversion needs to be done in
act_keyctl_dh_compute(). Length can be passed in as a size_t if it's
needed.



Ok, will do.


+   if (buflen > KEYCTL_KDF_MAX_OUTPUTLEN)
+   return -1;
+
+   buf = malloc(buflen + 1);


The other _alloc functions exist because the buffer length isn't known to
the caller in advance. If the buffer length is known in advance, the
caller should be allocating the buffer.


With the implementation of the _alloc and "non-alloc" function, I would assume
that this comment should be covered.


keyctl_dh_compute makes two syscalls: one to determine the buffer length,
and one 

Re: [PATCH] DH support: add KDF handling support

[Stephan Mueller]

Am Donnerstag, 14. Juli 2016, 04:00:57 schrieb Jeffrey Walton:

Hi Jeffrey,

> > Note, as shared secrets potentially post-processed by a KDF usually are
> > again used as key or data encryption keys, they need to be
> > truncated/expanded to a specific length anyway. A KDF inherently provides
> > the truncation support to any arbitrary length. Thus, I would think that
> > the caller needs to provide that length but does not need to truncate the
> > output itself.
> 
> As far as I know, there's no reduction in proof that a truncated hash
> is as secure as the non-truncated one. One of the reasons to provide
> the output length as a security parameter is to help avoid truncation
> and related hash output attacks.
> 
> Also see Kelsey's work on the subject;
> http://www.google.com/search?q=nist+kelsey+truncated+hash.

I understand that point. However, a KDF is not just a simple hash or 
truncation thereof. Furthermore, Kelsey is part of the NIST team that also 
developed SP800-108 (the KDF definition). Furthermore, the authors of 
SP800-56A (in particular Allen Roginsky who I met personally a number of 
times) work closely with Kelsey too.

So, I would not think that applying the standard KDF operation which is 
intended to "right-size" the DH output is nothing we should worry about.

I would rather worry about the size of the private key in the DH operation. 
The private key should be as small as cryptographically possible (e.g. 224 or 
256 bits) instead of half of the DH public key minus one (what TLS does) due 
to the reduced number of Sopie-Germain primes that are available in the latter 
case.

Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] DH support: add KDF handling support

[Jeffrey Walton]

> Note, as shared secrets potentially post-processed by a KDF usually are again
> used as key or data encryption keys, they need to be truncated/expanded to a
> specific length anyway. A KDF inherently provides the truncation support to
> any arbitrary length. Thus, I would think that the caller needs to provide
> that length but does not need to truncate the output itself.

As far as I know, there's no reduction in proof that a truncated hash
is as secure as the non-truncated one. One of the reasons to provide
the output length as a security parameter is to help avoid truncation
and related hash output attacks.

Also see Kelsey's work on the subject;
http://www.google.com/search?q=nist+kelsey+truncated+hash.

Jeff
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] DH support: add KDF handling support

[Mat Martineau]

Stephan,

On Tue, 12 Jul 2016, Stephan Mueller wrote:


Hi Mat, David,

During the development of this patch, I saw that the test
framework seems to be broken: when I change the expected
values by one bit, the test framework will still mark the
received result as PASS even though the returned data does
not match the expected data.


I tracked this down to the expect_payload function in the test framework, 
which does not properly handle multiline strings. I'll send a patch that 
adds a new expect_multiline function that should handle multiline output 
correctly.




---8<

Add the interface logic to support DH with KDF handling support.

The dh_compute code now allows the following options:

- no KDF support / output of raw DH shared secret:
 dh_compute   

- KDF support without "other information" string:
 dh_compute 


Why is  needed? Can it be determined from ?



- KDF support with "other information string:
 dh_compute  

The test to verify the code is based on a test vector used for the CAVS
testing of SP800-56A.

Signed-off-by: Stephan Mueller 
---
keyctl.c | 14 +-
keyutils.c   | 48 ++
keyutils.h   | 13 +
tests/keyctl/dh_compute/valid/runtest.sh | 83 
4 files changed, 156 insertions(+), 2 deletions(-)

diff --git a/keyctl.c b/keyctl.c
index edb03de..32478b3 100644
--- a/keyctl.c
+++ b/keyctl.c
@@ -1638,14 +1638,24 @@ static void act_keyctl_dh_compute(int argc, char 
*argv[])
char *p;
int ret, sep, col;

-   if (argc != 4)
+   if (argc != 4 && argc != 6 && argc != 7)
format();

private = get_key_id(argv[1]);
prime = get_key_id(argv[2]);
base = get_key_id(argv[3]);

-   ret = keyctl_dh_compute_alloc(private, prime, base, );
+   if (argc == 4)
+   ret = keyctl_dh_compute_alloc(private, prime, base, );
+   else if (argc == 6)
+   ret = keyctl_dh_compute_kdf(private, prime, base, argv[4],
+   argv[5], NULL, );
+   else if (argc == 7)
+   ret = keyctl_dh_compute_kdf(private, prime, base, argv[4],
+   argv[5], argv[6], );
+   else
+   error("dh_compute: unknown number of arguments");
+
if (ret < 0)
error("keyctl_dh_compute_alloc");

diff --git a/keyutils.c b/keyutils.c
index 2a69304..ffdd622 100644
--- a/keyutils.c
+++ b/keyutils.c
@@ -386,6 +386,54 @@ int keyctl_dh_compute_alloc(key_serial_t private, 
key_serial_t prime,
}

/*
+ * fetch DH computation results processed by a KDF into an
+ * allocated buffer
+ * - resulting buffer has an extra NUL added to the end
+ * - returns count (not including extraneous NUL)
+ */
+int keyctl_dh_compute_kdf(key_serial_t private, key_serial_t prime,
+ key_serial_t base, char *len, char *kdfname,
+ char *otherinfo, void **_buffer)


All of the other keyctl_* wrappers (without the _alloc) just do the 
syscall, with some packing/unpacking of structs where needed. The 
allocation behavior below is only found in the *_alloc functions (in the 
"utilities" section of keyutils.h) - I think it's worthwhile to have both 
keyctl_dh_compute_kdf() (for pre-allocated buffers) and 
keyctl_dh_compute_kdf_alloc().



+{
+   char *buf;
+   unsigned long buflen;
+   int ret;
+   struct keyctl_dh_params params = { .private = private,
+  .prime = prime,
+  .base = base };
+   struct keyctl_kdf_params kdfparams;
+
+   buflen = strtoul(len, NULL, 10);


The string to integer conversion needs to be done in 
act_keyctl_dh_compute(). Length can be passed in as a size_t if it's 
needed.



+   if (buflen > KEYCTL_KDF_MAX_OUTPUTLEN)
+   return -1;
+
+   buf = malloc(buflen + 1);


The other _alloc functions exist because the buffer length isn't known to 
the caller in advance. If the buffer length is known in advance, the 
caller should be allocating the buffer.


keyctl_dh_compute makes two syscalls: one to determine the buffer length, 
and one to do the DH calculations.



+   if (!buf)
+   return -1;
+
+   if (otherinfo) {
+   kdfparams.kdfname = kdfname;
+   kdfparams.kdfnamelen = strlen(kdfname);


If kdfname is a null-terminated string, then kdfnamelen seems 
unneccessary. I haven't reviewed the kernel side yet, but may comment more 
there. There are other examples of null-terminated string usage in the 
keyctl API, I'll comment more on this in the kernel patches.



+   kdfparams.otherinfo = otherinfo;
+   kdfparams.otherinfolen = strlen(otherinfo);


Same for otherinfolen.


+   } else {
+   kdfparams.kdfname = kdfname;
+  

[PATCH] DH support: add KDF handling support

[Stephan Mueller]

Hi Mat, David,

During the development of this patch, I saw that the test
framework seems to be broken: when I change the expected
values by one bit, the test framework will still mark the
received result as PASS even though the returned data does
not match the expected data.

---8<

Add the interface logic to support DH with KDF handling support.

The dh_compute code now allows the following options:

- no KDF support / output of raw DH shared secret:
  dh_compute   

- KDF support without "other information" string:
  dh_compute 

- KDF support with "other information string:
  dh_compute  

The test to verify the code is based on a test vector used for the CAVS
testing of SP800-56A.

Signed-off-by: Stephan Mueller 
---
 keyctl.c | 14 +-
 keyutils.c   | 48 ++
 keyutils.h   | 13 +
 tests/keyctl/dh_compute/valid/runtest.sh | 83 
 4 files changed, 156 insertions(+), 2 deletions(-)

diff --git a/keyctl.c b/keyctl.c
index edb03de..32478b3 100644
--- a/keyctl.c
+++ b/keyctl.c
@@ -1638,14 +1638,24 @@ static void act_keyctl_dh_compute(int argc, char 
*argv[])
char *p;
int ret, sep, col;
 
-   if (argc != 4)
+   if (argc != 4 && argc != 6 && argc != 7)
format();
 
private = get_key_id(argv[1]);
prime = get_key_id(argv[2]);
base = get_key_id(argv[3]);
 
-   ret = keyctl_dh_compute_alloc(private, prime, base, );
+   if (argc == 4)
+   ret = keyctl_dh_compute_alloc(private, prime, base, );
+   else if (argc == 6)
+   ret = keyctl_dh_compute_kdf(private, prime, base, argv[4],
+   argv[5], NULL, );
+   else if (argc == 7)
+   ret = keyctl_dh_compute_kdf(private, prime, base, argv[4],
+   argv[5], argv[6], );
+   else
+   error("dh_compute: unknown number of arguments");
+
if (ret < 0)
error("keyctl_dh_compute_alloc");
 
diff --git a/keyutils.c b/keyutils.c
index 2a69304..ffdd622 100644
--- a/keyutils.c
+++ b/keyutils.c
@@ -386,6 +386,54 @@ int keyctl_dh_compute_alloc(key_serial_t private, 
key_serial_t prime,
 }
 
 /*
+ * fetch DH computation results processed by a KDF into an
+ * allocated buffer
+ * - resulting buffer has an extra NUL added to the end
+ * - returns count (not including extraneous NUL)
+ */
+int keyctl_dh_compute_kdf(key_serial_t private, key_serial_t prime,
+ key_serial_t base, char *len, char *kdfname,
+ char *otherinfo, void **_buffer)
+{
+   char *buf;
+   unsigned long buflen;
+   int ret;
+   struct keyctl_dh_params params = { .private = private,
+  .prime = prime,
+  .base = base };
+   struct keyctl_kdf_params kdfparams;
+
+   buflen = strtoul(len, NULL, 10);
+   if (buflen > KEYCTL_KDF_MAX_OUTPUTLEN)
+   return -1;
+
+   buf = malloc(buflen + 1);
+   if (!buf)
+   return -1;
+
+   if (otherinfo) {
+   kdfparams.kdfname = kdfname;
+   kdfparams.kdfnamelen = strlen(kdfname);
+   kdfparams.otherinfo = otherinfo;
+   kdfparams.otherinfolen = strlen(otherinfo);
+   } else {
+   kdfparams.kdfname = kdfname;
+   kdfparams.kdfnamelen = strlen(kdfname);
+   kdfparams.otherinfo = NULL;
+   kdfparams.otherinfolen = 0;
+   }
+   ret = keyctl(KEYCTL_DH_COMPUTE, , buf, buflen, );
+   if (ret < 0) {
+   free(buf);
+   return -1;
+   }
+
+   buf[ret] = 0;
+   *_buffer = buf;
+   return ret;
+}
+
+/*
  * Depth-first recursively apply a function over a keyring tree
  */
 static int recursive_key_scan_aux(key_serial_t parent, key_serial_t key,
diff --git a/keyutils.h b/keyutils.h
index b321aa8..5026270 100644
--- a/keyutils.h
+++ b/keyutils.h
@@ -108,6 +108,16 @@ struct keyctl_dh_params {
key_serial_t base;
 };
 
+struct keyctl_kdf_params {
+#define KEYCTL_KDF_MAX_OUTPUTLEN1024/* max length of KDF output */
+#define KEYCTL_KDF_MAX_STRING_LEN   64  /* maximum length of strings */
+   char *kdfname;
+   uint32_t kdfnamelen;
+   char *otherinfo;
+   uint32_t otherinfolen;
+   uint32_t flags;
+};
+
 /*
  * syscall wrappers
  */
@@ -172,6 +182,9 @@ extern int keyctl_read_alloc(key_serial_t id, void 
**_buffer);
 extern int keyctl_get_security_alloc(key_serial_t id, char **_buffer);
 extern int keyctl_dh_compute_alloc(key_serial_t private, key_serial_t prime,
   key_serial_t base, void **_buffer);
+int keyctl_dh_compute_kdf(key_serial_t private, key_serial_t prime,
+