Re: libcrypto "Client DoS due to large DH parameter"

2018-06-12 Thread Stuart Henderson
On 2018/06/12 16:47, Theo Buehler wrote:
> On Tue, Jun 12, 2018 at 03:24:39PM +0100, Stuart Henderson wrote:
> > from https://github.com/openssl/openssl/commit/3984ef0b7 (advisory forwarded
> > below).  is it ok to just use this directly?
> > 
> > Index: dh_key.c
> > ===
> > RCS file: /cvs/src/lib/libcrypto/dh/dh_key.c,v
> > retrieving revision 1.27
> > diff -u -p -r1.27 dh_key.c
> > --- dh_key.c29 Jan 2017 17:49:22 -  1.27
> > +++ dh_key.c12 Jun 2018 14:08:13 -
> > @@ -104,10 +104,15 @@ generate_key(DH *dh)
> > int ok = 0;
> > int generate_new_key = 0;
> > unsigned l;
> > -   BN_CTX *ctx;
> > +   BN_CTX *ctx = NULL;
> 
> I don't object to doing this initialization, but I don't really see the
> point of it -- unless you plan to do 'goto err' instead of 'return 0'
> (but this would add a pointless generic ERR_R_BN_LIB error).

oh, that must be a holdover from Guido's first diff which did use goto err.

> > BN_MONT_CTX *mont = NULL;
> > BIGNUM *pub_key = NULL, *priv_key = NULL;
> >  
> > +   if (BN_num_bits(dh->p) > OPENSSL_DH_MAX_MODULUS_BITS) {
> > +   DHerr(DH_F_GENERATE_KEY, DH_R_MODULUS_TOO_LARGE);
> 
> We don't use these two parameter error macros anymore. Please use
> 
>   DHerror(DH_R_MODULUS_TOO_LARGE);

oops :) started the build but forgot to check it before sending the mail ...

updated for these two below.

> With that it's
> 
> ok tb
> 
> (provided that those more knowledgeable about the fine points of
> licencing agree that it's fine pulling it in.)

I'm not sure what ended up happening with the switch to Apache license,
but the license in their repo hasn't changed.

Index: dh/dh_key.c
===
RCS file: /cvs/src/lib/libcrypto/dh/dh_key.c,v
retrieving revision 1.27
diff -u -p -r1.27 dh_key.c
--- dh/dh_key.c 29 Jan 2017 17:49:22 -  1.27
+++ dh/dh_key.c 12 Jun 2018 15:15:39 -
@@ -108,6 +108,11 @@ generate_key(DH *dh)
BN_MONT_CTX *mont = NULL;
BIGNUM *pub_key = NULL, *priv_key = NULL;
 
+   if (BN_num_bits(dh->p) > OPENSSL_DH_MAX_MODULUS_BITS) {
+   DHerror(DH_R_MODULUS_TOO_LARGE);
+   return 0;
+   }
+
ctx = BN_CTX_new();
if (ctx == NULL)
goto err;



Re: libcrypto "Client DoS due to large DH parameter"

2018-06-12 Thread Theo Buehler
On Tue, Jun 12, 2018 at 03:24:39PM +0100, Stuart Henderson wrote:
> from https://github.com/openssl/openssl/commit/3984ef0b7 (advisory forwarded
> below).  is it ok to just use this directly?
> 
> Index: dh_key.c
> ===
> RCS file: /cvs/src/lib/libcrypto/dh/dh_key.c,v
> retrieving revision 1.27
> diff -u -p -r1.27 dh_key.c
> --- dh_key.c  29 Jan 2017 17:49:22 -  1.27
> +++ dh_key.c  12 Jun 2018 14:08:13 -
> @@ -104,10 +104,15 @@ generate_key(DH *dh)
>   int ok = 0;
>   int generate_new_key = 0;
>   unsigned l;
> - BN_CTX *ctx;
> + BN_CTX *ctx = NULL;

I don't object to doing this initialization, but I don't really see the
point of it -- unless you plan to do 'goto err' instead of 'return 0'
(but this would add a pointless generic ERR_R_BN_LIB error).

>   BN_MONT_CTX *mont = NULL;
>   BIGNUM *pub_key = NULL, *priv_key = NULL;
>  
> + if (BN_num_bits(dh->p) > OPENSSL_DH_MAX_MODULUS_BITS) {
> + DHerr(DH_F_GENERATE_KEY, DH_R_MODULUS_TOO_LARGE);

We don't use these two parameter error macros anymore. Please use

DHerror(DH_R_MODULUS_TOO_LARGE);

With that it's

ok tb

(provided that those more knowledgeable about the fine points of
licencing agree that it's fine pulling it in.)

> + return 0;
> + }
> +
>   ctx = BN_CTX_new();
>   if (ctx == NULL)
>   goto err;