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+
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 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)) >+ 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.
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
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-esr17: --- → fixed
You need to log in before you can comment on or make changes to this bug.