Doug Turner (:dougt) Reporter
	Description • 9 years ago

This is a meta bug.

Currently in a few places in our code, we are testing for the scheme 'https' to indicate that the content is loaded over a secure channel.  However, 'https' includes self-signed certs which have a difference security priority than trusted end-to-end verified https.

For example, when I load a service worker using a self signed certificate in Firefox, it works fine.  However in Chrome:

Uncaught (in promise) DOMException: Failed to register a ServiceWorker: An SSL certificate error occurred when fetching the script.

It looks like Chrome is protecting the user against a possible mitm/self-signed cert.

We should audit the places where we do these scheme-only tests, and instead ensure that certificate is value and isn't self-signed.

	Steve Workman [:sworkman] (INACTIVE)
	Comment 1 • 9 years ago

Flagging this for Richard.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)
	Comment 2 • 9 years ago

Is there a nice API that I can query which will check the cert and give me a nsresult back?

	Steve Workman [:sworkman] (INACTIVE)
	Comment 3 • 9 years ago

Adding keeler to needinfo - either rbarnes or keeler can answer comment #2.

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 4 • 9 years ago

I'm a little confused about the situation here. Has necko already successfully connected to the host providing the resources? If so, then it should be a secure connection. The one caveat is if the user has added a certificate error override. If that's what you want to check for, you should be able to use nsICertOverrideService.getValidityOverride().

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)
	Comment 5 • 9 years ago

In ServiceWorkerManager, it has not already connected or established a channel. I'd like to ask the current document (nsPIDOMWindow/nsDocument) if it was served from a valid SSL cert rather than just getting the document's scheme and comparing to https - https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp?from=ServiceWorkerManager.cpp&case=true#1082

	Richard Barnes [:rbarnes]
	Comment 6 • 9 years ago

I agree with keeler that this bug is incorrect as stated.  If a resource has loaded over HTTPS, it is trusted, either through our built in trust settings or the user's.

However, it does seem like the definition of a secure context is changing [1], so we should have a method that tells the caller whether a given principal represents a secure context.  I have re-titled the bug to reflect this.

	Richard Barnes [:rbarnes]
	Comment 7 • 9 years ago

