1413841 - (CVE-2018-5122) WebCryptoTask integer overflow

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[slei.casper]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.75 Safari/537.36

Steps to reproduce:

AesTask:: DoCrypt() in dom/crypto/WebCryptoTask.cpp
potential integer overflow. if dataLen is bigger than or equal to 0xfffffff0, maxLen would overflow, so mResult's length would be a small integer. then calling PK11_Encrypt or PK11_Decrypt could cause writing beyond mResult's buffer

    uint32_t dataLen = mData.Length();
    uint32_t maxLen = dataLen + 16;
    if (!mResult.SetLength(maxLen, fallible)) {
      return NS_ERROR_DOM_UNKNOWN_ERR;
    }
    uint32_t outLen = 0;

    // Perform the encryption/decryption
    if (mEncrypt) {
      rv = MapSECStatus(PK11_Encrypt(symKey.get(), mMechanism, &param,
                                     mResult.Elements(), &outLen,
                                     mResult.Length(), mData.Elements(),
                                     mData.Length()));
    } else {
      rv = MapSECStatus(PK11_Decrypt(symKey.get(), mMechanism, &param,
                                     mResult.Elements(), &outLen,
                                     mResult.Length(), mData.Elements(),
                                     mData.Length()));
    }

Comment 1

[Daniel Veditz [:dveditz]]

Can we have JavaScript strings big enough to cause this in practice? Maybe if dataLen is bytes and JS string length is characters?

JC: please take a look. If it's possible it's probably a sec-critical type bug.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

This shouldn't be exploitable. We pass `mResult.Length()` to both the encrypt and decrypt function. That's the `maxLen` argument. If the ciphertext and plaintext doesn't fit they'll both fail instead of writing OOB.

Comment 3

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

I've made a simple test case to construct a giant string. It fails allocating a string of 0xfffffff1 length with:

uncaught exception: out of memory

Test case: https://usr.bin.coffee/bigstring/

Comment 4

[slei.casper]

