In addition to checking specific protocols, the isOriginPotentiallyTrustworthy method should also check URI flags when determinign whether a URI is secure. nsMixedContentBlocker does this: https://dxr.mozilla.org/mozilla-central/rev/57ac9f63fc6953f4efeb0cc84a60192d3721251f/dom/security/nsMixedContentBlocker.cpp#547 We should audit the URI flags to determine which ones make sense to use for isOriginPotentiallyTrustworthy and nsMixedContentBlocker. Please take a look at bug 1237868 when doing this audit, as this bug potentially blocks this. Once the flags are determined, add them to the isOriginPotentiallyTrustworthy method. Then as a followup, we can do bug 1295777 to make nsMixedContentBlocker use isOriginPotentiallyTrustworthy().

Hey Patrick, I just discussed things with Kate and before starting any actual implementation work we wanted to make sure if you agree with our approach. Ultimately our mixedContentBlocker implementation should rely on isOriginPotentiallyTrustWorthy() and ideally we eliminate all unnecessary string comparisons needed to determine whether a specific protocol is trustworthy within that implementation. Would you be fine with us adding a protocolhandler flag URI_IS_POTENTIALLY_TRUSTWORTHY (or similar naming) which we then could simply check within our implementation. I guess what we need to accomplish is a balance act between DOM and NECKO, hence it makes most sense to add a new flag. Would you be willing to r+ such a patch?

I'm ok with this. I apologize for dropping the ball

This patch is a WIP and includes using the URI flags for nsMixedContentBlocker. There are one or two issues with this patch and the test_dv_certificate.py tests, and possibly toolkit/components/passwordmgr/test/mochitest/test_autocomplete_https_upgrade.html. https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a425ea12ed4f2ddab11fbb95d732452616089f0

Comment on attachment 8912412 [details] [diff] [review] Change IsOriginPotentiallyTrustworthy to use protocol flags Review of attachment 8912412 [details] [diff] [review]: ----------------------------------------------------------------- I am fine with that, but since this is critical I would like to have Dan take a look at it as well to make sure we are not missing something.

Btw, can we write some tests for isOriginPotentiallytrustworthy to make sure we are not regressing that? Or do we already have some?

