Closed Bug 844406 Opened 8 years ago Closed 8 years ago

Greasemonkey script stops working correctly in Firefox 20. 'self-hosted' builtin functions destroy js-callstack in certain case.

Categories

(Core :: JavaScript Engine, defect)

20 Branch
x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
firefox19 --- unaffected
firefox20 + verified
firefox21 + verified
firefox22 + verified

People

(Reporter: mjh563, Assigned: till)

References

Details

(Keywords: addon-compat, regression)

Attachments

(6 files, 1 obsolete file)

Attached file Testcase script.user.js (obsolete) —
After upgrading to Firefox 20, one of my Greasemonkey scripts stopped working. The attached file is a reduced testcase that demonstrates the problem. This script alerts "Success" in Firefox 19 but "Fail" in Firefox 20.

Steps to Reproduce:

1. Install Greasemonkey from https://addons.mozilla.org/en-US/firefox/addon/greasemonkey/
2. Install the script by clicking on the attached file
3. Load http://www.ebay.co.uk

Actual results:

1. An alert box shows "Fail"
2. In the Error Console: "Greasemonkey access violation: unsafeWindow cannot call GM_getResourceText."

Expected results:

An alert box shows "Success"
Regression range is

Last good nightly: 2012-11-22
First bad nightly: 2012-11-23

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=20ec9014f220&tochange=d8e4f06198dc
Attachment #717457 - Attachment mime type: application/octet-stream → text/javascript
Hmm, Step 2 of the STR doesn't seem to work. I think that's because Greasemonkey doesn't recognise the URL.

Revised Step 2:

Save the attachment as "Testcase script.user.js", then open that file in Firefox. A Greasemonkey installation window should appear when you do that.
Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/fcad43f8558f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20.0 Firefox/20.0 ID:20121122010354
Bad:
http://hg.mozilla.org/mozilla-central/rev/3b71d63eafd5
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20.0 Firefox/20.0 ID:20121122030852
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fcad43f8558f&tochange=3b71d63eafd5


Regression window(m-i)
Good:http://hg.mozilla.org/integration/mozilla-inbound/rev/7ae5ff606cf2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20.0 Firefox/20.0 ID:20121121153752
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/37c36aa6f563
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20.0 Firefox/20.0 ID:20121121165251
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7ae5ff606cf2&tochange=37c36aa6f563

Triggered by:
5593cd83590e	Till Schneidereit — Bug 784294 - Convert some array extras to self-hosted js implementations. r=Waldo The following methods are converted: - lastIndexOf - indexOf - forEach - some - every - reduce - reduceRight
Assignee: nobody → general
Blocks: 784294
Component: Untriaged → JavaScript Engine
Keywords: regression
OS: Linux → All
Product: Firefox → Core
Status: UNCONFIRMED → NEW
Ever confirmed: true
Looking into it now
Assignee: general → tschneidereit
Status: NEW → ASSIGNED
Attached file reduced test-case
So this is pretty weird: invoked from inside Array#forEach, GM_getResourceText seems to lose access to its environment. Or something.

Unfortunately, GreaseMonkey swallows all errors in scripts, so I don't know what goes on, internally.

I suspect that the problem is connected to scripts being executed in sandboxes, but I don't yet know more than that.
Attachment #717457 - Attachment is obsolete: true
The problem is that js callstack is different after landing Bug 784294. Then Greasemonkey throw an error.

e.g., Evaluate the following code in Scratchpad(Chrome privilege):

function a(){
  let stack = Components.stack;
  do {
    Components.classes["@mozilla.org/consoleservice;1"]
        .getService(Components.interfaces.nsIConsoleService)
        .logStringMessage(stack.language +" "+stack.filename);
  } while (stack = stack.caller);
  return true;
}
[1].forEach(function() {
	  a();
});


Before landing Bug 784294,
2 Scratchpad/1
2 Scratchpad/1
2 Scratchpad/1
0 null

