367055 - Cmd-Click / "Open Link in New foo" should not open file URLs from non-local pages

Status: RESOLVED FIXED

(Camino Graveyard :: Security, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

In bug 155132, we discovered that normal-click of a file:// url link from an online web page fails (and philippe mentioned on the forum he sees some sort of security message in his console), but Cmd-click (and presumably middle-click), drag-and-drop to a (new) tab, and "Open Link in New foo" in the context menu all open file://Volumes/ from the bmo-hosted testcase.

Firefox correctly disallows non-local pages to open file://Volumes/ using all of these methods (but allows local pages), and we need to do the same (check to see if the page with the link is local before allowing it to work).

Comment 1

[philippe (part-time)]

For the record, the message I get in the console.log (left click):
Camino[281] JS error: Security Error: Content at http://lab.phiw.dev/camino/155132testcase.html may not load or link to file:///Volumes/.
(testing with test case loaded on my local dev server).

In Minefields error console, I get the same error (it is actually a warning).
When command clicking this type of link, Minefield logs two messages:
Security Error: Content at http://lab.phiw.dev/camino/155132testcase.html may not load or link to file:///users/phiw/path/to/file/filename.html.

Error: uncaught exception: Load of file:///Volumes/ from http://lab.phiw.dev/camino/155132testcase.html denied.

Comment 2

[Stuart Morgan]

It looks like somehow we need to be running these loads through something that uses CheckLoadURIWithPrincipal, but I haven't looked enough to see where (if anywhere) that would be at a level that is reasonable for us to use.

Comment 3

[Nick Kreeger]

Here is a new test case:

http://nkreeger.com/misc/bug367055test.html

Comment 4

[Nick Kreeger]

Attachment: Patch V1

Here is the first version of the patch. Here we use the security manager to check out URI's. This not only disables "file://", but several other unsafe loads according to the source:

http://lxr.mozilla.org/mozilla1.8/source/caps/src/nsScriptSecurityManager.cpp#1314

This patch requires a build path include to "dist/include/caps".

The window or new tab will still open with this patch, but loading is disabled. If we need a different behavior, this code can be relocated elsewhere.

Comment 5

[Chris Lawson (gone)]

(In reply to comment #4)

> The window or new tab will still open with this patch, but loading is disabled.
> If we need a different behavior, this code can be relocated elsewhere.

This seems like rather odd behaviour at best. How about we just plain don't open the window?

A better solution would probably be for Core to have a pretty error page that can be thrown in situations like this. Does such a thing exist?

cl

Comment 6

[Nick Kreeger]

FYI Firefox 2 doesn't open the window or tab.

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #4)
> This patch requires a build path include to "dist/include/caps".

The project patches in bug 372020 do this.

Comment 8

[Stuart Morgan]

Comment on attachment 260775 [details] [diff] [review]
Patch V1

We should handle this in our command-click handler so that we don't open the new window.

Comment 9

[Nick Kreeger]

Attachment: Patch V2

Patch round 2. This time, I checked the call at the event-handler.

Comment 10

[Stuart Morgan]

Could this be factored somewhere re-usable and hook it up to the "Open Link in..." context menu items without too much pain? I'm fine with punting DnD for now, but it would be nice to cover the other cases.

Comment 11

[Nick Kreeger]

Attachment: WIP 3

This kinda gives us an idea of how to not show a menu item. I left in the ContentClickListener checking code as a fail-safe measure...

I might move this code down to at least BrowserWrapper in the next version.

Comment 12

[Stuart Morgan]

Comment on attachment 261521 [details] [diff] [review]
WIP 3

>+        nsCOMPtr<nsIURI> referrerURI;
>+        NS_NewURI(getter_AddRefs(referrerURI), [referrerURL UTF8String]);
>+        nsCOMPtr<nsIURI> targetURI;
>+        NS_NewURI(getter_AddRefs(targetURI), hrefURL);
>+        
>+        nsCOMPtr<nsIScriptSecurityManager> secManager = do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID);
>+        if (secManager && referrerURI && targetURI) {
>+          nsresult rv = secManager->CheckLoadURI(referrerURI, targetURI, NSLoadFlagsNone);
>+          if (NS_SUCCEEDED(rv)) {
>+            menuPrototype = mLinkMenu;
>+          }
>+        }

How about moving all this duplicate code into GeckoUtils as something like
PRBool GeckoUtils::CanLoadURIFromURI(nsACString sourceURI, nsACString targetURI);
(or whatever form of Gecko string type that makes it easy to call with [foo UTF8String] as a parameter).

Comment 13

[Nick Kreeger]

Attachment: WIP 4

This moves the shared code to |GeckoUtils|. For now, I'm not showing the popup menu for bad links.

Comment 14

[Stuart Morgan]

Probably disabling the dangerous actions in the menu would be the least surprising thing, but if removing them is easier that would work too.

Comment 15

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #7)
> (In reply to comment #4)
> > This patch requires a build path include to "dist/include/caps".
> 
> The project patches in bug 372020 do this.
 
These have landed.

(In reply to comment #14)
> Probably disabling the dangerous actions in the menu would be the least
> surprising thing, but if removing them is easier that would work too.

Per the meeting, we should just remove "Open Link in New Window" and "Open Link in New Tab" from the context menu for local links (and local image links).

Comment 16

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: enhanced testcase (images)

kreeger's testcase with image links added.

Comment 17

[Nick Kreeger]

Attachment: V5

Here is an updated patch, it needs a new BrowserWindow.nib for the new menu keys.

Comment 18

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Can this be done at all without a new nib (other than just not showing the CM entirely)?

Comment 19

[froodian (Ian Leue)]

Unfortunately doing this without a new nib would require bug 363895 to be fixed, which I suppose means we have to wait until after 1.5 to fix the context menu portion of this (due to l10n freeze).  I personally suggest we go with the CM solution in WIP4 (don't show the CM at all for the links in question) in the 1.5 timeframe, and file a followup to be fixed after release.

Comment 20

[Nick Kreeger]

(In reply to comment #18)
> Can this be done at all without a new nib (other than just not showing the CM
> entirely)?
> 

We could remove the 0 index item 3 times...

Comment 21

[Stuart Morgan]

(In reply to comment #20)
> We could remove the 0 index item 3 times...

Let's do that with a comment as to why, and file a follow-up to fix it correctly right after 1.5.

Comment 22

[Nick Kreeger]

Attachment: V6

Here it is... I filed bug 378081 as a followup and documented it in the code.

Comment 23

[Stuart Morgan]

Comment on attachment 262170 [details] [diff] [review]
V6

>+const int kOpenLinkInNewWindowTag = 106;
>+const int kOpenLinkInNewTabTag = 107;

Remove these; no need to create tags we aren't using at the moment.

>+  BOOL isBadLink = NO;

isUnsafeLink

>+    NSString* referrerURL = [[mBrowserView getBrowserView] getFocusedURLString];
>+    nsCOMPtr<nsIDOMElement> linkElement;
>+    nsAutoString hrefURL;
>+    GeckoUtils::GetEnclosingLinkElementAndHref(mDataOwner->mContextMenuNode,
>+                                               getter_AddRefs(linkElement),
>+                                               hrefURL);
>+    if (!GeckoUtils::GetIsUriSafe([referrerURL UTF8String], NS_ConvertUTF16toUTF8(hrefURL).get()))
>+      isBadLink = YES;

Put a comment before this block to the effect of "Verify that it's safe to open this link"

>+    // To avoid updating the BrowserWindow.nib close to release time, the
>+    // menu items to remove will be removed from index 0 three times. After
>+    // the 1.1 release, this needs to be changed (see bug 378081).
>+    [result removeItemAtIndex:0];
>+    [result removeItemAtIndex:0];
>+    [result removeItemAtIndex:0];  // remove separator

Also add a comment above all this that says that you are removing the "Open Link in *" items here, since it's not clear from the codes.

>+    // Fail-safe for safe URI loading (see bug 367055).

I'm not clear on why you say it's a fail-safe; isn't this the one and only check for command-clicked links?

>+GeckoUtils::GetIsUriSafe(const char* aReferrerUri, const char* aTargetUri)

This name is too vague. Use something descriptive like CanLoadURIFromURI, or better IsSafeToOpenURIFromURI (and reverse the order of the arguments if you use one of those names).


Also, please make sure there's no whitespace on blank lines when you respin.

Comment 24

[Nick Kreeger]

Attachment: V7

Respin to meet all comments.

Comment 25

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I filed bug 378301 to spin off the drag-drop case.

Comment 26

[Stuart Morgan]

Comment on attachment 262215 [details] [diff] [review]
V7

Looks great! r=me.

Comment 27

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 262215 [details] [diff] [review]
V7

can you expand on what "safe" means in the GeckoUtils header comment? Might be nice to have it documented a little better.

sr=pink

Comment 28

[Nick Kreeger]

Checked into the Mozilla 1.8 branch.

Comment 29

[Nick Kreeger]

Checked into HEAD, sorry bout the delay...