413161 - nsIPrincipal needs a stricter origin

Status: VERIFIED FIXED

(Core :: Security: CAPS, defect, P2)

Description

[Daniel Veditz [:dveditz]]

nsIPrincipal's origin attribute is temptingly convenient since it returns the host as a string, adjusted for document.domain, jar: nesting and other niceties. There is no equivalent method for places that need a strict origin without accounting for document.domain and that leads code to either use the wrong form (adjusting for domain when they should not) or to try to duplicate the URI->origin code which they often get wrong (forgetting to unnest jar:, etc).

nsIPrincipal should expose a strictOrigin attribute for this purpose. I'd actually like to make "origin" the strict one and introduce a originDomain for the adjusted one so people are explicit when they choose that unfortunate historical behavior, but then that requires changing a lot more code.

marking security-sensitive because the problem statement might encourage people to dig through the code looking for places that use origin when they should not.

This is needed for bug 408329

Comment 1

[Samuel Sidler (old account; do not CC)]

Pushing to next release.

Comment 2

[Mike Schroepfer]

Setting TM to Beta4 - we need to get this in next beta

Comment 3

[Mike Schroepfer]

DVeditz you have something cooking for this?

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Not gonna block b4 on this, leaving as P1 which means requires beta exposure, change that if we think we can leave this to RC.

Comment 5

[Daniel Veditz [:dveditz]]

One option on the 1.8 branch would be to change GetOrigin() itself to no longer reflect document.domain. It's currently used only by OJI/Java and caps itself, and caps can figure out the document.domain stuff correctly when it needs to. On trunk this is pretty perf-sensitive so we may need the extra API after all so we can cache the result. Also on trunk bz wants this to return a URI rather than a string.

Comment 6

[Johnny Stenback (:jst)]

Jonas has ideas here, reassigning.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

This should do it.

I took the liberty of removing the "cache" for the mOrigin URI in nsPrincipal as we really shouldn't be calling GetOrigin enough for that to make sense. Also, that wasn't the slow part anyway so it made little sense to have it.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Hmm.  LookupPolicy is a bit of a CAPS hotspot, but I guess we cache the domain policy, which is what needs the origin....  We hit GetOrigin about 50 times loading gmail.  We hit it about 100 times during startup.  Loading http://www.cnn.com calls it about 250 times.

You're probably right that these are not that significant.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I sort of doubt that 50 or 100 QIs are going to matter loading pages the size of gmail or cnn. If we do want to cache something we should cache the resulting string, not the intermediary nsIURI.

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 310148 [details] [diff] [review]
Patch to fix

r/sr=dveditz

Comment 11

[Boris Zbarsky [:bzbarsky]]

Yeah, I agree that if we cache we should cache the string.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in

Comment 13

[Jeff Walden [:Waldo]]

Infra work with no visible impact, can't tell a difference in behavior before/after this, and API unit testing seems like more trouble than it's worth, absent evidence that problems exist...

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Infra work with no visible impact

Uh... Not at all.  This is trivially detectable by whether your java applets are exploitable, for example.  There are testcases for this bug on the web, for crying out loud.  See bug 408329, which is prominently mentioned in comment 0.

And imo API unit testing here is a must.

What exactly gave you the idea that this is an impact-less change?

Comment 15

[Jeff Walden [:Waldo]]

Comment 0 makes this out to be a problem more of what the developer modifying this stuff is likely to use, not one with visible impact in and of itself.  The test for bug 408329 might test this, but I don't see why it wouldn't be associated with that bug rather than this one (and likewise for other such bugs).

But hey, there's a reason I left a comment -- if I made a mistake, it's corrected now.

Comment 16

[Jeff Walden [:Waldo]]

Although ideally there wouldn't have been a mistake for me to make, and this would have been tested already since that's so important...

Comment 17

[Steven Michaud [:smichaud] (Retired)]

This patch (or the functional equivalent) needs to be landed soon on the 1.8 branch.  JEP 0.9.6.4 is about to be landed on the 1.8 branch, and it will continue to use nsIPrincipal::GetOrigin().

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Are we fine with making this change on branch? I'm not sure we are given that this change really is something that requires revving the iid.

I guess we could add a new interface with a GetStricterOrigin function on...

Comment 19

[Steven Michaud [:smichaud] (Retired)]

> I guess we could add a new interface with a GetStricterOrigin
> function on...

This would require a new JEP ... which I can manage.

But if it's going to happen it needs to happen _soon_ -- well before a
2.0.0.15 release.  I need to know exactly what's going to land before
I start making changes to the JEP.  In fact I'd really prefer to make
these changes (and release a new JEP) after the new interface has
landed on the branch.

Comment 20

[Samuel Sidler (old account; do not CC)]

Jonas, are you going to have time to make a branch patch for this? We're hoping to freeze in a week...

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Branch patch

This should do it for the branch

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I won't be here next week, so if someone can land as soon as it's reviewed and approved that would rock!

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 323166 [details] [diff] [review]
Branch patch

