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

RESOLVED FIXED in Firefox 33

Status

()

defect
RESOLVED FIXED
5 years ago
4 months ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

({sec-other})

unspecified
mozilla34
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox33 fixed, firefox34 fixed, firefox-esr31 wontfix)

Details

(Whiteboard: [adv-main33-])

Attachments

(2 attachments, 3 obsolete attachments)

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?
> 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.
(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).
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.
(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.
Posted patch Tests. v1 (obsolete) — Splinter Review
This whole test setup comes from bug 923904, so some of the things it's testing
need to be rejiggered (like the checkGlobal stuff).
Attachment #8465097 - Flags: review?(bzbarsky)
This is green.
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.
Attachment #8465096 - Flags: review?(bzbarsky) → review+
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).
Attachment #8465097 - Flags: review?(bzbarsky) → review+
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.
Blocks: 1048659
This is technically r+ with comments, but I'll take another review if bz can
get to it.
Attachment #8465096 - Attachment is obsolete: true
Attachment #8465097 - Attachment is obsolete: true
Attachment #8467556 - Flags: review?(bzbarsky)
Attachment #8467557 - Flags: review+
Hm, looks like the union stuff has rooting hazard: https://pastebin.mozilla.org/5832562
This fixes a rooting hazard in the previous patch.
Attachment #8467556 - Attachment is obsolete: true
Attachment #8467556 - Flags: review?(bzbarsky)
Attachment #8468860 - Flags: review?(bzbarsky)
Assignee: nobody → bobbyholley
OS: Mac OS X → All
Hardware: x86 → All
No longer blocks: 1048659
Depends on: 1048659
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.
Attachment #8468860 - Flags: review?(bzbarsky) → review+
Target Milestone: --- → mozilla34
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.
Attachment #8468860 - Flags: approval-mozilla-aurora?
Attachment #8468860 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Flags: needinfo?(bzbarsky)
The failures only occurred on Android debug. Not sure what makes it so special on Aurora.
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?
Flags: needinfo?(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.
Flags: needinfo?(ryanvm)
Ryan, did you end up filing a bug on the test not working on Android, or should I?
Flags: needinfo?(ryanvm)
I have now. Bug 1057772.
Flags: needinfo?(ryanvm)
Whiteboard: [adv-main33-]
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.
Flags: qe-verify-
Group: core-security → core-security-release
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.