1177957 - Provide a [SecureContext] WebIDL extended attribute for exposing APIs only in secure contexts

Status: RESOLVED FIXED

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

Description

[Martin Thomson [:mt:]]

We're moving a lot of new features to secure contexts, but we need an easy way to determine if this really is a new context.

Comment 1

[Martin Thomson [:mt:]]

Not a new context, a secure context.

Comment 2

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

Secure context meaning what? page loaded from https without any communication with http?

Comment 3

[Martin Thomson [:mt:]]

I cited the specification in the URL field. Anne has a few issues with corner cases that the asked covers poorly, but otherwise, I think that this could be sufficient. It is basically https origins without http ancestors.

Comment 4

[Martin Thomson [:mt:]]

S/asked/spec damb this phone.

Comment 5

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

How would the setup work in SharedWorkers in case top level http page (origin A) has https iframe (origin B) which creates SharedWorker and then there is another tab which has https page (origin B) as top level browsing context and also that opens the SharedWorker? What kind of stuff should be exposed in the SharedWorker?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Or even more simply, what if you have an https:// page with a dedicated worker, and the https:// page has an http:// parent.  Should the worker be considered secure?

Comment 7

[Martin Thomson [:mt:]]

The shared worker case is one that the spec needs to address.

An https:// page with an http:// parent is considered not secure, so any worker it creates would be not secure: https://w3c.github.io/webappsec/specs/powerfulfeatures/#settings-secure step 2.2.2.

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK, so this is new state we have to propagate through all our globals, then, since clearly the worker 

Once we have this state there, adding an annotation like this is easy.

Comment 9

[Martin Thomson [:mt:]]

