Created attachment 776367 [details]
Happens in beta and nightly debug builds on all platforms according to bughunter
Assertion failure: inputLen >= spec->mac_size, at c:/work/mozilla/builds/beta/mozilla/security/nss/lib/ssl/ssl3con.c:2057
Will try to generate a testcase
Steps to repoduce is to load https://sc.lcsd.gov.hk/uniS/webcat.hkpl.gov.hk/lib/item;jsessionid=1414DB917107CA0666E4352692442460?id=chamo:3207909&theme=WEB&copies-sort=3%2Basc in a debug build and nearly crash on load
note, there seems to be a problem with this site. While trying to use wget to create the testcase i got:
Connecting to sc.lcsd.gov.hk (sc.lcsd.gov.hk)|126.96.36.199|:443... connected.
GnuTLS: Key usage violation in certificate has been detected.
Unable to establish SSL connection.
Seems to refer to:
PORT_Assert(inputLen >= spec->mac_size);
I cannnot reproduce the assertion using a debug build of tstclnt and settings that are probably close to what firefox beta with NSS 3.15 uses:
tstclnt -d . -p 443 -h sc.lcsd.gov.hk -o -T -v -V ssl3:tls1.1 -f -O < req.txt
GET / HTTP/1.1
(In reply to Carsten Book [:Tomcat] from comment #1)
> note, there seems to be a problem with this site. While trying to use wget
> to create the testcase i got:
> Connecting to sc.lcsd.gov.hk (sc.lcsd.gov.hk)|188.8.131.52|:443...
> GnuTLS: Key usage violation in certificate has been detected.
> Unable to establish SSL connection.
Works for me, wget reports a 404, on Fedora 19 and also another distribution.
Your wget uses GnuTLS. Which Linux distribution do you use?
(In reply to Kai Engert (:kaie) from comment #3)
> Works for me, wget reports a 404, on Fedora 19 and also another distribution.
> Your wget uses GnuTLS. Which Linux distribution do you use?
This was with the Version from cygwin
I cannot reproduce the crash from the given URL, but I can see the bug from the code because it's my fault.
The code checks the size of the input, but it assumes that decryption doesn't alter the size of the data. This is incorrect for case of a block cipher in NSS - the decryption may result in zero bytes of output if the input isn't a multiple of the block size.
The issue is around this line:
/* decrypt from cText buf to plaintext. */
rv = crSpec->decode(
»·······crSpec->decodeContext, plaintext->buf, (int *)&plaintext->len,
»·······plaintext->space, cText->buf->buf + ivLen, cText->buf->len - ivLen);
plaintext->len can end up zero.
However, I cannot see a way to exploit this (although others are better at that sort of thing than I). Both ssl_RemoveSSLv3CBCPadding and ssl_RemoveTLSCBCPadding will reject the length immediately so the good flag will be cleared.
The danger lies in ssl3_ComputeRecordMACConstantTime and ssl_CBCExtractMAC. Dealing with the latter first, because it's shorter, the main loop will exit immediately because originalLength is zero and |i < originalLength| will be false.
In ssl_ComputeRecordMACConstantTime, |inputLen| and |originalLen| will be zero (triggering the assertion in debug mode). |recordLength| will underflow, but it's only used to update a couple of bytes |header| and it doesn't matter that they'll be nonsense.
There there are two paths: ssl_ComputeRecordMAC in the fallback case (when NSS and libssl have version skew) and the constant time MAC. For the former, it gets a negative length and passes that to PK11_DigestOp and the like. This will either do nothing, or will try to hash all of memory and crash.
For the constant time code we have to go to lib/freebl/hmacct.c. Here, maxMACBytes will underflow but the calculation in numBlocks will push it positive again. It'll hash a little uninitialised data, but it doesn't go crazy.
Back in ssl3_HandleRecord, plaintext->len will be updated and will underflow, which is very dangerous, but the good flag must be false and so the function will send a fatal alert and return.
To test I updated a server to trigger this problem and commented the assertions to see what happened with a debug build of Chrome and it resulted in a connection error without crashing.
To address that I think the decryption failure should be handled synchronously. The code is security-sensitive-constant-time but the length of the ciphertext is public information and so it's ok to have different behaviour based on that. Additionally, some decryptions may not set the output length at all if they return an error.
I'll upload a patch to that effect in a sec.
Created attachment 778609 [details] [diff] [review]
Patch. Not yet tested at all so no r? flags.
I'm marking this sec-moderate because from comment 5 this doesn't sound bad. Feel free to adjust up or down as desired.
Created attachment 785022 [details] [diff] [review]
This is the patch that I ended up committing to Chromium for this issue.
Comment on attachment 785022 [details] [diff] [review]
r=wtc. Patch checked in on the NSS trunk:
Marked the bug fixed.
The code in question was added in NSS 3.14.3 to fix bug 822365.
This appears fixed in Firefox nightly and aurora (Firefox 26, 27), but not beta or release (Firefox 24, 25).
Assigning CVE-2013-1739 to this issue.
We should fix this in Firefox 25 and the corresponding Firefox ESR-24 by upgrading from NSS 3.15.1 to 3.15.2
It is not severe enough to be worth upgrading the last ESR 17 release from NSS 3.14.3
Adding Elio for picking up the patch.
NSS for mozilla-beta was updated to 3.15.2 in https://hg.mozilla.org/releases/mozilla-beta/rev/ee766b7c606e (bug 921090).
(In reply to Daniel Veditz [:dveditz] from comment #13)
> It is not severe enough to be worth upgrading the last ESR 17 release from
> NSS 3.14.3
Same for b2g18?
Who is doing the ESR24 patch for this since it is tracking+ for the next ESR24 release in the next two weeks?
Al, this bug has been fixed in NSS 3.15.2
It seems the ESR24 branch has already been updated to use NSS 3.15.2 in the meantime:
Changing flags to mark ESR24 as fixed.
NSS 3.14.4 was created for ESR 17 so this is fixed there now, too.