Open Bug 1171853 Opened 10 years ago Updated 2 years ago

View-source allows reading the browser cache from JavaScript

Categories

(Core :: Networking, defect, P3)

Product:
Core
Component:
Networking
Version:
38 Branch
Type:
defect

Tracking

()

People

(Reporter: juho.nurminen, Unassigned)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [necko-backlog])

Attachments

(2 files, 1 obsolete file)

Bug 1171853 - stop treating nested URIs as same-origin except jar:, about: and wyciwyg:,
59 bytes, text/x-review-board-request
smaug
: review+
bzbarsky
: review-
Details
Bug 1171853 - re-fix sending inner permissions for view-source URIs,
59 bytes, text/x-review-board-request
nika
: review+
Details
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/43.0.2351.0 Safari/537.36 Steps to reproduce: Open a same-origin view-source URI using JavaScript. E.g. from https://www.example.com/, open view-source:https://www.example.com/some/sensitive/url Actual results: The opened page contains a previously cached response, even when the response was marked with "cache-control: no-store" and "pragma: no-cache". Additionally, the content of the page is readable using JavaScript from the original (opener) page. Expected results: The view-source URI uses a different URI scheme than the original page, so it should be treated as a separate origin. Thus, the content should not be readable. The view-source scheme should also not return cached responses for documents explicitly marked as non-cacheable. In fact, those pages should not be cached by the browser in the first place. This is closely related to bug 263290. About the impact: We came across this while auditing a web application that contained various XSS vulnerabilities. The application also included a single URL with highly sensitive data. This URL was only accessible for a short period of time while the application was in a certain state, and because the state was only reachable by entering the users password, it should have been impossible to retrieve the sensitive data with a simple XSS exploit. Due to this Firefox weakness, however, we were able to retrieve a cached version of the sensitive document, and read the data using JavaScript.
Jesse, should it be possible for sites to link or load the view-source view? I thought that wasn't allowed.
Component: Untriaged → View Source
Flags: needinfo?(jruderman)
Product: Firefox → Toolkit
Linking to view-source: been allowed since at least 2003 (evidence in bug 218386), but it probably shouldn't be. I filed bug 1172165 for removing support. We could fix this by: A) making view-source: obey no-store better (bug 263290) B) making view-source: not be same-origin with anything C) making view-source: unlinkable (bug 1172165)
Flags: needinfo?(jruderman)
FYI, in Google Chrome, view-source *can* be opened via JavaScript, but there it's a separate origin, so this particular combination of issues doesn't exist. I think making view-source a separate origin should be the top priority in Firefox as well, the two other issues being slightly less critical.
In Firefox nesting pseudo schemes like view-source: and jar: are considered same-origin with the actual source of the data. "view-source:" is an instruction on how to interpret the content, but the content itself still comes from http://wherever. It could lead to security bugs in sites if other browsers treat them differently and sites rely on that, but Mozilla handling for these kinds of schemes is not a secret (so I'm unhiding the bug) and so far hasn't caused problems. The caching issue (how literally do we need to take those instructions vs. smoother operation of the browser) is already covered by the bug you reference.
Group: core-security
Component: View Source → Networking
Product: Toolkit → Core
Status: UNCONFIRMED → NEW
Depends on: 1172165
Ever confirmed: true
Bug 1172165 is now fixed on Nightly, can you confirm that this is now no longer an issue with Nightly? :-)
Flags: needinfo?(juho.nurminen)
Navigating to a view-source URL programmatically doesn't seem to be possible any more, so that part's fixed. However, it's still possible to access the DOM of a view-source page if you can convince the user to type in the "view-source:" part themself. E.g.: 1) var ref = window.open("/target-page"); 2) Tell the user to add "view-source:" to the URL 3) Use ref to access the cached page This would obviously be very difficult to exploit, I just wanted to point out it's still theoretically possible. Whether it's an issue, that's your call.
Flags: needinfo?(juho.nurminen)
That's only if the URL being view-sourced is same-origin with the page that called window.open(), right?
Yes, so there's only an issue when sensitive content gets cached. This isn't a SOP bypass.
Boris, would it make sense to use the same 'all schemes match' check I implemented in caps for bug 1172165 for NS_SecurityCompareURIs https://dxr.mozilla.org/mozilla-central/rev/6ea654cad929c9bedd8a4161a182b6189fbeae6a/netwerk/base/nsNetUtil.cpp#1673 ?
Flags: needinfo?(bzbarsky)
It might, yeah...
Flags: needinfo?(bzbarsky)
Whiteboard: [necko-backlog]
Blocks: 1243791
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P1
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: P1 → P3
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
I have a patch for this that makes nested URIs inaccessible to each other, with the only exception being wyciwyg *source* URIs (not target, because AIUI wyciwyg URIs will only appear as (toplevel?) document URIs in history navigation and so there's no need for them to be *targeted* - only to be able to link to non-wyciwyg:-uri content that would otherwise be same-origin -- please clarify if I misunderstood something there). My understanding from talking to dveditz and bz is that we don't need nested URIs to "work" with non-chrome content in any other circumstance. I believe moz-icon, view-source, wyciwyg, about: (bug 1228118 - should be fixable given Activity stream and no-more-XUL-addons!) and jar: are the only nested URIs we have at this point (feed/pcast was killed in bug 1420622, RIP). The only issue I ran into was remote jar. We have a test for that (yay!). I wrote a second patch that also punches a hole for jar: targets (but not source!) iff remote jar: is preffed on. I'm not really sure if that's the right solution. Others that occurred to me are: 1. add a more general exception for jar 2. remove support for the pref entirely (perhaps in a followup) 3. bin the approach in these patches entirely and do something more restrained in terms of restricting these URIs. Please let me know if I should pursue one of those and/or misunderstood the scope of wyciwyg.
( Why would wyciwyg be toplevel only. document.write can be used in iframes too. )
Comment on attachment 8939518 [details] Bug 1171853 - stop treating nested URIs as same-origin except jar:, about: and wyciwyg:, https://reviewboard.mozilla.org/r/209848/#review215378 I think we agreed to try to assert first that wyciwyg isn't even ever passed here.
Attachment #8939518 - Flags: review?(bugs)
Comment on attachment 8939519 [details] Bug 1171853 - carve out exception for jar: when remote jar is enabled, https://reviewboard.mozilla.org/r/209850/#review215380
Attachment #8939519 - Flags: review?(bugs)
Per further discussion on IRC I filed bug 1427726 to remove remote jar pref support.
Attachment #8939519 - Attachment is obsolete: true
Asserting for wyciwyg fails in the following situation: 1. create page with iframe 2. in iframe, document.write & document.close 3. navigate iframe 4. hit back button So that wasn't an option. Per IRC, the attached patch unnests 'wyciwyg' from source/target. I also added a slightly more general exception for jar: . I'm only allowing 1 level wyciwyg, if present, followed by 1 level jar, if present. The remainder should be non-nested (so same origin checks for jar:wyciwyg:... or jar:jar:file:///blah/foo.zip!/foo.jar!/foo or wyciwyg:wyciwyg will never work - I think that's OK). I think this is the strictest thing we can land reasonably quickly, and we may want to remove the jar: case when we remove remote jar: entirely. Removing the jar: case would also break local jar: . I don't think we care about that, but perhaps I'm wrong. Either way, it seemed worth just keeping this bug simple.
Comment on attachment 8939518 [details] Bug 1171853 - stop treating nested URIs as same-origin except jar:, about: and wyciwyg:, https://reviewboard.mozilla.org/r/209848/#review215556 ::: netwerk/base/nsNetUtil.cpp:2215 (Diff revision 2) > +StripWyciwygAndJAR(nsIURI *aURI) > +{ > + if (!aURI) { > + return nullptr; > + } > + const auto JAR_SCHEME = "jar"; char*, not auto. auto doesn't make the code any easier to read here. ::: netwerk/base/nsNetUtil.cpp:2233 (Diff revision 2) > + if (NS_FAILED(rv) || !uri) { > + return nullptr; > + } > + } > + bool isJAR = false; > + if (NS_SUCCEEDED(uri->SchemeIs(JAR_SCHEME, &isJAR)) && isJAR) { So this doesn't support nested jars. So as long as we keep supporting jars, we shouldn't break that part I think. Just rearrange the code a bit and make the Jar stripping a separate method and call it recursively?
Attachment #8939518 - Flags: review?(bugs) → review-
While last night I had a patch ready that addresses the review feedback here, when I did a sanity check I realized it didn't actually *work* against the approach suggested in comment #6 . It seems this is because internally, when doing wrapper checks we rely on nsIPrincipal::Subsumes . ContentPrincipals independently fetch the innermost URI to determine origin and originNoSuffix, and the FastEquals path that we take for principal::Subsumes checks originNoSuffix and originSuffix for equality and then bails immediately if they're equal (which they would be in the case of http://mozilla.org/ and view-source:http://mozilla.org/ ). To fix this, we'd need to ensure these URIs do not share origins, or alter FastEquals in some way. The latter seems prone to perf regressions. I think instead we can stop taking the inner URIs for non-wyciwyg/jar/about: URIs to determine their origin. (We could get rid of the about: case again with bug 1228118.) However, that seems a pretty intrusive change... I'm not sure if there are other, simpler solutions. Perhaps we could give view-source: and moz-icon items a null principal, but I expect that would break things. Specifically, it seems like that might break e.g. internal links in view-source: to navigate from one view-source page to another. I'll push to try with what I have right now (ie no longer getting the innermost URI for a principal) and see what breaks, if anything. I have at least verified that it breaks the steps in comment #6 so it actually works...
Comment on attachment 8940020 [details] Bug 1171853 - re-fix sending inner permissions for view-source URIs, https://reviewboard.mozilla.org/r/210282/#review216226 ::: dom/ipc/ContentParent.cpp:5172 (Diff revision 1) > } > > nsCOMPtr<nsIPrincipal> principal; > rv = ssm->GetChannelResultPrincipal(aChannel, getter_AddRefs(principal)); > NS_ENSURE_SUCCESS(rv, rv); > + // If this is a view-source principal, get a principal for the inner URI. nit: add a newline before this comment
Attachment #8940020 - Flags: review?(nika) → review+
Comment on attachment 8939518 [details] Bug 1171853 - stop treating nested URIs as same-origin except jar:, about: and wyciwyg:, https://reviewboard.mozilla.org/r/209848/#review216232 This is getting complicated, so another review from a different person is definitely needed. ::: caps/ContentPrincipal.cpp:204 (Diff revision 3) > if (uriPrincipal) { > return uriPrincipal->GetOriginNoSuffix(aOriginNoSuffix); > } > } > > + // Now unnest any remaining nested URIs and prefix the origin with their protocols. This could use some example. Perhaps mentioning that view-source: is treated as its own origin. ::: netwerk/base/nsNetUtil.cpp:2207 (Diff revision 3) > hostHash = mozilla::HashString(host); > > return mozilla::AddToHash(schemeHash, hostHash, NS_GetRealPort(baseURI)); > } > > +// Strip 1 layer of wyciwyg (if present), and any jar nesting. The comment doesn't talk about about: ::: netwerk/base/nsNetUtil.cpp:2221 (Diff revision 3) > + const char* WYCIWYG_SCHEME = "wyciwyg"; > + nsCOMPtr<nsIURI> uri(aURI); > + nsCOMPtr<nsINestedURI> nestedURI = do_QueryInterface(uri); > + > + bool isWyciwyg = false; > + if (nestedURI && NS_SUCCEEDED(uri->SchemeIs(WYCIWYG_SCHEME, &isWyciwyg)) && Could you verify that we don't end up creating nested wyciwyg urls when coming back from bfcache and replacing document using document.write or so. ::: netwerk/base/nsNetUtil.cpp:2249 (Diff revision 3) > + } > + isJAR = false; > + nestedURI = do_QueryInterface(uri); > + } > + > + return uri ? uri.forget() : nullptr; just return uri.forget() ? ::: netwerk/base/nsNetUtil.cpp:2273 (Diff revision 3) > - { > + // Note that target/source URIs can be null. > + nsCOMPtr<nsIURI> targetBaseURI = NS_GetSameOriginInnermostURI(aTargetURI); > + nsCOMPtr<nsIURI> sourceBaseURI = NS_GetSameOriginInnermostURI(aSourceURI); > + // If the target or source is still nested, they're never same origin: > + nsCOMPtr<nsINestedURI> nestedURI = do_QueryInterface(targetBaseURI); > + if (nestedURI) {} with ifs
Attachment #8939518 - Flags: review?(bugs) → review+
So I have a question. Why is view-source a nested URI at all? The whole point was to give it the principals of the thing being viewed, but if that's not what we want we should just change that behavior. This is basically what comment 23 suggests toward the end. I would feel _much_ more confident in us doing that than in trying to treat nested URIs differently for security purposes from the way they're designed to be treated, especially if we only do the latter in some cases.
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8939518 [details] Bug 1171853 - stop treating nested URIs as same-origin except jar:, about: and wyciwyg:, https://reviewboard.mozilla.org/r/209848/#review216352 I don't think this is right. The whole point of nested URIs is to give the outer thing the same principal as the inner. If we don't want that, we shouldn't have a nested URI.
Attachment #8939518 - Flags: review?(bzbarsky) → review-
That said, looking through the uses of nsINestedURI we have the following uses: 1) Error messages in the NS_ERROR_UNKNOWN_PROTOCOL in docshell. 2) The view-source check in nsDocShell::DoURILoad (is this still needed, if we always block loading of view-source from untrusted things anyway?) 3) view-source check in nsObjectLoadingContent::LoadObject (see #2). 4) Setting view-source title in nsHtml5StreamParser::SetViewSourceTitle 5) Setting view-source base URI in nsHtml5TreeOpExecutor::GetViewSourceBaseURI 6) DownloadPlatform::IsURLPossiblyFromWeb checks. 7) Various URIChainHasFlags checks that I have not audited carefully yet. 8) Various NS_GetInnermostURI that I have not audited carefully yet. #4, 5, 6 could all be fixed without nested URI bits: #4 and #5 could QI to a view-source-specific interface, and #6 calls into the relevant protocol handler, so could poke at the "inner" URI in a non-public way... The auditing for #7 and #8 is the hard part, but presumably we want that no matter what, if we want to use the new "same-origin inner" thing in some cases. We would definitely need _something_ to keep view-source:http// from linking to view-source:file:// or so.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #28) > So I have a question. Why is view-source a nested URI at all? The whole > point was to give it the principals of the thing being viewed, but if that's > not what we want we should just change that behavior. This is an interesting question. I've always thought of nested URIs as being any URI that has some other, inner URI that it relies on in one way or another. Cf. the nsINestedURI idl file: > * nsINestedURI is an interface that must be implemented by any nsIURI > * implementation which has an "inner" URI that it actually gets data > * from. For some things, like jar:file, treating them as same-origin with their inner (archive) file makes some amount of sense. For other things, like view-source (and feed: before we killed it), it doesn't because the way in which the nested version works "adds" things to the data from the inner URI that the inner origin on its own should not have access to. If we wanted to treat reader mode as a more formally 'nested' thing (in a perfect world I'd like to switch away from about:reader?url=foo), the same would apply there. My patch, I suppose, makes some nsINestedURIs be "special" and same-origin with their inner, but the default is for that to no longer be the case. > This is basically what comment 23 suggests toward the end. > > I would feel _much_ more confident in us doing that than in trying to treat > nested URIs differently for security purposes from the way they're designed > to be treated, especially if we only do the latter in some cases. OK. So are you saying that instead of changing the "default" behaviour for nsINestedURI and having some inline exceptions, we should have a different interface that lets you get a "related" URI, in order to get from "view-source:foo" to "foo", that isn't nsINestedURI, and give that the security behaviour we want? I'm a little concerned about what the (size of the) scope of those changes would be, but I'm OK with trying that out assuming I've understood your suggestion correctly.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(bzbarsky)
> I've always thought of nested URIs as being any URI that has some other, inner URI that it relies on in one way or another OK, so that's my fault for the documentation from bug 334407 not being good enough. The intent in that bug was most definitely around the security-related behavior of various URIs and the changes there just preserved the existing view-source behavior, I think. It's been a while... > My patch, I suppose, makes some nsINestedURIs be "special" and same-origin with their inner, but the default > is for that to no longer be the case. I guess I'd like to understand what "not same-origin" really means in practice, and how the various callsites mentioned in comment 30 are affected... > we should have a different interface that lets you get a "related" URI, in order to get from > "view-source:foo" to "foo", that isn't nsINestedURI, and give that the security behaviour we want? It's not clear to me that we even need something that generic. It might be enough to have nsIViewSourceURI or some such. Again, I'm not 100% clear on how that interacts with all the varios places mentioned in comment 30. > I'm a little concerned about what the (size of the) scope of those changes would be, That's fair. I guess it feels to me like the scope to be considered is about the same no matter what; it's just whether we're consciously evaluating all the consumers or not that differs, and I feel like we really should consciously evaluate all the consumers...
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #32) > > My patch, I suppose, makes some nsINestedURIs be "special" and same-origin with their inner, but the default > > is for that to no longer be the case. > > I guess I'd like to understand what "not same-origin" really means in > practice, and how the various callsites mentioned in comment 30 are > affected... Wouldn't it just mean they'd get security errors? I'm obviously less familiar with these call-sites than you, so if you have specific concerns (around some/all of them) that would be helpful to know no matter which approach we take, I think. If you're "just" saying we need to be very cautious and evaluate the individual call-sites, then that makes sense and I will try and have a look at this. > > I'm a little concerned about what the (size of the) scope of those changes would be, > > That's fair. I guess it feels to me like the scope to be considered is > about the same no matter what; it's just whether we're consciously > evaluating all the consumers or not that differs, and I feel like we really > should consciously evaluate all the consumers... And that is also fair. I will try to have a look at the alternative approach in the next few days.
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(bzbarsky)
> Wouldn't it just mean they'd get security errors? I don't know. I mean, they're digging through nested URIs. As I said above, I haven't had a chance to audit them to see why they're doing it. > If you're "just" saying we need to be very cautious and evaluate the individual call-sites Yep.
Flags: needinfo?(bzbarsky)
OK, bz, bholley and I just talked about this. The TL;DR is that it seems there's not that many nested URI schemes left, and this bug isn't super-high-priority-oh-dear-needs-fixing-now, so we'll try and go for the "ideal" fix we could come up with, which is just default nested URIs to not be same origin with their inner. To do this, we need to get rid of the existing nested URIs that might rely on that in some way. In particular: - wyciwyg turns out to not actually be nested (which is embarrassing, because it means a lot of the code in my patches is kinda bogus!) - remove remote JAR support (bug 1427726) - stop nesting about: URIs (bug 1228118) - (maybe) stop having nested moz-icon URIs (they don't really need to be nested, and it's doubtful much will break if we unnest them - file: listings and so on already use the non-nested variants; equally, I don't think making them non-same-origin with the nested URI will break anything that we intend to work). - remove the IO Service's newNestedSimpleURI exposed method (bug 1430237) - we need to figure out what the story is with jar: URI usage on android. I'll ping some of the android folks about that. I'll file a meta bug on this in a bit and unassign myself from this one in the meantime.
Assignee: gijskruitbosch+bugs → nobody
Status: ASSIGNED → NEW
Depends on: 1228118, 1427726
Flags: needinfo?(gijskruitbosch+bugs)
Blocks: 1430257
No longer depends on: 1228118, 1427726
I'm thinking about having a new cache flag/load option like "LOAD_ALLOW_NO_STORE_CACHED_CONTENT" that would be set by browser-feature consumers suggested here: https://searchfox.org/mozilla-central/rev/b469db5c90d618f4b202d5ef280e1a78224eb43b/netwerk/protocol/http/nsHttpResponseHead.cpp#806 W/o that flag we would always validate no-store with the server, regardless of any other load/cache flags.
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: