1254488 - [Presentation WebAPI] check mixed security context policy while starting a PresentationRequest

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Need to implement the start algorithm in latest version:

Using the document's settings object, run the prohibits mixed security contexts algorithm.
If the result of the algorithm is "Prohibits Mixed Security Contexts" and presentationUrl is an a priori unauthenticated URL, then return a Promise rejected with a SecurityError.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

CCing Christoph and Tanvi since they've worked on mixed content stuff.
I think I've lost track on the recent changes where explicit content policy [1] checks have been converted to happen automatically when an nsIChannel is created.
Though, there are still some manual NS_CheckContentLoadPolicy calls.


[1] nsMixedContentBlocker is implemented as an nsIContentPolicy.

Comment 2

[Kershaw Chang [:kershaw]]

Hi bz,

I have some questions about implementing the prohibits mixed security contexts algorithm[1]. Since jwatt seems not available now, I may need your opinions. Thanks.

My questions are:

1. According to bug 1220687, implementing HTTPS state seems really difficult. Currently, the only option we have is HttpsStateIsModern[2]. However, this function is not able to tell if the HTTPS state is "deprecated". What should I do if I want to know if the HTTPS state is not "none" as described in [1]?

2. What does the environment settings object stand for in Gecko? Is it the inner window?

3. How to get the responsible document of settings? Is it the nsIDocument?

[1] https://w3c.github.io/webappsec-mixed-content/#categorize-settings-object
[2] http://searchfox.org/mozilla-central/rev/3df383b8552c1f8059f5c21258388ddb5a2f33d0/dom/base/nsGlobalWindow.cpp#2390

Comment 3

[Boris Zbarsky [:bzbarsky]]

> What should I do if I want to know if the HTTPS state is not "none" as described in [1]?

This is supposed to be the thing used for mixed content blocking, right?  Check with Tanvi?

> 2. What does the environment settings object stand for in Gecko? Is it the inner window?

It depends.  In general, it's the global.  In a window context that's the inner window.  In a worker it's the worker global.  We don't have a class that actually represents a settings object.

> 3. How to get the responsible document of settings? Is it the nsIDocument?

GetDoc() on the relevant inner window.

Comment 4

[Kershaw Chang [:kershaw]]

Attachment: Implement mixed content security check in PresentationRequest

This patch basically reuses [1] to check the HTTPS state for the prohibits mixed security contexts algorithm.

Not sure if I understand the spec well since I am not familiar with those security stuff. Please take a quick look. Thanks.

[1] http://searchfox.org/mozilla-central/rev/d9a04f71630ce4203ff0a5e26722723045d035b5/dom/base/nsGlobalWindow.cpp#2390

Comment 5

[Tanvi Vyas[:tanvi]]

nsMixedContentBlocker.cpp does Mixed Content checks on subresource loads in Gecko.  When a channel is created, the security flags set in the channel's loadinfo determine if Mixed Content Blocker is called.  Where are you creating your channel for the Presentation Request?  What Content Type is assigned to the load (http://searchfox.org/mozilla-central/source/dom/base/nsIContentPolicyBase.idl)?

There are two types of mixed content... mixed active content and mixed passive content.  By default, mixed content blocker will block http active content on https pages.  Mixed passive content will be allowed, but the security UI of the page will be degraded.  For Presentations, do you want to block all types of mixed content (active and passive)?  Is the url bar or security indicators still present during a presentation?

To determine the security state of a page, you can read the nsIWebProgressListener flags off of the state object in the docshell's securityUI object.  It looks like a state of STATE_IS_SECURE would map to "modern", and STATE_IS_BROKEN may map to "deprecated".  STATE_IS_BROKEN can mean that mixed content loaded on the page or there is something wrong with the connection/certificate.  To distinguish this, there are additional web progress listener flags you can check - https://dxr.mozilla.org/mozilla-central/source/uriloader/base/nsIWebProgressListener.idl#253

