1172080 - Using an optional '!' in a principal's origin to denote extra data is spoofable

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Consider the URI file:///tmp/foo!appId=5&inBrowser=1

This is a totally reasonable URI using allowed URI chars.  A principal for this URI would have an origin that looks like it's for an app and whatnot, even though those are just part of the URI.

Bobby and I talked this over.  We basically want a separator char that's not in the list at https://url.spec.whatwg.org/#url-code-points and is not '%' and also doesn't appear in the http://mxr.mozilla.org/mozilla-central/source/dom/quota/QuotaManager.cpp?rev=f96332d03ecd#773 blacklist.

At first glance, that restricts us to backtick, caret, curly braces, square brackets, apostrope.

As a further complication, while nsStandardURL does in fact percent-escape those chars when they appear in it, nsSimpleURI does nothing of the sort.  You can use all those chars in a data: URI, for example.

Luckily, all those chars are also disallowed in schemes, and we do in fact enforce some sanity on scheme chars.  So the proposal is to have the origin be either a url or ^url^otherstuff (using caret).  Examination of the first char can tell you whether there is stuff at the end, and then an rfind for '^' will let you parse the extra data out.  This should work for all URI types.

Comment 1

[Bobby Holley (:bholley)]

Boris and I misunderstood each other a bit on IRC. My hope was that we could still do the same format that we have now, but switch to a character that was disallowed in URLs (i.e. ^). The prefixing thing is ugly and complicates parsing, so I'd rather avoid it if we can, since we'll be living with it forever, and seeing more of it as we move more stuff to OriginAttributes.

It seems like the main problem is that we currently create codebase principals for things that aren't URLs (see bug 1172022). Do we actually need to do that, or can we drop support for it and then assume that any |^| in the origin string is a separator?

Comment 2

[Boris Zbarsky [:bzbarsky]]

I _think_ if we drop support for it we're probably ok.  Modulo extension-implemented nsIURIs and whatnot...

Basically, I think we can avoid the prefix thing only if we restrict origin URIs to implementations that we have carefully audited and verified that they can't include our separator char without escaping it.

Comment 3

[Bobby Holley (:bholley)]

So as I mentioned on IRC - as long as we support file:// URIs, using something like |^| doesn't solve the problem. And given that, for the Desktop privacy stuff, we probably want origin attributes to be inherited from the docshell, we probably can't easily forbid someone creating an iframe and navigating it to a file: URI.

So I think the most straightforward solution is probably just to make .origin include the |!| no matter what. mystor offered to do a try push to see how many tests that breaks.

Comment 4

[Boris Zbarsky [:bzbarsky]]

A '^' in a filename will get %-escaped in a file:// URI in Gecko.

But yes, always including the '!' is another option for sure.  Then just an rfind would work.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=894eb71b9ee7

It looks like most of the errors are simple string comparisons which now fail with the '!' always on the end of the origin string, except that there is some more interesting breakage in bc1. I imagine fixing it wouldn't be too difficult.

Comment 6

[Bobby Holley (:bholley)]

