Last Comment Bug 413161 - nsIPrincipal needs a stricter origin
: nsIPrincipal needs a stricter origin
Status: VERIFIED FIXED
[sg:nse] needed for bug 408329
: verified1.8.1.15
Product: Core
Classification: Components
Component: Security: CAPS (show other bugs)
: unspecified
: All All
: P2 normal (vote)
: mozilla1.9
Assigned To: Jonas Sicking (:sicking) PTO Until July 5th
:
Mentors:
Depends on: 437723
Blocks: JEP/caps CVE-2008-2806
  Show dependency treegraph
 
Reported: 2008-01-19 13:14 PST by Daniel Veditz [:dveditz]
Modified: 2009-01-05 11:55 PST (History)
12 users (show)
pavlov: blocking1.9+
samuel.sidler+old: blocking1.8.1.12-
dveditz: blocking1.8.1.13-
dveditz: blocking1.8.1.15+
dveditz: wanted1.8.1.x+
asac: blocking1.8.0.next?
asac: wanted1.8.0.x?
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to fix (11.44 KB, patch)
2008-03-17 20:15 PDT, Jonas Sicking (:sicking) PTO Until July 5th
dveditz: review+
dveditz: superreview+
Details | Diff | Splinter Review
Branch patch (10.50 KB, patch)
2008-05-30 16:20 PDT, Jonas Sicking (:sicking) PTO Until July 5th
dveditz: review+
Details | Diff | Splinter Review
wrong patch, ignore (11.13 KB, patch)
2008-06-05 12:17 PDT, Daniel Veditz [:dveditz]
no flags Details | Diff | Splinter Review
updated to de-nest jars (11.11 KB, patch)
2008-06-05 12:21 PDT, Daniel Veditz [:dveditz]
jonas: review+
bzbarsky: superreview+
dveditz: approval1.8.1.15+
Details | Diff | Splinter Review

Description Daniel Veditz [:dveditz] 2008-01-19 13:14:18 PST
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) 2008-01-28 11:25:06 PST
Pushing to next release.
Comment 2 Mike Schroepfer 2008-01-29 21:49:28 PST
Setting TM to Beta4 - we need to get this in next beta
Comment 3 Mike Schroepfer 2008-02-11 20:11:28 PST
DVeditz you have something cooking for this?
Comment 4 Mike Beltzner [:beltzner, not reading bugmail] 2008-02-27 14:51:30 PST
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] 2008-03-09 23:01:13 PDT
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, jst@mozilla.com) 2008-03-14 16:59:18 PDT
Jonas has ideas here, reassigning.
Comment 7 Jonas Sicking (:sicking) PTO Until July 5th 2008-03-17 20:15:34 PDT
Created attachment 310148 [details] [diff] [review]
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 [:bz] 2008-03-17 20:29:33 PDT
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) PTO Until July 5th 2008-03-18 01:42:10 PDT
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] 2008-03-18 01:57:43 PDT
Comment on attachment 310148 [details] [diff] [review]
Patch to fix

r/sr=dveditz
Comment 11 Boris Zbarsky [:bz] 2008-03-18 06:59:34 PDT
Yeah, I agree that if we cache we should cache the string.
Comment 12 Jonas Sicking (:sicking) PTO Until July 5th 2008-03-18 18:20:04 PDT
Checked in
Comment 13 Jeff Walden [:Waldo] (remove +bmo to email) 2008-04-05 23:56:19 PDT
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 [:bz] 2008-04-06 08:20:25 PDT
> 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] (remove +bmo to email) 2008-04-06 10:04:19 PDT
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] (remove +bmo to email) 2008-04-06 10:07:43 PDT
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) 2008-04-25 12:13:34 PDT
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) PTO Until July 5th 2008-04-29 02:00:46 PDT
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) 2008-04-29 08:19:15 PDT
> 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) 2008-05-30 11:48:52 PDT
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) PTO Until July 5th 2008-05-30 16:20:31 PDT
Created attachment 323166 [details] [diff] [review]
Branch patch

This should do it for the branch
Comment 22 Jonas Sicking (:sicking) PTO Until July 5th 2008-05-30 16:34:01 PDT
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] 2008-06-03 14:08:53 PDT
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] 2008-06-04 00:00:53 PDT
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 [:bz] 2008-06-04 23:02:07 PDT
> 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 [:bz] 2008-06-04 23:03:33 PDT
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) PTO Until July 5th 2008-06-05 01:38:12 PDT
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 [:bz] 2008-06-05 10:19:56 PDT
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] 2008-06-05 12:17:09 PDT
Created attachment 323905 [details] [diff] [review]
wrong patch, ignore

Boris and/or Jonas: does this look good?
Comment 30 Daniel Veditz [:dveditz] 2008-06-05 12:21:10 PDT
Created attachment 323906 [details] [diff] [review]
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 [:bz] 2008-06-05 12:58:19 PDT
Comment on attachment 323906 [details] [diff] [review]
updated to de-nest jars

sr=bzbarsky
Comment 32 Daniel Veditz [:dveditz] 2008-06-05 14:49:55 PDT
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] 2008-06-06 17:16:37 PDT
fix checked in to the 1.8 branch
Comment 34 Boris Zbarsky [:bz] 2008-06-06 19:14:11 PDT
I filed bug 437723 to restore the nested URI line on trunk.
Comment 35 Al Billings [:abillings] 2008-06-10 16:22:08 PDT
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) PTO Until July 5th 2008-06-10 16:41:26 PDT
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] 2008-06-11 15:45:51 PDT
The code is checked into the 1.8 branch. I double-checked and it is there.

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