Note: There are a few cases of duplicates in user autocompletion which are being worked on.

[FIX]Make checkLoadURI configurable by-site

RESOLVED FIXED in mozilla1.8alpha5

Status

()

Core
Security: CAPS
P1
normal
RESOLVED FIXED
14 years ago
11 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla1.8alpha5
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Usage examples: see bug 84128 comment 194 and 199])

Attachments

(1 attachment, 3 obsolete attachments)

1.92 KB, patch
dveditz
: approval1.7.6-
Details | Diff | Splinter Review
(Assignee)

Description

14 years ago
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?
(Assignee)

Updated

14 years ago
Blocks: 193255
(Assignee)

Comment 1

14 years ago
Created attachment 140852 [details] [diff] [review]
Like so
(Assignee)

Updated

14 years ago
Attachment #140852 - Flags: review?(caillon)

Comment 2

14 years ago
*** Bug 135830 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

14 years ago
Blocks: 135830
(Assignee)

Comment 3

14 years ago
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.
(Assignee)

Comment 4

14 years ago
Caillon?  Any chance of an ETA on the review?
Summary: Make checkLoadURI configurable by-site → [FIX]Make checkLoadURI configurable by-site
(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.
Attachment #140852 - Flags: review?(caillon)
(Assignee)

Comment 6

13 years ago
Created attachment 146727 [details] [diff] [review]
Just the API change
Attachment #140852 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #146727 - Flags: review?(caillon)
(Assignee)

Comment 7

13 years ago
Created attachment 146728 [details] [diff] [review]
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 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
Attachment #146727 - Flags: review?(caillon) → review+
(Assignee)

Comment 9

13 years ago
> 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?
(Assignee)

Comment 10

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

Dan, could you sr?
Attachment #146727 - Flags: superreview?(dveditz)
(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...  :-)
> 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.
(Assignee)

Comment 13

13 years ago
> So, yes, please change the wording. 

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

Keep an eye on Tp
sr=dveditz
Attachment #146727 - Flags: superreview?(dveditz) → superreview+
(Assignee)

Comment 15

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

Checked in; Tp looks fine.
Attachment #146727 - Attachment is obsolete: true
(Assignee)

Comment 16

13 years ago
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...
Attachment #146728 - Flags: review?(caillon)

Comment 17

13 years ago
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.
(Assignee)

Comment 18

13 years ago
Ah, indeed.  That code was mostly copy/pasted from other places that access
content policy...
(Assignee)

Comment 19

13 years ago
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.
Priority: -- → P1
Target Milestone: --- → mozilla1.8alpha2
*** Bug 259285 has been marked as a duplicate of this bug. ***
bug 93787 seems similar to this one
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
Attachment #146728 - Flags: review?(caillon) → review+
(Assignee)

Comment 23

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

jst@moz
Attachment #146728 - Flags: superreview?
(Assignee)

Updated

13 years ago
Attachment #146728 - Flags: superreview?(jst)
(Assignee)

Updated

13 years ago
Attachment #146728 - Flags: superreview?
Comment on attachment 146728 [details] [diff] [review]
Behavior change (to be applied on top of previous patch)

sr=jst
Attachment #146728 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 25

13 years ago
Created attachment 164403 [details] [diff] [review]
Merged to trunk
(Assignee)

Updated

13 years ago
Attachment #146728 - Attachment is obsolete: true
(Assignee)

Comment 26

13 years ago
Fix checked in on trunk.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.8alpha2 → mozilla1.8alpha5

Updated

13 years ago
QA Contact: benc
Whiteboard: [Usage examples: see bug 84128 comment 194 and 199]
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 ?
Attachment #164403 - Flags: approval1.7.6?
Attachment #164403 - Flags: approval-aviary1.0.1?
(Assignee)

Comment 28

13 years ago
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 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.
Attachment #164403 - Flags: approval1.7.6?
Attachment #164403 - Flags: approval1.7.6-
Attachment #164403 - Flags: approval-aviary1.0.1?
Attachment #164403 - Flags: approval-aviary1.0.1-
Thanks for answering my question !

Comment 31

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

Comment 32

11 years ago
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".
(Assignee)

Comment 33

11 years ago
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?
You need to log in before you can comment on or make changes to this bug.