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)

defect
Not set
critical

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
May be related to/exactly the same as bug 803963, as this was never fixed on our side.
Keywords: crash
Version: unspecified → Trunk
Crash Signature: [@ nsContentList::ContentAppended(nsIDocument *,nsIContent *,nsIContent *,int)]
Keywords: reproducible
See Also: → 803963
Component: General → DOM: Core & HTML
Product: Firefox → Core
Looks like a null-deref while walking the DOM tree?  Let me see if I can reproduce...
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?
I'm using todays Windows nightly. I'm trying again in safe mode.
Ok, no crash in safe mode. Trying to narrow it down.
Got it. Crashes with NoScript. Just installing debugging tools on this machine if you need a better minidump.
It only crashes if BeelineReader.com is blocked by NoScript. So somehow NoScript blocks bookmark access to Beelinereader.com and bookmark crashes ff?!?
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...
Status: UNCONFIRMED → NEW
Ever confirmed: true
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)
Summary: crash @nsContentList::ContentAppended with BeeLine Bookmark → crash @nsContentList::ContentAppended with BeeLine Bookmark and NoScript, due to NoScript violating content policy API constraints
As far as I see, scriptrunner should work, though AttemptToExecute should do it synchronously.
Flags: needinfo?(bugs)
Fwiw, I'm 99% sure this behavior in NoScript is exploitable....
Group: core-security
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.
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)
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.  :(
(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).
Assignee: nobody → g.maone
Keywords: sec-vector
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.
Flags: sec-bounty?
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
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
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-
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.
I filed bug 932508 about moving stuff to a scriptrunner.
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.