788031 - Content Policy callbacks (including CSP) for the Java plugin should receive the java codebase as a URI

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Giorgio Maone [:ma1]]

Attachment: Scratchpad demonstrating this bug

Since some unidentified Gecko 17 build up to now, calls to nsIContentPolicy implementations are broken for the Java plugin (tested with Oracle Java 1.6.350.10 on Windows 7 64bit).

Specifically: 
1) nsIContentPolicy.shouldLoad() is never called
2) nsIContentPolicy.shouldProcess(aContentType, aContentLocation, ...) is called with a null aContentLocation value

The attached scratchpad-cp.js file, if loaded in a Scratchpad window (shift+F4, ctrl+O) and executed with chrome privileges (Environment>Browser, ctrl+R), clearly demonstrates the problem.

Expected output in the Error Console:

[Test CP] shouldLoad application/x-java-vm from http://java.sun.com/applets/jdk/1.4/demo/applets/ArcTest/ArcTest.class
[Test CP] shouldProcess application/x-java-vm from http://java.sun.com/applets/jdk/1.4/demo/applets/ArcTest/ArcTest.class

Actual result:

[Test CP] shouldProcess application/x-java-vm from null

This make any content policy which blocks content by checking its origin URL completely unusable on Firefox 17 and above (NoScript 2.5.4rc1 implements a suboptimal work-around, but by no way an acceptable solution).

This problem is IMHO particularly severe (marked severity as "major") in the light of the recent Java-related 0 day outbreaks, and I suspect it may also break internal stuff such as CSP or some kinds of hotfix mitigations, so I'm marking it as security sensitive for the time being.

Notice that on Firefox 15 I get

[Test CP] shouldLoad application/x-java-vm from http://java.sun.com/applets/jdk/1.4/demo/applets/ArcTest/ArcTest.class
[Test CP] shouldLoad application/x-java-vm from http://java.sun.com/applets/jdk/1.4/demo/applets/ArcTest/ArcTest.class

(shouldLoad is called twice, while shouldProcess is never called) which may or may not be another bug, but is surely more manageable.

Comment 1

[Benjamin Smedberg]

-> johns, almost certainly from bug 745030 or one of its followups. Is this for all of object/embed/applet or only applet?

Comment 2

[John Schoenick [:johns]]

This would be changed by bug 781126, though it's not clear that this is necessarily breakage. On applets, java's "URI" read from the "code" field is not a standard URI and was only incidentally parsed (wrongly in a lot of cases) by the browser.

I'll look more into this tomorrow.

Comment 3

[Giorgio Maone [:ma1]]

