Closed Bug 1328695 Opened 3 years ago Closed Last year

isOriginPotentiallyTrustworthy should consider URI Flags

Categories

(Core :: DOM: Security, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: tanvi, Assigned: baku)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [domsecurity-active])

Attachments

(2 files, 7 obsolete files)

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().
Summary: isOriginPotentiallyTrustworthy shoudl consider URI Flags → isOriginPotentiallyTrustworthy should consider URI Flags
Priority: -- → P2
Whiteboard: [domsecurity-active]
Assignee: nobody → kmckinley
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?
Flags: needinfo?(mcmanus)
I'm ok with this. I apologize for dropping the ball
Flags: needinfo?(mcmanus)
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
Attachment #8911277 - Flags: review?(tanvi)
Attachment #8911277 - Flags: review?(mcmanus)
Status: NEW → ASSIGNED
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.
Attachment #8912412 - Flags: review?(dveditz)
Attachment #8912412 - Flags: review?(ckerschb)
Attachment #8912412 - Flags: review+
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:
Attachment #8912412 - Flags: review?(dveditz) → review+
(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
Attached patch GTest for various URIs (obsolete) — Splinter Review
Attachment #8923704 - Flags: review?(ckerschb)
Updated based on feedback from dveditz.
Attachment #8912412 - Attachment is obsolete: true
Attachment #8912412 - Flags: review?(mcmanus)
Attachment #8923705 - Flags: review?(mcmanus)
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?
Attachment #8923704 - Flags: review?(ckerschb)
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?
Attachment #8923705 - Flags: review?(honzab.moz)
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.
Attachment #8923705 - Flags: review?(mcmanus)
Attachment #8923705 - Flags: review?(honzab.moz)
Attachment #8923705 - Flags: review+
Assignee: kate+bugzilla → nobody
Status: ASSIGNED → NEW
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.
Flags: needinfo?(ckerschb)
Blocks: 1268889
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?
Attachment #8923705 - Attachment is obsolete: true
Attachment #8979276 - Flags: review?(bzbarsky)
GTest updated with the ckerschb's comments.
Attachment #8923704 - Attachment is obsolete: true
Assignee: nobody → amarchesini
(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!
Flags: needinfo?(ckerschb)
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.
Attachment #8979276 - Flags: review?(bzbarsky) → review-
Attachment #8979276 - Attachment is obsolete: true
Attachment #8979862 - Flags: review?(bzbarsky)
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?
Flags: needinfo?(amarchesini)
Protocol app has been removed by bug 1285170.
I'm working on the moz-icon, and improve comments.
Flags: needinfo?(amarchesini)
More than a review request, this is a feedback request. Or something in between.
Attachment #8979862 - Attachment is obsolete: true
Attachment #8979862 - Flags: review?(bzbarsky)
Attachment #8980349 - Flags: review?(bzbarsky)
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:".
Attachment #8980349 - Flags: review?(bzbarsky) → feedback+
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.
Attachment #8980349 - Attachment is obsolete: true
Attachment #8981442 - Flags: review?(bzbarsky)
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...
Attachment #8981442 - Flags: review?(bzbarsky) → review+
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
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
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")) {
Flags: needinfo?(amarchesini)
> 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.
Flags: needinfo?(amarchesini)
You need to log in before you can comment on or make changes to this bug.