Closed Bug 1281787 Opened 3 years ago Closed 3 years ago

foo.com can access view-source:blob:http://foo.com/<uuid> for valid blob URIs (but not view-source:http://foo.com/ )

Categories

(Core :: Security: CAPS, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

(Keywords: csectype-other, sec-low, Whiteboard: [post-critsmash-triage][adv-main50-])

Attachments

(1 file, 1 obsolete file)

So here I was trying to write a good test for bug 1277583... I think I tried a little too hard. I don't know to what degree this is exploitable - I haven't tried - I just find it very surprising, and it seems to violate my(/our?) ideas about what DANGEROUS_TO_LOAD restrictions on schemes do and don't prevent.

STR:

1. open http://www.mozilla.org/
2. open the web console
3. run:

w = window.open("view-source:http://www.mozilla.org/", "_blank");

(this will fail, which is correct and expected.)

4. run:

u = URL.createObjectURL(new Blob(["this is a blob"]));
w = window.open("view-source:" + u, "_blank");

this opens a new tab (or window) with the view-source URI, and the content window has access to the result of "w", can read innerHTML etc.

AFAICT this won't work if the inner blob URI is invalid or belongs to a different domain. Small mercies. But even so, I don't think it should work at all. I'll leave nested view-source:blob stuff out of the test in bug 1277583 for now.
See Also: → 1277583
Group: core-security → dom-core-security
(In reply to Daniel Veditz [:dveditz] from comment #1)
> I assume you've disabled the popup blocker, or disabled it for
> www.mozilla.org -- I couldn't run your STR without doing that.

Oh, um, yes, probably as a side-effect of testing some other bug, yes. Sorry!


> ---(1)---
> If I open http://www.mozilla.org/ I get redirected to
> http_S_://www.mozilla.org/
> 
> window.open("view-source:http://www.mozilla.org") is therefore cross-origin
> and gives me
>   Error: Access to 'view-source:http://www.mozilla.org/' from script denied
> 
> However, same-origin window.open("view-source:https://www.mozilla.org/")
> works.
> 
> 
> ---(2)---
> if I add a link to the page, "view-source:"+same-origin URL will open (e.g.
> "view-source:https://www.mozilla.org"). A cross-origin view-source link will
> fail.
> 
> 
> -------
> 
> We've got a bug here but it's not related to "blob:"

This is surprising to me. On https, and with view-source:https://..., I still see:

Error: Access to 'view-source:https://www.mozilla.org/' from script denied

using:
window.open("view-source:" + location.href, "_blank");

(but the same happens without "_blank".)

What build are you testing on? I checked both 48 beta and nightly. Both produce this error. I just checked a clean profile on Nightly, and that shows this error and no popup blocker warning for the JS above, but shows the popup blocker when I try with a blob: URI.

In fact, one thing that might explain this: is security.view-source.reachable-from-inner-protocol perhaps set to true in the build/profile you're using to test with?
Flags: needinfo?(dveditz)
D'oh! yes, I had set that pref at some point.
Flags: needinfo?(dveditz)
Summary: same-origin view-source: URLs are not blocked → foo.com can access view-source:blob:http://foo.com/<uuid> for valid blob URIs (but not view-source:http://foo.com/ )
This is incredibly interesting to me, is view-source: still privileged in some way or does it trigger chrome level XBL bindings?  I'm not going to NI anyone and test this myself for now.

IIRC I've been able to trigger view-source: loads that were same origin recently.  I'll run back through my standard set of things I test to see if that's correct.

Very interesting indeed.
(In reply to :Gijs Kruitbosch from comment #0)
> u = URL.createObjectURL(new Blob(["this is a blob"]));
> w = window.open("view-source:" + u, "_blank");
> 
> this opens a new tab (or window) with the view-source URI, and the content
> window has access to the result of "w", can read innerHTML etc.

This gets even more interesting if you specify the type for the blob as 'application/x-view-source'.  You will get an exception generated at the content level with '<no message>' for the message.

If you check constructor.name for this exception you will get "Object" yet this constructor is not the Object function of that content window, or any window that is a child of it(testing this in a child iframe).

I'll dig into this deeper tomorrow but that has me intrigued.  I believe it's an exception that shouldn't be accessible to web content.  My theory right now is that adding 'view-source:' to a blob URL that has the type 'application/x-view-source' is attempting to feed it through the view-source: inner workings twice and bailing out with no clue.
It appears that the constructor is the Object function from the window that *should* have loaded if it didn't bail out.  Does anyone have any what is going on with this?  

I know everyone is taking time off etc, so I'm just going to leave this here.
(In reply to Jay Gilbert from comment #5)
> I'll dig into this deeper tomorrow but that has me intrigued.  I believe
> it's an exception that shouldn't be accessible to web content.  My theory
> right now is that adding 'view-source:' to a blob URL that has the type
> 'application/x-view-source' is attempting to feed it through the
> view-source: inner workings twice and bailing out with no clue.

I'm wrong here, this is actually because I'm testing in a child iframe.  

(In reply to Jay Gilbert from comment #6)
> It appears that the constructor is the Object function from the window that
> *should* have loaded if it didn't bail out.  Does anyone have any what is
> going on with this?  

With what I see so far I'm wrong here as well.  I think we need some more eyes on this one.  I do have some chrome level errors being generated here in regards to session storage.
(In reply to Jay Gilbert from comment #7)
> (In reply to Jay Gilbert from comment #5)
> > I'll dig into this deeper tomorrow but that has me intrigued.  I believe
> > it's an exception that shouldn't be accessible to web content.  My theory
> > right now is that adding 'view-source:' to a blob URL that has the type
> > 'application/x-view-source' is attempting to feed it through the
> > view-source: inner workings twice and bailing out with no clue.
> 
> I'm wrong here, this is actually because I'm testing in a child iframe.  

Iframes are still special for view-source: bug 1243791.
sec-low because it's same-origin and the page could read the data directly anyway?
Attached patch Tentative patch (obsolete) — Splinter Review
Kyle, how about this? It passes dom/base/test/test_mozfiledataurl.html so I'm assuming it doesn't break the web.

I initially thought we could just move the subsumes check down below the other protocol flag checks, but I think that would allow blob URIs from foo.com to link to blob URIs from bar.com, because of http://searchfox.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#823-827

So I ended up doing some refactoring... does this look sane to you? (I'll take a less confusing name for CheckLoadURIFlags, I just couldn't think of one...)
Attachment #8767863 - Flags: review?(khuey)
Comment on attachment 8767863 [details] [diff] [review]
Tentative patch

Seems Kyle is away. Boris, do you have time to look at this?
Attachment #8767863 - Flags: review?(khuey) → review?(bzbarsky)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
I'd like to understand the actual threat model here.  That is, what are we trying to fix?

Looking at bug 1172165, the "read the cache" issues don't see very relevant here, right?  Are we just trying to fence around the bugs being cited in "has caused quite a few security bugs in Firefox"?

I guess maybe that last is worth doing...
Comment on attachment 8767863 [details] [diff] [review]
Tentative patch

Is there a reason not to pass the sourceBaseURI/targetBaseURI to CheckLoadURIFlags, instead of regetting them from the sourceURI/targetURI?

>+    bool hasFlags = false;

Please move this down to right before the first use.

>+    CheckLoadURIFlags(nsIURI* aSourceURI, nsIURI* aTargetURI, uint32_t aFlags);

Needs documentation.

r=me with those.
Attachment #8767863 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #12)
> Are we just trying to fence around the bugs being cited in
> "has caused quite a few security bugs in Firefox"?
> 
> I guess maybe that last is worth doing...

Defense in depth / shut all the things that look like holes (even if they're dead ends right now) is pretty much my motivation, yeah. view-source:blob from same-origin blobs is not so bad, I just think there ought not to be exceptions. DANGEROUS_TO_LOAD needs to actually mean that, never mind what's in the inner URI chain.

Plus concretely that I want to land the test in 1277583 and was told not to do that until the blob thing got sorted out (which maybe I should (have) push(ed) back harder on...).
(In reply to :Gijs Kruitbosch from comment #14)
> (In reply to Boris Zbarsky [:bz] from comment #12)
> > Are we just trying to fence around the bugs being cited in
> > "has caused quite a few security bugs in Firefox"?
> > 
> > I guess maybe that last is worth doing...
> 
> Defense in depth / shut all the things that look like holes (even if they're
> dead ends right now) is pretty much my motivation, yeah. view-source:blob
> from same-origin blobs is not so bad, I just think there ought not to be
> exceptions. DANGEROUS_TO_LOAD needs to actually mean that, never mind what's
> in the inner URI chain.
> 
> Plus concretely that I want to land the test in 1277583 and was told not to
> do that until the blob thing got sorted out (which maybe I should (have)
> push(ed) back harder on...).

I'll be trying to step up my game and help you out with this as much as possible.  I'll get up with you on IRC so we can discuss things in depth.  

I know I was asked to do similar, but honestly I'm always afraid I'll muck things up more than actually help.  I'll be in touch, and probably have some new leads I;m working on to discuss with you.
Attached patch Patch v2Splinter Review
I addressed the comments. Would have just landed but I noticed when testing this against an updated version of the tests in 1277583 that blob URIs were now erroring to the console. I fixed that in this version, but I'd appreciate if you can doublecheck that that's OK / makes sense.
Attachment #8767863 - Attachment is obsolete: true
Attachment #8770503 - Flags: review?(bzbarsky)
Comment on attachment 8770503 [details] [diff] [review]
Patch v2

r=me
Attachment #8770503 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/fb5fa9e3a52b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Group: dom-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main50-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.