611897 - Only pass URL schemes to the OJI plugin that Java can understand

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

This bug was spun off from bug 589041, to fix that bug for OJI
plugins.

OJI uses the current script principal's origin to configure the
privileges of Java code that's called (via the nsISecureEnv interface)
from JavaScript code (aka JavaScript-to-Java LiveConnect).

As the testcase from bug 589041 shows, if the script principal's
origin is a malformed URL (or one that Java can't understand),
JavaScript-to-Java calls will run with abnormally high privileges:
Specifically, it will be allowed to read any file.

This happens in the following method in SecureInvocation.java (part of
the source distro for Sun/Oracle JDKs going back to Java 1.4.2):

private static ProtectionDomain
  getDefaultProtectionDomain(String urlString, boolean direct)

This method tries to construct a URL object from the urlString
parameter, using the URL(String) constructor.  If the constructor
fails with a MalformedURLException, the 'url' variable gets set to
'null'.  Then if 'url' is 'null' or its scheme/protocol is 'file', the
following permission is added to the PermissionCollection object from
which a ProtectionDomain will be created:

FilePermission("<<ALL FILES>>", "read")

The 'urlString' value passed to getDefaultProtectionDomain()
ultimately comes from a call to nsCSecurityContext::GetOrigin() in
Mozilla OJI code.

In my next comment I'll post a patch that changes this method to only
return an 'origin' 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
nsCSecurityContext::GetOrigin() method to only return an 'origin' that
Java will understand.

If the "original" origin 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)]

I'd like to post a Linux test build made with my patch from comment #1, plus some debug logging.  But wierdness in my current Linux build configuration prevents me from doing that.  Hopefully I'll find a way around it next week.

(As of a few months ago, the tryservers are only available do to trunk builds.)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Please set the TLD to "invalid" instead, since that's actually guaranteed to never be a working TLD?

Comment 4

[Steven Michaud [:smichaud] (Retired)]

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

Oops, forgot to mention that this is a patch on 1.9.1-branch code -- since it can only be tested on the 1.9.1 branch.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

> Please set the TLD to "invalid" instead, since that's actually guaranteed to
> never be a working TLD?

Good idea.  I'll try that out next week.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

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

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

Comment 7

[Daniel Veditz [:dveditz]]

