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

RESOLVED FIXED in Firefox 49

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mt, Assigned: jwatt)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla49
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

(URL)

Attachments

(6 attachments, 3 obsolete attachments)

5.75 KB, patch
Details | Diff | Splinter Review
4.39 KB, patch
Details | Diff | Splinter Review
15.28 KB, patch
Details | Diff | Splinter Review
12.49 KB, patch
Details | Diff | Splinter Review
11.72 KB, patch
Details | Diff | Splinter Review
9.52 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
Not a new context, a secure context.

Comment 2

3 years ago
Secure context meaning what? page loaded from https without any communication with http?
(Reporter)

Comment 3

3 years ago
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.
(Reporter)

Comment 4

3 years ago
S/asked/spec damb this phone.

Comment 5

3 years ago
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?
(Reporter)

Comment 7

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

Updated

3 years ago
See Also: → bug 1205866
(Assignee)

Updated

3 years ago
See Also: → bug 1162772
(Assignee)

Updated

2 years ago
Depends on: 1162772
See Also: bug 1162772
(Reporter)

Comment 9

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

Updated

2 years ago
Blocks: 1247124
(Assignee)

Comment 10

2 years ago
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
(Assignee)

Comment 11

2 years ago
Conversation has picked up again on public-script-coord:

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

Comment 12

2 years ago
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.
(Assignee)

Comment 17

2 years ago
Created attachment 8734225 [details] [diff] [review]
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.
Assignee: nobody → jwatt
You should be able to just add tests under dom/bindings/test; mochitest will let you test https vs not.
Blocks: 1266494
(Assignee)

Comment 19

2 years ago
Created attachment 8748423 [details] [diff] [review]
part 1 - Provide a [SecureContext] WebIDL extended attribute for exposing APIs only in secure contexts
Attachment #8748423 - Flags: review?(bzbarsky)
(Assignee)

Comment 20

2 years ago
Created attachment 8748424 [details] [diff] [review]
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.
Attachment #8734225 - Attachment is obsolete: true
Attachment #8748424 - Flags: review?(bzbarsky)
(Assignee)

Comment 21

2 years ago
(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.
(Assignee)

Comment 22

2 years ago
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.
(Assignee)

Comment 25

2 years ago
(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+
(Assignee)

Comment 28

2 years ago
Created attachment 8750845 [details] [diff] [review]
part 1 - Provide a [SecureContext] WebIDL extended attribute for exposing APIs only in secure contexts
Attachment #8748423 - Attachment is obsolete: true
(Assignee)

Comment 29

2 years ago
Created attachment 8750847 [details] [diff] [review]
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.
(Assignee)

Comment 30

2 years ago
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....
(Assignee)

Comment 33

2 years ago
Created attachment 8751491 [details] [diff] [review]
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".
Attachment #8750845 - Attachment is obsolete: true
(Assignee)

Comment 34

2 years ago
Created attachment 8751509 [details] [diff] [review]
part 1 - Make the WebIDL parser support the [SecureContext] extented attribute
(Assignee)

Comment 35

2 years ago
Created attachment 8751510 [details] [diff] [review]
part 3 - Make the WebIDL code generator support the [SecureContext] extended attribute
(Assignee)

Updated

2 years ago
Attachment #8748424 - Attachment description: part 2 - Tests for the SecureContext] WebIDL extended attribute → part 4 - Tests for the [SecureContext] WebIDL extended attribute
(Assignee)

Updated

2 years ago
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 36

2 years ago
(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.
(Assignee)

Comment 37

2 years ago
Created attachment 8751521 [details] [diff] [review]
Effect on codegen of adding [SecureContext] to the main HTMLInputElement interface
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+
(Assignee)

Comment 41

2 years ago
(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).
(Assignee)

Updated

2 years ago
Blocks: 1273687
(Assignee)

Comment 42

2 years ago
(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.  ;)

Comment 46

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/37cbe5c9d782
https://hg.mozilla.org/mozilla-central/rev/0e916fdec43b
https://hg.mozilla.org/mozilla-central/rev/4823d0415d82
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49

Updated

2 years ago
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.