Closed Bug 622199 Opened 9 years ago Closed 8 years ago

Tampering with location.toString confuses Flash plugin

Categories

(Core :: Plug-ins, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- wanted
status1.9.1 --- wontfix

People

(Reporter: sirdarckcat, Assigned: jaas)

References

()

Details

(Keywords: dev-doc-complete, Whiteboard: [sg:vector-high][unhide the hidden comments in bug 577325 when opening this up])

Attachments

(1 file, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.13) Gecko/20101203 Firefox/3.6.13
Build Identifier: 

I can set:
   location.toString = function(){ return 'http://victim.rckc.at/'; }

And plugins, such as Flash that base security decisions on the value of Location will think my location is some other.

Check the PoC
   http://attack.rckc.at/html/firefoxLocationExploit.html

Reproducible: Always
Hmm.  Yeah, this is a pretty serious bug in Flash... Can we just give them an NPAPI hook for getting the location instead of the stupid javascript: hackery they do?
Assignee: general → nobody
Component: JavaScript Engine → Plug-ins
QA Contact: general → plugins
I emailed psirt@adobe.com.

There's a mozilla-only way of doing this safer in firefox:
http://mxr.mozilla.org/mozilla/source/dom/public/idl/core/nsIDOM3Document.idl#63

But, it's mozilla-only. :(

A specific NPAPI call to get information from the loaded page (with stuff like, location, origin [document.domain aware], sandbox state [in the case of iframe@sandbox], etc..) would be great to have.
In what sense is that "mozilla-only"?  That's a standard property on Document objects in DOM Level 3 Core.

The problem is that there is no way to get at it via the NPAPI, right?
Hi!

Sorry, I meant getting the location from the DOM.. But now that I think about it.. it may have the same problems that window.location has.. since it depends on getting ownerDocument of the Node, which could be tampered by an attacker (though I could be wrong about that).

Anyways, nvm.. I am not so familiar with all these stuff yet.. But an NPAPI to give information about the origin/location/sandbox/etc.. would solve another outstanding (unrelated) issue with Flash.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [sg:vector-high]
From Adobe:
"For now, we've logged Adobe tracking number 796 for this report."
Thanks for logging this with psert.  They will assign it out.
Note that this is similar to bug 541530.
We should get an owner for this.
Josh, can you add more detail about what WE need to fix here. From what I understand sg:vector implies that we think it's not our problem. Also sg:high's get addressed rather randomly. We need more detail here about exactly what needs to be fixed.
Currently there is no way for Flash to fix this bug, because the only information they have can be poisoned by the website. That means we need to implement a new API to let plugins get the information reliably, and then Flash (and other plugins) need to use it.
Alternately, we could make location.toString readonly and non-configurable...
As per Josh's request, I am assigning to him.
Assignee: nobody → joshmoz
Is this still a problem? I can reproduce using the testcase on 3.6.20pre but not in Nightly. If I use the web console I don't get an error setting location.toString to a function, but location.toString() still gives me the right answer.
The web console goes through various hoops to interact with a content page, so to verify this we'd need to write a testcase that actually tests this from a true content window.
Josh asked me to comment here.

Short term I think we should make whatever we need readonly as to make existing plugins not exploitable.

Long term we need a dedicated NPAPI to communicate the security context that a plugin runs in. Security stuff is important to get right and the easiest way to do that is through an explicit API. In gecko we not only use a explicit API whenever we're dealing with security stuff, we even use explicit types (nsIPrincipal) as to make it really clear that security information is involved.

This isn't the first time that we're bitten by the fact that plugins use the location object to make security decisions, and likely won't be the last.

So IMO we should ASAP add API to NPAPI to make it easy, safe and fast for plugins to know which security context they are running in.

Once we have such an API and plugins have transitioned to it, we should slowly transition to making the location object more sane.
Draft of an NPAPI proposal to address this is here:

https://wiki.mozilla.org/NPAPI:DocumentURL

I'll propose it to plugin-futures soon.
Wouldn't the document origin, as opposed to its url, make more sense?
I *suspect* pages use all of the URL for various things, but I agree, for security decisions only the origin makes more sense.
I have no problem with exposing the document url too (though which one?  There are at least two different ones, and the draft doesn't say which), but for security decisions the url is not just less sense but completely nonsense in many cases.
I'm fine with providing the origin instead of the URL. A URL might be helpful but that convenience is outside of the scope of this bug. Spec updated.
Looks good.  May want to link to http://www.whatwg.org/specs/web-apps/current-work/multipage/origin-0.html#unicode-serialization-of-an-origin as the spec for what an origin is (presumably the NPAPI would hand over the UTF-8 encoding of that Unicode serialization).

On the other hand, worth checking with the plugin folks whether the ASCII serialization from http://www.whatwg.org/specs/web-apps/current-work/multipage/origin-0.html#ascii-serialization-of-an-origin would work better for them.  The difference is whether IDN gets converted to punycode or not.
Thanks. Spec updated again. Will get feedback on the format choice when we submit it to plugin-futures.
Last time I checked, Firefox also has trouble with "top" being overrideable, which causes problem's for Flash's third-party cookie blocking.
Attached patch fix v1.0 (obsolete) — Splinter Review
This is a patch based on the current (unapproved) spec. I'd appreciate it if a reviewer could pay special attention to the format in which I format the origin handed to the plugin.

In this patch I assume NPNVdocumentOrigin is assigned a value of 22, which isn't final. Also, the NPAPI header change will need to be pulled from upstream when we actually land this.
Attachment #558965 - Flags: review?(bzbarsky)
Comment on attachment 558965 [details] [diff] [review]
fix v1.0

>+++ b/dom/plugins/base/nsNPAPIPlugin.cpp
>+    nsAutoString normalizedUTF16Origin;
>+    normalizer->NormalizeUnicodeNFKC(utf16Origin, normalizedUTF16Origin);

Can that fail?  If it does, you should probably throw or something?

>+    nsCAutoString utf8Origin;
>+    CopyUTF16toUTF8(normalizedUTF16Origin, utf8Origin);
>+
>+    *(char**)result = ToNewCString(utf8Origin);

  *(char**)result = ToNewUTF8String(normalizedUTF16Origin);

If the result is null, should we still be returning NPERR_NO_ERROR?

>+++ b/dom/plugins/ipc/PluginInstanceChild.cpp
>+            *static_cast<char**>(aValue) = ToNewCString(v);

I wish there were a way to avoid this copy, but I don't see one offhand.  :(

Maybe we should ask for a string API that will either hand you the buffer if the string owns it or copy if it doesn't?  Followup bug is fine.  Especially useful if |v| does own the buffer here.

I'm going to assume that the nptest.cpp code does not leak the string.

r=me with the above nits fixed.
Attachment #558965 - Flags: review?(bzbarsky) → review+
Comment on attachment 558965 [details] [diff] [review]
fix v1.0

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

::: dom/plugins/test/mochitest/test_NPNVdocumentOrigin.html
@@ +12,5 @@
> +    function runTest() {
> +      var p = document.getElementById("plugin1");
> +
> +      var originFromPlugin = p.getNPNVdocumentOrigin();
> +      is(originFromPlugin, "http://mochi.test:8888", "Checking for expected origin.");

It seems to me that you probably want to test the document origin returned with respect to various frobbings, since it's "origin" and not actual location.  The value should be invariant across document.domain modifications, right?  I'd test that, both in the case where the modification sets document.domain to its original value, and in the case where the modification sets it to a different value.  More trickily, I'd test how it works for a plugin written into an about:blank page opened by a separate page.  And I'd bet if I gave this a few more seconds' thought I could come up with another quirky case or two.  It's security code, more tests always seems better, even if they might tread toward exceeding belt-and-suspenders-ness.
(In reply to Boris Zbarsky (:bz) from comment #26)

> I'm going to assume that the nptest.cpp code does not leak the string.

Looks like it would at a glance but I just skip the release/malloc since we're returning the same string we got from the browser. We're responsible for releasing what we got from the browser, but we're not responsible for releasing what we send to the browser.
(In reply to Boris Zbarsky (:bz) from comment #29)

> I don't follow comment 28....

I think you were commenting on this bit of code in the test plugin:

>  char *origin = NULL;
>  NPError err = NPN_GetValue(npp, NPNVdocumentOrigin, &origin);
>  if (err != NPERR_NO_ERROR) {
>    return false;
>  }
>
>  STRINGZ_TO_NPVARIANT(origin, *result);
>  return true;

We're responsible for freeing the string we get from NPN_GetValue. We're responsible for allocating the string we use for STRINGZ_TO_NPVARIANT. They're the same in this case, so I just don't free the NPN_GetValue string and use it for STRINGZ_TO_NPVARIANT without duping (malloc). That's what I meant when I said "I just skip the release/malloc".

I think this is legit, let me know if I'm missing something.
No, that's fine.  I just wanted to make sure that we were responsible for allocating the string we pass to STRINGZ_TO_NPVARIANT.  Thanks for clarifying that!
(In reply to Boris Zbarsky (:bz) from comment #2)
> Hmm.  Yeah, this is a pretty serious bug in Flash... Can we just give them
> an NPAPI hook for getting the location instead of the stupid javascript:
> hackery they do?

I'm guessing it used to be safe back when we had CAPS restrictions on properties? And now that we use compartments (and wrappers), we've dropped the protections against tampering with window.location?
Attached patch fix v1.1Splinter Review
Attachment #558965 - Attachment is obsolete: true
> I'm guessing it used to be safe back when we had CAPS restrictions on properties?

Looking back at the CAPS prefs we had in the pre-wrapper timeframe, I don't think so....
Depends on: 686538
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/262cfa5c56ae
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Summary: location.toString is not protected against tampering → Tampering with location.toString confuses Flash plugin
Duplicate of this bug: 627891
Josh, has Adobe released a version of Flash that uses this API yet?  And do you know how much we care about pointing out the existence of the issue in Firefox 3.6?  I have a testcase I'm fixing in connection with bug 577325 that obliquely points out this issue, just wondering when it might be safe to land it and unhide the few hidden comments in that bug.
Whiteboard: [sg:vector-high] → [sg:vector-high][unhide the hidden comments in bug 577325 when opening this up]
This isn't really flash specific. Most plugins that have a security model of any sort uses the same method to figure out the origin of the plugin that's being run.

So it seems unlikely that we can revert this in a foreseeable future :(
This feature appears not to have been documented on MDN.  I added the smallest of blurbs to the NPN_GetValue page as a stopgap:

https://developer.mozilla.org/en/NPN_GetValue

...but that's totally not where this should be mentioned at all, if we want people to see it, learn of its existence, and start using it.  Keywording this, CCing sheppy to get whatever else needs doing here, done.  Relevant details are probably what's in this bug's comments plus the proposed spec (which I assume is also the actual adopted spec):

https://wiki.mozilla.org/NPAPI:DocumentOrigin

This was fixed in Firefox 10.
Keywords: dev-doc-needed
We should open this up once we EOL Firefox 3.6 (soon).
Depends on: 743552
Group: core-security
Depends on: 995896
You need to log in before you can comment on or make changes to this bug.