1174733 - Browser API: we should be able to execute any script remotely

Status: RESOLVED FIXED

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

Description

[Paul Rouget [:paul]]

Webkit has a webview method that allows script execution in content.

For example:

> webview.executeScript({ code: "document.body.style.backgroundColor = 'red'" })

Maybe we could do the same thing with the Browser API.

Comment 1

[Paul Rouget [:paul]]

Attachment: proof of concept

Comment 2

[Paul Rouget [:paul]]

Example:

iframe.evalSource(`
new Promise((resolve, reject) => {
  setTimeout(() => resolve({foo: 42}), 2000);
})
`).then(rv => {
  console.log(rv);
}, error => {
  console.error(error);
});

Comment 3

[Paul Rouget [:paul]]

Note about attachment 8622481 [details] [diff] [review]: maybe we should not force the use of a promise.

Comment 4

[[:fabrice] Fabrice Desré]

What's the use case here? That looks similar to what you get from add-ons, only a bit more ugly because you send a string around.

Comment 5

[Paul Rouget [:paul]]

(In reply to Fabrice Desré [:fabrice] from comment #4)
> What's the use case here?

For example, we want to experiment with building a "card" to represent a website. We need to go through images in the DOM, extract colors and such. Building an API just for that will be bothersome as we might have to tweak the algorithm in the future and request the extend or deprecate API methods.

We don't want to be limited by the browser api.

Also, see all the use cases for webview.executeScript on Google.

> That looks similar to what you get from add-ons

Maybe. But why would there be a 2nd way to manipulate the iframe when there's the browser api?

Comment 6

[Paul Rouget [:paul]]

Comment on attachment 8622481 [details] [diff] [review]
proof of concept

I'm going to clean up this patch, write some tests, and post to dev-platform. I think this requires a broader discussion.

Comment 7

[Paul Rouget [:paul]]

Attachment: v1

Comment 8

[Paul Rouget [:paul]]

Updated the API and 

> let foo = 42;
> iframe.executeScript(`
> new Promise((resolve, reject) => {
>   setTimeout(() => resolve({foo: ${foo + 1}}), 2000);
> })
> `).then(rv => {
>   console.log(rv);
> }, error => {
>   console.error(error);
> });


And:

> iframe.executeScript("42 + 1").then(rv => {
>   console.log(rv);
> }, error => {
>   console.error(error);
> });

Comment 9

[Paul Rouget [:paul]]

Comment on attachment 8623047 [details] [diff] [review]
v1

Paul, I'd would very much like to have your feedback on this feature. There's also a thread on dev-platform about this. Feel free to comment here or in the thread.

Comment 10

[Paul Rouget [:paul]]

This is what I said in the dev-platform thread:

For some context: the browser.html project needs access to the DOM to
build some sort of tab previews (not a screenshot, something based on
colors, headers and images from the page), and we don't feel like
adding more and more methods to the Browser API to collect all the
information we need. It's just easier to be able to inject a script
and tune the preview algorithm in the system app instead of changing
the API all the time we need a new thing. It also doesn't sound like a
terrible thing to do as other vendors do a similar thing (Android's
executeScript, iOS's stringByEvaluatingJavaScriptFromString, and IE's
InvokeScript).

Comment 11

[Paul Rouget [:paul]]

Link to dev-paltform post: https://groups.google.com/d/msg/mozilla.dev.platform/iIHHFvCSoow/e_laA7zsck8J

Comment 12

[Paul Rouget [:paul]]

We think we should have a special permission for this method. "universalxss" might a good name :)

Comment 13

[Paul Rouget [:paul]]

Jonas mentioned:

[…] it might be a good idea to add some security features to the
API. I.e. something like:

browserAPI.executeScript(script, { origin: "http://a.com" });
and
browserAPI.executeScript(script, { url: "http://a.com/b.html" });

which would only execute the script if the url/origin match.

Comment 14

[Paul Rouget [:paul]]

Attachment: v2

Comment 15

[[:fabrice] Fabrice Desré]

I'll defer to Paul & Jonas, but I'm not a big fan of passing the script source instead of a script url. We could check that the script url is same origin with the caller of the browser api to prevent arbitrary script injection if the embedder has an xss flaw.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8623654 [details] [diff] [review]
v2

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

