READ near NULL while parsing XPath in a XSLT style-sheet

VERIFIED FIXED in Firefox 15

Status

()

Core
XSLT
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Nicolas Grégoire, Assigned: johns)

Tracking

({crash, testcase})

Trunk
mozilla15
crash, testcase
Points:
---

Firefox Tracking Flags

(firefox15 fixed, firefox-esr10- wontfix)

Details

(Whiteboard: [sg:dos][asan])

Attachments

(3 attachments, 6 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 617870 [details]
XSLT test case

An invalid XPath expression like <xsl:template match="key('mykey', " /> inside a XSLT style-sheet will trigger a near-NULL dereference, crashing the browser. This was tested on FF 11 and 12 under Linux and Windows.
(Reporter)

Comment 1

5 years ago
Created attachment 617875 [details]
XML file calling the XSLT one

ASan output:

###!!! ASSERTION: nextToken called beyoned the end: 'mCurrentItem', file /srv/repos/browser/mozilla-central/content/xslt/src/xpath/txExprLexer.cpp, line 78
ASAN:SIGSEGV
==2408== ERROR: AddressSanitizer crashed on unknown address 0x000000000018 (pc 0x7f0e3b042148 sp 0x7fff9dabdd60 bp 0x7fff9dabdeb0 T0)
AddressSanitizer can not provide additional info. ABORTING
Program received signal SIGSEGV, Segmentation fault.

gdb backtrace:

#0  txExprLexer::nextToken (this=0xbf946cb0) at /build/buildd/firefox-11.0+build1/build-tree/mozilla/content/xslt/src/xpath/txExprLexer.cpp:80
#1  0x0155e958 in txPatternParser::createKeyPattern (aLexer=..., aContext=0xa11c0300, aPattern=@0xbf946c2c)
    at /build/buildd/firefox-11.0+build1/build-tree/mozilla/content/xslt/src/xslt/txPatternParser.cpp:279
#2  0x0155eafe in txPatternParser::createLocPathPattern (aLexer=..., aContext=0xa11c0300, aPattern=@0xbf946c7c)
    at /build/buildd/firefox-11.0+build1/build-tree/mozilla/content/xslt/src/xslt/txPatternParser.cpp:173
#3  0x0155ed49 in txPatternParser::createUnionPattern (aLexer=..., aContext=0xa11c0300, aPattern=@0xbf946cc8)
    at /build/buildd/firefox-11.0+build1/build-tree/mozilla/content/xslt/src/xslt/txPatternParser.cpp:80
#4  0x0155ee70 in txPatternParser::createPattern (aPattern=..., aContext=0xa11c0300)
    at /build/buildd/firefox-11.0+build1/build-tree/mozilla/content/xslt/src/xslt/txPatternParser.cpp:59
#5  0x0155307b in getPatternAttr (aAttributes=<value optimized out>, aAttrCount=<value optimized out>, aName=0x1556144, aRequired=<value optimized out>, 
    aState=..., aPattern=...) at /build/buildd/firefox-11.0+build1/build-tree/mozilla/content/xslt/src/xslt/txStylesheetCompileHandlers.cpp:289
#6  0x01556144 in txFnStartTemplate (aNamespaceID=5, aLocalName=0xb3f34c80, aPrefix=0xa18327d0, aAttributes=0xa1788664, aAttrCount=1, aState=...)
    at /build/buildd/firefox-11.0+build1/build-tree/mozilla/content/xslt/src/xslt/txStylesheetCompileHandlers.cpp:1155
#7  0x01559ceb in txStylesheetCompiler::startElementInternal (this=0xa11c0300, aNamespaceID=5, aLocalName=0xb3f34c80, aPrefix=0xa18327d0, 
    aAttributes=0xa1788664, aAttrCount=1, aIDOffset=-1)
    at /build/buildd/firefox-11.0+build1/build-tree/mozilla/content/xslt/src/xslt/txStylesheetCompiler.cpp:327
