Closed Bug 327244 Opened 14 years ago Closed 7 years ago

Eliminate CheckLoadURI() and CheckLoadURIStr()

Categories

(Core :: Security: CAPS, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: bzbarsky, Assigned: mounir)

References

(Blocks 1 open bug)

Details

(Keywords: addon-compat, dev-doc-needed)

Attachments

(2 files, 1 obsolete file)

Callers should just use CheckLoadURIWithPrincipal() instead.  As things stand, we do weird things when a document URI doesn't match the principal's URI, for example.

To fix this we need to make the relevant CAPS stuff scriptable (bug 327242) and then expose nsIPrincipal for documents, nodes, and possibly browsers (sorta the same problem as bug 324464).
Blocks: clu
Blocks: 253761
Blocks: JEP/caps
Blocks: 330102
Depends on: 342484
Depends on: 342485
Depends on: 342486
Depends on: 342487
Depends on: 342489
Depends on: 348559
Flags: blocking1.9?
No longer blocks: 330102
Depends on: 370719
Flags: blocking1.9? → blocking1.9-
Whiteboard: [wanted-1.9]
Depends on: 408328
Flags: wanted1.9+
Whiteboard: [wanted-1.9]
QA Contact: caps
Attached patch Remove CheckLoadURIStr() (obsolete) — Splinter Review
Basic patch that change the two remaining calls of CheckLoadURIStr() to use CheckLoadURI() by simply creating URIs from the strings.
Assignee: dveditz → mounir
Status: NEW → ASSIGNED
Attachment #642852 - Flags: review?(jonas)
This is now using CheckLoadURI{Str,}WithPrincipal instead.
Attachment #642852 - Attachment is obsolete: true
Attachment #642852 - Flags: review?(jonas)
Attachment #642858 - Flags: review?(jonas)
Justin, can you review nsDocShell changes?
Attachment #642864 - Flags: review?(justin.lebar+bug)
Attachment #642864 - Flags: review?(jonas)
Blocks: 758258
Comment on attachment 642852 [details] [diff] [review]
Remove CheckLoadURIStr()

Review of attachment 642852 [details] [diff] [review]:
-----------------------------------------------------------------

::: caps/idl/nsIScriptSecurityManager.idl
@@ -117,5 @@
> -     * by scripts
> -     *
> -     * @deprecated Use checkLoadURIStrWithPrincipal instead of this function.
> -     */
> -    [deprecated] void checkLoadURIStr(in AUTF8String from, in AUTF8String uri,

Update the iid

::: content/base/src/contentAreaDropListener.js
@@ +89,5 @@
>        secMan.checkLoadURIStrWithPrincipal(sourceNode.nodePrincipal, uriString, flags);
> +    } else {
> +      secMan.checkLoadURI(ioService.newURI("file:///", null, null),
> +                          ioService.newURI(uriString, null, null),
> +                          flags);

Why not go all the way and create a principal and use checkLoadURIWithPrincipal here?

::: toolkit/content/nsDragAndDrop.js
@@ +591,5 @@
>        var sourceURI = sourceDoc ? sourceDoc.documentURI : "file:///";
>  
>        try {
> +        secMan.checkLoadURI(ioService.newURI(asourceURI, null, null),
> +                            draggedURI, nsIScriptSecurityManager.STANDARD);

You want to use checkLoadURIWithPrincipal here, and if sourceDoc is non-null use sourceDoc.nodePrincipal.

and 'asourceURI' should be 'sourceURI', no?
Comment on attachment 642864 [details] [diff] [review]
Remove CheckLoadURI()

Review of attachment 642864 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/base/nsIRefreshURI.idl
@@ +56,5 @@
>        *
>        * @param aBaseURI base URI to resolve refresh uri with.
>        * @param aHeader  The meta refresh header string.
>        */
> +    void setupRefreshURIFromHeader(in nsIURI aBaseURI, in nsIPrincipal principal, in ACString aHeader);

Update the documentation.
Attachment #642864 - Flags: review?(jonas) → review+
>@@ -5765,18 +5767,25 @@ NS_IMETHODIMP nsDocShell::SetupRefreshUR
>     nsresult rv;
>     nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(aChannel, &rv));
>     if (NS_SUCCEEDED(rv)) {
>         nsCAutoString refreshHeader;
>         rv = httpChannel->GetResponseHeader(NS_LITERAL_CSTRING("refresh"),
>                                             refreshHeader);
> 
>         if (!refreshHeader.IsEmpty()) {
>+            nsCOMPtr<nsIScriptSecurityManager> secMan =
>+                do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv);
>+            NS_ENSURE_SUCCESS(rv, rv);
>+
>+            nsCOMPtr<nsIPrincipal> principal;
>+            secMan->GetChannelPrincipal(aChannel, getter_AddRefs(principal));
>+
>             SetupReferrerFromChannel(aChannel);
>-            rv = SetupRefreshURIFromHeader(mCurrentURI, refreshHeader);
>+            rv = SetupRefreshURIFromHeader(mCurrentURI, principal, refreshHeader);

If GetChannelPrincipal fails, we'll crash in SetupRefreshURIFromHeader.  I'm
not familiar with it, but looking at the code, it seems quite possible that the
call could fail.

>+++ b/docshell/base/nsIRefreshURI.idl

Update IID here too.
Attachment #642864 - Flags: review?(justin.lebar+bug) → review+
Attachment #642858 - Flags: checkin+
Attachment #642864 - Flags: checkin+
Flags: in-testsuite-
OS: Linux → All
Hardware: x86 → All
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.