Closed Bug 1411458 Opened 2 years ago Closed 2 years ago

type confusion in VerifyCMSDetachedSignatureIncludingCertificate (potential RCE in parent process)

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 57+ fixed
firefox56 --- wontfix
firefox57 + fixed
firefox58 + fixed

People

(Reporter: keeler, Assigned: keeler)

Details

(Keywords: sec-critical, Whiteboard: [psm-assigned][adv-main57+][adv-esr52.5+][post-critsmash-triage])

Attachments

(4 files)

Attached file 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 on attachment 8922062 [details] [diff] [review]
1411458-check-cms-type.diff

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

Simple enough. r=jcj
Attachment #8922062 - Flags: review?(jjones) → review+
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.
Attachment #8922064 - Flags: review?(jjones) → review+
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 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.
Attachment #8922062 - Flags: sec-approval?
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.
Attachment #8922062 - Flags: sec-approval? → sec-approval+
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
Attachment #8922062 - Flags: approval-mozilla-esr52?
Attachment #8922062 - Flags: approval-mozilla-beta?
Comment on attachment 8922062 [details] [diff] [review]
1411458-check-cms-type.diff

Sec-crit, beta57+, ESR52.5+
Attachment #8922062 - Flags: approval-mozilla-esr52?
Attachment #8922062 - Flags: approval-mozilla-esr52+
Attachment #8922062 - Flags: approval-mozilla-beta?
Attachment #8922062 - Flags: approval-mozilla-beta+
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.
Attachment #8922299 - Flags: review?(dkeeler)
Comment on attachment 8922299 [details] [diff] [review]
1411458-check-cms-type.diff

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

D'oh. Thanks.
Attachment #8922299 - Flags: review?(dkeeler) → review+
Carrying over various approvals for bustage fix. Please check in attachment 8922299 [details] [diff] [review]. Thanks!
Keywords: checkin-needed
Group: core-security
https://hg.mozilla.org/mozilla-central/rev/22a772cfa62a
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Group: crypto-core-security → core-security-release
Whiteboard: [psm-assigned] → [psm-assigned][adv-main57+][adv-esr52.5+]
Flags: qe-verify-
Whiteboard: [psm-assigned][adv-main57+][adv-esr52.5+] → [psm-assigned][adv-main57+][adv-esr52.5+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.