973837 - object with data="view-source..." should be treated as an unknown scheme (like iframe)

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[Frederik Braun [:freddy]]

We successfully block view-source: navigation in Iframes, but an evildoer might easily switch to object tags:

> <object data="view-source:http://www.test.de" type="text/html"></object>
Handy data URL for testing this:
> data:text/html,<object data="view-source:http://www.test.de" type="text/html"></object>

Thanks to .mario for bringing this to our attention.

Comment 1

[Frederik Braun [:freddy]]

On a sidenote, I have verified that this "bypass" doesn't work with embed src. Data URL:
> data:text/html,<embed src="view-source:http://www.test.de" type="text/html"></embed>

Comment 2

[Boris Zbarsky [:bzbarsky]]

Try <embed> with SVG instead of HTML.

Comment 3

[Frederik Braun [:freddy]]

Like this?
> data:text/html,<embed src="view-source:http://www.test.de" type="image/svg+xml"></embed>
This is verified not loading. I still get a "plugin is needed" overlay

Comment 4

[Boris Zbarsky [:bzbarsky]]

Ah, presumably because view-source always returns "text/html" content or something.  OK, good.

Comment 5

[Bob Owen (:bobowen) PTO until 8th Oct]

Ah yes, objects don't load through docshell::DoURILoad. I think they get a channel via some other means.
I'll look at this when I get back, if nobody else has.

Comment 6

[Daniel Veditz [:dveditz]]

We should try to fix this in Firefox 30 so we don't have a partial fix for bug 624883 when we release.

Comment 7

[Bob Owen (:bobowen) PTO until 8th Oct]

Attachment: Check that view-source is not allowed for <object>s v1

Hi johns,

smaug suggested you might be able to help with my tests for this.

These tests work, but are a bit limited.

Do you know of a way that I can be notified when a document loads in the <object> or when it goes to fallback?
Then I can test the blocking of view-source more extensively.

The events (onload / onerror) don't seem to fire either way, which is what the spec suggests.

Also, if I put script inside the <object> tags, it always runs.
The spec is a bit ambiguous on this, but I think this might be a bug.
Blink has the same behaviour.

I'll upload the patch and r? you, hopefully that's OK.

Thanks,
Bob

Comment 8

[Bob Owen (:bobowen) PTO until 8th Oct]

Attachment: Don't allow view-source in <object>s v1

This is very similar to code that I used in nsDocShell to stop view-source in iframes.

I've put it in UpdateObjectParameters as this is where a malformed URI might be caught and this has the code that gets the URI from the channel for redirects as well.

Comment 9

[John Schoenick [:johns]]

Comment on attachment 8407594 [details] [diff] [review]
Don't allow view-source in <object>s v1

Review of attachment 8407594 [details] [diff] [review]:
-----------------------------------------------------------------

So this chunk of code looks sane, but I have a few issues with it as is.

I don't think UpdateObjectParameters is the right place for this - it does the crazy are-we-a-plugin-or-document-or-what checking and decides what our parameters are requesting we load, but leaves deciding to act on that to LoadObject, including security checks.

The right place in OBJLC would be by where we call CheckLoadPolicy [1], where we have several conditions that might make us override the type to Null with a fallback value. Note that LoadObject does any channel opening that might happen, so |if (mURI && !mChannelLoaded)| means we have a URI and intend to open it [2].

