1328695 - isOriginPotentiallyTrustworthy should consider URI Flags

Status: RESOLVED FIXED

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

Description

[Tanvi Vyas[:tanvi]]

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().

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

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?

Comment 2

[Patrick McManus [:mcmanus]]

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

Comment 3

[Kate McKinley [:kmckinley, :Kate]]

Attachment: WIP for using URI flags in IsOriginPotentiallyTrustworthy

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 4

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Change IsOriginPotentiallyTrustworthy to use protocol flags

https://treeherder.mozilla.org/#/jobs?repo=try&revision=edd9cad6614188e5f1cbf2c3b0249bd6d8e69677

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 7

[Daniel Veditz [:dveditz]]

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:

Comment 8

[Kate McKinley [:kmckinley, :Kate]]

(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

Comment 9

[Kate McKinley [:kmckinley, :Kate]]

Attachment: GTest for various URIs

Comment 10

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Change IsOriginPotentiallyTrustworthy to use protocol flags

Updated based on feedback from dveditz.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

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 12

[Christoph Kerschbaumer [: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?

Comment 13

[Honza Bambas (:mayhemer)]

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 14

[Patrick McManus [:mcmanus]]

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.

Comment 15

[Andrea Marchesini [:baku]]

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 16

[Honza Bambas (:mayhemer)]

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?

Comment 17

[Andrea Marchesini [:baku]]

Attachment: Change IsOriginPotentiallyTrustworthy to use protocol flags

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

Comment 18

[Andrea Marchesini [:baku]]

Attachment: GTest for various URIs

GTest updated with the ckerschb's comments.

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

(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 20

[Boris Zbarsky [:bzbarsky]]

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 21

[Andrea Marchesini [:baku]]

Attachment: Change IsOriginPotentiallyTrustworthy to use protocol flags

Comment 22

[Boris Zbarsky [: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?

Comment 23

[Andrea Marchesini [:baku]]

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

Comment 24

[Andrea Marchesini [:baku]]

Attachment: Change IsOriginPotentiallyTrustworthy to use protocol flags

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

Comment 25

[Boris Zbarsky [: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:".

Comment 26

[Andrea Marchesini [:baku]]

Attachment: Change IsOriginPotentiallyTrustworthy to use protocol flags

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 27

[Boris Zbarsky [: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...

Comment 28

[Pulsebot]

Pushed by [e-mail]:
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

Comment 29

[Noemi Erli[:noemi_erli]]

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

Comment 30

[Tanvi Vyas[:tanvi]]

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")) {

Comment 31

[Andrea Marchesini [:baku]]

> 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.