Closed Bug 1177957 Opened 9 years ago Closed 8 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: mt, Assigned: jwatt)

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(6 files, 3 obsolete files)

5.75 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
4.39 KB, patch
Details | Diff | Splinter Review
15.28 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
12.49 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
11.72 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
9.52 KB, patch
Details | Diff | Splinter Review
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.
Not a new context, a secure context.
Secure context meaning what? page loaded from https without any communication with http?
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.
S/asked/spec damb this phone.
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?
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?
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.
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.
See Also: → 1205866
See Also: → 1162772
Depends on: 1162772
See Also: 1162772
The annotation is [SecureContext] (https://w3c.github.io/webappsec-secure-contexts/#new).
The pull request to get this added to WebIDL 2 is currently a bit bogged down:

https://github.com/heycam/webidl/pull/65
Summary: Provide an annotation for exposing APIs only in secure contexts → Provide a [SecureContext] WebIDL extended attribute for exposing APIs only in secure contexts
Conversation has picked up again on public-script-coord:

https://lists.w3.org/Archives/Public/public-script-coord/2016JanMar/thread.html#msg104
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.
[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).
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.
Unrelated to that, but [SecureContext] stuff should be enabled in contexts where [ChromeOnly] is enabled.
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.
Attached patch patch - only hand tested (obsolete) — Splinter Review
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.
Assignee: nobody → jwatt
You should be able to just add tests under dom/bindings/test; mochitest will let you test https vs not.
Bear in mind that I'm unfamiliar with the bindings code, so this could do with close review.
Attachment #8734225 - Attachment is obsolete: true
Attachment #8748424 - Flags: review?(bzbarsky)
(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.
Adding dev-doc-needed to get this added to https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings
Keywords: dev-doc-needed
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?
Flags: needinfo?(jwatt)
> 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.
(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.
Flags: needinfo?(jwatt)
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.
Attachment #8748423 - Flags: review?(bzbarsky) → review-
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.
Attachment #8748424 - Flags: review?(bzbarsky) → review+
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.
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 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.
Attachment #8750845 - Flags: review-
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....
Hopefully this covers most things. I'll attach the parser changes themselves shortly as "part 1".
Attachment #8750845 - Attachment is obsolete: true
Attachment #8748424 - Attachment description: part 2 - Tests for the SecureContext] WebIDL extended attribute → part 4 - Tests for the [SecureContext] WebIDL extended attribute
Flags: needinfo?(bzbarsky)
(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 on attachment 8751509 [details] [diff] [review]
part 1 - Make the WebIDL parser support the [SecureContext] extented attribute

r=me.  Much better, thank you!
Attachment #8751509 - Flags: review+
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
Attachment #8751491 - Flags: review+
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.
Flags: needinfo?(bzbarsky)
Attachment #8751510 - Flags: review+
(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).
Blocks: 1273687
(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.
> I'll assume you mean results[1] (the non-partial interface)

Er, yes, that's what I meant.  ;)
Blocks: 1265767
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.
Flags: needinfo?(bzbarsky)
(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 )
Flags: needinfo?(bzbarsky)
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.
Component: DOM → DOM: Core & HTML
No longer depends on: DeprecateHTTP
See Also: 1205866
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: