Closed
Bug 1216365
Opened 9 years ago
Closed 8 years ago
nsMixedContentBlocker should use innerMostURI for aContentLocation
Categories
(Core :: DOM: Security, defect)
Core
DOM: Security
Tracking
()
People
(Reporter: tanvi, Assigned: ckerschb)
References
Details
(Keywords: sec-low, Whiteboard: [domsecurity-active][post-critsmash-triage][adv-main48-])
Attachments
(1 file, 1 obsolete file)
7.36 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
In bug https://bugzilla.mozilla.org/show_bug.cgi?id=1148732 we updated nsMixedContentBlocker to use the innerURI for requestingLocation. We should do the same for aContentLocation. Example: https://foo.com is the top level page. It loads local-custom-protocol:http://bar.com, where local-custom-protocol is an addon created protocol handler that is marked as URI_IS_LOCAL_RESOURCE. This will pass the check in nsMixedContentBlocker and the http load will be accepted. http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#504 Should we be using NS_URIChainHasFlags here or something else? Do we only care about the inner most uri (as we did in https://bugzilla.mozilla.org/show_bug.cgi?id=1148732#c12)? Should we call NS_URIChainHasFlags on the innermost uri? Thanks jwatt for catching this!
Updated•9 years ago
|
status-firefox41:
--- → wontfix
status-firefox42:
--- → affected
status-firefox43:
--- → affected
status-firefox44:
--- → affected
status-firefox-esr38:
--- → affected
Assignee | ||
Comment 2•8 years ago
|
||
As discussed with Tanvi, I am going to take that on.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Flags: needinfo?(tanvi)
Reporter | ||
Comment 3•8 years ago
|
||
Hey Chris, Looks like my comment 0 has some open questions: (In reply to Tanvi Vyas - please needinfo [:tanvi] from comment #0) > Should we be using NS_URIChainHasFlags here or something else? Do we only > care about the inner most uri (as we did in > https://bugzilla.mozilla.org/show_bug.cgi?id=1148732#c12)? Should we call > NS_URIChainHasFlags on the innermost uri? Does URIChainHasFlags check all nested protocols, or only the outermost? Should we check all nested protocols against the flag or just the innermost? We can discuss with Dan and determine the right thing to do here. Implementation wise, it should only be a few lines of code.
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8731515 -
Flags: review?(tanvi)
Assignee | ||
Updated•8 years ago
|
Whiteboard: [domsecurity-active]
Reporter | ||
Comment 5•8 years ago
|
||
Comment on attachment 8731515 [details] [diff] [review] bug_1216365_mcb_innermosturi.patch It appears that NS_URIChainHasFlags[1] is going to check all nested protocols for the flag. So the current implementation actually checks more than this new implementation[2]. [1] https://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsNetUtil.h#849 [2] >@@ -516,26 +527,26 @@ nsMixedContentBlocker::ShouldLoad(bool a > * "https", > * "moz-safe-about" > * > */ > bool schemeLocal = false; > bool schemeNoReturnData = false; > bool schemeInherits = false; > bool schemeSecure = false; >- if (NS_FAILED(NS_URIChainHasFlags(aContentLocation, nsIProtocolHandler::URI_IS_LOCAL_RESOURCE , &schemeLocal)) || >- NS_FAILED(NS_URIChainHasFlags(aContentLocation, nsIProtocolHandler::URI_DOES_NOT_RETURN_DATA, &schemeNoReturnData)) || >- NS_FAILED(NS_URIChainHasFlags(aContentLocation, nsIProtocolHandler::URI_INHERITS_SECURITY_CONTEXT, &schemeInherits)) || >- NS_FAILED(NS_URIChainHasFlags(aContentLocation, nsIProtocolHandler::URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT, &schemeSecure))) { >+ if (NS_FAILED(NS_URIChainHasFlags(innerContentLocation, nsIProtocolHandler::URI_IS_LOCAL_RESOURCE , &schemeLocal)) || >+ NS_FAILED(NS_URIChainHasFlags(innerContentLocation, nsIProtocolHandler::URI_DOES_NOT_RETURN_DATA, &schemeNoReturnData)) || >+ NS_FAILED(NS_URIChainHasFlags(innerContentLocation, nsIProtocolHandler::URI_INHERITS_SECURITY_CONTEXT, &schemeInherits)) || >+ NS_FAILED(NS_URIChainHasFlags(innerContentLocation, nsIProtocolHandler::URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT, &schemeSecure))) { > *aDecision = REJECT_REQUEST; > return NS_ERROR_FAILURE; > } Should we continue performing secure/insecure checks on all nested protocols, or switch to just using the innermost protocol? Just want to make sure that checking the innermost is all we need to do, as Christoph mentions in a comment in this new patch: >+ // Make sure to get the URI the load started with. No need to check >+ // outer schemes because all the wrapping pseudo protocols inherit the >+ // security properties of the actual network request represented >+ // by the innerMost URL. needinfo'ing Dan. Dan, is checking innermost protocol fine, or should we check the whole chain when trying to determine if a subresource load is secure or insecure? Thanks!
Flags: needinfo?(dveditz)
Comment 6•8 years ago
|
||
(In reply to Tanvi Vyas - please needinfo [:tanvi] from comment #5) > Dan, is checking innermost protocol fine, or should we check the whole chain > when trying to determine if a subresource load is secure or insecure? In practice, with our known current nsINestedURIs, we have lots of code that drills immediately down to the innermostURI. To the extent that the ones we know about are local transforms (view-source:, jar:) the security is determined by where the innermost data comes from. In theory one can imagine someone inventing a protocol for a service that processed the innermost URL against some internet service they may or may not connect to securely. Doing that kind of thing insecurely would be unacceptable these days and hopefully our add-on reviewers would reject it, but for add-ons that don't go through review would they be smart enough to avoid it? Using the innermost URL is going to work in practice, possibly forever. If someone adds a funky scheme like I proposed then I bet a bunch of the other GetInnermostURI uses will be broken at the same time. Doing it the way you propose is wrong, but I waffle on whether it's "good enough". Of course in a variation of my hypothetical example I can also imagine the creator lying and adding the URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT to "fix" that pesky mixed-content blocking.
Flags: needinfo?(dveditz)
Reporter | ||
Comment 7•8 years ago
|
||
Okay, thanks Dan for your feedback! Then lets use the innermostURI, because it is the most correct. Chris, I will review the rest of the patch.
Reporter | ||
Comment 8•8 years ago
|
||
Comment on attachment 8731515 [details] [diff] [review] bug_1216365_mcb_innermosturi.patch >@@ -639,17 +650,17 @@ nsMixedContentBlocker::ShouldLoad(bool a > if (docShell->GetDocument()->GetBlockAllMixedContent(isPreload)) { > // log a message to the console before returning. > nsAutoCString spec; >- rv = aContentLocation->GetSpec(spec); >+ rv = innerContentLocation->GetSpec(spec); > NS_ENSURE_SUCCESS(rv, rv); > NS_ConvertUTF8toUTF16 reportSpec(spec); > > const char16_t* params[] = { reportSpec.get()}; > CSP_LogLocalizedStr(MOZ_UTF16("blockAllMixedContent"), > params, ArrayLength(params), > EmptyString(), // aSourceFile > EmptyString(), // aScriptSample For this one, we may want to stick with aContentLocation, since more information (a longer URL) is better than less information for developers reading webconsole messages. Same for all of the calls to LogMixedContentMessage().
Attachment #8731515 -
Flags: review?(tanvi) → review+
Assignee | ||
Comment 9•8 years ago
|
||
Attachment #8731515 -
Attachment is obsolete: true
Attachment #8735709 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 10•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d360ab12c65
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4d360ab12c65
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•8 years ago
|
Group: dom-core-security → core-security-release
Updated•8 years ago
|
Whiteboard: [domsecurity-active] → [domsecurity-active][post-critsmash-triage]
Updated•8 years ago
|
status-firefox47:
--- → wontfix
status-firefox-esr45:
--- → wontfix
Whiteboard: [domsecurity-active][post-critsmash-triage] → [domsecurity-active][post-critsmash-triage][adv-main48-]
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•