The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in Firefox 23

Status

()

Core
Plug-ins
VERIFIED FIXED
5 years ago
2 years ago

People

(Reporter: johns, Assigned: johns)

Tracking

({sec-high})

Trunk
mozilla25
sec-high
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox16- wontfix, firefox17+ wontfix, firefox18+ wontfix, firefox19 wontfix, firefox20 wontfix, firefox21+ wontfix, firefox22+ wontfix, firefox23+ verified, firefox24+ verified, firefox25 verified, firefox-esr10 wontfix, firefox-esr1723+ verified, b2g18 unaffected, b2g-v1.1hd unaffected)

Details

(Whiteboard: [sg:high][adv-main23+][adv-esr1708+])

Attachments

(5 attachments, 5 obsolete attachments)

11.21 KB, patch
bsmedberg
: review+
abillings
: sec-approval+
johns
: checkin+
Details | Diff | Splinter Review
5.96 KB, patch
bsmedberg
: review+
Details | Diff | Splinter Review
9.63 KB, patch
johns
: review+
abillings
: sec-approval+
johns
: checkin+
Details | Diff | Splinter Review
1.95 KB, application/zip
Details
7.06 KB, patch
johns
: review+
johns
: checkin+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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>
Whiteboard: [sg:high]
(Assignee)

Updated

5 years ago
Depends on: 745030
Keywords: sec-high
(Assignee)

Updated

5 years ago
Blocks: 406541
No longer depends on: 406541
Probably not reasonable to fix and uplift to Beta (16) at this point, but hopefully we can figure out a fix appropriate for 17.
status-firefox16: --- → affected
status-firefox17: --- → affected
status-firefox18: --- → affected
tracking-firefox16: --- → -
tracking-firefox17: --- → +
tracking-firefox18: --- → +
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?
(Assignee)

Comment 3

5 years ago
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.
Status: NEW → ASSIGNED
Depends on: 783059
(Assignee)

Updated

5 years ago
status-firefox-esr10: --- → wontfix
status-firefox16: affected → wontfix

Comment 4

