Closed Bug 436965 Opened 12 years ago Closed 11 years ago

<iframe src="c:"></iframe> triggers "ASSERTION: This is unsafe"

Categories

(Core :: Layout: Images, Video, and HTML Frames, defect, P1)

x86
macOS
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: smaug)

References

(Blocks 1 open bug, )

Details

(4 keywords, Whiteboard: [sg:critical] post 1.8-branch)

Attachments

(3 files, 3 obsolete files)

Attached file testcase
Loading the testcase on one of my 10.5 machines triggers:

###!!! ASSERTION: This is unsafe: 'nsContentUtils::IsSafeToRunScript()', file /Users/jruderman/central/layout/base/nsDocumentViewer.cpp, line 1054

###!!! ASSERTION: Someone forgot to block scripts: 'aIsSafeToFlush == nsContentUtils::IsSafeToRunScript()', file /Users/jruderman/central/layout/base/nsPresShell.cpp, line 4505

###!!! ASSERTION: How did we get here if it's not safe to run scripts?: 'nsContentUtils::IsSafeToRunScript()', file /Users/jruderman/central/layout/base/nsPresShell.cpp, line 5556
Assignee: nobody → jonas
Flags: wanted1.9.1+
Priority: -- → P2
Version: unspecified → Trunk
The problem is that we're putting up the silly dialog that says we can't handle the scheme... and we're doing it with this stack:

#39 0x042a61b0 in nsPromptService::Alert (this=0xb5dd160, parent=0xd756120, dialogTitle=0xd790c60, text=0xb5e8f68) at /Users/bzbarsky/mozilla/profile/mozilla/embedding/components/windowwatcher/src/nsPromptService.cpp:143
#40 0x0428e956 in nsPrompt::Alert (this=0xd10c3e0, dialogTitle=0x0, text=0xb5e8f68) at /Users/bzbarsky/mozilla/profile/mozilla/embedding/components/windowwatcher/src/nsPrompt.cpp:198
#41 0x03408154 in nsDocShell::DisplayLoadError (this=0x8627a0, aError=2152398866, aURI=0x673d3e0, aURL=0x0, aFailedChannel=0x0) at /Users/bzbarsky/mozilla/profile/mozilla/docshell/base/nsDocShell.cpp:3227
#42 0x034112b5 in nsDocShell::InternalLoad (this=0x8627a0, aURI=0x673d3e0, aReferrer=0xd26e3d0, aOwner=0xd28fbf0, aFlags=0, aWindowTarget=0xd2bda60, aTypeHint=0x0, aPostData=0x0, aHeadersData=0x0, aLoadType=1, aSHEntry=0x0, aFirstParty=0, aDocShell=0x0, aRequest=0x0) at /Users/bzbarsky/mozilla/profile/mozilla/docshell/base/nsDocShell.cpp:7272
#43 0x033f9ca1 in nsDocShell::LoadURI (this=0x8627a0, aURI=0x673d3e0, aLoadInfo=0xc5b0880, aLoadFlags=0, aFirstParty=0) at /Users/bzbarsky/mozilla/profile/mozilla/docshell/base/nsDocShell.cpp:932
#44 0x01b41b80 in nsFrameLoader::ReallyStartLoading (this=0xd213b60) at /Users/bzbarsky/mozilla/profile/mozilla/content/base/src/nsFrameLoader.cpp:218
#45 0x01b2a67c in nsDocument::InitializeFrameLoader (this=0x1550800, aLoader=0xd213b60) at /Users/bzbarsky/mozilla/profile/mozilla/content/base/src/nsDocument.cpp:5082
#46 0x01b4167b in nsFrameLoader::LoadURI (this=0xd213b60, aURI=0x673d3e0) at /Users/bzbarsky/mozilla/profile/mozilla/content/base/src/nsFrameLoader.cpp:183
#47 0x01b40252 in nsFrameLoader::LoadFrame (this=0xd213b60) at /Users/bzbarsky/mozilla/profile/mozilla/content/base/src/nsFrameLoader.cpp:164
#48 0x01c10d93 in nsGenericHTMLFrameElement::LoadSrc (this=0xd15da30) at /Users/bzbarsky/mozilla/profile/mozilla/content/html/content/src/nsGenericHTMLElement.cpp:2833
#49 0x01c10f22 in nsGenericHTMLFrameElement::BindToTree (this=0xd15da30, aDocument=0x1550800, aParent=0xb58e8f0, aBindingParent=0x0, aCompileEventHandlers=1) at /Users/bzbarsky/mozilla/profile/mozilla/content/html/content/src/nsGenericHTMLElement.cpp:2856
#50 0x01b55fa0 in nsGenericElement::doInsertChildAt (aKid=0xd15da30, aIndex=0, aNotify=0, aParent=0xb58e8f0, aDocument=0x1550800, aChildArray=@0xb58e908) at /Users/bzbarsky/mozilla/profile/mozilla/content/base/src/nsGenericElement.cpp:3252

so totally inside an update.

The right thing to do is probably to put off the load until after the update is over.  That's generally a good thing to do, because a load start can run chrome JS no matter what, by sending necko notifications.

Alternately, the posing of the error dialog could be async.  Not sure which is better.
Group: core-security
So..  This can cause us to run script in the middle of frame construction, which is easily followed by a grisly death and certainly exploitable.  See bug 459710 for an example of the former.
Blocks: 459710
Flags: blocking1.9.0.4?
OK, so we are NOT in fact inside an update yet, since we're just inside BindToTree from the sink...
And in bug 459710 similarly.

Smaug, could we make frameloader init always async?  Or at least do it off a scriptrunner or some such?
Whiteboard: [sg:critical]
(In reply to comment #3)
> OK, so we are NOT in fact inside an update yet, since we're just inside
> BindToTree from the sink...
Um, we're not in update when calls are coming from sink. Evil.
Well, technically updates are for the mutation observer notifications.

The more I think about this, the more I think that doing the load sync under BindToTree is a bad idea and that that's what should happen either async or at end of update or at end of script blocking or something...  I'm not sure the sink puts in a script blocker either, fwiw.
(In reply to comment #6)
> The more I think about this, the more I think that doing the load sync under
> BindToTree is a bad idea and that that's what should happen either async or at
> end of update
Loading happens at the end of update, if there is update.
Async may change DOM event ordering etc. too much - even moving loading to happen
after update caused bad regressions.
Flags: blocking1.9.0.4? → blocking1.9.0.5?
Given the short cycle for 1.9.0.5 this probably has to wait.
Flags: blocking1.9.0.5? → wanted1.9.0.x+
I can take this. Unless you Jonas have already something ready for this.
Assignee: jonas → Olli.Pettay
(In reply to comment #6)
> The more I think about this, the more I think that doing the load sync under
> BindToTree is a bad idea and that that's what should happen either async or at
> end of update or at end of script blocking or something...  I'm not sure the
> sink puts in a script blocker either, fwiw.
Sync load in BindToTree is bad, sure. I've tried to make async+after update 
working, but no luck yet. The ordering of stuff gets tricky.

Sink doesn't seem to push any script blockers.
Feel free to add scriptblocker anyplace you think is needed. The current set is by no means exhaustive.
Adding scriptblockers around every bindtotree in the sink might be kinda nice, but are we really happy doing the load as soon as we return to the sink?
Bumping priority and marking a blocker, we should fix this security bug for 1.9.1. Smaug, are you able to continue working on this? If not, please let me know.
Flags: blocking1.9.1+
Priority: P2 → P1
Duplicate of this bug: 459710
(In reply to comment #14)
> Smaug, are you able to continue working on this?
Yeah, still working on this.
Note, (X)HTML is the easy part here, but making XUL and XBL work nicely...
I do have a patch which should pass all the mochi/chrome/browser-chrome tests, 
but I think the tests don't cover dynamic overlays and it seems we don't have
good tests for <frame> elements - most of the tests use <iframe>s.

The idea is anyway to delay frame loading (when there are no document updates).
Asynchronous loading isn't possible, that breaks lots of testcases.
What things fail if we delay loading? Is there any chance that we can fix those longer term?

Is the problem only that we're putting up the dialog? If so, maybe we can just ensure that the dialog is coming up async?
Attached patch WIP, v4 (obsolete) — Splinter Review
note, this is just a wip. Approach v4.
(In reply to comment #18)
> What things fail if we delay loading? Is there any chance that we can fix those
> longer term?
about:blank, for example, must load ASAP. Otherwise all sort of cases,
where iframe.contentDocument is expected to be available, will fail.

I need to verify this fixes all the things I want.
Running mochitest asserts way too much about script blockers - hard to say
which assertions are because of this bug and which are because of something else.

Would be great to get the patches for bug 430214 in soon. That might reduce
the noise at least a bit.
(In reply to comment #18)
> Is the problem only that we're putting up the dialog? If so, maybe we can just
> ensure that the dialog is coming up async?
That might be ok for some cases, but *maybe* not enough if there is |load| event listener for the frame.
Does the about:blank really need to start loading ASAP?  Or just to have an document viewer ASAP?
The load event must be fired for about:blank asap, so no, I don't think
having just document viewer is enough.
I think for <iframe> loading can be delayed up to DoneAddingChildren, but
because <frame> is a leaf, loading can't be delayed much longer than what
the patch does.
No longer blocks: 466057
Comment on attachment 349526 [details] [diff] [review]
WIP, v4

Not, good. This causes new assertions.
Attachment #349526 - Attachment is obsolete: true
I think bug 444224 must be fixed first to reduce assertion noise.
If bug 466057 is fixed, the patch may make sense after all.
Blocks: 467422
Depends on: 466057
Comment on attachment 349526 [details] [diff] [review]
WIP, v4

This bug really needs bug 466057 to be fixed first.
Attachment #349526 - Attachment is obsolete: false
Whiteboard: [sg:critical] → [sg:critical][has wip that depends on 466057]
Smaug, looks like bug 466057 is fixed now, can you make more progress here now, or are there other things blocking this as well?
Yes, I'll re-test the patch and ask reviews if I still think it is ok.
Comment on attachment 349526 [details] [diff] [review]
WIP, v4

this patch doesn't pass all the tests anymore :(
Attachment #349526 - Attachment is obsolete: true
(In reply to comment #31)
> (From update of attachment 349526 [details] [diff] [review])
> this patch doesn't pass all the tests anymore :(
Something to do with Bug 345339. I'll look at the problem tomorrow.
Blocks: 430800
Attached patch v5 (obsolete) — Splinter Review
This fixes also bug 430800.
Attachment #354689 - Flags: review?(jonas)
Whiteboard: [sg:critical][has wip that depends on 466057] → [sg:critical][needs review]
Comment on attachment 354689 [details] [diff] [review]
v5

> nsDocument::InitializeFrameLoader(nsFrameLoader* aLoader)
> nsDocument::FinalizeFrameLoader(nsFrameLoader* aLoader)
> nsDocument::InitializeFinalizeFrameLoaders()

Do we need these functions at all? Wouldn't it be cleaner to simply issue a script runner for each frame that needs to be initialized/finalized that just deals with that one initialization/finalization.


>diff --git a/layout/base/nsCSSFrameConstructor.cpp b/layout/base/nsCSSFrameConstructor.cpp
>@@ -13559,16 +13559,17 @@ nsCSSFrameConstructor::LazyGenerateChild
>     nsMenuPopupFrame* menuPopupFrame = static_cast<nsMenuPopupFrame *>(frame);
>     if (menuPopupFrame->HasGeneratedChildren()) {
>       if (mCallback)
>         mCallback(mContent, frame, mArg);
>       
>       return NS_OK;
>     }     
> 
>+    nsAutoScriptBlocker scriptBlocker;
>     // indicate that the children have been generated
>     menuPopupFrame->SetGeneratedChildren();
> #endif

Seems scary to have a scriptblocker that's #ifdef MOZ_XUL, but whose lifetime spans more than the ifdef.

Also, it currently is alive while ProcessAttachedQueue is called further down which is bad since ProcessAttachedQueues purpose is to run scripts.

>diff --git a/layout/base/nsPresShell.cpp b/layout/base/nsPresShell.cpp
>--- a/layout/base/nsPresShell.cpp
>+++ b/layout/base/nsPresShell.cpp
>@@ -2403,17 +2403,20 @@ PresShell::InitialReflow(nscoord aWidth,
>     // Run the XBL binding constructors for any new frames we've constructed
>     mDocument->BindingManager()->ProcessAttachedQueue();
> 
>     // Constructors may have killed us too
>     NS_ENSURE_STATE(!mHaveShutDown);
> 
>     // Now flush out pending restyles before we actually reflow, in
>     // case XBL constructors changed styles somewhere.
>-    mFrameConstructor->ProcessPendingRestyles();
>+    {
>+      nsAutoScriptBlocker scriptBlocker;
>+      mFrameConstructor->ProcessPendingRestyles();
>+    }

Why is this needed? Not sure what ProcessPendingRestyles ends up doing so mostly asking out of curiosity.
(same at the other callsites).

Does it bind the anonymous content which schedules the iframe scriptrunners?

Minusing based on the nsCSSFrameConstructor::LazyGenerateChild change.
Attachment #354689 - Flags: review?(jonas) → review-
(In reply to comment #34)
> (From update of attachment 354689 [details] [diff] [review])
> > nsDocument::InitializeFrameLoader(nsFrameLoader* aLoader)
> > nsDocument::FinalizeFrameLoader(nsFrameLoader* aLoader)
> > nsDocument::InitializeFinalizeFrameLoaders()
Unfortunately document (or something) needs to keep track on to-be-initialized
frameloaders, because docshell may want to cancel initialization or
docshell may want to ask if some frameloader is scheduled to be finalized.

> Seems scary to have a scriptblocker that's #ifdef MOZ_XUL, but whose lifetime
> spans more than the ifdef.
Oops.

> 
> Also, it currently is alive while ProcessAttachedQueue is called further down
> which is bad since ProcessAttachedQueues purpose is to run scripts.
Ah, right.
 
> Does it bind the anonymous content which schedules the iframe scriptrunners?
For example that yes.
(In reply to comment #35)
> (In reply to comment #34)
> > (From update of attachment 354689 [details] [diff] [review] [details])
> > > nsDocument::InitializeFrameLoader(nsFrameLoader* aLoader)
> > > nsDocument::FinalizeFrameLoader(nsFrameLoader* aLoader)
> > > nsDocument::InitializeFinalizeFrameLoaders()
> Unfortunately document (or something) needs to keep track on to-be-initialized
> frameloaders, because docshell may want to cancel initialization or
> docshell may want to ask if some frameloader is scheduled to be finalized.

The to-be-initialized information could be kept in the iframes docshell (for example by making it point to the scriptrunner), that'd also simplify the cancel-pending-initialization code.

Not sure about the schedule-to-be-finalized part though, though that looks messy anyway.

Anyway, I'm fine with leaving this as is given the complexity of rewriting, but it'd be nice to do at some point perhaps.
Attached patch v6 (obsolete) — Splinter Review
Attachment #354689 - Attachment is obsolete: true
Attachment #354960 - Flags: review?(jonas)
Comment on attachment 354960 [details] [diff] [review]
v6

Wanna add an assertion to ProcessPendingRestyles that checks that we're inside a scriptblocker?
Attachment #354960 - Flags: superreview+
Attachment #354960 - Flags: review?(jonas)
Attachment #354960 - Flags: review+
Attached patch with assertionSplinter Review
Attachment #354960 - Attachment is obsolete: true
Backed out because of some OSX pixel scrolling failures (which I can't reproduce locally on OSX nor on Linux).
The test failure has occurred also before. Going to re-land.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical][needs review] → [sg:critical]
Keywords: fixed1.9.1
Depends on: 472502
I assume this did land on 1.9.1?

Also, can we land this on 1.9.0?  That would be really nice for bug 453736.
Yes, this landed 1.9.1. (there is fixed1.9.1). I forgot the hg link.
This depends on bug 466057, so if we take this to 1.9.0, also that one is needed.
Uh, yeah. Olli, let's get both this bug and bug 466057 on the 1.9.0 branch. Can you work up a couple patches (or a combined one, whichever is better/easier)?
Flags: blocking1.9.0.7+
Attached patch 1.9.0 patchSplinter Review
No functional changes comparing to the 1.9.1/trunk patch.
Attachment #360071 - Flags: approval1.9.0.7?
Comment on attachment 360071 [details] [diff] [review]
1.9.0 patch

Approved for 1.9.0.7, a=dveditz for release-drivers.
Attachment #360071 - Flags: approval1.9.0.7? → approval1.9.0.7+
Keywords: fixed1.9.0.7
Flags: wanted1.8.1.x-
Whiteboard: [sg:critical] → [sg:critical] post 1.8-branch
Tomcat, this is another of those where I need a debug build in order to use the testcase. Can you verify this fix on OS X 10.5 with debug 1.9.0.7 nightly?
verified 1.9.0.7 using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.8pre) Gecko/2009021822 Firefox/3.0.8pre - have not seen this assertion on the testcase --> verified
Group: core-security
Product: Core → Core Graveyard
Component: Layout: HTML Frames → Layout: Images
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.