(In reply to Michael Layzell [:mystor] from comment #5)
> try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=894eb71b9ee7
> 
> It looks like most of the errors are simple string comparisons which now
> fail with the '!' always on the end of the origin string, except that there
> is some more interesting breakage in bc1. I imagine fixing it wouldn't be
> too difficult.

Yeah - we should generalize the hard-coded string comparisons wherever possible so that the tests pass with and without this change.

Do you have the cycles to make this happen?

Comment 7

[Nika Layzell [:nika] (ni? for response)]

How would you want to generalize the the string comparisons? Many of these tests take the form of something like:

  do_check_eq(ssm.getSystemPrincipal().origin, '[System Principal]');

I could obviously change that to

  do_check_eq(ssm.getSystemPrincipal().origin, '[System Principal]!');

But I'm not sure what the best other way to do it would be so that it worked on either - creating another system principal and getting it's origin?

I can probably get these tests passing soon, but it may not be for a few days, I've got a bunch of other stuff on my plate right now.

Comment 8

[Bobby Holley (:bholley)]

(In reply to Michael Layzell [:mystor] from comment #7)
> How would you want to generalize the the string comparisons? Many of these
> tests take the form of something like:
> 
>   do_check_eq(ssm.getSystemPrincipal().origin, '[System Principal]');

This one, sure - test_origin.js is a special case. But we want tests like in exactly one place. Any _other_ tests trying to detect should just compare against the actual system principal (or invoke ssm.isSystemPrincipal). The code depending on the exact origin format should be localized.

Comment 9

[Bobby Holley (:bholley)]

Thanks for taking this on Michael - over to you for now, let me know if you have any questions.

Comment 10

[Bobby Holley (:bholley)]

Here's an idea - how about we just make nsIPrincipal::origin throw for non-well-behaving URIs? That way we know that any origin string for which we're performing the origin->principal conversion must have been generated from a well-behaved principal, and thus we can apply our assumptions appropriately?

Comment 11

[Bobby Holley (:bholley)]

Boris, what do you think about doing this, and leaving the current !-as-optional scheme we have now?

Comment 12

[Boris Zbarsky [:bzbarsky]]

So whatever things get origin would work on some pages but not others?  I guess that's survivable depending on how defensively the consumers of .origin are written.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Not doing reviews right now from comment #12)
> So whatever things get origin would work on some pages but not others?  I
> guess that's survivable depending on how defensively the consumers of
> .origin are written.

Yeah. The issue is that, if we want |a.equals(b) i.f.f. a.origin === b.origin|, we have to do this, I think.

So Michael, what do you think about doing that instead? It's probably a 1-line change, unless throwing breaks any of the consumers.

Comment 14

[Nika Layzell [:nika] (ni? for response)]

Why would this be preferable to always adding the ! at the end? I'll look into what the impact is at some point, but I don't have the time to do it just at the moment.

I think that the adding a ! is probably less likely to break things, because there are likely to be people who want a unique identifier for a principal which isn't necessarily well-behaved.

Comment 15

[Bobby Holley (:bholley)]

(In reply to Michael Layzell [:mystor] from comment #14)
> Why would this be preferable to always adding the ! at the end? I'll look
> into what the impact is at some point, but I don't have the time to do it
> just at the moment.
> 
> I think that the adding a ! is probably less likely to break things, because
> there are likely to be people who want a unique identifier for a principal
> which isn't necessarily well-behaved.

The problem is that, in the general case, non-URL URIs _cannot_ have a unique string identifier, because they are considered same-origin only if the pointers compare equal. See: https://hg.mozilla.org/mozilla-central/annotate/c063c543ebc2/netwerk/base/nsNetUtil.h#l2315

So very fundamentally, we can't provide the invariant that consumers of .origin want, no matter how we munge the strings.

Comment 16

[Nika Layzell [:nika] (ni? for response)]

Attachment: Throw when requesting origin for poorly behaved URIs

I took a shot at throwing it together, but I haven't put any effort into testing it yet.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=93da86540b89

Comment 17

[Bobby Holley (:bholley)]

Comment on attachment 8626843 [details] [diff] [review]
Throw when requesting origin for poorly behaved URIs

Review of attachment 8626843 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the lag.

::: caps/nsPrincipal.cpp
@@ +124,5 @@
>        rv = NS_ERROR_FAILURE;
>      }
>    }
>  
> +  bool isBehaved;

This needs extensive commenting explaining why we're doing this, and the reasoning of why each of these is safe.

@@ +158,5 @@
> +    } else {
> +      nsAutoCString spec;
> +      origin->GetAsciiSpec(spec);
> +      return NS_ERROR_FAILURE;
> +    }

So, the aim of this is to be defensive against existing callers that might not be checking the return value, right? I think it's actually safer to just do aOrigin.Truncate() in that case, rather than feeding them data that we don't think is valid. And while we're at it, we might as well truncate the origin at the top of the method.

So this part would boil down to:
NS_ENSURE_TRUE(standardURL, NS_ERROR_FAILURE);
rv = origin->GetAsciiSpec(aOrigin);
NS_ENSURE_SUCCESS(rv, rv);

With comments explaining what's going on, of course.

Comment 18

[Nika Layzell [:nika] (ni? for response)]

I actually would like to make sure that we are actually only allowing well behaved URIs. I'm not sure if the restrictions I currently have in place are sufficient to get around this problem. If I remember correctly, a '!' is still a valid character in, for example, a file:// URI, which means that the path can still be spoofed after my patch. This patch only really deals with bug 1172022. I actually think we'd still need to change from a ! to a ^, so that it is illegal in the URIs we are using.

We could also perform a search for an ! when we get the origin URI's spec, and if there is one present, we insert a ! at the end? Sounds super sketchy, but may work without breaking any consumers. We'd still want to do the throwing to keep the equality invariant.

Comment 20

[Bobby Holley (:bholley)]

With your patch, we'll reject file:// URIs, since they're not nsIStandardURL right? I think that's the correct approach - the origin semantics of file:// are not very well-defined for the purposes of most callers, and I'm guessing they're going to get it wrong.