(In reply to Richard Barnes [:rbarnes] from comment #6)
> However, it does seem like the definition of a secure context is changing
> [1],

[1] http://www.w3.org/TR/powerful-features/

	Ben Kelly [:bkelly, not reviewing]
	Comment 8 • 9 years ago

(In reply to Richard Barnes [:rbarnes] from comment #6)
> However, it does seem like the definition of a secure context is changing
> [1], so we should have a method that tells the caller whether a given
> principal represents a secure context.  I have re-titled the bug to reflect
> this.

Note, a principal is not enough.  The algorithm in the spec requires knowing if your parent context is trusted as well.  So you really need something like a document or worker to determine trust.

	Andrew Overholt [:overholt]
	Comment 9 • 9 years ago

We decided we were good for our initial shipping of Service Worker without this.

	Andrew Overholt [:overholt]
	Comment 10 • 9 years ago

Missed blocking the post-SW-v1 tracker.

	Jesse Ruderman
	Comment 13 • 9 years ago

> +  if (host.Equals("127.0.0.1") ||
> +      host.Equals("localhost") ||
> +      host.Equals("::1")) {

Treating "127.0.0.1" as secure sounds good, but "localhost" may be dangerous:
https://twitter.com/sleevi_/status/649025202722967553

	Jonathan Watt [:jwatt] Assignee
	Comment 14 • 9 years ago

Good to know, thanks for taking a look.

	(no longer active)
	Comment 15 • 9 years ago

In nsDNSService2.cpp, we seem to treat "localhost" specially by setting the RESOLVE_OFFLINE flag, which is supposed to mean that we won't contact a DNS server to resolve the name, but as far as I can tell, nothing ever checks that flag.  Please check with Necko folks what's going on.

But more importantly, https://tools.ietf.org/html/rfc6761 suggests that we can treat "localhost" specially.  If we don't want to do that, we may need to resolve the name and compare the resolved address to 127.0.0.1 or some such (but I'm not an expert on this stuff.)

	Jonathan Watt [:jwatt] Assignee
	Comment 16 • 9 years ago

Thanks, Ehsan. I asked about the RESOLVE_OFFLINE flag in bug 87717 comment 59.

	Ben Kelly [:bkelly, not reviewing]
	Comment 17 • 9 years ago

It seems the patches here so far are document-only.  I think we need an API solution for APIs used on Workers as well.

For example, Cache API can be used on window via window.caches or Workers via self.caches.  Cache API should only be usable on secure origins.  We need a way to know if the worker is secure.

	Patrick McManus [:mcmanus]
	Comment 18 • 9 years ago

(In reply to [Vacation: Nov 3-19] from comment #15)
> In nsDNSService2.cpp, we seem to treat "localhost" specially by setting the
> RESOLVE_OFFLINE flag, which is supposed to mean that we won't contact a DNS
> server to resolve the name, but as far as I can tell, nothing ever checks
> that flag.  Please check with Necko folks what's going on.

that code just allows you to connect to localhost resources in offline mode.

	Jonathan Watt [:jwatt] Assignee
	Comment 19 • 9 years ago

(In reply to Ben Kelly [:bkelly] from comment #17)
> It seems the patches here so far are document-only.  I think we need an API
> solution for APIs used on Workers as well.

We do, but I think that should happen after we've sorted out the API for Window. (The implementation for WorkerGlobalScope will consume the Window API.)

	Jonathan Watt [:jwatt] Assignee
	Comment 20 • 9 years ago

This is blocked on the work in bug 1220687 which will provide API to get the 'HTTPS state' that algorithm for secure contexts depends on.

	Jonathan Watt [:jwatt] Assignee
	Comment 21 • 9 years ago

Comment on attachment 8674302 [details] [diff] [review]
part 1 - Implement nsIDocument::SettingsObjectIsSecureContext

This doesn't work when a document was created from a data: URI, javascript: URI, <iframe srcdoc>, document.write into e.g. about:blank, etc.

Supporting cases like these is basically why the 'HTTPS state' and 'secure context' concepts are so much more complicated than you might think at first.

	Jonathan Watt [:jwatt] Assignee
	Comment 22 • 9 years ago

(In reply to Jonathan Watt [:jwatt] from comment #21)
> This doesn't work when a document was created from a data: URI, javascript:
> URI, <iframe srcdoc>, document.write into e.g. about:blank, etc.

If you're waiting for this bug to be fixed but you don't care about that, consider using the temporary API being introduced in bug 1221365.

	Jonathan Watt [:jwatt] Assignee
	Comment 23 • 8 years ago

Bug 1220687 is a bit of a pain to implement, so Jet and I are wondering if we can go with a basic implementation of Window.isSecureContext() for now to free up the projects that are currently waiting on this feature. We can start with a conservative implementation then once bug 1220687 is fixed we can use that code and get the real implementation.

	Jonathan Watt [:jwatt] Assignee
	Comment 24 • 8 years ago

Comment on attachment 8717952 [details] [diff] [review]
patch to implement basic support

Boris, does the "This is a hack" bit look reasonable to you? If I've got this right this should follow the spec but may return false in some cases where it should return true (i.e. be conservative).

	Boris Zbarsky [:bzbarsky]
	Comment 25 • 8 years ago

Comment on attachment 8717952 [details] [diff] [review]
patch to implement basic support

I just looked at nsGlobalWindow::UpdateIsSecureContextState, nothing else, really.

1)  IsChromeWindow() tells you absolutely nothing about principals.  You can have a non-system-principal thing loaded in IsChromeWindow(), and vice versa.  For example, about:config is a system-principal thing, but the window it's loaded is is not IsChromeWindow().  I don't think we should be using IsChromeWindow() at all in this function.

2)  GetFirstNonSrcdocCreator is almost certainly buggy.

3)  Setting mIsSecureContext to true for a srcdoc is a bit weird.  At least assuming we plan to support the document.domain thing and hence allow dynamic changes in the secure context state on the parent of the srcdoc.

For the rest, this seems like an ok approximation of the spec.  It gets some things wrong (e.g. sandboxed http://localhost) but it's not clear to me that we're implementing those cases correctly in the non-sandboxed case either.

	Jonathan Watt [:jwatt] Assignee
	Comment 26 • 8 years ago

(In reply to Boris Zbarsky [:bz] from comment #25)
> 1)  IsChromeWindow() tells you absolutely nothing about principals.  You can
> have a non-system-principal thing loaded in IsChromeWindow(), and vice
> versa.  For example, about:config is a system-principal thing, but the
> window it's loaded is is not IsChromeWindow().  I don't think we should be
> using IsChromeWindow() at all in this function.

Good to know, thanks. Do we ever create chrome windows for documents loaded over the network though?

> 2)  GetFirstNonSrcdocCreator is almost certainly buggy.

I've fixed some issues and filed https://github.com/w3c/webappsec-secure-contexts/issues/18 - for now I'm just playing it safe and making popups non-secure contexts until this stuff is resolved.

> 3)  Setting mIsSecureContext to true for a srcdoc is a bit weird.

For that patch, if IsSrcdocDocument() returns true we know that |creator| is non-null, and the check above means that we would have returned already if creator wasn't a secure context. Admittedly that isn't completely obvious. I've tried to make the code clearer.

> At least
> assuming we plan to support the document.domain thing and hence allow
> dynamic changes in the secure context state on the parent of the srcdoc.

We should do that, but I think I'll do that as a follow-up.

> For the rest, this seems like an ok approximation of the spec.  It gets some
> things wrong (e.g. sandboxed http://localhost) but it's not clear to me that
> we're implementing those cases correctly in the non-sandboxed case either.

Yeah, this is definitely not perfect, but as long as isSecureContext isn't true when it shouldn't be I hope this is a reasonable approximation to unblock the features that are waiting on this (at least in the common usecases).

	Boris Zbarsky [:bzbarsky]
	Comment 28 • 8 years ago

> Do we ever create chrome windows for documents loaded over the network though?

Nothing prevents us from doing that.  I'm pretty sure some addons end up doing so, probably mostly by accident.

> Yeah, this is definitely not perfect, but as long as isSecureContext isn't true when it shouldn't be

The problem is that if it's false when it should be, then people will expect APIs to be present and they won't be, right?  I guess this is maybe not an issue if they're feature-detecting anyway... but I'm not sure how much I want to depend on that.  :(

	Boris Zbarsky [:bzbarsky]
	Comment 29 • 8 years ago

I'm not going to manage to get to this today, at least not if I want to actually do it right.  Will do on Monday.

	Boris Zbarsky [:bzbarsky]
	Comment 30 • 8 years ago

Comment on attachment 8723079 [details] [diff] [review]
patch

I think the "set to false if we have an opener" means that it's not really web-compatible to actually restrict any existing API to secure contexts in practice, right?  That needs to be _really_ carefully documented somewhere...

Also, it's weird that disowning your opener will bypass this check.

>+      // We don't want to walk up into chrome windows

Then why aren't you using GetScriptableParent?  But note that this will never return null; it'll return the thing whose parent you're getting when you've reached the root.

I really don't think we should be trying to reinvent GetScriptableParent here, in any case.

>+  // this issues is sorted out we set mIsSecureContext to false if we have an

"these issues are sorted out"?

>+    MOZ_ASSERT(doc, "We can't exist if our creator window doesn't have a doc");

That's not clear to me, actaully.  If our parent window is in an iframe that's been removed from the document, then its mDoc might be nulled out by now...  Worth testing, at least.

>+  // XXXjwatt FORWARD_TO_INNER_CREATE or MOZ_RELEASE_ASSERT(IsInnerWindow())?

The latter, I would think.

>+  void UpdateIsSecureContextState();
>+  bool IsSecureContext() const;

The second one needs to be public, but the first one should be at least protected, right?

I think the thing that really bothers me here is that I can't tell how ok this implementation is.  It _really_ depends on exactly how we plan to use this, and I'm worried about allowing in an implementation that we know is all sorts of broken and then people starting to use it on the assumption that it works.

So what _do_ we plan to use it for?

	Jonathan Watt [:jwatt] Assignee
	Comment 31 • 8 years ago

I've been making good progress on bug 1220687 lately, so I'll likely be leveraging that and will change the implementation here (again) to be complete, or close enough.

I'll answer some of the questions from above all the same.

(In reply to Boris Zbarsky [:bz] from comment #30)
> I think the "set to false if we have an opener" means that it's not really
> web-compatible to actually restrict any existing API to secure contexts in
> practice, right?  That needs to be _really_ carefully documented somewhere...
> 
> Also, it's weird that disowning your opener will bypass this check.

Hmm, yeah, the opener can potentially still access the popup (or can regain access to it), so that looks like a bug in the spec.

> I think the thing that really bothers me here is that I can't tell how ok
> this implementation is.  It _really_ depends on exactly how we plan to use
> this, and I'm worried about allowing in an implementation that we know is
> all sorts of broken and then people starting to use it on the assumption
> that it works.

An overly conservative implementation would be an issue for shipped features if we retrospectively put them behind isSecureContext, but less so for new features that start out that way.

As to whether this patch is all sorts of broken, the difference to the spec would be for documents that are not created from a direct network request. In this case the patche would have us check the origin for HTTPS (or creation URL in the case of sandboxed iframe) instead of checking the HTTPS state as the spec would have us do. I think either approach should give the same result except when a sandboxed, non-direct-network-requested document doesn't have a creation URL from which we can determine HTTPS'edness (e.g. srcdoc, data: URI, etc.). Anyway, I'll keep working on bug 1220687.

> So what _do_ we plan to use it for?

It's not clear to me who in Mozilla is planning on putting what shipped features behind isSecureContext. A search of the specs shows that at least the following refer to the Secure Contexts spec:

https://slightlyoff.github.io/ServiceWorker/spec/service_worker/
https://storage.spec.whatwg.org/
https://w3c.github.io/encrypted-media/
https://w3c.github.io/geofencing-api/
https://w3c.github.io/webappsec-mixed-content/
https://w3c.github.io/webappsec-subresource-integrity/
https://w3c.github.io/sensors/
https://w3c.github.io/web-nfc/
https://w3c.github.io/webappsec-clear-site-data/
https://w3c.github.io/webappsec-credential-management/
https://w3ctag.github.io/client-certificates/
https://webbluetoothcg.github.io/web-bluetooth/
https://wicg.github.io/BackgroundSync/
https://wicg.github.io/directory-upload/
https://wicg.github.io/paymentrequest/
https://wicg.github.io/web-payments-browser-api/
https://wicg.github.io/webusb/
https://www.w3.org/Submission/fido-web-api/
https://www.w3.org/TR/permissions/

Before spending a lot of time digging into those, figuring out what we've shipped, what shipped features we might put behind isSecureContext, and how likely those features are to have been used in sandboxed documents that didn't come from a direct network request, I think I'm going to push on with bug 1220687 and make the point mute.

	Jonathan Watt [:jwatt] Assignee
	Comment 32 • 8 years ago

More specifically, this is where I am going. In this patch I use nsIChannel::GetSecurityInfo() to determine HTTPS state (in bug 1220687 I'm working on propagating security info objects onto channels), but whether we keep using nsIChannel::GetSecurityInfo or add a GetHTTPSState method to nsIChannel the patch would be largely the same.

(bz: for your reference this implementation uses the proposed new algorithm that I send to you and Mike.)

	Jonathan Watt [:jwatt] Assignee
	Comment 33 • 8 years ago

Comment on attachment 8727886 [details] [diff] [review]
patch - implementation that uses HTTPS state

The recent discussions on exactly how [SecureContext] should work:

  https://lists.w3.org/Archives/Public/public-script-coord/2016JanMar/thread.html#msg104

seems to have resolved that this extended attribute should hide API when we're not in a Secure Context (rather than having that API throw).  That means that we need to determine much earlier than I've been doing whether a new Window is a Secure Context or not. Specifically, we need to determine that state before the prototype objects for the Window are set up. In code terms this means we need to know our state by the time nsGlobalWindow::SetNewDocument calls CreateNativeGlobalForInner.

	Jonathan Watt [:jwatt] Assignee
	Comment 34 • 8 years ago

The WebIDL API exposure-controlling code (PrefableDisablers::isEnabled) is only given a JSContext and JSObject, which gives us access to a JSCompartment. Boris suggested that it would be best to store the is-secure-context state on the compartment so that checking this state works the same way both for workers and on the main thread. Here's a patch to add a member to CompartmentCreationOptions to store this flag.

	Luke Wagner [:luke]
	Comment 35 • 8 years ago

Comment on attachment 8734236 [details] [diff] [review]
part 1 - Allow CompartmentCreationOptions to store Secure Context state.

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

forwarding to module owner

	Jonathan Watt [:jwatt] Assignee
	Comment 36 • 8 years ago

I was asked on IRC why hiding isn't a broken approach given that initial about:blank means that Secure Context state is not Window-invariant, and we won't realistically be able to change prototype exposure after we've set up the prototypes. It's probably worth noting here why the state *is* actually Window-invariant since it's not necessarily obvious.

In the embedding case:

 * If HTTPS embeds HTTP, mixed content blocking will block it, so that's
   a non-issue.

 * If HTTP embeds HTTPS the initial about:blank will inherit the HTTPS
   state "none" and be a non-secure context. The HTTPS page will also
   be a non-secure context though even though it's HTTPS state will be
   "modern", because it's parent is not a secure context.

In the popup scenario the HTTP opening HTTPS case is the same as for the embedding case, but mixed content blocking does not apply in the HTTPS opening HTTP case. In that case:

 * If HTTPS pops up HTTP, the initial about:blank will inherit the HTTPS
   state "modern" and the Secure Context algorithm will make it a Secure
   Context. However, we do not reuse the the initial about:blank Window
   object when the HTTP page loads because it has a different origin to
   the HTTPS page (their schemes are different).

So (hopefully, unless we've missed something) we're okay and Secure Context state is indeed Window invariant.

	Jason Orendorff [:jorendorff]
	Comment 37 • 8 years ago

Comment on attachment 8734236 [details] [diff] [review]
part 1 - Allow CompartmentCreationOptions to store Secure Context state.

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

Certainly. Please add a short comment above the getter, just saying that the flag doesn't affect JS engine behavior, but Gecko uses it to mark certain content windows (and a cross-reference to other code, or a link to this bug).

	Jonathan Watt [:jwatt] Assignee
	Comment 41 • 8 years ago

In part 2 I add an HttpsStateIsModern function to bypass the headache that is bug 1220687. On testing with:

https://jwatt.org/tests/is-secure-context.html
http://tmp.jwatt.org/is-secure-context.html

it seems to behave as it should, although I'm still working on web-platform-tests tests.

	Boris Zbarsky [:bzbarsky]
	Comment 42 • 8 years ago

I'm sorry I didn't get to this today.  I should be able to review it on Monday...

	Boris Zbarsky [:bzbarsky]
	Comment 43 • 8 years ago

Comment on attachment 8744245 [details] [diff] [review]
part 2 - Implement nsGlobalWindow::IsSecureContext

Actually, I have more time today than I thought, so here goes:

>+  bool isNullPrincipal;
>+  nsresult rv = principal->GetIsNullPrincipal(&isNullPrincipal);
>+  if (NS_SUCCEEDED(rv) && isNullPrincipal) {

  if (principal->GetIsNullPrincipal()) {

(it's marked [infallible] in the IDL, so we autogenerate the nicer signature for it).

>+    // Sandboxed content - try and use the loading node's principal instead:

This doesn't make sense to me.  If we have a page at URL A which loads a sandboxed iframe at URL B.... why are we using the principal of page A here, as opposed to the channel URI of the channel B was loaded from?  This deserves at least extensive comments with spec pointers, assuming it's correct.  

I assume the relevant part of the specs here is https://w3c.github.io/webappsec-secure-contexts/#settings-object step 5, but that's using the URL the document was loaded from...

Or is the idea to make this work with sandboxed javascript: and about:blank and the like?  Or something else?

>+  if (creatorOuterWin && creatorOuterWin != GetOuterWindow()) {

GetOuterWindow() == this, since we asserted IsOuterWindow(), right?

I guess GetSciptableParent() can return null if !mDocShell, but can we hit that case here?

If we _can_ (i.e. if GetScriptableParent() returns null), seems to me we should just return false instead of looking for an opener.

>+                 "We should be called early enough in document construction "
>+                 "that the document should not have had time to disown its "
>+                 "opener");

Is that true, though?  Consider this script on http://foo.com:

  var w = window.open("https://foo.com");
  w.opener = null;

what will that look like when you get to here?

>+  documentPrincipal->GetIsNullPrincipal(&isNullPrincipal);

See above.

>+    uri = aDocument->GetOriginalURI();

Ah, ok, this is step 5.  It might be a good idea to number the steps explicitly...  But then I don't know what the business in HttpsStateIsModern was about.

>+    nsCOMPtr<nsIContentSecurityManager> csm =
>+    do_GetService(NS_CONTENTSECURITYMANAGER_CONTRACTID);

Indent second line by 2 more, please.

>+  // Computes the value that will be returned by IsSecureContext():

... for the inner window that corresponds to the given document, right?

r- because of the HttpsStateIsModern issues around null principals...

	Boris Zbarsky [:bzbarsky]
	Comment 44 • 8 years ago

Comment on attachment 8744246 [details] [diff] [review]
part 5 - Expose Window.isSecureContext to content

r=me, but please add this link to the big comment at the top of the file too, and please add tests that exercise the various interesting cases here, now that we have an easy way for scripts to tell whether they're in a secure context or not.

	Jonathan Watt [:jwatt] Assignee
	Comment 45 • 8 years ago

(In reply to Boris Zbarsky [:bz] from comment #43)
> >+    // Sandboxed content - try and use the loading node's principal instead:
> 
> This doesn't make sense to me.  If we have a page at URL A which loads a
> sandboxed iframe at URL B.... why are we using the principal of page A here,
> as opposed to the channel URI of the channel B was loaded from?  This
> deserves at least extensive comments with spec pointers, assuming it's
> correct.  
> 
> I assume the relevant part of the specs here is
> https://w3c.github.io/webappsec-secure-contexts/#settings-object step 5, but
> that's using the URL the document was loaded from...
>
> Or is the idea to make this work with sandboxed javascript: and about:blank
> and the like?  Or something else?

Rather than clear this up here I've tried to do so in a code comment so that future readers of the code don't get similarly confused about this code.

> I guess GetSciptableParent() can return null if !mDocShell, but can we hit
> that case here?

Probably not. I've replaced that with an assertion to document our expectation.

> >+                 "We should be called early enough in document construction "
> >+                 "that the document should not have had time to disown its "
> >+                 "opener");
> 
> Is that true, though?  Consider this script on http://foo.com:
> 
>   var w = window.open("https://foo.com");
>   w.opener = null;
> 
> what will that look like when you get to here?

It is true, yes. We enter nsGlobalWindow::ComputeIsSecureContext before the script sets the opener to null. The stack looks like this:

  nsGlobalWindow::ComputeIsSecureContext
  nsGlobalWindow::SetNewDocument
  nsDocumentViewer::InitInternal
  nsDocumentViewer::Init
  nsDocShell::SetupNewViewer
  nsDocShell::Embed
  nsDocShell::CreateAboutBlankContentViewer
  nsDocShell::CreateAboutBlankContentViewer
  nsGlobalWindow::SetInitialPrincipalToSubject
  nsWindowWatcher::OpenWindowInternal
  nsWindowWatcher::OpenWindow2
  nsGlobalWindow::OpenInternal
  nsGlobalWindow::OpenJS
  nsGlobalWindow::OpenOuter
  nsGlobalWindow::Open
  mozilla::dom::WindowBinding::open

That's fortunate because it seems preferable that scripts disowning their popups doesn't affect the secure context state of the popups.

> Ah, ok, this is step 5.  It might be a good idea to number the steps
> explicitly...

I'm not very keen to do that given the potential for code [comments] and spec formatting to get out of sync. Sometimes we'd want to update the code but it is unfortunate to have to do so for formatting changes.

	Jonathan Watt [:jwatt] Assignee
	Comment 47 • 8 years ago

(In reply to Jonathan Watt [:jwatt] from comment #45)
> Rather than clear this up here I've tried to do so in a code comment so that
> future readers of the code don't get similarly confused about this code.

Oh, and this code was [doing a poor job of] trying to figure out what the origin of the document would have been had it not been sandboxed. In this latest patch I've added a method to nsIScriptSecurityManager to do that in a more robust way.

	Jonathan Watt [:jwatt] Assignee
	Comment 48 • 8 years ago

Add forgotten uuid change.

	Jonathan Watt [:jwatt] Assignee
	Comment 49 • 8 years ago

And further comments to make clear the limitations of HttpsStateIsModern. Sorry for the noise.

	Boris Zbarsky [:bzbarsky]
	Comment 50 • 8 years ago

> We enter nsGlobalWindow::ComputeIsSecureContext before the script sets the opener to null.

For the about:blank, yes.  But what about the actual "https://foo.com" load?

	Jonathan Watt [:jwatt] Assignee
	Comment 51 • 8 years ago

Doh! Indeed. In that case we should cache whether or not the opener was secure when we set mHadOriginalOpener.

	Boris Zbarsky [:bzbarsky]
	Comment 53 • 8 years ago

> Rather than clear this up here I've tried to do so in a code comment so that
> future readers of the code don't get similarly confused about this code.

Yes, that's by far the better solution.  ;)

	Boris Zbarsky [:bzbarsky]
	Comment 54 • 8 years ago

Comment on attachment 8744618 [details] [diff] [review]
part 2 - Implement nsGlobalWindow::IsSecureContext

>+    nsIPrincipal getChannelResultPrincipalIfNotSandboxed(in nsIChannel aChannel);

So this API is temporary until bug 1220687 is fixed, right?

Might be a good idea to document that.

>+    nsIDocument* creatorDoc = aDocument->GetParentDocument();

It's probably safest to return false if null, in case the parent win can get somewhat torn down while we're loading... Shouldn't happen, but..

Or at least assert nonnull.

>+    NS_ASSERTION(parentWin ==

MOZ_ASSERT, please.

>+  if (documentPrincipal->GetIsNullPrincipal()) {
>+    uri = aDocument->GetOriginalURI();

So what happens with a sandboxed data: iframe on some potentially trustworthy but non-https page?  Both per spec and in your impl?

>+    NS_ASSERTION(!mHadOriginalOpener,

MOZ_ASSERT.

r=me modulo my question about the potentially trustworthy non-https case...  That case looks fishy to me.

	Jonathan Watt [:jwatt] Assignee
	Comment 55 • 8 years ago

(In reply to Boris Zbarsky [:bz] from comment #54)
> >+    nsIPrincipal getChannelResultPrincipalIfNotSandboxed(in nsIChannel aChannel);
> 
> So this API is temporary until bug 1220687 is fixed, right?

Most likely. I'll document that and also mark it [noscript, nostdcall].

> >+  if (documentPrincipal->GetIsNullPrincipal()) {
> >+    uri = aDocument->GetOriginalURI();
> 
> So what happens with a sandboxed data: iframe on some potentially
> trustworthy but non-https page?  Both per spec and in your impl?

This seems to be an issue with the spec. I've filed:

https://github.com/w3c/webappsec-secure-contexts/issues/26
https://github.com/whatwg/fetch/issues/290

Neither the spec nor my implementation will consider the sandboxed document to be secure, although for slightly different reasons. Let's see where the spec bugs go.

	Jonathan Watt [:jwatt] Assignee
	Comment 56 • 8 years ago

There are two substantive changes.

I modified nsScriptSecurityManager::GetChannelResultPrincipal to add special handling for the value returned by GetForceInheritPrincipal when aIgnoreSandboxing is true, since GetForceInheritPrincipal always returns false when we're sandboxing.

I made HttpsStateIsModern use nsIContentSecurityManager::IsOriginPotentiallyTrustworthy instead of just checking for 'https' so that it will also return true for localhost etc.  This makes sandboxed data: etc. iframe on some potentially trustworthy but non-https pages secure.

	Jonathan Watt [:jwatt] Assignee
	Comment 57 • 8 years ago

Depending on the outcome of the issues linked in comment 55, if we make data: non-secure we can do it consistently by adding a check for that scheme and early return in nsGlobalWindow::ComputeIsSecureContext.

	Boris Zbarsky [:bzbarsky]
	Comment 58 • 8 years ago

Is GetForceInheritPrincipal() actually true much nowadays?  I thought we were trying to move everything to the security flags...

The new bits in GetChannelResultPrincipal bother me. They're making lots of assumptions about what the caller is trying to do that happen to be true here but may not be going forward....

	Jonathan Watt [:jwatt] Assignee
	Comment 59 • 8 years ago

(In reply to Boris Zbarsky [:bz] from comment #58)
> Is GetForceInheritPrincipal() actually true much nowadays?  I thought we
> were trying to move everything to the security flags...

GetForceInheritPrincipal is a convenience method that just checks the security flags. It returns:

  mSecurityFlags & nsILoadInfo::SEC_FORCE_INHERIT_PRINCIPAL

> The new bits in GetChannelResultPrincipal bother me. They're making lots of
> assumptions about what the caller is trying to do that happen to be true
> here but may not be going forward....

Yeah, the general idea in the code seemed like it would be stable, but it's not a great solution to duplicate the logic.  I played with various ways to try and avoid that but most of them were messy.  The one approach that would be cleaner would be to have load info objects remember whether they dropped the SEC_FORCE_INHERIT_PRINCIPAL flag and expose that information (via another flag).

	Boris Zbarsky [:bzbarsky]
	Comment 61 • 8 years ago

> GetForceInheritPrincipal is a convenience method that just checks the security flags.

Yes, but I meant do we often use SEC_FORCE_INHERIT_PRINCIPAL as opposed to SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS and company?  Is it really only used for about:blank in practice?  This stuff has been changing a lot, so I'm not sure where things stand now.  Anyway, given the new patch doesn't seem too relevant.

	Boris Zbarsky [:bzbarsky]
	Comment 62 • 8 years ago

Comment on attachment 8745952 [details] [diff] [review]
part 2 - expose whether SEC_FORCE_INHERIT_PRINCIPAL was dropped from an nsILoadInfo

r=me

	Jonathan Watt [:jwatt] Assignee
	Comment 63 • 8 years ago

(In reply to Boris Zbarsky [:bz] from comment #61)
I meant do we often use SEC_FORCE_INHERIT_PRINCIPAL as opposed to
> SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS and company?

We're at least using it in all docShell loads where we have a triggering principal and where isSrcdoc is true or nsContentUtils::ChannelShouldInheritPrincipal returns true:

https://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp?rev=b8b7fa054965&mark=10664-10700#10664

(Note isSrcdoc is passed to ChannelShouldInheritPrincipal's aForceInherit argument.)

So I think it's used here for data:, javascript:, wyciwyg: and srcdoc.

	Jonathan Watt [:jwatt] Assignee
	Comment 64 • 8 years ago

I've pulled this out from the "Implement nsGlobalWindow::IsSecureContext" patch since it's logically separate and probably good to get a last review on the bit that bothered you.

	Boris Zbarsky [:bzbarsky]
	Comment 67 • 8 years ago

Comment on attachment 8746879 [details] [diff] [review]
part 3 - Add a getChannelResultPrincipalIfNotSandboxed method to nsIScriptSecurityManager

If it's nostdcall, the C++ impl should just return nsresult, not NS_IMETHODIMP, right?

r=me

	Boris Zbarsky [:bzbarsky]
	Comment 68 • 8 years ago

Comment on attachment 8746882 [details] [diff] [review]
part 6 - Add Web Platform Tests for Secure Contexts

>+               URL.createObjectURL(new Blob(["<script>(opener||parent).postMessage(isSecureContext, '*')<\/script>"])),

You don't need to escape the '/' of "/script".

>+               "<script>(opener||parent).postMessage(isSecureContext, '*')<\/script>",

Likewise.

>+  return this;

You don't need this, in both constructors.

>+LoadTarget.prototype.open = function(loadType) {

Can we assert that loadType.loadInFlags contains this.loadInFlag?  Seems like we should be able to.

>+    iframe.setAttribute("sandbox", "allow-scripts allow-modals");

Why do we need allow-modals?

>+    domTarget.parentNode.removeChild(domTarget);

  domTarget.remove();

>+    let result = domTarget instanceof Window ?
>+          domTarget.isSecureContext : domTarget.contentWindow.isSecureContext;

This pops up in two places; may be worth factoring out.

>+  let loadTarget = loadTargets[current_target_index];

Why not move this to after the block where we might have incremented current_target_index, instead of doing a second assignment in that block?

>+          assert_false(value, loadType.desc + " should not be a Secure Context");

Could you include |"in " + loadTarget.desc| in that message?

Similar for the other messages.

r=me

	Pulsebot
	Comment 69 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/d07106878066
https://hg.mozilla.org/integration/mozilla-inbound/rev/24a113011b90
https://hg.mozilla.org/integration/mozilla-inbound/rev/c32539fd746a
https://hg.mozilla.org/integration/mozilla-inbound/rev/309a56e56277
https://hg.mozilla.org/integration/mozilla-inbound/rev/a921444a4b34

	Jonathan Watt [:jwatt] Assignee
	Comment 70 • 8 years ago

(In reply to Boris Zbarsky [:bz] from comment #68)
> You don't need to escape the '/' of "/script".

Left over from when this was inline in the HTML. Thanks.

> Why do we need allow-modals?

So that I could alert() during development. ;)

	Pulsebot
	Comment 71 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e1a99955893

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 72 • 8 years ago

Had to back these out for many tests failing with Assertion failure: principal->GetIsCodebasePrincipal(), at dom/base/nsGlobalWindow.cpp:2412.

Backouts:
https://hg.mozilla.org/integration/mozilla-inbound/rev/18c3745427c04961696b9178e1367abead511cd4
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e4f1107cd17a9809d18234f4a7c2da5420ea9ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e73a4d863d497211a681d4ec09a1e3390ea2dae
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e920902fcca6cb2b70e7174a69c8124e9d88799
https://hg.mozilla.org/integration/mozilla-inbound/rev/4855569da09441640395ac46bf8bf29e21708565
https://hg.mozilla.org/integration/mozilla-inbound/rev/fda8ec39ce04d816b58b6d8535791c4d5af53392

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a921444a4b344117f1ab87392f47ef617e23c351
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=26922609&repo=mozilla-inbound
01:16:02     INFO -  Assertion failure: principal->GetIsCodebasePrincipal(), at /home/worker/workspace/build/src/dom/base/nsGlobalWindow.cpp:2412
01:16:02     INFO -  #01: nsGlobalWindow::SetNewDocument [dom/base/nsGlobalWindow.cpp:2768]
01:16:02     INFO -  #02: nsDocumentViewer::InitInternal [layout/base/nsDocumentViewer.cpp:875]
01:16:02     INFO -  #03: nsDocumentViewer::Init [layout/base/nsDocumentViewer.cpp:619]
01:16:02     INFO -  #04: nsDocShell::SetupNewViewer [docshell/base/nsDocShell.cpp:9261]
01:16:02     INFO -  #05: nsDocShell::Embed [docshell/base/nsDocShell.cpp:7152]
01:16:02     INFO -  #06: nsDocShell::CreateContentViewer [docshell/base/nsDocShell.cpp:9069]
01:16:02     INFO -  #07: nsDSURIContentListener::DoContent [docshell/base/nsDSURIContentListener.cpp:130]
01:16:02     INFO -  #08: nsDocumentOpenInfo::TryContentListener [uriloader/base/nsURILoader.cpp:724]
01:16:02     INFO -  #09: nsDocumentOpenInfo::DispatchContent [uriloader/base/nsURILoader.cpp:398]
01:16:02     INFO -  #10: nsDocumentOpenInfo::OnStartRequest [uriloader/base/nsURILoader.cpp:261]
01:16:02     INFO -  #11: nsBaseChannel::OnStartRequest [netwerk/base/nsBaseChannel.cpp:802]
01:16:02     INFO -  #12: nsInputStreamPump::OnStateStart [netwerk/base/nsInputStreamPump.cpp:526]
01:16:02     INFO -  #13: nsInputStreamPump::OnInputStreamReady [netwerk/base/nsInputStreamPump.cpp:427]
01:16:02     INFO -  #14: nsInputStreamReadyEvent::Run [xpcom/io/nsStreamUtils.cpp:97]
01:16:02     INFO -  #15: nsThread::ProcessNextEvent [xpcom/threads/nsThread.cpp:989]
01:16:02     INFO -  #16: NS_ProcessNextEvent [xpcom/glue/nsThreadUtils.cpp:290]
01:16:02     INFO -  #17: mozilla::ipc::MessagePump::Run [ipc/glue/MessagePump.cpp:99]
01:16:02     INFO -  #18: MessageLoop::RunInternal [ipc/chromium/src/base/message_loop.cc:234]
01:16:02     INFO -  #19: MessageLoop::Run [ipc/chromium/src/base/message_loop.cc:479]
01:16:02     INFO -  #20: nsBaseAppShell::Run [widget/nsBaseAppShell.cpp:158]
01:16:02     INFO -  #21: nsAppStartup::Run [toolkit/components/startup/nsAppStartup.cpp:285]
01:16:02     INFO -  #22: XREMain::XRE_mainRun [toolkit/xre/nsAppRunner.cpp:4348]
01:16:02     INFO -  #23: XREMain::XRE_main [toolkit/xre/nsAppRunner.cpp:4451]
01:16:02     INFO -  #24: XRE_main [toolkit/xre/nsAppRunner.cpp:4559]
01:16:02     INFO -  #25: do_main [browser/app/nsBrowserApp.cpp:212]
01:16:02     INFO -  #26: main [browser/app/nsBrowserApp.cpp:360]
01:16:02     INFO -  #27: libc.so.6 + 0x2176d
01:16:02     INFO -  #28: _start

	Jonathan Watt [:jwatt] Assignee
	Comment 73 • 8 years ago

This happens, for example, when the "response-preview" sandboxed HTML iframe in netmonitor.xul loads. HttpsStateIsModern calls GetChannelResultPrincipalIfNotSandboxed which gets the parent's principal which is the system principal. We then fail the MOZ_ASSERT(principal->GetIsCodebasePrincipal()).

FWIW the iframe's content is set to the response body of a monitored network request:

https://mxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/netmonitor-view.js?rev=0a2e98e3e736&mark=3391-3391#3389

	Jonathan Watt [:jwatt] Assignee
	Comment 75 • 8 years ago

(The GetIsSystemPrincipal check at the top of HttpsStateIsModern isn't currently necessary since ComputeIsSecureContext will have already returned true before calling HttpsStateIsModern in that case, but it seems not a bad thing to have.)

	Pulsebot
	Comment 76 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed22935f909f
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc0204d48f0f
https://hg.mozilla.org/integration/mozilla-inbound/rev/287b9a367619

	Carsten Book [:Tomcat]
	Comment 77 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/ed22935f909f
https://hg.mozilla.org/mozilla-central/rev/bc0204d48f0f
https://hg.mozilla.org/mozilla-central/rev/287b9a367619

	Boris Zbarsky [:bzbarsky]
	Comment 78 • 8 years ago

Comment on attachment 8747496 [details] [diff] [review]
fixup to part 4

r=me

	Boris Zbarsky [:bzbarsky]
	Comment 79 • 8 years ago

That said, why does the "I am sandboxing it, so we should not trust it" argument not apply to https as well?  Might be worth a spec issue to figure out what the really right behavior is for sandboxing here....

	Jonathan Watt [:jwatt] Assignee
	Comment 80 • 8 years ago

My source comment is off base really, or at least incomplete. For an https page sandboxing a srcdoc we assume that the content of the srcdoc is also obtained via https. For chrome content with the system principal we can't infer that, and in fact the sandboxing indicates that the contrary may be just as likely. We have no way of knowing the security of the delivery mechanism of the sandboxed content so it seems best to me to assume it may have been delivered insecurely.

	Jonathan Watt [:jwatt] Assignee
	Comment 81 • 8 years ago

I've rewritten the comment to explain that:

        // If a document with the system principal is sandboxing a subdocument
        // that would normally inherit the embedding element's principal (e.g.
        // a srcdoc document) then the embedding document does not trust the
        // content that is written to the embedded document.  Unlike when the
        // embedding document is https, in this case we have no indication as
        // to whether the embedded document's contents are delivered securely
        // or not, and the sandboxing would possibly indicate that they were
        // not.  To play it safe we return false here.  (See bug 1162772
        // comment 73-80.)

	Pulsebot
	Comment 82 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/924ba6c65c4b
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb858a0c59b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/d338b07ac840

	Boris Zbarsky [:bzbarsky]
	Comment 83 • 8 years ago

> For an https page sandboxing a srcdoc we assume that the content of the srcdoc is also obtained via https

It is, sure.  But should it be considered a "secure context"?  That is the real question in my mind...  Seems to me like the page that frames the sandboxed stuff may NOT want to expose secure context APIs to it!

	Jonathan Watt [:jwatt] Assignee
	Comment 84 • 8 years ago

I've filed:

  https://github.com/w3c/webappsec-secure-contexts/issues/28

Note that rbarnes wants to put *all* new APIs (regardless of whether they are sensitive) behind a Secure Context check moving forwards, so it seems like authors would end up wanting sandboxed sub-documents to be secure contexts if they want them to have access to any new APIs.

	Carsten Book [:Tomcat]
	Comment 85 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/924ba6c65c4b
https://hg.mozilla.org/mozilla-central/rev/bb858a0c59b3
https://hg.mozilla.org/mozilla-central/rev/d338b07ac840

	Boris Zbarsky [:bzbarsky]
	Comment 86 • 8 years ago

I think rbarnes is largely wrong in his wanting, and this is a good illustration of why: it actually reduces security in this case.

	Richard Barnes [:rbarnes]
	Comment 87 • 8 years ago

bz: I would be interested in how you think security is being reduced here.

It seems like the most we have here is some cases where a non-secure thing can access a [SecureContexts] API.  That's not desirable, but it's not a reduction in security vis à vis the baseline state before the restriction.

In any case, we're diverging from the purpose of this bug...

	Boris Zbarsky [:bzbarsky]
	Comment 88 • 8 years ago

I think it's a reduction in security vis a vis being able to say that something is not secure and should not access APIs for which security actually matters, but still allowing access to new APIs for which security does not matter.

Or to put another way, if we make all new stuff [SecureContext] then to get access to a new Node method in our sandboxed thiing you suddenly have to allow it to access things that it really should _not_ have access to.  At least assuming there are things that are labeled [SecureContext] for reasons other than "we just want everyone to use https".

	Boris Zbarsky [:bzbarsky]
	Comment 89 • 8 years ago

But yes, all that is a tangent to this bug.  The right place to discuss these sorts of things at length is in the spec issue jwatt filed or other standards venues.

	Florian Scholz (Open Web Docs)
	Comment 90 • 8 years ago

Docs:
https://developer.mozilla.org/en-US/Firefox/Releases/49#Security
https://developer.mozilla.org/en-US/docs/Web/API/Window/isSecureContext
https://developer.mozilla.org/en-US/docs/Web/Security/Secure_Contexts

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 91 • 8 years ago

The SEC_FORCE_INHERIT_PRINCIPAL_WAS_DROPPED flag really should not have been a flag on nsILoadInfo. All the other flags are passed _from_ the creator of a channel _to_ the channel, however this flag is used to pass information from the channel to the consumer of a channel.

A cleaner solution would be to add a function like nsILoadInfo.wasInheritFlagDropped()

I'd appreciate if someone could fix this in a followup.

	Andrea Marchesini [:baku]
	Comment 92 • 8 years ago

See comment 91.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 93 • 8 years ago

(In reply to Jonas Sicking (:sicking) PTO Until July 5th from comment #91)
> The SEC_FORCE_INHERIT_PRINCIPAL_WAS_DROPPED flag really should not have been
> a flag on nsILoadInfo. All the other flags are passed _from_ the creator of
> a channel _to_ the channel, however this flag is used to pass information
> from the channel to the consumer of a channel.
> 
> A cleaner solution would be to add a function like
> nsILoadInfo.wasInheritFlagDropped()
> 
> I'd appreciate if someone could fix this in a followup.
I filed Bug 1287073.
I guess it's a small fix (?) so I can work on it
If jwatt would like to do it feel free to take it back. :)

Thanks

	Jonathan Watt [:jwatt] Assignee
	Comment 94 • 8 years ago

(In reply to Yoshi Huang[:allstars.chh] from comment #93)
> I filed Bug 1287073.
> I guess it's a small fix (?) so I can work on it
> If jwatt would like to do it feel free to take it back. :)

Thanks for filing a follow-up bug. Please do feel free to work on it.