1. View this URL - preferably while inspecting HTTP headers: http://webappsec-test.info/~mwobensmith/CSP/object-src/CSP_2_3.php Result: Applet is loaded, despite prohibited via CSP directive. There are no console errors. Ignore the text on the page that may or may not indicate test passed/failed; just note presence of applet. The applet is sourced from a subdomain different than the containing HTML page. Note that this is different than bug 908824, as no errors are generated and it affects FF24+.

We never open channels on behalf of java and have little control over what it can/cannot load. In bug 738396 we added the ability to predict what codebase java will see, and in bug 788031 we pass that to shouldProcess as the URI. If it makes it cleaner from a CSP perspective we can fire a fake "shouldLoad" for the codebase URI, but beyond that what URIs java choses to open (including cross-domain ones) are out of our control.

Matt: does Chrome have the same issue? Does the network request show up in our network log (clearly our bug then) or is it really coming from java as johns said above (would be nice to block if possible). Maybe CSP needs to look at ShouldProcess in addition to ShouldLoad?

RE: What does Chrome do? On Linux - the only platform where it supports Java AFAIK - it has the same behavior as FF. The applet loads, despite the CSP. RE: The network request. I can confirm what John says - the request for the class file comes from Java and not FF. It doesn't show up in FF's network log.

Any idea of when we might fix this?

The only real way to block this is to completely disallow plugins on pages with CSP. I'm fine with that proposal. In any case I don't think this needs to be a hidden bug. We should be able to have a public discussion about what this means.

Hey all, apologies for letting this one sit. I can't repro locally because the test case crashes my Firefox (that's a Linux/Java bug, not a Firefox bug). Will try to get a test case that doesn't crash my machine going, or else get a Windows VM. Matt and I sat down with this bug back in October and ran the test case while monitoring the network requests. The gist is what Matt said in Comment 3 - the call to applet just instantiates the Java plugin, which then performs all network requests, including loading the applet code. The only way to block this AFAICT is what bsmedberg suggested in Comment 5 - just block (or at least click to play) all plugins on pages with a CSP that touches objects (uses default-src or object-src). This isn't a great solution. It doesn't actually enforce the actual provided CSP, it just defaults to object-src 'none'. And I worry about it affecting CSP adoption, especially for users (like SendSafely [0]) who want to use both CSP and plugins. [0] http://blog.sendsafely.com/post/68798984897/bypassing-content-security-policy-with-java-applets That blog post is interesting because it shows that Webkit is able to block some objects. It blocks <embed> and <object>, but fails to block <applet>. Firefox fails to block all 3. Does anybody know what they're doing differently in this regard?

Chrome's implementation [0] is said to "attempt to block it" but that "plugin loading code is a strange and scary mess". [0] http://lists.w3.org/Archives/Public/public-webappsec/2013Nov/0004.html

Given that the link information is public, I don't see any reason for this bug to remain private. Can we open it? Firefox does the initial load for <object> and <embed>, so we can apply object-src to them. Java does the initial load for <applet> because we don't attempt to parse <applet> parameters in any way. However I thought we already fixes this for <object> and <embed> in bug 908824. Perhaps we fixed it for most plugins but not Java? Given that Java is known to break CSP, I'm not sure why we'd allow Java to run on any page with a network-restricting CSP rule. It doesn't make sense to promote CSP adoption for sites that also want to run Java. It's an interesting question, if we know that a plugin does most or all of its networking through the browser, whether we should allow it. This is specifically true of normal HTTP requests from Flash and I think Silverlight. Although there are still things like streaming protocols where the plugin bypasses the browser and does its own networking.

This has been posted in other CSP bugs, but just to make sure: Plugins *can* make network requests via NPAPI, which we can observe and block. They do not *have* to. Java, for instance, does not. Flash, IIRC, is well behaved and always does. If we allow an unknown plugin to load, even with no initial stream, we have no guarantee it wont go make network requests on its own and expose them to the page. One way to fix this would be a whitelist of plugins known to only use NPAPI for plugin requests, and blocking others (Java) from CSP pages via blocking their load in shouldProcess.

grobinson: are you still working on this?

Yes, I can be. I was never able to get this working on my Linux box, but now I have a desktop that dual boots Windows so I can probably get it going on that. Here's what needs to be done: 1. See if we block plugins loaded by <object> and <embed>, and, if we don't, if that's fixable. bsmedberg says that should've been fixed in bug 908824, but the sendsafely blog post says differently. 2. Based on Comment 9, it seems like we can't trust Java (at a minimum) on pages with CSP. If we can't fix that issue (all signs point to it being out of our control, unfortunately), we need to devise a default policy for handling plugins that can circumvent CSP on pages that set a CSP. Finally, as in Comment 8, the info in this bug is already public, and I support opening it.

Here's the relevant (dated) discussion in the WG: http://lists.w3.org/Archives/Public/public-webappsec/2013Nov/0001.html. I will try to resurrect this by putting a test case up.

