Bug 554354 (CVE-2010-3173)

Evil TLS Server can crash all Firefox from (at least) 3.0 ~ 3.6 (windows + linux)



8 years ago
7 years ago


(Reporter: rwilliams, Assigned: Nelson Bolyard (seldom reads bugmail))



Dependency tree / graph

Firefox Tracking Flags

(status1.9.2 .9-fixed, status1.9.1 .14-fixed)


(Whiteboard: [sg:dos] sg:moderate or sg:low for weak key issue? (connecting to stupid servers))


(1 attachment)



8 years ago
User-Agent:       Mozilla/4.0 (compatible; MSIE 7.0; Windows NT 5.1; .NET CLR 1.1.4322; .NET CLR 2.0.50727; .NET CLR 3.0.4506.2152; .NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9.2) Gecko/20100115 Firefox/3.6 (.NET CLR 3.5.30729)

When using DHE cipher suites, a correctly crafted server key exchange message will crash Firefox.

       struct {
           opaque dh_p<1..2^16-1>;
           opaque dh_g<1..2^16-1>;
           opaque dh_Ys<1..2^16-1>;
       } ServerDHParams;     /* Ephemeral DH parameters */

dh_p can be zero length, dh_g can be zero length - but when dh_Ys is zero length; Firefox crashes.

example encoding:
00 00 00 00 00 00

Signature on key is validated first - so easiest done by Evil TLS Server(TM).

Reproducible: Always

Steps to Reproduce:
Modify TLS server to send a 0 length dh_Ys.
Actual Results:  
On linux - firefox vanishes.
On windows - crash report application comes up.

Expected Results:  
Rejection of invalid dh params.

Some similar bad parameters are caught with errors about memory allocation failure.


8 years ago
Assignee: nobody → nelson
Priority: -- → P1
Version: unspecified → 3.11
Keywords: crash
Rob, thanks for the report. 
Do you have a test server with which I can conduct tests?  (if so, URL?)
Do you have any Firefox crash report numbers from which I might be able 
to get stack information? 

This could be trivially fixed in the SSL library by simply having the 
SSL library enforce the minimum field lengths.  But I'll bet the crash
was much deeper, suggesting that there are numerous levels of code that 
don't check their inputs, and all should be fixed.  I'll try to fix them
all once I've identified the full depth of the problem.
Ever confirmed: true

Comment 2

8 years ago
You're welcome.

I've created a temporary server at https://tls.secg.org:40008

This server is only offering DHE cipher suites.

The server key exchange has a random defect.

For each of the 3 DH components they may be:
1. correct
2. missing
3. 0 length
4. big (and mathematically wrong)
5. bigger than possible for the record size

There is a 1/125 (5*5*5) chance of having a correct key exchange.

Refreshing will produce several different errors including:
  (Error code: ssl_error_rx_malformed_server_key_exch)
  (Error code: sec_error_no_memory)
  (Error code: sec_error_keygen_fail)

Eventually, Firefox will crash.

Please advise if the server is unavailable, or when you have finished testing.

Comment 3

8 years ago
Please also advise if you just want the server to always produce the 0 length 3rd param.
Thanks.  I'm no longer employed to work on NSS, and now do so only in my 
spare time, which in this case will not be until this weekend.  Sorry. 
I think the random error causes will be fine.  Thanks.
NSS's libSSL detected most of the invalid SKE messages, but not all.

The first crash had this stack:

nss3.dll!SECKEY_PublicKeyStrength()        Line 1479 + 0x6 bytes
nss3.dll!SECKEY_PublicKeyStrengthInBits()  Line 1505 + 0x9 bytes
ssl3.dll!ssl3_SendClientKeyExchange()      Line 4763 + 0x9 bytes
ssl3.dll!ssl3_HandleServerHelloDone()      Line 5705 + 0x9 bytes
ssl3.dll!ssl3_HandleHandshakeMessage()     Line 8601 + 0x9 bytes
ssl3.dll!ssl3_HandleHandshake()            Line 8696 + 0x19 bytes
ssl3.dll!ssl3_HandleRecord()               Line 9033 + 0xd bytes
ssl3.dll!ssl3_GatherCompleteHandshake()    Line 206  + 0x17 bytes
ssl3.dll!ssl_GatherRecord1stHandshake()    Line 1258 + 0xb bytes
ssl3.dll!ssl_Do1stHandshake()              Line 151  + 0xf bytes
ssl3.dll!ssl_SecureSend()                  Line 1204 + 0x9 bytes
ssl3.dll!ssl_Send()                        Line 1601 + 0x1b bytes
nspr4.dll!PR_Send()                        Line 226  + 0x1e bytes
tstclnt.exe!main()                         Line 1020 + 0x21 bytes
tstclnt.exe!__tmainCRTStartup()            Line 597  + 0x17 bytes