But it seems like you want to check whether mixed content could be a potential issue even before a subresource request is made?  Is that right?  For a general document, that is hard to determine because just because a top level page is HTTP, doesn't mean that it won't have an HTTPS frame within it that would be subject to mixed content checks.  But maybe that isn't an issue for Presentations.

Mixed Content Blocker was written before the spec, so we are implementation doesn't map exactly what is speced.  It, for example, doesn't use IsOriginPotentiallyTrustworthy.

I'm not sure I've answered your questions here, but I'm unclear on exactly what you are trying to do.  So with more info, I may be able to help you more.

Thanks!

Comment 6

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8778127 [details] [diff] [review]
Implement mixed content security check in PresentationRequest

Review of attachment 8778127 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/presentation/PresentationRequest.cpp
@@ +353,5 @@
> +    return true;
> +  }
> +
> +  bool result = false;
> +  aDocument->EnumerateSubDocuments(HttpsStateInSubDocumentCallback, &result);

HttpStateInSubDocumentCallback is simple enough to be written as a lambda function.

@@ +358,5 @@
> +  return result;
> +}
> +
> +bool
> +PresentationRequest::IsPrioriAuthenticatedURL(const nsAString& aUrl)

Maybe we could put this function in nsIContentSecurityManager.idl

::: dom/presentation/tests/mochitest/test_presentation_mixed_security_contexts.html
@@ +127,5 @@
> +  then(testStart).
> +  then(testReconnect).
> +  then(testGetAvailabilityForAboutBlank).
> +  then(testGetAvailabilityForAboutSrcdoc).
> +  then(teardown);

add a test for data URL?

