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

RESOLVED FIXED in Firefox -esr17

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: Wan-Teh Chang, Assigned: Wan-Teh Chang)

Tracking

({sec-low})

trunk
3.14
sec-low

Firefox Tracking Flags

(firefox-esr10 wontfix, firefox-esr17 fixed)

Details

(Whiteboard: [sg:low])

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
Created attachment 619210 [details] [diff] [review]
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.
Attachment #619210 - Flags: review+
(Assignee)

Comment 1

6 years ago
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
(Assignee)

Comment 2

6 years ago
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.
Keywords: sec-low
Whiteboard: [sg:low]

Comment 3

6 years ago
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
(Assignee)

Comment 4

6 years ago
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].
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
status-firefox-esr10: --- → wontfix
Group: core-security
status-firefox-esr17: --- → fixed
You need to log in before you can comment on or make changes to this bug.