Last Comment Bug 406541 - (CVE-2013-1717) local java applet may read arbitrary files under certain circumstances
(CVE-2013-1717)
: local java applet may read arbitrary files under certain circumstances
Status: RESOLVED FIXED
[adv-main23+]
: sec-moderate
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla25
Assigned To: John Schoenick [:johns]
:
Mentors:
Depends on: 731947 736965 738396 741758 744898 752746 902375
Blocks:
  Show dependency treegraph
 
Reported: 2007-12-03 01:43 PST by georgi - hopefully not receiving bugspam
Modified: 2014-11-19 20:03 PST (History)
23 users (show)
john: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
wontfix
+
wontfix
+
fixed
+
fixed
fixed
-
wontfix
unaffected
unaffected


Attachments
a1.java - compiled a1.class must be saved in /tmp/DumbUglyB1llMarriedDumbUglyB1tch (692 bytes, text/plain)
2007-12-03 01:43 PST, georgi - hopefully not receiving bugspam
no flags Details
a1.html - open it locally after placing the applet in correct dir (342 bytes, text/html)
2007-12-03 01:44 PST, georgi - hopefully not receiving bugspam
no flags Details
[1/3] Make GetObjectBaseURI xpcom callable, mimic java's empty-codebase quirk (7.21 KB, patch)
2012-02-02 16:15 PST, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[2/3] Check file:/// URIs for same-directory as a sort of same-origin check (3.33 KB, patch)
2012-02-02 16:16 PST, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[3/3] Canonicalize the codebase as we see it for plugins (3.61 KB, patch)
2012-02-02 16:17 PST, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[1/2] GetObjectBaseURI should mimic java's behavior (5.51 KB, patch)
2012-02-16 14:00 PST, John Schoenick [:johns]
jst: review+
Details | Diff | Splinter Review
[2/2] Enforce same-directory for plugin codebase on file URIs (3.62 KB, patch)
2012-02-16 14:02 PST, John Schoenick [:johns]
jst: review-
Details | Diff | Splinter Review
[1/4] GetObjectBaseURI should match java's empty-string quirk (5.50 KB, patch)
2012-02-23 16:38 PST, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[2/4] Make GetObjectBaseURI xpcomable (5.02 KB, patch)
2012-02-23 16:39 PST, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[3/4] Run same-directory check on file URIs (5.21 KB, patch)
2012-02-23 16:39 PST, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[4/4] Canonicalize the codebase pushed to java (4.45 KB, patch)
2012-02-23 16:43 PST, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[4/4] Canonicalize the codebase pushed to java (4.37 KB, patch)
2012-02-23 17:18 PST, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
[1/4] GetObjectBaseURI should match java's empty-string quirk (5.49 KB, patch)
2012-02-24 12:18 PST, John Schoenick [:johns]
jst: review+
Details | Diff | Splinter Review
[2/4] Make GetObjectBaseURI xpcomable (5.02 KB, patch)
2012-02-24 12:19 PST, John Schoenick [:johns]
jst: review+
Details | Diff | Splinter Review
[3/4] Run same-directory check on file URIs (4.98 KB, patch)
2012-02-24 12:19 PST, John Schoenick [:johns]
jst: review+
Details | Diff | Splinter Review
[4/4] Canonicalize the codebase pushed to java (4.37 KB, patch)
2012-02-24 12:20 PST, John Schoenick [:johns]
jst: review+
Details | Diff | Splinter Review
Ensure we agree with java on applet codebase, and run security checks for file: codebase URIs. r=jst (16.20 KB, patch)
2012-02-27 14:03 PST, John Schoenick [:johns]
jst: review+
Details | Diff | Splinter Review
Always return a codebase for plugins, even if URI creation fails (1.59 KB, patch)
2012-03-01 15:33 PST, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
Followup - Handle failed URI creation, fix typo (2.68 KB, patch)
2012-03-02 13:14 PST, John Schoenick [:johns]
bzbarsky: review+
Details | Diff | Splinter Review
[checkin-needed] Followup - Handle failed URI creation, fix typo. r=bzbarsky (2.67 KB, patch)
2012-03-02 14:15 PST, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
aurora backout of the fix + followup in this bug. (11.57 KB, patch)
2012-04-12 19:58 PDT, Johnny Stenback (:jst, jst@mozilla.com)
jst: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Backout of aurora (14) for regressions (16.43 KB, patch)
2012-06-01 11:26 PDT, John Schoenick [:johns]
jst: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Backout of 15 (16.29 KB, patch)
2012-06-19 17:30 PDT, John Schoenick [:johns]
jst: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Backout of 16 (16.29 KB, patch)
2012-06-19 17:31 PDT, John Schoenick [:johns]
jst: review+
Details | Diff | Splinter Review
Add an allowFileDirectories argument to nsPrincipal::CheckMayLoad (28.55 KB, patch)
2013-04-04 17:02 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
Add check for java file codebase security (4.14 KB, patch)
2013-04-04 17:03 PDT, John Schoenick [:johns]
benjamin: review+
Details | Diff | Splinter Review
[Fold into patch on branches] Move strict file origin checking blob into nsOBJLC (6.57 KB, patch)
2013-04-04 17:04 PDT, John Schoenick [:johns]
benjamin: review+
Details | Diff | Splinter Review
Test (3.95 KB, patch)
2013-05-29 11:32 PDT, John Schoenick [:johns]
benjamin: review+
Details | Diff | Splinter Review
Add NS_URIIsLocalFile and NS_CheckStrictFileOriginPolicy (4.22 KB, patch)
2013-06-04 11:00 PDT, John Schoenick [:johns]
bzbarsky: review+
Details | Diff | Splinter Review
Use NS_CheckStrictOriginPolicy in CheckMayLoad (5.66 KB, patch)
2013-06-04 11:02 PDT, John Schoenick [:johns]
bzbarsky: review-
Details | Diff | Splinter Review
Add check for java file codebase security. r=bsmedberg (4.72 KB, patch)
2013-06-04 11:15 PDT, John Schoenick [:johns]
john: review+
Details | Diff | Splinter Review
Split NS_CheckStrictFileOriginPolicy out of nsPrincipal::CheckMayLoad (10.08 KB, patch)
2013-06-04 15:09 PDT, John Schoenick [:johns]
bzbarsky: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑esr17-
abillings: sec‑approval+
john: checkin+
Details | Diff | Splinter Review
Add check for java file codebase security. r=bsmedberg (4.76 KB, patch)
2013-06-04 15:12 PDT, John Schoenick [:johns]
john: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
bajaj.bhavana: approval‑mozilla‑esr17-
abillings: sec‑approval+
john: checkin+
Details | Diff | Splinter Review
Test. r=bsmedberg (4.49 KB, patch)
2013-08-07 15:00 PDT, John Schoenick [:johns]
john: review+
Details | Diff | Splinter Review
Test. r=bsmedberg (4.80 KB, patch)
2014-02-20 14:32 PST, John Schoenick [:johns]
john: review+
john: checkin+
Details | Diff | Splinter Review

