606729 - "ASSERTION: Not safe to run a parser-inserted script?" with <script async>

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: Not safe to run a parser-inserted script?: 'nsContentUtils::IsSafeToRunScript()', file content/base/src/nsScriptLoader.cpp, line 667

###!!! ASSERTION: Processing requests when running scripts is unsafe.: 'nsContentUtils::IsSafeToRunScript()', file content/base/src/nsScriptLoader.cpp, line 675

###!!! ASSERTION: This is unsafe! Fix the caller!: 'Error', file content/events/src/nsEventDispatcher.cpp, line 514

Comment 1

[Jesse Ruderman]

Attachment: assertion stacks

Comment 2

[Henri Sivonen (:hsivonen)]

Conflicting optimizations.

Comment 3

[Henri Sivonen (:hsivonen)]

(In reply to comment #2)
> Conflicting optimizations.

Actually not.

The problem is that the input "data:" causes NS_NewURI to fail to create an object. Thus, the URL of the script is null. Thus, the script loader thinks it has an inline script but the parser thinks it has created an external script.

Comment 4

[Henri Sivonen (:hsivonen)]

Attachment: WIP fix

This patch fixes the bug, but I want to add more test coverage before I request review. I also want to see if the other browsers fire the 'error' event in this case.

Comment 5

[Johnny Stenback (:jst)]

Henri, do you think this is a sg:critical bug or one that's not as bad?

Comment 6

[Henri Sivonen (:hsivonen)]

Attachment: Use a flag for script externalness separate from mUri being null

(In reply to comment #5)
> Henri, do you think this is a sg:critical bug or one that's not as bad?

I don't know what badness can come from running a script from within an update batch, but since I don't know, I think it would make sense to treat this as potentially serious for 4.0 and land the fix sooner than later. Note that <script src="data:">alert("foo");</script> alerts in Firefox 3.6.x, but in 3.6.x it doesn't cause the script to run at a prohibited point, so this isn't sg:critical for 3.6.x.

I tested onerror and onload in some browsers. Safari and Chrome fire onerror. IE9 beta 1 and Opera 10.63 (and Firefox 3.6.x for src="bogus:") don't fire either onerror or onload, so I went with the easier (engine-wise) majority behavior of not firing events here.

The patch applies on top of the fix for bug 604660, but that one is a beta7 blocker anyway.

Comment 7

[Henri Sivonen (:hsivonen)]

(In reply to comment #6)
> I tested onerror and onload in some browsers. Safari and Chrome fire onerror.
> IE9 beta 1 and Opera 10.63 (and Firefox 3.6.x for src="bogus:") don't fire
> either onerror or onload, so I went with the easier (engine-wise) majority
> behavior of not firing events here.

FWIW, HTML5 specifies the WebKit event firing behavior here. Regardless, I think we shouldn't do that as part of this bug. Instead, I think firing onerror should either be a follow-up non-security bug, since being HTML5-compliant with bogus URLs would require firing onerror in more cases than just the one being addressed here. Alternatively, we could push for a spec change on the grounds that the spec upholds minority behavior (though the WebKit behavior is kinda nice here).

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We should assume that running script inside an update batch can lead to exploitable crashes.

Comment 9

[Henri Sivonen (:hsivonen)]

Requesting blocker status to get an approval to land this and OTOH to make sure this gets landed.

When landing this, is it OK to use the commit message from the attached patch? After all, anyone who cares to look can figure out what the patch does from the code changes and the crashtest.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think that description is fine yes.

Comment 11

[Henri Sivonen (:hsivonen)]

Regarding the status whiteboard: I believe this isn't a security bug at all in Firefox 3.6. There's just a plain bug there: <script src="data:">foo();</script> runs foo(); there but in a way that's safe from the Gecko POV.

Comment 12

[Johnny Stenback (:jst)]

Blocking, let's get this landed.

Comment 13

[Henri Sivonen (:hsivonen)]

http://hg.mozilla.org/mozilla-central/rev/d6d9cb57b170