1036214 - Do a subsumes check on |object| and |any| WebIDL parameters

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

We have a very nasty class of bugs that involves passing cross-origin objects to self-hosted APIs and convincing those APIs to manipulate the cross-origin objects in unsafe ways (see bug 928415, bug 924329, bug 1015540).

Bug 856067 is a major piece here, because it prevents content from passing tricky objects like { manifestURL: xoWin.location } (or some obscure non-[Object] equivalent).

However, this mechanism doesn't handle the case of passing a xoWin.location directly as an |object| or |any| parameter. This is clearly less of a risk, since there's less potential to massage the object into something duck-typable. But it's still a risk that we should shore up.

The answer here is to do a subsumes check during argument conversion for |object| and |any|. This should be pretty straightforward to do in the bindings code, and is the WebIDL analog of bug 930091 (which does this for COWs).

This is mostly a problem for JS-implemented APIs, but it's not clear to me whether there's any harm in doing it unconditionally. Are there any perf-sensitive APIs with object/any arguments?

Comment 1

[Boris Zbarsky [:bzbarsky]]

> Are there any perf-sensitive APIs with object/any arguments?

For "object", no.

For "any", somewhat.  Console API, history pushState/replaceState, some IDB stuff, postMessage of various sorts, setTimeout/setInterval, Promise APIs.  That said, for all of those the cost of the operation is already much higher than a subsumes check, especially in the common same-origin case, when I expect subsumes to be pretty cheap.

A bigger problem, I suspect, is correctness.  Should I be able to do Promise.resolve(xoWin.location)? I would think so, yet if we did a check there it wouldn't work.

Comment 2

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #1)
> A bigger problem, I suspect, is correctness.  Should I be able to do
> Promise.resolve(xoWin.location)? I would think so, yet if we did a check
> there it wouldn't work.

Maybe the best compromise then is to only do this for JS-implemented WebIDL, since those are the APIs that basically _can't_ handle this situation safely (whereas C++ reasonably can, as long as nobody uses UncheckedUnwrap).

Comment 3

[Boris Zbarsky [:bzbarsky]]

So in practice this would only affect JS-implemented things that want to have object/any args that allow cross-origin Window or Location, right?

Note that the impl _can_ handle this safely if all it does with the value is hand it back to the web page later...  But I agree that's tough to enforce.

One option would be to do this but then if we find an API that really just transparently passes through the values add a new WebIDL type or an extended attribute, I guess.

Comment 4

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #3)
> So in practice this would only affect JS-implemented things that want to
> have object/any args that allow cross-origin Window or Location, right?

Right.

> Note that the impl _can_ handle this safely if all it does with the value is
> hand it back to the web page later...  But I agree that's tough to enforce.

Exactly.

> One option would be to do this but then if we find an API that really just
> transparently passes through the values add a new WebIDL type or an extended
> attribute, I guess.

Yes. In practice I really doubt anyone is depending on this. I'd rather do the secure thing by default, and then add an opt-out if we need it.

Comment 5

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=4aeaf05d40bf

Comment 6

[Bobby Holley (:bholley)]

Attachment: Do a subsumes check on object and any parameters (and things containing them) to JS-implemented WebIDL. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Tests. v1

This whole test setup comes from bug 923904, so some of the things it's testing
need to be rejiggered (like the checkGlobal stuff).

Comment 8

[Bobby Holley (:bholley)]

This is green.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8465096 [details] [diff] [review]
Do a subsumes check on object and any parameters (and things containing them) to JS-implemented WebIDL. v1

So I think it might be worth it, in getJSToNativeConversionInfo, to only add the conditional bits if |not isinstance(descriptorProvider, Descriptor) or descriptorProvider.interface.isJSImplemented()|.  In other words, skip doing it when we know for a fact we don't need to.

>+        templateBody = ("if (${passedToJSImpl} && !CallerSubsumes(${val})) >{\n" +
>+                        indent("ThrowErrorMessage(cx, MSG_PERMISSION_DENIED_TO_PASS_ARG, \"%s\");\n"
>+                               "%s" % (sourceDescription, exceptionCode)) +
>+                       "}\n" +
>+                       templateBody)

how about:

  templateBody = dedent("""
      if (${passedToJSImpl} && !CallerSubsumes(${val})) {
        ThrowErrorMessage(cx, MSG_PERMISSION_DENIED_TO_PASS_ARG, "%s");
        %s
      }
      """ % (sourceDecription, exceptionCode.rstrip())) + templateBody

should be more readable.  Similar in the isAny() case.

>+                "passedToJSImpl": toStringBool(isJSImplementedDescriptor(descriptorProvider))

I suspect this should actually be:

  "passedToJSImpl": "${passedToJSImpl}"

(both places, for sequence and mozmap) because otherwise if you have a dictionary containing a sequence of any/object or a MozMap of any/object the right thing won't happen.  Add tests for these cases?

The subsumesCheck for unions in getJSToNativeConversionInfo seems over-broad to me: it'll do the check even if the union member type involved is a Location, no?  But at the same time it's too narrow; see below.

>@@ -8174,17 +8204,18 @@ def getUnionTypeTemplateVars(unionType, type, descriptorProvider,
>+            passedToJSImpl=toStringBool(isJSImplementedDescriptor(descriptorProvider)))

I think we should handle this similarly to how dictionaries do: by passing the boolean to the unions TrySetTo* methods and SetToObject.  Otherwise, consider a union of a dictionary and a long, say, with the dictionary having some object/any members.  I think your code as-is won't end up checking those members.  Again, add a test.

