Closed Bug 1216365 Opened 4 years ago Closed 4 years ago

nsMixedContentBlocker should use innerMostURI for aContentLocation

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox41 --- wontfix
firefox42 --- wontfix
firefox43 --- wontfix
firefox44 --- wontfix
firefox47 --- wontfix
firefox48 --- fixed
firefox-esr38 --- wontfix
firefox-esr45 --- wontfix

People

(Reporter: tanvi, Assigned: ckerschb)

References

Details

(Keywords: sec-low, Whiteboard: [domsecurity-active][post-critsmash-triage][adv-main48-])

Attachments

(1 file, 1 obsolete file)

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!
Tanvi, any chance you wanna take this one?
Flags: needinfo?(tanvi)
As discussed with Tanvi, I am going to take that on.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Flags: needinfo?(tanvi)
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.
Attachment #8731515 - Flags: review?(tanvi)
Whiteboard: [domsecurity-active]
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)
(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)
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.
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+
Attachment #8731515 - Attachment is obsolete: true
Attachment #8735709 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/4d360ab12c65
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Group: dom-core-security → core-security-release
Whiteboard: [domsecurity-active] → [domsecurity-active][post-critsmash-triage]
Whiteboard: [domsecurity-active][post-critsmash-triage] → [domsecurity-active][post-critsmash-triage][adv-main48-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.