1411458 - type confusion in VerifyCMSDetachedSignatureIncludingCertificate (potential RCE in parent process)

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

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

Attachment: poc.xpi

VerifyCMSDetachedSignatureIncludingCertificate:
...

  UniqueNSSCMSMessage
    cmsMsg(NSS_CMSMessage_CreateFromDER(const_cast<SECItem*>(&buffer), nullptr,
                                        nullptr, nullptr, nullptr, nullptr,
                                        nullptr));
  if (!cmsMsg) {
    return NS_ERROR_CMS_VERIFY_ERROR_PROCESSING;
  }

A:if (!NSS_CMSMessage_IsSigned(cmsMsg.get())) {
    return NS_ERROR_CMS_VERIFY_NOT_SIGNED;
  }

B:NSSCMSContentInfo* cinfo = NSS_CMSMessage_ContentLevel(cmsMsg.get(), 0);
  if (!cinfo) {
    return NS_ERROR_CMS_VERIFY_NO_CONTENT_INFO;
  }

  // signedData is non-owning
C:NSSCMSSignedData* signedData =
    static_cast<NSSCMSSignedData*>(NSS_CMSContentInfo_GetContent(cinfo));

NSS_CMSContentInfo_GetContent is a poorly-designed API. It returns a void* and the caller is supposed to know what to cast it to (at C). VerifyCMSDetachedSignatureIncludingCertificate tries to ensure that the given message is a signedData message by calling NSS_CMSMessage_IsSigned (at A), but this checks if any message in the encapsulation hierarchy is a signedData, not just the 0th one (which is what we get at B).

The attached xpi crashes Firefox (on 64-bit linux, at least) if you open it. The "signature" in it consists of a digestedData message encapsulating a signedData message. NSS_CMSMessage_IsSigned returns true, NSS_CMSMessage_ContentLevel returns the digestedData content, and so the data at C is not a NSSCMSSignedData. When it later gets used, bad things happen.
I believe this has been incorrect since it was implemented in bug 896620.

[Tracking Requested - why for this release]: sec-critical

Comment 1

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

Attachment: 1411458-check-cms-type.diff

Comment 2

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

Attachment: 1411458-test.diff

Comment 3

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

Comment on attachment 8922062 [details] [diff] [review]
1411458-check-cms-type.diff

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

Simple enough. r=jcj

Comment 4

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

Comment on attachment 8922064 [details] [diff] [review]
1411458-test.diff

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

::: security/manager/ssl/tests/unit/test_signed_apps.js
@@ +223,5 @@
>  });
>  
> +add_test(function () {
> +  certdb.openSignedAppFileAsync(
> +    Ci.nsIX509CertDB.AppXPCShellRoot, original_app_path("bug_1411458"),

This is fine, but you forgot to include the poc.xpi into that folder in this commit.

Comment 5

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

Thanks!
The test file is there as test_signed_apps/bug_1411458.zip - splinter is just not good at showing binary files in patches.

Comment 6

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

Comment on attachment 8922062 [details] [diff] [review]
1411458-check-cms-type.diff

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Relatively easily, I think.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Tests are in another patch. Unfortunately the fix and the nature of the problem kind-of paint a bulls-eye on the issue.

Which older supported branches are affected by this flaw?
All of them.

If not all supported branches, which bug introduced the flaw?
n/a, but bug 896620 introduced this code.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This patch seems to apply fine to all branches.

How likely is this patch to cause regressions; how much testing does it need?
Unlikely - it only introduces a check we should have been doing all along. We have reasonably good test coverage in-tree.

Comment 7

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8922062 [details] [diff] [review]
1411458-check-cms-type.diff

sec-approval+.
We'll want this on Beta and ESR52 so please nominate the patch ASAP. We have a beta build tomorrow and we want it in it if possible.

Comment 8

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

Comment on attachment 8922062 [details] [diff] [review]
1411458-check-cms-type.diff

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-crit
User impact if declined: remote code execution
Fix Landed on Version: not landed yet
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: bug 896620
[User impact if declined]: remote code execution
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not landed yet in nightly
[Needs manual test from QE? If yes, steps to reproduce]: might be a good idea: just download and open "poc.xpi" (attachment 8921687 [details]). It won't crash the browser if the fix worked.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: it just adds a check that should have already been present
[String changes made/needed]: none

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8922062 [details] [diff] [review]
1411458-check-cms-type.diff

Sec-crit, beta57+, ESR52.5+

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2bdce9b76f2135fd100f073b6ccaf8bb9a69fed

Comment 11

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/15663d3f8728 for

Mac: https://treeherder.mozilla.org/logviewer.html#?job_id=139693664&repo=mozilla-inbound
Windows: https://treeherder.mozilla.org/logviewer.html#?job_id=139693682&repo=mozilla-inbound
Android: https://treeherder.mozilla.org/logviewer.html#?job_id=139693662&repo=mozilla-inbound

build bustage.

Comment 12

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

Attachment: 1411458-check-cms-type.diff

This updates the original diff to add the missing symbol to nss.symbols, which should solve the issue from the backout. I won't obsolete the old patch because I'm not totally sure what the procedure is here.

Comment 13

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

Comment on attachment 8922299 [details] [diff] [review]
1411458-check-cms-type.diff

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

D'oh. Thanks.

Comment 14

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

Carrying over various approvals for bustage fix. Please check in attachment 8922299 [details] [diff] [review]. Thanks!

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/22a772cfa62ae6908751b1cdd98a9234f2ea6105

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c060b92db7ee
https://hg.mozilla.org/releases/mozilla-esr52/rev/3b899f872623

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/22a772cfa62a

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2aba0fc325a
add a test for bug 1411458 r=jcj

Comment 19

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/e2aba0fc325a