738396 - Java applets may read arbitrary files on a user's system

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[John Schoenick [:johns]]

Investigating 736965 which is fallout from a related security issue 406541, I discovered the fixes in 406541 are woefully inadequate. There are numerous ways to invoke java plugins which bypass various security checks. I found at least three different issues in the loading code. All of the below issues apply to 3.6+ and probably older.

The most severe of these is <param> elements can override the <object> tag's codebase as seen by java. That means this will load from a non-file: context

>  <applet width=500 height=500 codebase="." code="home.user.some.folder.object.class">
>    <param name="codebase" value="file:///">
>  </applet>

The security check will assume codebase "." is valid, but java will use the file:/// codebase, allowing read access to the full filesystem. There is no prompt when this applet loads.

For this to be successful, you must know the location of your applet on the user's machine - either through guessing a cache path, tricking the user into downloading it before hand, or possibly using something like flash to create a file with a more deterministic name.

Other issues in this code:
 - <object> tags default codebase to the current directory, rather than domain-root, unlike <applet> tags. We need to emulate this quirk to avoid breakage like 736965
 - <param> tags appear to be passed as raw html to java, leading to potential disagreements between parsers on malformed HTML.
 - Some code checks for application/x-java-vm to trigger java quirks, but it is possible with <embed> to create a an applet with e.g. application/x-java-applet. The code that forces the MIME to x-java-vm only applies to <object> tags.
