233108 - [FIX]Make checkLoadURI configurable by-site

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

I want to make checkLoadURI configurable per-site.  The patch I posted in bug
84128 comment 194 (dear God) does that, more or less.  The one issue is that it
relies on converting the URI to a principal.

Now I expect that a number of callers already have a principal.  So I propose
the following setup:

1)  Add a noscript checkLoadURI that takes a principal
2)  Make this be the main impl
3)  Have the URI version create a principal from the uri and call the other
    version.

This will leave JS code calling CheckLoadURI a little less efficient, but not
much and will improve efficiency (and correctness, since there is more to a
principal than just a URI) of other code that calls this function.

Seem reasonable, caillon?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so

Comment 2

[Ian Neal]

*** Bug 135830 has been marked as a duplicate of this bug. ***

Comment 3

[Boris Zbarsky [:bzbarsky]]

Mental note.  The change to nsDocument::SetBaseURI needs to call GetPrincipal(),
not just use mPrincipal, since the principal is generated lazily.  Please let me
know if you want an updated patch with that fixed.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Caillon?  Any chance of an ETA on the review?

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

(In reply to comment #3)
> Mental note.  The change to nsDocument::SetBaseURI needs to call GetPrincipal(),
> not just use mPrincipal, since the principal is generated lazily.  Please let me
> know if you want an updated patch with that fixed.

Please do.  Additionally, if you could split this up into "API changes vs
capability changes", that would really be nice.  I agree with all three proposed
API changes outlined in comment 0, but need some time to think about the
checkLoadURI being configurable by capability policies portion.  In particular
(for my personal notes) I want to re-hash how our capability things work, and
how well they mesh with an eventual security zone policy.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Just the API change

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Behavior change (to be applied on top of previous patch)

Well, that was painful.  I need to land some of this crap that's all over my
tree.... :(

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 146727 [details] [diff] [review]
Just the API change

> NS_IMETHODIMP
> nsScriptSecurityManager::CheckLoadURI(nsIURI *aSourceURI, nsIURI *aTargetURI,
>                                       PRUint32 aFlags)
> {
>+    NS_PRECONDITION(aSourceURI, "CheckLoadURI called with null source URI");
>+    if (!aSourceURI) {
>+        return NS_ERROR_UNEXPECTED;
>+    }

NS_ENSURE_ARG_POINTER

>+    
>+    nsCOMPtr<nsIPrincipal> sourcePrincipal;
>+    nsresult rv = CreateCodebasePrincipal(aSourceURI,
>+                                          getter_AddRefs(sourcePrincipal));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    return CheckLoadURIWithPrincipal(sourcePrincipal, aTargetURI, aFlags);
>+}
>+
>+NS_IMETHODIMP
>+nsScriptSecurityManager::CheckLoadURIWithPrincipal(nsIPrincipal* aPrincipal,
>+                                                   nsIURI *aTargetURI,
>+                                                   PRUint32 aFlags)
>+{
>+    NS_PRECONDITION(aPrincipal, "CheckLoadURIWithPrincipal must have a principal");
>+    if (!aPrincipal) {
>+        return NS_ERROR_UNEXPECTED;
>+    }

NS_ENSURE_ARG_POINTER

>+
>+    if (aPrincipal == mSystemPrincipal) {
>+        // Allow access
>+        return NS_OK;
>+    }
>+    
>+    nsCOMPtr<nsIURI> sourceURI;
>+    aPrincipal->GetURI(getter_AddRefs(sourceURI));
>+
>+    NS_ASSERTION(sourceURI, "Principals passed to CheckLoadURIWithPrincipal must have a URI!");

The assertion text is wrong, or your code is.  System principals don't have a
URI....

>+    
>     nsresult rv;
>     //-- get the source scheme
>     nsXPIDLCString sourceScheme;
>-    rv = GetBaseURIScheme(aSourceURI, getter_Copies(sourceScheme));
>+    rv = GetBaseURIScheme(sourceURI, getter_Copies(sourceScheme));

|nsresult rv = GetBase....|



>Index: content/xsl/public/nsIDocumentTransformer.h


>   NS_IMETHOD LoadStyleSheet(nsIURI* aUri, nsILoadGroup* aLoadGroup,
>-                            nsIURI* aReferrerUri) = 0;
>+                            nsIPrincipal* aCallerPrincipal) = 0;

Would "aDocumentPrincipal" be a better name here (and in subsequent places)? 
Otherwise, just aPrincipal will work.  Obviously if its passed in, its from a
caller....


>Index: content/xbl/src/nsXBLService.cpp
>+  // XXXbz wouldn't anything with a chrome URI get the system principal?

No.  Bug 160042.


r=caillon

Comment 9

[Boris Zbarsky [:bzbarsky]]

> NS_ENSURE_ARG_POINTER

Will do.

> The assertion text is wrong, or your code is.  System principals don't have a
> URI....

Are there system principals other than mSystemPrincipal?  This assert comes
after the early bail if aPrincipal == mSystemPrincipal, no?  I can change the
text of the assert to say "non-system principals" if you prefer.

> |nsresult rv = GetBase....|

Will do.

> Would "aDocumentPrincipal" be a better name here (and in subsequent places)? 

I see nothing in the API restricting this to documents, and it would be unclear
which document.

> Obviously if its passed in, its from a caller....

The point is that the principal identifies the caller.  As in, it tells us who
is calling us.  aPrincipal could be a principal the caller wants us to use
somewhere or a whole slew of other things.  I can change this if you want, but I
did spend a good amount of time picking this name, and I think it's the best
compromise between aPrincipal and
aPrincipalThatWeShouldUseForAllSecurityChecks... ;)

> No.  Bug 160042.

Should I remove the comment, or just change it to reference that bug?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146727 [details] [diff] [review]
Just the API change

Dan, could you sr?

Comment 11

[Christopher Aillon (sabbatical, not receiving bugmail)]

(In reply to comment #9)
>> The assertion text is wrong, or your code is.  System principals don't have a
>> URI....
> 
> Are there system principals other than mSystemPrincipal?  This assert comes
> after the early bail if aPrincipal == mSystemPrincipal, no?  I can change the
> text of the assert to say "non-system principals" if you prefer.

Right.  I meant that the assertion that "Principals passed to
CheckLoadURIWithPrincipal must have a URI!" is misleading because it is valid
for system principals (and you are correct, there is only one system principal;
otherwise pointer comparisons would not work) to be passed in, as the code
handles it, and those do not have a URI.  So, yes, please change the wording. 
Sorry for being enigmatical.


> Should I remove the comment, or just change it to reference that bug?

Your call.  XXXbz implies you want to fix it though, so you may elect to just
discard it...  :-)

Comment 12

[Christopher Aillon (sabbatical, not receiving bugmail)]

> I think it's the best compromise between aPrincipal and
aPrincipalThatWeShouldUseForAllSecurityChecks... ;)