(In reply to comment #6)
> I need to make some changes to this patch, which I'll do next week.

Please do it today or tomorrow, code freeze for the branch is Thursday and you'll need to leave time to get reviews.

Comment 8

[Daniel Veditz [:dveditz]]

(In reply to comment #1)
> If the "original" origin 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.

Why not just return an error, "no java for you!"?

If returning an error doesn't stop java execution then I worry about the error return further down if origin is null. What happens then?

Comment 9

[Steven Michaud [:smichaud] (Retired)]

> Why not just return an error, "no java for you!"?

Then you may break Java in unexpected ways, like what happened when I
did the same kind of thing in JEP 0.9.7.4's workaround for bug 589041,
and triggered bug 606737 and bug 607678.

I thought we'd agreed on a URL scheme whitelist (bug 589041 comment
#50).

> If returning an error doesn't stop java execution then I worry about
> the error return further down if origin is null. What happens then?

I don't understand what you're saying.  With my current patch, a null
origin gets replaced by a fake URL that Java can understand, but can't
use to access the network.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

> With my current patch, a null origin gets replaced by a fake URL
> that Java can understand, but can't use to access the network.

Oops, that's wrong.

nsCSecurityContext::GetOrigin() does an error return if it finds a
NULL origin.  And now I see this is what you're referring to.

I'll look into what happens when nsCSecurityContext::GetOrigin() does
an error return.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev1 (fix various problems)

> I'll look into what happens when nsCSecurityContext::GetOrigin()
> does an error return.

Since my patch for bug 610525 landed, I don't think it's any longer
possible that nsCSecurityContext::GetOrigin() will fail to find an
origin while doing JavaScript-to-Java LiveConnect.

But there's only one place this method is called (from
getAndPackSecurityInfo() in the OJI plugin's remotejni.cpp), and the
caller doesn't check for an error return.  So I've bullet-proofed
nsCSecurityContext::GetOrigin() as much as possible.  And I now call
it from browser-side OJI code, just before every possible
JavaScript-to-Java call, and abort the call if (by some very unlikely
chance) nsCSecurityContext::GetOrigin() does an error return.

We really shouldn't say "no Java for you" unless absolutely necessary
-- we're usually deep inside plugin code, and it's hard to predict
what Java will do when we start breaking things.  I wish I'd thought
of my fake URL solution when I was doing my workaround for bug 589041
in JEP 0.9.7.4 -- it would have been much less disruptive.

I've also followed Boris' suggestion from bug 611910 comment #4 and
bug 611910 comment #6 to put some of my patch into nsNetUtil.h where
it can be shared (for example by my patch for bug 611910).

Note that this patch (like the previous one) is on the 1.9.1 branch.

The OJI plugin's getAndPackSecurityInfo() calls
nsCSecurityContext::GetOrigin() with a 256-byte stack buffer -- which
is why I do the same thing from ProxyJNIEnv's various methods.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

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

Boris, please review the changes I've made to nsNetUtil.h.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

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 14

[Boris Zbarsky [:bzbarsky]]

If we're actually going for unique hostnames, shouldn't we do something better than rand() seeded with a known value that doesn't update very often?

Comment 15

[Steven Michaud [:smichaud] (Retired)]

> If we're actually going for unique hostnames, shouldn't we do
> something better than rand() seeded with a known value that doesn't
> update very often?

If you have a better idea, please suggest it.  I'm not an expert on
this.

Comment 16

[Boris Zbarsky [:bzbarsky]]

I think it would make more sense to generate a UUID as the hostname.  See the code in nsNullPrincipal::Init.  You'll want to strip the '{' and '}' chars off the string ToProvidedString creates, but other than that it should work.

Comment 17

[Steven Michaud [:smichaud] (Retired)]

OK, I'll try that.  New patch coming up.

Comment 18

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev2 (follow Boris' suggestion, sort of)

This patch makes NS_MakeRandomInvalidURLString() optionally use
nsIUUIDGenerator to generate a UUID for the hostname, and falls back
to using NS_MakeRandomString().

The fallback is needed if nsIUUIDGenerator fails.  But I also
discovered that using nsIUUIDGenerator is a significant performance
hit if it's called at all often (as it is by the OJI plugin and my
current OJI patch).  Possibly it's using a randomness provider that
blocks until sufficient randomness is available (I tested on Linux).

So I've made my OJI patch (this bug's patch) tell
NS_MakeRandomInvalidURLString() to always use NS_MakeRandomString().

Since the OJI plugin is obsolete and seldom used on Windows or Linux,
I figure this isn't a great loss.

My new patch for bug 611910 tells NS_MakeRandomInvalidURLString() to
use nsIUUIDGenerator.

Comment 19

[Boris Zbarsky [:bzbarsky]]

OK, so I feel like I'm missing something.

Do we actually need unique/unguessable return values for what Java needs here security-wise?  If that's the case, why are not particularly guessable but pretty guessable ones ok?  If not, why not just use a constant string?

Comment 20

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev3 (deal better with performance hit)

Here's a new patch that does a better job of dealing with the
performance hit I mentioned in comment #18.

Turns out this isn't caused by a randomness provider, or by any
Mozilla code.  Instead it seems to be a subtle problem with the pipe
that the OJI plugin uses to pass information from JavaScript code
running on the main thread to Java code running on secondary threads.
Somehow, rapidly changing the "origin" causes a dramatic slowdown in
the operation of this pipe.

So if we need to use a fake origin, my new patch dramatically reduces
the frequency with which that fake origin is changed.  This doesn't
open a security hole:  Some of Java's same-origin checks may now pass
that would fail it the fake origin changed more often.  But
same-origin checks are only used to grant (or deny) network access,
and none of the fake origins (which are invalid URLs) can be used to
access the network.

My new patch also changes NS_MakeRandomString() in nsNetUtil.h.

Turns out the only reason I didn't see the above-mentioned performance
hit when using NS_MakeRandomString() is that it only changed the seed
(passed to srandom()) every second -- so that frequent calls would
return the same "random" string.

A comment in the previous code claims the granularity of PR_Now() on
"the Mac" is one second.  But this was only true of Mac Carbon code,
which has been gone since FF 2.0 days.  So my code now assumes that
PR_Now() has millisecond granularity -- which (judging by the
"milliseconds since epoch" comment) was the original intention all
along.

Comment 21

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #19)

> why not just use a constant string?

This would be visible in our source code, and hence easily guessable.

I think my new patch is a better solution.

Comment 22

[Steven Michaud [:smichaud] (Retired)]

> Do we actually need unique/unguessable return values for what Java
> needs here security-wise?

We don't need unique return values, and we may not need unguessable
ones -- presuming I'm right about Java only using same-origin checks
to grant or deny network access.

Somehow, though, I feel more comfortable with what my current patch
does.

Comment 23

[Boris Zbarsky [:bzbarsky]]

OK, thanks for explaining.

1)  If we fail to get the UUID generator service, I think we should just throw
    an exception or equivalent.  That should not be happening.

