406541 - (CVE-2013-1717) local java applet may read arbitrary files under certain circumstances

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[georgi - hopefully not receiving bugspam]

Attachment: 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]

Attachment: a1.html - open it locally after placing the applet in correct dir

Comment 2

[Kenneth Russell]

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]

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]

[sg:moderate] imo

Comment 5

[Johnny Stenback (:jst)]

georgi, is this still a problem with an up to date java plugin?

Comment 6

[Johnny Stenback (:jst)]

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]]

Attachment: [1/3] Make GetObjectBaseURI xpcom callable, mimic java's empty-codebase quirk

Comment 8

[John Schoenick [:johns]]

Attachment: [2/3] Check file:/// URIs for same-directory as a sort of same-origin check

Comment 9

[John Schoenick [:johns]]

Attachment: [3/3] Canonicalize the codebase as we see it for plugins

Comment 10

[John Schoenick [:johns]]

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]]

Attachment: [1/2] GetObjectBaseURI should mimic java's behavior

Comment 12

[John Schoenick [:johns]]

Attachment: [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)]

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]]

Attachment: [1/4] GetObjectBaseURI should match java's empty-string quirk

Comment 15

[John Schoenick [:johns]]

Attachment: [2/4] Make GetObjectBaseURI xpcomable

Comment 16

[John Schoenick [:johns]]

Attachment: [3/4] Run same-directory check on file URIs

Comment 17

[John Schoenick [:johns]]

Attachment: [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]]

Attachment: [4/4] Canonicalize the codebase pushed to java

Fix CnP error

Comment 19

[John Schoenick [:johns]]

Attachment: [1/4] GetObjectBaseURI should match java's empty-string quirk

Fix whitespace

Comment 20

[John Schoenick [:johns]]

Attachment: [2/4] Make GetObjectBaseURI xpcomable

fixup whitespace

Comment 21

[John Schoenick [:johns]]

Attachment: [3/4] Run same-directory check on file URIs

Fixup whitespace, remove debugging LOG() statement

Comment 22

[John Schoenick [:johns]]

Attachment: [4/4] Canonicalize the codebase pushed to java

whitespace.

Comment 23

[John Schoenick [:johns]]

Survived a try build here

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

Comment 24

[Johnny Stenback (:jst)]

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)]

Comment on attachment 600484 [details] [diff] [review]
[4/4] Canonicalize the codebase pushed to java

Looks good! r=jst

Comment 26

[John Schoenick [:johns]]

Attachment: 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)]

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)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e4675fb8fc9

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1e4675fb8fc9

Comment 30

[Mark Banner (:standard8)]

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)]

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 [:bzbarsky]]

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 [:bzbarsky]]

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]]

I'm looking into this

Comment 35

[John Schoenick [:johns]]

Attachment: 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 [:bzbarsky]]

Why do you need to clone?  Can you not swap like the old code used to?

Comment 37

[John Schoenick [:johns]]

@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 [:bzbarsky]]

> @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]]

Attachment: 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 [:bzbarsky]]

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]]

Attachment: [checkin-needed] Followup - Handle failed URI creation, fix typo. r=bzbarsky

changes per bz, checkin-needed

Comment 42

[Johnny Stenback (:jst)]

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

Comment 43

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b0b135e60d35

Comment 44

[Alex Keybl [:akeybl]]

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]]

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)]

Attachment: 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]]

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)]

Backout landed on aurora:

https://hg.mozilla.org/releases/mozilla-aurora/rev/96a1b7f055c9

Comment 49

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

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]]

Attachment: Backout of aurora (14) for regressions

Comment 51

[John Schoenick [:johns]]

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]]

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]]

Backed out from beta 14.
https://hg.mozilla.org/releases/mozilla-beta/rev/4782adb17edf

Comment 54

[Daniel Veditz [:dveditz]]

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]]

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]]

Attachment: Backout of 15

Comment 57

[John Schoenick [:johns]]

Attachment: Backout of 16

Comment 58

[John Schoenick [:johns]]

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]]

(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]]

(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]]

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 62

[Johnny Stenback (:jst)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/419408a8db2f
https://hg.mozilla.org/mozilla-central/rev/47f814827db6

Comment 63

[Johnny Stenback (:jst)]

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]]

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 - ex-MoCo]]

Removing CVE number and advisory tags.

Comment 66

[John Schoenick [:johns]]

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]]

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]]

Attachment: 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]]

Attachment: 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]]

Attachment: [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]]

Attachment: Test

Comment 72

[John Schoenick [:johns]]

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]]

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]]

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]]

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 [:bzbarsky]]

Why do we need that CheckMayLoad change, exactly?

Comment 77

[John Schoenick [:johns]]

(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 [:bzbarsky]]

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]]

(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]

Could we just disallow java on file: URIs completely (behind a pref, I guess)?

Comment 81

[Boris Zbarsky [:bzbarsky]]

> 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]]

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]]

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 [:bzbarsky]]

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]]

Attachment: 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]]

Attachment: Use NS_CheckStrictOriginPolicy in CheckMayLoad

Split out to avoid touching this on branches.

Comment 87

[John Schoenick [:johns]]

Attachment: Add check for java file codebase security. r=bsmedberg

Use NS_CheckStrictFileOriginPolicy instead of CheckMayLoad, carrying over r+

Comment 88

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

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]]

Attachment: 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]]

Attachment: Add check for java file codebase security. r=bsmedberg

Comment 92

[Boris Zbarsky [:bzbarsky]]

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]]

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]]

Re-tracking now that there's a ready patch

Comment 95

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

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]]

(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]]

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]]

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]]

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 100

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9eafe94ea972
https://hg.mozilla.org/mozilla-central/rev/b06bd5858999

Comment 101

[Ryan VanderMeulen [:RyanVM]]

Are we intending to uplift these to 23/24?

Comment 102

[John Schoenick [:johns]]

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]]

Comment on attachment 758243 [details] [diff] [review]
Split NS_CheckStrictFileOriginPolicy out of nsPrincipal::CheckMayLoad

see comment 102

Comment 104

[John Schoenick [:johns]]

(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]]

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 106

[John Schoenick [:johns]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fc3d9e72357b
https://hg.mozilla.org/releases/mozilla-beta/rev/363615b93e48

Comment 107

[John Schoenick [:johns]]

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]]

Comment on attachment 758243 [details] [diff] [review]
Split NS_CheckStrictFileOriginPolicy out of nsPrincipal::CheckMayLoad

(see comment 107)

Comment 109

[Alex Keybl [:akeybl]]

Since this is sec-low, let's be risk averse.

Comment 110

[John Schoenick [:johns]]

Attachment: 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]]

@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 - ex-MoCo]]

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]]

Yes that's fine now, we stopped supporting ESR-17 (where this isn't fixed) in December.

Comment 114

[John Schoenick [:johns]]

Attachment: Test. r=bsmedberg

Un-bitrotted

Comment 115

[John Schoenick [:johns]]

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

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

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

Comment 116

[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 117

[John Schoenick [:johns]]

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) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/277862f099a4

Comment 119

[John Schoenick [:johns]]

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