1723826 - Probably harmless integer overflow in ImportSymmetricKeyTask::BeforeCrypto()

Status: RESOLVED FIXED

(Core :: DOM: Web Crypto, defect)

Description

[mozillabugs]

Attachment: bug_2042_poc_2.htm

ImportSymmetricKeyTask::BeforeCrypto() (dom/crypto/WebCryptoTask.cpp) can experience an integer overflow when called with an unexpectedly-large argument. This causes a miscorrespondence between the actual key byte count (0x20000010 in the POC, below) and the byte count that FF records in its mKey object (0x80 in the POC, below). This doesn't appear to cause a problem at present, but would if something used the unoverflowed count to index an object containing only the overflowed number of bytes.

The bug is in line 1507, which computes 8 * mKeyData.Length() without checking for overflow. The function then stores the overflowed value into its mKey object on line 1525.

A POC is attached that provokes the overflow.

1484: virtual nsresult BeforeCrypto() override {
1485:     nsresult rv;
1486: 
1487:     // If we're doing a JWK import, import the key data
1488:     if (mDataIsJwk) {
...
1498:     }
1499:     // Check that we have valid key data.
1500:     if (mKeyData.Length() == 0 &&
1501:         !mAlgName.EqualsLiteral(WEBCRYPTO_ALG_PBKDF2)) {
1502:       return NS_ERROR_DOM_DATA_ERR;
1503:     }
1504: 
1505:     // Construct an appropriate KeyAlorithm,
1506:     // and verify that usages are appropriate
1507:     uint32_t length = 8 * mKeyData.Length();  // bytes to bits
1508:     if (mAlgName.EqualsLiteral(WEBCRYPTO_ALG_AES_CBC) ||
1509:         mAlgName.EqualsLiteral(WEBCRYPTO_ALG_AES_CTR) ||
1510:         mAlgName.EqualsLiteral(WEBCRYPTO_ALG_AES_GCM) ||
1511:         mAlgName.EqualsLiteral(WEBCRYPTO_ALG_AES_KW)) {
1512:       if (mKey->HasUsageOtherThan(CryptoKey::ENCRYPT | CryptoKey::DECRYPT |
1513:                                   CryptoKey::WRAPKEY | CryptoKey::UNWRAPKEY)) {
1514:         return NS_ERROR_DOM_DATA_ERR;
1515:       }
1516: 
1517:       if (mAlgName.EqualsLiteral(WEBCRYPTO_ALG_AES_KW) &&
1518:           mKey->HasUsageOtherThan(CryptoKey::WRAPKEY | CryptoKey::UNWRAPKEY)) {
1519:         return NS_ERROR_DOM_DATA_ERR;
1520:       }
1521: 
1522:       if ((length != 128) && (length != 192) && (length != 256)) {
1523:         return NS_ERROR_DOM_DATA_ERR;
1524:       }
1525:       mKey->Algorithm().MakeAes(mAlgName, length);
...
1562:     if (NS_FAILED(mKey->SetSymKey(mKeyData))) {
1563:       return NS_ERROR_DOM_OPERATION_ERR;
1564:     }
1565: 
1566:     mKey->SetType(CryptoKey::SECRET);
1567: 
1568:     if (mDataIsJwk && !JwkCompatible(mJwk, mKey)) {
1569:       return NS_ERROR_DOM_DATA_ERR;
1570:     }
1571: 
1572:     mEarlyComplete = true;
1573:     return NS_OK;
1574:   }

Use the POC by starting FF 89.0.2 debug and attaching a debugger to it. Set a BP on line 1507, then load the POC. When the BP fires the first time, examine mKeyData.Length() and notice that it's 0x20000010 . Now step line 1507 and notice that length is 0x80, thus showing the overflow.

Continue execution, skipping all succeeding BPs. No apparent problem arises, though the POC takes several minutes to execute. The POC wraps and unwraps the long key, and no apparent security issue arises. However, the long key cannot successfully be used to encrypt anything; FF throws an exception when the POC attempts that.

The bug is present in trunk.

Comment 1

[Daniel Veditz [:dveditz]]

Dana: can you sanity check our thinking here? You feed in a long bogus key, and because of the overflow you mightr be able to fool us that it's a valid check (if the wrapped length checks out). The truncated length doesn't cause any buffer overflow, just incorrect processing. Is that "just a bug" or a potential security problem for webcrypto users?

Comment 2

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

As far as I can tell, it's "just a bug" - I don't see a security problem.

Comment 3

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

Martinho - do you have time to address this?

Comment 4

[R. Martinho Fernandes [:rmf]]

Yeah, I can address this.

Comment 5

[R. Martinho Fernandes [:rmf]]

Attachment: Bug 1723826 - Check for integer overflow with key length r=keeler

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8acfd9f29ad8
Check for integer overflow with key length r=keeler

Comment 7

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/8acfd9f29ad8