2)  I'd prefer not hardcoding "36" when we have a perfectly good NSID_LENGTH
    lying around that we can use (with a few subtracted obviously).

3)  Are there plans to use NS_CheckIsJavaCompatibleURLString anywhere other
    than this code?

4)  PR_Now on modern Mac has microsecond granularity as far as I can tell, but
    given that I don't think we need NS_MakeRandomString at all (see item 1), I
    think that's not relevant.

5)  GetFakeOrigin should return |const nsCString&| and be a const method,
    right?

Comment 24

[Steven Michaud [:smichaud] (Retired)]

> 1) If we fail to get the UUID generator service, I think we should
>    just throw an exception or equivalent.  That should not be
>    happening.

In both this bug's patch and my patch for bug 611010, we very much
want to avoid saying "no Java for you", if at all possible.  And my
changes substantially improve NS_MakeRandomString().  So I'd really
prefer to keep falling back to it.

> 2) I'd prefer not hardcoding "36" when we have a perfectly good
>    NSID_LENGTH lying around that we can use (with a few subtracted
>    obviously).

OK, I'll use 'NSID_LENGTH - 3'.  The reason I used '36' is that
NSID_LENGTH (at 39) is (currently) one longer than the length of a
UUID (presumably to account for null termination), and I was a bit
worried this might change (while the actual length of a UUID is
extremely unlike to change).

But yes, it's always better to use a macro for this kind of value.  So
I'll do that.

> 3) Are there plans to use NS_CheckIsJavaCompatibleURLString anywhere
>    other than this code?

Yes.  It's already used by my patch for bug 611910.  And I suspect
we'll want to use it in a patch for bug 594443.

> 5) GetFakeOrigin should return |const nsCString&| and be a const
>    method, right?

Oops, stupid mistake.  I'll fix it.

Comment 25

[Boris Zbarsky [:bzbarsky]]

> we very much want to avoid saying "no Java for you"

Look, if the UUID service is not present, then nsDocument objects can't be instantiated at all.  So either the browser won't start, or you're so deep inside XPCOM shutdown that the component is not longer being handed out by the service manager.  In either case, no java seems perfectly fine to me.

Comment 26

[Steven Michaud [:smichaud] (Retired)]

OK, I'll get rid of the fallback to NS_MakeRandomString(), and make appropriate other changes.

