Closed Bug 1204155 (CVE-2015-7200) Opened 9 years ago Closed 9 years ago

Missing status check in CryptoKey::SetSymKey creates potential security bug

Categories

(Core :: DOM: Security, defect)

Product:
Core
Component:
DOM: Security
Version:
40 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla44
Tracking Flags:
Tracking Status
firefox41 --- wontfix
firefox42 --- fixed
firefox43 --- fixed
firefox44 --- fixed
firefox-esr38 --- wontfix

People

(Reporter: q1, Assigned: ttaubert)

Details

(Keywords: sec-moderate, Whiteboard: [post-critsmash-triage][adv-main42+])

Attachments

(4 files)

0001-Bug-1204155-Account-for-OOM-in-CryptoKey-SetSymKey.patch
4.41 KB, patch
mt
: review+
Details | Diff | Splinter Review
0002-Bug-1204155-Check-if-we-re-OOM-when-calling-SECKEY_C.patch
7.15 KB, patch
mt
: review+
Details | Diff | Splinter Review
0001-Bug-1204155-Account-for-OOM-when-setting-or-copying-.patch
9.78 KB, patch
ttaubert
: review+
Sylvestre
: approval-mozilla-aurora+
Details | Diff | Splinter Review
0001-Bug-1204155-Account-for-OOM-when-setting-or-copying-.patch
9.78 KB, patch
ttaubert
: review+
Sylvestre
: approval-mozilla-beta+
Details | Diff | Splinter Review 
CryptoKey::SetSymKey (dom\crypto\CryptoKey.cpp) assigns its argument to a member variable, but the destination isa CryptoBuffer, which isa FallibleTArray, so the assignment can fail if FF is nearly OOM due to some other operation. If it fails, the Javascript program that invoked SetSymKey (such as via crypto.subtle,deriveKey) receives a zero-length key. If it doesn't check the length before using the key for an encryption or signing operation, the resulting ciphertext or signature will be trivially weak.


Details:
--------

(cryptokey.cpp)

297: void CryptoKey::SetSymKey(const CryptoBuffer& aSymKey)
298: {
299:   mSymKey = aSymKey;
300: }

(cryptokey.h)

61: class CryptoKey final : public nsISupports,
62:                         public nsWrapperCache,
63:                         public nsNSSShutDownObject
64: {
...
196:  CryptoBuffer mSymKey;
...
199:};

(cryptobuffer.h)

21: class CryptoBuffer : public FallibleTArray<uint8_t>
...

There is also a similar bug in CryptoKey::ReadStructuredClone line 1128.