#8  0x0155a104 in txStylesheetCompiler::startElement (this=0xa11c0300, aName=0xa11c0300, aAttrs=0xa153ef00, aAttrCount=1, aIDOffset=-1)
    at /build/buildd/firefox-11.0+build1/build-tree/mozilla/content/xslt/src/xslt/txStylesheetCompiler.cpp:202
#9  0x01563d9b in txStylesheetSink::HandleStartElement (this=0xa18802e0, aName=0xa11c0800, aAtts=0xa153ef00, aAttsCount=2, aIndex=-1, aLineNumber=2)
    at /build/buildd/firefox-11.0+build1/build-tree/mozilla/content/xslt/src/xslt/txMozillaStylesheetCompiler.cpp:159
#10 0x011cefcb in nsExpatDriver::HandleStartElement (this=0xa16cda60, aValue=0xa11c0800, aAtts=0xa153ef00)
    at /build/buildd/firefox-11.0+build1/build-tree/mozilla/parser/htmlparser/src/nsExpatDriver.cpp:411
Confirmed on OS X with nightly.

Crash is bp-f50f5322-88bc-4e64-b678-f87bf2120424.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: x86 → All
Version: 12 Branch → Trunk
Note: the crash does not require ASan builds.

This looks like a constant offset from null and not exploitable.
Keywords: crash, testcase
Summary: [ASan] READ near NULL while parsing XPath in a XSLT style-sheet → READ near NULL while parsing XPath in a XSLT style-sheet
Whiteboard: [sg:dos][asan]
John, any chance you could have a go at this one? Seems like it should be pretty simple and you did a great job on the other XSLT bug.
Assignee: nobody → jschoenick
(Assignee)

Comment 5

5 years ago
Created attachment 618498 [details] [diff] [review]
Don't proceed past the end token, add more checks for uninitialized parser usage.
Attachment #618498 - Flags: review?(jonas)
(Assignee)

Comment 6

5 years ago
(hit enter too soon)

The problem is createKeyPattern is jumping forward twice, and nextToken() reads off the end of the list into null. The only way mCurrentItem can be null is if we read past the list, or if we haven't parsed anything yet.

This changes it to not read past the end node, which is what seems like we intended - all the logic in txPatternParser doesn't look for overflows, but does watch for end nodes.

Also adds a few more assertions to catch uninitialized parser usage elsewhere.
Status: NEW → ASSIGNED
(Assignee)

Comment 7

5 years ago
Created attachment 618501 [details] [diff] [review]
Don't proceed past the end token, add more checks for uninitialized parser usage.

This version fixes pushBack() to make more sense. This would break logic that expects to read two nodes past the end, and then pushbacks once, but nothing appears to do this (nor should it expect it to work).
Attachment #618498 - Attachment is obsolete: true
Attachment #618498 - Flags: review?(jonas)
Attachment #618501 - Flags: review?(jonas)
(Assignee)

Comment 8

5 years ago
Created attachment 618502 [details] [diff] [review]
Don't proceed past the end token, add more checks for uninitialized parser usage.

Bugzilla really should complain when you upload the same exact file twice.
Attachment #618501 - Attachment is obsolete: true
Attachment #618501 - Flags: review?(jonas)
Attachment #618502 - Flags: review?(jonas)
(Assignee)

Comment 9

5 years ago
Comment on attachment 618502 [details] [diff] [review]
Don't proceed past the end token, add more checks for uninitialized parser usage.

Or not:

https://tbpl.mozilla.org/?tree=Try&rev=67129c59be0b

So clearly logic is expecting to push past the end node, I'll have to take a closer look at this tomorrow.
Attachment #618502 - Flags: review?(jonas)
IIRC the intent was to never read past the end token. We could certainly add an extra level of security and return end tokens no matter how far past the end you read, but given that the worst that can happen is a crash it might not be worth it.
(Assignee)

Comment 11

5 years ago
Created attachment 618869 [details] [diff] [review]
[2/2] Remove pushBack() from txExprLexer, change users to peek(), add peekAhead() for one case
(Assignee)

Comment 12