Description georgi - hopefully not receiving bugspam 2007-12-03 01:43:08 PST
Created attachment 291181 [details]
a1.java - compiled a1.class must be saved in /tmp/DumbUglyB1llMarriedDumbUglyB1tch

recent trunk has restrictions on what local html can access

in bug 402998 Comment #8 someone with sun.com email asked to "post a test" for local applet circumventing restrictions.

it is like beating a death horse, but here it is:

if the path of the locally saved applet is known at applet compile time, the applet can read any file.

note that if the luser saves files in a single directory, a two stage attack may be successful with high probability.

suppose the applet is saved in directory:
/tmp/DumbUglyB1llMarriedDumbUglyB1tch

it should be instantiated like this:
<applet codebase="file:///" 
code="tmp.DumbUglyB1llMarriedDumbUglyB1tch.a1">
</applet>

and the applet should contain:
/*
 * This is the path to the applet filename:
 * */
package tmp.DumbUglyB1llMarriedDumbUglyB1tch;
public class a1 extends Applet {
Comment 1 georgi - hopefully not receiving bugspam 2007-12-03 01:44:50 PST
Created attachment 291182 [details]
a1.html - open it locally after placing the applet in correct dir
Comment 2 Kenneth Russell 2007-12-03 15:25:15 PST
Agreed, this should not be possible and is a bug on the Java side. Appropriate people have been notified, and we will update this bug with progress.
Comment 3 georgi - hopefully not receiving bugspam 2007-12-04 05:28:28 PST
when i was a coder in the old dos days, we were instructed to call our application bugs "uncontrollable process in the operating systems" whenever possible ;)
Comment 4 georgi - hopefully not receiving bugspam 2008-01-31 23:09:56 PST
[sg:moderate] imo
Comment 5 Johnny Stenback (:jst, jst@mozilla.com) 2011-09-30 09:58:10 PDT
georgi, is this still a problem with an up to date java plugin?
Comment 6 Johnny Stenback (:jst, jst@mozilla.com) 2011-10-10 23:45:46 PDT
I just tested, and it still is a problem with the testcase as written. The reason being that the testcase sets a codebase of file:///, which means anything on the users filesystem is considered same origin. If I change the testcase to not set a codebase (and move the class file around so it's found) then I get a permission denied error.

I guess we could consider something along the lines of forbidding setting of the codebase to a file:// uri at all, which would force the codebase to be the location of the containing page. Not great, but better than what we have now.
Comment 7 John Schoenick [:johns] 2012-02-02 16:15:40 PST
Created attachment 594006 [details] [diff] [review]
[1/3] Make GetObjectBaseURI xpcom callable, mimic java's empty-codebase quirk
Comment 8 John Schoenick [:johns] 2012-02-02 16:16:33 PST
Created attachment 594007 [details] [diff] [review]
[2/3] Check file:/// URIs for same-directory as a sort of same-origin check
Comment 9 John Schoenick [:johns] 2012-02-02 16:17:33 PST
Created attachment 594008 [details] [diff] [review]
[3/3] Canonicalize the codebase as we see it for plugins
Comment 10 John Schoenick [:johns] 2012-02-02 16:23:02 PST
WIP patches

#1 - Makes GetObjectBaseURI xpcom-able so we can use this as the canonical base URI, to avoid duplicating logic. It also adds a mimicry of java's empty-codebase quirk (previously we were security checking different URLs than java)

#2 - Using the now-correct GetObjectBaseURI, check file-URLs for same-directory.

#3 - In the code that caches attributes for communication with the plugin, force codebase to the processed result of GetObjectBaseURI, such that we ensure agreement upon codebase.
Comment 11 John Schoenick [:johns] 2012-02-16 14:00:36 PST
Created attachment 597984 [details] [diff] [review]
[1/2] GetObjectBaseURI should mimic java's behavior
Comment 12 John Schoenick [:johns] 2012-02-16 14:02:45 PST
Created attachment 597985 [details] [diff] [review]
[2/2] Enforce same-directory for plugin codebase on file URIs

Simpler approach to this - since we don't know of any other java bugs/quirks with parsing the codebase, forcing the attribute to be the normalized path is probably overkill. 

These patches ensure GetObjectBaseURI() matches what the plugin will see by emulating java's quirk, and then runs a security check on same-directory for file URIs
Comment 13 Johnny Stenback (:jst, jst@mozilla.com) 2012-02-16 17:38:18 PST
Comment on attachment 597985 [details] [diff] [review]
[2/2] Enforce same-directory for plugin codebase on file URIs

It's unfortunately that we can't share some of this logic with nsPrincipal::CheckMayLoad(), but I don't really see an easy way to do that, given the differences. However, I'd like to see the guts of this file related stuff split out into its own function, that would simplify the return paths etc, you can do the fallback only once in the calling code, and it would make this code a bit easier to read. Other than that, this looks correct to me.

