1033492 - WebCrypto: heap-buffer-overflow crash [@PK11_EnterSlotMonitor]

Status: VERIFIED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: testcase

Tested with http://hg.mozilla.org/integration/mozilla-inbound/rev/4ffff7bdb4c8a3ddb3f4d356385350b9bd3125d4

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 2

[Richard Barnes [:rbarnes]]

Note that the test case only needs the generateKey call, not the rest.

After a brief look, my hypothesis is that it's effectively trying to free an unallocated object:

(1) GenerateAsymmetricKeyPairTask::GenerateAsymmetricKeyPairTask() is not permitting a key to be generated, but doesn't fail properly.  (The combination of "sign" and "RSAES-PKCS1-v1_5" is invalid.)

(2) As a result, no key pair is actually generated.

(3) ReleaseNSSResources() still tries to free the key objects, even though they don't exist.

I'll try to take a look at this tonight.

Comment 3

[Richard Barnes [:rbarnes]]

Attachment: bug-1033492.patch

Turns out it was a failure to initialize a pointer to nullptr before using nullptr as a signal of it not having been assigned.  But I went ahead and fixed the usage issue while I was there.

The test case now behaves correctly, failing with DataError due to the incorrect "usage" value provided.

Comment 4

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

Comment on attachment 8449916 [details] [diff] [review]
bug-1033492.patch

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

Unless I'm misunderstanding the key usages changes you're making, I think AddUsageIntersecting should be where that gets handled.
In any case, maybe that should be handled in a separate bug. r=me for 'SECKEYPublicKey* pubKey = nullptr;'
Also, we should make sure that kind of thing isn't happening elsewhere in this code.
Additionally, the testcase should be added to the test suite. However, since this code is in Aurora, there's a process so we don't 0-day ourselves, in case you weren't familiar with it already: https://wiki.mozilla.org/Security/Bug_Approval_Process

::: dom/crypto/Key.h
@@ +112,5 @@
>    void SetExtractable(bool aExtractable);
>    void SetAlgorithm(KeyAlgorithm* aAlgorithm);
> +
> +  // Usage-related utilities
> +  static nsresult StringToUsage(const nsString& aUsage, 

nit: trailing space

::: dom/crypto/WebCryptoTask.cpp
@@ +1352,5 @@
> +      Key::KeyUsage usage;
> +      mEarlyRv = Key::StringToUsage(aKeyUsages[i], usage);
> +      if (NS_FAILED(mEarlyRv)) {
> +        return;
> +      } else if (!(usage & (publicAllowedUsages | privateAllowedUsages))) {

Shouldn't AddUsageIntersecting handle this and return an error?

Comment 5

[Richard Barnes [:rbarnes]]

Attachment: bug-1033492.patch r=keeler

This patch has only the nullptr assignment.

Comment 6

[Richard Barnes [:rbarnes]]

Comment on attachment 8450690 [details] [diff] [review]
bug-1033492.patch r=keeler

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

  Unknown

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

  It's a one-line patch, and the check-in comment indicate that there is a crash.

Which older supported branches are affected by this flaw?

  Aurora 32

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

  998471

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

  Included patch should also apply to Aurora.

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

  Unlikely.

Comment 7

[Daniel Veditz [:dveditz]]

Comment on attachment 8450690 [details] [diff] [review]
bug-1033492.patch r=keeler

sec-approval+ and aurora a+ = dveditz

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c59adf6c245

Can we land the testcase too?

Comment 9

[Richard Barnes [:rbarnes]]

Attachment: bug-1033492.1.patch r=keeler

Added test case.

Comment 10

[Richard Barnes [:rbarnes]]

Attachment: bug-1033492.2.patch

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4c59adf6c245

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Test: https://hg.mozilla.org/integration/mozilla-inbound/rev/2c7546553188

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c3683c06f6d8

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2c7546553188

Comment 15

[Kamil Jozwiak [:kjozwiak]]

Reproduced the original issue several times using the following build:
- http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1404320841/

fx34: [PASSED]
- http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1408701721/

fx33: [PASSED]
- http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-aurora-linux64-asan/1408739215/

fx32: [PASSED]
- http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-beta-linux64-asan/1408738736/

Went through the following test cases for each of the above channels:

- opened the poc in 10 different tabs and windows
- opened the poc in 10 different private browser tabs and windows
- opened the poc in 10 different e10s tabs and windows (m-c only)