But, more generally, I'm not sure that duplicating security checks into <object> are a good idea. If this is only an issue with document types, which it seems to be, it seems that the docshell checks should happen after uriLoader->OpenChannel, or we should have a special LoadFromExistingChannel call in the docshell path that can do the checks that normally happen in LoadURI. We've had this issue before with iframe sandboxing :(

On the other hand, if this applies to arbitrary content types (should img src be allowed to try to load view-source?) it should probably be blocked by load policy or something higher up.

(If it's significantly more work to fix it elsewhere, I'm okay with r+'ing a version of this patch with the check moved alongside the other checks in LoadObject and opening a follow-up)

[1] http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsObjectLoadingContent.cpp#1986

[2] If UpdateObjectParameters cannot determine a type without a channel, it returns eType_Loading, which LoadObject handles by just calling OpenChannel. OnStartRequest then sees we're eType_Loading and calls LoadObject again, but with mChannelLoaded set.

Comment 10

[John Schoenick [:johns]]

Comment on attachment 8407592 [details] [diff] [review]
Check that view-source is not allowed for <object>s v1

Review of attachment 8407592 [details] [diff] [review]:
-----------------------------------------------------------------

So we can use SpecialPowers to access a chrome-only MozObjectLoadingContent interface that exposes a little more info, including our loaded type:

http://dxr.mozilla.org/mozilla-central/source/dom/webidl/HTMLObjectElement.webidl#80

See e.g.:
http://dxr.mozilla.org/mozilla-central/source/content/base/test/test_bug810494.html#32

It sucks that we don't fire events, if the spec says we should it probably wouldn't be too hard to add now that the class is factored (slightly) more sanely.

Comment 11

[Bob Owen (:bobowen) PTO until 8th Oct]

(In reply to John Schoenick [:johns] from comment #9)

Thanks for the comprehensive reply.
Sorry for the delay, it was a long weekend with two public holidays in the UK for Easter.

> I don't think UpdateObjectParameters is the right place for this - it does
> the crazy are-we-a-plugin-or-document-or-what checking and decides what our
> parameters are requesting we load, but leaves deciding to act on that to
> LoadObject, including security checks.

Ah, OK ... as I said, after a fair bit of deliberation I thought here, because this is near where a malformed URL would be caught.
 
> The right place in OBJLC would be by where we call CheckLoadPolicy [1],
> where we have several conditions that might make us override the type to
> Null with a fallback value. Note that LoadObject does any channel opening
> that might happen, so |if (mURI && !mChannelLoaded)| means we have a URI and
> intend to open it [2].

This was one of my other options. :-)
Or possibly in CanHandleURI, but this would only work for document types.
After a quick IRC chat with freddyb a week or so ago, he seemed to think we should try blocking any <object> data URI that has a view-source scheme.

> On the other hand, if this applies to arbitrary content types (should img
> src be allowed to try to load view-source?) it should probably be blocked by
> load policy or something higher up.

When I tried to find somewhere common to do the checks further down the processing, I didn't have the relevant context of whether I was in a frame or object.
I don't know anything about load policy, but it looks like it might work as the content type gets passed in.

> (If it's significantly more work to fix it elsewhere, I'm okay with r+'ing a
> version of this patch with the check moved alongside the other checks in
> LoadObject and opening a follow-up)

Thanks, I'll look into load policy first and see if I can make that work.


(In reply to John Schoenick [:johns] from comment #10)

> So we can use SpecialPowers to access a chrome-only MozObjectLoadingContent
> interface that exposes a little more info, including our loaded type:

Yeah, I'd picked this up from other tests, but I was only looking at the fallback type.
I hadn't twigged that the type goes to TYPE_NULL, thanks.
My main problem was knowing when to check for contentWindow, as when the load is allowed, part of that loading is async. That's why I was using data URIs.
However I think this |.displayedType| should get set synchronously, so may solve my problem.
Although, it might mean I can't test redirects in an automated way at the moment.
 
> It sucks that we don't fire events, if the spec says we should it probably
> wouldn't be too hard to add now that the class is factored (slightly) more
> sanely.

Like I say, the spec is a bit ambiguous for this particular case, but I'm not sure we are always firing events where it is clear.
Maybe have to leave this to another bug.

Comment 12

[Bob Owen (:bobowen) PTO until 8th Oct]

(In reply to Bob Owen (:bobowen) from comment #11)
> (In reply to John Schoenick [:johns] from comment #9)

> Thanks, I'll look into load policy first and see if I can make that work.

So, it definitely looks like I could modify an existing or add a new content policy to block view-source in anything but TYPE_DOCUMENT.
The main problem is that although this could work for frame and object loads, the original behaviour requested for frame loads was to treat it as an unknown scheme.  The way that nsDocShell currently checks load policy wouldn't give this result, so for the moment I think I'll look into adding the code into nsObjectLoadingContent::LoadObject.

Comment 13

[Bob Owen (:bobowen) PTO until 8th Oct]

Attachment: Don't allow view-source in <object>s v2

So, as I explained in comment 12 I don't think content policy will currently work well for other types of frames, so I'm just adding to OBJLC for the moment.

There is a plan to replace content policy et al., so it probably makes sense to wait until some of that is in place and then look at moving these checks.

Comment 14

[Bob Owen (:bobowen) PTO until 8th Oct]

Attachment: Check that view-source is not allowed for <object>s v2

Using displayedType works for standard URIs, so I could add tests for various other nestings of view-source.

Comment 15

[John Schoenick [:johns]]

Comment on attachment 8412514 [details] [diff] [review]
Don't allow view-source in <object>s v2

Review of attachment 8412514 [details] [diff] [review]:
-----------------------------------------------------------------

So this looks fine, with the continued nit that it doesn't seem like the right level for this check in general :-/ Can we open a follow-up bug to reject loading special URIs like view-source in arbitrary content contexts? (I'm equally upset that you can load chrome:// URIs in content...)

Comment 16

[John Schoenick [:johns]]

Comment on attachment 8412518 [details] [diff] [review]
Check that view-source is not allowed for <object>s v2

Review of attachment 8412518 [details] [diff] [review]:
-----------------------------------------------------------------

This should probably also check redirects to view-source URIs, but otherwise LGTM

Comment 17

[Bob Owen (:bobowen) PTO until 8th Oct]

(In reply to John Schoenick [:johns] from comment #15)

> So this looks fine, with the continued nit that it doesn't seem like the
> right level for this check in general :-/ Can we open a follow-up bug to
> reject loading special URIs like view-source in arbitrary content contexts?
> (I'm equally upset that you can load chrome:// URIs in content...)

Thanks for the reviews.
Bug 1003759 raised as a follow-up.

(In reply to John Schoenick [:johns] from comment #16)

> This should probably also check redirects to view-source URIs, but otherwise
> LGTM

Yeah, testing redirects is difficult, because of the async nature and the lack of an error event.
I've tested this manually (just done it again since the move to LoadObject to make sure).
Raised bugs 1003799 and 1003797, to look into this.

Comment 18

[Bob Owen (:bobowen) PTO until 8th Oct]

try push: https://tbpl.mozilla.org/?tree=Try&rev=d58a7cc7516c

landing order:
https://bugzilla.mozilla.org/attachment.cgi?id=8412514
https://bugzilla.mozilla.org/attachment.cgi?id=8412518

Thanks.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b1a07e50882
https://hg.mozilla.org/integration/mozilla-inbound/rev/0687336665f1

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9b1a07e50882
https://hg.mozilla.org/mozilla-central/rev/0687336665f1

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

Please nominate for Aurora/Beta uplift with risk assessment and what, if any, verification STR can be used by QA

Comment 22

[Bob Owen (:bobowen) PTO until 8th Oct]

Comment on attachment 8412514 [details] [diff] [review]
Don't allow view-source in <object>s v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): This was a pre-existing issue that is related to bug 624883 , which already has a fix in firefox30.

User impact if declined: If the techniques explained in bug 624883 are being exploited, once that avenue is closed by that fix they could switch to using <object>s without this fix.

Testing completed (on m-c, etc.): Automated tests added for various different view-source URIs, including nested ones (e.g. feed:view-source:).
Also manually tested when the view-source URI is as the result of a redirect.
Difficult to automate this at the moment, bug 1003799 raised as a follow-up.
No QA verification.
data: URI in the description can be used for verification and modified to test nested URIs.
I've set up a TinyURL to allow manual redirect testing, this does a 301 to view-source:http://example.com
data:text/html,<object data="http://tinyurl.com/view-source-redirect" type="text/html" height=500 width=1000></object>

Risk to taking this patch (and alternatives if risky): Medium - the fix is isolated to one part of the code and is very similar to the code for bug 624883, however the code for and the usages of <object> are quite complex.
Bug 624883 caused a regression to a view-source add-on, that was resolved by a change to the add-on. It is far less likely that <object> would be being used in this way.

String or IDL/UUID changes made by this patch: None

Comment 23

[Bob Owen (:bobowen) PTO until 8th Oct]

Comment on attachment 8412518 [details] [diff] [review]
Check that view-source is not allowed for <object>s v2

The are tests for the uplift requested in comment 22.
They should be uplifted after that patch.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/19ec557badde
https://hg.mozilla.org/releases/mozilla-aurora/rev/55f7aaaf703f

https://hg.mozilla.org/releases/mozilla-beta/rev/acb6d71ba45d
https://hg.mozilla.org/releases/mozilla-beta/rev/ff4da156e917

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/acb6d71ba45d
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/ff4da156e917

Comment 26

[Andrei Vaida [:avaida]]

I was able to confirm the fix for this issue on Windows 7 64-bit, using the information available in Comment 22 with:
  * Firefox 30 Beta 5 (Build ID: 20140515140857) [1],
  * Aurora 31 2014-05-19 [2],
  * Nightly 32 2014-05-19 [3].

[1] Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 
[2] Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0
[3] Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:32.0) Gecko/20100101 Firefox/32.0