Eduardo Vela N Reporter
	Description • 14 years ago

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

	Eduardo Vela N Reporter
	Comment 1 • 14 years ago

Related:
http://jsbin.com/window.location

Kinda the same:
https://bugzilla.mozilla.org/show_bug.cgi?id=541530

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 14 years ago

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?

	Eduardo Vela N Reporter
	Comment 3 • 14 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 14 years ago

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?

	Eduardo Vela N Reporter
	Comment 5 • 14 years ago

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.

	Eduardo Vela N Reporter
	Comment 6 • 14 years ago

From Adobe:
"For now, we've logged Adobe tracking number 796 for this report."

	Charles
	Comment 7 • 14 years ago

Thanks for logging this with psert.  They will assign it out.

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 13 years ago

Note that this is similar to bug 541530.

	Josh Aas Assignee
	Comment 9 • 13 years ago

We should get an owner for this.

	Sheila Mooney
	Comment 10 • 13 years ago

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.

	Benjamin Smedberg
	Comment 11 • 13 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 13 years ago

Alternately, we could make location.toString readonly and non-configurable...

	Sheila Mooney
	Comment 13 • 13 years ago

As per Josh's request, I am assigning to him.

	Daniel Veditz [:dveditz]
	Comment 14 • 13 years ago

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.

	Johnny Stenback (:jst)
	Comment 15 • 13 years ago

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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 16 • 13 years ago

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.

	Josh Aas Assignee
	Comment 17 • 13 years ago

Draft of an NPAPI proposal to address this is here:

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

I'll propose it to plugin-futures soon.

	Boris Zbarsky [:bzbarsky]
	Comment 18 • 13 years ago

Wouldn't the document origin, as opposed to its url, make more sense?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 19 • 13 years ago

I *suspect* pages use all of the URL for various things, but I agree, for security decisions only the origin makes more sense.

	Boris Zbarsky [:bzbarsky]
	Comment 20 • 13 years ago

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.

	Josh Aas Assignee
	Comment 21 • 13 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 22 • 13 years ago

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.

	Josh Aas Assignee
	Comment 23 • 13 years ago

Thanks. Spec updated again. Will get feedback on the format choice when we submit it to plugin-futures.

	Adam Barth
	Comment 24 • 13 years ago

Last time I checked, Firefox also has trouble with "top" being overrideable, which causes problem's for Flash's third-party cookie blocking.

	Josh Aas Assignee
	Comment 25 • 13 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 26 • 13 years ago

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.

	Jeff Walden [:Waldo]
	Comment 27 • 13 years ago

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.

	Josh Aas Assignee
	Comment 28 • 13 years ago

(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.

	Boris Zbarsky [:bzbarsky]
	Comment 29 • 13 years ago

I don't follow comment 28....

	Josh Aas Assignee
	Comment 30 • 13 years ago

(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.

	Boris Zbarsky [:bzbarsky]
	Comment 31 • 13 years ago

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!

	Jesse Ruderman
	Comment 32 • 13 years ago

(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?

	Boris Zbarsky [:bzbarsky]
	Comment 34 • 13 years ago

> 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....

	Josh Aas Assignee
	Comment 35 • 13 years ago

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/262cfa5c56ae

	Jeff Walden [:Waldo]
	Comment 37 • 13 years ago

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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 38 • 13 years ago

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 :(

	Jeff Walden [:Waldo]
	Comment 39 • 13 years ago

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.

	Josh Aas Assignee
	Comment 40 • 13 years ago

We should open this up once we EOL Firefox 3.6 (soon).

	Eric Shepherd [:sheppy]
	Comment 41 • 13 years ago

Documentation updated:

https://developer.mozilla.org/en/Gecko_Plugin_API_Reference/Plug-in_Development_Overview#Working_with_URLs

And mentioned on Firefox 10 for developers.