(In reply to John Schoenick [:johns] from comment #2)
> This would be changed by bug 781126, though it's not clear that this is
> necessarily breakage.

It is breakage. Many content policies block by type AND URI (e.g. NoScript, Adblock Plus, RequestPolicy...).

Notice also that calling just shouldProcess() at execution time, and omitting shouldLoad(), likely breaks any content policy (like RequestPolicy) which aims to prevent CSRF attacks, because the blocking, if any, can happen only when the HTTP request has been already completed.

> On applets, java's "URI" read from the "code" field is
> not a standard URI and was only incidentally parsed (wrongly in a lot of
> cases) by the browser.

In facts, current NoScript work-around tries to emulate this now missing parsing step.
This may be done better, but it needs to be done nevertheless for most content policy implementations to keep working as users expect or be useful at all.
The most interesting bit, for NoScript at least, is the @codebase property/attribute which sets the "origin" (the class loader root, actually) where the Java classes are loaded from. I agree that this, merged with @code and @archive, may lead to confusion or plain wrongness, but it should be reliable enough to tell at least which host is serving the Java binaries and whether the load is same-site or cross-site.

Comment 5

[Benjamin Smedberg]

I don't believe that this bug needs to remain hidden: it is present on nightly/aurora only and doesn't affect the correctness of in-product features.

Comment 6

[Giorgio Maone [:ma1]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)
> I don't believe that this bug needs to remain hidden: it is present on
> nightly/aurora only and doesn't affect the correctness of in-product
> features.

Isn't CSP an in-product feature?

http://evil.hackademix.net/csp/applet/

Comment 7

[Benjamin Smedberg]

Ah yes, you can break CSP with this. I still don't think it's serious enough to keep hidden though. We're definitely going to fix this before release. We should definitely add this to the CSP tests!

Comment 8

[John Schoenick [:johns]]

(In reply to Giorgio Maone from comment #3)
> (In reply to John Schoenick [:johns] from comment #2)
> > This would be changed by bug 781126, though it's not clear that this is
> > necessarily breakage.
> 
> It is breakage. Many content policies block by type AND URI (e.g. NoScript,
> Adblock Plus, RequestPolicy...).

The URI, however, is determined by java, using non-standard information. E.g.:

> <object classid="java:a.b">
>   <param name="codebase" value="http://google.com/">
> </object>

Loads http://google.com/a/b.class
Without codebase, on page foo.com/java/, loads foo.com/java/a/b.class
In an applet tag, on foo.com/java, loads foo.com/a/b.class
In an object tag with codebase "ftp:" loads ftp://foo.com/java/a/b.class
In an applet tag with codebase "ftp:" loads ftp://foo.com/a/b.class
(this is off of the top of my head and might not be exact, but you get the idea)

Trying to determine what URI java will ultimately try to open is beyond Firefox. The fact that we blindly appended "code" onto the current baseURI previously did not in any way produce a reliable URL, so giving that URL to shouldLoad/shouldProcess consumers is a false promise. And if you blocked things based on origin, malicious sites could bypass it by constructing a URI that FF and Java will see completely differently

> Notice also that calling just shouldProcess() at execution time, and
> omitting shouldLoad(), likely breaks any content policy (like RequestPolicy)
> which aims to prevent CSRF attacks, because the blocking, if any, can happen
> only when the HTTP request has been already completed.

We never open a channel for java - it always starts without a channel, and then loads resources on its own (not even via NPAPI). shouldProcess() is called before we instantiate the plugin, and java's own network requests follow that. Previously we would call shouldLoad() and, in some cases, could be tricked into opening a channel (that would never be used) on the mis-constructed URI, which we would call shouldProcess on - which is nowhere near useful or correct

> > On applets, java's "URI" read from the "code" field is
> > not a standard URI and was only incidentally parsed (wrongly in a lot of
> > cases) by the browser.
> 
> In facts, current NoScript work-around tries to emulate this now missing
> parsing step.
> This may be done better, but it needs to be done nevertheless for most
> content policy implementations to keep working as users expect or be useful
> at all.
> The most interesting bit, for NoScript at least, is the @codebase
> property/attribute which sets the "origin" (the class loader root, actually)
> where the Java classes are loaded from. I agree that this, merged with @code
> and @archive, may lead to confusion or plain wrongness, but it should be
> reliable enough to tell at least which host is serving the Java binaries and
> whether the load is same-site or cross-site.

Assuming java's odd pseudo-URL parsing of classid, data, code, and the associated <param> values can't be used to change origin, we could reasonably guess at what host java will ultimately make requests to by looking at codebase. I (or someone) would need to review java's applet code and be sure that there is, in fact, no other way to specify an origin. Adding it blindly now is implying to content policy consumers that we know what java's behavior will be, when we do not.

We can leave this marked as sec until bug 738396 lands, as we have other gotchas with java's request handling that need to be addressed.

Comment 9

[John Schoenick [:johns]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> Ah yes, you can break CSP with this. I still don't think it's serious enough
> to keep hidden though. We're definitely going to fix this before release. We
> should definitely add this to the CSP tests!

Bug 781126 also fixed the relevant in-browser CSP for the new behavior, and we do have some tests -- thunderbird's docshell permissions tests, iframe sandbox tests

Comment 10

[Daniel Veditz [:dveditz]]

So is this basically a duplicate of bug 781126 then? Tbird uses content policies to block plugins and remote content from mail messages.

CC'ing CSP folks to make sure we've got a testcase that covers this.

Comment 11

[Daniel Veditz [:dveditz]]

I'm rating this sec-high based on the thunderbird case. For CSP only (i.e. Firefox) it's probably more sec-moderate.

Comment 12

[John Schoenick [:johns]]

(In reply to Daniel Veditz [:dveditz] (mostly offline until Sept 4) from comment #11)
> I'm rating this sec-high based on the thunderbird case. For CSP only (i.e.
> Firefox) it's probably more sec-moderate.

To be clear, I don't think there is any issue with thunderbird nor firefox here, bug 745030 regressed this, and bug 781126 fixed it. The issue is third party CSP implementations get a lot less info about what java plugin we're going to instantiate, but previously they got *wrong* info, so it's not really much of a regression.

Our behavior now is correct, but this bug is drawing attention to the fact that we have no idea what codebase java will end up using (bug 406541, bug 738396), which is one of the things bug 745030 actually makes possible to fix.

Comment 13

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

Based on comment 12, this would appear to be a sec-moderate at this point. Is that correct?

Comment 14

[John Schoenick [:johns]]

(In reply to Al Billings [:abillings] from comment #13)
> Based on comment 12, this would appear to be a sec-moderate at this point.
> Is that correct?

Yes, the issue wherein CSP doesn't receieve a proper codebase for java is an extension of bug 406541, so should be a sec-moderate.

The change wherein we no longer pass shouldLoad() for java is intentional, so there's no regression here.

Comment 15

[Alex Keybl [:akeybl]]

We don't track sec-moderate non-regression issues for release, and will let the security team re-nominate as necessary.

Comment 16

[John Schoenick [:johns]]

Discussed this in triage - there is no direct security issue here. We don't offer CSP a URI for java plugins, but our previous behavior was less correct (an invalid URI).

I think this should remain sec flagged until bug 406541 lands to avoid drawing attention to our java codebase handling, however.

Comment 17

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

What does our CSP implementation do in this problematic case?

Surely object-src must be made to able to block such loads. Even if the full URL is not available, having shouldLoad called for such loads would seem to be necessary for CSP, unless this part of CSP is implemented in a different way.

Comment 18

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Tanvi, this sounds like a problem for mixed content blocker too. Is it?

Comment 19

[John Schoenick [:johns]]

(In reply to Brian Smith (:bsmith) from comment #17)
> What does our CSP implementation do in this problematic case?
> 
> Surely object-src must be made to able to block such loads. Even if the full
> URL is not available, having shouldLoad called for such loads would seem to
> be necessary for CSP, unless this part of CSP is implemented in a different
> way.

Because java is instantiated without a channel, and then goes and opens its own channels outside of our control, only shouldProcess() is called for java plugins. They can be blocked as expected from there. After bug 738396 lands, we'll have the necessary code to tell CSP what *codebase* java will use, which might be helpful to pass to CSP.

Comment 20

[Garrett Robinson [:grobinson]]

Attachment: Scratchpad demonstrating this bug (w/ updated link)

In investigating this bug, I noticed the scratchpad demo was no longer working (the described error messages did not appear in the Error Console/Browser Console). This is because the link to an example applet was broken and redirecting to some random Oracle website. I replaced it with a link to an example applet from the Java tutorial and now it works again and logs the expected error messages.

It would also be good to update the link on the CSP demo page (evil.hackademix.net/csp/applet/).

As a side note, accessing the CSP demo page as it is now appears to expose another Java bug. It causes my entire browser to become incredibly slow (but not totally unresponsive, Ctrl-Q still quits after a few seconds waits). This problem does not occur in Chrome. I am running Ubuntu 13.04 with openjdk-7 and icedtea-7. I am trying to get in touch with someone working on Java integration to see if this is worth reporting as a separate bug.

Comment 21

[Tanvi Vyas[:tanvi]]

Looks like Bug 738396 just landed.

Mixed Content Blocker treats requests for resources made by the plugin (TYPE_SUBREQUEST) as Mixed Display Content.  If shouldLoad is not called for these requests, then two things will happen:
1) Users might see the lock icon when they should see the globe
2) Users who have turned off mixed display content will experience mixed content loads anyway.

From the first comment it says that shouldProcess is called but shoudLoad is not.  in nsMixedContentBlocker.cpp shouldProcess calls shouldLoad.  I thought this was true of other content policies as well. (example: http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDataDocumentContentPolicy.cpp#137).  But we do depend on a non-null aContentLocation.

Comment 22

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas [:tanvi] from comment #21)
> But we do depend on a non-null
> aContentLocation.

Actually, if aContentLocation is null and the Requesting Location is https, then we will classify the content as Mixed Content (display or active depending on the Content Type) and block if the corresponding pref to block is set to true.

So, if shouldProcess is called with a null aContentLocation for java object subrequests, then I think that means we will end up treating them as mixed content, even if they aren't.

Comment 23

[John Schoenick [:johns]]

(In reply to Tanvi Vyas [:tanvi] from comment #21)
> Looks like Bug 738396 just landed.
> 
> Mixed Content Blocker treats requests for resources made by the plugin
> (TYPE_SUBREQUEST) as Mixed Display Content.  If shouldLoad is not called for
> these requests, then two things will happen:
> 1) Users might see the lock icon when they should see the globe
> 2) Users who have turned off mixed display content will experience mixed
> content loads anyway.
> 
> From the first comment it says that shouldProcess is called but shoudLoad is
> not.  in nsMixedContentBlocker.cpp shouldProcess calls shouldLoad.  I
> thought this was true of other content policies as well. (example:
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/
> nsDataDocumentContentPolicy.cpp#137).  But we do depend on a non-null
> aContentLocation.

Java does not use NPAPI to make any web requests, period. If it wants to make any requests it does so in its own process without informing us. This is not something we can reasonably fix on our end. If we wanted to be conservative we could always assume the worst when java was running.

As for this bug, the issue is that CSP isn't informed of any URI when java loads. Now that 738396 has landed we can pass the codebase java will use to shouldProcess, but calling shouldLoad still does not make sense as we don't load any channel :(

Comment 24

[Benjamin Smedberg]

We could also refuse to run Java (or any non-Flash plugin) when a restrictive CSP is present. File that separately?

Comment 25

[John Schoenick [:johns]]

Attachment: For object/embed/applet tags, pass baseURI to shouldProcess if mURI is null

Now that 738396 has landed mBaseURI is now what java will actually use as a codebase. Passing this to shouldProcess is slightly more useful than passing null.

Comment 26

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to John Schoenick [:johns] from comment #23)
> As for this bug, the issue is that CSP isn't informed of any URI when java
> loads. Now that 738396 has landed we can pass the codebase java will use to
> shouldProcess, but calling shouldLoad still does not make sense as we don't
> load any channel :(

Not only CSP but all nsIContentPolicies should receive the Java codebase so they all have the opportunity to block the applet/object. CSP is just a special case of that.

The main limitation of that would be that CSP, mixed content blocker, and other similar content policies would not be able to handle redirects properly, because our redirect handling code is (will be) assuming that Gecko HTTP channels are used for the loads. If the Java plugin gets its HTTP proxy configuration from Gecko then we may be able to set up a special proxy for it to use, which proxies through Gecko so that we can control the requests it makes. But, that sounds like a lot of work so I suggest we just punt on the redirect and subrequest aspects for now.

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #24)
> We could also refuse to run Java (or any non-Flash plugin) when a
> restrictive CSP is present. File that separately?

Let's discuss that in a separate place. Others have had ideas about how to block plugins selectively  by type in CSP, and doing that should not be necessary for this bug, unless we are concerned about the limitation I just mentioned in my previous paragraph. object-src 'none' should block all plugins already.

Comment 27

[John Schoenick [:johns]]

(In reply to Brian Smith (:briansmith), was [e-mail] (:bsmith) from comment #26)
> (In reply to John Schoenick [:johns] from comment #23)
> > As for this bug, the issue is that CSP isn't informed of any URI when java
> > loads. Now that 738396 has landed we can pass the codebase java will use to
> > shouldProcess, but calling shouldLoad still does not make sense as we don't
> > load any channel :(
> 
> Not only CSP but all nsIContentPolicies should receive the Java codebase so
> they all have the opportunity to block the applet/object. CSP is just a
> special case of that.

Sorry, that's what I meant. The attached patch makes the baseURI available to content policy in all cases where there is no mURI

> The main limitation of that would be that CSP, mixed content blocker, and
> other similar content policies would not be able to handle redirects
> properly, because our redirect handling code is (will be) assuming that
> Gecko HTTP channels are used for the loads. If the Java plugin gets its HTTP
> proxy configuration from Gecko then we may be able to set up a special proxy
> for it to use, which proxies through Gecko so that we can control the
> requests it makes. But, that sounds like a lot of work so I suggest we just
> punt on the redirect and subrequest aspects for now.

Java only makes subrequests - we instantiate it without any file or URI period, and if it decides to open any URIs it does so outside of our control or ability to predict. I highly doubt it asks gecko for proxy configuration but I could be wrong.

> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #24)
> > We could also refuse to run Java (or any non-Flash plugin) when a
> > restrictive CSP is present. File that separately?
> 
> Let's discuss that in a separate place. Others have had ideas about how to
> block plugins selectively  by type in CSP, and doing that should not be
> necessary for this bug, unless we are concerned about the limitation I just
> mentioned in my previous paragraph. object-src 'none' should block all
> plugins already.

Comment 28

[John Schoenick [:johns]]

Comment on attachment 772731 [details] [diff] [review]
For object/embed/applet tags, pass baseURI to shouldProcess if mURI is null

https://hg.mozilla.org/integration/mozilla-inbound/rev/616f0b208426

Comment 29

[Ed Morley [:emorley]]

(In reply to John Schoenick [:johns] from comment #28)
> Comment on attachment 772731 [details] [diff] [review]
> For object/embed/applet tags, pass baseURI to shouldProcess if mURI is null
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/616f0b208426

https://hg.mozilla.org/mozilla-central/rev/616f0b208426

Comment 30

[Daniel Veditz [:dveditz]]

Would be nice to fix this on ESR24 given the longevity of that branch.

Comment 31

[John Schoenick [:johns]]

Comment on attachment 772731 [details] [diff] [review]
For object/embed/applet tags, pass baseURI to shouldProcess if mURI is null

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
Per comment 30, this is a simple change affecting how content policy works for some plugins (largely java)

User impact if declined: 
Features/addons using content policy will have less information about java and other less-common plugins to base their allow/block decisions on

Fix Landed on Version:
25

Risk to taking this patch (and alternatives if risky): 
very low

String or UUID changes made by this patch:
None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 32

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

Release Management, I'd like to take this for ESR24 for longterm stability and because it touches Java.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr24/rev/27ed31ccbb79

Comment 34

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed null content location problem with current shipping FF24.
Verified correct behavior using attached test on 24esr and 25, 2013-10-16.