Attachment 712902 Details for Bug 838692

[patch] Named Sandboxed Navigation fix to nsDocShell

bug838692Code001.txt (text/plain), 6.77 KB, created by Bob Owen (:bobowen)

(hide)

Description:

Filename:

MIME Type:

Creator: Bob Owen (:bobowen)

Size: 6.77 KB

patch

obsolete

>diff -r bc108d2ce8d1 docshell/base/nsDocShell.cpp
>--- a/docshell/base/nsDocShell.cpp	Tue Feb 05 22:40:34 2013 -0800
>+++ b/docshell/base/nsDocShell.cpp	Mon Feb 11 17:38:34 2013 +0000
>@@ -3099,24 +3099,28 @@ nsDocShell::FindItemWithName(const PRUni
>     NS_ENSURE_ARG_POINTER(_retval);
> 
>     // If we don't find one, we return NS_OK and a null result
>     *_retval = nullptr;
> 
>     if (!*aName)
>         return NS_OK;
> 
>-    if (!aRequestor)
>-    {
>-        nsCOMPtr<nsIDocShellTreeItem> foundItem;
>+    if (aRequestor) {
>+        // We are being called as part of the recursive search just hand
>+        // straight off to the search by actual name function.
>+        return DoFindItemWithName(aName, aRequestor, aOriginalRequestor,
>+                                  _retval);
>+    } else {
> 
>         // This is the entry point into the target-finding algorithm.  Check
>         // for special names.  This should only be done once, hence the check
>         // for a null aRequestor.
> 
>+        nsCOMPtr<nsIDocShellTreeItem> foundItem;
>         nsDependentString name(aName);
>         if (name.LowerCaseEqualsLiteral("_self")) {
>             foundItem = this;
>         }
>         else if (name.LowerCaseEqualsLiteral("_blank"))
>         {
>             // Just return null.  Caller must handle creating a new window with
>             // a blank name himself.
>@@ -3155,26 +3159,29 @@ nsDocShell::FindItemWithName(const PRUni
>                 // Note: _content should always exist.  If we don't have one
>                 // hanging off the treeowner, just create a named window....
>                 // so don't return here, in case we did that and can now find
>                 // it.                
>                 // XXXbz should we be using |root| instead of creating
>                 // a new window?
>             }
> #endif
>+        } else {
>+            // Do the search for item by an actual name.
>+            DoFindItemWithName(aName, aRequestor, aOriginalRequestor,
>+                               getter_AddRefs(foundItem));
>         }
> 
>         if (foundItem && !CanAccessItem(foundItem, aOriginalRequestor)) {
>             foundItem = nullptr;
>         }
> 
>+        // DoFindItemWithName only returns active items and we don't check if
>+        // the item is active for the special cases.
>         if (foundItem) {
>-            // We return foundItem here even if it's not an active
>-            // item since all the names we've dealt with so far are
>-            // special cases that we won't bother looking for further.
> 
>             // If our document is sandboxed, we need to do some extra checks.
>             uint32_t sandboxFlags = 0;
> 
>             nsCOMPtr<nsIDocument> doc = do_GetInterface(aOriginalRequestor);
> 
>             if (doc) {
>                 sandboxFlags = doc->GetSandboxFlags();
>@@ -3186,27 +3193,24 @@ nsDocShell::FindItemWithName(const PRUni
> 
>                 // Is the found item not a top level browsing context and not ourself ?
>                 nsCOMPtr<nsIDocShellTreeItem> selfAsItem = static_cast<nsIDocShellTreeItem *>(this);
>                 if (foundItem != root && foundItem != selfAsItem) {
>                     // Are we an ancestor of the foundItem ?
>                     bool isAncestor = false;
> 
>                     nsCOMPtr<nsIDocShellTreeItem> parentAsItem;
>-                    GetSameTypeParent(getter_AddRefs(parentAsItem));
>+                    foundItem->GetSameTypeParent(getter_AddRefs(parentAsItem));
> 
>                     while (parentAsItem) {
>-                        nsCOMPtr<nsIDocShellTreeItem> tmp;
>-                        parentAsItem->GetParent(getter_AddRefs(tmp));
>-
>-                        if (tmp && tmp == selfAsItem) {
>+                        if (parentAsItem == selfAsItem) {
>                             isAncestor = true;
>                             break;
>                         }
>-                        parentAsItem = tmp;
>+                        parentAsItem->GetSameTypeParent(getter_AddRefs(parentAsItem));
>                     }
> 
>                     if (!isAncestor) {
>                         // No, we are not an ancestor and our document is
>                         // sandboxed, we can't allow this.
>                         foundItem = nullptr;
>                     }
>                 } else {
>@@ -3224,25 +3228,29 @@ nsDocShell::FindItemWithName(const PRUni
>                             break;
>                         }
>                         tmp->GetParent(getter_AddRefs(tmp));
>                     }
>                 }
>             }
> 
>             foundItem.swap(*_retval);
>-            return NS_OK;
>-        }
>-    }
>-
>-    // Keep looking
>-        
>+        }
>+        return NS_OK;
>+    }
>+}
>+
>+nsresult
>+nsDocShell::DoFindItemWithName(const PRUnichar * aName,
>+                             nsISupports * aRequestor,
>+                             nsIDocShellTreeItem * aOriginalRequestor,
>+                             nsIDocShellTreeItem ** _retval)
>+{
>     // First we check our name.
>-    if (mName.Equals(aName) && ItemIsActive(this) &&
>-        CanAccessItem(this, aOriginalRequestor)) {
>+    if (mName.Equals(aName) && ItemIsActive(this)) {
>         NS_ADDREF(*_retval = this);
>         return NS_OK;
>     }
> 
>     // This QI may fail, but the places where we want to compare, comparing
>     // against nullptr serves the same purpose.
>     nsCOMPtr<nsIDocShellTreeItem> reqAsTreeItem(do_QueryInterface(aRequestor));
> 
>diff -r bc108d2ce8d1 docshell/base/nsDocShell.h
>--- a/docshell/base/nsDocShell.h	Tue Feb 05 22:40:34 2013 -0800
>+++ b/docshell/base/nsDocShell.h	Mon Feb 11 17:38:34 2013 +0000
>@@ -556,16 +556,23 @@ protected:
>     // Security checks to prevent frameset spoofing.  See comments at
>     // implementation sites.
>     static bool CanAccessItem(nsIDocShellTreeItem* aTargetItem,
>                                 nsIDocShellTreeItem* aAccessingItem,
>                                 bool aConsiderOpener = true);
>     static bool ValidateOrigin(nsIDocShellTreeItem* aOriginTreeItem,
>                                  nsIDocShellTreeItem* aTargetTreeItem);
> 
>+    // Separate function to do the actual name (i.e. not _top, _self etc.)
>+    // searching for FindItemWithName.
>+    nsresult DoFindItemWithName(const PRUnichar * aName,
>+                                 nsISupports * aRequestor,
>+                                 nsIDocShellTreeItem * aOriginalRequestor,
>+                                 nsIDocShellTreeItem ** _retval);
>+
>     // Returns true if would have called FireOnLocationChange,
>     // but did not because aFireOnLocationChange was false on entry.
>     // In this case it is the caller's responsibility to ensure
>     // FireOnLocationChange is called.
>     // In all other cases false is returned.
>     bool SetCurrentURI(nsIURI *aURI, nsIRequest *aRequest,
>                        bool aFireOnLocationChange,
>                        uint32_t aLocationFlags);

Flags:

imelven: feedback+

Actions: View | Diff | Review

Attachments on bug 838692: 710786 | 712895 | 712902 | 713432 | 713436 | 713440 | 714291 | 714370 | 718537 | 722755 | 725773 | 741406 | 745536 | 747648 | 748064 | 763645