r- to get this split into its own function.
Comment 14 John Schoenick [:johns] 2012-02-23 16:38:40 PST
Created attachment 600231 [details] [diff] [review]
[1/4] GetObjectBaseURI should match java's empty-string quirk
Comment 15 John Schoenick [:johns] 2012-02-23 16:39:07 PST
Created attachment 600232 [details] [diff] [review]
[2/4] Make GetObjectBaseURI xpcomable
Comment 16 John Schoenick [:johns] 2012-02-23 16:39:27 PST
Created attachment 600233 [details] [diff] [review]
[3/4] Run same-directory check on file URIs
Comment 17 John Schoenick [:johns] 2012-02-23 16:43:17 PST
Created attachment 600237 [details] [diff] [review]
[4/4] Canonicalize the codebase pushed to java

So I discovered that "file:" also triggers bad behavior from java. This should switch the scheme to file but keep the path unchanged, but java also interprets this as "/" path on the new scheme.

Because we don't particularly care about preserving this behavior, resurrect the canonicalize-codebase patch to ensure agreement (and effectively break anyone relying on "scheme:" codebases). This avoids adding even more code to mimic java (especially since we can't parse relative URIs easily with nsIURI) and protects against any other unexpected issues in its URI parsing.
Comment 18 John Schoenick [:johns] 2012-02-23 17:18:41 PST
Created attachment 600256 [details] [diff] [review]
[4/4] Canonicalize the codebase pushed to java

Fix CnP error
Comment 19 John Schoenick [:johns] 2012-02-24 12:18:39 PST
Created attachment 600481 [details] [diff] [review]
[1/4] GetObjectBaseURI should match java's empty-string quirk

Fix whitespace
Comment 20 John Schoenick [:johns] 2012-02-24 12:19:08 PST
Created attachment 600482 [details] [diff] [review]
[2/4] Make GetObjectBaseURI xpcomable

fixup whitespace
Comment 21 John Schoenick [:johns] 2012-02-24 12:19:38 PST
Created attachment 600483 [details] [diff] [review]
[3/4] Run same-directory check on file URIs

Fixup whitespace, remove debugging LOG() statement
Comment 22 John Schoenick [:johns] 2012-02-24 12:20:08 PST
Created attachment 600484 [details] [diff] [review]
[4/4] Canonicalize the codebase pushed to java

whitespace.
Comment 23 John Schoenick [:johns] 2012-02-24 12:20:48 PST
Survived a try build here

https://tbpl.mozilla.org/?tree=Try&rev=498965274875
Comment 24 Johnny Stenback (:jst, jst@mozilla.com) 2012-02-24 15:28:53 PST
Comment on attachment 600483 [details] [diff] [review]
[3/4] Run same-directory check on file URIs

...
+    nsCOMPtr<nsIURI> docURI;
+    nsCOMPtr<nsIURI> baseURI;
+    GetObjectBaseURI(aTypeHint, getter_AddRefs(baseURI));
+    rv = thisContent->NodePrincipal()->GetURI(getter_AddRefs(docURI));

Given that docURI comes from the principal, I'd recommending doing s/doc(ument)?/origin/ in this whole patch, as the document uri can be different from the origin uri (especially when dealing with javascript: and data: URIs). Making that change would make this naming follow surrounding code better.

+      if (NS_FAILED(docURI->SchemeIs("file", &isfile)) ||
+          (isfile && !IsFileCodebaseAllowable(baseURI.get(), docURI.get()))) {

No need for those .get() calls.

r=jst with those changes made.
Comment 25 Johnny Stenback (:jst, jst@mozilla.com) 2012-02-24 15:32:30 PST
Comment on attachment 600484 [details] [diff] [review]
[4/4] Canonicalize the codebase pushed to java

Looks good! r=jst
Comment 26 John Schoenick [:johns] 2012-02-27 14:03:01 PST
Created attachment 601053 [details] [diff] [review]
Ensure we agree with java on applet codebase, and run security checks for file: codebase URIs. r=jst

docURI renamed to originURI, and rolled into one since the latter wont apply cleanly without the former.
Comment 27 Johnny Stenback (:jst, jst@mozilla.com) 2012-02-28 13:57:38 PST
Comment on attachment 601053 [details] [diff] [review]
Ensure we agree with java on applet codebase, and run security checks for file: codebase URIs. r=jst

r=jst
Comment 28 Johnny Stenback (:jst, jst@mozilla.com) 2012-02-28 20:54:57 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e4675fb8fc9
Comment 29 Ed Morley [:emorley] 2012-02-29 11:36:49 PST
https://hg.mozilla.org/mozilla-central/rev/1e4675fb8fc9
Comment 30 Mark Banner (:standard8) 2012-03-01 03:04:19 PST
This has caused a serious regression in Thunderbird - bug 731947 - Plugins are no longer blocked in email as per Thunderbird's content policy (or at least the application/x-test plugin isn't blocked).
Comment 31 Mark Banner (:standard8) 2012-03-01 06:20:26 PST
So I've just confirmed that although the code is trying to set allowPlugins and allowJavascript to false on the docShell (this is via nsMsgContentPolicy::ShouldLoad), it is no longer happening on the message pane docShell.
Comment 32 Boris Zbarsky [:bz] 2012-03-01 07:05:31 PST
Which is no longer happening?  The call doesn't get to the docshell?  Or the docshell setting is ignored?

Is the issue that nsObjectLoadingContent::GetObjectBaseURI got changed for the case of not-applets so that the "instantiate without a URI" codepath is now not passing in a useful URI anymore and security checks that rely on that fail?
Comment 33 Boris Zbarsky [:bz] 2012-03-01 07:06:11 PST
We should probably get a core bug followup filed blocking this one on the issue, and request tracking13 on it.
Comment 34 John Schoenick [:johns] 2012-03-01 10:54:16 PST
I'm looking into this
Comment 35 John Schoenick [:johns] 2012-03-01 15:33:05 PST
Created attachment 602167 [details] [diff] [review]
Always return a codebase for plugins, even if URI creation fails

The problem is this call is failing:

> nsContentUtils::NewURIWithDocumentCharset(aURI, codebase,
>                                           thisContent->OwnerDoc(),
>                                           baseURI);

