611910 - Only pass URL schemes to NPRuntime Java plugins that Java can understand

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

Java Plugin2 uses the current document's URL or documentURI properties
to configure the privileges of Java code that's called from JavaScript
(JavaScript-to-Java LiveConnect).

As the testcase from bug 589041 shows, if document.URL or
document.documentURI is a malformed URL (or one that Java can't
understand), the Java Plugin2 client process will crash.

This happens in the following method in Plugin2Manager.java (part of
Sun/Oracle's source distro for Java 6 SE):

public URL getDocumentBase()

This method calls MessagePassingExecutionContext.getDocumentBase(),
which in turn uses JSObject.getMember() to get the current browser
page's document.URL or document.documentURI properties.  It feeds the
result to the URL(String) constructor, and throws a RuntimeException
if the constructor throws a MalformedURLException.

A RuntimeException causes the current process to abort.

In my next comment I'll post a patch that changes the
NPN_GetProperty() method (called by Java Plugin2's implementation of
JSObject.getMember()) to only return a document.URL or
document.documentURI property that Java will understand -- that won't
cause the URL(String) constructor to throw a MalformedURLException.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

As promised in comment #0, my patch changes the NPN_GetProperty()
method to only return a document.URL or document.documentURI property
that Java will understand.

If the "original" document.URL or document.documentURI property isn't
a proper URL, or if its scheme isn't in the small lists of
schemes/protocols that Java understands, it returns a "fake" URL with
the "http" scheme and a random hostname ending in an invalid TLD
(currently ".xyz").  The idea is to return a URL that won't make the
URL(String) constructor choke, but which also won't cause any usable
network privileges to be granted.

Comment 2

[Steven Michaud [:smichaud] (Retired)]

At some point I'll post a tryserver build made with my patch from comment #1 plus some debug logging.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Since bug 589041 only causes Java Plugin2 to crash, this bug isn't really security sensitive.  But I suspect it should be kept hidden until bug 589041 is no longer restricted.

Comment 4

[Boris Zbarsky [:bzbarsky]]

If we can't share the code between here and bug 611897 (why not?), then we at least need comments on both chunks of code pointing to each other so bugs will get fixed in both of they're discovered.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

(In response to comment #4)

I assume you're talking about the MakeRandomString() method and associated code.

Tell me where you think it should go, so that it can be shared.

Comment 6

[Boris Zbarsky [:bzbarsky]]

nsNetUtil.h?

Comment 7

[Steven Michaud [:smichaud] (Retired)]

I'll add a MakeRandomInvalidURL() to nsNetUtil.h.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 490310 [details] [diff] [review]
Fix

I need to make some changes to this patch, which I'll do next week.

Comment 9

[Daniel Veditz [:dveditz]]

We need this on branch, too, to fix bug 589041. I think it also fixes bug 594443 which should have the same underlying cause.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev1 (fix various problems)

I've followed Boris' suggestion to move part of my patch to
nsNetUtil.h, where it can be shared (notably by my patch for bug
611897).  I've asked Boris to review that part of both patches at bug
611897.

Josh, please review the rest of the patch (the NPRuntime part).

Comment 11

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 490933 [details] [diff] [review]
Fix rev1 (fix various problems)

I've already found one mistake:

I need to add the following line to nsNetUtil.h.  Windows builds fail without it:

#include <stdlib.h>

Comment 12

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev2 (generate UUID for hostname)

This follows a suggestion made by Boris in bug 611897 comment #16.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev4 (change NS_MakeRandomInvalidURLString() per Boris' suggestion)

This patch follows Boris' suggestions from bug 611897.

Comment 14

[Steven Michaud [:smichaud] (Retired)]

Josh, please do your review as soon as possible.

There's a branch code freeze tonight, and this bug is supposed block
the next releases on both branches.

I actually doubt that it should block -- this bug isn't a security
hole for Java Plugin2 (just a crash bug).  But if there's to be any
chance for this bug to be fixed on the branches today, you need to do
your review right away.

Comment 15

[Steven Michaud [:smichaud] (Retired)]

This bug isn't going to get fixed before tonight's freeze -- we've run out of time.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev5 (update nsNetUtil.h with changes from final patch for bug 611897)

Comment 17

[Josh Aas]

Comment on attachment 491752 [details] [diff] [review]
Fix rev5 (update nsNetUtil.h with changes from final patch for bug 611897)

>+  NPUTF8* propertyName = _utf8fromidentifier(property);
>+  if (!propertyName)
>+    return true;
>+  bool notURL =
>+    (PL_strcasecmp(propertyName, "URL") &&
>+     PL_strcasecmp(propertyName, "documentURI"));
>+  nsMemory::Free(propertyName);

In case '_memfree' uses a different free memory function in the future you should probably just use '_memfree' instead of 'nsMemory::Free'. That way it will always match the 'utf8fromidentifier' allocator.

>diff --git a/modules/plugin/base/src/nsNPAPIPluginInstance.h b/modules/plugin/base/src/nsNPAPIPluginInstance.h
>--- a/modules/plugin/base/src/nsNPAPIPluginInstance.h
>+++ b/modules/plugin/base/src/nsNPAPIPluginInstance.h
...
> public:
>   // True while creating the plugin, or calling NPP_SetWindow() on it.
>   PRPackedBool mInPluginInitCall;
> 
>+  PRPackedBool mIsJavaPlugin;

I don't like duplicating data like this, it leads to sync issues. If you want to know if an instance is a Java instance get it's plugin (nsNPAPIPluginInstance::GetPlugin) and then get the tag for the plugin (nsPluginHost::TagForPlugin). That has an "mIsJavaPlugin" variable.

Comment 18

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev6 (follow Josh's suggestions)

How's this?

Comment 19

[Josh Aas]

Comment on attachment 491858 [details] [diff] [review]
Fix rev6 (follow Josh's suggestions)

>+  nsNPAPIPluginInstance* inst = (nsNPAPIPluginInstance*) npp->ndata;
>+  nsNPAPIPlugin* plugin = inst->GetPlugin();

I realize "ndata" should never be null here but you may want to consider a null check anyway, just to be safe.

Comment 20

[Steven Michaud [:smichaud] (Retired)]

> I realize "ndata" should never be null here but you may want to consider a null
> check anyway, just to be safe.

OK, I'll do that when I land the patch.

New patches coming up for the branches.  I'll ask you to review them when they're ready.

Comment 21

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 491858 [details] [diff] [review]
Fix rev6 (follow Josh's suggestions)

Landed on trunk:
http://hg.mozilla.org/mozilla-central/rev/16c2e141d418

Comment 22

[Daniel Holbert [:dholbert]]

Backed out, due to apparent leaks during crashtest:
backout: http://hg.mozilla.org/mozilla-central/rev/0f7432f2cb5d
merge:   http://hg.mozilla.org/mozilla-central/rev/0830d8f97936

There were leaks on Linux Debug crashtest runs for this push & the one after:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1290192341.1290192765.26978.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1290192624.1290192989.28478.gz

The leaks both looked like this:
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 196 bytes during test execution
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsLocalFile with size 124 bytes
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsPluginHost with size 60 bytes
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsStringBuffer with size 8 bytes
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsTArray_base with size 4 bytes

Comment 23

[Daniel Holbert [:dholbert]]

Seems to affect Linux Debug Reftest, too. (That run just completed & had the exact same objects leaked):
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1290192456.1290193921.32328.gz

Comment 24

[Steven Michaud [:smichaud] (Retired)]

I wish you'd let the tests run for one more cycle -- this may be another random orange.

I'll try to reproduce the leaks locally.

Comment 25

[Steven Michaud [:smichaud] (Retired)]

Though if it *is* a random orange, it may keep happening after my patch was backed out.

I hadn't at first notice that you let the tests run for two cycles.

Comment 26

[Steven Michaud [:smichaud] (Retired)]

I didn't see these leaks when I tested on the tryserver yesterday.

Comment 27

[Daniel Holbert [:dholbert]]

At backout time, I was pretty sure it was nonrandom because
(a) it was the exact same leak for two subsequent crashtest-runs (and now two reftest runs, too*)
(b) I couldn't find a matching intermittent leak in bugzilla
(c) one of the leaked objects is nsPluginHost, and this bug's push touched nsPluginHost.h and 2 other plugin-related files

* Second reftest failure with this leak:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1290192801.1290194448.2190.gz

So, this looks almost certainly nonrandom.  Odd that it didn't show up on TryServer, though -- did your TryServer run have Linux Debug cycles?

Comment 28

[Steven Michaud [:smichaud] (Retired)]

> did your TryServer run have Linux Debug cycles?

Yes.  I ran two sets of tests, and both sets had 32-bit and 64-bit Linux cycles (opt and debug).

You're probably right this isn't random.  But I can't see how my patch could have caused it.

Could you trigger new runs of the tests that failed?

I'll run a new set of tryserver tests and see what happens.

Comment 29

[Daniel Holbert [:dholbert]]

(In reply to comment #28)
> Could you trigger new runs of the tests that failed?

I can't, but RelEng can.  They actually prefer that bugs be filed for re-triggers, so they can track the various types of requests that come in.

So, I filed Bug 613555 to get reftest & crashtest re-triggered on your push's cycle.

Comment 30

[Steven Michaud [:smichaud] (Retired)]

Oh shit, never mind.

Now I see what I did wrong :-(

(The patches I tested yesterday were slightly different than the one I landed today.)

I'll try again in a bit.

Thanks very much for filing that bug to re-run the tests ... even though I now see it won't be necessary.

Comment 31

[Daniel Holbert [:dholbert]]

Ok, resolved the re-running-tests bug as INVALID.  Glad you figured out what was going on. :)

Comment 32

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 491858 [details] [diff] [review]
Fix rev6 (follow Josh's suggestions)

Landed again on trunk:
http://hg.mozilla.org/mozilla-central/rev/73b2cd4fbd95

The problem with the previous patch is that I called nsPluginHost::GetInst() without releasing what it returned.

Comment 33

[Steven Michaud [:smichaud] (Retired)]

Attachment: 1.9.2-branch patch

Comment 34

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 491973 [details] [diff] [review]
1.9.2-branch patch

Oops.  Part of patch is missing.

Comment 35

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 491973 [details] [diff] [review]
1.9.2-branch patch

No, nothing is missing.

My changes to nsNetUtil.h have already been landed.

Comment 36

[Steven Michaud [:smichaud] (Retired)]

Attachment: 1.9.1-branch patch

Comment 37

[Josh Aas]

Comment on attachment 491973 [details] [diff] [review]
1.9.2-branch patch

>+  nsNPAPIPluginInstance* inst = (nsNPAPIPluginInstance*) npp->ndata;
>+  if (!inst || !inst->mLibrary)
>+    return false;
>+  nsNPAPIPlugin* plugin = ((PluginPRLibrary *)inst->mLibrary)->GetPlugin();

I'm not sure the assumption you're making with this cast is valid. The "mLibrary" member variable is of type "PluginLibrary", which is a base class for "PluginPRLibrary" and "PluginModuleParent". If the plugin is out-of-process then this variable could be of type "PluginModuleParent". Maybe you should move the 'nsNPAPIPlugin' member for the library into the base class.

Comment 38

[Steven Michaud [:smichaud] (Retired)]

Attachment: 1.9.2-branch patch rev1 (fix problem)

>>+  nsNPAPIPluginInstance* inst = (nsNPAPIPluginInstance*) npp->ndata;
>>+  if (!inst || !inst->mLibrary)
>>+    return false;
>>+  nsNPAPIPlugin* plugin = ((PluginPRLibrary *)inst->mLibrary)->GetPlugin();
>
> I'm not sure the assumption you're making with this cast is
> valid. The "mLibrary" member variable is of type "PluginLibrary",
> which is a base class for "PluginPRLibrary" and
> "PluginModuleParent". If the plugin is out-of-process then this
> variable could be of type "PluginModuleParent".

Sigh.

> Maybe you should move the 'nsNPAPIPlugin' member for the library
> into the base class.

Rather than changing a cross-module API, and then only on the 1.9.2
branch, I think it's best to add a (weak) mPlugin member to
nsNSAPIPluginInstance (like the one that already exists on the trunk).
In fact this is probably what I should have done from the beginning.

Comment 39

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 492235 [details] [diff] [review]
1.9.2-branch patch rev1 (fix problem)

Landed on the 1.9.2 branch:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3f47fec824f9

Comment 40

[Daniel Veditz [:dveditz]]

I'd like to get this into 1.9.1.13/1.9.2.16 if we need a respin for any reason. We've already taken bug 611897 which sort of gives the game away, and this variant will affect more 3.6 people than the OJI version.

Comment 41

[Steven Michaud [:smichaud] (Retired)]

As I said at bug 594443 comment #8, I think it's unlikely that the patches for this bug (bug 611910) or bug 611897 will fix bug 594443.

There's no evidence that JavaScript-to-Java LiveConnect is involved with bug 594443.

On the other hand, a URL scheme whitelist might work for bug 594443.

Comment 42

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 491987 [details] [diff] [review]
1.9.1-branch patch

Josh, please review my 1.9.1-branch patch, so that it's ready to land when/where it's judged to be needed.

Comment 43

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 491987 [details] [diff] [review]
1.9.1-branch patch

Landed on the 1.9.1 branch:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fd2fcd4a4686

Comment 44

[Steven Michaud [:smichaud] (Retired)]

I believe this patch ended up being included in the 1.9.2.13 and 1.9.1.16 releases (as the result of respins).  Is that correct?

Comment 45

[:Gavin Sharp [email: [e-mail]]]

No, it wasn't included in those earlier releases because of respins - this bug's "status" markers are still correct. Respins generally occur on "relbranches" (mini-branches off the "main" branch), so they don't automatically pick up later changes landed on the "main" branch.

Comment 46

[Steven Michaud [:smichaud] (Retired)]

OK, thanks.  I've now confirmed what you said by looking under the appropriate tags here (http://hg.mozilla.org/releases/mozilla-1.9.2/tags) and here (http://hg.mozilla.org/releases/mozilla-1.9.1/tags).

I misread some of the recent entries in the branches' tinderbox pushlogs, and this led me to think my patch had been included in the 1.9.2.13 and 1.9.1.16 releases.

Comment 47

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

Are the STR to verify the fix for this using the testcase from bug 589041 on Windows or Linux (since OS X didn't crash there)?

Comment 48

[Steven Michaud [:smichaud] (Retired)]

This bug was reproducible on Windows, Linux and OS X -- but only with
Java Plugin2.  So you need to make sure you're using Java Plugin2 (aka
the new Java plugin) when you try to reproduce this bug (using the
testcase from bug 589041).

Of course you also need to test with builds that don't (yet) have my
patch.

Comment 49

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

(In reply to comment #48)
> This bug was reproducible on Windows, Linux and OS X -- but only with
> Java Plugin2.  So you need to make sure you're using Java Plugin2 (aka
> the new Java plugin) when you try to reproduce this bug (using the
> testcase from bug 589041).

 Thanks.
 
> Of course you also need to test with builds that don't (yet) have my
> patch.

If I didn't know this, then I'd be unqualified for my job, Steven. Basic QA. :-)

Comment 50

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

Where does one get the new Java Plugin2? Perhaps I'm misunderstanding but I looked at http://javaplugin.sourceforge.net/ and the JEP is only for OS X. I'm attempting to look at this on XP.

Comment 51

[Steven Michaud [:smichaud] (Retired)]

Apple's port of Sun/Oracle's Java Plugin2 "comes with" on both OS X
10.5.8 and 10.6.X (it's been bundled with Apple's Java Updates for
both OSs for about a year and a half).  To be sure you have it, make
sure you've installed Apple's latest Java Update for whichever OS
version (10.5.8 or 10.6.X) you're testing with -- Java for Mac OS X
10.5 Update 8 or Java for Mac OS X 10.6 Update 3.  (Apple's Java
Plugin2 is in /Library/Internet Plugin-Ins/JavaPlugin2_NPAPI.plugin.)

Then you need to remove JavaEmbeddingPlugin.bundle and
MRJPlugin.plugin from the Contents/MacOS/plugins/ directory of
whatever distro you're testing with.  (When the browser is looking for
plugins, this is the directory it looks in first (before other
directories like /Library/Internet Plug-Ins/).  So when the JEP is
here, Firefox will use it instead of whatever other Java plugin is
installed elsewhere.)

Java Plugin2 is included with Java 6 on Windows and Linux, and is (I
think) always the default on Windows.  It's usually also the default
on Linux ... though there can be exceptions (I can go into greater
detail about this if necessary).

Sun/Oracle's OJI plugin is also included with Sun/Oracle's Java 6 on
both Windows and Linux.  But (like I just said) it's normally not
used.  (The OJI plugin *isn't* included on OS X -- the JEP (which also
uses OJI) takes its place there.)

Comment 52

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

For XP, I installed the latest Java 6 and I ran both the testcases and had no crashes whatsoever in 1.9.2.13, which is pre-fix. 

I'll take a look on OS X 10.6.

Comment 53

[Steven Michaud [:smichaud] (Retired)]

> For XP, I installed the latest Java 6 and I ran both the testcases
> and had no crashes whatsoever in 1.9.2.13, which is pre-fix.

Remember that these are client-process crashes (crashes in Java
Plugin2's client process (not Firefox's client process)).  So you
might have missed them (I did the first time I tested).

Here's the best way to test for them:

1) Run Firefox and load any Java applet (e.g. the Clock applet at
   http://java.sun.com/applets/jdk/1.4/demo/applets/Clock/example1.html).

2) Load bug 589041's testcase in a different tab or window and run one
   of its tests.

3) Check to see if the applet from step 1 is still running -- it won't
   be if Java Plugin2's client process has crashed.

(For more info see bug 589041 comment #13.)

Comment 54

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

Thanks. 

Verified for 1.9.2.14 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.14pre) Gecko/20110111 Namoroka/3.6.14pre ( .NET CLR 3.5.30729) on XP. Running as discussed in comment 53 crashes the other applet pre-fix and does not do so post-fix.

Verified for 1.9.1.17 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.17pre) Gecko/20110111 Shiretoko/3.5.17pre ( .NET CLR 3.5.30729).

