Closed Bug 1195735 (CVE-2015-7187) Opened 4 years ago Closed 4 years ago

To disable JS, set { script: false } when creating the panel, but inline JS is still executing

Categories

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

defect
Not set

Tracking

(firefox41 wontfix, firefox42 fixed, firefox43 fixed, firefox44 fixed)

RESOLVED FIXED
mozilla44
Tracking Status
firefox41 --- wontfix
firefox42 --- fixed
firefox43 --- fixed
firefox44 --- fixed

People

(Reporter: jason, Assigned: Gijs)

References

Details

(Keywords: sec-moderate, Whiteboard: [post-critsmash-triage][adv-main42+])

Attachments

(1 file)

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 }
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.
Summary: 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 → To disable JS, set { script: false } when creating the panel, but inline JS is still executing
Matteo, can you investigate/clarify what's up here?
Component: Untriaged → General
Flags: needinfo?(zer0)
Product: Firefox → Add-on SDK
Version: 40 Branch → unspecified
How often is this add-on SDK feature used in addons? How many are now insecure?
Flags: sec-bounty?
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)
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!
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?
Flags: needinfo?(kmaglione+bmo)
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.
Flags: needinfo?(kmaglione+bmo)
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.
Group: core-security → toolkit-core-security
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.
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.
Attached patch ,Splinter Review
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.
Attachment #8668491 - Flags: review?(zer0)
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?
Attachment #8668491 - Flags: review?(zer0) → review+
(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.
Flags: needinfo?(zer0)
(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.
(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.
Dave, where does this need to land? hg or still also github?
Flags: needinfo?(dtownsend)
(In reply to :Gijs Kruitbosch from comment #18)
> Dave, where does this need to land? hg or still also github?

Hg
Flags: needinfo?(dtownsend)
Assignee: nobody → gijskruitbosch+bugs
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
https://hg.mozilla.org/mozilla-central/rev/dcf488b73918
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
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
Attachment #8668491 - Flags: approval-mozilla-beta?
Attachment #8668491 - Flags: approval-mozilla-aurora?
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
Attachment #8668491 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
Flags: needinfo?(sledru)
Comment on attachment 8668491 [details] [diff] [review]
,

OK!
Flags: needinfo?(sledru)
Attachment #8668491 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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".
Group: toolkit-core-security → core-security-release
Thank you everyone who worked to fix this bug!
Depends on: 1211470
Flags: sec-bounty? → sec-bounty+
Hey guys - if there is going to be a bounty, can we give credit to these names?

Jason Hamilton
Peter Arremann
Sylvain Giroux

Thanks!
Thank you Jason!
(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.
Yes, thank you!
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main42+]
Alias: CVE-2015-7187
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.