1338277 - NULL deref [@ txExpandedNameMap_base::removeItem]

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: log.txt

Marking as s-s for now. There have been some UAFs found in XSLT recently and we just got a fuzzer up and running.

==7358==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f14bde55996 bp 0x7fff2b029dc0 sp 0x7fff2b029d90 T0)
    #0 0x7f14bde55995 in Hdr /home/worker/workspace/build/src/obj-firefox/dist/include/nsTArray.h:642:32
    #1 0x7f14bde55995 in Elements /home/worker/workspace/build/src/obj-firefox/dist/include/nsTArray.h:1150
    #2 0x7f14bde55995 in IndexOf<txExpandedName, txMapItemComparator> /home/worker/workspace/build/src/obj-firefox/dist/include/nsTArray.h:1273
    #3 0x7f14bde55995 in txExpandedNameMap_base::removeItem(txExpandedName const&) /home/worker/workspace/build/src/dom/xslt/base/txExpandedNameMap.cpp:97
    #4 0x7f14bdec1859 in remove /home/worker/workspace/build/src/dom/xslt/base/txExpandedNameMap.h:126:20
    #5 0x7f14bdec1859 in removeVariable /home/worker/workspace/build/src/dom/xslt/xslt/txVariableMap.h:74
    #6 0x7f14bdec1859 in removeVariable /home/worker/workspace/build/src/dom/xslt/xslt/txExecutionState.cpp:518
    #7 0x7f14bdec1859 in txRemoveVariable::execute(txExecutionState&) /home/worker/workspace/build/src/dom/xslt/xslt/txInstructions.cpp:737
    #8 0x7f14bdf1c23d in txXSLTProcessor::execute(txExecutionState&) /home/worker/workspace/build/src/dom/xslt/xslt/txXSLTProcessor.cpp:49:14
    #9 0x7f14bdee5108 in txMozillaXSLTProcessor::TransformToDoc(nsIDOMDocument**, bool) /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:676:14
    #10 0x7f14bdeeb2ab in TransformToDocument /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:645:12
...
see log.txt

Comment 1

[Tyson Smith [:tsmith]]

Attachment: test_case.html

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

The obvious null check here isn't enough, since other scarier looking crashes happen with that.
peterv and pike might remember some of this code.

Comment 3

[Tyson Smith [:tsmith]]

Should the sec-rating be changed?

Comment 4

[Axel Hecht [:Pike]]

Also Jonas. I have to admit, I don't even recall what all those xslt stylesheet things are supposed to do in theory, let alone the code that implements them.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Regression range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9a8f2342fb3116d23989087e026448d38a3768c5&tochange=fc706d376f0658e560a59c3dd520437b18e8c4a4

I see Bindings-related stuff on the stack, so maybe bug 1039986?

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Here's a stack from a current nightly:
https://crash-stats.mozilla.com/report/index/8c54dcfb-faa6-43b1-9556-c4f622170210

Comment 7

[Boris Zbarsky [:bzbarsky]]

Bindings stuff on the stack just means "yeah, we got called from JS"....

This is a crash in the guts of the XSLT stuff, not really bindings related per se.  Looking in that range, we have changes to XSLT code from bug 1191645.  But I don't think we're doing any actual URL-fetch stylesheet loading that would have been affected by that.

It also looks like on current Mac I can't run those year-old nightlies at all.  They crash on startup.  :(

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Local bisection says this was caused by bug 932517.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bb8446a6d8d

Comment 9

[Boris Zbarsky [:bzbarsky]]

Not caused.  Testcase just uses "let" in a non-strict no-version script, so ends up throwing errors before that changeset.  Change the "let" to "var" in the testcase and I expect you will reproduce with earlier builds....

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Well, that was a fun night of slow builds down the drain.

New regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c3c4c902e9cd&tochange=31879b88cc82

Comment 11

[Peter Van der Beken [:peterv]]

This is probably related to the issue I mentioned in bug 1336832 comment 7, we need to propagate errors from matching instead of blindly continuing executing. In this case we try to call txExecutionState::removeVariable even though we skipped txExecutionState::bindVariable due to a matching error.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 52+, wontfix for 51.

Comment 13

[Julien Cristau [:jcristau]]

Looks like a wontfix for 52 as well now.

Comment 14

[Peter Van der Beken [:peterv]]

Attachment: Branch patch v1

This is a spot fix we should take on branches, the underlying issue would be fixed by the trunk patch in bug 1336828.

Comment 15

[Eric Rahm [:erahm]]

Comment on attachment 8840869 [details] [diff] [review]
Branch patch v1

Review of attachment 8840869 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 16

[Peter Van der Beken [:peterv]]

Comment on attachment 8840869 [details] [diff] [review]
Branch patch v1

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not easily I think.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The testcase does.

Which older supported branches are affected by this flaw?

All.

If not all supported branches, which bug introduced the flaw?

All branches are affected.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Should just apply to branches.

How likely is this patch to cause regressions; how much testing does it need?

Shouldn't cause regressions, it just adds a null-check before dereferencing a pointer.

Note that this is a lower-risk patch intended for branches. I have a different, much bigger change for trunk (in bug 1336828). Please let me know if it's ok to not land the branch patch on trunk, otherwise we could land this on trunk too, and then backout and land the riskier change from bug 1336828 only on trunk.

Comment 17

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

Comment on attachment 8840869 [details] [diff] [review]
Branch patch v1

Sec-approval is only necessary for sec-high and sec-critical rated issues that affect more than trunk. Since this is not high or critical, I'm clearing. You can just check in.

Comment 18

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/421407ec02c07437ced56c950b03c74e6faea56b
Bug 1338277 - Deal better with failed matching for variables, r=erahm.

Comment 19

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

This was backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b5d6fe8ac8fef915a078629fcf5984c666b31d82 for failing the crashtest it added.

Comment 20

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ddbfa3adcfaded3f220333bdf632edde5c42872a
Bug 1338277 - Deal better with failed matching for variables, r=erahm.

Comment 21

[Carsten Book [:Tomcat]]

backed out for failing on own crashtest in https://hg.mozilla.org/integration/mozilla-inbound/rev/227e9f482196f7068f0e17f83f4436ab8770233c

Comment 22

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=60a7eb26b5ca

Comment 23

[Peter Van der Beken [:peterv]]

I think we should just fix this with the patch from bug 1349717. After annotating the asserts the testcase still leaks too with just this patch.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Per comment 23.