Well. being a principal sort of implies that we should use it for security
checks n stuff...  ;-)  I don't really care that much.  I just thought it an odd
name.  Feel free to leave it.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> So, yes, please change the wording. 

Will do.

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 146727 [details] [diff] [review]
Just the API change

Keep an eye on Tp
sr=dveditz

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146727 [details] [diff] [review]
Just the API change

Checked in; Tp looks fine.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146728 [details] [diff] [review]
Behavior change (to be applied on top of previous patch)

Moving on to what this bug is actually about...

Comment 17

[jag (Peter Annema)]

Comment on attachment 146728 [details] [diff] [review]
Behavior change (to be applied on top of previous patch)

? (char*)loadURIPrefGroup,

There should be no need to do that cast.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Ah, indeed.  That code was mostly copy/pasted from other places that access
content policy...

Comment 19

[Boris Zbarsky [:bzbarsky]]

Chris, are we likely to end up with large-scale CAPS rearch in 1.8?  If not, I
think I'd like to check in the existing patch to make this usable on intranets
without having to completely disable the security check....  The number of bug
reports I've seen where people had that check disabled altogether so they could
use Mozilla at work (4 or 5 at this point) scares me.

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

*** Bug 259285 has been marked as a duplicate of this bug. ***

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

bug 93787 seems similar to this one

Comment 22

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 146728 [details] [diff] [review]
Behavior change (to be applied on top of previous patch)

I hoped to get this done via a zone policy, but in light of that not happening,
this is reasonable.

The main thing I don't like is using something designed for checking javascript
object access for this purpose.  If someone has a classinfo which reports a
name of "checkloaduri" it will conflict.  Will that be a huge issue?  Probably
not.

r=caillon

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146728 [details] [diff] [review]
Behavior change (to be applied on top of previous patch)

jst@moz

Comment 24

[Johnny Stenback (:jst)]

Comment on attachment 146728 [details] [diff] [review]
Behavior change (to be applied on top of previous patch)

sr=jst

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to trunk

Comment 26

[Boris Zbarsky [:bzbarsky]]

Fix checked in on trunk.

Comment 27

[Serge Gautherie (:sgautherie)]

Comment on attachment 164403 [details] [diff] [review]
Merged to trunk

Shouldn't this security related enhancement be included in Mv1.7.x and FFv1.0.x
too ?

Comment 28

[Boris Zbarsky [:bzbarsky]]

The patch that approval have been requested for depends on the api changes that are 
also attached to this bug.  I am strongly against making such api changes on the api-
stable branches for an enhancement. request 

Comment 29

[Daniel Veditz [:dveditz]]

Comment on attachment 164403 [details] [diff] [review]
Merged to trunk

This is not a security hole, the only reason we'd ship a ff1.0.1 or a Moz1.7.6.
Then there's the interface changes bz mentions which drivers also don't want
any more of on the branch. Denied on driver's behalf.

Comment 30

[Serge Gautherie (:sgautherie)]

Thanks for answering my question !

Comment 31

[Jesse Ruderman]

*** Bug 312229 has been marked as a duplicate of this bug. ***

Comment 32

[jrivers]

It would be nice if the text at:

http://www.mozilla.org/releases/mozilla1.7.12/known-issues.html#psm

was updated to describe this new process, instead of the old (ineffectual) one.

That link comes up first on google when searching for "Security Error content at may not load or link to".

Comment 33

[Boris Zbarsky [:bzbarsky]]

How would that help?  The new setup doesn't work in Mozilla 1.7.12, so that would be a lie.

If you're saying the new setup should be documented, then I agree.  Is there not a devmo page about it?