I missed things in my trunk review :-(

>-     * codebase URI otherwise. An origin is defined as:
>-     * scheme + host + port.
>+     * The origin of this principal's codebase URI.
>+     * An origin is defined as: scheme + host + port.

ObNit: This comment is not entirely correct (but not a new problem): for schemes that don't support a host this could be a full URI. (Also the port may not be part of the returned string if it's the default port, although it's correct that the port is still part of the origin itself.)

>+  if (!origin) {
>       NS_ASSERTION(mCert, "No Domain or Codebase for a non-cert principal");

"Domain or" could be taken out of the assertion now.

>   // chrome: URLs don't have a meaningful origin, so make
>   // sure we just get the full spec for them.

Here we're returning the full spec, in the new GetPrincipalDomainOrigin() there's no special-casing for chrome which means you'll get things of the form "chrome://browser". Given that's only used in LookupPolicy, EnablePrivilege, and CheckConfirmDialog it should be fine -- chrome should be privileged and not end up hitting LookupPolicy on its own origin, and if by some strange future mutation we get non-privileged chrome lumping the policy for each chrome package together will be a footprint win. EnablePrivilege and CheckConfirmDialog will again not apply at all to privileged chrome, and unsigned unprivileged chrome won't bring up those dialogs unless the hidden developer codebase pref is set.

"Unprivileged chrome" can arise if someone sneaks a document into a skin or locale package

I don't see a problem here, I just wanted to make this difference explicit in case Boris or Johnny think of something.
 
>   // XXX this should be removed in favor of the solution in
>   // bug 160042.

We wontfixed that bug so this comment can go

>+GetPrincipalDomainOrigin(nsIPrincipal* aPrincipal,
>+                         nsACString& aOrigin)
>+{
>+  aOrigin.Truncate();
>+
>+  nsCOMPtr<nsIURI> uri;
>+  aPrincipal->GetDomain(getter_AddRefs(uri));
>+  if (!uri) {
>+    aPrincipal->GetURI(getter_AddRefs(uri));
>+  }

The GetOrigin code de-nested JARs, this now does not. EnablePrivilge() and CheckConfirmDialog() aren't going to matter in practice, but that means LookupPolicy() will get the wrong origin for code inside JAR files. To the extent they're in jars in order to be signed it won't matter, but if they're just convenient libraries I think we may have broken same-origin if they need to interact with other parts of the site. Also probably affects per-site-enabling for NoScript, I haven't tested but I think you won't be able to enable script inside jars. Even less likely to affect users, the ActiveX-ClassID check uses LookupPolicy, although people who used that generally either enabled a control (like WMP) globally or not, not per site.

Since we're shipping this on trunk I'll file a separate bug with some testcases on this for 1.9.0.1, but I think I need to give the patch a r- for the branch.

Since Jonas is out I'll write up a new branch patch and find a new reviewer.

Comment 24

[Daniel Veditz [:dveditz]]

Comment on attachment 323166 [details] [diff] [review]
Branch patch

The cases I thought would fail seem to be fine, testing both the trunk and a branch build with this patch. I'd like a second opinion from BZ, but this seems to be fine, and if there aren't any known problems then I  think I prefer the branch to match the tested trunk behavior.

r=dveditz

Comment 25

[Boris Zbarsky [:bzbarsky]]

> The GetOrigin code de-nested JARs, this now does not.

That seems like a bug to me...  Ideally we'd fix it on both branch and trunk.

I guess we can take behavior identical to trunk on branch and then change it in the next branch dot release as well as in 1.9.0.1, but it seems weird to me to change behavior just to change it back.  It makes all the sense in the world to preserve the existing branch LookupPolicy behavior, I think.

Comment 26

[Boris Zbarsky [:bzbarsky]]

I should also note that the whole point of GetPrincipalDomainOrigin was to preserve the old GetOrigin behavior while changing GetOrigin to do the new thing.  So any deviation between what GetPrincipalDomainOrigin does now and what GetOrigin used to do is a clear bug.

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, I noticed the lack of jar protocol handling when I ported the patch. But I think it's safer to do what we're doing on trunk than do nothing. I guess write a new branch patch would be an ok alternative too.

Comment 28

[Boris Zbarsky [:bzbarsky]]

I think we should update the patch so it won't change the current branch behavior.

And file a trunk/1.9.0.x bug to undo the change there too.

Comment 29

[Daniel Veditz [:dveditz]]

Attachment: wrong patch, ignore

Boris and/or Jonas: does this look good?

Comment 30

[Daniel Veditz [:dveditz]]

Attachment: updated to de-nest jars

oops, uploaded a test version to try to see if the lack of de-nesting produced testable flaws. This is the real one.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 323906 [details] [diff] [review]
updated to de-nest jars

sr=bzbarsky

Comment 32

[Daniel Veditz [:dveditz]]

Comment on attachment 323906 [details] [diff] [review]
updated to de-nest jars

Since this mostly Jonas's original patch we don't really need his r=, can get it later

Approved for 1.8.1.15, a=dveditz for release-drivers

Comment 33

[Daniel Veditz [:dveditz]]

fix checked in to the 1.8 branch

Comment 34

[Boris Zbarsky [:bzbarsky]]

I filed bug 437723 to restore the nested URI line on trunk.

Comment 35

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

I used the demo in bug 408329 earlier today and verified that 408329 is fixed in branch now. Is there anything else to do for *this* bug to verify it? We have the new JEP as well, which I've verified.

Comment 36

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't think there is much that needs to be verified in this bug. Code inspection should be enough.

Comment 37

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

The code is checked into the 1.8 branch. I double-checked and it is there.