1042798 - document.write throws a security exception when called through NPAPI

Status: RESOLVED FIXED

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

Description

[jmullenix.estream]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase with that script

This seems to work just fine for me in Firefox 31.

> Run this snippet:

How exactly are you running it?

Comment 2

[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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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?

Comment 4

[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!

Comment 5

[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

Comment 6

[Boris Zbarsky [:bzbarsky]]

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/[e-mail]-cbb266205ea7/try-win32/ is the one without that changeset and the one in https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-81168f57c51f/try-win32/ is with that changeset.

Comment 7

[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

Comment 8

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 9

[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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

[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...

Comment 11

[Bob Owen (:bobowen) - PTO until 3rd April]

(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).

Comment 12

[Boris Zbarsky [:bzbarsky]]

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...

Comment 13

[Bobby Holley (:bholley)]

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.

Comment 14

[Bobby Holley (:bholley)]

Bob, mind writing up the patch?

Comment 15

[Bob Owen (:bobowen) - PTO until 3rd April]

(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.

Comment 16

[Bob Owen (:bobowen) - PTO until 3rd April]

https://hg.mozilla.org/try/rev/6949c51b4063

Comment 17

[Bob Owen (:bobowen) - PTO until 3rd April]

Oops ... sorry:
https://tbpl.mozilla.org/?tree=Try&rev=c512b5566b09

Comment 18

[Bob Owen (:bobowen) - PTO until 3rd April]

Attachment: Use an AutoEntryScript in nsNPAPIPlugin _evaluate as we are about to run script and need to ensure the correct JSContext* gets pushed v1

Just a straight switch for the AutoEntryScript, which will enter the right compartment for us.

Comment 19

[Boris Zbarsky [:bzbarsky]]

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?

Comment 20

[Bob Owen (:bobowen) - PTO until 3rd April]

Attachment: Test: ensure window.open followed by document.writeln doesn't throw security error when called through NPN evaluate v1

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.

Comment 21

[Boris Zbarsky [: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.

Comment 22

[Bob Owen (:bobowen) - PTO until 3rd April]

Thanks Bobby, Boris.

Try push with test:
https://tbpl.mozilla.org/?tree=Try&rev=ce044fe8d172

Comment 23

[Bob Owen (:bobowen) - PTO until 3rd April]

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

Comment 24

[Bob Owen (:bobowen) - PTO until 3rd April]

And this time with some actual tests running, sorry:
https://tbpl.mozilla.org/?tree=Try&rev=93152e0314a6

Comment 25

[Bob Owen (:bobowen) - PTO until 3rd April]

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

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0debc766d237
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e9f64aa7434

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0debc766d237
https://hg.mozilla.org/mozilla-central/rev/1e9f64aa7434

Comment 28

[Bob Owen (:bobowen) - PTO until 3rd April]

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.

Comment 29

[jmullenix.estream]

Looks good! Running the Windows Standard version of Nightly, the code in question no longer throws a security exception.

Thanks!!

Comment 30

[Bob Owen (:bobowen) - PTO until 3rd April]

(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 31

[Bob Owen (:bobowen) - PTO until 3rd April]

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

Comment 32

[Bob Owen (:bobowen) - PTO until 3rd April]

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.

Comment 33

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 34

[Lawrence Mandel [:lmandel] (use needinfo)]

(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.

Comment 35

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/34fe3ed2fb64
https://hg.mozilla.org/releases/mozilla-aurora/rev/0b6ed5ee05cc

https://hg.mozilla.org/releases/mozilla-beta/rev/74a868d4fccd
https://hg.mozilla.org/releases/mozilla-beta/rev/55731dcb4b17

Comment 37

[Lukas Blakk [:lsblakk] use ?needinfo]

This doesn't meet ESR criteria.

Comment 38

[Mike Kaply [:mkaply]]

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).