With codebase "" and base "mailbox:///home/johns/moz/tb-dbg/mozilla/_tests/mozmill/mozmillprofile/Mail/Local%20Folders/pluginPolicy?number=0"

Causing this function to return NS_OK, but not return a URI. This could happen before, but only if the object had a codebase attribute, which the test doesn't cover.

Attached patch returns the object's baseURI if codebase URI creation fails.
Comment 36 Boris Zbarsky [:bz] 2012-03-01 16:48:21 PST
Why do you need to clone?  Can you not swap like the old code used to?
Comment 37 John Schoenick [:johns] 2012-03-01 16:59:38 PST
@bz - The function is exposed to xpcom now, so the old assert(aURI = null) quirk is undesirable. (especially as this is a trivial function performance-wise)
Comment 38 Boris Zbarsky [:bz] 2012-03-01 19:19:46 PST
> @bz - The function is exposed to xpcom now, so the old assert(aURI = null) quirk is 
> undesirable.

If you want to, you can set *aURI to null before swapping to make the swap safe.  I agree that just asserting that *aURI is null is no longer OK.

> especially as this is a trivial function performance-wise

Cloning some URIs can copy megabytes of data.  Let's just not do it unless we really really have to.

(On a separate note, why exactly is this function capitalized in the IDL?  The convention it lowercase first letter in IDL....)
Comment 39 John Schoenick [:johns] 2012-03-02 13:14:44 PST
Created attachment 602461 [details] [diff] [review]
Followup - Handle failed URI creation, fix typo

New patch - restores using uri swapping when possible, and fixes the capitalization in the idl
Comment 40 Boris Zbarsky [:bz] 2012-03-02 13:56:21 PST
Comment on attachment 602461 [details] [diff] [review]
Followup - Handle failed URI creation, fix typo

Drop the unnecessary else after return, return the rv that succeeded instead of NS_OK, and r=me.
Comment 41 John Schoenick [:johns] 2012-03-02 14:15:34 PST
Created attachment 602486 [details] [diff] [review]
[checkin-needed] Followup - Handle failed URI creation, fix typo. r=bzbarsky

changes per bz, checkin-needed
Comment 42 Johnny Stenback (:jst, jst@mozilla.com) 2012-03-02 16:52:20 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0b135e60d35
Comment 43 Ed Morley [:emorley] 2012-03-04 04:56:47 PST
https://hg.mozilla.org/mozilla-central/rev/b0b135e60d35
Comment 44 Alex Keybl [:akeybl] 2012-03-15 16:25:54 PDT
Tracking for the ESR that is released alongside Firefox 13. We'll want to make sure that we fix bug 731947 at the same time.
Comment 45 John Schoenick [:johns] 2012-04-06 17:40:28 PDT
The regression in Bug 736965 cannot be fully fixed without a bigger change to make our codebase detection in ObjectLoadingContent and cached parameters in PluginInstanceHost take <param> tags into consideration. Given that the code is very fragile and that we have almost no tests covering plugin loading and java specifically, this is rather risky regression-wise.

There are also and there are other much bigger security issues in ObjectLoadingContent that require refactoring of the class. This is happening in Bug 738396, which will also include better tests for java loading.

After talking with jst, I think the best course of action is to back this patch out of aurora, and address the regressions along with the further security issues in 738396.
Comment 46 Johnny Stenback (:jst, jst@mozilla.com) 2012-04-12 19:58:24 PDT
Created attachment 614666 [details] [diff] [review]
aurora backout of the fix + followup in this bug.

[Approval Request Comment]
Reason: The fix for this caused several regressions which are too risky to fix on aurora (see earlier comments). 
User impact if declined: java won't load in several cases
Testing completed (on m-c, etc.): This is a backout to a known good state (with a moderate security bug w/o this fix).
Risk to taking this patch (and alternatives if risky): We either take this, or we ship with java broken in significant ways, or we take a much riskier fix (which isn't quite done yet), i.e. not an option.
String changes made by this patch: n/a
Comment 47 Alex Keybl [:akeybl] 2012-04-16 16:32:00 PDT
Comment on attachment 614666 [details] [diff] [review]
aurora backout of the fix + followup in this bug.

[Triage Comment]
Approved for Aurora 13 given the regressions tracked in bug 736965 and bug 741758.
Comment 48 Johnny Stenback (:jst, jst@mozilla.com) 2012-04-16 17:33:06 PDT
Backout landed on aurora:

https://hg.mozilla.org/releases/mozilla-aurora/rev/96a1b7f055c9
Comment 49 Al Billings [:abillings] 2012-05-14 13:46:59 PDT
The original issue in comment 0 is fixed now. It is not entirely clear which changes the current bug state is tracking. Are we now tracking the status of the regressions caused by the initial checkin within this bug?
Comment 50 John Schoenick [:johns] 2012-06-01 11:26:19 PDT
Created attachment 629267 [details] [diff] [review]
Backout of aurora (14) for regressions
Comment 51 John Schoenick [:johns] 2012-06-01 11:31:13 PDT
Comment on attachment 629267 [details] [diff] [review]
Backout of aurora (14) for regressions

Back out of 14 as well for the same reasons as above. Bug 745030 should be landing shortly as a proper fix for this as well as unfixed security issues in Bug 738396 - but wont be suitable for aurora.

[Approval Request Comment]
User impact if declined: Major plugin loading regressions, mainly with java.
Testing completed (on m-c, etc.): Returning to known-good state
Risk to taking this patch (and alternatives if risky): Re-introduces sg-moderate issue
String or UUID changes made by this patch: Changes nsIObjectLoadingContent uuid (cannot revert to original UUID due to subsequent changes)
Comment 52 Alex Keybl [:akeybl] 2012-06-01 14:48:29 PDT
Comment on attachment 629267 [details] [diff] [review]
Backout of aurora (14) for regressions

[Triage Comment]
Approving the backout. Please make sure to update the status flags once landed. Thanks!
Comment 53 Andrew McCreight [:mccr8] 2012-06-04 16:22:46 PDT
Backed out from beta 14.
https://hg.mozilla.org/releases/mozilla-beta/rev/4782adb17edf
Comment 54 Daniel Veditz [:dveditz] 2012-06-14 16:18:09 PDT
I guess 15 is still "fixed", although we may get around to backing it out later if bug 745030 doesn't land soon.
Comment 55 Daniel Veditz [:dveditz] 2012-06-14 16:21:56 PDT
Is FIrefox 14 a "wontfix", or do we expect to somehow get this checked in again? I'm assuming it's simply not in this release. Are the regressions likely to get fixed before Firefox 15 makes it to Beta? Maybe we should just back it out from there now if that's not likely.
Comment 56 John Schoenick [:johns] 2012-06-19 17:30:45 PDT
Created attachment 634674 [details] [diff] [review]
Backout of 15
Comment 57 John Schoenick [:johns] 2012-06-19 17:31:20 PDT
Created attachment 634675 [details] [diff] [review]
Backout of 16
Comment 58 John Schoenick [:johns] 2012-06-19 17:40:44 PDT
Comment on attachment 634674 [details] [diff] [review]
Backout of 15

Bug 745030 may not land this cycle and bug 738396 will need to land on top of it to fix further issues with java. Additionally, fully fixing the regressions here might require some of the changes in 738396.

I talked with jst and he agrees the best course of action is to back this out from remaining branches, and re-land it merged with the 738396 changes once 745030 is landed (when it can be done without regressions)

[Approval Request Comment]
User impact if declined: Significant Java regressions
Testing completed (on m-c, etc.): Returning to known good state
Risk to taking this patch (and alternatives if risky): re-introduces sg-moderate issue
String or UUID changes made by this patch: Changes nsIObjectLoadingContent UUID to match FF14
Comment 59 Alex Keybl [:akeybl] 2012-06-24 13:30:49 PDT
(In reply to John Schoenick [:johns] from comment #58)
> String or UUID changes made by this patch: Changes nsIObjectLoadingContent
> UUID to match FF14

Does this have any possibility of compatibility regressions? If we're not sure, we should loop somebody in from AMO (like jorgev).
Comment 60 John Schoenick [:johns] 2012-06-25 13:03:46 PDT
(In reply to Alex Keybl [:akeybl] from comment #59)
> Does this have any possibility of compatibility regressions? If we're not
> sure, we should loop somebody in from AMO (like jorgev).

I don't believe so - this removes a single call addons were not likely to be using (getObjectBaseURI) and thus changes the interface and its UUID to match what is on beta and aurora - addons that have compatibility there should be fine through trunk.

Additionally, I believe we will get away with fixing this properly with 745030 + 738396 without changing the interface again, the changes in bug 745030 mean we don't actually need to export this function via the IDL
Comment 61 Alex Keybl [:akeybl] 2012-06-28 09:51:48 PDT
Comment on attachment 634674 [details] [diff] [review]
Backout of 15

Thanks John and Johnny. Approving for Aurora 15 given r+ and the above risk assessment.
Comment 63 Johnny Stenback (:jst, jst@mozilla.com) 2012-08-23 17:03:24 PDT
Just to clarify, this was reopened because the last checkin here was a backout. There's a new version of this fix coming, but that builds on some significant changes that were needed in order to be able to fix this sanely.
Comment 64 John Schoenick [:johns] 2013-01-03 15:25:10 PST
Hang on, why was this marked fixed in 18? AFAIK this is still an issue, along with bug 738396. Bug 745030 and it's follow-ups make this possible to fix sanely, but any fix could not be landed on esr10, so nothing has been pushed yet.
Comment 65 Al Billings [:abillings] 2013-01-03 15:47:25 PST
Removing CVE number and advisory tags.
Comment 66 John Schoenick [:johns] 2013-04-02 10:45:53 PDT
Bug 738396 fixes most of the codebase handling issues here, so all we need to fix this is the proper same-directory policy for file:/// URIs
Comment 67 John Schoenick [:johns] 2013-04-02 10:47:29 PDT
Comment on attachment 601053 [details] [diff] [review]
Ensure we agree with java on applet codebase, and run security checks for file: codebase URIs. r=jst

This needs a new approach based on bug 745030 and bug 738396
Comment 68 John Schoenick [:johns] 2013-04-04 17:02:00 PDT
Created attachment 733642 [details] [diff] [review]
Add an allowFileDirectories argument to nsPrincipal::CheckMayLoad

Allows us to do same origin file checking while bypassing the no-directories rule
Comment 69 John Schoenick [:johns] 2013-04-04 17:03:01 PDT
Created attachment 733644 [details] [diff] [review]
Add check for java file codebase security

Based on the patches in bug 738396 -- add a same origin check to the codebase if the plugin is java and the codebase is a file URI
Comment 70 John Schoenick [:johns] 2013-04-04 17:04:21 PDT
Created attachment 733645 [details] [diff] [review]
[Fold into patch on branches] Move strict file origin checking blob into nsOBJLC

We presumably wont want to land the CheckMayLoad change on branches, this just duplicates that code into ObjectLoadingContent minus the directory check. This patch would be landed into the add-codebase-check patch on branches only.
Comment 71 John Schoenick [:johns] 2013-05-29 11:32:30 PDT
Created attachment 755520 [details] [diff] [review]
Test
Comment 72 John Schoenick [:johns] 2013-05-29 11:40:41 PDT
Comment on attachment 733642 [details] [diff] [review]
Add an allowFileDirectories argument to nsPrincipal::CheckMayLoad

bz, would you be the right person to review this?
Comment 73 John Schoenick [:johns] 2013-05-29 11:45:58 PDT
Comment on attachment 733644 [details] [diff] [review]
Add check for java file codebase security

Based on the fixes in bug 738396 - do a same-origin check on file URIs to enforce our strict file origin policy (but note that we don't same-origin check java otherwise :-/)
Comment 74 John Schoenick [:johns] 2013-05-29 11:47:38 PDT
Comment on attachment 733645 [details] [diff] [review]
[Fold into patch on branches] Move strict file origin checking blob into nsOBJLC

If we take this on any branches we probably wont want to uplift the CheckMayLoad() change - this just duplicates some same-file-origin policy code into ObjectLoadingContent for branches only.
Comment 75 John Schoenick [:johns] 2013-05-29 11:51:33 PDT
Comment on attachment 755520 [details] [diff] [review]
Test

Test that java plugins whose codebase violates the strict file origin policy don't spawn.
Comment 76 Boris Zbarsky [:bz] 2013-05-30 12:20:05 PDT
Why do we need that CheckMayLoad change, exactly?
Comment 77 John Schoenick [:johns] 2013-05-30 13:58:46 PDT
(In reply to Boris Zbarsky (:bz) from comment #76)
> Why do we need that CheckMayLoad change, exactly?

We want to do a same origin check for java's codebase value when it is on the local filesystem, to enforce our strict file origin checks, but it needs to allow directory targets. We could move the strict file origin stuff out of CheckMayLoad to another utility function, but it constitutes the bulk of the function and didn't really seem any less messy. If you have a suggestion for a cleaner way I'm more than willing to rework it.
Comment 78 Boris Zbarsky [:bz] 2013-05-30 14:06:47 PDT
So the codebase can be a directory?

I would rather have some sort of utility function called from both places, honestly, since CheckMayLoad() is really supposed to be about loading things and that's not what we're doing here.  Unless we need logic from CheckMayLoad other than the file:// handling?
Comment 79 John Schoenick [:johns] 2013-05-30 14:29:53 PDT
(In reply to Boris Zbarsky (:bz) from comment #78)
> So the codebase can be a directory?
> 
> I would rather have some sort of utility function called from both places,
> honestly, since CheckMayLoad() is really supposed to be about loading things
> and that's not what we're doing here.  Unless we need logic from
> CheckMayLoad other than the file:// handling?

The problem is java assumes everything under codebase is fair game for it to load, but doesn't go through us to open channels, so checking that the page may load that base URI is as close as we can get.

We normally skip the same-origin check, but we want to do the strict file same-origin check in the case of file:// URIs, which is what CheckMayLoad is enforcing.

However, CheckMayLoad is now basically a wrapper around SecurityCompareURIs that also implements strict file origin. If we split it out, All that would remain of CheckMayLoad would be:

> NS_IMETHODIMP
> nsPrincipal::CheckMayLoad(nsIURI* aURI, bool aReport, bool aAllowIfInheritsPrincipal)
> {
>    if (aAllowIfInheritsPrincipal) {
>     // If the caller specified to allow loads of URIs that inherit
>     // our principal, allow the load if this URI inherits its principal
>     if (nsPrincipal::IsPrincipalInherited(aURI)) {
>       return NS_OK;
>     }
>   }
> 
>   if (!nsScriptSecurityManager::SecurityCompareURIs(mCodebase, aURI) && !CheckStrictFileOriginPolicy()) {
>     return NS_ERROR_DOM_BAD_URI;
>   }
> 
>   return NS_OK;
> }

So assuming that SecurityCompareURIs is going to succeed on two file:// URIs, the other checks are redundant. Though, if we did add some additional file restrictions to SecurityCompareURIs at some point, we would conceivably want them to apply here.

Perhaps CheckMayLoad should just be merged with SecurityCompareURIs? Or the strict-file-origin policy moved to SecurityCompareURIs instead of CheckMayLoad?
Comment 80 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2013-05-30 14:46:27 PDT
Could we just disallow java on file: URIs completely (behind a pref, I guess)?
Comment 81 Boris Zbarsky [:bz] 2013-05-30 14:57:48 PDT
> So assuming that SecurityCompareURIs is going to succeed on two file:// URIs

It's not.  It's going to fail on two distinct file:// URIs if the file origin policy is strict.  The CheckStrictFileOriginPolicy() bit actually allows the check to pass even though two different file:// URIs are normally considered different origins.

> Perhaps CheckMayLoad should just be merged with SecurityCompareURIs?

No, because SecurityCompareURIs is a stricter and symmetric check while CheckMayLoad is a looser and asymmetric check.

> All that would remain of CheckMayLoad would be:

That seems perfectly reasonable to me if we have a second consumer who in fact wants the modified CheckMayLoad file://-related behavior there.
Comment 82 John Schoenick [:johns] 2013-05-30 15:06:17 PDT
Comment on attachment 733642 [details] [diff] [review]
Add an allowFileDirectories argument to nsPrincipal::CheckMayLoad

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #80)
> Could we just disallow java on file: URIs completely (behind a pref, I
> guess)?

With the work on bug 738396 this is actually not too hard to fix, changes to CheckMayLoad aside. If java behaves and stays confined to its codebase its risk shouldn't be any higher than on a site, though it is java...

(In reply to Boris Zbarsky (:bz) from comment #81)
> > So assuming that SecurityCompareURIs is going to succeed on two file:// URIs
> 
> It's not.  It's going to fail on two distinct file:// URIs if the file
> origin policy is strict.  The CheckStrictFileOriginPolicy() bit actually
> allows the check to pass even though two different file:// URIs are normally
> considered different origins.
> 
> > Perhaps CheckMayLoad should just be merged with SecurityCompareURIs?
> 
> No, because SecurityCompareURIs is a stricter and symmetric check while
> CheckMayLoad is a looser and asymmetric check.
> 
> > All that would remain of CheckMayLoad would be:
> 
> That seems perfectly reasonable to me if we have a second consumer who in
> fact wants the modified CheckMayLoad file://-related behavior there.

Okay, that sounds sane then. Do you have any preference as to where this should live?
Comment 83 John Schoenick [:johns] 2013-05-30 15:08:42 PDT
Comment on attachment 733645 [details] [diff] [review]
[Fold into patch on branches] Move strict file origin checking blob into nsOBJLC

This patch is just duplicating the strict file origin code into nsObjectLoadingContent for branches only, it'll remain necessary independent of how we factor it on trunk
Comment 84 Boris Zbarsky [:bz] 2013-05-30 15:28:01 PDT
If we can put the file:// bits in nsNetUtil, I would vote there, but I haven't read them that carefully...  Failing that, nsContentUtils?
Comment 85 John Schoenick [:johns] 2013-06-04 11:00:59 PDT
Created attachment 758043 [details] [diff] [review]
Add NS_URIIsLocalFile and NS_CheckStrictFileOriginPolicy

Add these bits to nsNetUtil. URIIsLocalFile is wanted by nsObjectLoadingContent when it is doing the checks, so I promoted it as well.
Comment 86 John Schoenick [:johns] 2013-06-04 11:02:48 PDT
Created attachment 758046 [details] [diff] [review]
Use NS_CheckStrictOriginPolicy in CheckMayLoad

Split out to avoid touching this on branches.
Comment 87 John Schoenick [:johns] 2013-06-04 11:15:53 PDT
Created attachment 758053 [details] [diff] [review]
Add check for java file codebase security. r=bsmedberg

Use NS_CheckStrictFileOriginPolicy instead of CheckMayLoad, carrying over r+
Comment 88 Boris Zbarsky [:bz] 2013-06-04 12:58:13 PDT
Comment on attachment 758043 [details] [diff] [review]
Add NS_URIIsLocalFile and NS_CheckStrictFileOriginPolicy

>+  MOZ_ASSERT(util, "should have netutil");

Then why do we null-check it later?

>+NS_CheckStrictFileOriginPolicy(nsIURI *aURI,

This should probably be called NS_RelaxStrictFileOriginPolicy.

And aURI should be aTargetURI, while aCodebaseURI is aSourceURI, I think, with appropriate renaming for fileURL, codebaseFileURL, codebaseIsDir, codebaseFile, codebaseParent.  Otherwise it's not clear what's going on with this check; recall that this is an _asymmetric_ check, so the order of arguments is very important.  Worth documenting exactly what this function is meant for and what the arguments mean.

For example, your call to this function in the other patch got the order backwards...  Which would have been easier to notice if the two changes had been in the same patch, by the way.  That way blame would be easier to track too.  Please do combine the patches like that.

>+      (!aAllowDirectoryURIs &&
>+      (NS_FAILED(targetFile->IsDirectory(&targetIsDir)) || targetIsDir))) {

That second line needs to be indented one more space.

r=me with the above fixed.
Comment 89 Boris Zbarsky [:bz] 2013-06-04 12:58:46 PDT
Comment on attachment 758046 [details] [diff] [review]
Use NS_CheckStrictOriginPolicy in CheckMayLoad

This gets the check backwards.  And this should be part of the patch that adds the new method anyway, I think.
Comment 90 John Schoenick [:johns] 2013-06-04 15:09:52 PDT
Created attachment 758243 [details] [diff] [review]
Split NS_CheckStrictFileOriginPolicy out of nsPrincipal::CheckMayLoad

I made enough changes to this that I'd appreciate a second look, this would be bad code to make a mistake in (like swapping argument orders)

(In reply to Boris Zbarsky (:bz) from comment #88)
> Comment on attachment 758043 [details] [diff] [review]
> Add NS_URIIsLocalFile and NS_CheckStrictFileOriginPolicy
>
> >+  MOZ_ASSERT(util, "should have netutil");
>
> Then why do we null-check it later?

Removed

> >+NS_CheckStrictFileOriginPolicy(nsIURI *aURI,
>
> This should probably be called NS_RelaxStrictFileOriginPolicy.
>
> And aURI should be aTargetURI, while aCodebaseURI is aSourceURI, I think,
> with appropriate renaming for fileURL, codebaseFileURL, codebaseIsDir,
> codebaseFile, codebaseParent.  Otherwise it's not clear what's going on with
> this check; recall that this is an _asymmetric_ check, so the order of
> arguments is very important.  Worth documenting exactly what this function
> is meant for and what the arguments mean.

I renamed these as such, and added an explanatory comment

> For example, your call to this function in the other patch got the order
> backwards...  Which would have been easier to notice if the two changes had
> been in the same patch, by the way.  That way blame would be easier to track
> too.  Please do combine the patches like that.

I originally split these such that CheckMayLoad could be unchanged should we land this on branches, but we can split it if deemed necessary at that point.

> >+      (!aAllowDirectoryURIs &&
> >+      (NS_FAILED(targetFile->IsDirectory(&targetIsDir)) || targetIsDir))) {
>
> That second line needs to be indented one more space.
>
> r=me with the above fixed.

(In reply to Boris Zbarsky (:bz) from comment #89)
> Comment on attachment 758046 [details] [diff] [review]
> Use NS_CheckStrictOriginPolicy in CheckMayLoad
>
> This gets the check backwards.  And this should be part of the patch that
> adds the new method anyway, I think.

Indeed it does, I had swapped the argument order to the call at one point and neglected to update the callers. However, while trying to figure out why my test didn't catch this, I found that nsIFile::Contains() recursion argument is ignored now, so we actually don't implement file same origin policy as described at:
https://developer.mozilla.org/en-US/docs/Same-origin_policy_for_file:_URIs -- I'll open a followup for this
Comment 91 John Schoenick [:johns] 2013-06-04 15:12:35 PDT
Created attachment 758249 [details] [diff] [review]
Add check for java file codebase security. r=bsmedberg
Comment 92 Boris Zbarsky [:bz] 2013-06-05 23:11:46 PDT
Comment on attachment 758243 [details] [diff] [review]
Split NS_CheckStrictFileOriginPolicy out of nsPrincipal::CheckMayLoad

This looks great.  r=me, and sorry for the lag....
Comment 93 John Schoenick [:johns] 2013-06-06 11:42:18 PDT
Comment on attachment 758249 [details] [diff] [review]
Add check for java file codebase security. r=bsmedberg

[Security approval request comment]
This is just an enhancement on top of bug 738396 to enforce our file origin policy to java's codebase for sites loaded from a file URI:
https://developer.mozilla.org/en-US/docs/Same-origin_policy_for_file:_URIs

It requires 738396, but does not introduce any significant risk on top of it.

How easily could an exploit be constructed based on the patch?
The only functional change is an explicit check for file strict origin policy for java codebase, so pretty easily.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
A new comment explains the check, which is pretty obvious regardless.

Which older supported branches are affected by this flaw?
All, but not relevant to b2g where java is not present.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should not be difficult to rebase on all branches where 738396 lands

How likely is this patch to cause regressions; how much testing does it need?
Fairly low, only adds one additional check to java's codebase, and only when loading a local file:/// URI. Might break some odd enterprise setups that involve running java on the local disk, but there is already a pref to disable strict file origin checking.
Comment 94 John Schoenick [:johns] 2013-06-06 11:45:49 PDT
Re-tracking now that there's a ready patch
Comment 95 Al Billings [:abillings] 2013-06-06 14:47:25 PDT
Comment on attachment 758249 [details] [diff] [review]
Add check for java file codebase security. r=bsmedberg

Sec-approval+ but since this is only a sec-moderate, it doesn't *require* sec-approval to go into trunk.
Comment 96 John Schoenick [:johns] 2013-06-06 16:22:54 PDT
(In reply to Al Billings [:abillings] from comment #95)
> Sec-approval+ but since this is only a sec-moderate, it doesn't *require*
> sec-approval to go into trunk.

Ack, good point.
Comment 97 bhavana bajaj [:bajaj] 2013-06-07 18:04:18 PDT
Tracking the issue due to the risk called out in comment #93 and already marking it as wontfix as it looks like the dependent bug 738396 will be fixed on FX23.
Comment 98 John Schoenick [:johns] 2013-07-02 16:41:04 PDT
Comment on attachment 758243 [details] [diff] [review]
Split NS_CheckStrictFileOriginPolicy out of nsPrincipal::CheckMayLoad

https://hg.mozilla.org/integration/mozilla-inbound/rev/9eafe94ea972
Comment 99 John Schoenick [:johns] 2013-07-02 16:41:22 PDT
Comment on attachment 758249 [details] [diff] [review]
Add check for java file codebase security. r=bsmedberg

https://hg.mozilla.org/integration/mozilla-inbound/rev/b06bd5858999
Comment 101 Ryan VanderMeulen [:RyanVM] 2013-07-03 13:54:35 PDT
Are we intending to uplift these to 23/24?
Comment 102 John Schoenick [:johns] 2013-07-03 13:54:46 PDT
Comment on attachment 758249 [details] [diff] [review]
Add check for java file codebase security. r=bsmedberg

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Not a FF bug, works around java having lax security checks with file:/// URIs

User impact if declined:
sec-low - saving a malicious site locally, with embedded java applets, and then running it, would allow read-only access to the full filesystem.

Testing completed (on m-c, etc.):
On m-c

Risk to taking this patch (and alternatives if risky):
Low, the bulk of the patch is moving functions around to be accessible by plugin code, and additional checks are only done for sites running on file:/// URIs.

String or IDL/UUID changes made by this patch:
None
Comment 103 John Schoenick [:johns] 2013-07-03 13:55:51 PDT
Comment on attachment 758243 [details] [diff] [review]
Split NS_CheckStrictFileOriginPolicy out of nsPrincipal::CheckMayLoad

see comment 102
Comment 104 John Schoenick [:johns] 2013-07-03 13:56:15 PDT
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #101)
> Are we intending to uplift these to 23/24?

beat me by 11 seconds :-P
Comment 105 Ryan VanderMeulen [:RyanVM] 2013-07-08 06:57:01 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/462bf273f287
https://hg.mozilla.org/releases/mozilla-aurora/rev/3967f5179aab

This doesn't apply cleanly to beta without bug 738396. I'll land it there once bug 738396 gets ba+.
Comment 107 John Schoenick [:johns] 2013-07-08 14:00:22 PDT
Comment on attachment 758249 [details] [diff] [review]
Add check for java file codebase security. r=bsmedberg

This was tracked for esr17, so nominating for a call on whether or not to uplift there -- See comment 102
Comment 108 John Schoenick [:johns] 2013-07-08 14:00:50 PDT
Comment on attachment 758243 [details] [diff] [review]
Split NS_CheckStrictFileOriginPolicy out of nsPrincipal::CheckMayLoad

(see comment 107)
Comment 109 Alex Keybl [:akeybl] 2013-07-23 10:25:13 PDT
Since this is sec-low, let's be risk averse.
Comment 110 John Schoenick [:johns] 2013-08-07 15:00:35 PDT
Created attachment 787151 [details] [diff] [review]
Test. r=bsmedberg

Add third case to test for issue found by bug 902375.

This just adds a third applet tag to the existing logic, keeping r+
Comment 111 John Schoenick [:johns] 2014-01-27 12:40:56 PST
@dveditz - Can this bug and bug 738396 have their test cases landed now that this is on all branches? (Is there a specific policy for test cases and other follow ups as such?)
Comment 112 Al Billings [:abillings] 2014-01-27 13:40:50 PST
John, please land the tests.

Generally, six to eight weeks after release of the fix to the public (or when the *following* release comes out) is the guideline.
Comment 113 Daniel Veditz [:dveditz] 2014-01-31 14:14:15 PST
Yes that's fine now, we stopped supporting ESR-17 (where this isn't fixed) in December.
Comment 114 John Schoenick [:johns] 2014-02-20 14:32:10 PST
Created attachment 8379262 [details] [diff] [review]
Test. r=bsmedberg

Un-bitrotted
Comment 116 Wes Kocher (:KWierso) 2014-02-20 16:23:22 PST
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 117 John Schoenick [:johns] 2014-02-25 13:35:01 PST
Attempt 2 at test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/277862f099a4

Extensive try run since this push burned the tree before:
https://tbpl.mozilla.org/?tree=Try&rev=b656bdd77ce1
Comment 118 Wes Kocher (:KWierso) 2014-02-25 20:16:07 PST
https://hg.mozilla.org/mozilla-central/rev/277862f099a4
Comment 119 John Schoenick [:johns] 2014-02-26 12:00:31 PST
This was fixed in 25, the additional landing here only applies to the mochitest

Note You need to log in before you can comment on or make changes to this bug.