Last Comment Bug 894370 - (CVE-2013-1739) Assertion failure: inputLen >= spec->mac_size, at c:/work/mozilla/builds/beta/mozilla/security/nss/lib/ssl/ssl3con.c:2057
(CVE-2013-1739)
: Assertion failure: inputLen >= spec->mac_size, at c:/work/mozilla/builds/beta...
Status: RESOLVED FIXED
[adv-main25+][adv-esr24-1+][adv-esr17...
: assertion, regression, sec-moderate
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.14.3
: All All
: P1 normal (vote)
: 3.15.2
Assigned To: Adam Langley
:
:
Mentors:
Depends on:
Blocks: 532972 CVE-2013-1620 921090
  Show dependency treegraph
 
Reported: 2013-07-16 06:21 PDT by Carsten Book [:Tomcat]
Modified: 2015-02-25 20:16 PST (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
wontfix
+
fixed
fixed
fixed
25+
fixed
25+
fixed
25+
fixed
fixed
fixed


Attachments
stack (5.65 KB, text/plain)
2013-07-16 06:21 PDT, Carsten Book [:Tomcat]
no flags Details
patch (663 bytes, patch)
2013-07-19 12:11 PDT, Adam Langley
no flags Details | Diff | Splinter Review
patch (3.08 KB, patch)
2013-08-02 08:41 PDT, Adam Langley
wtc: review+
wtc: checked‑in+
Details | Diff | Splinter Review

Description Carsten Book [:Tomcat] 2013-07-16 06:21:28 PDT
Created attachment 776367 [details]
stack

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
Comment 1 Carsten Book [:Tomcat] 2013-07-16 06:42:32 PDT
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)|202.53.141.71|:443... connected.
GnuTLS: Key usage violation in certificate has been detected.
Unable to establish SSL connection.
Comment 2 Kai Engert (:kaie) (on vacation) 2013-07-16 07:37:48 PDT
Seems to refer to:

ssl3_ComputeRecordMACConstantTime(
...
    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 

cat req.txt:
GET / HTTP/1.1
Host: sc.lcsd.gov.hk
Comment 3 Kai Engert (:kaie) (on vacation) 2013-07-16 07:39:59 PDT
(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)|202.53.141.71|:443...
> connected.
> 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?
Comment 4 Carsten Book [:Tomcat] 2013-07-18 23:36:22 PDT
(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
Comment 5 Adam Langley 2013-07-19 08:05:29 PDT
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.
Comment 6 Adam Langley 2013-07-19 12:11:18 PDT
Created attachment 778609 [details] [diff] [review]
patch

Patch. Not yet tested at all so no r? flags.
Comment 7 Andrew McCreight [:mccr8] 2013-07-24 10:47:40 PDT
I'm marking this sec-moderate because from comment 5 this doesn't sound bad.  Feel free to adjust up or down as desired.
Comment 8 Adam Langley 2013-08-02 08:41:59 PDT
Created attachment 785022 [details] [diff] [review]
patch

This is the patch that I ended up committing to Chromium for this issue.
Comment 9 Wan-Teh Chang 2013-08-02 16:48:23 PDT
Comment on attachment 785022 [details] [diff] [review]
patch

r=wtc. Patch checked in on the NSS trunk:
https://hg.mozilla.org/projects/nss/rev/56436aa3463f
Comment 10 Wan-Teh Chang 2013-08-02 18:12:35 PDT
Marked the bug fixed.

The code in question was added in NSS 3.14.3 to fix bug 822365.
Comment 11 Daniel Veditz [:dveditz] 2013-09-25 13:33:44 PDT
This appears fixed in Firefox nightly and aurora (Firefox 26, 27), but not beta or release (Firefox 24, 25).
Comment 12 Daniel Veditz [:dveditz] 2013-09-25 13:56:08 PDT
Assigning CVE-2013-1739 to this issue.
Comment 13 Daniel Veditz [:dveditz] 2013-09-26 10:09:28 PDT
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
Comment 14 Kai Engert (:kaie) (on vacation) 2013-09-27 09:01:20 PDT
Adding Elio for picking up the patch.
Comment 15 Alex Keybl [:akeybl] 2013-10-14 08:10:35 PDT
NSS for mozilla-beta was updated to 3.15.2 in https://hg.mozilla.org/releases/mozilla-beta/rev/ee766b7c606e (bug 921090).
Comment 16 Ryan VanderMeulen [:RyanVM] 2013-10-14 10:56:57 PDT
(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?
Comment 17 Al Billings [:abillings] 2013-10-15 16:18:20 PDT
Who is doing the ESR24 patch for this since it is tracking+ for the next ESR24 release in the next two weeks?
Comment 18 Kai Engert (:kaie) (on vacation) 2013-10-18 05:42:54 PDT
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:
http://hg.mozilla.org/releases/mozilla-esr24/file/default/security/nss/TAG-INFO
Comment 19 Al Billings [:abillings] 2013-10-18 10:53:06 PDT
Changing flags to mark ESR24 as fixed.
Comment 20 Daniel Veditz [:dveditz] 2013-10-25 16:22:19 PDT
NSS 3.14.4 was created for ESR 17 so this is fixed there now, too.

Note You need to log in before you can comment on or make changes to this bug.