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

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: smichaud, Assigned: smichaud)

Tracking

({crash, verified1.9.1, verified1.9.2})

Trunk
crash, verified1.9.1, verified1.9.2
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+, status1.9.2 wanted, blocking1.9.1 .17+, status1.9.1 .17-fixed)

Details

(Whiteboard: (goes with bug 611897))

Attachments

(4 attachments, 6 obsolete attachments)

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

Updated

8 years ago
Blocks: 589041
(Assignee)

Comment 1

8 years ago
Created attachment 490310 [details] [diff] [review]
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.
Assignee: nobody → smichaud
Status: NEW → ASSIGNED
(Assignee)

Updated

8 years ago
Attachment #490310 - Flags: review?(joshmoz)
(Assignee)

Comment 2

8 years ago
At some point I'll post a tryserver build made with my patch from comment #1 plus some debug logging.
(Assignee)

Comment 3

8 years ago
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.
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.
(Assignee)

Comment 5

8 years ago
(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.
nsNetUtil.h?
(Assignee)

Comment 7

8 years ago
I'll add a MakeRandomInvalidURL() to nsNetUtil.h.
(Assignee)

Comment 8

8 years ago
Comment on attachment 490310 [details] [diff] [review]
Fix

I need to make some changes to this patch, which I'll do next week.
Attachment #490310 - Flags: review?(joshmoz)
(Assignee)

Updated

8 years ago
blocking2.0: --- → ?
We need this on branch, too, to fix bug 589041. I think it also fixes bug 594443 which should have the same underlying cause.
blocking1.9.1: --- → .16+
blocking1.9.2: --- → .13+
status1.9.1: --- → wanted
status1.9.2: --- → wanted
Blocks: 594443
Created attachment 490933 [details] [diff] [review]
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).
Attachment #490310 - Attachment is obsolete: true
Attachment #490933 - Flags: review?(joshmoz)
Whiteboard: [sg:high]
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>
Created attachment 491051 [details] [diff] [review]
Fix rev2 (generate UUID for hostname)

This follows a suggestion made by Boris in bug 611897 comment #16.
Attachment #490933 - Attachment is obsolete: true
Attachment #491051 - Flags: review?(joshmoz)
Attachment #490933 - Flags: review?(joshmoz)

Updated

8 years ago
blocking2.0: ? → betaN+
Created attachment 491633 [details] [diff] [review]
Fix rev4 (change NS_MakeRandomInvalidURLString() per Boris' suggestion)

This patch follows Boris' suggestions from bug 611897.
Attachment #491051 - Attachment is obsolete: true
Attachment #491633 - Flags: review?(joshmoz)
Attachment #491051 - Flags: review?(joshmoz)
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.
This bug isn't going to get fixed before tonight's freeze -- we've run out of time.
Created attachment 491752 [details] [diff] [review]
Fix rev5 (update nsNetUtil.h with changes from final patch for bug 611897)
Attachment #491633 - Attachment is obsolete: true
Attachment #491752 - Flags: review?(joshmoz)
Attachment #491633 - Flags: review?(joshmoz)

Comment 17

8 years ago
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.
Attachment #491752 - Flags: review?(joshmoz) → review-
Created attachment 491858 [details] [diff] [review]
Fix rev6 (follow Josh's suggestions)

How's this?
Attachment #491752 - Attachment is obsolete: true
Attachment #491858 - Flags: review?(joshmoz)

Comment 19

8 years ago
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.
Attachment #491858 - Flags: review?(joshmoz) → review+
> 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 on attachment 491858 [details] [diff] [review]
Fix rev6 (follow Josh's suggestions)

Landed on trunk:
http://hg.mozilla.org/mozilla-central/rev/16c2e141d418
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
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
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.
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.
I didn't see these leaks when I tested on the tryserver yesterday.
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?
> 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.
(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.
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.
Ok, resolved the re-running-tests bug as INVALID.  Glad you figured out what was going on. :)
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.
Created attachment 491973 [details] [diff] [review]
1.9.2-branch patch
Attachment #491973 - Flags: review?(joshmoz)
Comment on attachment 491973 [details] [diff] [review]
1.9.2-branch patch

Oops.  Part of patch is missing.
Attachment #491973 - Flags: review?(joshmoz)
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.
Attachment #491973 - Flags: review?(joshmoz)
Created attachment 491987 [details] [diff] [review]
1.9.1-branch patch
Attachment #491987 - Flags: review?(joshmoz)

Comment 37

8 years ago
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.
Attachment #491973 - Flags: review?(joshmoz) → review-
Created attachment 492235 [details] [diff] [review]
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.
Attachment #491973 - Attachment is obsolete: true
Attachment #492235 - Flags: review?(joshmoz)

Updated

8 years ago
Attachment #492235 - Flags: review?(joshmoz) → review+
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
(Assignee)

Updated

8 years ago
status1.9.2: wanted → .14-fixed

Updated

8 years ago
blocking1.9.1: .16+ → .17+
blocking1.9.2: .13+ → .14+
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.
blocking1.9.1: .17+ → ?
blocking1.9.2: .14+ → ?
Whiteboard: [sg:high] → [sg:high] (goes with bug 611897)
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 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.

Updated

8 years ago
Attachment #491987 - Flags: review?(joshmoz) → review+
(Assignee)

Updated

8 years ago
Attachment #491987 - Flags: approval1.9.1.17?
blocking1.9.1: ? → .17+
blocking1.9.2: ? → .14+
(Assignee)

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
status1.9.1: wanted → .17-fixed
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
Attachment #491987 - Flags: approval1.9.1.17?
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?
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.
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.
(Assignee)

Updated

8 years ago
No longer blocks: 594443

Updated

8 years ago
Depends on: 620773
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)?
Whiteboard: [sg:high] (goes with bug 611897) → [sg:high] (goes with bug 611897) [qa-needs-STR] [qa-examined-191] [qa-examined-192]
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.
(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. :-)
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.
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.)
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.
> 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.)
Whiteboard: [sg:high] (goes with bug 611897) [qa-needs-STR] [qa-examined-191] [qa-examined-192] → [sg:high] (goes with bug 611897) [qa-examined-191] [qa-examined-192]
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).
Keywords: verified1.9.1, verified1.9.2
Whiteboard: [sg:high] (goes with bug 611897) [qa-examined-191] [qa-examined-192] → [sg:high] (goes with bug 611897)
Alias: CVE-2011-0060
Unhiding since this is only a plugin process abort.
Alias: CVE-2011-0060
Group: core-security
Whiteboard: [sg:high] (goes with bug 611897) → (goes with bug 611897)
Keywords: crash

Comment 57

8 years ago
I backed this out of 1.9.2: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8f0513110015
blocking1.9.2: .14+ → ?
status1.9.2: .14-fixed → wanted
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

8 years ago
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.
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.
I checked 1.9.2.15 (where this is removed) on pogo.com and the Java applets appear to be working fine there.
> 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

8 years ago
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

8 years ago
CC'd you on the bug.

Updated

8 years ago
Depends on: 629030

Updated

8 years ago
blocking1.9.2: ? → ---
You need to log in before you can comment on or make changes to this bug.