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

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: jruderman, Assigned: hsivonen)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
x86
Mac OS X
assertion, testcase
Points:
---

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [sg:critical?] less bad for 3.6?)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
Created attachment 485549 [details]
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
(Reporter)

Comment 1

8 years ago
Created attachment 485550 [details]
assertion stacks
(Assignee)

Comment 2

8 years ago
Conflicting optimizations.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
(Assignee)

Comment 3

8 years ago
(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.
(Assignee)

Comment 4

8 years ago
Created attachment 486049 [details] [diff] [review]
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.
Henri, do you think this is a sg:critical bug or one that's not as bad?
(Assignee)

Comment 6

8 years ago
Created attachment 486298 [details] [diff] [review]
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.
Attachment #486049 - Attachment is obsolete: true
Attachment #486298 - Flags: review?(jonas)
(Assignee)

Comment 7

8 years ago
(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?
(Assignee)

Comment 9

8 years ago
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.
(Assignee)

Comment 11

8 years ago
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+
(Assignee)

Comment 13

8 years ago
http://hg.mozilla.org/mozilla-central/rev/d6d9cb57b170
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Group: core-security
You need to log in before you can comment on or make changes to this bug.