After landing Bug 784294
2 Scratchpad/1
2 Scratchpad/1
2 self-hosted
2 Scratchpad/1
0 null
So, I think that this is not Firefox bug, 
Greasemonkey/Scriptish should modify their code,
Jorge: can you reach out to the people at Greasemonkey/Scriptish?
Tracking for now so we can keep this on our radar. If the fix is external, we'll remove tracking once contact has been made.
This is already logged on the Greasemonkey issue tracker; see
https://github.com/greasemonkey/greasemonkey/issues/1715
As this is being tracked on greasemonkey's issue tracker (and there appears to be a fix over there) un-tracking here.
Can anyone more knowledgeable let me know what the implications of this are?

Today, Greasemonkey (sometimes) exposes privileged APIs to user scripts.  When they are called, the call stack is inspected to guarantee that only chrome + user scripts are on the stack, to ensure that content code is not gaining access to these privileged methods.  (It's possible for a malicious/poorly written script to leak references to them to content.)

I don't quite know what self-hosted really means.  Is it actually safe to assume that "self hosted" is not content-controlled, and thus possibly malicious?
I've taken a closer look, and logged details in the linked Greasemonkey issue.

This change seems to make our security checks impossible; "self-hosted" code from the user script sandbox and from content cannot be disambiguated by examining the stack.

Is there perhaps a better way to implement this sort of check (JS caller is trusted to do X only if it comes from the sandbox we create and not from content)?
How do I set this to tracking for 20 again?  This seems to be a big breaking change, at least from Greasemonkey's perspective.  The "workaround" originally reported at Greasemonkey's issue is not appropriate: it just disables our security checks.
(In reply to Anthony Lieuallen from comment #13)

> This change seems to make our security checks impossible; "self-hosted" code
> from the user script sandbox and from content cannot be disambiguated by
> examining the stack.

Could you please attach testcase?
It's a difficult test case to make, so the steps to reproduce are a bit ugly.  But here's what I've got for now:

Install this patched version of Greasemonkey which logs (to the console, via dump()), its activity in apiLeakCheck, and never returns false (which would stop the checks):
https://dl.dropbox.com/u/6784911/greasemonkey-1.8.b844406.xpi
Then install this user script:
https://gist.github.com/arantius/5078045
Then load this page:
https://dl.dropbox.com/u/6784911/b844406.html

In firefox 19, the console displays:

calling GM_getValue ...
>>> apiLeakCheck ...
resource://greasemonkey/util/apiLeakCheck.js
chrome://greasemonkey/content/miscapis.js
resource://greasemonkey/util/hitch.js
file:///.../Firefox/Profiles/w1ajyiyj.test/gm_scripts/API_Leaker/b844406.user.js
<<< apiLeakCheck
exporting GM_getValue
>>> apiLeakCheck ...
resource://greasemonkey/util/apiLeakCheck.js
chrome://greasemonkey/content/miscapis.js
resource://greasemonkey/util/hitch.js
https://dl.dropbox.com/u/6784911/b844406.html
Would have blocked here!
<<< apiLeakCheck

That is: the user script itself calls GM_getValue once, and apiLeakCheck logs the stack that it sees: greasemonkey itself, and the user script inside the profile gm_scripts directory.  Then it leaks the reference to this function to the content scope.  The content scope calls, with a funky syntax, this leaked method.  The apiLeakCheck sees the content scope in the stack: the last "https://" URL, and logs that it would have (patch does not actually) blocked the call, here.

Repeat that with Firefox 20, and the console shows:

calling GM_getValue ...
>>> apiLeakCheck ...
resource://greasemonkey/util/apiLeakCheck.js
chrome://greasemonkey/content/miscapis.js
resource://greasemonkey/util/hitch.js
file:///.../Firefox/Profiles/w1ajyiyj.test/gm_scripts/API_Leaker/b844406.user.js
<<< apiLeakCheck
exporting GM_getValue
>>> apiLeakCheck ...
resource://greasemonkey/util/apiLeakCheck.js
chrome://greasemonkey/content/miscapis.js
resource://greasemonkey/util/hitch.js
self-hosted
Would have blocked here!
self-hosted
Would have blocked here!
<<< apiLeakCheck

The stack when the script itself calls the method is unchanged.  However, when content calls the leaked reference, the content-scoped filename property is gone, now only "self-hosted" is displayed.  If the same weird syntax is used inside a script, as in the content-hosted example, then the stack trace looks exactly the same as the call from content: they cannot be disambiguated.  We cannot selectively allow scripts running in our sandbox to call the function, and block leaked references to content from being used.

(And given the design of Greasemonkey, we cannot make it impossible for references to leak.  To be useful, scripts need a reference to content, to alter it.  And lots of scripts are poorly written, and make leaks like this unintentionally.  Or advanced attackers may be able to abuse even more benign accesses to content.  So we have to do this sort of check.)
(In reply to Anthony Lieuallen from comment #16)

> The stack when the script itself calls the method is unchanged.  However,
> when content calls the leaked reference, the content-scoped filename
> property is gone, now only "self-hosted" is displayed. 

I van confirmed.  Missing js callstak of the content-scoped  filename.

And I request to track for Firefox 20 and later.
Summary: Greasemonkey script stops working correctly in Firefox 20 → Greasemonkey script stops working correctly in Firefox 20. 'self-hosted' builtin functions destroy js-callstack in certain case.
Till - can you enumerate the options here?  We'll be going to build with FF20 beta 3 tomorrow and will only consider low-risk, speculative fixes in the next week (up to beta 4) after that do we have the option of backing out the changes in bug 784294 to not ship this regression in FF20?  Or other potential remedies?
Flags: needinfo?(tschneidereit)
Argh, I'm sorry for dropping the ball on this bug. My bugmail filters clearly don't always work as intended.

The "self-hosted" entry shouldn't be contained in the stack, so this definitely is a regression from self-hosting. I'll bet it's caused by the same issue as bug 787927, so I'll investigate that.

In short, it seems like we're not always setting the "self-hosted" flag on functions when cloning them, which is why this problem only occurs in scenarios that involve sandboxes. 

I'm hopeful that a solution isn't going to be risky: it should consist of correctly copying the state of a flag during a cloning path where that doesn't yet happen.

In case the solution turns out to be more risky, backing out bug 784294 would be an option, yes. It's a completely self-contained change and should be easy to back out.
Flags: needinfo?(tschneidereit)
Given the feedback in comment 23 and the fact that we're approaching beta 4, let's go ahead with backing out bug 784294 for FF20 and continue to track this for a proper fix later.
I'd very much like to instead check in the fix for bug 787927 instead. That is very low-risk and fixes this issue, too. Will formally request uplift once I get a review.
Depends on: 787927
Fixed through fixing bug 787927
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Verified fixed with Firefox 20 beta 5, on Windows XP 32-bit, build ID: 20130313170052. After following the STR from the description and comment 2, I get an alert box that shows "Success".  

Also, bug 787927 is verified too with the beta 5 build.
QA Contact: manuela.muntean
Verified also on Mac OSX 10.7.5 and Ubuntu 12.04 32-bit, with Firefox 20 beta 5.
I've just updated Firefox 20/beta channel (I still don't know how to communicate which build I'm actually using now besides "beta channel on such-and-such date").  What I see in the console when I execute my test case from comment 16 is:

calling GM_getValue ...
>>> apiLeakCheck ...
resource://greasemonkey/util/apiLeakCheck.js
chrome://greasemonkey/content/miscapis.js
resource://greasemonkey/util/hitch.js
file:///C:/Users/t-bone/AppData/Roaming/Mozilla/Firefox/Profiles/w1ajyiyj.test/g
m_scripts/API_Leaker/b844406.user.js
<<< apiLeakCheck
exporting GM_getValue
>>> apiLeakCheck ...
resource://greasemonkey/util/apiLeakCheck.js
chrome://greasemonkey/content/miscapis.js
resource://greasemonkey/util/hitch.js
self-hosted
Would have blocked here!
<<< apiLeakCheck

Or: the call from the sandbox identifies itself as such, so the basic case (detect in-sandbox execution) generally works.  But the content-scoped call still says "self-hosted" in the stack trace.  And only once.  Is that expected?
I can confirm that the problem is still reproducible in Firefox20.0b5.

http://hg.mozilla.org/releases/mozilla-beta/rev/163304f85fc1
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0 ID:20130313170052
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Anthony Lieuallen from comment #29)
> I've just updated Firefox 20/beta channel (I still don't know how to
> communicate which build I'm actually using now besides "beta channel on
> such-and-such date").

Go to about:buildconfig and click the "Built from" link, which should take you to a page on hg.mozilla.org. Somewhere near the top of the page should be a yellow box saying "FIREFOX_20_0b5_RELEASE".
'self-hosted' built-in functions still destroy js-callstack in certain case.

Steps to Reproduce:
Evaluate the following code in Scratchpad(Chrome privilege):

window.a = function a(){
  let stack = Components.stack;
  do {
    Components.classes["@mozilla.org/consoleservice;1"]
        .getService(Components.interfaces.nsIConsoleService)
        .logStringMessage(stack.language +" "+stack.filename);
  } while (stack = stack.caller);
  return true;
};
['["test"].forEach(a);'].forEach(setTimeout);

Expected Results Firefox19.0.2 (before landing Bug 784294):
 2 Scratchpad/1
 2 Scratchpad/1
 0 null

Actual Results Firefox20.0b5(after landing Bug 784294 and Bug 787927):
 2 Scratchpad/1
 2 self-hosted
 0 null
@Till,
I think that it is time to back bug784294 out because Firefox20 will be released soon.
> Expected Results Firefox19.0.2
>  2 Scratchpad/1
>  2 Scratchpad/1
>  0 null
> 
> Actual Results Firefox20.0b5
>  2 Scratchpad/1
>  2 self-hosted
>  0 null

I was also able to reproduce these errors, on an Ubuntu 32-bit machine, with the builds specified above.
[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 784294, bug 787927
User impact if declined: Some addons (especially GreaseMonkey) fail, plus some top-crashers caused by the attempted fix in bug 787927 (see bug 851788)
Testing completed (on m-c, etc.): this is a beta-specific backout
Risk to taking this patch (and alternatives if risky): none, straight reversal to previous implementation of the Array extras
String or UUID changes made by this patch: none

I will work on fixing the regression for Aurora, but I agree that not backing out 784294 is too risky at this point.
Attachment #725891 - Flags: approval-mozilla-beta?
Comment on attachment 725891 [details] [diff] [review]
Backout of self-hosted array extras from beta

Thanks for preparing this backout, please land asap (before tomorrow morning) so that we can make sure to collect 20.0b5 feedback (and crash volume) to confirm this works as intended.
Attachment #725891 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Firefox 20 beta 6 - build ID: 20130320062118 - Ubuntu 12.04 32-bit 

1) After following the STR from comment 32, I don't get those results anymore; all I get is:

Timestamp: 03/21/2013 10:54:44 AM
Error: The Components object is deprecated. It will soon be removed.
Source File: Scratchpad/1
Line: 11


Timestamp: 03/21/2013 10:54:44 AM
Error: Error: Permission denied to access property 'stack'
Source File: Scratchpad/1
Line: 11

2) After following the STR from the description and comment 2, I get an alert box that shows "Success".
Firefox 20 beta 6 - build ID: 20130320062118 - Mac OSX 10.7.5

1) After following the STR from comment 32, I get the expected results:

 2 Scratchpad/1
 2 Scratchpad/1
 0 null

2) After following the STR from the description and comment 2, I get an alert box that shows "Success".


