KEYS: fixes for new keyctl_dh_compute() KDF extension
Subsystems (3)
cover KEYS: fixes for new keyctl_dh_compute() KDF extension
This patch series fixes several bugs in the KDF extension to keyctl_dh_compute() currently sitting in keys-next: a way userspace could cause an infinite loop, two ways userspace could cause the use of uninitialized memory, a misalignment, and missing __user annotations.
Eric Biggers (5):
KEYS: DH: forbid using digest_null as the KDF hash
KEYS: DH: don't feed uninitialized "otherinfo" into KDF
KEYS: DH: don't feed uninitialized result memory into KDF
KEYS: DH: ensure the KDF counter is properly aligned
KEYS: DH: add __user annotations to keyctl_kdf_params
include/uapi/linux/keyctl.h | 4 ++-- security/keys/dh.c | 50 ++++++++++++++++++++++----------------------- 2 files changed, 26 insertions(+), 28 deletions(-)
Signature · 1 line
2.12.2
PATCH 1/5 KEYS: DH: forbid using digest_null as the KDF hash
From: Eric Biggers <[email protected]>
Requesting "digest_null" in the keyctl_kdf_params caused an infinite loop in kdf_ctr() because the "null" hash has a digest size of 0. Fix it by rejecting hash algorithms with a digest size of 0.
--- security/keys/dh.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
security/keys/dh.c +11 -1
@@ -89,6 +89,7 @@ static int kdf_alloc(struct kdf_sdesc **sdesc_ret, char *hashname)struct crypto_shash *tfm;struct kdf_sdesc *sdesc;int size; +int err;/* allocate synchronous hash */ tfm =crypto_alloc_shash (hashname,0 ,0 );
@@ -97,16 +98,25 @@ static int kdf_alloc(struct kdf_sdesc **sdesc_ret, char *hashname)return PTR_ERR (tfm); } + err = -EINVAL ; +if (crypto_shash_digestsize (tfm) ==0 ) + goto out_free_tfm; + + err = -ENOMEM ; size =sizeof (struct shash_desc ) +crypto_shash_descsize (tfm); sdesc =kmalloc (size,GFP_KERNEL );if (!sdesc) -return -ENOMEM ; + goto out_free_tfm; sdesc->shash.tfm = tfm; sdesc->shash.flags =0x0 ; *sdesc_ret = sdesc;return 0 ; + +out_free_tfm: +crypto_free_shash (tfm); +return err; }static void kdf_dealloc (struct kdf_sdesc *sdesc)
Signature · 1 line
2.12.2
PATCH 2/5 KEYS: DH: don't feed uninitialized "otherinfo" into KDF
In reply to Eric BiggersFrom: Eric Biggers <[email protected]>
If userspace called KEYCTL_DH_COMPUTE with kdf_params containing NULL otherinfo but nonzero otherinfolen, the kernel would allocate a buffer for the otherinfo, then feed it into the KDF without initializing it. Fix this by always doing the copy from userspace (which will fail with EFAULT in this scenario).
--- security/keys/dh.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
security/keys/dh.c +1 -1
@@ -317,7 +317,7 @@ long __keyctl_dh_compute(struct keyctl_dh_params __user *params,* Concatenate SP800-56A otherinfo past DH shared secret -- the * input to the KDF is (DH shared secret || otherinfo) */ -if (kdfcopy && kdfcopy->otherinfo && +if (kdfcopy &©_from_user (kbuf + resultlen, kdfcopy->otherinfo, kdfcopy->otherinfolen) !=0 ) { ret = -EFAULT ;
Signature · 1 line
2.12.2
PATCH 3/5 KEYS: DH: don't feed uninitialized result memory into KDF
In reply to Eric BiggersFrom: Eric Biggers <[email protected]>
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.
--- security/keys/dh.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-)
security/keys/dh.c +10 -12
@@ -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 )
Signature · 1 line
2.12.2
Re: [PATCH 3/5] KEYS: DH: don't feed uninitialized result memory into KDF
Am Donnerstag, 20. April 2017, 07:46:31 CEST schrieb Eric Biggers:
Hi Eric,
Quoted 5 lines
From: Eric Biggers <[email protected]>
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.
Quoted 37 lines
Signed-off-by Eric Biggers <[email protected]>--- security/keys/dh.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-)security/keys/dh.c +5
-11@@ -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,
security/keys/dh.c long __keyctl_dh_compute(struct keyctl_dh_params __user · line 338
Quoted 10 lines
security/keys/dh.c +5
-1+ 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
Re: [PATCH 3/5] KEYS: DH: don't feed uninitialized result memory into KDF
Hi Stephan,
Quoted 14 lines · Stephan Müller
Am Donnerstag, 20. April 2017, 07:46:31 CEST schrieb Eric Biggers:
Hi Eric,
Quoted 5 lines
From: Eric Biggers <[email protected]>
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.
Quoted 6 lines
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.
Quoted 1 line
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
Re: [PATCH 3/5] KEYS: DH: don't feed uninitialized result memory into KDF
Am Donnerstag, 20. April 2017, 19:46:02 CEST schrieb Eric Biggers:
Hi Eric,
Quoted 18 lines
Hi Stephan,
Quoted 15 lines · Stephan Müller
Am Donnerstag, 20. April 2017, 07:46:31 CEST schrieb Eric Biggers:
Hi Eric,
Quoted 5 lines
From: Eric Biggers <[email protected]>
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.
Quoted 8 lines
Quoted 6 lines
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.
Quoted 2 lines
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.
Quoted 4 lines
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.
Quoted 2 lines
- 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.
Quoted 3 lines
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
Re: [PATCH 3/5] KEYS: DH: don't feed uninitialized result memory into KDF
Hi Stephan,
Quoted 11 lines · Stephan Müller
Quoted 3 lines
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
David Howells
maintainerRe: [PATCH 3/5] KEYS: DH: don't feed uninitialized result memory into KDF
Quoted 11 lines · Eric Biggers
Quoted 10 lines
Quoted 2 lines
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.
Quoted 7 lines
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
Re: [PATCH 3/5] KEYS: DH: don't feed uninitialized result memory into KDF
Quoted 21 lines · David Howells
Quoted 11 lines · Eric Biggers
Quoted 10 lines
Quoted 2 lines
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.
Quoted 7 lines
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.
Quoted 2 lines
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
PATCH 4/5 KEYS: DH: ensure the KDF counter is properly aligned
In reply to Eric BiggersFrom: Eric Biggers <[email protected]>
Accessing a 'u8[4]' through a '__be32 *' violates alignment rules. Just make the counter a __be32 instead.
--- security/keys/dh.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-)
security/keys/dh.c +3 -13
@@ -130,14 +130,6 @@ static void kdf_dealloc(struct kdf_sdesc *sdesc)kzfree (sdesc); } -/* convert 32 bit integer into its string representation */ -static inline void crypto_kw_cpu_to_be32 (u32 val,u8 *buf) -{ -__be32 *a = (__be32 *)buf; - - *a =cpu_to_be32 (val); -} -/* * Implementation of the KDF in counter mode according to SP800-108 section 5.1 * as well as SP800-56A section 5.8.1 (Single-step KDF).
@@ -154,16 +146,14 @@ static int kdf_ctr(struct kdf_sdesc *sdesc, const u8 *src, unsigned int slen,unsigned int h =crypto_shash_digestsize (desc->tfm);int err =0 ;u8 *dst_orig = dst; -u32 i =1 ; -u8 iteration[sizeof (u32)]; +__be32 counter =cpu_to_be32 (1 );while (dlen) { err =crypto_shash_init (desc);if (err) goto err; -crypto_kw_cpu_to_be32 (i, iteration); - err =crypto_shash_update (desc, iteration,sizeof (u32)); + err =crypto_shash_update (desc, (u8 *)&counter,sizeof (__be32));if (err) goto err;
@@ -189,7 +179,7 @@ static int kdf_ctr(struct kdf_sdesc *sdesc, const u8 *src, unsigned int slen, dlen -= h; dst += h; - i++; + counter =cpu_to_be32 (be32_to_cpu (counter) +1 ); } }
Signature · 1 line
2.12.2
PATCH 5/5 KEYS: DH: add __user annotations to keyctl_kdf_params
In reply to Eric BiggersFrom: Eric Biggers <[email protected]>
--- include/uapi/linux/keyctl.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
include/uapi/linux/keyctl.h +2 -2
@@ -70,8 +70,8 @@ struct keyctl_dh_params { };struct keyctl_kdf_params { -char *hashname; -char *otherinfo; +char __user *hashname; +char __user *otherinfo;__u32 otherinfolen;__u32 __spare[8 ]; };
Signature · 1 line
2.12.2
David Howells
maintainerStephan,
Quoted 4 lines · Eric Biggers
This patch series fixes several bugs in the KDF extension to
keyctl_dh_compute()currently sitting in keys-next: a way userspace could cause an infinite loop, two ways userspace could cause the use of uninitialized memory, a misalignment, and missing__userannotations.
Do you want to ack these or do they need fixing?
David
Stephan Müller
Acked-byAm Freitag, 28. April 2017, 17:53:00 CEST schrieb David Howells:
Hi David,
Quoted 9 lines
Stephan,
Quoted 4 lines · Eric Biggers
This patch series fixes several bugs in the KDF extension to
keyctl_dh_compute()currently sitting in keys-next: a way userspace could cause an infinite loop, two ways userspace could cause the use of uninitialized memory, a misalignment, and missing__userannotations.Do you want to ack these or do they need fixing?
All patches except patch 3/5:
For the patch 3/5 I would send an updated patch shortly which changes the kmalloc to kzalloc.
Ciao
Stephan
Am Freitag, 28. April 2017, 17:53:00 CEST schrieb David Howells:
Hi David,
Quoted 9 lines
Stephan,
Quoted 4 lines · Eric Biggers
This patch series fixes several bugs in the KDF extension to
keyctl_dh_compute()currently sitting in keys-next: a way userspace could cause an infinite loop, two ways userspace could cause the use of uninitialized memory, a misalignment, and missing__userannotations.Do you want to ack these or do they need fixing?
Please see the replacement for patch 3/5 below.
Thanks
Stephan
---8<---
Quoted 1 line
From
7229546f5ca0eed032208c03e7aef47f6b52ca18Mon Sep 17 00:00:00 2001
From: Stephan Mueller <[email protected]>
Date: Mon, 1 May 2017 16:45:22 +0200
Subject: [PATCH] keys: SP800-56A - preserve leading zeros for shared secret
The shared secret that is to be processed shall be the unchanged result of the DH mathematical primitive. The leading zeros shall be preserved.
In addition, the kernel memory that is used as input to the KDF is zeroized to ensure that no leaks exist.
--- include/linux/mpi.h | 2 +- lib/mpi/mpicoder.c | 10 ++++++---- security/keys/dh.c | 4 ++-- 3 files changed, 9 insertions(+), 7 deletions(-)
include/linux/mpi.h +1 -1
@@ -78,7 +78,7 @@ int mpi_fromstr(MPI val, const char *str);u32 mpi_get_keyid (MPI a,u32 *keyid);void *mpi_get_buffer (MPI a,unsigned *nbytes,int *sign);int mpi_read_buffer (MPI a,uint8_t *buf,unsigned buf_len,unsigned *nbytes, -int *sign); +int *sign,bool skip_lzeros);void *mpi_get_secure_buffer (MPI a,unsigned *nbytes,int *sign);int mpi_write_to_sgl (MPI a,struct scatterlist *sg,unsigned nbytes,int *sign);
lib/mpi/mpicoder.c +6 -4
@@ -137,11 +137,12 @@ static int count_lzeros(MPI a)* the data to-be-written on -EOVERFLOW in case buf_len was too * small. * @sign: if not NULL, it will be set to the sign of a. +* @skip_lzeros:Skip the leading zeros of the MPI before writing to buffer. * * Return: 0 on success or error code in case of error */ int mpi_read_buffer (MPI a,uint8_t *buf,unsigned buf_len,unsigned *nbytes, -int *sign) +int *sign,bool skip_lzeros) {uint8_t *p;#if BYTES_PER_MPI_LIMB ==4
@@ -152,7 +153,7 @@ int mpi_read_buffer(MPI a, uint8_t *buf, unsigned buf_len, unsigned *nbytes,#error please implement for this limb size.#endif unsigned int n =mpi_get_size (a); -int i, lzeros; +int i, lzeros =0 ;if (!buf || !nbytes)return -EINVAL ;
@@ -160,7 +161,8 @@ int mpi_read_buffer(MPI a, uint8_t *buf, unsigned buf_len, unsigned *nbytes,if (sign) *sign = a->sign; - lzeros =count_lzeros (a); +if (skip_lzeros) + lzeros =count_lzeros (a);if (buf_len < n - lzeros) { *nbytes = n - lzeros;
@@ -219,7 +221,7 @@ void *mpi_get_buffer(MPI a, unsigned *nbytes, int *sign)if (!buf)return NULL ; - ret =mpi_read_buffer (a, buf, n, nbytes, sign); + ret =mpi_read_buffer (a, buf, n, nbytes, sign, true);if (ret) {kfree (buf);
security/keys/dh.c +2 -2
@@ -296,7 +296,7 @@ long __keyctl_dh_compute(struct keyctl_dh_params __user *params, }/* allocate space for DH shared secret and SP800-56A otherinfo */ - kbuf =kmalloc (kdfcopy ? (resultlen + kdfcopy->otherinfolen) : resultlen, + kbuf =kzalloc (kdfcopy ? (resultlen + kdfcopy->otherinfolen) : resultlen,GFP_KERNEL );if (!kbuf) { ret = -ENOMEM ;
@@ -318,7 +318,7 @@ long __keyctl_dh_compute(struct keyctl_dh_params __user *params,if (ret) goto error5; - ret =mpi_read_buffer (result, kbuf, resultlen, &nbytes,NULL ); + ret =mpi_read_buffer (result, kbuf, resultlen, &nbytes,NULL , false);if (ret !=0 ) goto error5;
Signature · 1 line
2.9.3