5 years ago
(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?
I've emailed John about this.
Now that beta 5 has gone to build we're past taking speculative fixes. Wontfixing for 17.
status-firefox17: affected → wontfix

Updated

4 years ago
status-firefox18: affected → wontfix
Is this still a current sec-high issue?
John, what is the current status of this?  This is the oldest open DOM sec-high or sec-critical goal. :P
Flags: needinfo?(jschoenick)

Updated

4 years ago
Component: Security → Plug-ins
(Assignee)

Comment 9

4 years ago
Created attachment 731337 [details] [diff] [review]
Construct codebase URI for Java plugins as java would
Flags: needinfo?(jschoenick)
(Assignee)

Comment 10

4 years ago
Created attachment 731338 [details] [diff] [review]
Only pass canonicalized codebase to Java
(Assignee)

Comment 11

4 years ago
Created attachment 731339 [details] [diff] [review]
[Branches only, foldme] Avoid changing nsIObjectLoadingContent IID on branches
(Assignee)

Comment 12

4 years ago
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.
Attachment #731337 - Flags: review?(benjamin)
(Assignee)

Comment 13

4 years ago
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.
Attachment #731338 - Flags: review?(benjamin)
(Assignee)

Comment 14

4 years ago
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.
Attachment #731339 - Flags: review?(benjamin)
(Assignee)

Comment 15

4 years ago
This was previously tracked (and not fixed...), re-noming now that we have patches.
status-b2g18: --- → unaffected
status-firefox19: --- → wontfix
status-firefox20: --- → affected
status-firefox21: --- → affected
status-firefox22: --- → affected
status-firefox-esr17: --- → affected
tracking-firefox20: --- → ?
tracking-firefox21: --- → ?
tracking-firefox22: --- → ?
tracking-firefox-esr17: --- → ?
Too late for 20 at this point.
status-firefox20: affected → wontfix
tracking-firefox20: ? → ---
(Assignee)

Comment 17

4 years ago
Created attachment 731956 [details] [diff] [review]
[Branches only, foldme] Avoid changing nsIObjectLoadingContent IID on branches

Inadvertently attached old version of this patch that doesn't compile...
Attachment #731339 - Attachment is obsolete: true
Attachment #731339 - Flags: review?(benjamin)
Attachment #731956 - Flags: review?(benjamin)
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.
tracking-firefox21: ? → +
tracking-firefox22: ? → +
tracking-firefox-esr17: ? → ---
Keywords: qawanted, verifyme
QA Contact: mwobensmith
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 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.
Attachment #731337 - Flags: superreview?(jst)
Attachment #731337 - Flags: review?(benjamin)
Attachment #731337 - Flags: review+
Comment on attachment 731338 [details] [diff] [review]
Only pass canonicalized codebase to Java

How do we test this?
Attachment #731338 - Flags: superreview?(jst)
Attachment #731338 - Flags: review?(benjamin)
Attachment #731338 - Flags: review+
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.
Attachment #731956 - Flags: review?(benjamin) → review+
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
Attachment #731337 - Flags: superreview?(jst) → superreview+
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
Attachment #731338 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 25

4 years ago
(In reply to Johnny Stenback (:jst, jst@mozilla.com) 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, jst@mozilla.com) 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.
(In reply to John Schoenick [:johns] from comment #25)
> (In reply to Johnny Stenback (:jst, jst@mozilla.com) 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, jst@mozilla.com) 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!
Dropping qawanted but leaving verifyme. Matt is assigned to verify this once it lands.
Keywords: qawanted
(Assignee)

Comment 28

4 years ago
Created attachment 740385 [details] [diff] [review]
Only pass canonicalized codebase to Java. r=bsmedberg sr=jst

Fix possible off-by-one, carrying over r
Attachment #731338 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #740385 - Flags: superreview+
Attachment #740385 - Flags: review+
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 .
status-firefox21: affected → wontfix
Hello all - any movement here?
Flags: needinfo?(jschoenick)
(Assignee)

Comment 31

4 years ago
Created attachment 755508 [details] [diff] [review]
Add nptest java plugin
Flags: needinfo?(jschoenick)
(Assignee)

Comment 32

4 years ago
Created attachment 755510 [details] [diff] [review]
Test
(Assignee)

Comment 33

4 years ago
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.
Attachment #755508 - Flags: review?(benjamin)
(Assignee)

Comment 34

4 years ago
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.
Attachment #755510 - Flags: review?(benjamin)
(Assignee)

Comment 35

4 years ago
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.
Attachment #731337 - Flags: sec-approval?
(Assignee)

Comment 36

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

[Security approval request comment]
(See above)
Attachment #740385 - Flags: sec-approval?
(Assignee)

Comment 37

4 years ago
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.
Attachment #731956 - Flags: sec-approval?
(Assignee)

Updated

4 years ago
status-firefox23: --- → affected
status-firefox24: --- → affected
tracking-firefox23: --- → ?
tracking-firefox24: --- → ?
tracking-firefox-esr17: --- → ?
(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.
status-firefox22: affected → wontfix
tracking-firefox23: ? → +
tracking-firefox24: ? → +
tracking-firefox-esr17: ? → 23+
Whiteboard: [sg:high] → [sg:high][land once m-c is FF25]
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.
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.
(Assignee)

Comment 41

4 years ago
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 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.
Attachment #731337 - Flags: sec-approval? → sec-approval+
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.
Attachment #740385 - Flags: sec-approval? → sec-approval+
Whiteboard: [sg:high][land once m-c is FF25] → [sg:high][checkin after 6/25]
Attachment #731956 - Flags: sec-approval?
Attachment #755508 - Flags: review?(benjamin) → review+
Comment on attachment 755510 [details] [diff] [review]
Test

The printf in nptest.cpp seems like a testing leftover? r=me with that gone
Attachment #755510 - Flags: review?(benjamin) → review+
(Assignee)

Comment 45

4 years ago
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
Attachment #731337 - Flags: checkin+
(Assignee)

Comment 46

4 years ago
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
Attachment #740385 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/de553526d1f8
https://hg.mozilla.org/mozilla-central/rev/5d38d2a6e4c1
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
(Assignee)

Updated

4 years ago
Depends on: 888834
(Assignee)

Comment 48

4 years ago
Created attachment 769872 [details] [diff] [review]
Test. r=bsmedberg

Folds in a few more cases for the regression in bug 888834, carrying over r+
Attachment #755510 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #769872 - Flags: review+
(Assignee)

Updated

4 years ago
Flags: in-testsuite?
Whiteboard: [sg:high][checkin after 6/25] → [sg:high]
(Assignee)

Comment 49

4 years ago
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
Attachment #731337 - Flags: approval-mozilla-esr17?
Attachment #731337 - Flags: approval-mozilla-beta?
Attachment #731337 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 50

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

(see comment 49)
Attachment #740385 - Flags: approval-mozilla-esr17?
Attachment #740385 - Flags: approval-mozilla-beta?
Attachment #740385 - Flags: approval-mozilla-aurora?
Comment on attachment 731337 [details] [diff] [review]
Construct codebase URI for Java plugins as java would

after 6/25 so let's uplift.
Attachment #731337 - Flags: approval-mozilla-esr17?
Attachment #731337 - Flags: approval-mozilla-esr17+
Attachment #731337 - Flags: approval-mozilla-beta?
Attachment #731337 - Flags: approval-mozilla-beta+
Attachment #731337 - Flags: approval-mozilla-aurora?
Attachment #731337 - Flags: approval-mozilla-aurora+
Attachment #740385 - Flags: approval-mozilla-esr17?
Attachment #740385 - Flags: approval-mozilla-esr17+
Attachment #740385 - Flags: approval-mozilla-beta?
Attachment #740385 - Flags: approval-mozilla-beta+
Attachment #740385 - Flags: approval-mozilla-aurora?
Attachment #740385 - Flags: approval-mozilla-aurora+
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?!?!).
status-b2g-v1.1hd: --- → unaffected
status-firefox24: affected → fixed
status-firefox25: --- → fixed
Flags: needinfo?(jschoenick)
Keywords: branch-patch-needed
(Assignee)

Comment 53

4 years ago
(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
status-firefox23: affected → fixed
Flags: needinfo?(jschoenick)
(Assignee)

Comment 54

4 years ago
https://hg.mozilla.org/releases/mozilla-esr17/rev/331fa2b73ddf
https://hg.mozilla.org/releases/mozilla-esr17/rev/6e57263710ec
status-firefox-esr17: affected → fixed
(Assignee)

Comment 55

4 years ago
Created attachment 772768 [details]
Test case demonstrating this issue
Keywords: branch-patch-needed
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
Status: RESOLVED → VERIFIED
status-firefox23: fixed → verified
status-firefox24: fixed → verified
status-firefox25: fixed → verified
status-firefox-esr17: fixed → verified
Keywords: verifyme
Whiteboard: [sg:high] → [sg:high][adv-main23+][adv-esr1708+]
Duplicate of this bug: 964055
The blog post linked in bug 964055 doesn't talk about Java plugins at all.
(Assignee)

Comment 59

3 years ago
Created attachment 8379263 [details] [diff] [review]
Test. r=bsmedberg

Un-bitrotted, rebased on java test plugin which was split out and added in bug 971279
Attachment #755508 - Attachment is obsolete: true
Attachment #769872 - Attachment is obsolete: true
Attachment #8379263 - Flags: review+
(Assignee)

Comment 60

3 years ago
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
Attachment #8379263 - Flags: checkin+
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

Updated

3 years ago
Flags: needinfo?(jschoenick)
(Assignee)

Comment 62

3 years ago
(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
Flags: needinfo?(jschoenick)
https://hg.mozilla.org/mozilla-central/rev/19a4abd04dce
status-firefox27: --- → ?
status-firefox28: --- → ?
status-firefox29: --- → ?
status-firefox30: --- → fixed
(Assignee)

Comment 64

3 years ago
This was fixed in 25, the additional landing here only applies to the mochitest
status-firefox27: ? → ---
status-firefox28: ? → ---
status-firefox29: ? → ---
status-firefox30: fixed → ---
Flags: in-testsuite? → in-testsuite+
Group: core-security
You need to log in before you can comment on or make changes to this bug.