Closed Bug 1042798 Opened 5 years ago Closed 5 years ago

document.write throws a security exception when called through NPAPI

Categories

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

31 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
firefox31 --- ?
firefox32 + fixed
firefox33 + fixed
firefox34 + fixed
firefox-esr31 --- wontfix

People

(Reporter: jmullenix.estream, Assigned: bobowen)

References

Details

(Keywords: dev-doc-needed, regression, site-compat)

Attachments

(3 files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140716183446

Steps to reproduce:

Run this snippet:

_docWin=window.open('about:blank','newWindow','width='+w+',height='+h+',menubar=0'+',toolbar=0'+',status=0'+',scrollbars=1'+',resizable=1')

_docWin.moveTo(Math.max(0, window.screenX - _docWin.outerWidth), window.screenY);

var winContent = window.document;
var winContent_Head = winContent.head.innerHTML;
var winContent_Body = winContent.body.innerHTML;		

var winContent_Head = winContent_Head.replace(/A.swf/g,"B.swf");	
var winContent_Body = winContent_Body.replace(/A.swf/g,"B.swf");		

_docWin.document.writeln(winContent_Head);



Actual results:

This now (as of FF 31) throws the following exception, and aborts:

code: 18
message: "The operation is insecure."
name: "SecurityError"
result: 2152923154



Expected results:

This did not throw in previous versions.
This seems to work just fine for me in Firefox 31.

> Run this snippet:

How exactly are you running it?
Flags: needinfo?(jmullenix.estream)
Thanks for the help!

This snippet is part of a function called by a .SWF via ActionScript 3's ExternalInterface.call. The main .SWF is loaded by an HTML file, and this bit of JavaScript lives in a separate .JS file on the same machine. The structure looks like this:

..\main.html
..\assets\js\problem_file.js

It's worked well for a few years.
Flags: needinfo?(jmullenix.estream)
Is this running over file:// or http://?

If the former, are you able to provide the scripts to someone else to run from file:// ?

If the latter, can you provide a link to the page?  If you can't link to the page, are you willing to use http://mozilla.github.io/mozregression/ to narrow down when the problem first appears for you?
Flags: needinfo?(jmullenix.estream)
It happens over both, file:// and http://.

I can't provide scripts at this time, but I will go through the mozregression process today.

Thanks again!
Flags: needinfo?(jmullenix.estream)
I've run through mozregression, and the result is below. It appears this issue was introduced ~March 21st of this year, although I could swear I hit a working nightly from March 31st, as well.

The tool wasn't able to entirely complete its work. I tried a few times, and it always ran into server erros 500, 503 or 504, and exited with an error message like 'Back-end server is at capacity'.

Please let me know whether I need to complete the process. I'd be happy to try another time.


MOZREGRESSION OUTPUT:

Ensuring we have enough metadata to get a pushlog...
Last good revision: e84a391b604b (2014-03-20)
First bad revision: c148f0b0c8b4 (2014-03-21)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e84a391b604b&tochange=c148f0b0c8b4

... attempting to bisect inbound builds (starting from previous day, to make sure no inbound revision is missed)
Getting http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/03/2014-03-19-03-02-01-mozilla-central/firefox-31.0a1.en-US.win32.txt
Getting inbound builds between 3bc3b9e2cd99 and c148f0b0c8b4
Thanks!  In that range, https://hg.mozilla.org/mozilla-central/rev/496ae3bc07aa looks like the most likely culprit.

Are you willing to try two builds for me?  The one in https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bzbarsky@mozilla.com-cbb266205ea7/try-win32/ is the one without that changeset and the one in https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bzbarsky@mozilla.com-81168f57c51f/try-win32/ is with that changeset.
Flags: needinfo?(jmullenix.estream)
I'm seeing the security exception in both of these builds. I'm not certain I'm running them correctly, however. In their about menus, I see version 31.0. Is that correct, or am I accidentally running my normal Firefox, somehow?

This is what I did for each:
- download and extract 'firefox-31.0a1.en-US.win32.zip'
- from the extracted folder, run firefox.exe
Flags: needinfo?(jmullenix.estream)
> In their about menus, I see version 31.0.

It should say 31.0a1...

> - from the extracted folder, run firefox.exe

Did you quit Firefox first?
Flags: needinfo?(jmullenix.estream)
I did have Firefox open. That was the problem, thanks!

Yes, the exception is thrown in the second Nightly build, but not the first.
Flags: needinfo?(jmullenix.estream)
[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

Alright, great.  Bob, Bobby, could you take a look, please?  Looks like we're not ending up with the right JSContext in nsNPAPIPlugin.cpp anymore, so when document.open (called by document.writeln) does its GetDocumentFromContext bit it doesn't get one...
Blocks: 978042
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(bobowencode)
Flags: needinfo?(bobbyholley)
Summary: document.write throws a security exception when called through a new window created with window.open. → document.write throws a security exception when called through NPAPI
(In reply to Boris Zbarsky [:bz] from comment #10)

> Alright, great.  Bob, Bobby, could you take a look, please?  Looks like
> we're not ending up with the right JSContext in nsNPAPIPlugin.cpp anymore,
> so when document.open (called by document.writeln) does its
> GetDocumentFromContext bit it doesn't get one...

Ah, yes, this was one of the early changes, we decided on AutoSafeJSContext instead of an AutoEntryScript.

Unfortunately, we can't just revert that change because I got rid of nsIScriptContext::Evaluate string in changeset d2730edb36c4.
We would have to revert that one as well and that changed interface ID (not sure if that would cause a problem).

Anyway we probably just need to decide if we need an AutoJSAPI initialised with legacy error reporting or an AutoEntryScript.

Bobby - which do you think it should be?  We didn't have the AutoJSAPI around when I did this one.
Or maybe we're just entering the wrong compartment or something?

I should be able to pick this up pretty quickly on Monday, if I don't get chance before (at a wedding today and Birthday meal for someone tomorrow).
Flags: needinfo?(bobowencode)
This isn't about compartments.  Right now document.open looks at the JSContext* passed in and tries to get to an nsIScriptContext from it.  So you need to have the right JSContext.  See bug 1041602 comment 3 and following...
Yeah, looks like we need an AutoEntryScript after all - bug 978042 comment 28 was totally wrong even at the time, and even more wrong in the light of the semantics that we (later) invented in bug 989528. We're planning to execute script, so we need an AutoEntryScript.
Flags: needinfo?(bobbyholley)
Bob, mind writing up the patch?
Flags: needinfo?(bobowencode)
(In reply to Bobby Holley (:bholley) from comment #14)
> Bob, mind writing up the patch?

No problem at all, I get onto it right now.
Flags: needinfo?(bobowencode)
Just a straight switch for the AutoEntryScript, which will enter the right compartment for us.
Attachment #8463291 - Flags: review?(bobbyholley)
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
Comment on attachment 8463291 [details] [diff] [review]
Use an AutoEntryScript in nsNPAPIPlugin _evaluate as we are about to run script and need to ensure the correct JSContext* gets pushed v1

Can we please add a test to ensure this doesn't break again?
Attachment #8463291 - Flags: review?(bobbyholley) → review+
This fails before the fix and passes after, and seems fairly minimal.

I did try to compile back to the revision before the regression, but had trouble running the test.
I think that mach and the test harness have changed quite a bit since then.
Attachment #8463557 - Flags: review?(bzbarsky)
Comment on attachment 8463557 [details] [diff] [review]
Test: ensure window.open followed by document.writeln doesn't throw security error when called through NPN evaluate v1

Looks lovely.
Attachment #8463557 - Flags: review?(bzbarsky) → review+
Thanks Bobby, Boris.

Try push with test:
https://tbpl.mozilla.org/?tree=Try&rev=ce044fe8d172
Just thought I ought to try the test on more platforms as it uses window.open.
These tests are skipped for some things, but just to be safe:
https://tbpl.mozilla.org/?tree=Try&rev=10bde00304bc
And this time with some actual tests running, sorry:
https://tbpl.mozilla.org/?tree=Try&rev=93152e0314a6
Try push from comment 24 looks good.

Please land in the following order (fix then test), thanks:
https://bugzilla.mozilla.org/attachment.cgi?id=8463291
https://bugzilla.mozilla.org/attachment.cgi?id=8463557
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0debc766d237
https://hg.mozilla.org/mozilla-central/rev/1e9f64aa7434
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Hopefully, we  have a fix for this problem in the latest Nightly build:
https://nightly.mozilla.org/

Would you mind trying it out?
Once we're sure it's working, I'll request that we get this uplifted.
Flags: needinfo?(jmullenix.estream)
Looks good! Running the Windows Standard version of Nightly, the code in question no longer throws a security exception.

Thanks!!
Flags: needinfo?(jmullenix.estream)
(In reply to jmullenix.estream from comment #29)
> Looks good! Running the Windows Standard version of Nightly, the code in
> question no longer throws a security exception.

Excellent, thanks for testing this.

> Thanks!!

No problem, I'll request this is uplifted to Beta.
Comment on attachment 8463291 [details] [diff] [review]
Use an AutoEntryScript in nsNPAPIPlugin _evaluate as we are about to run script and need to ensure the correct JSContext* gets pushed v1

Approval Request Comment
[Feature/regressing bug #]: Introduced by patch Part 6 in Bug 978042. Where change meant a different JSContext was being pushed.

[User impact if declined]: Any similar script called via plugins will cause exceptions.

[Describe test coverage new/current, TBPL]: New test added to dom/plugins/test/mochitest/test_npruntime_npnevaluate.htm
Also manually tested be original reporter.

[Risks and why]: Medium - this is a simple fix which should duplicate the original context pushing. However, given where the change is, I think medium is appropriate.

[String/UUID change made/needed]: None
Attachment #8463291 - Flags: approval-mozilla-beta?
Attachment #8463291 - Flags: approval-mozilla-aurora?
Comment on attachment 8463557 [details] [diff] [review]
Test: ensure window.open followed by document.writeln doesn't throw security error when called through NPN evaluate v1

Test mentioned in approval request in comment 31.
Attachment #8463557 - Flags: approval-mozilla-beta?
Attachment #8463557 - Flags: approval-mozilla-aurora?
Given that medium risk, can you verify that the fix is good on m-c before we decide on whether to uplift? If you can verify this week, we can get this into beta4 on Monday.
Flags: needinfo?(bobowencode)
(In reply to Lawrence Mandel [:lmandel] from comment #33)
> Given that medium risk, can you verify that the fix is good on m-c before we
> decide on whether to uplift? If you can verify this week, we can get this
> into beta4 on Monday.

Oop. I see that the reporter did test in comment 29 and all is well.
Flags: needinfo?(bobowencode)
Comment on attachment 8463291 [details] [diff] [review]
Use an AutoEntryScript in nsNPAPIPlugin _evaluate as we are about to run script and need to ensure the correct JSContext* gets pushed v1

Given that the reporter verified that the fix is good in comment 29, I'm going to approve for Aurora and Beta in order to get this into beta3 and give a few more days to catch any fallout.
Attachment #8463291 - Flags: approval-mozilla-beta?
Attachment #8463291 - Flags: approval-mozilla-beta+
Attachment #8463291 - Flags: approval-mozilla-aurora?
Attachment #8463291 - Flags: approval-mozilla-aurora+
Attachment #8463557 - Flags: approval-mozilla-beta?
Attachment #8463557 - Flags: approval-mozilla-beta+
Attachment #8463557 - Flags: approval-mozilla-aurora?
Attachment #8463557 - Flags: approval-mozilla-aurora+
Flags: in-testsuite+
This doesn't meet ESR criteria.
I'd like to request ESR approval again. I have an enterprise client that is hitting it and it's a tiny fix.

And it's a regression (which does meat ESR criteria).
You need to log in before you can comment on or make changes to this bug.