::: dom/apps/PermissionsTable.jsm
@@ +145,5 @@
>                             },
> +                           "browser:universalxss": {
> +                             app: DENY_ACTION,
> +                             trusted: DENY_ACTION,
> +                             privileged: DENY_ACTION,

I don't see a reason to deny this to privileged scripts.

This permission is no worse than systemXHR. I.e. it doesn't allow doing anything that systemXHR doesn't.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Other than that the patch looks good to me. But I don't know the code well enough to review, so it's better if someone that knows the browser API does.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Actually, only thing I think we should change about the API is to *require* that the 'origin' or 'url' is passed.

Comment 19

[Paul Rouget [:paul]]

(In reply to Fabrice Desré [:fabrice] from comment #15)
> I'll defer to Paul & Jonas, but I'm not a big fan of passing the script
> source instead of a script url. We could check that the script url is same
> origin with the caller of the browser api to prevent arbitrary script
> injection if the embedder has an xss flaw.

The problem is then you are forced to embed your code inside a file, and you can't change anything during runtime. That would force people to use a dataurl and do some ugly encoding.

Comment 20

[Paul Rouget [:paul]]

Attachment: v3

Addressed Jonas' comments.

I didn't manage to enforce the url/origin option within the webidl. The check is done in BrowserElementParent.js.

Also, talked to Paul last week, he told me he was ok with this functionality as long as there's a new permission for it.

Comment 21

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8627596 [details] [diff] [review]
v3

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

::: dom/browser-element/BrowserElementChildPreload.js
@@ +1015,5 @@
> +    let sandbox = new Cu.Sandbox(content, {
> +      sandboxPrototype: content,
> +      wantXrays: true,
> +      sandboxName: "browser-api-execute-script"
> +    });

This looks correct to me. But I would like bholley to double check.

@@ +1016,5 @@
> +      sandboxPrototype: content,
> +      wantXrays: true,
> +      sandboxName: "browser-api-execute-script"
> +    });
> +    

nit: trailing white spaces

::: dom/browser-element/mochitest/browserElement_ExecuteScript.js
@@ +11,5 @@
> +SpecialPowers.addPermission("browser:universalxss", true, window.document);
> +
> +addEventListener('unload', function() {
> +  SpecialPowers.removePermission("browser:universalxss", window.document);
> +});

Use SpecialPowers.pushPermissions instead of add/remove manually.

@@ +28,5 @@
> +  const bail = () => {
> +    ok(false, `scriptId: ${scriptId++}`);
> +  }
> +
> +  let validOption = { url: 'data:text/html,foo' };

Should also check "uri" option.

