554354 - (CVE-2010-3173) Evil TLS Server can crash all Firefox from (at least) 3.0 ~ 3.6 (windows + linux)

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[rwilliams]

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.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 2

[rwilliams]

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

[rwilliams]

Please also advise if you just want the server to always produce the 0 length 3rd param.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Attachment: 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.

Comment 8

[Robert Relyea]

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

r+

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).

bob

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 10

[Wan-Teh Chang]

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
    !pubk->u.rsa.modulus.len
instead of
    !pubk->u.rsa.modulus.data
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.

Comment 11

[Daniel Veditz [:dveditz]]

NSS 3.12.7 was landed on the 1.9.2 branch for 1.9.2.9, and this was fixed on the 1.9.1 branch by landing NSS 3.12.8 for 1.9.1.14

From the patch and description this sounds like a null-deref. If that's wrong could it be worse than a DoS?

Comment 12

[rwilliams]

They aren't stupid - they're EVIL.

Comment 13

[Daniel Veditz [:dveditz]]

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.

Comment 14

[Wan-Teh Chang]

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.