1336836 - (CVE-2017-5424) NULL deref at nsNCRFallbackEncoderWrapper::Encode during XPCOM shutdown after XSLT processing

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Nicolas Grégoire]

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

Comment 1

[Nicolas Grégoire]

Attachment: XPCshell PoC

Comment 2

[Jesse Schwartzentruber (:truber)]

Attachment: testcase.html

Comment 3

[Jesse Schwartzentruber (:truber)]

Confirmed in mozilla-central rev 1cc159c7a044.

Comment 4

[Jesse Schwartzentruber (:truber)]

Attachment: tcmin.html

Comment 5

[Jesse Schwartzentruber (:truber)]

Doesn't seem to be XSLT related at all.

Comment 6

[Daniel Veditz [:dveditz]]

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.

Comment 7

[Tyson Smith [:tsmith]]

Attachment: debug_log.txt

Comment 8

[Valentin Gosu [:valentin] (he/him)]

I guess the crash just needs an extra null check.
The assertion fails because we assume do_CreateInstance will always succeed.

Comment 9

[Henri Sivonen (:hsivonen) (temporarily away from Bugzilla)]

Attachment: Add null check

(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.

Comment 10

[Henri Sivonen (:hsivonen) (temporarily away from Bugzilla)]

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.

Comment 11

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

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.

Comment 12

[Henri Sivonen (:hsivonen) (temporarily away from Bugzilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1bcbd44a34a82c9c96b8e651c867fc94f1690f82
Bug 1336836 - Null-check mEncoder for XPCOM shutdown. r=emk.

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/1bcbd44a34a8

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora/Beta approval on this when you get a chance.

Comment 15

[Henri Sivonen (:hsivonen) (temporarily away from Bugzilla)]

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

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 8837607 [details] [diff] [review]
Add null check

esr52 is synced from beta, no need for separate approval.

Comment 17

[Gerry Chang [:gchang]]

Comment on attachment 8837607 [details] [diff] [review]
Add null check

Fix a security issue. Aurora53+.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/661e35b83224

Comment 19

[Julien Cristau [:jcristau]]

Comment on attachment 8837607 [details] [diff] [review]
Add null check

add null check, beta52+

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7084f0d7a43e

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/7084f0d7a43e

Comment 22

[Daniel Veditz [:dveditz]]

does not meet the qualifications for the security bug bounty.