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.
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
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
(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
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).
Bugzilla really should complain when you upload the same exact file twice.
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.
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.
Attachment #618502 - Attachment is obsolete: true
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.
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
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
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?
(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: --- → ?
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: ? → -
Folded + rebased on recent central
Status: ASSIGNED → RESOLVED
Last Resolved: 7 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
You need to log in before you can comment on or make changes to this bug.