crypto: dh - fix calculating encoded key size

Commit Message

Eric Biggers July 11, 2018, 3:59 a.m. UTC

From: Eric Biggers <[e-mail]>

It was forgotten to increase DH_KPP_SECRET_MIN_SIZE to include 'q_size',
causing an out-of-bounds write of 4 bytes in crypto_dh_encode_key(), and
an out-of-bounds read of 4 bytes in crypto_dh_decode_key().  Fix it.
Also add a BUG_ON() if crypto_dh_encode_key() doesn't exactly fill the
buffer, as that would have found this bug without resorting to KASAN.

Reported-by: [e-mail]
Fixes: e3fe0ae12962 ("crypto: dh - add public key verification test")
Signed-off-by: Eric Biggers <[e-mail]>
---
 crypto/dh_helper.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Herbert Xu July 11, 2018, 7:26 a.m. UTC | #1

On Tue, Jul 10, 2018 at 08:59:05PM -0700, Eric Biggers wrote:
> From: Eric Biggers <[e-mail]>
> 
> It was forgotten to increase DH_KPP_SECRET_MIN_SIZE to include 'q_size',
> causing an out-of-bounds write of 4 bytes in crypto_dh_encode_key(), and
> an out-of-bounds read of 4 bytes in crypto_dh_decode_key().  Fix it.
> Also add a BUG_ON() if crypto_dh_encode_key() doesn't exactly fill the
> buffer, as that would have found this bug without resorting to KASAN.
> 
> Reported-by: [e-mail]
> Fixes: e3fe0ae12962 ("crypto: dh - add public key verification test")
> Signed-off-by: Eric Biggers <[e-mail]>

Is it possible to return an error and use WARN_ON instead of BUG_ON?
Or do the callers not bother to check for errors?

Thanks,

Stephan Mueller July 11, 2018, 3:45 p.m. UTC | #2

Am Mittwoch, 11. Juli 2018, 05:59:05 CEST schrieb Eric Biggers:

Hi Eric,

> From: Eric Biggers <[e-mail]>
> 
> It was forgotten to increase DH_KPP_SECRET_MIN_SIZE to include 'q_size',
> causing an out-of-bounds write of 4 bytes in crypto_dh_encode_key(), and
> an out-of-bounds read of 4 bytes in crypto_dh_decode_key().  Fix it.
> Also add a BUG_ON() if crypto_dh_encode_key() doesn't exactly fill the
> buffer, as that would have found this bug without resorting to KASAN.
> 
> Reported-by: [e-mail]
> Fixes: e3fe0ae12962 ("crypto: dh - add public key verification test")
> Signed-off-by: Eric Biggers <[e-mail]>

Thanks.

Reviewed-by: Stephan Müller <[e-mail]>

Ciao
Stephan

Eric Biggers July 11, 2018, 4:27 p.m. UTC | #3

On Wed, Jul 11, 2018 at 03:26:56PM +0800, Herbert Xu wrote:
> On Tue, Jul 10, 2018 at 08:59:05PM -0700, Eric Biggers wrote:
> > From: Eric Biggers <[e-mail]>
> > 
> > It was forgotten to increase DH_KPP_SECRET_MIN_SIZE to include 'q_size',
> > causing an out-of-bounds write of 4 bytes in crypto_dh_encode_key(), and
> > an out-of-bounds read of 4 bytes in crypto_dh_decode_key().  Fix it.
> > Also add a BUG_ON() if crypto_dh_encode_key() doesn't exactly fill the
> > buffer, as that would have found this bug without resorting to KASAN.
> > 
> > Reported-by: [e-mail]
> > Fixes: e3fe0ae12962 ("crypto: dh - add public key verification test")
> > Signed-off-by: Eric Biggers <[e-mail]>
> 
> Is it possible to return an error and use WARN_ON instead of BUG_ON?
> Or do the callers not bother to check for errors?
> 

The callers do check for errors, but at the point of the proposed BUG_ON() a
buffer overflow may have already occurred, so I think a BUG_ON() would be more
appropriate than a WARN_ON().  Of course, it would be better to prevent any
buffer overflow from occurring in the first place, but that's already the
purpose of the 'len != crypto_dh_key_len(params)' check; the issue was that
'crypto_dh_key_len()' calculated the wrong length.

- Eric

Herbert Xu July 19, 2018, 8:31 a.m. UTC | #4

On Wed, Jul 11, 2018 at 09:27:56AM -0700, Eric Biggers wrote:
>
> The callers do check for errors, but at the point of the proposed BUG_ON() a
> buffer overflow may have already occurred, so I think a BUG_ON() would be more
> appropriate than a WARN_ON().  Of course, it would be better to prevent any
> buffer overflow from occurring in the first place, but that's already the
> purpose of the 'len != crypto_dh_key_len(params)' check; the issue was that
> 'crypto_dh_key_len()' calculated the wrong length.

How about adding an end argument to dh_pack_data so that it can
check that the buffer isn't overflown each time it's called?

Then if you cared you could also have one final check at the end
of the function to ensure the buffer is fully used.

As we can easily avoid having a BUG_ON here I think we should since
a BUG_ON would leave the machine unresponsive which should be a
last resort.

Thanks,

Patch

diff --git a/crypto/dh_helper.c b/crypto/dh_helper.c
index a7de3d9ce5ace..87ad6e2e87644 100644
--- a/crypto/dh_helper.c
+++ b/crypto/dh_helper.c
@@ -14,7 +14,7 @@ 
 #include <crypto/dh.h>
 #include <crypto/kpp.h>
 
-#define DH_KPP_SECRET_MIN_SIZE (sizeof(struct kpp_secret) + 3 * sizeof(int))
+#define DH_KPP_SECRET_MIN_SIZE (sizeof(struct kpp_secret) + 4 * sizeof(int))
 
 static inline u8 *dh_pack_data(void *dst, const void *src, size_t size)
 {
@@ -61,7 +61,8 @@  int crypto_dh_encode_key(char *buf, unsigned int len, const struct dh *params)
 	ptr = dh_pack_data(ptr, params->key, params->key_size);
 	ptr = dh_pack_data(ptr, params->p, params->p_size);
 	ptr = dh_pack_data(ptr, params->q, params->q_size);
-	dh_pack_data(ptr, params->g, params->g_size);
+	ptr = dh_pack_data(ptr, params->g, params->g_size);
+	BUG_ON(ptr != (u8 *)buf + len);
 
 	return 0;
 }