Implement Window.isSecureContext

RESOLVED FIXED in Firefox 49

Status

()

defect
--
major
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: dougt, Assigned: jwatt)

Tracking

(Depends on 1 bug, Blocks 3 bugs, {dev-doc-complete})

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

(URL)

Attachments

(7 attachments, 12 obsolete attachments)

4.06 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
829 bytes, patch
bzbarsky
: review+
Details | Diff | Splinter Review
4.21 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
7.45 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
14.13 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
14.32 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.79 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 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.
Flagging this for Richard.
Flags: needinfo?(rlb)
Is there a nice API that I can query which will check the cert and give me a nsresult back?
Adding keeler to needinfo - either rbarnes or keeler can answer comment #2.
Flags: needinfo?(dkeeler)
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().
Flags: needinfo?(dkeeler)
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
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.
Flags: needinfo?(rlb)
Summary: tests for url scheme https:// isn't enough. → Add an API to determine when a context is secure
(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/
(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.
We decided we were good for our initial shipping of Service Worker without this.
No longer blocks: ServiceWorkers-v1
Missed blocking the post-SW-v1 tracker.
Status: NEW → ASSIGNED

Comment 13

4 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
(Assignee)

Comment 14

4 years ago
Good to know, thanks for taking a look.

Comment 15

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

Comment 16

4 years ago
Thanks, Ehsan. I asked about the RESOLVE_OFFLINE flag in bug 87717 comment 59.
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.

Updated

4 years ago
See Also: → 899099

Updated

4 years ago
Blocks: 1217133
(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.
(Assignee)

Comment 19

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

Comment 20

4 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.
Assignee: sworkman → jwatt
Depends on: 1220687
Summary: Add an API to determine when a context is secure → Implement Window.isSecureContext
(Assignee)

Comment 21

4 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.
Attachment #8674302 - Attachment is obsolete: true
(Assignee)

Comment 22

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

Updated

4 years ago
Depends on: 1223624
(Assignee)

Updated

4 years ago
Blocks: 1212239
(Assignee)

Updated

3 years ago
Blocks: 1177957
No longer depends on: 1223624
See Also: 11779571223624
(Assignee)

Comment 23

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

Comment 24

3 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).
Attachment #8717952 - Flags: feedback?(bzbarsky)
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.
Attachment #8717952 - Flags: feedback?(bzbarsky) → feedback+
(Assignee)

Comment 26

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

Comment 27

3 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #8674303 - Attachment is obsolete: true
Attachment #8717952 - Attachment is obsolete: true
Attachment #8723079 - Flags: review?(bzbarsky)
> 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.  :(
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.
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?
Attachment #8723079 - Flags: review?(bzbarsky)
(Assignee)

Comment 31

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

Comment 32

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

Updated

3 years ago
Attachment #8723079 - Attachment is obsolete: true
(Assignee)

Comment 33

3 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.
Attachment #8727886 - Attachment is obsolete: true
(Assignee)

Comment 34

3 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.
Attachment #8734236 - Flags: review?(luke)
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
Attachment #8734236 - Flags: review?(luke) → review?(jorendorff)
(Assignee)

Comment 36

3 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.
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).
Attachment #8734236 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 39

3 years ago
Attachment #8744245 - Flags: review?(bzbarsky)
(Assignee)

Comment 41

3 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.
I'm sorry I didn't get to this today.  I should be able to review it on Monday...
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...
Attachment #8744245 - Flags: review?(bzbarsky) → review-
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.
Attachment #8744246 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 45

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

Comment 46

3 years ago
Attachment #8744245 - Attachment is obsolete: true
Attachment #8744572 - Flags: review?(bzbarsky)
(Assignee)

Comment 47

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

Comment 48

3 years ago
Add forgotten uuid change.
Attachment #8744572 - Attachment is obsolete: true
Attachment #8744572 - Flags: review?(bzbarsky)
Attachment #8744573 - Flags: review?(bzbarsky)
(Assignee)

Comment 49

3 years ago
And further comments to make clear the limitations of HttpsStateIsModern. Sorry for the noise.
Attachment #8744573 - Attachment is obsolete: true
Attachment #8744573 - Flags: review?(bzbarsky)
Attachment #8744586 - Flags: review?(bzbarsky)
> 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?
(Assignee)

Comment 51

3 years ago
Doh! Indeed. In that case we should cache whether or not the opener was secure when we set mHadOriginalOpener.
(Assignee)

Comment 52

3 years ago
Attachment #8744586 - Attachment is obsolete: true
Attachment #8744586 - Flags: review?(bzbarsky)
Attachment #8744618 - Flags: review?(bzbarsky)
> 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.  ;)
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.
Attachment #8744618 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 55

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

Comment 56

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

Comment 57

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

Comment 59

3 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).
> 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.
Comment on attachment 8745952 [details] [diff] [review]
part 2 - expose whether SEC_FORCE_INHERIT_PRINCIPAL was dropped from an nsILoadInfo

r=me
Attachment #8745952 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 63

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

Updated

3 years ago
Attachment #8745952 - Attachment description: patch - expose whether SEC_FORCE_INHERIT_PRINCIPAL was dropped from an nsILoadInfo → part 2 - expose whether SEC_FORCE_INHERIT_PRINCIPAL was dropped from an nsILoadInfo
(Assignee)

Comment 64

3 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.
Attachment #8746879 - Flags: review?(bzbarsky)
(Assignee)

Comment 65

3 years ago
Attachment #8744618 - Attachment is obsolete: true
Attachment #8745788 - Attachment is obsolete: true
Attachment #8746880 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8744246 - Attachment description: part 3 - Expose Window.isSecureContext to content → part 5 - Expose Window.isSecureContext to content
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
Attachment #8746879 - Flags: review?(bzbarsky) → review+
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
Attachment #8746882 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 70

3 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. ;)
(Assignee)

Updated

3 years ago
Blocks: 1269050
(Assignee)

Updated

3 years ago
Blocks: 1269052
(Assignee)

Updated

3 years ago
Blocks: 1269053
(Assignee)

Updated

3 years ago
Blocks: 1269054
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
Flags: needinfo?(jwatt)
(Assignee)

Comment 73

3 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
Flags: needinfo?(jwatt)
(Assignee)

Comment 74

3 years ago
Attachment #8747496 - Flags: review?(bzbarsky)
(Assignee)

Comment 75

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

Updated

3 years ago
Keywords: leave-open
Comment on attachment 8747496 [details] [diff] [review]
fixup to part 4

r=me
Attachment #8747496 - Flags: review?(bzbarsky) → review+
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....
(Assignee)

Comment 80

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

Comment 81

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

Updated

3 years ago
Keywords: leave-open
> 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!
(Assignee)

Comment 84

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

Comment 85

3 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
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
I think rbarnes is largely wrong in his wanting, and this is a good illustration of why: it actually reduces security in this case.
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...
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".
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.
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.
See comment 91.
Flags: needinfo?(jwatt)
(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
(Assignee)

Comment 94

3 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.
Flags: needinfo?(jwatt)
You need to log in before you can comment on or make changes to this bug.