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.
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.
Note: the crash does not require ASan builds. This looks like a constant offset from null and not exploitable.
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.
Created attachment 618498 [details] [diff] [review] Don't proceed past the end token, add more checks for uninitialized parser usage.
(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.
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).
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.
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.
Created attachment 618869 [details] [diff] [review] [2/2] Remove pushBack() from txExprLexer, change users to peek(), add peekAhead() for one case
Created attachment 618871 [details] [diff] [review] [1/2] Don't progress past end in nextToken, add a few more sanity checks.
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
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
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
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.
Created attachment 620912 [details] [diff] [review] xpath: Don't progress past end in nextToken, remove pushBack() logic. r=sicking Folded + rebased on recent central
Verified fixed in 5/9/12 nightly build. No longer crashes as it does with Firefox 12.