Closed Bug 1336836 (CVE-2017-5424) Opened 3 years ago Closed 3 years ago

NULL deref at nsNCRFallbackEncoderWrapper::Encode during XPCOM shutdown after XSLT processing

Categories

(Core :: Internationalization, defect, critical)

44 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox-esr45 --- wontfix
firefox51 --- wontfix
firefox52 + fixed
firefox-esr52 52+ fixed
firefox53 + fixed
firefox54 + fixed

People

(Reporter: nicolas.gregoire, Assigned: hsivonen)

References

Details

(4 keywords, Whiteboard: [post-critsmash-triage][adv-main52-])

Attachments

(4 files, 1 obsolete file)

Not sure which component this problem belongs...
I set the "security" flag even if this is just a NULL deref, because it happens during mozilla::ShutdownXPCOM() which is surprising.

Tested under xpcshell "JavaScript-C54.0a1"

[+] Relevant parts

xsl_str = '<xsl:stylesheet xmlns:xsl="http://www.w3.org/1999/XSL/Transform" version="1.0">' +
          '  <xsl:output method="html" encoding="iso-8859-1"/>' +
          '  <xsl:template match="/">' +
          '    <xsl:variable name="url">http://aaa?bbb\xffccc</xsl:variable>' +
          '    <xsl:value-of select="document($url)"/>' +
          '    <base href="{$url}"/>' +
          '  </xsl:template>' +
          '</xsl:stylesheet>'

[+] How to reproduce

$ ./poc_xslt_NULL_nsNCRFallbackEncoderWrapper_Encode.js
ASAN:DEADLYSIGNAL
=================================================================
==20365==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fd3865b7da3 bp 0x7fff2c6e3150 sp 0x7fff2c6e2fe0 T0)
    #0 0x7fd3865b7da2 in nsNCRFallbackEncoderWrapper::Encode(nsAString_internal const&, nsACString_internal&) /home/worker/workspace/build/src/intl/uconv/nsNCRFallbackEncoderWrapper.cpp:59:19
    #1 0x7fd38672c565 in mozilla::net::nsStandardURL::nsSegmentEncoder::EncodeSegmentCount(char const*, mozilla::net::nsStandardURL::URLSegment const&, short, nsCString&, bool&, unsigned int) /home/worker/workspace/build/src/netwerk/base/nsStandardURL.cpp:215:21
    #2 0x7fd386730743 in mozilla::net::nsStandardURL::BuildNormalizedSpec(char const*) /home/worker/workspace/build/src/netwerk/base/nsStandardURL.cpp:743:30
    #3 0x7fd38673e770 in mozilla::net::nsStandardURL::SetSpec(nsACString_internal const&) /home/worker/workspace/build/src/netwerk/base/nsStandardURL.cpp:1554:14
    #4 0x7fd386758ae3 in mozilla::net::nsStandardURL::Init(unsigned int, int, nsACString_internal const&, char const*, nsIURI*) /home/worker/workspace/build/src/netwerk/base/nsStandardURL.cpp:3260:16
    #5 0x7fd386e284d7 in NewURI /home/worker/workspace/build/src/netwerk/protocol/http/nsHttpHandler.cpp:125:19
    #6 0x7fd386e284d7 in mozilla::net::nsHttpHandler::NewURI(nsACString_internal const&, char const*, nsIURI*, nsIURI**) /home/worker/workspace/build/src/netwerk/protocol/http/nsHttpHandler.cpp:1943
    #7 0x7fd38668e497 in mozilla::net::nsIOService::NewURI(nsACString_internal const&, char const*, nsIURI*, nsIURI**) /home/worker/workspace/build/src/netwerk/base/nsIOService.cpp:649:12
    #8 0x7fd3866be339 in NS_NewURI(nsIURI**, nsACString_internal const&, char const*, nsIURI*, nsIIOService*) /home/worker/workspace/build/src/netwerk/base/nsNetUtilInlines.h:115:14
    #9 0x7fd3866be5b7 in NS_NewURI(nsIURI**, nsAString_internal const&, char const*, nsIURI*, nsIIOService*) /home/worker/workspace/build/src/netwerk/base/nsNetUtilInlines.h:126:12
    #10 0x7fd38b04c4ca in mozilla::dom::SetBaseURIUsingFirstBaseWithHref(nsIDocument*, nsIContent*) /home/worker/workspace/build/src/dom/html/HTMLSharedElement.cpp:176:7
    #11 0x7fd38b04d229 in mozilla::dom::HTMLSharedElement::UnbindFromTree(bool, bool) /home/worker/workspace/build/src/dom/html/HTMLSharedElement.cpp:318:7
    #12 0x7fd388e8d80e in nsDocument::cycleCollection::Unlink(void*) /home/worker/workspace/build/src/dom/base/nsDocument.cpp:1895:5
    #13 0x7fd38b0efa0d in nsHTMLDocument::cycleCollection::Unlink(void*) /home/worker/workspace/build/src/dom/html/nsHTMLDocument.cpp:194:1
    #14 0x7fd3863aa0df in nsCycleCollector::CollectWhite() /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:3331:5
    #15 0x7fd3863ad468 in nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool) /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:3673:24
    #16 0x7fd3863ad0c4 in nsCycleCollector::ShutdownCollect() /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:3591:10
    #17 0x7fd3863b0e8a in Shutdown /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:3882:5
    #18 0x7fd3863b0e8a in nsCycleCollector_shutdown(bool) /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:4200
    #19 0x7fd386545715 in mozilla::ShutdownXPCOM(nsIServiceManager*) /home/worker/workspace/build/src/xpcom/build/XPCOMInit.cpp:1006:3
    #20 0x7fd387c9658b in XRE_XPCShellMain(int, char**, char**, XREShellData const*) /home/worker/workspace/build/src/js/xpconnect/src/XPCShellImpl.cpp:1631:10
    #21 0x4df071 in main /home/worker/workspace/build/src/js/xpconnect/shell/xpcshell.cpp:68:18
    #22 0x7fd3811c182f in __libc_start_main /build/glibc-t3gR2i/glibc-2.23/csu/../csu/libc-start.c:291
    #23 0x41b988 in _start (/work/firefox/mc-asan/xpcshell+0x41b988)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /home/worker/workspace/build/src/intl/uconv/nsNCRFallbackEncoderWrapper.cpp:59:19 in nsNCRFallbackEncoderWrapper::Encode(nsAString_internal const&, nsACString_internal&)
