User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.13) Gecko/20101203 Firefox/3.6.13 Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.13) Gecko/20101203 Firefox/3.6.13 If a malicious site does: <iframe name="foo" src="view-source:http://www.foo.com/"></iframe> It is then possible to get a user to hit ctrl-A, ctrl-C, ctrl-V and steal the user's data. Note: This kind of attack is similar to click jacking but involves copy-pasting. Reproducible: Always Steps to Reproduce: 1.Create a page which has <iframe name="foo" src="view-source:http://www.foo.com/"></iframe> 2.Visit the page 3.Notice that the source code is being rendered in an iframe Actual Results: The source code is being rendered. Expected Results: A blank page should instead be rendered.

Paul Stone brought this to our attention last year through his "Click-dragging" talk, but I can't currently find a bug on it. view-source: is one variant, as is framing sensitive HTML information that the user can reach but the attacker can't (for example, facebook or the user's bank information).

Special-casing "view-source:" by blanking the frame as suggested by the summary doesn't solve the problem, it only solves it for view-source. On the other hand, view-source is a litle bit special. In addition to allowing access to possibly interesting meta-data in the HTML source (hidden anti-CSRF tokens?), view-source has the advantage of disabling script-based "frame busting" clickjacking defenses (again credit Paul Stone). The defense for both is to add the X-Frame-Options: DENY header (supported by recent 3.6.x builds as well as other browsers), but that requires proactive work on the server's part.

Daniel: thanks for looking into this. Afaik, view-source: doesn't honnor the X-Frame-Options: DENY header (i.e. I can iframe the source even if I'm not allowed to iframe the page). I would like to add to this discussion that Chrome doesn't render view-source in iframes. I believe that's the right thing to do (i.e. i think having a list of things which don't get rendered in iframe contexts make sense).

(In reply to comment #3) > Daniel: thanks for looking into this. Afaik, view-source: doesn't honnor the > X-Frame-Options: DENY header (i.e. I can iframe the source even if I'm not > allowed to iframe the page). Nope. That was fixed in bug 586891. I just confirmed that it's still fixed. Do you have evidence to the contrary?

Hmm, I may have spoken too soon in comment 4. I'll dig into this more.

Okay, we're all clear. Here's my test (and sorry for the spam): http://hackmill.com/tests/x-frame-options-test.html

Sorry, i failed to test correctly. It seems the X-Frame-Options: DENY is honored.

Brandon, is this bug still valid given comment 7?

I suppose we can mark this fixed by bug 586891. dveditz, does that sound reasonable?

It seems like the bad uses of <iframe src='view-source:...'> greatly outweigh the good uses. And, in fact, I am not sure there are any practical good uses. So, let's just move forward with this. Of course, I question why we'd blacklist view-source as opposed to whitelisting the things that we think are appropriate for iframes. It seems all of about:* except about:blank, chrome:*, jar:*, app:*, etc. could all be excluded from the whitelist, though that may require us to change a bunch of tests.

This came up as part of Paul Stone's BlackHat USA 2013 talk (in combination with bug 711043).

(In reply to Brandon Sterne (:bsterne) from comment #9) > I suppose we can mark this fixed by bug 586891. dveditz, does that sound > reasonable? Tacitly leaving this open was not much of an answer; let me fix that. Bug 586891 was a separate issue, where attackers could bypass XFO protection using view-source: -- but assumes view-source: frames are benign otherwise. This bug is questioning that assumption (and in comment 10 bsmith threatens to expand the bug's scope quite a bit). I feel pretty confident we could kill view-source: without breaking too much (maybe some Firefox-specific web tutorials?), but trying to tackle the bigger picture at the same time could slow this bug down a lot.

I don't really know how the frame loading code works but we should also render blank pages for URLS like feed:view-source:http://example.com and pcast:view-source:http://example.com and probably jar:view-source:http://example.com/bar.zip

Adding a test case url.

Given that nested URLs are not a "web standard concept" at the moment doing the equivalent of a network error for when view-source is used outside the address bar seems like a good thing, independent of security benefits. We should try to prevent spreading view-source I think lest the web becomes dependent upon sub-schemes.

We need an owner here.

This bug makes timing attacks particularly interesting in Firefox. Search for "view source" in the paper here: http://www.contextis.com/research/white-papers/pixel-perfect-timing-attacks-html5/

This patch adds a check into nsDocShell::InternalLoad for non top-level docshells that checks all the nested browsing URIs for the view-source scheme and if found loads about:blank instead. Seems to work for tanvi's test in comment 14 and because it is in InternalLoad, also works if the frame is navigated (e.g. by window.open). Also tested manually for feed:view-source:, pcast:view-source: and other deeper nestings. I think this should cover any nested browsing context (frame, iframe, object), but I've only tested iframe so far. "View Page Source" and "View Frame Source" still work as does a view-source URI in the address bar. Still need to check for jar:view-source:, I'm not sure it will cover this and of course write mochitests. I also need to check the parameters on my recursive call to InternalLoad try push: https://tbpl.mozilla.org/?tree=Try&rev=5aa5cfa15730

How about a patch that actually compiles :) Sorry, I forgot that I'd made the changes on top of the fix for bug 785310. new try push: https://tbpl.mozilla.org/?tree=Try&rev=11ad65a7a9c1

The proper fix here is to only special case view-source when navigating a top-level browsing context and otherwise return a network error (which is what should happen per http://fetch.spec.whatwg.org/ for unknown schemes which is what view-source would be in that context).

(In reply to Anne (:annevk) from comment #20) > The proper fix here is to only special case view-source when navigating a > top-level browsing context and otherwise return a network error (which is > what should happen per http://fetch.spec.whatwg.org/ for unknown schemes > which is what view-source would be in that context). OK, I'll look into doing that, thanks. Just quickly tried returning an exception code from my current patch instead of navigating to about:blank, but this doesn't give the same result as a different invalid scheme. I think I need to move my check into DoURILoad where it tries to get a Channel for the URI, I'll try this tomorrow, if I get a chance.

With this patch it now acts as if view-source is like any other unknown scheme when not top-level. My only concern is that because it is using the same error reporting mechanism, it doesn't give any indication that this is because it is being using in a frame. Having the change in DoURILoad makes much more sense though, as it is right by existing code that returns NS_ERROR_UNKNOWN_PROTOCOL if a channel cannot be found for the URI. I still need to look at jar:view-source: From the try run, I'll need to fix some existing tests as well.

Mochitests for view-source in iframe. Tests with feed:, pcast: and jar: as per comment 13. Also tests the interaction when nested with invalid protocols.

The error message for an invalid protocol within a nested URI was already fairly broken. For example if you go to: feed:wibble:http://example.com You get an error page that complains that "feed" (or what ever is first) isn't associated with any program. So I've changed this error message, in the four places that I found it in, to include a list of protocols. In three places this will be in the above case: Firefox doesn't know how to open this address, because one of the following protocols (feed, wibble) isn't associated with any program or is not allowed in this context. The list stops at wibble, because the nesting of the URIs breaks down at that point. The fourth is slightly different, but with similar wording changes. I still need to fix the tests this breaks in the try run here: https://tbpl.mozilla.org/?tree=Try&rev=5f149a0e890d

Removed CSP tests around the use of view-source: in an iframe in content/base/test/csp/test_CSP_frameancestors.html as no longer required given the total ban. Changed parser/htmlparser/tests/mochitest/test_viewsource.html to use a new window instead of an iframe. New try runs: https://tbpl.mozilla.org/?tree=Try&rev=3048ef80b302 https://tbpl.mozilla.org/?tree=Try&rev=a7111d7f14e5

Comment on attachment 8367518 [details] [diff] [review] bug 624883 fix V2 - Treat view-source as an unrecognised scheme if not top-level. You need to change the name of the string when changing the value; that's how the l10n folks know they need a new translation. I'm really not that happy with hardcoding a protocol here, but adding a protocol flag here seems like slight overkill. Unless we think there are other protocols we'd want this behavior for? So I guess it's OK to hardcode it for now with a comment explaining why this check really is view-source-specific. >+ if (tempURI && !NS_FAILED(rv2)) { It's probably better form to test rv2 before even looking at tempURI. >+ if (isViewSource || NS_FAILED(rv)) { Similar here. r=me with those issues fixed.

Comment on attachment 8368286 [details] [diff] [review] bug 624883 tests V2 - ensure view-source is forbidden in iframes. This is relying on chrome tests not running in a chrome docshell. It might be better to not rely on that (so use an <iframe type="content"> which contains the actual iframe you test in). If we're going to use promises, we need to be a bit careful: given promise.then(foo, bar) if foo throws the exception is lost from the point of view of the test harness. What you may want to do instead is: promise.then(foo, bar) .then(undefined, function (e) { ok(false, "Unexpected exception thrown", e); }); or something. >+ var testSrc = testCase.protocols + "://example.com/!/"; This needs to be outdented two spaces. Good to see we have a test that effectively tests that things work in a toplevel document. r=me with the nits fixed

(In reply to Boris Zbarsky [:bz] from comment #28) > Comment on attachment 8368286 [details] [diff] [review] > bug 624883 tests V2 - ensure view-source is forbidden in iframes. Thanks for the reviews. > This is relying on chrome tests not running in a chrome docshell. It might > be better to not rely on that (so use an <iframe type="content"> which > contains the actual iframe you test in). I tried this and ran into a problem in the Promise creation method (createNetworkErrorMessagePromise) where it tries to access the frame's docShell at the end. This doesn't exist on the normal content iframes. > If we're going to use promises, we need to be a bit careful: given > promise.then(foo, bar) if foo throws the exception is lost from the point of > view of the test harness. What you may want to do instead is: > > promise.then(foo, bar) > .then(undefined, function (e) { > ok(false, "Unexpected exception thrown", e); > }); > > or something. I see, thanks. This is my first use of promises. > >+ var testSrc = testCase.protocols + "://example.com/!/"; > > This needs to be outdented two spaces. Whoops, vim was having a few problems with the indentation inside the xul file, I missed that one. I'm also having a few problems with the rename of protocolNotFound for the fix patch. I've changed it to unknownProtocolFound, which needed to be done in quite a few other files (dtds and xhtmls). However I seem to have gained an "&f=regular" on the end of the error message. So, it appears to be having problems parsing the error page URI.

> where it tries to access the frame's docShell at the end. Just because frame.docShell is a XUL thing? You can probably frame.contentWindow .QueryInterface(Components.interfaces.nsIInterfaceRequestor) .getInterface(Components.interfaces.nsIWebNavigation) to get the docshell. > However I seem to have gained an "&f=regular" on the end of the error message. That's bug 967342. Update?

Comment on attachment 8371414 [details] [diff] [review] bug 624883 tests V3 - ensure view-source is forbidden in iframes. In addition to Comment 31. Enter key submitted my patch before I'd finished. :-) (In reply to Boris Zbarsky [:bz] from comment #30) > You can probably > > frame.contentWindow > .QueryInterface(Components.interfaces.nsIInterfaceRequestor) > .getInterface(Components.interfaces.nsIWebNavigation) That worked a treat, thanks bz. > > However I seem to have gained an "&f=regular" on the end of the error message. > > That's bug 967342. Update? Thanks, I found this just before I finished last night. The fix works, but the real culprit is the query string parsing code in the error page, it's ... erm ... interesting. I'm guessing it was written on an old version of JavaScript originally. It searches for specific strings and relies on the order of the parameters. In particular the description (d) must come at the end. These changes were fairly big so I'm r?ing again, just to be safe.

(In reply to Boris Zbarsky [:bz] from comment #27) > You need to change the name of the string when changing the value; that's > how the l10n folks know they need a new translation. protocolNotFound changed to unknownProtocolFound. > I'm really not that happy with hardcoding a protocol here, but adding a > protocol flag here seems like slight overkill. Unless we think there are > other protocols we'd want this behavior for? The "view-source" string seems to be hard coded in most places it is used, not really an excuse for me following along though. :) > So I guess it's OK to hardcode it for now with a comment explaining why this > check really is view-source-specific. Thanks, hopefully I've understood what you're saying and the new comment makes sense. > >+ if (tempURI && !NS_FAILED(rv2)) { > > It's probably better form to test rv2 before even looking at tempURI. > > >+ if (isViewSource || NS_FAILED(rv)) { > > Similar here. Both switched, thanks. > r=me with those issues fixed. Again, I've set r? as the changes included quite a lot of extra files.

Comment on attachment 8371414 [details] [diff] [review] bug 624883 tests V3 - ensure view-source is forbidden in iframes. r=me, thanks!

Comment on attachment 8371427 [details] [diff] [review] bug 624883 fix V3 - Treat view-source as an unrecognised scheme if not top-level. Missed one nit last time: !NS_FAILED(rv2) == NS_SUCCEEDED(rv2). r=me with that

(In reply to Boris Zbarsky [:bz] from comment #35) > Missed one nit last time: !NS_FAILED(rv2) == NS_SUCCEEDED(rv2). > > r=me with that Fixed, so r=bz Kicking off a full try run before landing to make sure no other tests rely on view-source in frames. https://tbpl.mozilla.org/?tree=Try&rev=3ed691aac03f

Try run was green after a handful of retries, so hopefully this will land cleanly.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a57801cec8be https://hg.mozilla.org/integration/mozilla-inbound/rev/4e44b26f3c67

Comment on attachment 8371536 [details] [diff] [review] bug 624883 fix V4 - Treat view-source as an unrecognised scheme if not top-level. >-protocolNotFound=%S is not a registered protocol. >+unknownProtocolFound=One of the following %S is not a registered protocol or is not allowed in this context. (The other versions of this file already had parentheses around the %S, but without them, this version doesn't read quite as cleanly as it used to.)

https://hg.mozilla.org/mozilla-central/rev/a57801cec8be https://hg.mozilla.org/mozilla-central/rev/4e44b26f3c67

One doubt looking at the string: is using "Firefox" (hard-coded, instead of &brandShortName; or similar) correct in this context?

I suspect so, since it's only used in the Firefox-specific appstrings.properties files. The webapprt and dom ones don't use "Firefox", right?

(In reply to Boris Zbarsky [:bz] from comment #42) > I suspect so, since it's only used in the Firefox-specific > appstrings.properties files. The webapprt and dom ones don't use "Firefox", > right? DOM is definitely fine: no references to Firefox. I did another check and appstrings.properties is plenty of "Firefox" references, while strings in neterror.dtd didn't actually change. So this bug should be good, or we just have a wider problem on appstrings.properties (I don't recall any complaints). I don't see anything related to webapprt in patches/landings in this bug though http://hg.mozilla.org/mozilla-central/log/default/webapprt/locales/en-US/webapprt/overrides/appstrings.properties

(In reply to neil@parkwaycc.co.uk from comment #39) > Comment on attachment 8371536 [details] [diff] [review] > >+unknownProtocolFound=One of the following %S is not a registered protocol or is not allowed in this context. > (The other versions of this file already had parentheses around the %S, but > without them, this version doesn't read quite as cleanly as it used to.) I see what you mean there, happy to re-open and change, but I'll wait until the point below is settled. (In reply to Francesco Lodolo [:flod] from comment #43) > I don't see anything related to webapprt in patches/landings in this bug > though That's because the webapprt appstrings.properties file didn't have the existing protocolNotFound string in it. I'm not sure whether it should have. All the other files have the same properties defined, three identically and the dom one with different values. The following are not in the webapprt one: connectionFailure dnsNotFound externalProtocolChkMsg externalProtocolLaunchBtn externalProtocolPrompt externalProtocolTitle externalProtocolUnknown fileNotFound isprinting malformedURI malwareBlocked netInterrupt netTimeout phishingBlocked resendButton.label unknownProtocolFound

(In reply to Bob Owen (:bobowen) from comment #44) > That's because the webapprt appstrings.properties file didn't have the > existing protocolNotFound string in it. > I'm not sure whether it should have. Neither am I (there was some discussion in bug 707294 about it). I think the main point is to determine if webapprt can reach the code calling these strings.

webapprt's version needs to be kept up to date (it can certainly trigger that particular error). This mess with a bunch of forked files needs to be cleaned up :(

(via some combination of bug 306369, bug 302309, bug 336029)

I filed bug 972090.

(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #48) > I filed bug 972090. Thanks Gavin. With that issue being tracked in a separate bug, I'll change that message. (In reply to neil@parkwaycc.co.uk from comment #39) > >+unknownProtocolFound=One of the following %S is not a registered protocol or is not allowed in this context. > (The other versions of this file already had parentheses around the %S, but > without them, this version doesn't read quite as cleanly as it used to.) Parentheses added. bz: hopefully this will be a quick review. :) Don't think this would cause any problems, here's a quick try push just in case: https://tbpl.mozilla.org/?tree=Try&rev=99d63b35762b

(In reply to Bob Owen (:bobowen) from comment #49) This time with the correct patch exported ... sorry.

As a general rule, you shouldn't change strings already landed on mozilla-central. This is a really minor change so it's fine, and I guess it would also create a hell of pain to fix (like renaming this string in all clones of this file), but it would be nice to double check also strings before landing, not just code ;-)

(In reply to Francesco Lodolo [:flod] from comment #51) > As a general rule, you shouldn't change strings already landed on > mozilla-central. > > This is a really minor change so it's fine, and I guess it would also create > a hell of pain to fix (like renaming this string in all clones of this > file), but it would be nice to double check also strings before landing, not > just code ;-) Sorry, didn't think about it already having landed, as it was part of the same bug and such a small change. Who is the best person to CC / r? on bugs, if I change any strings like this in the future?

You can cc me or :pike, review should go through the standard channel (module peer/owner, etc.) An explanation is available on MDN, in general when in doubt it's probably better to just change the ID https://developer.mozilla.org/en-US/docs/Making_String_Changes As I said, in this specific case it's fine to just fix the string without new ID. If you're curious, you can also check what locales did so far on central http://transvision.mozfr.org/string/?entity=dom/chrome/appstrings.properties:unknownProtocolFound&repo=central

Comment on attachment 8375395 [details] [diff] [review] Bug 624883 - add parentheses around protocol list in dom unknownProtocolFound string. r=me

(In reply to Boris Zbarsky [:bz] from comment #54) > Comment on attachment 8375395 [details] [diff] [review] > Bug 624883 - add parentheses around protocol list in dom > unknownProtocolFound string. > > r=me Thanks bz. The try push was all green. Checkin only needed for Attachment 8375395 [details] [diff]: Bug 624883 - add parentheses around protocol list in dom unknownProtocolFound string. Thanks.