5 years ago
Created attachment 618871 [details] [diff] [review]
[1/2] Don't progress past end in nextToken, add a few more sanity checks.
(Assignee)

Updated

5 years ago
Attachment #618502 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #618871 - Attachment description: Bug 748365 - Don't progress past end in nextToken, add a few more sanity checks. → [1/2] Don't progress past end in nextToken, add a few more sanity checks.
(Assignee)

Updated

5 years ago
Attachment #618869 - Attachment description: Bug 748365 - Remove pushBack() from txExprLexer, change users to peek(), add peekAhead() for one case → [2/2] Remove pushBack() from txExprLexer, change users to peek(), add peekAhead() for one case
(Assignee)

Comment 13

5 years ago
Created attachment 618883 [details] [diff] [review]
[2/2] Remove pushBack() from txExprLexer, change users to peek(), add peekAhead() for one case

So the intent was clearly not to read past the end node - nothing does proper null checks or checks for mType == Token::END before doing nextToken(). Enforcing this in nextToken, however, breaks pushBack() - if nextToken() might be a no-op, pushBack() might back us up further than where we started.

pushBack() also requires the lexer list to be doubly linked. So let's just remove it. I changed all use cases of the form |nextToken() -> Process ? ... : pushBack()| to |peek() -> Process ? nextToken() && ...| and added peekAhead() for one case where we need to look two nodes ahead.

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=fb769a499d87
Attachment #618869 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #618871 - Flags: review?(jonas)
(Assignee)

Comment 14

5 years ago
Comment on attachment 618883 [details] [diff] [review]
[2/2] Remove pushBack() from txExprLexer, change users to peek(), add peekAhead() for one case

These look good on try
Attachment #618883 - Flags: review?(jonas)
Attachment #618883 - Flags: review?(jonas) → review+
Attachment #618871 - Flags: review?(jonas) → review+
The second patch is a pretty scary change though. Especially given how little tests we have checked in for XSLT :(

So I don't think we'll want to take this on branches, which might be ok since this doesn't appear to be exploitable.
Dan, given that this doesn't appear exploitable, just a DoS, can we remove the security flag?
(Assignee)

Updated

5 years ago
Attachment #618871 - Flags: checkin?(jonas)
(Assignee)

Updated

5 years ago
Attachment #618883 - Flags: checkin?(jonas)
(Assignee)

Comment 17

5 years ago
(In reply to Jonas Sicking (:sicking) from comment #15)
> The second patch is a pretty scary change though. Especially given how
> little tests we have checked in for XSLT :(
> 
> So I don't think we'll want to take this on branches, which might be ok
> since this doesn't appear to be exploitable.

A good deal of tests managed to explode on try without the pushback fix, but this could certainly stand to ride the trains.

This should probably be tracker esr10 for 15+ in that case
status-firefox-esr10: --- → affected
tracking-firefox-esr10: --- → ?
(Assignee)

Comment 18

5 years ago
Can I bug you land this? I lack commit access.
This is only an sg:dos bug and risky, so we've chosen to wontfix this for the ESR.
status-firefox-esr10: affected → wontfix
tracking-firefox-esr10: ? → -
(Assignee)

Updated

5 years ago
Attachment #618883 - Attachment is obsolete: true
Attachment #618883 - Flags: checkin?(jonas)
(Assignee)

Updated

5 years ago
Attachment #618871 - Attachment is obsolete: true
Attachment #618871 - Flags: checkin?(jonas)
(Assignee)

Comment 20

5 years ago
Created attachment 620912 [details] [diff] [review]
xpath: Don't progress past end in nextToken, remove pushBack() logic. r=sicking

Folded + rebased on recent central
https://hg.mozilla.org/integration/mozilla-inbound/rev/baf3b13a912e

Updated

5 years ago
Attachment #620912 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/baf3b13a912e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox15: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Verified fixed in 5/9/12 nightly build. No longer crashes as it does with Firefox 12.
Status: RESOLVED → VERIFIED
Group: core-security
You need to log in before you can comment on or make changes to this bug.