Firefox 20 beta 6 - build ID: 20130320062118 - Windows XP

1) After following the STR from comment 32, all I get is:

Error: Error: Permission denied to access property 'stack'
Source File: Scratchpad/1
Line: 11

2) After following the STR from the description and comment 2, I get an alert box that shows "Success".

Verified fixed based on comment 38 and comment 39.
Depends on: 853417
In Firefox Nightly 22.0a1 (2013-03-21) I am seeing no change since comment 29.  Please use those STR; the original report's STR do not exercise the true bug.

The stack trace still shows an entry whose filename is "self-hosted", in the called-from-content case.  This should not be so, right?

(p.s. Where do I get beta 6 from? my beta channel install says it's b5 and up to date.)
(In reply to Anthony Lieuallen from comment #40)

> (p.s. Where do I get beta 6 from? my beta channel install says it's b5 and
> up to date.)

ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/20.0b6-candidates/build1/
(In reply to Anthony Lieuallen from comment #40)
> In Firefox Nightly 22.0a1 (2013-03-21) I am seeing no change since comment
> 29.  Please use those STR; the original report's STR do not exercise the
> true bug.
> 
> The stack trace still shows an entry whose filename is "self-hosted", in the
> called-from-content case.  This should not be so, right?

With Firefox 20 beta 6, on Ubuntu 12.04 32-bit, after using the STR from comment 32, I don't get an entry whose filename is "self-hosted", in the Error Console (Tools -> Web Developer -> Error Console).

Do you still reproduce the issue with Firefox 20 beta 6? Which OS are you using?
> With Firefox 20 beta 6, on Ubuntu 12.04 32-bit, after using the STR from
> comment 32, I don't get an entry whose filename is "self-hosted", in the
> Error Console (Tools -> Web Developer -> Error Console).

Sorry, I meant comment 29, not 32.
The problem isn't really a self-hosted function bein on the stack. Instead, setTimeout stores the calling function's filename and line / column number. I have a patch fixing this in bug 853417, which I hope to uplift to Aurora once it's pushed to central.

Here's a much simpler set of STR:

1. run this code in the console or scratchpad (or a website):

["console.log((new Error).fileName)"].forEach(setTimeout);
Status: REOPENED → ASSIGNED
(In reply to Till Schneidereit [:till] from comment #44)
> The problem isn't really a self-hosted function bein on the stack. Instead,
> setTimeout stores the calling function's filename and line / column number.
> I have a patch fixing this in bug 853417, which I hope to uplift to Aurora
> once it's pushed to central.
> 
> Here's a much simpler set of STR:
> 
> 1. run this code in the console or scratchpad (or a website):
> 
> ["console.log((new Error).fileName)"].forEach(setTimeout);

With Firefox 20 beta 6 on Ubuntu 12.04 32-bit, the Error Console doesn't show any errors, and the Web Console shows this:

[17:06:15.199]  Scratchpad/1    Scratchpad/1:10
With the latest Nightly (build ID: 20130321030940), on the same machine (Ubuntu), the Web Console shows: 

[17:14:05.702] self-hosted self-hosted:348

Till, do you get the same results?
> Till, do you get the same results?

Yes, I do. The bug was fixed on beta by backing out all self-hosted code. For Aurora and mozilla-central, it will be fixed by pushing the patch in bug 853417, so those are currently still affected.
Thanks, sorry for confusion.  With 20b6 I indeed see the content URL as expected, in my steps from 16.  Confirmed fixed.  I was using 20b5 earlier.

I'll go CC myself on bug 853417 so I can watch that fix to post-20 Firefoxen.
Underlying problem is fixed, so this should reproduce in the next Nightly, anymore. Requesting uplift to Aurora now.
(In reply to Till Schneidereit [:till] from comment #49)
> Underlying problem is fixed, so this should reproduce in the next Nightly,
> anymore. Requesting uplift to Aurora now.

... and I forgot actually doing that after some details held up the central. Requested beta uplift now.
(In reply to Till Schneidereit [:till] from comment #50)
> (In reply to Till Schneidereit [:till] from comment #49)
> > Underlying problem is fixed, so this should reproduce in the next Nightly,
> > anymore. Requesting uplift to Aurora now.
> 
> ... and I forgot actually doing that after some details held up the central.
> Requested beta uplift now.

853417, has been approved on beta now in favour of this and help fix top-crasher(Bug 851788).

Requesting QA verification once Fx 21 Beta 2 builds are available .
Verified fixed with Firefox 21 beta 2 (build ID: 20130408165307), on Mac OSX 10.7.5 and Ubuntu 12.04 32-bit, using the STR from comment 44.
Keywords: qawanted
This is fixed on trunk, too. Do I need to take additional steps for that to be verified, or can I close the bug now?
Flags: needinfo?(bbajaj)
(In reply to Till Schneidereit [:till] from comment #53)
> This is fixed on trunk, too. Do I need to take additional steps for that to
> be verified, or can I close the bug now?

Based on QA verification in comment# 52, you can go ahead and close the bug.
Flags: needinfo?(bbajaj)
Thanks for the info!
Status: ASSIGNED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Verified fixed with Firefox 22 beta 2 (build ID: 20130514181517), on Mac OSX 10.8.3, Ubuntu 12.10 32-bit and Windows Vista 32-bit, using the STR from comment 44.
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
Duplicate of this bug: 844345
You need to log in before you can comment on or make changes to this bug.