Closed
Bug 924423
Opened 11 years ago
Closed 11 years ago
crash @nsContentList::ContentAppended with BeeLine Bookmark and NoScript, due to NoScript's forced bookmarklet execution using sync XHR in a nsIContentPolicy::shouldLoad() callback
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox-esr24 | --- | unaffected |
b2g18 | --- | unaffected |
b2g-v1.1hd | --- | unaffected |
b2g-v1.2 | --- | unaffected |
People
(Reporter: johnp, Assigned: ma1)
References
()
Details
(Keywords: crash, reproducible, sec-vector)
Crash Data
1. Go to http://www.beelinereader.com/install 2. Bookmark one of the options 3. Clicking it crashes Firefox Crash report: e9e6cc1b-f13b-4eae-95d2-9f2be2131008
Reporter | ||
Comment 1•11 years ago
|
||
May be related to/exactly the same as bug 803963, as this was never fixed on our side.
Reporter | ||
Updated•11 years ago
|
Reporter | ||
Updated•11 years ago
|
Crash Signature: [@ nsContentList::ContentAppended(nsIDocument *,nsIContent *,nsIContent *,int)]
Keywords: reproducible
See Also: → 803963
Reporter | ||
Updated•11 years ago
|
Component: General → DOM: Core & HTML
Product: Firefox → Core
Comment 2•11 years ago
|
||
Looks like a null-deref while walking the DOM tree? Let me see if I can reproduce...
Comment 3•11 years ago
|
||
I can't reproduce over here, in a current-ish Mac nightly. Which exact build are you reproducing this in? Does it happen in safe mode?
Reporter | ||
Comment 4•11 years ago
|
||
I'm using todays Windows nightly. I'm trying again in safe mode.
Reporter | ||
Comment 5•11 years ago
|
||
Ok, no crash in safe mode. Trying to narrow it down.
Reporter | ||
Comment 6•11 years ago
|
||
Got it. Crashes with NoScript. Just installing debugging tools on this machine if you need a better minidump.
Reporter | ||
Comment 7•11 years ago
|
||
It only crashes if BeelineReader.com is blocked by NoScript. So somehow NoScript blocks bookmark access to Beelinereader.com and bookmark crashes ff?!?
Comment 8•11 years ago
|
||
Thanks. Now I can reproduce. So what happens here is that the <body> gets an appendChild call. The child being appended is a <script> element. Then we land in nsContentList::ContentAppended with aFirstNewContent being the <script> and aContainer being the <body>, but aFirstNewContent->GetParent() is null. This is not supposed to be possible, and the code in ContentAppended doesn't expect this situation. It walks the subtree like so: 783 for (nsIContent* cur = aFirstNewContent; 784 cur; 785 cur = cur->GetNextNode(aContainer)) { which is supposed to be safe as long as aContainer is in fact on the parent chain of aFirstNewContent...
Updated•11 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 9•11 years ago
|
||
So what happens here is that mozilla::dom::HTMLScriptElement::BindToTree calls nsScriptElement::MaybeProcessScript which calls, eventually, nsScriptLoader::ShouldLoadScript, which checks content policies. The noscript content policy implementation seems to do a sync XHR(!) which leads us to several assertions (e.g. because event dispatch happens while under BindToTree here), and worse yet while the event loop is still spinning we run a javascript: URI from readability stuff which ends up removing the <script> in question from the DOM. Then the stack unwinds out of BindToTree and we notify the append, but the node has already been removed from the DOM, so the DOM is in an inconsistent state and we crash. Olli, Jonas, is it reasonable to do MaybeProcessScript off a scriptrunner to work around this noscript bug? Giorgio, doing sync XHR inside a content policy ShouldLoad/ShouldProcess implementation is not OK. Was that not clear from the comments in the API? If so, we should fix the comments...
Flags: needinfo?(jonas)
Flags: needinfo?(g.maone)
Flags: needinfo?(bugs)
Updated•11 years ago
|
Summary: crash @nsContentList::ContentAppended with BeeLine Bookmark → crash @nsContentList::ContentAppended with BeeLine Bookmark and NoScript, due to NoScript violating content policy API constraints
Comment 10•11 years ago
|
||
As far as I see, scriptrunner should work, though AttemptToExecute should do it synchronously.
Flags: needinfo?(bugs)
Comment 11•11 years ago
|
||
Fwiw, I'm 99% sure this behavior in NoScript is exploitable....
Group: core-security
Comment 12•11 years ago
|
||
And indeed, this is possibly the same as bug 803963, which was fixed by making the addon to use the API the way it should be used.
Assignee | ||
Comment 13•11 years ago
|
||
FWIW, NoScript perform this hack *exclusively* when the user tries to execute a bookmarklet on a not-whitelisted page (which gets temporarily enabled to run Javascript, but just until the synchronous completion of the bookmarklet), and only if said bookmarklet actually tries to include some further script, which would otherwise be loaded asynchronously (after Javascript has been disabled back). Hence I'm not sure it's so easy to exploit, and exploiting it requires some social engineering anyway. At any rate I'm investigating ways to obtain the same result (i.e. executing complex bookmarklets with nested scripts on non-whitelisted pages) by other means, and if this is considered critical I can ship a NoScript version which does not support this feature anymore in the meanwhile, even though it would be quite annoying for users.
Flags: needinfo?(g.maone)
Comment 14•11 years ago
|
||
Hmm. So the sync XHR case is limited to <script> tags? If so, just moving those to a scriptrunner might be enough.... I'm not sure about AttemptToExecute. Maybe in practice that can't happen in situations when it's unsafe to run scripts? But yes, we definitely need to do stuff sync there, since we need to determine the parser-blocking stuff sync. :(
Assignee | ||
Comment 15•11 years ago
|
||
(In reply to Boris Zbarsky [:bz] (Vacation Oct 12 - Oct 27) from comment #14) > Hmm. So the sync XHR case is limited to <script> tags? Yes it is. It just emulates <script> synchronously (even firing a synthetic load event).
Updated•11 years ago
|
Assignee: nobody → g.maone
Keywords: sec-vector
Flags: needinfo?(jonas)
I'd be fine with moving MaybeProcessScript to a scriptrunner. Though it would only improve the situation for <script> and not for a pile of other types of loads. Though sounds like the NoScript bug only happens for <script>. Of course, I'd be much happier if NoScript fixed its problem here since doing sync XHR loads during content policy checks definitely is not ok. We can work around it right now, but there's no guarantee that the workaround will stay working.
Updated•11 years ago
|
Flags: sec-bounty?
Assignee | ||
Comment 17•11 years ago
|
||
It should be fixed by NoScript 2.6.8.3rc3, https://addons.mozilla.org/en-US/firefox/addon/noscript/versions/?page=1#version-2.6.8.3rc3 Please verify, and I'll immediately submit 2.6.8.3 in the stable AMO channel and ask for an expedite review. Thank you.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•11 years ago
|
Summary: crash @nsContentList::ContentAppended with BeeLine Bookmark and NoScript, due to NoScript violating content policy API constraints → crash @nsContentList::ContentAppended with BeeLine Bookmark and NoScript, due to NoScript's forced bookmarklet execution using sync XHR in a nsIContentPolicy::shouldLoad() callback
Comment 18•11 years ago
|
||
Thank you for reporting this to help keep NoScript users safe. Unfortunately the Mozilla security bug bounty does not cover bugs in 3rd party add-ons.
Flags: sec-bounty? → sec-bounty-
Comment 19•11 years ago
|
||
Cleaning up list of security bugs for b2g18. This bug doesn't need to be backported either due to it affecting a later version of Fx or another reason.
status-b2g18:
--- → unaffected
Comment 20•11 years ago
|
||
I filed bug 932508 about moving stuff to a scriptrunner.
Updated•11 years ago
|
status-b2g-v1.1hd:
--- → unaffected
status-b2g-v1.2:
--- → unaffected
status-firefox-esr24:
--- → unaffected
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•