1297393 - Make passing of subject principals to webidl entry points explicit

Status: RESOLVED FIXED

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

Description

[Nika Layzell [:nika] (ni? for response)]

Right now if I need to know the subject principal of my called in a webidl function defined in C++, I need to call `nsContentUtils::SubjectPrincipal()` somewhere in order to get it. This makes this function unsafe to call from C++, because SubjectPrincipal() MOZ_CRASHes if called with no JS on the stack.

I have been bitten by this twice, where C++ code is consuming an API intended for JS, and calls the webidl endpoint with no JS on the stack. It would be nice if we had some way to make the fact that the endpoint cares about the subject principal more explicit through something like an explicit parameter on the C++ side.

This could take the form of a [explicit_principal] attribute or similar, which would pass the Subject principal as the second last argument, immediately before the ErrorResult argument.

Comment 1

[Bobby Holley (:bholley)]

Making it explicit and having the binding layer pass it sounds great to me.

Comment 2

[Nika Layzell [:nika] (ni? for response)]

bz, how difficult would something like this be to implement, and is it something which we would want?

The idea is that the attribute would cause the generated function calls in the binding code to also pass the subject principal explicitly.

Comment 3

[Bobby Holley (:bholley)]

My guess is that the binding work is very easy, and the hard part is adding the attribute and updating the signatures for all the consumers that need it.

Comment 4

[Boris Zbarsky [:bzbarsky]]

The binding bits are very simple.

Whether we want it, I'm torn.  The main pro is making the subject principal explicit and maybe allowing decoupling it from the current compartment.  The main con is decoupling it from the current compartment and whether that violates someone's assumptions...

I think on balance this is probably worth doing.

Comment 5

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #4)
> The binding bits are very simple.
> 
> Whether we want it, I'm torn.  The main pro is making the subject principal
> explicit and maybe allowing decoupling it from the current compartment.  The
> main con is decoupling it from the current compartment and whether that
> violates someone's assumptions...

Well, the caller will still need to explicitly pass it everywhere that it is used. So I'm not really sure what the threat model is. It's not like it will persist across script execution or anything.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Sure.  I'm not saying I have a coherent worry here, just a general vague one and one specific implementation concern: we'll need to change which principal we pass on codepaths where we right now change the compartment to affect the principal.  This shouldn't be hard to do; we just have to not blindly pass aSubjectPrincipal when there was a current compartment modification in our stackframe.

Comment 7

[(no longer active)]

Baku said he'll help with this!

Comment 8

[Andrea Marchesini [:baku]]

> This could take the form of a [explicit_principal] attribute or similar,
> which would pass the Subject principal as the second last argument,
> immediately before the ErrorResult argument.

nsIPrincipal is main-thread only. We should pass a nullptr if a [ExplicitPrincipal] method is called in workers.

Comment 9

[Andrea Marchesini [:baku]]

Attachment: part 1 - WebIDL [ExplicitPrincipal]

Comment 10

[Andrea Marchesini [:baku]]

Attachment: part 1 - WebIDL [ExplicitPrincipal]

Comment 11

[Andrea Marchesini [:baku]]

Attachment: part 1 - WebIDL [ExplicitPrincipal]

Comment 12

[Andrea Marchesini [:baku]]

Attachment: part 2 - DataTransferItem

Comment 13

[Andrea Marchesini [:baku]]

Attachment: part 3 - DataTransferItemList

Comment 14

[Andrea Marchesini [:baku]]

Attachment: part 5 - location

Comment 15

[Andrea Marchesini [:baku]]

Attachment: part 6 - CSSStyleSheet

Comment 16

[Andrea Marchesini [:baku]]

Attachment: part 4 - location pre explicitPrincipal

Comment 17

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