::: dom/presentation/tests/mochitest/test_presentation_sender_startWithDevice.html
@@ +19,5 @@
>  var connection;
>  
>  function testSetup() {
>    return new Promise(function(aResolve, aReject) {
> +    request = new PresentationRequest("about:blank");

use "https://example.com" instead?

Comment 7

[Kershaw Chang [:kershaw]]

(In reply to Tanvi Vyas [:tanvi] from comment #5)

Thanks for such a detailed explanation!

> nsMixedContentBlocker.cpp does Mixed Content checks on subresource loads in
> Gecko.  When a channel is created, the security flags set in the channel's
> loadinfo determine if Mixed Content Blocker is called.  Where are you
> creating your channel for the Presentation Request?  What Content Type is
> assigned to the load
> (http://searchfox.org/mozilla-central/source/dom/base/nsIContentPolicyBase.
> idl)?
> 

According to the spec [1], I just want to know if the settings want to prohibit mixed security contexts.

> There are two types of mixed content... mixed active content and mixed
> passive content.  By default, mixed content blocker will block http active
> content on https pages.  Mixed passive content will be allowed, but the
> security UI of the page will be degraded.  For Presentations, do you want to
> block all types of mixed content (active and passive)?  Is the url bar or
> security indicators still present during a presentation?
> 

In a presentation session, the content is usually loaded at receiver side. I think the mixed content blocker at receiver side plays the role to block mixed contents by default.

> To determine the security state of a page, you can read the
> nsIWebProgressListener flags off of the state object in the docshell's
> securityUI object.  It looks like a state of STATE_IS_SECURE would map to
> "modern", and STATE_IS_BROKEN may map to "deprecated".  STATE_IS_BROKEN can
> mean that mixed content loaded on the page or there is something wrong with
> the connection/certificate.  To distinguish this, there are additional web
> progress listener flags you can check -
> https://dxr.mozilla.org/mozilla-central/source/uriloader/base/
> nsIWebProgressListener.idl#253
> 

Thanks for the information. Currently, I use [2] to determine the HTTPS state. Hope this is an acceptable solution.

> But it seems like you want to check whether mixed content could be a
> potential issue even before a subresource request is made?  Is that right? 

As I mentioned above, I want to implement [1] which only checks whether to prohibit mixed content.  


[1] https://w3c.github.io/webappsec-mixed-content/#categorize-settings-object
[2] http://searchfox.org/mozilla-central/rev/3df383b8552c1f8059f5c21258388ddb5a2f33d0/dom/base/nsGlobalWindow.cpp#2390

Comment 8

[Kershaw Chang [:kershaw]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #6)
> Comment on attachment 8778127 [details] [diff] [review]
> Implement mixed content security check in PresentationRequest
> 
> Review of attachment 8778127 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/presentation/PresentationRequest.cpp
> @@ +353,5 @@
> > +    return true;
> > +  }
> > +
> > +  bool result = false;
> > +  aDocument->EnumerateSubDocuments(HttpsStateInSubDocumentCallback, &result);
> 
> HttpStateInSubDocumentCallback is simple enough to be written as a lambda
> function.
> 

Thanks for the nice suggestion.

> @@ +358,5 @@
> > +  return result;
> > +}
> > +
> > +bool
> > +PresentationRequest::IsPrioriAuthenticatedURL(const nsAString& aUrl)
> 
> Maybe we could put this function in nsIContentSecurityManager.idl

I am not sure about this since it seems this is only used in PresentationRequest.
Let's see what does smaug think.

> 
> :::
> dom/presentation/tests/mochitest/test_presentation_mixed_security_contexts.
> html
> @@ +127,5 @@
> > +  then(testStart).
> > +  then(testReconnect).
> > +  then(testGetAvailabilityForAboutBlank).
> > +  then(testGetAvailabilityForAboutSrcdoc).
> > +  then(teardown);
> 
> add a test for data URL?
> 

Added.

> :::
> dom/presentation/tests/mochitest/test_presentation_sender_startWithDevice.
> html
> @@ +19,5 @@
> >  var connection;
> >  
> >  function testSetup() {
> >    return new Promise(function(aResolve, aReject) {
> > +    request = new PresentationRequest("about:blank");
> 
> use "https://example.com" instead?

Nice suggestion! Thanks.

Comment 9

[Kershaw Chang [:kershaw]]

Attachment: Implement mixed content security check in PresentationRequest - v2

Please also see comment #4. Thanks.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8779246 [details] [diff] [review]
Implement mixed content security check in PresentationRequest - v2

>+  bool result = false;
>+  aDocument->EnumerateSubDocuments(
>+    [](nsIDocument* aDocument, void* aData) -> bool {
>+      bool *res = static_cast<bool*>(aData);
>+      if (nsContentUtils::HttpsStateIsModern(aDocument)) {
>+        *res = true;
>+      }
>+      return !*res;
>+    },
>+    &result);
>+
This is not what the spec says.I admit, it is hard to interpret)
Don't you need to go from aDocument to its ancestors (but in Gecko case not to chrome doc),
not to sub documents.

Comment 11

[Kershaw Chang [:kershaw]]

(In reply to Olli Pettay [:smaug] from comment #10)
> Comment on attachment 8779246 [details] [diff] [review]
> Implement mixed content security check in PresentationRequest - v2
> 
> >+  bool result = false;
> >+  aDocument->EnumerateSubDocuments(
> >+    [](nsIDocument* aDocument, void* aData) -> bool {
> >+      bool *res = static_cast<bool*>(aData);
> >+      if (nsContentUtils::HttpsStateIsModern(aDocument)) {
> >+        *res = true;
> >+      }
> >+      return !*res;
> >+    },
> >+    &result);
> >+
> This is not what the spec says.I admit, it is hard to interpret)
> Don't you need to go from aDocument to its ancestors (but in Gecko case not
> to chrome doc),
> not to sub documents.

Thanks for pointing this out.
I've got it wrong completely.

Comment 12

[Kershaw Chang [:kershaw]]

Attachment: Implement mixed content security check in PresentationRequest - v3

Fixed the previous comment.

Please take a look again.

Comment 13

[Kershaw Chang [:kershaw]]

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

Comment 14

[Kershaw Chang [:kershaw]]

Attachment: Implement mixed content security check in PresentationRequest, r=smaug

Carry r+.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/efe097e5399a
Run the the prohibits mixed security contexts algorithm and check a priori unauthenticated URL in PresentationRequest, r=smaug

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/efe097e5399a