Re: [PATCH] crypto: dh - fix calculating encoded key size
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, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] crypto: dh - fix calculating encoded key size
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
> >
> > 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: syzbot+6d38d558c25b53b8f...@syzkaller.appspotmail.com
> > Fixes: e3fe0ae12962 ("crypto: dh - add public key verification test")
> > Signed-off-by: Eric Biggers
>
> 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
Re: [PATCH] crypto: dh - fix calculating encoded key size
Am Mittwoch, 11. Juli 2018, 05:59:05 CEST schrieb Eric Biggers:
Hi Eric,
> From: Eric Biggers
>
> 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: syzbot+6d38d558c25b53b8f...@syzkaller.appspotmail.com
> Fixes: e3fe0ae12962 ("crypto: dh - add public key verification test")
> Signed-off-by: Eric Biggers
Thanks.
Reviewed-by: Stephan Müller
Ciao
Stephan
Re: [PATCH] crypto: dh - fix calculating encoded key size
On Tue, Jul 10, 2018 at 08:59:05PM -0700, Eric Biggers wrote:
> From: Eric Biggers
>
> 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: syzbot+6d38d558c25b53b8f...@syzkaller.appspotmail.com
> Fixes: e3fe0ae12962 ("crypto: dh - add public key verification test")
> Signed-off-by: Eric Biggers
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,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