Jonas specifically wants a character that is a valid filename character, so that it's more easy to serialize into indexedDB filenames etc.

Comment 21

[Boris Zbarsky [:bzbarsky]]

file:// URIs are nsIStandardURL last I checked.

Comment 22

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #21)
> file:// URIs are nsIStandardURL last I checked.

Ah, ok. Regardless, we could certainly forbid them explicitly - what do people think about that?

From my testing, .origin on file:// URI principals will currently just give the full path (i.e. it doesn't match the per-directory same-origin-policy we enforce). So I doubt we'd break much by doing that, and we might root out a few bugs in the process.

Comment 23

[Nika Layzell [:nika] (ni? for response)]

With my patch, file:// URIs work fine.

 > let uri = makeURI("file:///some/file/path");
undefined
 > let principal = Services.scriptSecurityManager.getNoAppCodebasePrincipal(uri);
undefined
 > principal.origin
"file:///some/file/path"

And you can spoof too
 > let uri = makeURI("file:///some/file/path!appId=1000&inBrowser=1");
undefined
 > let principal = Services.scriptSecurityManager.getNoAppCodebasePrincipal(uri);
undefined
 > principal.origin
"file:///some/file/path!appId=1000&inBrowser=1"

You just can't do stuff like this:
 > let uri = makeURI("mailto:[e-mail]");
undefined
 > let principal = Services.scriptSecurityManager.getNoAppCodebasePrincipal(uri);
undefined
 > principal.origin
[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIPrincipal.origin]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: debugger eval code :: <TOP_LEVEL> :: line 1"  data: no]

On another note, we can't break origins with file:// URIs without also breaking everything files related to my patches in bug 1165263, because permissions on file URIs is pretty important, and currently origins match the semantics for file:// URIs which the permissions manager wants.

Comment 24

[Bobby Holley (:bholley)]

(In reply to Michael Layzell [:mystor] from comment #23)
> With my patch, file:// URIs work fine.

Comment 21 would explain that.

> On another note, we can't break origins with file:// URIs without also
> breaking everything files related to my patches in bug 1165263, because
> permissions on file URIs is pretty important, and currently origins match
> the semantics for file:// URIs which the permissions manager wants.

Does this mean that the permission manager wants per-file permissions? That seems odd, because it doesn't match the same-origin policy we apply to file:// (per-directory, to a first approximation).

Comment 25

[Bobby Holley (:bholley)]

Hm, though actually it looks like NS_SecurityCompareURIs does per-file:

http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsNetUtil.h#2286

Boris, am I just imagining the per-directory thing?

Comment 26

[Boris Zbarsky [:bzbarsky]]

The per-directory thing is implemented via two things:

1)  nsPrincipal::CheckMayLoad implements it, effectively treating such loads as
    same-origin.  this allows XHR to your own subdirectories ad whatnot.
2)  A file:// document load started by another file:// document that's in the same or
    ancestor directory inherits its principal.  This is implemented via
    nsContentUtils::ChannelShouldInheritPrincipal calling CheckMayLoad for file:// URIs and
    inheriting if true.

Comment 27

[Bobby Holley (:bholley)]

Oh I see - in that case, we can sanely support file://. And we should also ignore the second part of comment 20 - "^" _is_ allowed in filenames (which was why bz chose it in comment 0).

So mystor is right: we should also replace "!" with "^". Sorry for the noise.

Comment 28

[Nika Layzell [:nika] (ni? for response)]

Yup, ^ is allowed in filenames, but is %-escaped in our URI type. I'll get around to changing that next time I'm review-blocked.

I should also probably write some tests to make sure that the types which are whitelisted obey the constraint that .Equals is equivalent to .origins being equal. They are currently whitelisted because they were causing large numbers of test failures :), rather than any useful property of them.

Comment 29

[Boris Zbarsky [:bzbarsky]]

> I'll get around to changing that next time I'm review-blocked.

No, no.  It being %-escaped is _good_.  It means that a URI can't spoof it!

At least nsStandardURL can't.  nsSimpleURI can do whatever, of course.

Comment 30

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #29)
> > I'll get around to changing that next time I'm review-blocked.
> 
> No, no.  It being %-escaped is _good_.  It means that a URI can't spoof it!

I think by "changing that" he meant "change ! to ^".