The annotation is [SecureContext] (https://w3c.github.io/webappsec-secure-contexts/#new).

Comment 10

[Jonathan Watt [:jwatt]]

The pull request to get this added to WebIDL 2 is currently a bit bogged down:

https://github.com/heycam/webidl/pull/65

Comment 11

[Jonathan Watt [:jwatt]]

Conversation has picked up again on public-script-coord:

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

Comment 12

[Jonathan Watt [:jwatt]]

I've been taking a look at what would be involved in implementing this. It seems that we split NativeProperties into 'regular' and 'chromeOnly'. I'm not sure how adding in 'secureContextOnly' should affect the code. If there needs to be overlap between properties then it makes things more complicated, but if we can say that [ChromeOnly] subsumes [SecureContext] the changes should be more straightforward.

Comment 13

[Boris Zbarsky [:bzbarsky]]

[SecureContext] should work the same way that [Pref] and [Func] do, in my opinion.

Note that you'll want to work on top of the changes from bug 1011826 (which I expect will land imminently).

Comment 14

[Boris Zbarsky [:bzbarsky]]

Oh, and the reason [SecureContext] should not work like [ChromeOnly], or at least _a_ reason, is that [ChromeOnly] affects enumeration order but we probably don't want [SecureContext] to affect it.

Comment 15

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

Unrelated to that, but [SecureContext] stuff should be enabled in contexts where [ChromeOnly] is enabled.

Comment 16

[Boris Zbarsky [:bzbarsky]]

I agree; but we can do that as an explicit check in the "check for a secure context" function.  In particular, this means that it will be enabled in xrays when an extension pokes at a web page that's not in a secure context.

Comment 17

[Jonathan Watt [:jwatt]]

Attachment: patch - only hand tested

My biggest issue with fixing this bug is figuring out where I should add tests under dom/bindings/.

Obviously this patch also depends on the chain of bugs blocking this bug.

Comment 18

[Boris Zbarsky [:bzbarsky]]

You should be able to just add tests under dom/bindings/test; mochitest will let you test https vs not.

Comment 19

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Provide a [SecureContext] WebIDL extended attribute for exposing APIs only in secure contexts

Comment 20

[Jonathan Watt [:jwatt]]

Attachment: part 4 - Tests for the [SecureContext] WebIDL extended attribute

Bear in mind that I'm unfamiliar with the bindings code, so this could do with close review.

Comment 21

[Jonathan Watt [:jwatt]]

(In reply to Olli Pettay [:smaug] from comment #15)
> Unrelated to that, but [SecureContext] stuff should be enabled in contexts
> where [ChromeOnly] is enabled.

It seems like Window.isSecureContext and [SecureContext] should be in sync, so special casing the bindings code to check nsContentUtils::ThreadsafeIsCallerChrome() seems like the wrong thing to do.  Possibly nsGlobalWindow::ComputeIsSecureContext should be doing that call instead.

Comment 22

[Jonathan Watt [:jwatt]]

Adding dev-doc-needed to get this added to https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8748423 [details] [diff] [review]
part 1 - Provide a [SecureContext] WebIDL extended attribute for exposing APIs only in secure contexts

I think Olli is right.

The issue here is pretty subtle, though, so I'd like to explain my reasoning to see what I'm missing.

The point of [SecureContext] is to control API exposure.  But API exposure to _whom_?  Presumably the caller.  On the web, the caller and the object involved are same-origin (unless we plan to add cross-origin-accessible [SecureContext] things, which I hope we're not), so the distinction is not that important.  But once browser extensions are involved, the situation becomes more complicated.  And in particular, the obvious question becomes: should an extension poking at DOM objects via Xrays see [SecureContext] APIS exposed or not?  Seems like it should.

If so, this means two things:

1)  The secure context check should be done on the current compartment of the JSContext, not the compartment of the object, both in CGConstructorEnabled and in PrefableDisablers.

2)  Either that check needs to explicitly treat the system principal as secure or we need to make sure that all system principal compartments are marked as secure.  It's even possible that the right answer here is to reverse the boolean stored in the JSCompartment and treat all non-window compartments as secure, though I'm not entirely sure that's the right thing for XPConnect sandboxes.  But certainly we want to treat JSMs and the like as secure contexts, I would think.

Thoughts?

Comment 24

[Boris Zbarsky [:bzbarsky]]

> should an extension poking at DOM objects via Xrays see [SecureContext] APIS exposed or not? 

I meant DOM objects that did NOT come from a secure thing, of course.

Comment 25

[Jonathan Watt [:jwatt]]

(In reply to Boris Zbarsky [:bz] from comment #23)
> should an extension poking at DOM objects [that did NOT come from a secure
> thing] see [SecureContext] APIS exposed or not?  Seems like it should.

Agreed.

> 1)  The secure context check should be done on the current compartment of
> the JSContext, not the compartment of the object, both in
> CGConstructorEnabled and in PrefableDisablers.

Yup.

> 2)  Either that check needs to explicitly treat the system principal as
> secure or we need to make sure that all system principal compartments are
> marked as secure.

To me it seems preferable that all system principal compartments be marked as secure.

> It's even possible that the right answer here is to
> reverse the boolean stored in the JSCompartment and treat all non-window
> compartments as secure, though I'm not entirely sure that's the right thing
> for XPConnect sandboxes.

I don't know much about compartments to understand the implications of doing that.

> But certainly we want to treat JSMs and the like
> as secure contexts, I would think.

That seems right.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8748423 [details] [diff] [review]
part 1 - Provide a [SecureContext] WebIDL extended attribute for exposing APIs only in secure contexts

r- pending a patch that checks the context.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8748424 [details] [diff] [review]
part 4 - Tests for the [SecureContext] WebIDL extended attribute

r+ with the renaming to secureContext1, etc, as discussed on IRC.

Comment 28

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Provide a [SecureContext] WebIDL extended attribute for exposing APIs only in secure contexts

Comment 29

[Jonathan Watt [:jwatt]]

Attachment: codegen effect of the patch to mark directory upload interfaces as [SecureContext] in bug 1212239

This attachment shows the change to the generated code when the directory upload partial interfaces are marked as [SecureContext] using the following patch from bug 1212239:

https://bugzilla.mozilla.org/attachment.cgi?id=8750818&action=diff

Looks correct to me.

Comment 30

[Jonathan Watt [:jwatt]]