==20365==ABORTING
Attached file XPCshell PoC
Group: core-security → dom-core-security
Attached file testcase.html (obsolete) —
Confirmed in mozilla-central rev 1cc159c7a044.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: in-testsuite?
Version: 54 Branch → Trunk
Attached file tcmin.html
Attachment #8834049 - Attachment is obsolete: true
Doesn't seem to be XSLT related at all.
Component: XSLT → Networking
OS: Linux → Unspecified
Hardware: x86_64 → Unspecified
Flags: sec-bounty?
Valentin: are URL parser crashes your area?

As a null deref this doesn't look exploitable, but given how that code works (and that it's now calling ICU) it makes me nervous.
Flags: needinfo?(valentin.gosu)
Keywords: sec-low
Attached file debug_log.txt
I guess the crash just needs an extra null check.
The assertion fails because we assume do_CreateInstance will always succeed.
Flags: needinfo?(valentin.gosu) → needinfo?(hsivonen)
Flags: needinfo?(hsivonen)
Attached patch Add null checkSplinter Review
(In reply to Daniel Veditz [:dveditz] from comment #6)
> (and that it's now calling ICU)

Why do multiple people now talk as though we'd have switched / were about to switch to ICU for character encoding conversion? I sure hope we haven't because I'm working on replacing uconv with encoding_rs, not ICU.

(In reply to Valentin Gosu [:valentin] from comment #8)
> I guess the crash just needs an extra null check.

Yes.

> The assertion fails because we assume do_CreateInstance will always succeed.

It's so annoying that we keep running code that does encoding conversions after uconv has been unregistered for shutdown. Fortunately, this code will be replaced with non-XPCOM Rust (bug 1261841) in due course.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Attachment #8837607 - Flags: review?(VYV03354)
Attachment #8837607 - Flags: review?(VYV03354) → review+
Comment on attachment 8837607 [details] [diff] [review]
Add null check

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

It's unclear whether there is something exploitable here. In any case, the problem only shows up at XPCOM shutdown.

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

Yes.

> Which older supported branches are affected by this flaw?

All.

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

Bug 1202366.

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

The same patch applies.

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

It's just a null check. Extremely unlikely to cause regressions.
Attachment #8837607 - Flags: sec-approval?
Comment on attachment 8837607 [details] [diff] [review]
Add null check

This is a sec-low null deref. You only need sec-approval for critical or high rated issues that affect multiple branches. Clearing the sec-approval request.
Attachment #8837607 - Flags: sec-approval?
https://hg.mozilla.org/mozilla-central/rev/1bcbd44a34a8
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Please request Aurora/Beta approval on this when you get a chance.
Blocks: 1202366
Flags: needinfo?(hsivonen)
Version: Trunk → 44 Branch
Comment on attachment 8837607 [details] [diff] [review]
Add null check

[Approval Request Comment]
> If this is not a sec:{high,crit} bug, please state case for ESR consideration:

Trivial patch that makes Firefox behave better under ASAN.

> User impact if declined: 

Probably none.

> Fix Landed on Version:

54

> Risk to taking this patch (and alternatives if risky): 

Extremely low risk.

> 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 1202366

> [User impact if declined]:

Probably none.

> [Is this code covered by automated tests?]:

No.

> [Has the fix been verified in Nightly?]:

Landed uneventfully on Nightly.

> [Needs manual test from QE? If yes, steps to reproduce]: 

See comment 0

> [List of other uplifts needed for the feature/fix]:

None

> [Is the change risky?]:

No

> [Why is the change risky/not risky?]:

Trivial null check that only affects shutdown time.

> [String changes made/needed]:

None
Flags: needinfo?(hsivonen)
Attachment #8837607 - Flags: approval-mozilla-esr52?
Attachment #8837607 - Flags: approval-mozilla-beta?
Attachment #8837607 - Flags: approval-mozilla-aurora?
Comment on attachment 8837607 [details] [diff] [review]
Add null check

esr52 is synced from beta, no need for separate approval.
Attachment #8837607 - Flags: approval-mozilla-esr52?
Comment on attachment 8837607 [details] [diff] [review]
Add null check

Fix a security issue. Aurora53+.
Attachment #8837607 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8837607 [details] [diff] [review]
Add null check

add null check, beta52+
Attachment #8837607 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Component: Networking → Internationalization
does not meet the qualifications for the security bug bounty.
Flags: sec-bounty? → sec-bounty-
Group: dom-core-security → core-security-release
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main52+]
Alias: CVE-2017-5424
Whiteboard: [post-critsmash-triage][adv-main52+] → [post-critsmash-triage][adv-main52-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.