Comment 27

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev4 (follow Boris' suggestions)

Comment 28

[Steven Michaud [:smichaud] (Retired)]

jst and dveditz:

If there's to be any chance that a fix for this bug makes tonight's
code freeze, you need to do your reviews right away.

Comment 29

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 491630 [details] [diff] [review]
Fix rev4 (follow Boris' suggestions)

Oops, I found a flaw in my current patch (in the OJI code).  New patch coming up.

Please start your reviews anyway -- the new patch won't change very much.

And nsNetUtil.h won't change at all from my current patch.

Comment 30

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev5 (fix potential security hole in previous patch)

My previous patch opened a potential security hole:

It would have made a single nsCSecurityContext object persist for the
entire life of a ProxyJNIEnv object.  One ProxyJNIEnv object is
(potentially) created for every thread.  But JavaScript-to-Java calls
are always made on the main thread -- so they're always made from the
same ProxyJNIEnv object.

As I said above, I don't think it's a problem for the same fake origin
to be used for every call from a ProxyJNIEnv object.  But the
nsCSecurityContext object encapsulates additional security
information, which it *would* (potentially) be a problem to share
between different instances of the OJI plugin.  (I say "potentially"
because we're talking about non-applet security, and I'm not sure it's
possible to legitimately make non-applet JavaScript-to-Java calls with
enhanced privileges.)

My new patch takes care of this by only making a fake origin persist
for the life of a ProxyJNIEnv object.

Comment 31

[Johnny Stenback (:jst)]

Comment on attachment 491669 [details] [diff] [review]
Fix rev5 (fix potential security hole in previous patch)

- In ProxyJNIEnv::~ProxyJNIEnv():

+    NS_IF_RELEASE(mContext);

This looks unrelated to this fix, but looks like a valid potential leak fix. But there's also some potential risk in doing this. If you feel confident in this fix, I'm fine with taking it, else, we could leave this out until we have time to leave it bake someplace.

- In nsCSecurityContext::GetOrigin(char* buf, int buflen):

+        printf("nsCSecurityContext::GetOrigin(): origin '%s' not Java-compatible, changing to '%s'\n",
+               origin.get(), mFakeOrigin.get());

Remove the printf. Or at least wrap it in #ifdef DEBUG or somesuch

r=jst with that.

Comment 32

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 491669 [details] [diff] [review]
Fix rev5 (fix potential security hole in previous patch)

> +    NS_IF_RELEASE(mContext);
>
> This looks unrelated to this fix, but looks like a valid potential
> leak fix.  But there's also some potential risk in doing this. If
> you feel confident in this fix, I'm fine with taking it, else, we
> could leave this out until we have time to leave it bake someplace.

In fact it comes from the previous version of my patch -- where I
noticed the leak.  Since mContext is initialized to NULL, I'm pretty
sure it won't cause any trouble.

> +        printf("nsCSecurityContext::GetOrigin(): origin '%s' not"
> +               "Java-compatible, changing to '%s'\n",
> +               origin.get(), mFakeOrigin.get());
>
> Remove the printf. Or at least wrap it in #ifdef DEBUG or somesuch

This strayed from my debug patch.  I'll get rid of it on landing.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 491669 [details] [diff] [review]
Fix rev5 (fix potential security hole in previous patch)

>+++ b/modules/oji/src/ProxyJNI.cpp

The repeated copy/paste here really looks like it wants to be a helper function.

>+++ b/modules/oji/src/nsCSecurityContext.cpp

Drive-by, but:

     buf[originlen] = nsnull; // Gotta terminate it.

should be '\0' instead, no?

And ideally we would just output into an nsCString, not a fixed-size buffer.  But whatever; this is not that important.

>+++ b/netwerk/base/public/nsNetUtil.h
>+#define NU_FAKE_SCHEME "http://"
>+#define NU_FAKE_TLD ".invalid"

Not sure where the "NU" prefix comes from.  Why not just "NS"?

>+NS_MakeRandomInvalidURLString(nsCString& result)
>+  result.Assign(NU_FAKE_SCHEME);

AssignLitera, please

>+  result.Append(NU_FAKE_TLD);

And AppendLiteral here.

r=me with those nits.

Comment 34

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #32)

> In fact it comes from the previous version of my patch -- where I
> noticed the leak.  Since mContext is initialized to NULL, I'm pretty
> sure it won't cause any trouble.

On second thought, though, it may be better to leave sleeping dogs
lie.  So if I land this patch tonight, I'll leave out the call to
NS_IF_RELEASE(mContext) from the ProxyJNIEnv destructor.

Comment 35

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 491669 [details] [diff] [review]
Fix rev5 (fix potential security hole in previous patch)

Landed on the 1.9.1 branch with jst's and bzbarsky's fixes:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/05202664ed20

I decided not to wait for dveditz' review.  If he has any suggestions, he can make them later.

Comment 36

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 491669 [details] [diff] [review]
Fix rev5 (fix potential security hole in previous patch)

Landed on the 1.9.2 branch:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/85be45fb4a34

Comment 37

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #33)

>>+++ b/netwerk/base/public/nsNetUtil.h
>>+#define NU_FAKE_SCHEME "http://"
>>+#define NU_FAKE_TLD ".invalid"
>
> Not sure where the "NU" prefix comes from.  Why not just "NS"?

NU stands for NetUtil.

NS is fine, though, and that's what's in the final patch.

Comment 38

[Steven Michaud [:smichaud] (Retired)]

Gregory Fleischer, please test tomorrow's 1.9.1-branch (FF 3.5.X)
Linux nightly plus the OJI plugin with the first of your testcases
from bug 589041.

Comment 39

[Mike Hommey [:glandium]]

Attachment: Backport for 1.9.0

Only difference is includes in netwerk/base/public/nsNetUtil.h