Comment 55

[Martin Stránský [:stransky] (ni? me)]

Attachment: 1.8 version

Comment 56

[Brandon Sterne (:bsterne)]

Unhiding since this is only a plugin process abort.

Comment 57

[LegNeato]

I backed this out of 1.9.2: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8f0513110015

Comment 58

[Steven Michaud [:smichaud] (Retired)]

Chris and Dan:

Given the large number of bug reports on this issue on the 1.9.2
branch (FF 3.6.14), you might want to consider backing out the patches
for bug 611910 and bug 620773 on the trunk, too -- at least sometime
before the FF4 release.

As best I can tell, the original bug reported here is seldom
encountered, and not terribly severe (a plugin crash).  So the side
effects of my patches (as they now stand) currently look much worse
than the problem my patches fixed.

I currently have no idea why these problems are happening.  It will
take at least a couple of days of careful digging to get to the bottom
of this.  And it might take quite a bit longer to fix the problem
(whatever it turns out to be) -- especially considering how fragmented
the market for Java applets is, and how difficult it is to do
comprehensive testing.

It's also puzzling that I didn't discover these problems in my
original tests (of which I did quite a lot), and that they weren't
reported until quite a long time after my patches had landed.  But
here it's easier to understand the reasons why.  Many Java applets are
proprietary, and have a relatively small customer base.  So it's
possible for us to break them and not realize it until we've done a
major release.