There is also a similar bug in ImportKeyTask::SetKeyData (dom\crypto\WebCryptoTask.cpp line 1289, though that one appears to be latent, at least as to the code path that arises from window.crypto.subtle,deriveKey (ImportSymmetricKeyTask::BeforeCrypto gets called after the assignment. It checks whether mKeyData.Length() == 0, and errors out if so). There may, however, be other non-latent paths into SetKeyData.
Group: core-security → dom-core-security
The following JS illustrates the issue. Use it by attaching a debugger to FF.

First run it without any breakpoints. You should see an alert box saying "keytype is oct with length 43 and k <base64 data>".

Next put a breakpoint on CryptoKey::SetSymKey and run the JS again. Skip the assignment and proceed to emulate an OOM. You should see no exceptions, with an alert box saying "keytype is oct with length 0 and k".

-------------------

<!DOCTYPE html>

<html>
<head>
</head>
<body>
<script type="text/javascript">

    window.crypto.subtle.generateKey(
        {
            name: "ECDH", namedCurve: "P-256",
        },
        true, ["deriveKey"]
    )
    .then(function (key) {

        window.crypto.subtle.deriveKey(
            {
                name: "ECDH", namedCurve: "P-256",
                public: key.publicKey,
            },
            key.privateKey,
            {
                name: "AES-CBC", length: 256,
            },
            true, ["encrypt", "decrypt"]
        )
        .then(function (keydata) {

            window.crypto.subtle.exportKey(
                "jwk", keydata)

            .then(function (keydata) {
                alert(
                    "keytype is " + keydata.kty + " with length " + keydata.k.length + " and k " + keydata.k);
            })
            .catch(function (err) {
                alert(err);
            });
        })
        .catch(function (err) {
            alert(err);
        });

    })
    .catch(function (err) {
        alert(err);
    });

</script>
</body>
</html>
Ack! "Skip the assignment and proceed to emulate an OOM" is ambiguous. It should say "Emulate an OOM by skipping the assignment and continuing execution".
Flags: sec-bounty?
Flags: needinfo?(rlb)
This uses CryptoBuffer.Assign() instead of the = operator for SetSymKey() to catch OOM situations.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #8660648 - Flags: review?(rlb)
Did the same thing for CryptoKey::SetPrivateKey() and ::SetPublicKey() where SECKEY_CopyPrivateKey() and SECKEY_CopyPublicKey() can fail due to OOM.
Attachment #8660649 - Flags: review?(rlb)
Seems an unlikely and unreliable attack vector, but this could unknowingly affect people leading to weak keys which would be an ongoing problem for them.
Keywords: sec-moderate
Comment on attachment 8660648 [details] [diff] [review]
0001-Bug-1204155-Account-for-OOM-in-CryptoKey-SetSymKey.patch

Hey Martin, would you maybe have time for a look?
Attachment #8660648 - Flags: review?(martin.thomson)
Attachment #8660649 - Flags: review?(martin.thomson)
Attachment #8660648 - Flags: review?(martin.thomson) → review+
Attachment #8660649 - Flags: review?(martin.thomson) → review+
Thank you Martin!
Attachment #8660648 - Flags: review?(rlb)
Attachment #8660649 - Flags: review?(rlb)
Comment on attachment 8660648 [details] [diff] [review]
0001-Bug-1204155-Account-for-OOM-in-CryptoKey-SetSymKey.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Seems an unlikely and unreliable attack vector, but this could unknowingly affect people leading to weak keys which would be an ongoing problem for them.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

From looking at the patch it's obvious we're checking for OOM situations, not much more than that.

Which older supported branches are affected by this flaw?

Everything from ESR 38 to Nightly 44.

If not all supported branches, which bug introduced the flaw?

Bug 995385, i.e. Firefox 32, although pref'ed off at that time.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Same risk, the patches posted here apply perfectly on Aurora and Beta, almost perfectly on Release and ESR 38.

How likely is this patch to cause regressions; how much testing does it need?

Quite unlikely to cause regressions, we also have tests covering that code.
Attachment #8660648 - Flags: sec-approval?
I assume that some versions of FirefoxOS are affected as well, not sure how the versioning scheme there works.
status-firefox41: --- → affected
status-firefox42: --- → affected
status-firefox43: --- → affected
status-firefox44: --- → affected
status-firefox-esr38: --- → affected
Tim, I think that the trick here will be to uplift 3.21 when we have some evidence that it works.  This is the usual process.  We have an NSS call in a few minutes where we will decide what the contents of 3.21 are.  I will advise.

I wouldn't consider a bug of this severity to be worth doing anything exceptional for, but we do generally uplift NSS to beta once we are sure that it's OK.
(In reply to Martin Thomson [:mt:] from comment #11)
> Tim, I think that the trick here will be to uplift 3.21 when we have some
> evidence that it works.  This is the usual process.  We have an NSS call in
> a few minutes where we will decide what the contents of 3.21 are.  I will
> advise.

Those are all just changes in dom/crypto/, not in NSS.

> I wouldn't consider a bug of this severity to be worth doing anything
> exceptional for, but we do generally uplift NSS to beta once we are sure
> that it's OK.

Yeah, I figured it's not a very sensitive bug but I wanted that confirmed :)
Plan is to cut a 3.21 beta early next week.  We will ask for uplift of that once NSS releases, which is usually a week after we land a beta in m-c, assuming that we discover no issues in that time.
Comment on attachment 8660648 [details] [diff] [review]
0001-Bug-1204155-Account-for-OOM-in-CryptoKey-SetSymKey.patch

Sec-moderate bugs don't need sec-approval so I'm clearing the request. https://wiki.mozilla.org/Security/Bug_Approval_Process
Attachment #8660648 - Flags: sec-approval?
(In reply to Al Billings [:abillings] from comment #14)
> Sec-moderate bugs don't need sec-approval so I'm clearing the request.
> https://wiki.mozilla.org/Security/Bug_Approval_Process

Ah, I didn't know that. Sorry for the hassle.
(In reply to Martin Thomson [:mt:] from comment #13)
> Plan is to cut a 3.21 beta early next week.  We will ask for uplift of that
> once NSS releases, which is usually a week after we land a beta in m-c,
> assuming that we discover no issues in that time.

I'm confused, these patches don't depend on NSS, do they? Maybe we're just talking past each other? :)
Gah, I'm sorry, you are right.  I'm getting wires crossed.  If it were up to me, I'd go for uplift to aurora or beta only, they aren't especially dangerous.
https://hg.mozilla.org/mozilla-central/rev/48dfe8c77b2f
https://hg.mozilla.org/mozilla-central/rev/a4bf09cda682
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox44: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Group: dom-core-security → core-security-release
Approval Request Comment
[Feature/regressing bug #]: Introduced with initial WebCrypto API landing
[User impact if declined]: We don't fail (as we should) when copying or setting CryptoKeys fails due to OOM.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: Low risk, covered by tests.
[String/UUID change made/needed]: None.

Squashed patches for Aurora uplift r=mt
Attachment #8669568 - Flags: review+
Attachment #8669568 - Flags: approval-mozilla-aurora?
Approval Request Comment
[Feature/regressing bug #]: Introduced with initial WebCrypto API landing
[User impact if declined]: We don't fail (as we should) when copying or setting CryptoKeys fails due to OOM.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: Low risk, covered by tests.
[String/UUID change made/needed]: None.

Squashed patches for Beta uplift r=mt
Attachment #8669583 - Flags: review+
Attachment #8669583 - Flags: approval-mozilla-beta?
Comment on attachment 8669568 [details] [diff] [review]
0001-Bug-1204155-Account-for-OOM-when-setting-or-copying-.patch

Taking in two branches.
As the patch mostly checks error code, it should be pretty safe. Should be in 42 beta 4.
Attachment #8669568 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8669583 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: sec-bounty? → sec-bounty+
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main42+]
Alias: CVE-2015-7200
Flags: needinfo?(rlb)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: