| Message ID | 20170420054633.14572-4-ebiggers3@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
Am Donnerstag, 20. April 2017, 07:46:31 CEST schrieb Eric Biggers: Hi Eric, > From: Eric Biggers <ebiggers@google.com> > > The result of the Diffie-Hellman computation may be shorter than the > input prime number. Only calculate the KDF over the actual result; > don't include additional uninitialized memory. Thank you for catching that (and all the rest). But I think this patch is not correct. If the DH operation results in a shorter value, the trailing part must be set to null and the KDF calculated over the entire prime length. Thus, if the DH result is shorter than the prime, the memory should look like DH result || 0x00 <as often as needed to make it prime length> || otherinfo. Thus, instead of this patch, I would think that the kmalloc call should be changed to a kzalloc. > > Signed-off-by: Eric Biggers <ebiggers@google.com> > --- > security/keys/dh.c | 22 ++++++++++------------ > 1 file changed, 10 insertions(+), 12 deletions(-) > > diff --git a/security/keys/dh.c b/security/keys/dh.c > index 1c1cac677041..a3a8607107f5 100644 > --- a/security/keys/dh.c > +++ b/security/keys/dh.c > @@ -313,17 +313,6 @@ long __keyctl_dh_compute(struct keyctl_dh_params __user > *params, goto error4; > } > > - /* > - * Concatenate SP800-56A otherinfo past DH shared secret -- the > - * input to the KDF is (DH shared secret || otherinfo) > - */ > - if (kdfcopy && > - copy_from_user(kbuf + resultlen, kdfcopy->otherinfo, > - kdfcopy->otherinfolen) != 0) { > - ret = -EFAULT; > - goto error5; > - } > - > ret = do_dh(result, base, private, prime); > if (ret) > goto error5; > @@ -333,8 +322,17 @@ long __keyctl_dh_compute(struct keyctl_dh_params __user > *params, goto error5; > > if (kdfcopy) { > + /* > + * Concatenate SP800-56A otherinfo past DH shared secret -- the > + * input to the KDF is (DH shared secret || otherinfo) > + */ > + if (copy_from_user(kbuf + nbytes, kdfcopy->otherinfo, > + kdfcopy->otherinfolen) != 0) { > + ret = -EFAULT; > + goto error5; > + } > ret = keyctl_dh_compute_kdf(sdesc, buffer, buflen, kbuf, > - resultlen + kdfcopy->otherinfolen); > + nbytes + kdfcopy->otherinfolen); > } else { > ret = nbytes; > if (copy_to_user(buffer, kbuf, nbytes) != 0) Ciao Stephan
Hi Stephan, On Thu, Apr 20, 2017 at 03:27:17PM +0200, Stephan Müller wrote: > Am Donnerstag, 20. April 2017, 07:46:31 CEST schrieb Eric Biggers: > > Hi Eric, > > > From: Eric Biggers <ebiggers@google.com> > > > > The result of the Diffie-Hellman computation may be shorter than the > > input prime number. Only calculate the KDF over the actual result; > > don't include additional uninitialized memory. > > Thank you for catching that (and all the rest). But I think this patch is not > correct. If the DH operation results in a shorter value, the trailing part > must be set to null and the KDF calculated over the entire prime length. > > Thus, if the DH result is shorter than the prime, the memory should look like > DH result || 0x00 <as often as needed to make it prime length> || otherinfo. > > Thus, instead of this patch, I would think that the kmalloc call should be > changed to a kzalloc. > > Is this in the standard? And is it the user-specified length of the prime number, or the length after stripping leading zeroes? Also, note that the numbers are being represented in big endian format; is that required, or just coincidental? With big endian numbers leading zeroes go at the beginning, not the end, otherwise their value will be changed... By the way: do we really need this in the kernel at all, given that it's just doing some math on data which userspace has access to? - Eric
Am Donnerstag, 20. April 2017, 19:46:02 CEST schrieb Eric Biggers: Hi Eric, > Hi Stephan, > > On Thu, Apr 20, 2017 at 03:27:17PM +0200, Stephan Müller wrote: > > Am Donnerstag, 20. April 2017, 07:46:31 CEST schrieb Eric Biggers: > > > > Hi Eric, > > > > > From: Eric Biggers <ebiggers@google.com> > > > > > > The result of the Diffie-Hellman computation may be shorter than the > > > input prime number. Only calculate the KDF over the actual result; > > > don't include additional uninitialized memory. > > > > Thank you for catching that (and all the rest). But I think this patch is > > not correct. If the DH operation results in a shorter value, the trailing > > part must be set to null and the KDF calculated over the entire prime > > length. > > > > Thus, if the DH result is shorter than the prime, the memory should look > > like DH result || 0x00 <as often as needed to make it prime length> || > > otherinfo. > > > > Thus, instead of this patch, I would think that the kmalloc call should be > > changed to a kzalloc. > > Is this in the standard? That is the gotcha that is not explicitly written in the standard. However, based on my experience with other implementations and tests like ECDSA and RSA CAVS testing, the standards seem to always interpreted to use the full allowed length. If the mathematical result is shorter than the defined length, it should be zero-padded. > And is it the user-specified length of the prime > number, or the length after stripping leading zeroes? It should be the length of the prime used for the DH operation. As the prime is given with the DH parameters, the DH parameter set defines the length of the prime. I cannot say about the stripping of leading zeros of user-provided primes because I have no idea where that is defined. I would need to do some further research on this matter and check with the authors of the standard. > Also, note that the > numbers are being represented in big endian format; is that required, or > just coincidental? With big endian numbers leading zeroes go at the > beginning, not the end, otherwise their value will be changed... The numbers are MPI and are therefore big endian formats. Also the counter in the KDF is a big endian format which is mandated in the spec. You are right that the zeros go to the beginning of the number and I made a wrong statement in my initial remark regarding the zero value. > > - Ericinning and my initial remark regarding where the zeros are is wrong. Yet, IMHO we should not stip the zeros before applying the KDF as this would imply that the KDF result is different. > > By the way: do we really need this in the kernel at all, given that it's > just doing some math on data which userspace has access to? It is the question about how we want the keys subsystem to operate. The DH shared secret shall not be used as a key. But the DH operation is part of the key subsystem. If there is never a case where the result of the DH operation is used in the kernel, then the KDF can be removed and my patches could be reverted. However, in this case, the entire DH business could be questioned as this can easily be done in user space as well. Ciao Stephan
Hi Stephan, On Thu, Apr 20, 2017 at 08:38:30PM +0200, Stephan Müller wrote: > > > > By the way: do we really need this in the kernel at all, given that it's > > just doing some math on data which userspace has access to? > > It is the question about how we want the keys subsystem to operate. The DH > shared secret shall not be used as a key. But the DH operation is part of the > key subsystem. If there is never a case where the result of the DH operation > is used in the kernel, then the KDF can be removed and my patches could be > reverted. However, in this case, the entire DH business could be questioned as > this can easily be done in user space as well. > Well, who exactly is asking for Diffie-Hellman in the kernel at all? If it can be done in userspace then it should be done there. Having it in the kernel means having yet another API that's callable by unprivileged users and needs to be audited for security vulnerabilities. Just because the kernel can support doing hashes or has an arbitrary-precision arithmetic library or whatever doesn't mean it's the right place to do random crypto stuff. - Eric
Eric Biggers <ebiggers3@gmail.com> wrote: > > > By the way: do we really need this in the kernel at all, given that it's > > > just doing some math on data which userspace has access to? > > > > It is the question about how we want the keys subsystem to operate. The DH > > shared secret shall not be used as a key. But the DH operation is part of > > the key subsystem. If there is never a case where the result of the DH > > operation is used in the kernel, then the KDF can be removed and my > > patches could be reverted. However, in this case, the entire DH business > > could be questioned as this can easily be done in user space as well. > > > > Well, who exactly is asking for Diffie-Hellman in the kernel at all? If it > can be done in userspace then it should be done there. Having it in the > kernel means having yet another API that's callable by unprivileged users > and needs to be audited for security vulnerabilities. Just because the > kernel can support doing hashes or has an arbitrary-precision arithmetic > library or whatever doesn't mean it's the right place to do random crypto > stuff. I understood that there is the possibility of offloading this to hardware. David
On Thu, Apr 27, 2017 at 04:15:19PM +0100, David Howells wrote: > Eric Biggers <ebiggers3@gmail.com> wrote: > > > > > By the way: do we really need this in the kernel at all, given that it's > > > > just doing some math on data which userspace has access to? > > > > > > It is the question about how we want the keys subsystem to operate. The DH > > > shared secret shall not be used as a key. But the DH operation is part of > > > the key subsystem. If there is never a case where the result of the DH > > > operation is used in the kernel, then the KDF can be removed and my > > > patches could be reverted. However, in this case, the entire DH business > > > could be questioned as this can easily be done in user space as well. > > > > > > > Well, who exactly is asking for Diffie-Hellman in the kernel at all? If it > > can be done in userspace then it should be done there. Having it in the > > kernel means having yet another API that's callable by unprivileged users > > and needs to be audited for security vulnerabilities. Just because the > > kernel can support doing hashes or has an arbitrary-precision arithmetic > > library or whatever doesn't mean it's the right place to do random crypto > > stuff. > > I understood that there is the possibility of offloading this to hardware. > Okay, but where is this hardware, where are drivers for it, and how do we know this API is actually going to be compatible with it? Will it be just for performance, or for other reasons too? None of this seems to have been explained. Eric
diff --git a/security/keys/dh.c b/security/keys/dh.c index 1c1cac677041..a3a8607107f5 100644 --- a/security/keys/dh.c +++ b/security/keys/dh.c @@ -313,17 +313,6 @@ long __keyctl_dh_compute(struct keyctl_dh_params __user *params, goto error4; } - /* - * Concatenate SP800-56A otherinfo past DH shared secret -- the - * input to the KDF is (DH shared secret || otherinfo) - */ - if (kdfcopy && - copy_from_user(kbuf + resultlen, kdfcopy->otherinfo, - kdfcopy->otherinfolen) != 0) { - ret = -EFAULT; - goto error5; - } - ret = do_dh(result, base, private, prime); if (ret) goto error5; @@ -333,8 +322,17 @@ long __keyctl_dh_compute(struct keyctl_dh_params __user *params, goto error5; if (kdfcopy) { + /* + * Concatenate SP800-56A otherinfo past DH shared secret -- the + * input to the KDF is (DH shared secret || otherinfo) + */ + if (copy_from_user(kbuf + nbytes, kdfcopy->otherinfo, + kdfcopy->otherinfolen) != 0) { + ret = -EFAULT; + goto error5; + } ret = keyctl_dh_compute_kdf(sdesc, buffer, buflen, kbuf, - resultlen + kdfcopy->otherinfolen); + nbytes + kdfcopy->otherinfolen); } else { ret = nbytes; if (copy_to_user(buffer, kbuf, nbytes) != 0)