749841 - Reject Diffie-Hellman generators and public values equal to zero or one

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

Attachment: Fix ssl3_HandleServerKeyExchange, by Adam Langley

[I marked this bug as security-sensitive but the security severity
rating should be low.]

Nikos Mavrogiannopoulos originally reported this bug.

NSS doesn't reject a Diffie-Hellman public value of 1.  In addition,
NSS's rejection of zero is incomplete.

I checked the Diffie-Hellman related code in NSS.  I found three
places where NSS builds a Diffie-Hellman public key.  None of them
validates the public value or the generator.  The attached patch by
Adam Langley rejects Diffie-Hellman generators and public values
equal to zero or one in ssl3_HandleServerKeyExchange.  The other
two places are seckey_ExtractPublicKey and SECKEY_ImportDERPublicKey.

NSS should also check for bad public value or generator in
mozilla/security/nss/lib/freebl/dh.c.

1. DH_GenParam: this function already ensures the generator is in
the right range.  It only has two minor problems.
- The range [2..p-1] in the comment should be [2..p-1).
- When the random number it generates as the candidate generator
  falls outside that range, it sets the candidate to 3.  I think
  we can start with 2.

2. DH_NewKey: it is not checking if the generator is in the range
[2..p-1).  (I'm not sure if it needs to check that.)  It is not
checking if it generated a bad public value.

3. DH_Derive: it is not checking if the peer's public value is bad.

Comment 1

[Wan-Teh Chang]

Comment on attachment 619210 [details] [diff] [review]
Fix ssl3_HandleServerKeyExchange, by Adam Langley

Patch checked in on the NSS trunk (NSS 3.14).

Checking in ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.179; previous revision: 1.178
done

Comment 2

[Wan-Teh Chang]

Comment on attachment 619210 [details] [diff] [review]
Fix ssl3_HandleServerKeyExchange, by Adam Langley

>-	if (dh_g.len == 0 || dh_g.len > dh_p.len + 1 ||
>-	   (dh_g.len == 1 && dh_g.data[0] == 0))
>+	if (dh_g.len > dh_p.len || !ssl3_BigIntGreaterThanOne(&dh_g))
> 	    goto alert_loser;

Note the change from
    dh_g.len > dh_p.len + 1
to
    dh_g.len > dh_p.len

Both Adam and I think the original test is off by one.  Adam made
a similar change to the test dh_Ys.len > dh_p.len + 1.

Comment 3

[Robert Relyea]

I'm OK with this check, but we really should have the check in softoken as wtc suggests. Adding the check to ssl and the pk11wrap layer, however is probably overkill.

bob

Comment 4

[Wan-Teh Chang]

Bob: do you think it is sufficient to just add a check to DH_Derive?

The SSL/TLS ephemeral Diffie-Hellman cipher suites do not give us the
'q' parameter, so we can't do the y^q mod p == 1 check, but we can
check that y lies within the interval [2,p-1].