Comment 31

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Use ^ instead of ! to delimit originAttributes from the URI in nsIPrincipal.origin

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=491fe593dcd9
This is a first pass at using ^ instead of ! to delimit originAttributes from the URI. Tests seem to pass (except for crashes in Windows 8 xpcshell, which I don't _think_ is related - retrying the tests). 

This patch is built on top of my work for bug 1165263, so it will have to land after 1165263, but ideally would land at the same time. This is so that we don't have to add another migration for the permissions database to migrate from using ! for origins to ^.

Comment 32

[Bobby Holley (:bholley)]

Comment on attachment 8629484 [details] [diff] [review]
Part 2: Use ^ instead of ! to delimit originAttributes from the URI in nsIPrincipal.origin

Review of attachment 8629484 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great! Thank you. :-)

Comment 33

[Nika Layzell [:nika] (ni? for response)]

I just got the time to do some tests, and `about:`, `moz-safe-about:`, `indexeddb:` etc, are not actually well-behaved :(

makeURI("about:foo^appId=1&inBrowser=1").spec == "about:foo^appId=1&inBrowser=1";
makeURI("moz-safe-about:foo^appId=1&inBrowser=1").spec == "moz-safe-about:foo^appId=1&inBrowser=1";
makeURI("indexeddb:foo^appId=1&inBrowser=1").spec == "indexeddb:foo^appId=1&inBrowser=1";

What should I do about that? We do want to have origins defined for these URIs I think, but we can't depend on the caret being OK. I could reject any of these URIs whose spec contains a caret character? But that seems sketchy :(

I could always include an `^appId=??` for these URIs as well, but that also feels sketchy.

Any thoughts as to what a good next step would be? (The reason why we care about about: moz-safe-about: and indexeddb: is because those URIs are URIs which currently are having their specs taken during tests).

Comment 34

[Bobby Holley (:bholley)]

All of these URIs have a very limited number of valid values, all of which are under our control, right? I'm fine with just throwing if we ever try to make a principal for one of those schemes with a caret in it.

Comment 35

[Nika Layzell [:nika] (ni? for response)]

(In reply to Bobby Holley (:bholley) from comment #34)
> All of these URIs have a very limited number of valid values, all of which
> are under our control, right? I'm fine with just throwing if we ever try to
> make a principal for one of those schemes with a caret in it.

Should I throw when I make the principal, or when I make the origin? The patch in part 1 only throws when you actually request the origin, not when you make the principal. Should I add the code there, or add a different block of code to verify that there are no carets elsewhere?

Comment 36

[Bobby Holley (:bholley)]

Doing it when the origin is requested is fine, I guess, given that consumers of that already need to be written defensively.

Comment 37

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Throw when requesting origin for poorly behaved URIs

I added a bunch of explanatory comments, and am now checking for a '^' character in the non-nsIStandardURL schemes.

Who would the right person to contact be to confirm that a "no '^' in indexeddb:" restriction is acceptable?

Comment 38

[Bobby Holley (:bholley)]

Comment on attachment 8630137 [details] [diff] [review]
Part 1: Throw when requesting origin for poorly behaved URIs

Review of attachment 8630137 [details] [diff] [review]:
-----------------------------------------------------------------

Ask janv about the indexedDB stuff.

::: caps/nsPrincipal.cpp
@@ +124,5 @@
>        rv = NS_ERROR_FAILURE;
>      }
>    }
>  
> +  // We check to see if we are looking at a "well behaved" scheme. Some schemes

Nit: "well-behaved" and "well-defined" should have hyphens, here and below.

@@ +128,5 @@
> +  // We check to see if we are looking at a "well behaved" scheme. Some schemes
> +  // (such as mailto:) don't have a well-defined concept of an origin, which means
> +  // that we cannot produce an origin for them. All nsIStandardURLs have well-defined
> +  // origin meanings, but some schemes which are not nsIStandardURL still have well defined
> +  // origin meaning as well. These are checked for here, and handled seperately.

I think this comment isn't a clear as it could be. Here's a sketch of a new one:

We want the invariant that prinA.origin == prinB.origin i.f.f. prinA.equals(prinB). However, this requires that we impose certain constraints on the behavior and origin semantics of principals, and in particular, forbid creating origin strings principals whose equality constraints are not expressible as strings (i.e. object equality). Moreover, we want to forbid URIs containing the magic "^" we use as a separating character for origin attributes.

These constraints can generally be achieved by restricting .origin to nsIStandardURL-based URIs, but there are a few other URI schemes that we need to handle.

Comment 39

[Nika Layzell [:nika] (ni? for response)]