(In reply to J.C. Jones [:jcj] from comment #3)
> I've made a simple test case to construct a giant string. It fails
> allocating a string of 0xfffffff1 length with:
> 
> uncaught exception: out of memory
> 
> Test case: https://usr.bin.coffee/bigstring/

it's a potential bug.

webcrypto would call this passing data using arraybuffer.
https://developer.mozilla.org/en-US/docs/Web/API/SubtleCrypto/encrypt


and arraybuffer's max length is Number.MAX_SAFE_INTEGER
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/ArrayBuffer

I tested on Firefox with ArrayBuffer's max length which is ```new Uint8Array(0x7ffffffe)```, but if the max length of ArrayBuffer is changed in the future, this might be a problem.

Comment 5

[slei.casper]

(In reply to Tim Taubert [:ttaubert] from comment #2)
> This shouldn't be exploitable. We pass `mResult.Length()` to both the
> encrypt and decrypt function. That's the `maxLen` argument. If the
> ciphertext and plaintext doesn't fit they'll both fail instead of writing
> OOB.

I didn't see any check about MaxLen in https://searchfox.org/mozilla-central/source/security/nss/lib/pk11wrap/pk11obj.c#945

maybe it would be checked in C_Encrypt?

crv = PK11_GETTAB(slot)->C_Encrypt(session, (unsigned char *)data,
                                       dataLen, out, &len);

Comment 6

[slei.casper]

(In reply to slei.casper from comment #5)
> (In reply to Tim Taubert [:ttaubert] from comment #2)
> > This shouldn't be exploitable. We pass `mResult.Length()` to both the
> > encrypt and decrypt function. That's the `maxLen` argument. If the
> > ciphertext and plaintext doesn't fit they'll both fail instead of writing
> > OOB.
> 
> I didn't see any check about MaxLen in
> https://searchfox.org/mozilla-central/source/security/nss/lib/pk11wrap/
> pk11obj.c#945
> 
> maybe it would be checked in C_Encrypt?
> 
> crv = PK11_GETTAB(slot)->C_Encrypt(session, (unsigned char *)data,
>                                        dataLen, out, &len);

I add a breakpoint in C_Encrypt and quickly fails into NSC_Encrypt, NSC_Encrypt didn't check the `len`, and directly use `len` in 	    crv = NSC_EncryptUpdate(hSession, pData, ulDataLen, pEncryptedData, 
	                            pulEncryptedDataLen);


https://searchfox.org/mozilla-central/source/security/nss/lib/softoken/pkcs11c.c#1370

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

(In reply to slei.casper from comment #6)
> I add a breakpoint in C_Encrypt and quickly fails into NSC_Encrypt,
> NSC_Encrypt didn't check the `len`, and directly use `len` in 	    crv =
> NSC_EncryptUpdate(hSession, pData, ulDataLen, pEncryptedData, 
> 	                            pulEncryptedDataLen);

Right, NSC_EncryptUpdate() does the following:

> unsigned int maxout = *pulEncryptedPartLen;

And that's what all of these functions have to do. And are hopefully doing. It's not ideal but with a generic layer like that we can't know what output buffer requirements different crypto algorithms have.

AES_Encrypt() for example has all the proper checks before writing to a given buffer.

https://searchfox.org/mozilla-central/source/security/nss/lib/freebl/rijndael.c#1052

GCM_EncryptUpdate() too:

https://searchfox.org/mozilla-central/source/security/nss/lib/freebl/gcm.c#660

That said, it might not hurt to try and fuzz these APIs directly with at least the common algorithms.

We could definitely be paranoid and ensure that `dataLen < 0xfffffff0` in the WebCrypto API.

Comment 8

[Phabricator Automation]

One revision was made private due to unknown Bugzilla groups.

Comment 9

[Phabricator Automation]

One revision was made private due to unknown Bugzilla groups.

Comment 10

[Phabricator Automation]

One revision was made private due to unknown Bugzilla groups.

Comment 11

[Phabricator Automation]

One revision was made private due to unknown Bugzilla groups.

Comment 12

[Phabricator Automation]

One revision was made private due to unknown Bugzilla groups.

Comment 13

[Phabricator Automation]

One revision was made private due to unknown Bugzilla groups.

Comment 14

[Phabricator Automation]

One revision was made private due to unknown Bugzilla groups.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

I'd call this sec-low, given how unlikely that is going to be a problem.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

A link to the patch, while the bot is having fun:

https://phabricator.services.mozilla.com/D188

Comment 17

[Phabricator Automation]

One revision was made private due to unknown Bugzilla groups.

Comment 18

[Phabricator Automation]

One revision was made private due to unknown Bugzilla groups.

Comment 19

[Phabricator Automation]

One revision was made private due to unknown Bugzilla groups.

Comment 20

[Phabricator Automation]

One revision was made private due to unknown Bugzilla groups.

Comment 21

[Phabricator Automation]

One revision was made private due to unknown Bugzilla groups.

Comment 22

[Phabricator Automation]

One revision was made private due to unknown Bugzilla groups.

Comment 23

[Phabricator Automation]

One revision was made private due to unknown Bugzilla groups.

Comment 24

[Phabricator Automation]

One revision was made private due to unknown Bugzilla groups.

Comment 25

[Phabricator Automation]

One revision was made private due to unknown Bugzilla groups.

Comment 26

[Phabricator Automation]

One revision was made private due to unknown Bugzilla groups.

Comment 27

[Phabricator Automation]

One revision was made private due to unknown Bugzilla groups.

Comment 28

[Phabricator Automation]

One revision was made private due to unknown Bugzilla groups.

Comment 29

[Phabricator Automation]

One revision was made private due to unknown Bugzilla groups.

Comment 30

[Phabricator Automation]

One revision was made private due to unknown Bugzilla groups.

Comment 31

[Phabricator Automation]

One revision was made private due to unknown Bugzilla groups.

Comment 32

[Phabricator Automation]

One revision was made private due to unknown Bugzilla groups.

Comment 33

[Phabricator Automation]

One revision was made private due to unknown Bugzilla groups.

Comment 34

[Phabricator Automation]

One revision was made private due to unknown Bugzilla groups.

Comment 35

[Tim Taubert [:ttaubert] (inactive)]

Trying to tame phab-bot.

Comment 36

[Phabricator Automation]

One revision was made private due to unknown Bugzilla groups.

Comment 37

[Tim Taubert [:ttaubert] (inactive)]

Gentle review ping? :)

Comment 38

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Apparently I don't have access to that object :(

Comment 39

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

Please fix review permissions; might need to select a different reviewer since keeler is unavailable next week.

Comment 40

[Mark Côté [:mcote]]

ttaubert, did you (or someone else) apply a custom policy to that revision?

Comment 41

[Tim Taubert [:ttaubert] (inactive)]

I just did, yeah. Now David can see and edit it. I'd expect the bot to do that automatically next time?

Comment 42

[Mark Côté [:mcote]]

Yes, it should apply policies automatically.  Not entirely sure why it didn't this time, after we fixed it... anyway we're making some changes to this code to make it a bit more robust.

Comment 43

[Phabricator Automation]

Comment on attachment 8924995 [details]
Bug 1413841 - Check for integer overflow in AesTask::DoCrypto() r=keeler

David Keeler [:keeler] (use needinfo) has approved the revision.

https://phabricator.services.mozilla.com/D188#7027

Comment 44

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/707c38afa7a3

Comment 45

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance. It grafts cleanly as-landed.

Comment 46

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8924995 [details]
Bug 1413841 - Check for integer overflow in AesTask::DoCrypto() r=keeler

Approval Request Comment
[Feature/Bug causing the regression]: Long-standing bug.
[User impact if declined]: Highly unlikely OOB write when passing a gigantic ArrayBuffer to the WebCrypto API, hitting an algorithm or PKCS11 module that doesn't properly check buffer lengths. All algorithms exposed by us via the WebCrypto API seem to do all the required checks.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: We can't really verify this.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: This small patch adds a simple test to check whether an integer addition will overflow. We have automated tests that tell that the check seems fine and doesn't reject valid data.
[String changes made/needed]: None.

Comment 47

[Gerry Chang [:gchang]]

Comment on attachment 8924995 [details]
Bug 1413841 - Check for integer overflow in AesTask::DoCrypto() r=keeler

Fix a security issue. Beta58+.

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9db3839609e0