934016 - (CVE-2013-5605) Null_Cipher (used during handshake) does not respect maxOutputLen, copying an attacker-supplied # of bytes

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Ryan Sleevi]

The implementation of Null_Cipher uses PORT_Mempcy to copy data from the input buffer to the output buffer, as seen at http://mxr.mozilla.org/nss/source/lib/ssl/ssl3con.c#819

However, it does not validate or cap inputLen to be <= maxOutputLen. As a result, an attacker that sends a malformed SSL record may overwrite memory, including potentially memory on the stack.

Comment 1

[Ryan Sleevi]

This was discovered by my colleague, [e-mail], while performing fuzzing on NSS.

Comment 2

[Ryan Sleevi]

Attachment: Cap to maxOutputLen in Null_Cipher

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 826186 [details] [diff] [review]
Cap to maxOutputLen in Null_Cipher

Review of attachment 826186 [details] [diff] [review]:
-----------------------------------------------------------------

Ryan, you didn't add a reviewer to the review request. This can happen if the reviewer you chose isn't part of the security group. In that case, CC them to the bug first.

Anyway, I suggest that this check be made a PORT_Assert in Null_Cipher, and instead we do the real check in ssl3_HandleRecord.

I believe the problem is actually here in ssl3_HandleRecord:

    if (isTLS && cText->buf->len - ivLen > (MAX_FRAGMENT_LENGTH + 2048)) {
        ssl_ReleaseSpecReadLock(ss);
        SSL3_SendAlert(ss, alert_fatal, record_overflow);
        PORT_SetError(SSL_ERROR_RX_RECORD_TOO_LONG);
        return SECFailure;
    }

    ...

    /* 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);

    ...

It seems to me we need to be checking (cText->buf->len - ivLen > plaintext->space) instead, or in addition to, the above check.

If you think adding the check to Null_Cipher only is the best approach, then please add a comment about why only Null_Cipher needs to do the check.

Comment 4

[Wan-Teh Chang]

Comment on attachment 826186 [details] [diff] [review]
Cap to maxOutputLen in Null_Cipher

Review of attachment 826186 [details] [diff] [review]:
-----------------------------------------------------------------

I think this patch is reasonable behavior for Null_Cipher
when the output buffer is too small. I suggest an alternative
fix that causes a failure in that case.

::: lib/ssl/ssl3con.c
@@ +818,5 @@
>  static SECStatus
>  Null_Cipher(void *ctx, unsigned char *output, int *outputLen, int maxOutputLen,
>  	    const unsigned char *input, int inputLen)
>  {
> +    *outputLen = PR_MIN(maxOutputLen, inputLen);

This function should fail if inputLen > maxOutputLen.

I suggest adding the following at the beginning of the function:

    if (inputLen > maxOutputLen) {
        *outputLen = 0; /* Matches what PK11_CipherOp does */
        PORT_SetError(SEC_ERROR_OUTPUT_LEN);
        return SECFailure;
    }
    ... the original code ...

I suspect the callers of SSLCipher ignore *outputLen on failure.

Comment 5

[Ryan Sleevi]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO me if you want a response) from comment #3)
> Comment on attachment 826186 [details] [diff] [review]
> Cap to maxOutputLen in Null_Cipher
> 
> Review of attachment 826186 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ryan, you didn't add a reviewer to the review request. This can happen if
> the reviewer you chose isn't part of the security group. In that case, CC
> them to the bug first.
> 
> Anyway, I suggest that this check be made a PORT_Assert in Null_Cipher, and
> instead we do the real check in ssl3_HandleRecord.
> 
> I believe the problem is actually here in ssl3_HandleRecord:
> 
>     if (isTLS && cText->buf->len - ivLen > (MAX_FRAGMENT_LENGTH + 2048)) {
>         ssl_ReleaseSpecReadLock(ss);
>         SSL3_SendAlert(ss, alert_fatal, record_overflow);
>         PORT_SetError(SSL_ERROR_RX_RECORD_TOO_LONG);
>         return SECFailure;
>     }
> 
>     ...
> 
>     /* 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);
> 
>     ...
> 
> It seems to me we need to be checking (cText->buf->len - ivLen >
> plaintext->space) instead, or in addition to, the above check.
> 
> If you think adding the check to Null_Cipher only is the best approach, then
> please add a comment about why only Null_Cipher needs to do the check.

Because Null_Cipher should match the API contract of the other ciphers. That's the root of the bug. I think the additional checking could be seen as orthogonal, and could certainly bolster against additional concerns, but PK11CipherOp or the individual operations are all required to not write more than maxOutputLen.

That said, Wan-Teh is correct, that instead of copying the max, I should be mimicing the full error behaviour (SEC_ERROR_OUTPUT_LEN) instead.

Comment 6

[Ryan Sleevi]

Attachment: patch2.diff

Properly match the API contract.

Comment 7

[Wan-Teh Chang]

Comment on attachment 826216 [details] [diff] [review]
patch2.diff

r=wtc.

Comment 8

[Ryan Sleevi]

Comment on attachment 826216 [details] [diff] [review]
patch2.diff

Checked in as https://hg.mozilla.org/projects/nss/rev/e79a09364b5e

Comment 9

[Ryan Sleevi]

dveditz: We'll presumably need a CVE for this. Remotely exploitable ability for attacker to copy arbitrary memory = sad.

Comment 10

[Wan-Teh Chang]

Comment on attachment 826216 [details] [diff] [review]
patch2.diff

Review of attachment 826216 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/ssl/ssl3con.c
@@ +819,5 @@
>  Null_Cipher(void *ctx, unsigned char *output, int *outputLen, int maxOutputLen,
>  	    const unsigned char *input, int inputLen)
>  {
> +    if (inputLen > maxOutputLen) {
> +        *outputLen = 0;  /* Match PK11_CipherOp in setting outputLen */

I have to admit I was surprised that PK11_CipherOp
sets *outputLen to 0 for any PKCS #11 error:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pk11wrap/pk11cxt.c&rev=1.10&mark=705-710,716-722#705

For CKR_BUFFER_TOO_SMALL, I think PK11_CipherOp should
set *outputLen to the required output buffer size.

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 826216 [details] [diff] [review]
patch2.diff

Review of attachment 826216 [details] [diff] [review]:
-----------------------------------------------------------------

Just setting sec-approval? so that people who handle approvals are aware that this already got checked into the NSS public repository, and so it is publicly-disclosed. I think we're supposed to use the sec-approval process for NSS, too, right?

Comment 12

[Al Billings [:abillings - ex-MoCo]]

We should be using sec-approval for everything but using it as a notification is kind of pointless unless you're telling us that someone screwed up, checked it in, and won't do it again.

The whole point of sec-approval is that we gate when security work gets checked in to lessen the chances of a zero day and to lower exposure.

Comment 13

[Daniel Veditz [:dveditz]]

well, it's also "hey this got checked in, I don't know what flags to set please make sure it's all OK for other branches". I'd rather have that sort of thing flagged than not if the people involved in the bug aren't sure.

Comment 14

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 826216 [details] [diff] [review]
patch2.diff

All right. I'm clearing the flag unless there is something actionable to do with it.

Comment 15

[Ryan Sleevi]

I checked this in to Moz-NSS because our security team wanted this checked into Chromium's repository so that we could get a release out. Checking it into Chromium would have had the effect of disclosing, independent of sec-approval.

Comment 16

[Al Billings [:abillings - ex-MoCo]]

Ryan, is there a way we can coordinate these checkins in the future so we can know ahead of time and possibly massage dates of the checkins, if necessary?

Comment 17

[Ryan Sleevi]

Al: That was the path I was advocating, but there was some resistance from our security-team on that coordination. I'll let inferno respond to the coordination effort, which is why I added him explicitly.

Comment 18

[Abhishek Arya]

