327244 - Eliminate CheckLoadURI() and CheckLoadURIStr()

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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).

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Remove CheckLoadURIStr()

Basic patch that change the two remaining calls of CheckLoadURIStr() to use CheckLoadURI() by simply creating URIs from the strings.

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: Remove CheckLoadURIStr()

This is now using CheckLoadURI{Str,}WithPrincipal instead.

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: Remove CheckLoadURI()

Justin, can you review nsDocShell changes?

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 6

[Justin Lebar (not reading bugmail)]

>@@ -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.

Comment 7

[Mounir Lamouri (:mounir)]

try: https://tbpl.mozilla.org/?tree=Try&rev=d00fd7009533

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/913bd4954d7b
https://hg.mozilla.org/mozilla-central/rev/34fbd30f4f08