Comment on attachment 8912412 [details] [diff] [review] Change IsOriginPotentiallyTrustworthy to use protocol flags Review of attachment 8912412 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/file/nsHostObjectProtocolHandler.cpp @@ +775,5 @@ > NS_IMETHODIMP > nsBlobProtocolHandler::GetProtocolFlags(uint32_t *result) > { > Unused << nsHostObjectProtocolHandler::GetProtocolFlags(result); > + *result |= URI_IS_LOCAL_RESOURCE | URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT; In theory a blob could have been created by an insecure context and then passed (postMessage?) to a secure context. The secure context shouldn't be able to read the contents (same-origin policy) but could maybe use it in an <img> or <script> tag. Is it insecure or secure? I guess we're saying here "the insecureness, if any, has already happened, and the data is already local". That's no different than a data: url (we have no idea where it came from) so should be fine. ::: netwerk/base/nsIProtocolHandler.idl @@ +302,5 @@ > const unsigned long URI_SYNC_LOAD_IS_OK = (1<<17); > > /** > * URI is secure to load in an https page and should not be blocked > + * by nsMixedContentBlocker - about: and https: currently This comment change doesn't seem right -- this patch adds this flag to far more than about: and https:

(In reply to Daniel Veditz [:dveditz] from comment #7) > Comment on attachment 8912412 [details] [diff] [review] > Change IsOriginPotentiallyTrustworthy to use protocol flags > > Review of attachment 8912412 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/file/nsHostObjectProtocolHandler.cpp > @@ +775,5 @@ > > NS_IMETHODIMP > > nsBlobProtocolHandler::GetProtocolFlags(uint32_t *result) > > { > > Unused << nsHostObjectProtocolHandler::GetProtocolFlags(result); > > + *result |= URI_IS_LOCAL_RESOURCE | URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT; > > In theory a blob could have been created by an insecure context and then > passed (postMessage?) to a secure context. The secure context shouldn't be > able to read the contents (same-origin policy) but could maybe use it in an > <img> or <script> tag. Is it insecure or secure? > > I guess we're saying here "the insecureness, if any, has already happened, > and the data is already local". That's no different than a data: url (we > have no idea where it came from) so should be fine. Absolutely true. We do not currently keep the information as to the original context of the blob, and we already treat it as a local resource, so it already gets this special treatment in the mixed-content blocker. In the current IsOriginPotentiallyTrustworthy, we explicitly reject anything with a blob: scheme, so I think this is safe to remove. https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9512124a91825de0462d91d2ab1cb3d09df3bab

Updated based on feedback from dveditz.

Comment on attachment 8923704 [details] [diff] [review] GTest for various URIs Review of attachment 8923704 [details] [diff] [review]: ----------------------------------------------------------------- Looks already pretty good, pretty incorporate my suggestions and I'll have a final look - should be quick. ::: dom/security/test/gtest/TestSecureContext.cpp @@ +17,5 @@ > + > +struct TestExpectations { > + char uri[kURIMaxLength ]; > + bool expectedResult; > + nsresult expectedReturn; I think the expectedReturn is redundant because it's NS_OK in all the cases within TestExpectations anyway, please remove. @@ +39,5 @@ > + { "http://xyzzy.localhost", false, NS_OK }, > + { "http://127.0.0.1", true, NS_OK }, > + { "resource://xyzzy", true, NS_OK }, > + { "moz-extension://xyzzy", true, NS_OK }, > + { "data:data:text/plain;charset=utf-8;base64,eHl6enk=", false, NS_OK }, probably worth adding * blob * mailto * moz-icon * javascript @@ +46,5 @@ > + uint32_t numExpectations = sizeof(uris) / sizeof(TestExpectations); > + nsCOMPtr<nsIScriptSecurityManager> ssManager = do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID); > + ASSERT_TRUE(!!ssManager); > + nsCOMPtr<nsIContentSecurityManager> csManager = do_GetService(NS_CONTENTSECURITYMANAGER_CONTRACTID); > + ASSERT_TRUE(!!csManager); For the ScriptSecurityManager you could just use nsScriptSecurityManager::GetScriptSecurityManager()->CreadeCodeBase... within the for loop within this test and the other. For nsIContentSecurityManager I think this is the only way to generate a new one. Can we share that object between tests? Basically move the line above to the top and reuse within test 1 and test 2? @@ +48,5 @@ > + ASSERT_TRUE(!!ssManager); > + nsCOMPtr<nsIContentSecurityManager> csManager = do_GetService(NS_CONTENTSECURITYMANAGER_CONTRACTID); > + ASSERT_TRUE(!!csManager); > + nsCOMPtr<nsIIOService> ioService = do_GetIOService(); > + ASSERT_TRUE(!!ioService); You are not using ioService, please remove. @@ +54,5 @@ > + nsresult rv; > + for (uint32_t i = 0; i < numExpectations; i++) { > + nsCOMPtr<nsIPrincipal> prin; > + nsAutoCString uri(uris[i].uri); > + rv = ssManager->CreateCodebasePrincipalFromOrigin(uri, getter_AddRefs(prin)); I think you can just use nsScriptSecurityManager::GetScriptSecurityManager()->CreadeCodeBase... here and also in the other test. @@ +72,5 @@ > + > + nsresult rv; > + nsCOMPtr<nsIPrincipal> sysPrin; > + bool isPotentiallyTrustworthy; > + rv = ssManager->GetSystemPrincipal(getter_AddRefs(sysPrin)); You can simply use nsContentUtils::GetSystemPrincipal() @@ +77,5 @@ > + ASSERT_EQ(rv, NS_OK); > + rv = csManager->IsOriginPotentiallyTrustworthy(sysPrin, &isPotentiallyTrustworthy); > + ASSERT_EQ(rv, NS_OK); > + ASSERT_TRUE(isPotentiallyTrustworthy); > +} Can you also add a Test IsOriginPotentiallyTrustworthyWithNullPrincipal (and potentially even ExpandedPrincipal) so we have test coverage for all of them?

Comment on attachment 8923705 [details] [diff] [review] Change IsOriginPotentiallyTrustworthy to use protocol flags Honza, it seems Pat is not responding. Would you be fine with reviewing this patch?

I think bz should do the review here. I (and Pat too) don't have a super-deep knowledge of how these flags exactly effect sec checks. Boris knows best.

Comment on attachment 8923705 [details] [diff] [review] Change IsOriginPotentiallyTrustworthy to use protocol flags Review of attachment 8923705 [details] [diff] [review]: ----------------------------------------------------------------- hey - I've been stalling on this trying to understand the big picture (and failing). But it appears you don't really need that as much as my go ahead on the actual changes in necko. I'm fine with those - so if you're good with that level of review, I'm ok with the approach.

This patch is kind-of needed by Clear-Site-Data where we should support this cleaning header only for apriori safe URIs. Should we move on with these patches? I think so. I can update the gtest patch.

Comment on attachment 8923705 [details] [diff] [review] Change IsOriginPotentiallyTrustworthy to use protocol flags Review of attachment 8923705 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/file/nsHostObjectProtocolHandler.cpp @@ +757,5 @@ > nsHostObjectProtocolHandler::GetFlagsForURI(nsIURI *aURI, uint32_t *aResult) > { > Unused << nsHostObjectProtocolHandler::GetProtocolFlags(aResult); > if (IsFontTableURI(aURI) || IsBlobURI(aURI)) { > + *aResult |= URI_IS_LOCAL_RESOURCE; // | URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT; the `; // |` should probably go away?

This is just the Kate's patch rebased. bz, do you mind to give the last review?

GTest updated with the ckerschb's comments.

(In reply to Andrea Marchesini [:baku] from comment #15) > This patch is kind-of needed by Clear-Site-Data where we should support this > cleaning header only for apriori safe URIs. > Should we move on with these patches? I think so. I can update the gtest > patch. I am not completely sure I understand the question here. If the question is, should we move on with this bug then the answer is yes. Thanks for taking on the work!

Comment on attachment 8979276 [details] [diff] [review] Change IsOriginPotentiallyTrustworthy to use protocol flags I would really appreciate it if you would describe the intended observable behavior differences this patch is making in the commit message (probably in the form of a bulleted list or something). That would make it way easier to review (e.g. I could compare my understanding of the changes being made to what the intent is) and to do code archeology on later. r- until that information is in the commit message and I am able to use that in the review.

Comment on attachment 8979862 [details] [diff] [review] Change IsOriginPotentiallyTrustworthy to use protocol flags The commit message does not accurately describe the changes this patch makes, as far as I can tell. Before this patch, I believe the following schemes were considered potentially trustworthy: https file resource app moz-extension wss and the following schemes were allowed to get loaded in a secure context without failing mixed-content checks: https about chrome blob moz-icon data file moz-extension resource moz-anno external protocols javascript: ws wss This patch adds "moz-icon" to the first list, no? And removes "app"? Why those changes? The second list does seem to be unchanged, because a bunch of the things that are getting the new flag added were already whitelisted due to having other flags. I'm not sure I follow the blob bit too. I can see the argument for why adding this flag to blob does not hurt (because we're asserting we don't have a blob URL in IsOriginPotentiallyTrustworthy anyway), but why is the flag addition needed?

Protocol app has been removed by bug 1285170. I'm working on the moz-icon, and improve comments.

More than a review request, this is a feedback request. Or something in between.

Comment on attachment 8980349 [details] [diff] [review] Change IsOriginPotentiallyTrustworthy to use protocol flags >Before, the trusted URI schemes were: https, file, resource, app, >moz-extension and wss. The same behavior is kept by this patch, adding >URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT flag here: OK. So I feel pretty strongly that we should rename that flag to indicate its real meaning. Probably to URI_IS_POTENTIALLY_TRUSTWORTHY. Also, the behavior is not being kept by the patch; more on this below. >. nsFileProtocolHandler >. nsResProtocolHandler >. ExtensionProtocolHandler >. nsBlobProtocolHandler: see https://bugzilla.mozilla.org/show_bug.cgi?id=1328695#c7 >. BaseWebSocketChannel (if encrypted) I don't think we should have this list which just duplicates the patch logic. Probably what the commit message should say is something like this: Before this change, the trusted URI schemes, based on a string whitelist, were: https, file, resource, app, moz-extension and wss. This change removes "app" from the list (since we don't implement it), and adds "about" to the list (because we control the delivery of that). It's still not 100% clear to me why we're adding "blob" to the definition. Again, the linked-to comment explains why it probably doesn't hurt, but the code involved never reaches "blob" at all. And in spec terms, marking blob this way is a bit odd. I'd really like to understand what's going on there. Put another way, what breaks, if anything, if we do not add the flag there. If nothing, I think we shouldn't add it. >+++ b/netwerk/base/nsIProtocolHandler.idl >+ * The protocol for this URI is considered a priori safe to load in an >+ * https page and should not be blocked. Well, no. What this flag _really_ means after this change is that all origins whose URI has this scheme are considered potentially trustworthy. The effect on "loading in an https page" is sort of a side-effect; lots of things that are not "Potentially trustworthy" we allow loading in a secure context, afaict. For example "data:".

bz, thanks for your help. I applied all your comments (I hope so). In particular, I removed the blob changes: if needed, they will go in a follow up.

Comment on attachment 8981442 [details] [diff] [review] Change IsOriginPotentiallyTrustworthy to use protocol flags r=me. Thank you for bearing with the back-and-forth, and sorry for the lag...

Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/aa4238ab9742 Use protocol flags to determine if a URI is potentially trustworthy r=ckerschb, r=dveditz, r=mcmanus, r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/dada0c7fbc6f GTests for isOriginPotentiallyTrustworthy, r=ckerschb

https://hg.mozilla.org/mozilla-central/rev/aa4238ab9742 https://hg.mozilla.org/mozilla-central/rev/dada0c7fbc6f

Comment on attachment 8981442 [details] [diff] [review] Change IsOriginPotentiallyTrustworthy to use protocol flags Hi baku, Does URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT still exist? Do we need it? Can you add in comments somewhere some of the protocols considered for URI_IS_POTENTIALLY_TRUSTWORTHY. i.e. the applicable protocols you removed from this if statement: - if (scheme.EqualsLiteral("https") || - scheme.EqualsLiteral("file") || - scheme.EqualsLiteral("resource") || - scheme.EqualsLiteral("app") || - scheme.EqualsLiteral("moz-extension") || - scheme.EqualsLiteral("wss")) {

> Does URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT still exist? Do we need it? No, It has been renamed to URI_IS_POTENTIALLY_TRUSTWORTHY > Can you add in comments somewhere some of the protocols considered for > URI_IS_POTENTIALLY_TRUSTWORTHY. i.e. the applicable protocols you removed > from this if statement: I can extend the comment here: netwerk/base/nsIProtocolHandler.idl follow up.