(In reply to Ryan Sleevi from comment #17)
> Al: That was the path I was advocating, but there was some resistance from
> our security-team on that coordination. I'll let inferno respond to the
> coordination effort, which is why I added him explicitly.

Here is what I suggested in the chromium bug (https://code.google.com/p/chromium/issues/detail?id=314225) - to commit the patch to Chromium trunk (since we have an upcoming m31 release and it is a sec-critical bug.) and we won't declare it in the release notes (or assign CVE) until all the consumers have patched it. Also, it didn't mean we can commit it to Firefox trunk since we need to follow Mozilla's sec-approval process (which as i understand is to explicitly request for approval for committing and not just merging to a branch). 

In the future, we are open to ideas to improve here. Like we will make sure to give heads up on upcoming chromium side commit. We won't be able to hold the commit for too long since usually our codereviews are public and searchable, and our security releases go around every 2 weeks (one stable release and two stable patches).

Comment 19

[Daniel Veditz [:dveditz]]

(In reply to Abhishek Arya from comment #18)
> and we won't declare it in the release notes (or assign CVE) until all
> the consumers have patched it.

"Assign" or "publish" the CVE? Given the nature of NSS as a shared library this bug should only get a single CVE, it should not have a "firefox one" and a "chrome one". Since NSS is currently hosted by Mozilla we assigned it one from our pool yesterday (CVE-2013-5605) but if you've already given it one we can use that.

Comment 20

[Abhishek Arya]

(In reply to Daniel Veditz [:dveditz] from comment #19)
> (In reply to Abhishek Arya from comment #18)
> > and we won't declare it in the release notes (or assign CVE) until all
> > the consumers have patched it.
> 
> "Assign" or "publish" the CVE? Given the nature of NSS as a shared library
> this bug should only get a single CVE, it should not have a "firefox one"
> and a "chrome one". Since NSS is currently hosted by Mozilla we assigned it
> one from our pool yesterday (CVE-2013-5605) but if you've already given it
> one we can use that.

I think our release is going out next week. I will make sure that we don't declare this in the release notes (since i think you won't have time to release the patch by then).

Since we are not declaring this, we have not assigned the CVE yet. So, we will just use CVE-2013-5605 (and i agree we don't need multiple cves for this :)).

Comment 21

[Al Billings [:abillings - ex-MoCo]]

The only way this will go out by next week is if we did an emergency chemspill release to get it out. We're debating that right now.

It *would* be nice if our partners didn't do things in a way that caused us to have to do emergency security releases though (and warned us when they were going to, rather than after the fact).

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

From discussion in email it looks like we will not chemspill for this, and we'll get it into FF26 (releasing on Dec 10th) as well as other branches including B2g 26.  Does this impact ESR24?

Comment 23

[Daniel Veditz [:dveditz]]

Yes, Esr24 is affected. I don't know why we're not planning an interim release to address this.

Comment 24

[Kai Engert (:KaiE:)]

It has been proposed to use several patches for the intermediate NSS release, appropriate for ESR24 and 26.

I've filed bug 935959 to track the landing of that NSS version into the various Mozilla branches.

I propose to move approvals over to that bug.

Comment 25

[Matt Wobensmith [:mwobensmith][:matt:]]

Hello Kai, Brian, other developers.

Do you have any ideas on how to ensure that 
a) we've addressed the problem, and 
b) we haven't broken anything?

Aside from a coordinated effort to use SSL-enabled sites and look for regressions, any other information you might have would be greatly appreciated by QA. 

Thanks.

Comment 26

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I am curious about the conditions under which this bug can be reproduced.

Comment 1 says that this was found by [e-mail] by fuzzing NSS. Was that fuzzing done by writing a program that calls ssl3_HandleRecord directly with various arguments? Or, was that fuzzing done in a way that simulates a real SSL connection? If the latter, could you be more specific regarding the circumstances in which the bug can be triggered.

Comment 27

[Andrew Tinits]

(In reply to Brian Smith (:briansmith, was :bsmith; Please NEEDINFO? me if you want a response) from comment #26)
> I am curious about the conditions under which this bug can be reproduced.
> 
> Comment 1 says that this was found by [e-mail] by fuzzing NSS.
> Was that fuzzing done by writing a program that calls ssl3_HandleRecord
> directly with various arguments? Or, was that fuzzing done in a way that
> simulates a real SSL connection? If the latter, could you be more specific
> regarding the circumstances in which the bug can be triggered.

The fuzzing was done on real SSL connections between Chromium and a custom SSL server.  The bug can be triggered by a server sending a single malformed record in response to a client hello.

Comment 28

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Andrew Tinits from comment #27)
> The fuzzing was done on real SSL connections between Chromium and a custom
> SSL server.  The bug can be triggered by a server sending a single malformed
> record in response to a client hello.

Thanks Andrew. I tried to reproduce this yesterday in Firefox, but I was finding that NSS was rejecting the malformed records due to other checks on the record size. Could you share which record size you used in the header of that malformed record? Thanks again.

Comment 29

[Wan-Teh Chang]

Brian: use a record length of 18434, which is MAX_FRAGMENT_LENGTH + 2048 + 2.

This will evade the gs->remainder > (MAX_FRAGMENT_LENGTH + 2048 + 5) check
in ssl3_GatherData, but is bigger than the MAX_FRAGMENT_LENGTH + 2048 space
allocated by the sslBuffer_Grow(&gs->buf, 4096) call in ssl_InitGather.
sslBuffer_Grow always allocates at least MAX_FRAGMENT_LENGTH + 2048 bytes.

ss->gs.buf is passed to ssl3_HandleRecord as the 'databuf' argument, which
gets assigned to the 'plaintext' local variable.

Do you want the 18434-byte record? It seems that you know how to generate
one. You just need to know the right size.

Comment 30

[Wan-Teh Chang]

Brian: I noticed that the record size checks in ssl3_HandleRecord are only
performed if 'isTLS' is true. That seems wrong.

We should also find out if ssl3_GatherData should allow 5 more bytes than
MAX_FRAGMENT_LENGTH + 2048.

Comment 31

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Wan-Teh Chang from comment #30)
> Brian: I noticed that the record size checks in ssl3_HandleRecord are only
> performed if 'isTLS' is true. That seems wrong.
> 
> We should also find out if ssl3_GatherData should allow 5 more bytes than
> MAX_FRAGMENT_LENGTH + 2048.

The value 2048 represents that absolute largest overhead possible of a ciphertext record over a plaintext record. For a plaintext record (which is what this bug is about), the maximum allowed size is MAX_FRAGMENT_LENGTH. It is also possible to calculate tighter bounds for encrypted records, but the overhead depends on the symmetric encryption and authentication method used.

I believe that the record length checks were originally restricted to TLS 1.0 because some SSL 3.0 implementations might not obey the limits in the TLS 1.0 spec. However, my memory about this is very hazy.

Comment 32

[Lukas Blakk [:lsblakk] use ?needinfo]

Last one - Matt did bug 935959 make this bug fixed or is there other work to be done here?

Comment 33

[Matt Wobensmith [:mwobensmith][:matt:]]

My understanding is that all work was covered in related bug 935959. If someone disagrees, they can add more info.

Comment 34

[Lukas Blakk [:lsblakk] use ?needinfo]

marking this accordingly then, it went out in the dot release for 25.0.1