Closed Bug 1413841 (CVE-2018-5122) Opened 2 years ago Closed 2 years ago

WebCryptoTask integer overflow

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- wontfix
firefox57 --- wontfix
firefox58 --- fixed
firefox59 --- fixed

People

(Reporter: slei.casper, Assigned: ttaubert)

Details

(Keywords: csectype-intoverflow, sec-low, Whiteboard: [domsecurity-active][adv-main58+][post-critsmash-triage])

Attachments

(1 file)

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()));
    }
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.
Group: firefox-core-security → dom-core-security
Component: Untriaged → DOM: Security
Flags: needinfo?(jjones)
Product: Firefox → Core
Flags: needinfo?(jjones)
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'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/
(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.
(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);
(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
(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.
One revision was made private due to unknown Bugzilla groups.
Assignee: nobody → ttaubert
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
I'd call this sec-low, given how unlikely that is going to be a problem.
A link to the patch, while the bot is having fun:

https://phabricator.services.mozilla.com/D188
Keywords: sec-criticalsec-low
Trying to tame phab-bot.
Group: core-security
Group: core-security
Gentle review ping? :)
Flags: needinfo?(dkeeler)
Apparently I don't have access to that object :(
Flags: needinfo?(dkeeler)
Please fix review permissions; might need to select a different reviewer since keeler is unavailable next week.
Flags: needinfo?(ttaubert)
ttaubert, did you (or someone else) apply a custom policy to that revision?
I just did, yeah. Now David can see and edit it. I'd expect the bot to do that automatically next time?
Flags: needinfo?(ttaubert)
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.
Priority: -- → P2
Whiteboard: [domsecurity-active]
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
Attachment #8924995 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/707c38afa7a3
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Please request Beta approval on this when you get a chance. It grafts cleanly as-landed.
Flags: needinfo?(ttaubert)
Group: dom-core-security → core-security-release
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.
Flags: needinfo?(ttaubert)
Attachment #8924995 - Flags: approval-mozilla-beta?
Comment on attachment 8924995 [details]
Bug 1413841 - Check for integer overflow in AesTask::DoCrypto() r=keeler

Fix a security issue. Beta58+.
Attachment #8924995 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [domsecurity-active] → [domsecurity-active][adv-main58+]
Alias: CVE-2018-5122
Flags: qe-verify-
Whiteboard: [domsecurity-active][adv-main58+] → [domsecurity-active][adv-main58+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.