1586176 - (CVE-2019-11745) Out-of-bounds write when passing an output buffer smaller than the block size to NSC_EncryptUpdate

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Craig Disselkoen]

In NSC_EncryptUpdate, suppose that the *pulEncryptedPartLen parameter is smaller than the block size.

We assume the following path through the function:

...
if (crv != CKR_OK)  // false
...
if (!pEncryptedPart) {  // false
...
if (context->doPad) {  // true
    /* deal with previous buffered data */
    if (context->padDataLength != 0) {  // true
        /* fill in the padded to a full block size */
        for (...) {
            // this loop continues until context->padDataLength == context->blockSize
            // for this to happen, it's sufficient that ulPartLen >= context->blockSize
            //   i.e., that there are at least context->blockSize bytes of input
            // or more precisely, this will happen when
            //   ulPartLen + context->padDataLength >= context->blockSize
        }
        /* not enough data to encrypt yet? then return */
        if (context->padDataLength != context->blockSize) {  // false, due to above
          ...
        }
        /* encrypt the current padded data */
        rv = (*context->update)(context->cipherInfo, pEncryptedPart,
                                &padoutlen, context->blockSize, context->padBuf,
                                context->blockSize);

In the call to context->update, the fourth parameter indicates the maximum length which can be written to the output buffer pEncryptedPart. This parameter is set to be context->blockSize, even though only *pulEncryptedPartLen bytes remain in the output buffer, and we were assuming that *pulEncryptedPartLen was less than the block size.

There are two ramifications of this: first, we can have a small out-of-bounds write here. The call to context->update can write up to about context->blockSize bytes off of the end of the output buffer.

Second, context->update will write to the variable padoutlen indicating the number of bytes written. Supposing context->update does write the allowed maximum of context->blockSize bytes to the buffer, we will have padoutlen == context->blockSize. Then:

pEncryptedPart += padoutlen;  // advancing the pEncryptedPart pointer beyond the end of the buffer
maxout -= padoutlen;

Since maxout was initialized to be *pulEncryptedPartLen and is thus smaller than the block size in this case, the subtraction will wrap around and give a very large positive value for maxout due to its unsigned type.

The subsequent use of maxout here:

/* do it: NOTE: this assumes buf size in is >= buf size out! */
rv = (*context->update)(context->cipherInfo, pEncryptedPart,
                        &outlen, maxout, pPart, ulPartLen);

will allow context->update to write up to maxout many bytes, that is, effectively arbitrarily many bytes to pEncryptedPart. This is an out-of-bounds write arbitrarily far beyond the buffer.

I think the first call to context->update should pass maxout rather than context->blockSize as the fourth parameter, so that the maximum output length is maxout rather than context->blockSize. I.e.,

        rv = (*context->update)(context->cipherInfo, pEncryptedPart,
-                               &padoutlen, context->blockSize, context->padBuf,
+                               &padoutlen, maxout, context->padBuf,
                                context->blockSize);

The related function NSC_DecryptUpdate already does this: it uses maxout for the maximum-output-length parameter to context->update every time it calls context->update.

Comment 1

[Craig Disselkoen]

Attachment: 0001-Bug-1586176-EncryptUpdate-should-use-maxout-not-bloc.patch

Comment 2

[J.C. Jones [:jcj] (he/they)]

Kevin, please provide the first-pass security classification.

Comment 3

[Deian Stefan]

I wanted to write a test for this last night, but couldn't find tests that consume the APIs directly. If ya'll point me to a sane example I'm happy to take a stab at adding a test.

Comment 4

[Kevin Jacobs [:kjacobs]]

I didn't get as far into this today as I was hoping. However, the sanest example I see is pk11_cbc_unittest.cc, which uses CKM_AES_CBC_PAD to call NSC_EncryptUpdate via PK11_Encrypt.

The trick seems to be getting if (context->padDataLength != 0) to be true, because on the first invocation, the context (and padDataLength) is fresh. From an initial pass, this doesn't seem to be possible in PK11_Encrypt, but there are other paths that will need to be checked. PK11_CipherOp looks to be more flexible.

I'll continue looking into this next week. Thanks!

Comment 5

[Deian Stefan]

Attachment: 0001-Bug-1586176-Edge-case-multi-part-EncryptUpdate-test.patch

Test case to trigger the bug

Comment 6

[Kevin Jacobs [:kjacobs]]

Thanks. I confirmed both outcomes from the report.

To exploit we need two things then: 1) multi-part encryption, and 2) the property that a call can be made with *pulEncryptedPartLen (maxout) of less than one block, via attacker-control or a programming error. Webcrypto is not affected as neither condition holds. This leaves the following notable paths (among others that are more trivially unexploitable).

1. ssl3_MACEncryptRecord: If p1Len is less than 256, it is moved to oddLen and encrypted with p2 (where maxout will be at least "MAC bytes" long). This should be safe.
2. SRTP calls this function for both encrypt and decrypt, but uses in-place operations (where outLen == inLen), so the arbitrary overwrite is not possible (*pulEncryptedPartLen < BLOCK_SIZE && *pulEncryptedPartLen == ulPartLen). This still leaves the potential for an up-to-blocksize overrun.

I'm only seeing a Firefox exploit path through #2, but I'd label this sec-high as: 1) #2 suggests this is remotely exploitable since it's called to decrypt, and 2) the bug lies in a general-purpose function with many callers. If one fails to protect (or session is inappropriately reused), we could well be vulnerable to the arbitrary overwrite case.