r=me with the above issues fixed.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8465097 [details] [diff] [review]
Tests. v1

r=me if you add some tests for deeper nesting (sequences of stuff, dictionaries containing sequences of stuff, etc).

Comment 11

[Bobby Holley (:bholley)]

Putting a MozMap in a dictionary asserts in Codegen when invoking getMemberTrace. I've added a comment in that function indicating that the appropriate tests should be added if that's ever supported.

Comment 12

[Bobby Holley (:bholley)]

Attachment: Do a subsumes check on object and any parameters (and things containing them) to JS-implemented WebIDL. v2 r=bz

This is technically r+ with comments, but I'll take another review if bz can
get to it.

Comment 13

[Bobby Holley (:bholley)]

Attachment: Tests. v2 r=bz

Comment 14

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=f7a9fc6c43bb

Comment 15

[Bobby Holley (:bholley)]

Hm, looks like the union stuff has rooting hazard: https://pastebin.mozilla.org/5832562

Comment 16

[Bobby Holley (:bholley)]

Attachment: Do a subsumes check on object and any parameters (and things containing them) to JS-implemented WebIDL. v3 r=bz

This fixes a rooting hazard in the previous patch.

Comment 17

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=a80310766e88

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8468860 [details] [diff] [review]
Do a subsumes check on object and any parameters (and things containing them) to JS-implemented WebIDL. v3 r=bz

>+            templateBody = (dedent("""

Using fill() here with $*{exceptionCode} seems like it would look nicer.  So like so:

            templateBody = fill("""
                if ($${passedToJSImpl} && !CallerSubsumes($${val})) {
                  ThrowErrorMessage(cx, MSG_PERMISSION_DENIED_TO_PASS_ARG, "${sourceDescription}");
                  $*{exceptionCode}
                }
                """,
                sourceDescription=sourceDescription,
                exceptionCode=exceptionCode) + templateBody

and note the indentation.  Also, the comment should say something about how the goal of the descriptorProvider checks is to skip outputting this block if we know we're not being used for a JS-implemented API.

Similar for the isAny() case.

>+                               "done = true;\n" % (unionArgumentObj, indent(exceptionCode.rstrip())))

I'd either use fill() here or drop the \n after the %s in the string and the rstrip() here.

>+          if (passedToJSImpl && !CallerSubsumes(obj)) {
>+              ThrowErrorMessage(cx, MSG_PERMISSION_DENIED_TO_PASS_ARG, "%s");

Binding code uses two-space indentation in C++.

>+MSG_DEF(MSG_PERMISSION_DENIED_TO_PASS_ARG, 0, "Permission denied to pass cross-origin object as an argument.")

This should have s/0/1/ and s/an argument/{0}/, I'd think.

r=me with those fixes.

Comment 19

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/715e09f33cd9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/13dae58b4a5e

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/715e09f33cd9
https://hg.mozilla.org/mozilla-central/rev/13dae58b4a5e

Comment 21

[Bobby Holley (:bholley)]

Comment on attachment 8468860 [details] [diff] [review]
Do a subsumes check on object and any parameters (and things containing them) to JS-implemented WebIDL. v3 r=bz

This is one of 3 bugs that fixes bug 856067, a sec-high. The other two bugs are on mozilla 33, but this one is on 34. I think we should take this on 33 so that we can effectively uplift the fix for bug 856067 by one cycle.

Approval Request Comment
[Feature/regressing bug #]: Longstanding.
[User impact if declined]: sec-high
[Describe test coverage new/current, TBPL]: Very heavy test coverage. Recently landed on m-c.
[Risks and why]: Relatively low risk. The only danger would be that we would generate a security check in case where we shouldn't have on per-spec. But the chances of that happening is low I think (especially given the test coverage). And even if it did happen, the chance of it actually breaking websites is very small.
[String/UUID change made/needed]: None.

If I'm on PTO and a backport is necessary, please flag Boris.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/57cdea2c224d
https://hg.mozilla.org/releases/mozilla-aurora/rev/ea25dee8ad3f

Comment 23

[Ryan VanderMeulen [:RyanVM]]

I had to back this out from Aurora for Android debug mochitest failures in the new test. Boris, can you please take a look?
https://hg.mozilla.org/releases/mozilla-aurora/rev/947dd9a0f12b

https://tbpl.mozilla.org/php/getParsedLog.php?id=46490075&tree=Mozilla-Aurora

Comment 24

[Ryan VanderMeulen [:RyanVM]]

The failures only occurred on Android debug. Not sure what makes it so special on Aurora.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Tests using this stuff apparently need skip-if = (debug == false || os == "android") in some cases?  Certainly http://hg.mozilla.org/mozilla-central/rev/9acc076ad18f did that.

Just do that for now and file a followup to figure out why that's needed?

Comment 26

[Boris Zbarsky [:bzbarsky]]

That said, some other tests that use it are not disabled there...  Again, just disabling on Aurora on Android, plus a followup, might make sense.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3e7c3624064d
https://hg.mozilla.org/releases/mozilla-aurora/rev/0acd8fe92235

Comment 28

[Boris Zbarsky [:bzbarsky]]

Ryan, did you end up filing a bug on the test not working on Android, or should I?

Comment 29

[Ryan VanderMeulen [:RyanVM]]

I have now. Bug 1057772.

Comment 30

[Matt Wobensmith [:mwobensmith][:matt:]]

We are relying on existing test coverage and will not be manually verifying this bug fix. If more testing is required, please give us details and we'll be happy to oblige.