Regarding the xpconnect sandboxing issue, I think we should address that in a follow-up bug where we can hopefully get some input from bholley.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8750845 [details] [diff] [review]
part 1 - Provide a [SecureContext] WebIDL extended attribute for exposing APIs only in secure contexts

>+++ b/dom/bindings/Codegen.py
>+                self.secureContext is True or

Just self.secureContext, please.

>+                                  str(condition.secureContext).lower(),

toStringBool(condition.secureContext), pleaes.  And nix the comment just above this line.

>+            # that exposure depends on the privilages of the running code
>+            # (rather than the privilages of the object's creator).

s/privilages/privileges/, both places.  And in the other place where this comment got copied.  But see below.

>+            conditions.append("JS::CompartmentCreationOptionsRef(js::GetContextCompartment(aCx)).secureContext()")

I would prefer it if we had an inline mozilla::dom::IsSecureContext(JSContext*) in BindingUtils.h.  The documentation for it would explain why it only depends on the JSContext, instead of having those comments in the two callsites, and then the callsites would just call IsSecureContext(cx).

>+++ b/dom/bindings/parser/WebIDL.py

Meta-comment: Please split out the parser parts into a separate patch under the codegen patch.  This patch will have its own tests and whatnot, and is completely independent of the codegen changes.

>+                # This just gets propagated to all our members.

This is wrong.  In particular, you're only doing this for _partial_ interfaces, not non-partial ones.  We should add tests for this.  This should be testable via webidl parser tests, so doesn't rely on codegen at all.

Also, this patch is not implementing the following spec provisions:

1) "The [SecureContext] extended attribute MUST NOT be specified on both an interface member and the interface or partial interface definition the interface member is declared on."  This one could use a parser test as well.

2) "An interface without the [SecureContext] extended attribute MUST NOT inherit from another interface that does specify [SecureContext]."  Again, needs a parser test.

3) "The [SecureContext] extended attribute MUST take no arguments."  Again, needs a parser test.

>+++ b/js/xpconnect/wrappers/AccessCheck.h

Why are these methods needed?  They don't seem to be used.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8750847 [details] [diff] [review]
codegen effect of the patch to mark directory upload interfaces as [SecureContext] in bug 1212239

Hmm.  Why did sAttributes_disablers50 even exist before the [SecureContext] was added?  Was there already a [Pref] annotation on it or something?  I don't know which interface members this corresponds to, or on what interface, so hard to tell what's going on....

Comment 33

[Jonathan Watt [:jwatt]]

Attachment: part 2 - WebIDL parser tests for the [SecureContext] extented attribute

Hopefully this covers most things. I'll attach the parser changes themselves shortly as "part 1".

Comment 34

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Make the WebIDL parser support the [SecureContext] extented attribute

Comment 35

[Jonathan Watt [:jwatt]]

Attachment: part 3 - Make the WebIDL code generator support the [SecureContext] extended attribute

Comment 36

[Jonathan Watt [:jwatt]]

