nsIPrincipal needs a stricter origin

VERIFIED FIXED in mozilla1.9

Status

()

Core
Security: CAPS
P2
normal
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: dveditz, Assigned: sicking)

Tracking

({verified1.8.1.15})

unspecified
mozilla1.9
verified1.8.1.15
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
blocking1.8.1.12 -
blocking1.8.1.13 -
blocking1.8.1.15 +
wanted1.8.1.x +
blocking1.8.0.next ?
wanted1.8.0.x ?
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:nse] needed for bug 408329)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

9 years ago
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
(Reporter)

Updated

9 years ago
Whiteboard: [sg:nse]
(Reporter)

Updated

9 years ago
Flags: wanted1.8.1.x+
Flags: blocking1.9?
Flags: blocking1.8.1.12+
Whiteboard: [sg:nse] → [sg:nse] needed for bug 408329

Updated

9 years ago
Flags: blocking1.9? → blocking1.9+
Priority: -- → P1
Pushing to next release.
Flags: blocking1.8.1.13+
Flags: blocking1.8.1.12-
Flags: blocking1.8.1.12+

Comment 2

9 years ago
Setting TM to Beta4 - we need to get this in next beta
Target Milestone: --- → mozilla1.9beta4

Comment 3

9 years ago
DVeditz you have something cooking for this?
Whiteboard: [sg:nse] needed for bug 408329 → [sg:nse][needs patch] needed for bug 408329
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.
Target Milestone: mozilla1.9beta4 → mozilla1.9

Updated

9 years ago
Flags: tracking1.9+ → blocking1.9+
Priority: P1 → P2
(Reporter)

Comment 5

9 years ago
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.
Flags: blocking1.8.1.14+
Flags: blocking1.8.1.13-
Flags: blocking1.8.1.13+
(Reporter)

Updated

9 years ago
Blocks: 293973
Jonas has ideas here, reassigning.
Assignee: dveditz → jonas
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.
Attachment #310148 - Flags: superreview?(dveditz)
Attachment #310148 - Flags: review?(dveditz)
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.
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.
(Reporter)

Comment 10

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

r/sr=dveditz
Attachment #310148 - Flags: superreview?(dveditz)
Attachment #310148 - Flags: superreview+
Attachment #310148 - Flags: review?(dveditz)
Attachment #310148 - Flags: review+
Yeah, I agree that if we cache we should cache the string.
Checked in
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
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...
Flags: in-testsuite-
> 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?
Flags: in-testsuite- → in-testsuite?
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.
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...
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().
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...
> 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.
Jonas, are you going to have time to make a branch patch for this? We're hoping to freeze in a week...
Whiteboard: [sg:nse][needs patch] needed for bug 408329 → [sg:nse][needs branch patch] needed for bug 408329
Created attachment 323166 [details] [diff] [review]
Branch patch

This should do it for the branch
Attachment #323166 - Flags: superreview?(dveditz)
Attachment #323166 - Flags: review?(dveditz)
Whiteboard: [sg:nse][needs branch patch] needed for bug 408329 → [sg:nse][needs r/sr dveditz] needed for bug 408329
I won't be here next week, so if someone can land as soon as it's reviewed and approved that would rock!
(Reporter)

Comment 23

9 years ago
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.
Attachment #323166 - Flags: superreview?(dveditz)
Attachment #323166 - Flags: review?(dveditz)
Attachment #323166 - Flags: review-
(Reporter)

Comment 24

9 years ago
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
Attachment #323166 - Flags: superreview?(bzbarsky)
Attachment #323166 - Flags: review-
Attachment #323166 - Flags: review+
Whiteboard: [sg:nse][needs r/sr dveditz] needed for bug 408329 → [sg:nse][needs sr bz] needed for bug 408329
> 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.
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.
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.
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.
Whiteboard: [sg:nse][needs sr bz] needed for bug 408329 → [sg:nse][needs respin] needed for bug 408329
(Reporter)

Comment 29

9 years ago
Created attachment 323905 [details] [diff] [review]
wrong patch, ignore

Boris and/or Jonas: does this look good?
Attachment #323166 - Attachment is obsolete: true
Attachment #323905 - Flags: superreview?(bzbarsky)
Attachment #323905 - Flags: review?(jonas)
Attachment #323166 - Flags: superreview?(bzbarsky)
(Reporter)

Comment 30

9 years ago
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.
Attachment #323905 - Attachment is obsolete: true
Attachment #323906 - Flags: superreview?(bzbarsky)
Attachment #323906 - Flags: review?(jonas)
Attachment #323905 - Flags: superreview?(bzbarsky)
Attachment #323905 - Flags: review?(jonas)
(Reporter)

Updated

9 years ago
Attachment #323905 - Attachment description: updated to de-nest jars → wrong patch, ignore
Comment on attachment 323906 [details] [diff] [review]
updated to de-nest jars

sr=bzbarsky
Attachment #323906 - Flags: superreview?(bzbarsky) → superreview+
Whiteboard: [sg:nse][needs respin] needed for bug 408329 → [sg:nse][needs r sicking] needed for bug 408329
(Reporter)

Comment 32

9 years ago
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
Attachment #323906 - Flags: approval1.8.1.15+
(Reporter)

Updated

9 years ago
Whiteboard: [sg:nse][needs r sicking] needed for bug 408329 → [sg:nse] needed for bug 408329
(Reporter)

Comment 33

9 years ago
fix checked in to the 1.8 branch
Keywords: fixed1.8.1.15
I filed bug 437723 to restore the nested URI line on trunk.
Depends on: 437723
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.
I don't think there is much that needs to be verified in this bug. Code inspection should be enough.
Status: RESOLVED → VERIFIED
The code is checked into the 1.8 branch. I double-checked and it is there.
Keywords: fixed1.8.1.15 → verified1.8.1.15
Attachment #323906 - Flags: review?(jonas) → review+
(Reporter)

Updated

9 years ago
Group: security

Updated

8 years ago
Flags: wanted1.8.0.x?
Flags: blocking1.8.0.next?
You need to log in before you can comment on or make changes to this bug.