1195735 - (CVE-2015-7187) To disable JS, set { script: false } when creating the panel, but inline JS is still executing

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[jason]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2403.155 Safari/537.36

Steps to reproduce:

1. Create a browser extension for Firefox.

2. Create a panel with script: false:

function createPanel () {
        var sd = require("sdk/self").data;
        myPanel = require("sdk/panel").Panel({ 
                        width: 640, 
                        height: 522, 
                        allow: { script: false },
                        contentScriptFile: [ sd.url("full.js"), sd.url("popupscript.js") ]
    

3. Pull an external html page into the panel and include: <script>alert('this shouldnt happen');</script>

Issue was found on firefox 40, but I believe it exists prior.


Actual results:

Alert box with "this shouldnt happen" appears.


Expected results:

Inline script should have been ignored due to this flag:

allow: { script: false }

Comment 1

[jason]

This URL https://developer.mozilla.org/en-US/Add-ons/SDK/High-Level_APIs/panel#allow says that to disable/remove JS, set 'allow for scripts' to false. I'm using allow: { script: false } when creating the panel but that doesn't seem to do anything.

I marked this as a security issue because people might think they're protected from remote execution of JS when it is currently allowed.

Comment 2

[:Gijs (he/him)]

Matteo, can you investigate/clarify what's up here?

Comment 3

[Daniel Veditz [:dveditz]]

How often is this add-on SDK feature used in addons? How many are now insecure?

Comment 4

[jason]

I'm trying to get my extension fully reviewed. My extension was not accepted for fully reviewed because I'm pulling remote JS. The reviewers asked me to define all JS locally, and not pull any remotely.

So I modified my addon to include all javascript with the xpi, however since remote JS can still be included, the change I made is basically for show since; I could technically still include additional JS and call it.

I don't have a personal preference either way, but it's obviously a security concern, and it sounds like extension reviewers are asking addons to disable dynamic scripts - which currently is not possible due to this flag not working.

For reference: I spoke with Andreas Wagner / TheOne and Spoji on IRC (#amo-editors)

Comment 5

[Kris Maglione [:kmag]]

I can only find three instances on AMO, but they're all for `Page`, not `Panel`:

https://mxr.mozilla.org/addons/source/526422/resources/dinbendon-reminder/lib/dinbendon-parser.js#110
https://mxr.mozilla.org/addons/source/567704/resources/heuristic_network_prefetcher/lib/main.js#34
https://mxr.mozilla.org/addons/source/502374/resources/ltt-notifier/lib/main.js#858

Comment 6

[Kris Maglione [:kmag]]

Ah. Local grep turned up a bunch more:

https://mxr.mozilla.org/addons/source/640248/resources/jc-multitool-panel-v4/lib/genPassword.js#304
https://mxr.mozilla.org/addons/source/640248/resources/jc-multitool-panel-v4/data/trashcan/junk.txt#925 (in a .txt file that's actually a script... ugh)
https://mxr.mozilla.org/addons/source/630630/resources/viablitzqrcode/lib/main.js#29
https://mxr.mozilla.org/addons/source/464696/resources/drweb-scan-link/lib/main.js#15
https://mxr.mozilla.org/addons/source/400540/resources/conversor-web/lib/main.js#24
https://mxr.mozilla.org/addons/source/394670/resources/ip-address-and-domain-information/lib/main.js#14

Those are all Page or Widget, though. Still no Panel.

Comment 7

[Sylvain Giroux [:Spoji]]

I'm the AMO reviewer for Jason's add-on. While the panel Jason opens doesn't include /run any script, we, as reviewers, cannot confirm that it will stay this way over time. Same thing for other add-on that need to get fully reviewed before getting pushed to public on AMO.

Can someone confirm us that this is a bug? It there an alternative / workaround?

For sure the safest way would be to integrate all panel's content locally inside the XPI; but that's something that would require some weeks of development for the developer. We are trying to find a way to save time for the developer while keeping the environment safe for the end users.

Thanks!

Comment 8

[Daniel Veditz [:dveditz]]

Kris: even though the cases you found (which match the ones I found) are Widget rather than panel, if disabling scripts in panels fails are we sure it works in Widgets?

Comment 9

[Kris Maglione [:kmag]]

No, it if doesn't work in panels, I suspect it doesn't work anywhere else either. But I know that panels have some special hacks that the other two modules don't, so it's possible that the problem is specific to the former.

I'll try to find time to test later today.

Comment 10

[Kris Maglione [:kmag]]

It's probably worth noting that the widget module has been removed from Firefox. I'd be much more worried about this being used in page workers, anyway, since I suspect they're more likely to use them in security-sensitive ways.

Comment 11

[:Gijs (he/him)]

Purely from code inspection it looks like this won't affect page workers:

https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/frame/utils.js#84

and line 104.

Whereas for the panel code, I don't see anything that does anything with the allow object. It just gets ignored, AFAICT.

Comment 12

[:Gijs (he/him)]

With a trivial test:

var pageWorker = require("sdk/page-worker").Page({
  contentURL: "data:text/html,<body> <script>console.log('this is a test')</script>",
  allow: {script: false},
});



shows no logs, and twiddling script: true does show that log. Alerts don't work (presumably because the page is invisible).

Looking at how to fix the Panel code now.

Comment 13

[:Gijs (he/him)]

Attachment: ,

If I'd known it was this difficult I would have just suggested to remove the docs for this. Anyway, it looks like this is what it takes.

Comment 14

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8668491 [details] [diff] [review]
,

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

Looks good to me, I would address just the attribute there, and we should probably add a test case in `test-panel` to ensure that the javascript is disabled; to prevent regression.

::: addon-sdk/source/lib/sdk/panel/utils.js
@@ +287,5 @@
>  
> +    try {
> +      swapFrameLoaders(backgroundFrame, viewFrame);
> +      // We need to re-set this because... swapFrameLoaders. Or something.
> +      let shouldEnableScript = panel.getAttribute("sdkscriptenabled") == "true";

Is there a reason why you couldn't use `options.allowJavascript` here, instead of setting and getting the value from the node?

Comment 15

[:Gijs (he/him)]

(In reply to Matteo Ferretti [:matteo][:zer0] from comment #14)
> Comment on attachment 8668491 [details] [diff] [review]
> ,
> 
> Review of attachment 8668491 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me, I would address just the attribute there, and we should
> probably add a test case in `test-panel` to ensure that the javascript is
> disabled; to prevent regression.
> 
> ::: addon-sdk/source/lib/sdk/panel/utils.js
> @@ +287,5 @@
> >  
> > +    try {
> > +      swapFrameLoaders(backgroundFrame, viewFrame);
> > +      // We need to re-set this because... swapFrameLoaders. Or something.
> > +      let shouldEnableScript = panel.getAttribute("sdkscriptenabled") == "true";
> 
> Is there a reason why you couldn't use `options.allowJavascript` here,
> instead of setting and getting the value from the node?

Because you can change the option for the page worker, so I implemented that, but the options object in here is going to be the old one that was passed in at creation time, and therefore it might be out of date if the consumer changed the value in the meantime.

FWIW, the swapFrameLoaders thing is a bug anyway, but there we go.

Comment 16

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to :Gijs Kruitbosch from comment #15)
> > Is there a reason why you couldn't use `options.allowJavascript` here,
> > instead of setting and getting the value from the node?
> 
> Because you can change the option for the page worker, so I implemented
> that, but the options object in here is going to be the old one that was
> passed in at creation time, and therefore it might be out of date if the
> consumer changed the value in the meantime.

I think in that case we should be probably refer to the model, that will be up to date, but I don't want to make this patch wait longer, so it's OK to me keep as is. 

> FWIW, the swapFrameLoaders thing is a bug anyway, but there we go.

Yeah. About the tests, as mentioned on IRC we're on the same page, so yes, the unit test is definitely a follow up bug, I don't want to block landing this patch just for a regression checking.

Comment 17

[:Gijs (he/him)]

(In reply to Matteo Ferretti [:matteo][:zer0] from comment #16)
> (In reply to :Gijs Kruitbosch from comment #15)
> > > Is there a reason why you couldn't use `options.allowJavascript` here,
> > > instead of setting and getting the value from the node?
> > 
> > Because you can change the option for the page worker, so I implemented
> > that, but the options object in here is going to be the old one that was
> > passed in at creation time, and therefore it might be out of date if the
> > consumer changed the value in the meantime.
> 
> I think in that case we should be probably refer to the model, that will be
> up to date, but I don't want to make this patch wait longer, so it's OK to
> me keep as is. 

Right, I looked at doing that, but this code is in panel/utils.js and that doesn't have the model info for the panel - modelFor and the model collection only exists in the panel.js file, so we have to pass it in, but IIRC it didn't exist yet at call time... it's all very messy.

Comment 18

[:Gijs (he/him)]

Dave, where does this need to land? hg or still also github?

Comment 19

[Dave Townsend [:mossop]]

(In reply to :Gijs Kruitbosch from comment #18)
> Dave, where does this need to land? hg or still also github?

Hg

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dcf488b73918

Comment 21

[:Gijs (he/him)]

Comment on attachment 8668491 [details] [diff] [review]
,

Approval Request Comment
[Feature/regressing bug #]: sec-moderate security issue affecting add-ons
[User impact if declined]: idem
[Describe test coverage new/current, TreeHerder]: there is existing test coverage for SDK panels. I intend to write a minimal testcase for this issue in a followup bug
[Risks and why]: reasonably low, because of the existing tests, and us still having considerable beta runway, and it being restricted to add-ons
[String/UUID change made/needed]: no

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8668491 [details] [diff] [review]
,

Taking it in aurora, Giks, do you see an issue if this rides the train from aurora? Thanks

Comment 23

[:Gijs (he/him)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #22)
> Comment on attachment 8668491 [details] [diff] [review]
> ,
> 
> Taking it in aurora, Giks, do you see an issue if this rides the train from
> aurora? Thanks

This was reported by add-on devs & reviewers who are/were (hoping to) rely[ing] on this working. I think this is low enough risk and that security guarantees we give API consumers are important enough that it makes sense to take this on beta instead of letting it ride with 43.

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8668491 [details] [diff] [review]
,

OK!

Comment 25

[Sylvain Giroux [:Spoji]]

(In reply to :Gijs Kruitbosch from comment #23)
> This was reported by add-on devs & reviewers who are/were (hoping to)
> rely[ing] on this working. I think this is low enough risk and that security
> guarantees we give API consumers are important enough that it makes sense to
> take this on beta instead of letting it ride with 43.

As an AMO reviewer, this is definitively something that I rely on to make sure the add-on is safe for the users before approving it to public. For this particular problem, I had to force the add-on developer to change his code because of this. Without this fix, this is something that will most likely come back in the future. 

I was lucky enough to have a good add-on developer that agreed to do the changes and just not replying with a message like "Well it's a Firefox issue, not my problem".

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/744b9efcd17d

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e93e54b574c4

Comment 28

[Sylvain Giroux [:Spoji]]

Thank you everyone who worked to fix this bug!

Comment 29

[jason]

Hey guys - if there is going to be a bounty, can we give credit to these names?

Jason Hamilton
Peter Arremann
Sylvain Giroux

Thanks!

Comment 30

[Sylvain Giroux [:Spoji]]

Thank you Jason!

Comment 31

[Al Billings [:abillings - ex-MoCo]]

(In reply to jason from comment #29)
> Hey guys - if there is going to be a bounty, can we give credit to these
> names?

I assume you mean an advisory? If so, yes, we can do that.

Comment 32

[jason]

Yes, thank you!