I'll do my best to find a way to fix my patches for bug 611910 and bug
620773.  But then I think it's best that we try to reach out for
testing (of whatever I've come up with) to those who've reported side
effects of my current patches.

Comment 59

[Benjamin Smedberg]

Since we're no longer using JEP on trunk, but using javaplugin2, and we cannot reproduce the bug on trunk, I don't think there's value in backing this out on trunk.

Comment 60

[Steven Michaud [:smichaud] (Retired)]

This has nothing to do with the JEP.

There are some indications that the problems reported on the 1.9.2 branch can also be reproduced on the trunk.  But, of course, most users of Java applets aren't testing on the trunk.

I'll know more once I can reproduce the problem(s) myself.

Comment 61

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

I checked 1.9.2.15 (where this is removed) on pogo.com and the Java applets appear to be working fine there.

Comment 62

[Steven Michaud [:smichaud] (Retired)]

> There are some indications that the problems reported on the 1.9.2
> branch can also be reproduced on the trunk.

I'm finally able to reproduce bug 629030 on the 1.9.2 branch, but not
on the trunk (see bug 629030 comment #50).  As best I can currently
tell, what people see on the trunk is a different problem, unconnected
with my patch for bug 611910, and even with Firefox (since it also
happens in IE8) -- see bug 629030 comment #49.

Comment 63

[Henry Jen]

We would like to look into the test case for bug 589041 to see if there is anything could be done in Java plugin side. Could someone help us get access to it?

Comment 64

[LegNeato]

CC'd you on the bug.