Closed Bug 606729 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: DOM: HTML Parser, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: jruderman, Assigned: hsivonen)

Details

(Keywords: assertion, testcase, Whiteboard: [sg:critical?] less bad for 3.6?)

Attachments

(3 files, 1 obsolete file)

Attached file 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
Attached file assertion stacks
Conflicting optimizations.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
(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.
Attached patch WIP fix (obsolete) — Splinter Review
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.
Henri, do you think this is a sg:critical bug or one that's not as bad?
(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.
Attachment #486049 - Attachment is obsolete: true
Attachment #486298 - Flags: review?(jonas)
(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).
We should assume that running script inside an update batch can lead to exploitable crashes.
Whiteboard: [sg:critical?] less bad for 3.6?
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.
blocking2.0: --- → ?
I think that description is fine yes.
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.
Blocking, let's get this landed.
blocking2.0: ? → final+
http://hg.mozilla.org/mozilla-central/rev/d6d9cb57b170
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Group: core-security
You need to log in before you can comment on or make changes to this bug.