@@ +78,5 @@
> +    }).then(bail, (error) => {
> +      is(error.name, 'Value returned (resolve) by promise is not a valid JSON object', `scriptId: ${scriptId++}`);
> +      return iframe.executeScript('42', {})
> +    }).then(bail, error => {
> +      console.log(42, error);

nit: leftover?

::: dom/webidl/BrowserElement.webidl
@@ +171,5 @@
>    void clearMatch();
>  
> +  [Throws,
> +   Pref="dom.mozBrowserFramesEnabled",
> +   CheckPermissions="browser browser:universalxss"]

Unfortunately this does not do what you want. It means both "browser" and "browser:universalxss" could use this API.
For now you have to check one of the additional permission in code like setNFCFocus. It also means this API will be visible for the permissions listed.

So you have already done the check in nsBrowserElement.cpp you could remove "browser:universalxss" here.

Comment 22

[Bobby Holley (:bholley)]

Comment on attachment 8627596 [details] [diff] [review]
v3

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

Seems good, but f- to make sure those two points get addressed.

::: dom/apps/PermissionsTable.jsm
@@ +145,5 @@
>                             },
> +                           "browser:universalxss": {
> +                             app: DENY_ACTION,
> +                             trusted: DENY_ACTION,
> +                             privileged: ALLOW_ACTION,

This seems to be based on Jonas' review comments in comment 11, yeah? My objection in [1] still stands, and I don't think it's been addressed yet.

So either Jonas and I need to hash this out, or we should just restrict it everywhere and only use it in browser.html for now.

[1] https://groups.google.com/d/msg/mozilla.dev.platform/iIHHFvCSoow/Lp8kBb3KQ0oJ

::: dom/browser-element/BrowserElementChildPreload.js
@@ +1013,5 @@
> +    }
> +
> +    let sandbox = new Cu.Sandbox(content, {
> +      sandboxPrototype: content,
> +      wantXrays: true,

So you're choosing to use Xrays between your script and the content page, right? I think that's the right decision. But in order to do that properly, you need to pass |[content]| as the principal (rather than just |content|) in order to give yourself Xrays, and remove the deprecated 'wantXrays' field.

Comment 23

[Paul Rouget [:paul]]

(In reply to Bobby Holley (:bholley) from comment #22)
> Comment on attachment 8627596 [details] [diff] [review]
> v3
> 
> Review of attachment 8627596 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems good, but f- to make sure those two points get addressed.
> 
> ::: dom/apps/PermissionsTable.jsm
> @@ +145,5 @@
> >                             },
> > +                           "browser:universalxss": {
> > +                             app: DENY_ACTION,
> > +                             trusted: DENY_ACTION,
> > +                             privileged: ALLOW_ACTION,
> 
> This seems to be based on Jonas' review comments in comment 11, yeah? My
> objection in [1] still stands, and I don't think it's been addressed yet.
> 
> So either Jonas and I need to hash this out, or we should just restrict it
> everywhere and only use it in browser.html for now.
> 
> [1]
> https://groups.google.com/d/msg/mozilla.dev.platform/iIHHFvCSoow/Lp8kBb3KQ0oJ

Quoting [1]:

> But that says nothing about multiple consumers of the browser API, right?
> The origin information gives us one bit that tells us whether we're in a
> browser element or not. The Browser App uses mozbrowser, and that's where
> users enter all their sensitive data. With the proposed API, this sensitive
> data would be vulnerable to XSS from any other app using the browser API.

Can you give an example? Are you thinking of an app that would embed another app
and use executeScript against the other app? I could restrict executeApp targets
to non-app docshells.

There's the "browser" privilege, and the "executeScript" privilege. The "executeScript"
privilege is not used in Gaia's apps. But I could just disable it directly in Gecko for
non browser.html builds.

> ::: dom/browser-element/BrowserElementChildPreload.js
> @@ +1013,5 @@
> > +    }
> > +
> > +    let sandbox = new Cu.Sandbox(content, {
> > +      sandboxPrototype: content,
> > +      wantXrays: true,
> 
> So you're choosing to use Xrays between your script and the content page,
> right? I think that's the right decision. But in order to do that properly,
> you need to pass |[content]| as the principal (rather than just |content|)
> in order to give yourself Xrays, and remove the deprecated 'wantXrays' field.

Understood.

Comment 24

[Bobby Holley (:bholley)]

(In reply to Paul Rouget [:paul] from comment #23)
> > But that says nothing about multiple consumers of the browser API, right?
> > The origin information gives us one bit that tells us whether we're in a
> > browser element or not. The Browser App uses mozbrowser, and that's where
> > users enter all their sensitive data. With the proposed API, this sensitive
> > data would be vulnerable to XSS from any other app using the browser API.
> 
> Can you give an example? Are you thinking of an app that would embed another
> app
> and use executeScript against the other app? I could restrict executeApp
> targets
> to non-app docshells.

So. The discussion here is about whether iframe.executeScript adds any fundamentally new security surface than we had before. Jonas argues that it's not any different from System XHR (i.e. being able to fetch cross-origin resources), because everything that happens inside of a browser frame gets a separate origin and therefore a sandboxed cookie jar, which means that localStorage, cookies, indexedDB, etc all access a different set of data from "normal" iframes.

My objection was that, assuming that the user is using the browser app (which uses a mozbrowser) to browse the web, the "inBrowser=true" origin is actually the origin where all of the sensitive data lives. So if we have some other random app (that is not the browser app) using the mozBrowser API, it would be problematic to grant it universal XSS to all of that data.

Thinking about it again, I think my concern is misguided, because from my reading of the code, apps inside browser frames _also_ have the appId of the app that created the mozBrowser (I originally thought they didn't, since we wouldn't want to incorrectly say that browser content is part of the app, but it looks like we handle that case at [1]). So if two different apps use mozBrowsers, the content inside each of them have (appId, inBrowser) tuples of (X, true) and (Y, true) where X != Y, which means that private data is properly sandboxed between. them. Do I have that right, pauljt?

If so, my remaining concern is more of a UX one. If a.com loads b.com in an iframe, users are accustomed to being able to enter their passwords into b.com without having them leak to a.com. Do we really want the browser app to be able to break this metaphor? What do you think pauljt?

[1] https://hg.mozilla.org/mozilla-central/annotate/e7e69cc8c07b/caps/nsScriptSecurityManager.cpp#l267

> There's the "browser" privilege, and the "executeScript" privilege. The
> "executeScript"
> privilege is not used in Gaia's apps. But I could just disable it directly
> in Gecko for
> non browser.html builds.

It depends on whether other, non-browser.html consumers would find this API useful. What do you think, Kan-Ru?

> > So you're choosing to use Xrays between your script and the content page,
> > right? I think that's the right decision. But in order to do that properly,
> > you need to pass |[content]| as the principal (rather than just |content|)
> > in order to give yourself Xrays, and remove the deprecated 'wantXrays' field.
> 
> Understood.

Thinking about this one some more, I think we also need to disable Xray waivers on these sandboxes. Waivers are basically impossible for architectures like Blink to implement, and so if people start using .wrappedJSObject, those scripts will be fundamentally incompatible with any standardized version of this API. So I think we should save ourselves the trouble by forbidding that from the outset.

Paul R, if you file a bug blocking this one and needinfo me, I will implement an "allowWaivers" option on the sandbox ctor.

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> Thinking about it again, I think my concern is misguided, because from my
> reading of the code, apps inside browser frames _also_ have the appId of the
> app that created the mozBrowser (I originally thought they didn't, since we
> wouldn't want to incorrectly say that browser content is part of the app,
> but it looks like we handle that case at [1]). So if two different apps use
> mozBrowsers, the content inside each of them have (appId, inBrowser) tuples
> of (X, true) and (Y, true) where X != Y, which means that private data is
> properly sandboxed between. them. Do I have that right, pauljt?

Right, the mozbrowser in two separate apps get two separate cookie jars. So if MyEvilApp uses a mozbrowser, the content inside that mozBrowser will use a separate cookie jar from the pages inside the browser.

> If so, my remaining concern is more of a UX one. If a.com loads b.com in an
> iframe, users are accustomed to being able to enter their passwords into
> b.com without having them leak to a.com. Do we really want the browser app
> to be able to break this metaphor? What do you think pauljt?

If the user navigates to a.com, which inside it displays b.com, and the user then enters the b.com password, then the user has lost no matter what.

What the user typed their password into might not have been b.com at all, but rather a part of the a.com page that was simply made to look like b.com. In fact, a.com might be dynamically loading stuff from b.com using systemXHR and rendering a correct-looking website, but that's still part of a.com.

Or it might actually be b.com that is rendered, but a.com is overlaying a text input at the same location and grabbing your password that way.

Don't type a password into a window if you don't trust the URL at the top of that window with the password.

Comment 26

[Bobby Holley (:bholley)]

Ok, that seems reasonable enough to me.

Comment 27

[Paul Rouget [:paul]]

Attachment: v4 (r=kchen)

Addressed Kanru's comment.

Fixed sandbox instantiation. Test will fail until allowWaivers is supported (bug 1182409).

Comment 28

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Bobby Holley (:bholley) from comment #24)
> > There's the "browser" privilege, and the "executeScript" privilege. The
> > "executeScript"
> > privilege is not used in Gaia's apps. But I could just disable it directly
> > in Gecko for
> > non browser.html builds.
> 
> It depends on whether other, non-browser.html consumers would find this API
> useful. What do you think, Kan-Ru?

Gaia might be interested to use this to experiment things so I think it's fine to expose it with additional permissions.

Comment 29

[Frederik Braun [:freddy] (reo duty for Fx124)]

I'm concerned that this is diluting the current security strategy for the browser API, where we have carefully and selectively added features to it that make sense and do not endanger the user or the website that is being framed (stealing cookies, looking into other origins, etc.).

Comment 30

[Bobby Holley (:bholley)]

(In reply to Frederik Braun [:freddyb] from comment #29)
> I'm concerned that this is diluting the current security strategy for the
> browser API

Is comment 25 not compelling for you? Can you explain why?

> where we have carefully and selectively added features to it
> that make sense and do not endanger the user or the website that is being
> framed (stealing cookies, looking into other origins, etc.).

We already allow screenshotting, FWIW, which allows stealing lots of sensitive data.

Comment 31

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We also allow clickjacking which essentially allows injecting arbitrary user clicks.

In effect, I don't actually agree that we have "carefully and selectively added features". Even the current API allows enough control of the content inside the iframe that we have to assume that sensitive data can be extracted.

I think we'll make our, and developers, lives easier if we simply treat the browser API as "the embedding page has full control over the contained page, but that's ok since it's all sandboxed anyway".

Comment 32

[Bobby Holley (:bholley)]

Comment on attachment 8632048 [details] [diff] [review]
v4 (r=kchen)

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

::: dom/browser-element/BrowserElementChildPreload.js
@@ +1015,5 @@
> +    let sandbox = new Cu.Sandbox([content], {
> +      sandboxPrototype: content,
> +      sandboxName: "browser-api-execute-script",
> +      allowWaivers: false
> +    });

You might want a sameZoneAs: content here to increase GC locality.

r=me on the creation of the sandbox. Let me know if you want me to look at anything else.

Comment 33

[Paul Rouget [:paul]]

Attachment: v4 (r=kchen,r=bobbyholley)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1f5b9e2ef1e

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/59c2d8026df3

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/59c2d8026df3

Comment 36

[Ayushi Sinha]

Can you tell me that on which firefox version I can use this executeScript() API?
Thanks for your help.

Comment 37

[Paul Rouget [:paul]]

This has been removed a while back. Firefox 42 apparently.