(In reply to Andrea Marchesini [:baku] from comment #8)
> nsIPrincipal is main-thread only. We should pass a nullptr if a
> [ExplicitPrincipal] method is called in workers.

Hmm, I'm not quite sure about that. Someone could easily start to interpret nullptr as if C++ had called the method with nullptr.
Are there other options here?  Something like Maybe<nsIPrincipal*> , where not-initialized value means the callee must do something sane? Or something else ?

Comment 18

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

Comment on attachment 8791616 [details] [diff] [review]
part 4 - location pre explicitPrincipal

Sorry, I'm not quite happy with this approach. This makes it even more harder to fix the issue that we may throw Gecko-internal errors to the web.
I would hope either: some code uses only error types defined in webidl and dom and in other specs and then passing ErrorResult everywhere is fine, or use nsresult for Gecko internal stuff and ErrorResult for the case when we're about to pass the value to bindings layer. (and we should then just fix layer closest to bindings to not use gecko-only errors, and not fix the issue everywhere.)

If you disagree, please explain why and ask review again.


I do think we should either start doing some automatic conversion from gecko errors to web exposed errors in ErrorResult, or MOZ_ASSERT in ErrorResult that only web exposed error types are used with it. Probably both, so that even if debug builds wouldn't catch all the cases using MOZ_ASSERT, we wouldn't accidentally expose anything Gecko internal error values to the web in opt builds but rather some (possibly wrong) web errors.

Comment 19

[Boris Zbarsky [:bzbarsky]]

There are some options, yes.  Offhand:

1)  Pass a Maybe<nsIPrincipal*>.  This requires an NS_IsMainThread check.  Or maybe we can get away with a branch on the principal value (that is, in the binding code convert null to empty Maybe).

2)  Forbid this annotation on APIs exposed in workers.  Not clear yet whether this would restrict its use in practice or not...  We don't actually have _that_ maybe SubjectPrincipal() calls in our code, and none are in worker-exposed stuff at first glance.  We have more IsCallerChrome() callers, but I think they're all mainthread-only stuff (I mean, the function itself is; I mean the calling code in general).  ThreadsafeIsCallerChrome could be a problem, if we want to replace it with this API.  But that wouldn't work anyway, because on workers the principal is not what represents chromeness.

We may need a separate annotation for "tell me if my caller is chrome" or something.

Comment 20

[Andrea Marchesini [:baku]]

Attachment: part 4 - location pre explicitPrincipal

Comment 21

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

Comment on attachment 8792550 [details] [diff] [review]
part 4 - location pre explicitPrincipal

I don't understand why *Internal stuff starts to use ErrorResult.
Am I misunderstanding something here?

Comment 22

[Andrea Marchesini [:baku]]

Attachment: part 1 - WebIDL [ExplicitPrincipal]

Comment 23

[Andrea Marchesini [:baku]]

Attachment: part 2 - DataTransferItem

Comment 24

[Andrea Marchesini [:baku]]

Attachment: part 3 - DataTransferItemList

Comment 25

[Andrea Marchesini [:baku]]

Attachment: part 4 - location

Comment 26

[Andrea Marchesini [:baku]]

Attachment: part 5 - CSSStyleSheet

Comment 27

[Peter Van der Beken [:peterv]]

Comment on attachment 8793643 [details] [diff] [review]
part 1 - WebIDL [ExplicitPrincipal]

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