(In reply to Bobby Holley (:bholley) from comment #38)
> 
> Ask janv about the indexedDB stuff.

Hey janv, we want to use a ^ as a character to separate origin attributes from the spec of the origin URI. Right now, we seem to be using these origins for indexeddb URIs.

The patch I have here right now, which is intended to prevent spoofing of these origin attributes, prevents you from creating origins for indexeddb: URIs with a '^' character in them. Is this an unacceptable restriction?

> I think this comment isn't a clear as it could be. Here's a sketch of a new
> one:
> 
> We want the invariant that prinA.origin == prinB.origin i.f.f.
> prinA.equals(prinB). However, this requires that we impose certain
> constraints on the behavior and origin semantics of principals, and in
> particular, forbid creating origin strings principals whose equality
> constraints are not expressible as strings (i.e. object equality). Moreover,
> we want to forbid URIs containing the magic "^" we use as a separating
> character for origin attributes.
> 
> These constraints can generally be achieved by restricting .origin to
> nsIStandardURL-based URIs, but there are a few other URI schemes that we
> need to handle.

I agree that that is considerably more eloquent than what I wrote. I'm going to use that :)

Comment 40

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Throw when requesting origin for poorly behaved URIs

Better comments (thanks to bholley)

Comment 41

[Bobby Holley (:bholley)]

Comment on attachment 8630602 [details] [diff] [review]
Part 1: Throw when requesting origin for poorly behaved URIs

Review of attachment 8630602 [details] [diff] [review]:
-----------------------------------------------------------------

::: caps/nsPrincipal.cpp
@@ +127,5 @@
>  
> +  // We want the invariant that prinA.origin == prinB.origin i.f.f.
> +  // prinA.equals(prinB). However, this requires that we impose certain constraints
> +  // on the behavior and origin semantics of principals, and in particular, forbid
> +  // creating origin strings principals whose equality constraints are not

origin strings _for_ principals

@@ +131,5 @@
> +  // creating origin strings principals whose equality constraints are not
> +  // expressible as strings (i.e. object equality). Moreover, we want to forbid URIs
> +  // containing the magic "^" we use as a separating character for origin
> +  // attributes.
> +

uber-nit: Add // here to indicate that it's a single comment.

Comment 42

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Throw when requesting origin for poorly behaved URIs

Fixed some nits

Comment 43

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ff004760a1f
https://hg.mozilla.org/integration/mozilla-inbound/rev/84fe04b2e7d1

Comment 44

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

This (and everything else from ehsan's push) backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/de3fb216f066 for mass android reftest bustage:

https://treeherder.mozilla.org/logviewer.html#?job_id=11676048&repo=mozilla-inbound

Comment 45

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3386b7f0af78
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb346b9b9f98

Comment 46

[Nika Layzell [:nika] (ni? for response)]

fixed patch is on inbound

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3386b7f0af78
https://hg.mozilla.org/mozilla-central/rev/fb346b9b9f98

Comment 48

[Jan Varga [:janv]]

(In reply to Michael Layzell [:mystor] from comment #39)
> (In reply to Bobby Holley (:bholley) from comment #38)
> > 
> > Ask janv about the indexedDB stuff.
> 
> Hey janv, we want to use a ^ as a character to separate origin attributes
> from the spec of the origin URI. Right now, we seem to be using these
> origins for indexeddb URIs.
> 
> The patch I have here right now, which is intended to prevent spoofing of
> these origin attributes, prevents you from creating origins for indexeddb:
> URIs with a '^' character in them. Is this an unacceptable restriction?
> 

I think it shouldn't be a problem. However I hope .originSuffix will never contain the special characters declared here:
http://mxr.mozilla.org/mozilla-central/source/dom/quota/QuotaManager.cpp?rev=f96332d03ecd#773

Comment 49

[Nika Layzell [:nika] (ni? for response)]

(In reply to Jan Varga [:janv] from comment #48)
> 
> I think it shouldn't be a problem. However I hope .originSuffix will never
> contain the special characters declared here:
> http://mxr.mozilla.org/mozilla-central/source/dom/quota/QuotaManager.
> cpp?rev=f96332d03ecd#773

I don't believe it contains any of those, but bholley would know better.

Comment 50

[Bobby Holley (:bholley)]

(In reply to Michael Layzell [:mystor] from comment #49)
> (In reply to Jan Varga [:janv] from comment #48)
> > 
> > I think it shouldn't be a problem. However I hope .originSuffix will never
> > contain the special characters declared here:
> > http://mxr.mozilla.org/mozilla-central/source/dom/quota/QuotaManager.
> > cpp?rev=f96332d03ecd#773
> 
> I don't believe it contains any of those, but bholley would know better.

It shouldn't, so long as we don't do something dumb, but there's technically nothing that would prevent it in the case of string-valued attributes. I'll add some code to catch that.