SECKEY_PublicKeyStrength should validate its inputs.  And of course, 
ssl3_HandleServerKeyExchange should detect zero length inputs.
SECKEY_CreateDHPrivateKey should also validate (or at least sanity check)
its input params.  Failure to do so doesn't cause a crash, but may cause 
some very peculiar errors, such as SEC_ERROR_NO_MEMORY.


8 years ago
Target Milestone: --- → 3.12.7
Created attachment 435449 [details] [diff] [review]
patch v1, look for invalid DH params in libSSL and in SECKEY funcs

In this patch, I do some sanity checks on the DH params.  
Among other things, I require prime p to be at least 505 bits long, and
I require the base and the server's public value both to be non-zero and to 
be no longer than the prime p, in both libSSL and SECKEY_CreateDHPrivateKey.  Some of these things are not strictly required, but I think they're appropriate.  I require non-empty input params in SECKEY_PublicKeyStrength.

Bob, please review.
Attachment #435449 - Flags: review?(rrelyea)

Comment 8

8 years ago
Comment on attachment 435449 [details] [diff] [review]
patch v1, look for invalid DH params in libSSL and in SECKEY funcs


The SECKEY_PublicKeyStrenth() is probably over kill, but doesn't hurt.

NSS does depend on the caller to pass 'good' stuff in general. Where we really should check stuff is when we generate it internally (like off the wire for SSL, or decode it out of ASN.1).

Attachment #435449 - Flags: review?(rrelyea) → review+
Bug 554354: SSL client doesn't validate ECDH params from server, r=rrelyea

Checking in ssl/ssl3con.c;     new revision: 1.138; previous revision: 1.137
Checking in cryptohi/seckey.c; new revision: 1.52;  previous revision: 1.51
Last Resolved: 8 years ago
Resolution: --- → FIXED
status1.9.1: --- → wanted
status1.9.2: --- → wanted

Comment 10

7 years ago
Comment on attachment 435449 [details] [diff] [review]
patch v1, look for invalid DH params in libSSL and in SECKEY funcs

r=wtc.  Your comment 7 should be added to the source code as comments
because two points are not obvious at all:

1.The 505-bit (or 512 in the source code) minimum prime p length. The
source of this minimum should be cited.

2. The "+ 1" in expressions like
    dh_g.len > dh_p.len + 1

In SECKEY_PublicKeyStrength, it seems better to check
instead of
because you may have a non-NULL pubk->u.rsa.modulus.data
and a 0 pubk->u.rsa.modulus.len, and the subsequent
pubk->u.rsa.modulus.data[0] would be reading beyond the
end of the buffer.
Attachment #435449 - Flags: review+


7 years ago
Blocks: 587234
NSS 3.12.7 was landed on the 1.9.2 branch for, and this was fixed on the 1.9.1 branch by landing NSS 3.12.8 for

From the patch and description this sounds like a null-deref. If that's wrong could it be worse than a DoS?
Blocks: 575620
status1.9.1: wanted → .14-fixed
status1.9.2: wanted → .9-fixed
Whiteboard: [sg:dos]
Whiteboard: [sg:dos] → [sg:dos] sg:moderate or sg:low for weak key issue? (connecting to stupid servers)

Comment 12

7 years ago
They aren't stupid - they're EVIL.
If the servers were evil they'd transmit your data in plain text. They're stupid because they think 256-bit DHE is the same as 256-bit AES.

Assigning CVE-2010-3173 to the fix for weak DHE keys.
Alias: CVE-2010-3173

Comment 14

7 years ago
dveditz: you misunderstood what this bug report is about.
This bug report is about evil servers.  Broken servers
that use 256-bit DHE keys cannot crash NSS.
Group: core-security
You need to log in before you can comment on or make changes to this bug.