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

Status: VERIFIED FIXED

(Core :: XSLT, defect)

Description

[Nicolas Grégoire]

Attachment: 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.

Comment 1

[Nicolas Grégoire]

Attachment: 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

Comment 2

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

Confirmed on OS X with nightly.

Crash is bp-f50f5322-88bc-4e64-b678-f87bf2120424.

Comment 3

[Daniel Veditz [:dveditz]]

Note: the crash does not require ASan builds.

This looks like a constant offset from null and not exploitable.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 5

[John Schoenick [:johns]]

Attachment: Don't proceed past the end token, add more checks for uninitialized parser usage.

Comment 6

[John Schoenick [:johns]]

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

Comment 7

[John Schoenick [:johns]]

Attachment: 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).

Comment 8

[John Schoenick [:johns]]

Attachment: 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.

Comment 9

[John Schoenick [:johns]]

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.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 11

[John Schoenick [:johns]]

Attachment: [2/2] Remove pushBack() from txExprLexer, change users to peek(), add peekAhead() for one case

Comment 12

[John Schoenick [:johns]]

Attachment: [1/2] Don't progress past end in nextToken, add a few more sanity checks.

Comment 13

[John Schoenick [:johns]]

Attachment: [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

Comment 14

[John Schoenick [:johns]]

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

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Dan, given that this doesn't appear exploitable, just a DoS, can we remove the security flag?

Comment 17

[John Schoenick [:johns]]

(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

Comment 18

[John Schoenick [:johns]]

Can I bug you land this? I lack commit access.

Comment 19

[Alex Keybl [:akeybl]]

This is only an sg:dos bug and risky, so we've chosen to wontfix this for the ESR.

Comment 20

[John Schoenick [:johns]]

Attachment: xpath: Don't progress past end in nextToken, remove pushBack() logic. r=sicking

Folded + rebased on recent central

Comment 21

[Johnny Stenback (:jst)]

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

Comment 22

[Ed Morley [:emorley]]

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

Comment 23

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

Verified fixed in 5/9/12 nightly build. No longer crashes as it does with Firefox 12.