Let me know if my analysis missed anything.

Comment 7

[Craig Disselkoen]

If y'all are interested, this bug was found with Pitchfork, a symbolic execution tool we're working on to detect constant-time violations. In this case, Pitchfork found inputs that triggered the arbitrary write and then used that power to trivially violate constant-time (e.g. by overwriting some pointer to point to a secret key - I don't remember the precise details at this moment). I believe Deian has already mentioned the tool to some of you.

Comment 8

[J.C. Jones [:jcj] (he/they)]

Franziskus, can you take a first pass at this patch?

Comment 9

[Franziskus Kiefer [:franziskus]]

Comment on attachment 9099058 [details] [diff] [review]
0001-Bug-1586176-Edge-case-multi-part-EncryptUpdate-test.patch

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

::: gtests/pk11_gtest/pk11_cbc_unittest.cc
@@ +205,5 @@
>    EXPECT_EQ(0, memcmp(kInput, decrypted, input_len));
>  }
>  
> +TEST_P(Pkcs11CbcPadTest, ContextEncryptTwoBadParts) {
> +  uint8_t encrypted[block_size() - 2]; // Output is less than block size

This won't compile on Windows. Maybe just do 14?

Comment 10

[Franziskus Kiefer [:franziskus]]

Comment on attachment 9098741 [details] [diff] [review]
0001-Bug-1586176-EncryptUpdate-should-use-maxout-not-bloc.patch

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

Looks like the right fix to me, thanks!

Comment 11

[Kevin Jacobs [:kjacobs]]

Comment on attachment 9098741 [details] [diff] [review]
0001-Bug-1586176-EncryptUpdate-should-use-maxout-not-bloc.patch

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Moderate difficulty. The change highlights the problem, but an attacker has to trigger a moderately-complex set of conditions to exploit. The tests will not be landed until a later date.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: All
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: Little risk and clean application to all branches.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely. It's a single line change and the corresponding Decrypt function already uses maxout as in this patch.

Comment 12

[Deian Stefan]

Attachment: 0001-Bug-1586176-EncryptUpdate-should-use-maxout-not-bloc.patch

rebased, r=me

Comment 13

[Deian Stefan]

Attachment: 0002-Bug-1586176-Edge-case-multi-part-EncryptUpdate-test..patch

rebased + addressed comment, r=me

Comment 14

[Deian Stefan]

Sorry didn't mean to remove the sec-approval request; should i re-flag and just point to :kjacobs' comment?

Comment 15

[Kevin Jacobs [:kjacobs]]

Comment on attachment 9100044 [details] [diff] [review]
0001-Bug-1586176-EncryptUpdate-should-use-maxout-not-bloc.patch

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Moderate difficulty. The change highlights the problem, but an attacker has to trigger a moderately-complex set of conditions to exploit. The tests will not be landed until a later date.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: Ally
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: Little risk and clean application to all branches.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely. It's a single line change and the corresponding Decrypt function already uses maxout as in this patch.

Comment 16

[Tom Ritter [:tjr]]

Comment on attachment 9100044 [details] [diff] [review]
0001-Bug-1586176-EncryptUpdate-should-use-maxout-not-bloc.patch

Beta/Release Uplift Approval Request

• User impact if declined: sec-high in network code sits around for another release.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Per kjacobs: "Unlikely. It's a single line change and the corresponding Decrypt function already uses maxout as in this patch."
• String changes made/needed:

Comment 17

[Tom Ritter [:tjr]]

I'm requesting uplift to beta; but leaving the sec-approval flag so it stays in-queue.

This has me nervous since it's in networking code (outside the sandbox.) I don't think we should land this in Nightly without also landing it in beta. So if Beta gets approved; we'll land it in both, and if it doesn't, we'll re-evaluate.

Either way we should not land the test until the normal 4-weeks after the release train gets the fix.

Comment 18

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 9100044 [details] [diff] [review]
0001-Bug-1586176-EncryptUpdate-should-use-maxout-not-bloc.patch

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: When we go to Beta, we'll want to catch ESR as well. If it's just up to me, I'd be OK on waiting to land this one for next cycle though (Beta 71, ESR 68.3), though patch risk is indeed low.
• User impact if declined: sec-high in network code sits around for ESR
• Fix Landed on Version: n/a
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Per kjacobs: "Unlikely. It's a single line change and the corresponding Decrypt function already uses maxout as in this patch."
• String or UUID changes made by this patch:

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

Dan what do you think about the riskiness here? The build is tomorrow so I have to decide by Sunday night.
I'm inclined to go for it, since folks seem to agree the patch risk is low even though last minute changes in network code always feel a bit scary.

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

After more discussion with Tom I'm going to go ahead and uplift, but a bit later today after the merge.

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

After even more discussion: This would need an NSS release as well so we may hold off till either a point release in mid-70, or till 71.

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9100044 [details] [diff] [review]
0001-Bug-1586176-EncryptUpdate-should-use-maxout-not-bloc.patch

Beta/Release Uplift Approval Request

• User impact if declined:
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):
• String changes made/needed:

Comment 23

[J.C. Jones [:jcj] (he/they)]

Tom, can you allocate a CVE for this?

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Keep in mind that if we point release Fx70, we'll need to do the same for 68.2esr. Is there a particularly-urgent reason this can't ship in Fx71/68.3esr in early December?

Comment 25

[J.C. Jones [:jcj] (he/they)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #24)

Keep in mind that if we point release Fx70, we'll need to do the same for 68.2esr. Is there a particularly-urgent reason this can't ship in Fx71/68.3esr in early December?

I filed a bug for ESR, too (bug 1588558), but while this issue is severe, I don't see any reason to consider it critical -- or critical enough for a point release on its own.

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

OK, calling it wontfix for 70.

Comment 27

[J.C. Jones [:jcj] (he/they)]

Daiki, a heads-up for you on this one. We're deferring it for up to a cycle here and it might have wider impacts for you and your customers than Firefox.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9100044 [details] [diff] [review]
0001-Bug-1586176-EncryptUpdate-should-use-maxout-not-bloc.patch

Uplift approval for this patch will run through the various dependent "Update NSS" bugs on file rather than through this bug directly.

Comment 29

[Tom Ritter [:tjr]]

Comment on attachment 9100044 [details] [diff] [review]
0001-Bug-1586176-EncryptUpdate-should-use-maxout-not-bloc.patch

Plan is NSS 3.46.2 for a potential 70 dot release, 3.47.1 for 71 beta, and 3.44.3 for 68 ESR.  If we get a dot release, we'd probably want to land this sooner rather than later so we know it's stable for that. But otherwise I'd be inclined to land it later.... But cutting NSS releases is tricky and we don't want to do it before releng is ready to take it.  So I think the conclusion is: JC, you can land this when you think is best and it fits the NSS schedule.

Comment 30

[J.C. Jones [:jcj] (he/they)]

https://hg.mozilla.org/projects/nss/rev/1e22a0c93afe9f46545560c86caedef9dab6cfda

Comment 31

[J.C. Jones [:jcj] (he/they)]

https://hg.mozilla.org/releases/mozilla-beta/rev/4b6cb17b74d9

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/ea1bc0fb2dda

Comment 33

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 34

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 35

[Tom Ritter [:tjr]]

Attachment: advisory.txt