The esteemed Mr. Wobensmith has created a test case (http://webappsec-test.info/~mwobensmith/CSP/applet/CSP_applet.php) that attempts to load a Java applet using: 1. <applet> 2. <object> 3. <embed> Note that we (incorrectly) load the applet in all 3 cases at the moment. Also note that no requests appear in the Web Console, which indicates the load of the applet itself is bypassing Firefox's network stack and therefore our nsIContentPolicy-based CSP implementation. We need to see if it's possible to reign in this behavior and allow the content policy to have more control over these loads. If that's impossible, we'll need to default to click-to-play all plugins on pages that set a CSP, or some other proposal similar to Comment 5.

(In reply to Garrett Robinson [:grobinson] from comment #13) > The esteemed Mr. Wobensmith has created a test case > (http://webappsec-test.info/~mwobensmith/CSP/applet/CSP_applet.php) that > attempts to load a Java applet using: > > 1. <applet> > 2. <object> > 3. <embed> > > Note that we (incorrectly) load the applet in all 3 cases at the moment. > Also note that no requests appear in the Web Console, which indicates the > load of the applet itself is bypassing Firefox's network stack and therefore > our nsIContentPolicy-based CSP implementation. > > We need to see if it's possible to reign in this behavior and allow the > content policy to have more control over these loads. Other than inserting ourself into the java process and trying to insert binary detours, or intercepting their networking with forced proxy settings or something else, it is not. This would rapidly get very hacky and would need to be customized to every plugin we want to somehow force into compliance, and is probably not anything we want to spend time on. Plugins like java are not sandboxed or even contained - they run as their own process and can do whatever they want. If they do not ask us to handle network streams for them and do not implement/respect CSP, they will not work. Note on the link above that all these cases use e.g. <object code="...">. "code" is not an attribute we use, and it's not even a valid URI. This is only passed to java, which then goes on to use its own logic to decide what to load. In the case of java *specifically*, we replicate the logic java uses to determine its "codebase", due to other security issues with its behavior in the past. A side effect of this is that CSP can see what codebase java will use in shouldProcess. With more understanding of java's security rules than i have, we might be able to determine if allowing a java load with a specific codebase will cause it to violate CSP or not, but that is not a general solution. > If that's impossible, > we'll need to default to click-to-play all plugins on pages that set a CSP, > or some other proposal similar to Comment 5. If we know in advance that a plugin uses NPAPI for all network requests, we could whitelist it. We still would not be able to differentiate between "active" and "passive" content in that case.

> If we know in advance that a plugin uses NPAPI for all network requests, we could whitelist it. What is the strength of this guarantee? Is it a plugin author promising "We only use NPAPI", but with the potential for a plugin update to change that behavior in a difficult-to-detect way (e.g. unless you audit it with Wireshark)? > We still would not be able to differentiate between "active" and "passive" content in that case. Not sure what you mean here. Is this mixed content terminology?

> With more understanding of java's security rules than i have, we might be able to determine if allowing a java load with a specific codebase will cause it to violate CSP or not, but that is not a general solution. I agree this is a poor solution, and will probably be difficult to write and maintain. We may have to go with bsmedberg's "block all plugins on pages w/ CSP" suggestion from Comment 5. Chrome is able to block these loads and enforce CSP correctly (try the test case in Chrome). How can they do that, but we can't? Pepper?

(In reply to Garrett Robinson [:grobinson] from comment #16) > Chrome is able to block these loads and enforce CSP correctly (try the test > case in Chrome). How can they do that, but we can't? Pepper? They are probably just better at guessing or re-implementing of how those URIs resolve.

(In reply to Garrett Robinson [:grobinson] from comment #15) > > If we know in advance that a plugin uses NPAPI for all network requests, we could whitelist it. > > What is the strength of this guarantee? Is it a plugin author promising "We > only use NPAPI", but with the potential for a plugin update to change that > behavior in a difficult-to-detect way (e.g. unless you audit it with > Wireshark)? I'm not sure. We could try and confirm with adobe (or someone who knows enough about flash) that it does/doesn't circumvent NPAPI for network requests. Adobe is unlikely to make major changes to flash going forward, so that might be good enough for the special case of flash only. > > We still would not be able to differentiate between "active" and "passive" content in that case. > > Not sure what you mean here. Is this mixed content terminology? Yes, I'm not 100% familiar with this either, but I believe there was some effort to classify Flash loads as for "active" content (script, etc) and passive, such as images. AFAIK, there is no reasonable way to do this for any plugin, including flash. (In reply to Garrett Robinson [:grobinson] from comment #16) > > With more understanding of java's security rules than i have, we might be able to determine if allowing a java load with a specific codebase will cause it to violate CSP or not, but that is not a general solution. > > I agree this is a poor solution, and will probably be difficult to write and > maintain. We may have to go with bsmedberg's "block all plugins on pages w/ > CSP" suggestion from Comment 5. > > Chrome is able to block these loads and enforce CSP correctly (try the test > case in Chrome). How can they do that, but we can't? Pepper? AFAIK Java only works via NPAPI in chrome. Someone could dive into the chromium source code and look, but, one possibility is that they emulate java's processing "code"/"codebase" to determine from where it will try to load, and CSP block based on that. This would not affect subsequent accesses made by java itself. This would be easy to verify: Write a simple applet that makes a network request that violates CSP, but load it from a location allowed by CSP. If that is all they are doing, we can already do the same: shouldProcess() is given the result of our "codebase" detection for java, before we proceed with launching it. If they are somehow filtering individual requests from java once it is already running, they are doing something more advanced as mentioned above.

Part of the problem here is that CSP is only checked in nsIContentPolicy's shouldLoad (shouldProcess is a no-op that always accepts). Since the behavior wrt Java was changed somewhat recently (see bug 788031 comment 19), the guess at the codebase uri is only passed to nsIContentPolicies via ShouldProcess. Therefore, CSP is not utilizing it at the moment. This should be easy to fix by adding a special case for TYPE_OBJECT in CSP's ShouldProcess. I'll take a stab at a patch now.

This patch correctly blocks all 3 attempted Java loads in the Comment 13 test case. Two other things are needed for landing: 1. The violation reporting behavior is a bit confusing - I see 1 reported violation, when I would expect 3. This is almost certainly due to the decision caching in shouldLoad. 2. Needs a test

Comment on attachment 8403003 [details] [diff] [review] 908933_csp_block_java.patch Review of attachment 8403003 [details] [diff] [review]: ----------------------------------------------------------------- Yep, needs tests. Please re-flag when you've got 'em. ::: content/base/src/contentSecurityPolicy.js @@ +894,5 @@ > + aContext, > + aMimeType, > + aExtra); > + } > + Can we make the main contents of shouldLoad an internal function that's called from both shouldLoad and here from shouldProcess, then inside shouldLoad *avoid* all the cases where shouldProcess calls it? This way we don't end up with duplicates. For example, pseudocode: shouldLoad() { if (TYPE_OBJECT) { return ACCEPT; } internalHelperFunc(args); } shouldProcess() { if (!TYPE_OBJECT) { return ACCEPT; } internalHelperFunc(args); }

Note that we can change how we call shouldLoad/ShouldProcess for plugins pretty easily, if it would make more sense to call ShouldLoad despite having no channel, for instance: http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsObjectLoadingContent.cpp#1989

After discussing this problem with Sid, Garrett and Tanvi, I think we actually want to fix that. Garretts initial patch was bitrotted, due to the new implementation of CSP. Here is a new one. Now the question, what is the best way to test this?

The test plugin can pretend to be java, which should trigger the shouldProcess logic and so on: > <object type="application/x-java-test" codebase="foo"> See [1] for an example of using this along with the test plugin's getJavaCodebase() to see if it was allowed to load or not. This doesn't actually guarantee that java's own security rules forbid loading URIs outside the codebase under all circumstances, so we still have the general "plugins can violate CSP" problem AFAIK. [1] http://dxr.mozilla.org/mozilla-central/source/dom/plugins/test/mochitest/test_bug738396.html

Thanks John for providing the info on how to test. The attached patch verifies that TYPE_OBJECT when loaded using: > <object type="application/x-java-test" codebase="foo"></object> is subject to ShouldProcess but not ShouldLoad. John, Sid, can you take a look?

Comment on attachment 8470304 [details] [diff] [review] bug_908933_tests.patch This looks fine in that it checks that the special codepath for java still CSP checks the codebase URI -- I assume some other test is making sure shouldLoad fails? However, because of the various fun ways we can try to load java and maybe open a channel before spawning it, I would include a few more permutations: > <object classid="java:foo"></object> > <object classid="java:foo" codebase="./bar"></object> > <object data="foo" classid="java:foo" codebase="./bar"></object> > <object type="application/x-java-test"></object> > <!-- There's actually already a test somewhere making sure codebase can't hide or be aliased, but... --> > <object classid="java:foo"> > <param codebase="bar"> > </object> > <applet codebase="bar"></applet> > <embed type="application/x-java-test"> > <embed type="application/x-java-test" codebase="bar"> > <embed src="foo.class" codebase="bar" type="application/x-java-test"> > <!-- It would take multiple paragraphs to explain why this works and doesn't open a channel --> > <embed src="foo.class"> > <embed src="foo.class" codebase="bar"> These should all either fail in shouldProcess or shouldLoad. I think the object data=something" tests should hit shouldLoad first, but because of the odd rules with java, we should test that it hits something, because of really convoluted logic in these instances -- e.g., <embed src="foo.class"> will both select java and ignore src, opening no channel, thus no shouldLoad. Also, what should <object type="application/x-shockwave-flash">, or another no-URI-no-codebase-not-java plugin do? We still don't know that a plugin with no URI can't be used to violate CSP, and I'm pretty sure many can. Java, at least, should always have some kind of codebase URI because of bug 738396, but outside the object-src-none case, it could load with an allowed URI and then go on to violate CSP later.

(In reply to John Schoenick [:johns] from comment #26) > However, because of the various fun ways we can try to load java and maybe > open a channel before spawning it, I would include a few more permutations: That is actually a great idea, updated the test to include various permutations. Thanks John for the feedback. > Also, what should <object type="application/x-shockwave-flash">, or another > no-URI-no-codebase-not-java plugin do? That worries me, initially included that test as well, but that load does not end up going trough shouldLoad or shouldProcess. Not sure what we can/should do here. Maybe Sid knows.

> That worries me, initially included that test as well, but that load does > not end up going trough shouldLoad or shouldProcess. Not sure what we > can/should do here. Maybe Sid knows. What triggers channel creations in that case? Possibly we don't have control if it happens out of necko's reach and there's nothing we can do aside from passing the information via NPAPI. (Example: private browsing, see bug 468877).

Plugins should always pass through shouldProcess() before they spawn, even if they have no URI, specifically so that we can block a no-URI-no-codebase-no-nothing plugin from instantiating: http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsObjectLoadingContent.cpp#2152 If it is not, that's probably a bug

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #28) > What triggers channel creations in that case? Plugins can spawn with no channel, and still talk to the page via javascript, which could them provide them a URL of some sort. E.g. flash could be loaded with: > <object type="application/x-shockwave-flash"> > <script> > document.querySelector("object").LoadMovie(1, "./Foo.swf") > </script> Where LoadMovie is a function defined by flash. In that case, they would make the channel via NPAPI, but another plugin, e.g. java, might not (I don't know if java provides a similar mechanism). Flash can also open raw sockets that wouldn't go through NPAPI, I have no idea if just the JS API with no loaded .swf is enough to persuade it to open one. (In reply to Sid Stamm [:geekboy or :sstamm] from comment #28) > Possibly we don't have > control if it happens out of necko's reach and there's nothing we can do > aside from passing the information via NPAPI. (Example: private browsing, > see bug 468877). I've said this before, but, the only way to fully enforce CSP *and* allow a plugin to spawn would be whitelisting known-behaved plugins. I don't know that any plugin at all has been audited to be known to behave.

(In reply to John Schoenick [:johns] from comment #30) > Plugins can spawn with no channel, and still talk to the page via > javascript, which could them provide them a URL of some sort. E.g. flash > could be loaded with: > > <object type="application/x-shockwave-flash"> > > <script> > > document.querySelector("object").LoadMovie(1, "./Foo.swf") > > </script> > > Where LoadMovie is a function defined by flash. In that case, they would > make the channel via NPAPI, but another plugin, e.g. java, might not (I > don't know if java provides a similar mechanism). > > Flash can also open raw sockets that wouldn't go through NPAPI, I have no > idea if just the JS API with no loaded .swf is enough to persuade it to open > one. Sockets - and internal RTMP streaming APIs, which use them - are the only thing the Flash Player does w/r/t networking that does not use NPAPI, AFAIK. LoadMovie is one of the Flash Player's external APIs that doesn't rely on what's implemented in the SWF. There is a comprehensive list somewhere with all of them. However, there isn't a way to do this for Sockets. So, it wouldn't be possible to instantiate a socket within a Flash Player instance purely from the external JS API. > I've said this before, but, the only way to fully enforce CSP *and* allow a > plugin to spawn would be whitelisting known-behaved plugins. I don't know > that any plugin at all has been audited to be known to behave. I agree with this.

Comment on attachment 8473397 [details] [diff] [review] bug_908933_tests.patch Review of attachment 8473397 [details] [diff] [review]: ----------------------------------------------------------------- I think this test is pretty good, but would like to see johns review it too. ::: content/base/test/csp/test_csp_shouldprocess.html @@ +36,5 @@ > + "http://mochi.test:8888/tests/content/base/test/csp/test6", > + // blocked by shouldLoad > + "http://mochi.test:8888/tests/content/base/test/csp/test7.class", > + "http://mochi.test:8888/tests/content/base/test/csp/test8.class", > +]; Do we want positives too (things that do load despite a CSP being present), or is that unnecessary? @@ +48,5 @@ > + else { > + ok(false, "ShouldLoad or ShouldProcess incorreclty blocks TYPE_OBJECT with uri: " + aURI + "!"); > + } > + if (tests.length == 0) { > + window.examiner.remove(); Do we want to detect duplicates? This test will fail if a URL is run through both shouldLoad and shouldProcess... maybe that's ok, but if we want to be able to detect things blocked twice (and thus probably duplicate violation reports), we should instead maintain this list and a "already checked" flag for each URL). @@ +61,5 @@ > +examiner.prototype = { > + observe: function(subject, topic, data) { > + if (topic === "csp-on-violate-policy") { > + var asciiSpec = > + SpecialPowers.getPrivilegedProps(SpecialPowers.do_QueryInterface(subject, "nsIURI"), "asciiSpec"); Nit: I wouldn't bother to wrap this, or would wrap it after the first opening parenthesis. @@ +85,5 @@ > +} > + > +SpecialPowers.pushPrefEnv( > + { "set": [['plugin.java.mime', 'application/x-java-test']] }, > + loadFrame); Do you also have to enable the test plugin? > setTestPluginEnabledState(SpecialPowers.Ci.nsIPluginTag.STATE_ENABLED, "Java Test Plug-in"); http://dxr.mozilla.org/mozilla-central/source/dom/plugins/test/mochitest/test_bug738396.html#13 (I am not familiar enough with our plugin infrastructure to be certain.)

Comment on attachment 8467938 [details] [diff] [review] bug_908933.patch Review of attachment 8467938 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsCSPService.cpp @@ +218,2 @@ > return NS_ERROR_FAILURE; > + } Thanks for bracing this. @@ +220,5 @@ > > + if (aContentType != nsIContentPolicy::TYPE_OBJECT) { > + *aDecision = nsIContentPolicy::ACCEPT; > + return NS_OK; > + } Please put a comment before this if block to note why we do this. Somthing like "only TYPE_OBJECT doesn't hit ShouldLoad and needs to be checked"). Also, do we want TYPE_OBJECT_SUBREQUEST or other types here too or do those hit ShouldLoad first?

Comment on attachment 8473397 [details] [diff] [review] bug_908933_tests.patch Review of attachment 8473397 [details] [diff] [review]: ----------------------------------------------------------------- This LGTM with the comment below ::: content/base/test/csp/test_csp_shouldprocess.html @@ +17,5 @@ > + * We load variations of 'objects' and make sure all the > + * resource loads are correctly blocked by CSP. > + * For all the testing we use a CSP with "object-src 'none'" > + * so that all the loads are either blocked by > + * shouldProcess or shouldLoad. If this test is supposed to cover both java and non-java plugins, it should contain some non-java plugins, since java plugins invoke a rather special load path (which setting plugin.java.mime to x-java-test is triggering for those tags) sorry to keep adding to the cases here, but object tag load logic is scary: > <object type="application/x-test"></object> > <object type="application/x-test" data="foo.tst"></object> > <object data="foo.tst"></object> > <embed src="foo.txt"> > <embed type="application/x-test"> > <embed type="application/x-test" src="foo.tst">

Comment on attachment 8467938 [details] [diff] [review] bug_908933.patch ># HG changeset patch ># User Christoph Kerschbaumer <mozilla@christophkerschbaumer.com> ># Date 1407264428 25200 ># Tue Aug 05 11:47:08 2014 -0700 ># Node ID 79082a1963e7a5dd7aff5193089f590d3609c633 ># Parent fda0b0569fe861e7b6ef2c381d23a83aecf8b7ef >Bug 908933 - CSP: Call ShouldLoad inside ShouldProcess for TYPE_OBJECT (r=sstamm) > >diff --git a/content/base/src/nsCSPService.cpp b/content/base/src/nsCSPService.cpp >--- a/content/base/src/nsCSPService.cpp >+++ b/content/base/src/nsCSPService.cpp >@@ -209,21 +209,32 @@ CSPService::ShouldProcess(uint32_t > nsIURI *aContentLocation, > nsIURI *aRequestOrigin, > nsISupports *aRequestContext, > const nsACString &aMimeTypeGuess, > nsISupports *aExtra, > nsIPrincipal *aRequestPrincipal, > int16_t *aDecision) > { >- if (!aContentLocation) >+ if (!aContentLocation) { > return NS_ERROR_FAILURE; >+ } If there is no content location, this might be an object load with no URI or channel. To account for these cases, you need to check if(!aContentLocation && aContentType != TYPE_OBJECT) (similar to http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp#610). In the case of aContentLocation && aContentType == TYPE_OBJECT, what is the right behavior? Call shouldLoad() or allow the load? How would shouldLoad work for csp without aContentLocation?

(In reply to Tanvi Vyas [:tanvi] from comment #35) > > { > >- if (!aContentLocation) > >+ if (!aContentLocation) { > > return NS_ERROR_FAILURE; > >+ } > > If there is no content location, this might be an object load with no URI or > channel. To account for these cases, you need to check if(!aContentLocation > && aContentType != TYPE_OBJECT) (similar to > http://mxr.mozilla.org/mozilla-central/source/content/base/src/ > nsMixedContentBlocker.cpp#610). In the case of aContentLocation && > aContentType == TYPE_OBJECT, what is the right behavior? Call shouldLoad() > or allow the load? As explained in the tests for this patch, for java-plugins shouldLoad is not triggered, only shouldProcess, hence shouldProcess should call ShouldLoad for loads of TYPE_OBJECT internally.

(In reply to John Schoenick [:johns] from comment #34) > If this test is supposed to cover both java and non-java plugins, it should > contain some non-java plugins, since java plugins invoke a rather special > load path (which setting plugin.java.mime to x-java-test is triggering for > those tags) > > sorry to keep adding to the cases here, but object tag load logic is scary: More test cases are always welcome. I think we can not solve everything at once in this patch. I suggest we incorporate the suggested tests from John and then land this as is, since this patch IMHO increases CSP coverage for java plugins - which is great. We should then revisit shouldLoad in general once we are done with Bug 1006868 - Revamp Gecko Security Hooks. I will go ahead and incorporate the suggested tests and suggestions from the previous comments and land the patches for this bug.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #36) > (In reply to Tanvi Vyas [:tanvi] from comment #35) > > > { > > >- if (!aContentLocation) > > >+ if (!aContentLocation) { > > > return NS_ERROR_FAILURE; > > >+ } > > > > If there is no content location, this might be an object load with no URI or > > channel. To account for these cases, you need to check if(!aContentLocation > > && aContentType != TYPE_OBJECT) (similar to > > http://mxr.mozilla.org/mozilla-central/source/content/base/src/ > > nsMixedContentBlocker.cpp#610). In the case of aContentLocation && > > aContentType == TYPE_OBJECT, what is the right behavior? Call shouldLoad() > > or allow the load? > > As explained in the tests for this patch, for java-plugins shouldLoad is not > triggered, only shouldProcess, hence shouldProcess should call ShouldLoad > for loads of TYPE_OBJECT internally. But you may have an object with no aContentLocation. In which case, you will end up returning NS_ERROR_FAILURE instead of calling ShouldLoad() within ShouldProcess() or just returning ACCEPT.

(In reply to Tanvi Vyas [:tanvi] from comment #38) > But you may have an object with no aContentLocation. In which case, you > will end up returning NS_ERROR_FAILURE instead of calling ShouldLoad() > within ShouldProcess() or just returning ACCEPT. The current code returns NS_ERROR_FAILURE in case the is no aContentLocation in shouldProcess [1]. I don't want to change this behavior for this patch. In general, I agree, we should find out why shouldProcess does slightly different things in case there is no aConentLocation for CSP [1] and MCB [2]. ShouldProcess should behave the same for both cases. [1] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPService.cpp#223 [2] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp#616

https://hg.mozilla.org/integration/mozilla-inbound/rev/e81f1e1c9c9f https://hg.mozilla.org/integration/mozilla-inbound/rev/7481265f0325

sorry had to back this out for test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2953591&repo=mozilla-inbound

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #39) > (In reply to Tanvi Vyas [:tanvi] from comment #38) > > But you may have an object with no aContentLocation. In which case, you > > will end up returning NS_ERROR_FAILURE instead of calling ShouldLoad() > > within ShouldProcess() or just returning ACCEPT. > > The current code returns NS_ERROR_FAILURE in case the is no aContentLocation > in shouldProcess [1]. I don't want to change this behavior for this patch. > In general, I agree, we should find out why shouldProcess does slightly > different things in case there is no aConentLocation for CSP [1] and MCB > [2]. ShouldProcess should behave the same for both cases. > > [1] > http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPService. > cpp#223 > [2] > http://mxr.mozilla.org/mozilla-central/source/content/base/src/ > nsMixedContentBlocker.cpp#616 Please file a followup bug for the no ContentLocation and type object case.

This one should have landed a while ago! Unbitrotted the attached patches, we need to push to try though, to figure out why those tests are failing on Android! (try is closed at the moment though).

Unbitrotted the test patch. Carrying over r+ for both patches from sstamm and john.

Ethan, we are triaging sec bugs at the moment, are you still on it to get it fixed? Or can you find someone to get it fixed?

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #45) > Ethan, we are triaging sec bugs at the moment, are you still on it to get it > fixed? Or can you find someone to get it fixed? Yes I am coming back to work on CSP bugs. I will work on this one ASAP!

Unbitrotted the patch again. I will run it on TreeHerder later.

I am going to unbitrot the test patch and just found all CSP tests are moved to dom/security/test/csp (by Bug 1117650 - Consider moving all dom-security related tests into dom/security/test). I think the test files (test_csp_shouldprocess.html) of this bug should also be located under dom/security/test/csp. Chris, am I right? If so, I have another question. I also found the file names of lots of test files were renamed/changed after moving to the new location. Should I rename files in this patch by following any rules?

Another question. In the file "test_csp_shouldprocess.html" of the test patch, there are codes such as: // blocked by shouldProcess "http://mochi.test:8888/tests/dom/base/test/csp/test1", "http://mochi.test:8888/tests/dom/base/test/csp/test2", ... Do these files (test1, test2, ...) exist in the repository before? Since the directory dom/base/test/csp/ doesn't exist now, I don't know what these files look like.

Hey Ethan, so here is what we would have to do: 1) We have to move the test files in dom/security/test/csp 2) rename file_csp_shouldprocess.html to file_shouldprocess.html also rename test_csp_shouldprocess.html to test_shouldprocess.html and also update all paths to match dom/security/test/csp 3) adopt the paths of http://mochi.test:8888/tests/dom/base/test/csp/test1 to match dom/security/test/csp and also add a comment that those files don't actually exist. Since loading of them should be blocked by shouldProcess we don't really need those files (but please adopt the paths) 4) Let me have a quick look once you are ready by setting the f? flag Thanks for looking into that problem.

Have to rebase the patch again due to conflicts.

Unbitrotted the test patch according to comment 51. However, some errors happen and have to be fixed.

Hi Chris, I am still working on the errors with the test patch. I see some error messages such as: JavaScript error: , line 0: TypeError: NetworkError when attempting to fetch resource. 672 INFO TEST-UNEXPECTED-FAIL | dom/security/test/csp/test_shouldprocess.html | ShouldLoad or ShouldProces s incorreclty blocks TYPE_OBJECT with uri: http://mochi.test/! It seems to me that the string "http://mochi.test:8888/tests/dom/security/test/csp/test1" is not successfully extracted by the code var asciiSpec = SpecialPowers.getPrivilegedProps(SpecialPowers. do_QueryInterface(subject, "nsIURI"), "asciiSpec"); I can see test7.class and test8.class are passed. But test1 to 6 failed. Do you have any idea about this?

Recently we changed contentPolicies to pass the internal contentType to shouldLoad()/shouldProcess(), hence we have to convert the internal type [1] to the external type before checking for TYPE_OBJECT. [1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsIContentPolicyBase.idl#216

Correct the patch according to comment 55.

Add some code comments.

Chris, as you talked to me offline, the patch does fail in mochitest on Android platform. INFO TEST-UNEXPECTED-FAIL | dom/security/test/csp/test_shouldprocess.html | Test timed out. https://treeherder.mozilla.org/#/jobs?repo=try&revision=83af77acc31d&selectedJob=15959906 https://treeherder.mozilla.org/logviewer.html#?job_id=15959906&repo=try I'll try to investigate on how to fix this issue. In the meanwhile, if you can recall how did you resolve such problem, please offer me a clue.

Comment on attachment 8711811 [details] [diff] [review] bug_908933_check_external_contentpolicytype_in_shouldprocess.patch Review of attachment 8711811 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for providing this information. It works!

In the file logcat-emulator-5554.log (http://goo.gl/8bNn6l), there is an error message: GeckoConsole: [JavaScript Warning: "Content Security Policy: Couldn't parse invalid host 'http://mochi.test:8888'"] Chris, I remember you told me that you encountered the same issue on Android while using nsContentUtils::InternalContentPolicyTypeToExternal(). Is that correct? If so, how did you solve the problem?

(In reply to Ethan Tseng [:ethan] from comment #61) > In the file logcat-emulator-5554.log (http://goo.gl/8bNn6l), there is an > error message: > GeckoConsole: [JavaScript Warning: "Content Security Policy: Couldn't parse > invalid host 'http://mochi.test:8888'"] > > Chris, I remember you told me that you encountered the same issue on Android > while using nsContentUtils::InternalContentPolicyTypeToExternal(). Is that > correct? > If so, how did you solve the problem? Yeah, looking through the logs I can't really tell what's going on. Interesting is that we correclty test7 and test8, for which we call shouldLoad(). I think it's best if you log some debug information and run that test again on treeherder. In particular we need to find out why the parser can't parse 'http://mochi.test:8888' which seems like a totally valid URL to me. Further, I think it makes sense to add more debug information to shouldProcess, in particular interesting is: URL, ContentPolicyType, potentially the CSP object. Thanks for looking into this!

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #62) > Yeah, looking through the logs I can't really tell what's going on. > Interesting is that we correclty test7 and test8, for which we call > shouldLoad(). I think it's best if you log some debug information and run > that test again on treeherder. In particular we need to find out why the > parser can't parse 'http://mochi.test:8888' which seems like a totally valid > URL to me. > Further, I think it makes sense to add more debug information to > shouldProcess, in particular interesting is: URL, ContentPolicyType, > potentially the CSP object. Okay, I'll work toward this way. Thanks for your guidance.

Add debug message in CSPService::ShouldProcess().

1. Remove single-quotes in the value of "default-src" directive. 2. Skip this mochitest on Fennec platform (will be explained in the following comment).

Q: Why we skip the test on Fennec (Android) platform? A: CSPService::ShouldProcess() is never called on that platform. = Detailed explanation = The call stacks of CSPService::ShouldProcess() are as follows: frame #0: CSPService::ShouldProcess() frame #1: nsContentPolicy::CheckPolicy() frame #2: nsContentPolicy::ShouldProcess() frame #3: NS_CheckContentProcessPolicy() frame #4: nsObjectLoadingContent::CheckProcessPolicy() frame #5: nsObjectLoadingContent::LoadObject()TYPE_NULL frame #6: HTMLSharedObjectElement::DoneAddingChildren() ... (more and not in our concern) The key point here is in frame #5. nsObjectLoadingContent::LoadObject() calls its internal function UpdateObjectParameters(). It calls another function GetTypeOfContent(), which returns an ObjectType value corresponding to the type of content we would support the given MIME type as, taking capabilities and plugin state into account. The test_shouldprocess.html uses the MIME type "application/x-java-test". On most platforms, GetTypeOfContent() returns eType_Plugin for this MIME type; however, on Fennec platform, it returns eType_Null. Then nsObjectLoadingContent::LoadObject() does not check process policy at all. This was proved by the log in treeherder result in comment 67 (https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ff30e2e710b). 02-15 05:02:25.416 1635 1646 I Gecko : UpdateObjectParameters: newMime = application/x-java-test 02-15 05:02:25.426 1635 1646 I Gecko : UpdateObjectParameters: GetTypeofContent = 4

Chris, according to my comment 71, I think it's reasonable to skip mochitest of this bug on fennec platform. If you agree it, could you please review my patches? BTW, I am not sure whether it is a bug that fennec doesn't support Java applet plugin. Do you know any person that I can consult with?

(In reply to Ethan Tseng [:ethan] from comment #72) > Chris, according to my comment 71, I think it's reasonable to skip mochitest > of this bug on fennec platform. If you agree it, could you please review my > patches? > > BTW, I am not sure whether it is a bug that fennec doesn't support Java > applet plugin. Do you know any person that I can consult with? This sounds like the java test plugin isn't loaded, so the plugin system doesn't recognize the x-java-test MIME as a plugin, and tests using it won't work. There's nothing disabling this code or preventing java from running on android, so this is probably a bug (the right fix might be having ObjectLoadingContent refuse to ever load java on android so we don't need to worry about it)

(In reply to John Schoenick [:johns] from comment #73) > (In reply to Ethan Tseng [:ethan] from comment #72) > > Chris, according to my comment 71, I think it's reasonable to skip mochitest > > of this bug on fennec platform. If you agree it, could you please review my > > patches? > > > > BTW, I am not sure whether it is a bug that fennec doesn't support Java > > applet plugin. Do you know any person that I can consult with? > > This sounds like the java test plugin isn't loaded, so the plugin system > doesn't recognize the x-java-test MIME as a plugin, and tests using it won't > work. There's nothing disabling this code or preventing java from running on > android, so this is probably a bug (the right fix might be having > ObjectLoadingContent refuse to ever load java on android so we don't need to > worry about it) John, thanks for the info, could you propose a solution to the problem and guide us to the right code?

I'm about 99% positive that FxAndroid hardcodes only supporting Flash, and if that's not the case it should be.

The previous did not effectively disable test_shouldprocess.html on fennec. Fix a typo in mochitest.ini.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #75) > I'm about 99% positive that FxAndroid hardcodes only supporting Flash, and > if that's not the case it should be. I think you are right. I looked up some references, and they also say that Android platform doesn't support Java applet plugin. (For example: http://www.technipages.com/can-java-applets-run-on-android) (In reply to John Schoenick [:johns] from comment #73) > This sounds like the java test plugin isn't loaded, so the plugin system > doesn't recognize the x-java-test MIME as a plugin, and tests using it won't > work. There's nothing disabling this code or preventing java from running on > android, so this is probably a bug (the right fix might be having > ObjectLoadingContent refuse to ever load java on android so we don't need to > worry about it) Actually I am not sure if ObjectLoadingContent() tries to load Java applet on Android or not. I guess not because the ObjectType is determined as eType_Null. If so, it should not be a bug. We just need to avoid running this test case on Android. Does this make sense?

Comment on attachment 8720171 [details] [diff] [review] Part1 - CSP: Call ShouldLoad inside ShouldProcess for TYPE_OBJECT Review of attachment 8720171 [details] [diff] [review]: ----------------------------------------------------------------- r=me

Comment on attachment 8721217 [details] [diff] [review] Part2 - CSP tests: ShouldProcess should block TYPE_OBJECT. Review of attachment 8721217 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/security/test/csp/mochitest.ini @@ +224,5 @@ > [test_child-src_worker.html] > skip-if = buildapp == 'b2g' #investigate in bug 1222904 > +[test_shouldprocess.html] > +# Fennec platform does not support Java applet plugin > +skip-if = toolkit == 'android' That's the only thing that changed in the patch, right? r=me

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #80) > That's the only thing that changed in the patch, right? r=me Yes! I verified that this patch effectively disables test_shouldprocess.html on Fennec (comment 77). Thanks for your review Chris!

Refresh comment "r=ckerschb". Carrying r+ flag.

Refresh comment "r=ckerschb". Carrying r+ flag.

A follow-up bug 1250814 was filed to track the concern mentioned in comment 73.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf293d7d7ac6 https://hg.mozilla.org/integration/mozilla-inbound/rev/41cb765011b6

https://hg.mozilla.org/mozilla-central/rev/bf293d7d7ac6 https://hg.mozilla.org/mozilla-central/rev/41cb765011b6

Clearing out my queue - since this bug already landed I am removing the ni? request.