::: dom/bindings/Codegen.py
@@ +7424,5 @@
>                                                            idlNode.identifier.name))
>          else:
>              cgThings.append(CGCallGenerator(
>                  self.isFallible(),
> +                self.explicitPrincipal(),

Can't this use idlNode.getExtendedAttribute('ExplicitPrincipal')?

@@ +13831,5 @@
>          elif returnType.isObject() or returnType.isSpiderMonkeyInterface():
>              args.append(Argument("JS::MutableHandle<JSObject*>", "aRetVal"))
>  
> +        # And the nsIPrincipal
> +        if 'explicitPrincipal' in self.extendedAttrs:

And here self.member.getExtendedAttribute('ExplicitPrincipal')?

::: dom/bindings/Configuration.py
@@ +582,5 @@
>              attrs = self.extendedAttributes['all'].get(name, [])
>              maybeAppendInfallibleToAttrs(attrs, throws)
> +
> +            if member.getExtendedAttribute("ExplicitPrincipal"):
> +                attrs.append("explicitPrincipal")

And then you don't need these changes in Configuration.py

Comment 28

[(no longer active)]

This is no longer needed for bug 1297687.

Comment 29

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

Comment on attachment 8793643 [details] [diff] [review]
part 1 - WebIDL [ExplicitPrincipal]

ehsan said he could review this. I'm rather overloaded with reviews right now.

Comment 30

[(no longer active)]

Comment on attachment 8793643 [details] [diff] [review]
part 1 - WebIDL [ExplicitPrincipal]

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

r=me with Peter's comments addressed.

Comment 31

[(no longer active)]

Comment on attachment 8793644 [details] [diff] [review]
part 2 - DataTransferItem

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

::: dom/webidl/DataTransferItem.webidl
@@ +14,2 @@
>    void getAsString(FunctionStringCallback? _callback);
> +  [Throws,ExplicitPrincipal]

Actually, I'm not a fan of the name ExplicitPrincipal.  How about [NeedsSubjectPrincipal]?

Also, as a nit, space after comma!

Comment 32

[(no longer active)]

Comment on attachment 8793645 [details] [diff] [review]
part 3 - DataTransferItemList

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

::: dom/events/DataTransfer.cpp
@@ +476,5 @@
>  {
>    Optional<nsAString> format;
>    format = &aFormat;
>    ErrorResult rv;
> +  ClearData(format, Some(nsContentUtils::SubjectPrincipal()), rv);

It's quite sad that you have to do this to support nsIDOMDataTransfer.  :(

But it turns out that there's no consumers for the XPCOM versions of clearData() or mozClearDataAt().  Therefore, I think it's better to remove both of these XPCOM methods from nsIDOMDataTransfer.  What do you think?

(This can obviously be done in a follow-up!)

Comment 33

[(no longer active)]

Comment on attachment 8793646 [details] [diff] [review]
part 4 - location

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

::: dom/base/Location.cpp
@@ +914,4 @@
>  {
> +  if (!aSubjectPrincipal) {
> +    return false;
> +  }

This should never happen, so please MOZ_ASSERT instead.

Comment 34

[(no longer active)]

Comment on attachment 8793647 [details] [diff] [review]
part 5 - CSSStyleSheet

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

::: layout/style/CSSStyleSheet.cpp
@@ +1820,5 @@
>  CSSStyleSheet::GetCssRules(nsIDOMCSSRuleList** aCssRules)
>  {
>    ErrorResult rv;
> +  nsCOMPtr<nsIDOMCSSRuleList> rules =
> +    GetCssRules(Some(nsContentUtils::SubjectPrincipal()), rv);

Same comment for these ones as well.

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e716d57f4b2
Make passing of subject principals to webidl entry points explicit - part 1 - WebIDL [NeedsSubjectPrincipal], r=ehsan, r=peterv
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a5ddd3a3758
Make passing of subject principals to webidl entry points explicit - part 2 - DataTransferItem, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3d03e3a73d3
Make passing of subject principals to webidl entry points explicit - part 3 - DataTransferItemList, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc5925a07224
Make passing of subject principals to webidl entry points explicit - part 4 - Location, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/68898ebb4390
Make passing of subject principals to webidl entry points explicit - part 5 - CSSStyleSheet, r=ehsan

Comment 36

[Andrea Marchesini [:baku]]

> >  CSSStyleSheet::GetCssRules(nsIDOMCSSRuleList** aCssRules)

> Same comment for these ones as well.

They are used by addons.

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0e716d57f4b2
https://hg.mozilla.org/mozilla-central/rev/6a5ddd3a3758
https://hg.mozilla.org/mozilla-central/rev/a3d03e3a73d3
https://hg.mozilla.org/mozilla-central/rev/cc5925a07224
https://hg.mozilla.org/mozilla-central/rev/68898ebb4390