(In reply to Boris Zbarsky [:bz] from comment #32)
> Hmm.  Why did sAttributes_disablers50 even exist before the [SecureContext]
> was added?  Was there already a [Pref] annotation on it or something?  I
> don't know which interface members this corresponds to, or on what
> interface, so hard to tell what's going on....

The diff linked in comment 29 is adding [SecureContext] to the |partial interface HTMLInputElement| in dom/webidl/HTMLInputElement.webidl that comes from:

https://wicg.github.io/directory-upload/proposal.html#file-input

And yes, all the members of this interface have [Pref] annotations on them which is why sAttributes_disablers50 already exists.

There should be enough context in the diff to make out which interface members are being affected here. sMethods_disablers9 is used for the two entries after line 4106, and sAttributes_disablers50 is used for the two entries after line 4201.

Comment 37

[Jonathan Watt [:jwatt]]

Attachment: Effect on codegen of adding [SecureContext] to the main HTMLInputElement interface

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8751509 [details] [diff] [review]
part 1 - Make the WebIDL parser support the [SecureContext] extented attribute

r=me.  Much better, thank you!

Comment 39

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8751491 [details] [diff] [review]
part 2 - WebIDL parser tests for the [SecureContext] extented attribute

>+def WebIDLTest(parser, harness):
>+    parser = parser.reset()

No need to reset it up front; it comes in reset.

For the first test, you probably want to harness.check(len(results[0].members), 6) just to make sure there's no weirdness going on.  Similar for the other tests.

There is no point checking the members of the partial interface in that test, imo; no one cares about them.  Again, similar for the other tests.  Just checking the members of the non-partial interface is good.

What you probably _should_ test is that exact IDL snippet but with the partial interface coming before the interface, and then checking that the members of results[0] have the right extended attribute.

Please add a test that if A is securecontext, B is not, A implements B, then the methods copied from B are not securecontext.  But note https://github.com/heycam/webidl/issues/118

r=me

r=me

Comment 40

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8751510 [details] [diff] [review]
part 3 - Make the WebIDL code generator support the [SecureContext] extended attribute

>+        assert isinstance(secureContext, bool)
...
>+            interfaceMember.getExtendedAttribute("SecureContext") or False,

So this is relying on interfaceMember.getExtendedAttribute("SecureContext") returning a boolean when the attr is present (but None when absent)?

It might be clearer/safer to just pass in:

  interfaceMember.getExtendedAttribute("SecureContext") is not None

>+            conditions.append("mozilla::dom::IsSecureContext(aCx)")

I thought we had decided in that email thread with bholley that we should expose if either the JSContext compartment _or_ the object compartment is a secure context?  Otherwise expanded-principal globals touching a securecontext web page (e.g. those used by frame scripts) won't work correctly, I expect.

>+  JS::CompartmentCreationOptionsRef(js::GetContextCompartment(aCx));

Indent this line by two spaces, please.

>+    if (secureContext && !dom::IsSecureContext(cx)) {

Why is the "dom::" prefix needed?

>+    // XXXjwatt: Consider whether/when sandboxes should be able to see

Make this a folllowup bug, please.

r=me with the above dealt with.

Comment 41

[Jonathan Watt [:jwatt]]

(In reply to Boris Zbarsky [:bz] from comment #39)
> What you probably _should_ test is that exact IDL snippet but with the
> partial interface coming before the interface, and then checking that the
> members of results[0] have the right extended attribute.

results[0] is then the partial interface, and since you said not to bother testing that I'll assume you mean results[1] (the non-partial interface).

Comment 42

[Jonathan Watt [:jwatt]]

(In reply to Boris Zbarsky [:bz] from comment #40)
> Why is the "dom::" prefix needed?

It is not. It was simply to make it easier to distinguish between calls to this IsSecureContext function and calls to methods of the same name in textual search results. Since I've renamed this function given the change to make it return true if either the JSContext compartment _or_ the object compartment is a secure context, so there is no longer any use for the dom:: prefix.

Comment 43

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37cbe5c9d782
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e916fdec43b

Comment 44

[Boris Zbarsky [:bzbarsky]]

> I'll assume you mean results[1] (the non-partial interface)

Er, yes, that's what I meant.  ;)

Comment 45

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4823d0415d82

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/37cbe5c9d782
https://hg.mozilla.org/mozilla-central/rev/0e916fdec43b
https://hg.mozilla.org/mozilla-central/rev/4823d0415d82

Comment 47

[Boris Zbarsky [:bzbarsky]]

It might be worth documenting the interaction with system principals and Xrays and sandboxes and so forth...  But yes, the general documentation for this is the spec.

Comment 48

[Jean-Yves Perrier [:teoli]]

(In reply to Jonathan Watt [:jwatt] from comment #22)
> Adding dev-doc-needed to get this added to
> https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings

Bz, as this has been added to the WebIDL spec, do we still need to update this document? (I think we don't put standard extended attribute there, only custom ones: https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Custom_extended_attributes )

Comment 49

[Jean-Yves Perrier [:teoli]]

I tried to capture these info, but I am really not sure I am right.
Please review https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#SecureContext
and reset to dev-doc-needed if I misunderstood.