- nodePrinicpal()->GetURI() is returning NULL on object tags in one situation with <object> tags, causing security checks to be skipped.
- (probably non-issue) according to oracle docs, <applet> should follow HTML4 specs, which state "Values for [codebase] may only refer to subdirectories of the directory containing the current document". This is never enforced (except on file:/// URIs with the patch in 406541), and contradict's java's behavior of defaulting codebase to "/" for <applet>

Comment 1

[Daniel Veditz [:dveditz]]

Probably not reasonable to fix and uplift to Beta (16) at this point, but hopefully we can figure out a fix appropriate for 17.

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

John, we're a little over a week away from moving 17 to the Beta audience - any work done on this yet and if not can you give an idea of when you'll be working on it?

Comment 3

[John Schoenick [:johns]]

I've been waiting on landing more followups to this code until bug 783059 lands, as it's especially regression-prone (see original landing of bug 406541). I will try to have something ready before the uplift.

Comment 4

[Alex Keybl [:akeybl]]

(In reply to John Schoenick [:johns] from comment #3)
> I've been waiting on landing more followups to this code until bug 783059
> lands, as it's especially regression-prone (see original landing of bug
> 406541). I will try to have something ready before the uplift.

Any updates here John? Is there something low risk we can do to resolve this issue in the FF17 timeframe?

Comment 5

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

I've emailed John about this.

Comment 6

[Lukas Blakk [:lsblakk] use ?needinfo]

Now that beta 5 has gone to build we're past taking speculative fixes. Wontfixing for 17.

Comment 7

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

Is this still a current sec-high issue?

Comment 8

[Andrew McCreight [:mccr8]]

John, what is the current status of this?  This is the oldest open DOM sec-high or sec-critical goal. :P

Comment 9

[John Schoenick [:johns]]

Attachment: Construct codebase URI for Java plugins as java would

Comment 10

[John Schoenick [:johns]]

Attachment: Only pass canonicalized codebase to Java

Comment 11

[John Schoenick [:johns]]

Attachment: [Branches only, foldme] Avoid changing nsIObjectLoadingContent IID on branches

Comment 12

[John Schoenick [:johns]]

Comment on attachment 731337 [details] [diff] [review]
Construct codebase URI for Java plugins as java would

So this got hairy fast, but I think should be decently-safe in that most of the changes reside within an |if (isJava)| block, so worst-case scenario is we break java...

The problem is multi-fold:
- Java uses "codebase" as a special value for where it is allowed to run, e.g. |file:///|, and assumes the browser will security-check it (despite not parsing it the way any browser would...)
- Java honors the last "codebase" value it sees, either on in tag attributes *or* in <param> children
- nsPluginInstanceOwner has a good deal of quirks relating to how it passes parameters and attributes to plugins
- nsObjectLoadingContent just looks at the attribute, not params, and without any of these quirks.
- Java doesn't parse URIs like the rest of the universe. E.g. no-codebase means ".", codebase="" means "/", and codebase="file:" means "file:///"

I opened Bug 853995 to move building the parameters we pass to the plugin to content (instead of having nsPluginInstanceOwner poke at the DOM), but that is non-trivial and wouldn't be suitable for branches. Instead, this patch basically copy-pastes a chunk of nsPluginInstanceOwner logic into nsObjectLoadingContent so they can arrive at the same codebase, with a patch for bug 853995 in progress to remove the code duplication from trunk.

Another issue is that we now parse the URI differently if we end up with Java, but can look at the URI's file extension in some cases and change plugin type. For example |<embed src="file.class">| will build file.class as a normal URI, before seeing .class and switching to java. I sort-of-elegantly fixed this by having UpdateObjectParameters re-call itself with a aJavaURI parameter at this point to redo the URI building steps with |isJava = true|.

Finally, now that we made a reasonable attempt to build the same mBaseURI that java would use, I added a java-only CheckLoadURIWithPrincipal security check against mBaseURI.

So risk-wise this patch looks scary, but is largely known-good code from nsPluginInstanceOwner, and fallout would likely only affect java plugins.

Comment 13

[John Schoenick [:johns]]

Comment on attachment 731338 [details] [diff] [review]
Only pass canonicalized codebase to Java

This causes nsPluginInstanceOwner to get the parsed-and-canonicalized codebase from nsObjectLoadingContent, and pass only that as a "codebase" value in the case of java plugins.

This guarantees that other, undiscovered quirks in how java determines its codebase would simply result in us overriding the codebase to our interpretation, instead of security-checking a different codebase than java decides to use.

Comment 14

[John Schoenick [:johns]]

Comment on attachment 731339 [details] [diff] [review]
[Branches only, foldme] Avoid changing nsIObjectLoadingContent IID on branches

This would be folded against the canonicalize-codebase patch for branches only -- it moves the GetBaseURI() function to nsObjectLoadingContent.h and uses a nasty static-cast to get at it to avoid changing the nsIObjectLoadingContent IID on release branches.

Comment 15

[John Schoenick [:johns]]

This was previously tracked (and not fixed...), re-noming now that we have patches.

Comment 16

[Andrew McCreight [:mccr8]]

Too late for 20 at this point.

Comment 17

[John Schoenick [:johns]]

Attachment: [Branches only, foldme] Avoid changing nsIObjectLoadingContent IID on branches

Inadvertently attached old version of this patch that doesn't compile...

Comment 18

[Alex Keybl [:akeybl]]

We'll track for the ESR once this is resolved. Since this is a sec-high and appears to carry some plugin risk, we'll want to land ASAP on branches and follow up with QA regression testing. Any guidance there would be very helpful.

If this isn't ready to land in time for our second or third beta, we should strongly consider sec-approving next cycle instead.

Comment 19

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

QA is ready to test when this gets landed. Basic concept is well-understood. Let me know if there are any other similar things to look for (param element vs object params, other types of paths, etc.).

Comment 20

[Benjamin Smedberg]

Comment on attachment 731337 [details] [diff] [review]
Construct codebase URI for Java plugins as java would

I'd like a second review on these patches; tagging jst though he might redirect.

Comment 21

[Benjamin Smedberg]

Comment on attachment 731338 [details] [diff] [review]
Only pass canonicalized codebase to Java

How do we test this?

Comment 22

[Benjamin Smedberg]

Comment on attachment 731956 [details] [diff] [review]
[Branches only, foldme] Avoid changing nsIObjectLoadingContent IID on branches

Kinda hacky, but I think we're safe here since web content can't spoof nsIObjectLoadingContent.

Comment 23

[Johnny Stenback (:jst)]

Comment on attachment 731337 [details] [diff] [review]
Construct codebase URI for Java plugins as java would

- In nsObjectLoadingContent::UpdateObjectParameters():

+    mydomElement->GetElementsByTagNameNS(xhtml_ns, NS_LITERAL_STRING("param"),
+                                         getter_AddRefs(allParams));
+    if (allParams) {
+      uint32_t numAllParams;
+      allParams->GetLength(&numAllParams);
+      for (uint32_t i = 0; i < numAllParams; i++) {
+        nsCOMPtr<nsIDOMNode> pnode;
+        allParams->Item(i, getter_AddRefs(pnode));
+        nsCOMPtr<nsIDOMElement> domelement = do_QueryInterface(pnode);
+        if (domelement) {
+          nsAutoString name;
+          domelement->GetAttribute(NS_LITERAL_STRING("name"), name);
+          name.Trim(" \n\r\t\b", true, true, false);
+          if (name.EqualsIgnoreCase("codebase")) {
+            // Find the first plugin element parent
+            nsCOMPtr<nsIDOMNode> parent;
+            nsCOMPtr<nsIDOMHTMLObjectElement> domobject;
+            nsCOMPtr<nsIDOMHTMLAppletElement> domapplet;
+            pnode->GetParentNode(getter_AddRefs(parent));
+            while (!(domobject || domapplet) && parent) {
+              domobject = do_QueryInterface(parent);
+              domapplet = do_QueryInterface(parent);
+              nsCOMPtr<nsIDOMNode> temp;
+              parent->GetParentNode(getter_AddRefs(temp));
+              parent = temp;
+            }

You could do all this with far less reference counting and QI calls if you used NS_GetContentList() and the internal interfaces, but I'm not convinced it's worth rewriting this for that alone.

+    codebaseStr.AssignLiteral("/");
+    // XXX(johns): This doesn't cover the case of "https:" which java would
+    //             interpret as "https:///" but weinterpret as this document's

Insert the missing space in weinterpret.

sr=jst

Comment 24

[Johnny Stenback (:jst)]

Comment on attachment 731338 [details] [diff] [review]
Only pass canonicalized codebase to Java

- In nsPluginInstanceOwner::EnsureCachedAttrParamArrays():

+  // (Bug XXXXXX FIXME) java has quirks in its codebase parsing, pass the
+  // absolute codebase URI as content sees it.

Replace XXXXXX with a bug number there?

+    mCachedAttrParamNames [nextAttrParamIndex] = ToNewUTF8String(NS_LITERAL_STRING("codebase"));
+    mCachedAttrParamValues[nextAttrParamIndex] = ToNewUTF8String(NS_ConvertUTF8toUTF16(codebaseStr));

Wow, that's a pretty wonky string copy pattern we've got going here. Maybe file a followup bug to replace the above patterns with strdup() in this whole file?

sr=jst

Comment 25

[John Schoenick [:johns]]

(In reply to Johnny Stenback (:jst, [e-mail]) from comment #23)
> You could do all this with far less reference counting and QI calls if you
> used NS_GetContentList() and the internal interfaces, but I'm not convinced
> it's worth rewriting this for that alone.

(In reply to Johnny Stenback (:jst, [e-mail]) from comment #24)
> Wow, that's a pretty wonky string copy pattern we've got going here. Maybe
> file a followup bug to replace the above patterns with strdup() in this
> whole file?

This is all just copy-pasted nsPluginInstanceOwner code -- I opened bug 853995 to clean this up and de-duplicate it.

Comment 26

[Johnny Stenback (:jst)]

(In reply to John Schoenick [:johns] from comment #25)
> (In reply to Johnny Stenback (:jst, [e-mail]) from comment #23)
> > You could do all this with far less reference counting and QI calls if you
> > used NS_GetContentList() and the internal interfaces, but I'm not convinced
> > it's worth rewriting this for that alone.
> 
> (In reply to Johnny Stenback (:jst, [e-mail]) from comment #24)
> > Wow, that's a pretty wonky string copy pattern we've got going here. Maybe
> > file a followup bug to replace the above patterns with strdup() in this
> > whole file?
> 
> This is all just copy-pasted nsPluginInstanceOwner code -- I opened bug
> 853995 to clean this up and de-duplicate it.

Yup, didn't mean to suggest that this was your doing, it's all over this file from way long ago (and you did the right thing in following the existing coding patterns here)! Thanks for filing the followup!

Comment 27

[u279076]

Dropping qawanted but leaving verifyme. Matt is assigned to verify this once it lands.

Comment 28

[John Schoenick [:johns]]

Attachment: Only pass canonicalized codebase to Java. r=bsmedberg sr=jst

Fix possible off-by-one, carrying over r

Comment 29

[bhavana bajaj [:bajaj]]

Given https://bugzilla.mozilla.org/show_bug.cgi?id=738396#c18 and considering we are just two beta's away for Fx21 wontfixng this for fx21 . This will be a better fit for next cycle .

Comment 30

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

Hello all - any movement here?

Comment 31

[John Schoenick [:johns]]

Attachment: Add nptest java plugin

Comment 32

[John Schoenick [:johns]]

Attachment: Test

Comment 33

[John Schoenick [:johns]]

Comment on attachment 755508 [details] [diff] [review]
Add nptest java plugin

Adds another target for nptest that claims x-java-test, and adds a getJavaCodebase function, so we can test that we're passing the intended codebase to java tags.

Comment 34

[John Schoenick [:johns]]

Comment on attachment 755510 [details] [diff] [review]
Test

Uses the new java test plugin target to ensure we see the expected codebase in various situations.

Comment 35

[John Schoenick [:johns]]

Comment on attachment 731337 [details] [diff] [review]
Construct codebase URI for Java plugins as java would

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Pretty clearly reveals we are mis-handling java codebase, if the linked bug is also a restricted bug, this would quickly give someone the information they need to figure out the issue.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The code is pretty clear that we didn't audit java's codebase previously.

Which older supported branches are affected by this flaw?
All current branches. Irrelevant to b2g as we don't support plugins there.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Yes. Some of the riskier changes are replaced with simplier workarounds for branches only (the foldme patches)

How likely is this patch to cause regressions; how much testing does it need?
Low-to-moderate chance of introducing regressions, which would likely only affect certain configurations of java embedding.

Comment 36

[John Schoenick [:johns]]

Comment on attachment 740385 [details] [diff] [review]
Only pass canonicalized codebase to Java. r=bsmedberg sr=jst

[Security approval request comment]
(See above)

Comment 37

[John Schoenick [:johns]]

Comment on attachment 731956 [details] [diff] [review]
[Branches only, foldme] Avoid changing nsIObjectLoadingContent IID on branches

[Security approval request comment]
See above. This just avoids changing the nsIObjectLoadingContent IID on branches.

Comment 38

[Alex Keybl [:akeybl]]

(In reply to John Schoenick [:johns] from comment #35)
> Do comments in the patch, the check-in comment, or tests included in the
> patch paint a bulls-eye on the security problem?
> The code is pretty clear that we didn't audit java's codebase previously.
>
> How likely is this patch to cause regressions; how much testing does it need?
> Low-to-moderate chance of introducing regressions, which would likely only
> affect certain configurations of java embedding.

Give this, I think we should be cautious and fix for the first time in 23/24/25 and the corresponding ESR, to get a full 6 weeks of testing before release.

Comment 39

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

The problem is

> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?
> Pretty clearly reveals we are mis-handling java codebase, if the linked bug
> is also a restricted bug, this would quickly give someone the information
> they need to figure out the issue.
> 
> Do comments in the patch, the check-in comment, or tests included in the
> patch paint a bulls-eye on the security problem?
> The code is pretty clear that we didn't audit java's codebase previously.

This means that once we check in, we're going to be pretty exposed for the whole six week cycle.

Comment 40

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

Given this bug's potential impact (this is less severe than remote code execution, and isn't necessarily easy to exploit), I think favoring extra bake time over reduced trunk exposure seems reasonable.

Comment 41

[John Schoenick [:johns]]

There is also bug 406541, which is a much less severe bug that depends on the changes here to fix. We could open a public version of that bug, then land these changes folded with 406541 as a "cleanup java codebase handling" patch. Without a locked bug to bring attention to it and the valid-premise for cleaning up this code, it's less obvious that there's a more severe issue to be found.

Comment 42

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

Comment on attachment 731337 [details] [diff] [review]
Construct codebase URI for Java plugins as java would

Approving for trunk AFTER trunk becomes Firefox 25 on 6/24 or 25.

Comment 43

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

Comment on attachment 740385 [details] [diff] [review]
Only pass canonicalized codebase to Java. r=bsmedberg sr=jst

Approving for trunk AFTER trunk becomes Firefox 25 on 6/24 or 25.

Comment 44

[Benjamin Smedberg]

Comment on attachment 755510 [details] [diff] [review]
Test

The printf in nptest.cpp seems like a testing leftover? r=me with that gone

Comment 45

[John Schoenick [:johns]]

Comment on attachment 731337 [details] [diff] [review]
Construct codebase URI for Java plugins as java would

https://hg.mozilla.org/integration/mozilla-inbound/rev/de553526d1f8

Comment 46

[John Schoenick [:johns]]

Comment on attachment 740385 [details] [diff] [review]
Only pass canonicalized codebase to Java. r=bsmedberg sr=jst

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d38d2a6e4c1

Comment 47

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/de553526d1f8
https://hg.mozilla.org/mozilla-central/rev/5d38d2a6e4c1

Comment 48

[John Schoenick [:johns]]

Attachment: Test. r=bsmedberg

Folds in a few more cases for the regression in bug 888834, carrying over r+

Comment 49

[John Schoenick [:johns]]

Comment on attachment 731337 [details] [diff] [review]
Construct codebase URI for Java plugins as java would

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Technically a java bug: It assumes the browser will security-check its plugin-specific parameters, but we do not.

User impact if declined:
sec-high. Potential for untrusted java applets to get read-only access to the user's filesystem if used in conjunction with some method to download a file to a guessable path.

Testing completed (on m-c, etc.):
Has test case, on m-c. One minor regression found in bug 888834, which is now fixed and would be folded into the uplift patch.

Risk to taking this patch (and alternatives if risky):
In relation to java, there is a moderate regression risk. Java does not construct its "codebase" attribute in a very sane fashion, mis-constructing URLs and assuming |codebase=""| means |codebase="/"| among other things. Because we now force agreement on the codebase parameter, any quirks in java we did not emulate properly would break some java applets (e.g. bug 888834). These would be trivial fixes, if found.

Outside of embedded java, the risk is low, as all new code only applies to Java.

String or IDL/UUID changes made by this patch:
None

Comment 50

[John Schoenick [:johns]]

Comment on attachment 740385 [details] [diff] [review]
Only pass canonicalized codebase to Java. r=bsmedberg sr=jst

(see comment 49)

Comment 51

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 731337 [details] [diff] [review]
Construct codebase URI for Java plugins as java would

after 6/25 so let's uplift.

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/819cefc60ade
https://hg.mozilla.org/releases/mozilla-aurora/rev/20a1cd5be4fb

There are some conflicts in nsObjectLoadingContent.cpp on esr17 that will need resolving. Also, this requires binary approval to land on beta due to the changes to nsIObjectLoadingContent.idl (comment 49 said no IDL changes?!?!).

Comment 53

[John Schoenick [:johns]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #52)
> Also, this requires binary approval to land on beta due to
> the changes to nsIObjectLoadingContent.idl (comment 49 said no IDL
> changes?!?!).

This is why the |Avoid changing IID on branches| patch exists. I should've clarified in the whiteboard, apologies.

https://hg.mozilla.org/releases/mozilla-beta/rev/24df0a207416
https://hg.mozilla.org/releases/mozilla-beta/rev/10544c91e1d6

Comment 54

[John Schoenick [:johns]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/331fa2b73ddf
https://hg.mozilla.org/releases/mozilla-esr17/rev/6e57263710ec

Comment 55

[John Schoenick [:johns]]

Attachment: Test case demonstrating this issue

Comment 56

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

QA has completed an extensive pass on this bug and signs off.

Confirmed original path loading exploit on OpenJDK6, Linux, FF22.
Verified fixed in FF25 m-c, 2013-07-08.
Verified fixed in FF24, 2013-07-10.
Verified fixed in FF23, 2013-07-10.
Verified fixed in FF17esr, 2013-07-10.

Verified that:
- Bad paths prevent JRE from loading applet at all, or
- Bad paths cause applet not to display, with error from JRE (security error or class not found).
- Normal use cases for applet loading - via object, embed and applet tags - work correctly.
- Other test cases for applet loading - nested tags, redirects, site vs root relative URLs, edge cases - work correctly.

JDKs used:
OpenJDK6u45/Ubuntu 13.0.4
OpenJDK7u21/Ubuntu 13.0.4
Oracle JDK 7u21/Ubuntu 13.0.4
Oracle JDK 7u25/Ubuntu 13.0.4
Oracle JDK 7u21/Mac OSX 10.8
Oracle JDK 7u25/Mac OSX 10.8

Comment 58

[Andrew McCreight [:mccr8]]

The blog post linked in bug 964055 doesn't talk about Java plugins at all.

Comment 59

[John Schoenick [:johns]]

Attachment: Test. r=bsmedberg

Un-bitrotted, rebased on java test plugin which was split out and added in bug 971279

Comment 60

[John Schoenick [:johns]]

Comment on attachment 8379263 [details] [diff] [review]
Test. r=bsmedberg

Pushed test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b24d1437c6f3

Try:
https://tbpl.mozilla.org/?tree=Try&rev=fd55a944e195

Comment 61

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I backed this (and everything else from that push to inbound) out in http://hg.mozilla.org/integration/mozilla-inbound/rev/128cbf1edc40 due to various Java/plugin related failures they caused:
Crashtest failure: https://tbpl.mozilla.org/php/getParsedLog.php?id=35003251&tree=Mozilla-Inbound
XPCShell failure: https://tbpl.mozilla.org/php/getParsedLog.php?id=35003355&tree=Mozilla-Inbound

Comment 62

[John Schoenick [:johns]]

(In reply to Wes Kocher (:KWierso) from comment #61)
> I backed this (and everything else from that push to inbound) out in
> http://hg.mozilla.org/integration/mozilla-inbound/rev/128cbf1edc40 due to
> various Java/plugin related failures they caused:
> Crashtest failure:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=35003251&tree=Mozilla-
> Inbound
> XPCShell failure:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=35003355&tree=Mozilla-
> Inbound

Attempt 2 to push test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19a4abd04dce

Extensive try run since this push burned the tree before:
https://tbpl.mozilla.org/?tree=Try&rev=b656bdd77ce1

Comment 63

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/19a4abd04dce

Comment 64

[John Schoenick [:johns]]

This was fixed in 25, the additional landing here only applies to the mochitest