396263 - AppleScript "set URL" can use page context

Status: RESOLVED FIXED

(Camino Graveyard :: OS Integration, defect)

Description

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

"set URL" in AppleScript can use the page context.  This is a potential security issue.

I wonder if bug 394107 could provide the basis for the JS context that Stuart mentions in bug 178917 comment 36/37?

Comment 1

[Stuart Morgan]

Bug 394107 is about intercepting keyboard events, not JS context.

Comment 2

[Benjamin Smedberg]

Is this a bad thing? This is just like bookmarklets: unless untrusted apps could call this API, it sounds like a feature not a bug.

Comment 3

[Stuart Morgan]

Any app in the system can send Apple Events, so this is a potential vector for stealing passwords that are stored in the Keychain; it's the same argument as bug 178917.

Comment 4

[Jesse Ruderman]

"set URL" sounds like the kind of API that might be used to open a web page in a browser.  If an app isn't careful about what kinds of URLs it sends, and Camino interprets a javascript: URL as "execute this JavaScript in the context of the current page", that leads to badness.

In contrast, bug 178917 would add a separate API for explicitly executing JavaScript, which is less likely to result in an app *inadvertently* participating in an attack.

Comment 5

[Peter Jaros (:peeja)]

Jesse: The "normal" way to open a URL is via the "open location" event.  "set URL" actually sets a property of a particular window/tab, and therefore will execute javascript: in page context, just like a link or a bookmarklet in that window/tab.  It won't be called by apps which are just sending Camino a URL to open in general.

As it stands, "set URL" calls |[theBrowserWrapper loadURI:newURI referrer:nil flags:NSLoadFlagsNone focusContent:YES allowPopups:NO]| to actually navigate.  That's a terribly overloaded method as it is.  I don't know what the scope of the flags parameter is.  If that would be an appropriate way to signal not to run javascript in the current page context, we could use that.  However, I believe that distinction will have to be made outside of Camino code.

Relevant flags: http://mxr.mozilla.org/mozilla/source/embedding/browser/cocoa/src/NSBrowserView.h#80

Comment 6

[Peter Jaros (:peeja)]

Attachment: Fix

Ok, this'll stop AS from navigating a tab/window to a javascript: or data: URL.  That should fix the problem.

Comment 7

[Stuart Morgan]

Comment on attachment 297822 [details] [diff] [review]
Fix

>+    return;
>+  }
>+  else
>+    [self loadURI:newURI referrer:nil flags:NSLoadFlagsNone focusContent:YES allowPopups:NO];
> }

No else after a return. Otherwise, looks good!

Comment 8

[Mark Mentovai]

Comment on attachment 297822 [details] [diff] [review]
Fix

>Index: src/appleevents/ScriptingSupport.mm

> - (void)setCurrentURI:(NSString *)newURI

>+  // Dont allow javascript: or data: URLs for security reasons.

"Don't"

>+  NSString *scheme = [[newURI componentsSeparatedByString:@":"] objectAtIndex:0];

How often do we do our own URL parsing?  I think it'd be better to say:

  NSString* scheme = [[NSURL URLWithString:newURI] scheme];

(It's even more correct to make an nsIURI and call SchemeIs since Gecko's responsible for all URI stuff, but making a new nsIURI here would look ugly, and I think I'd be satisfied with an NSURL.)

>+  if ([scheme isEqualToString:@"javascript"] ||
>+      [scheme isEqualToString:@"data"]) {

Scheme comparisons should always be case-insensitive.  Use lowercaseString when you assign to scheme, or use caseInsensitiveCompare:.

Comment 9

[Mark Mentovai]

Comment on attachment 297822 [details] [diff] [review]
Fix

>+  if ([scheme isEqualToString:@"javascript"] ||
[...]
>+  }
>+  else
>+    [self loadURI:newURI referrer:nil flags:NSLoadFlagsNone focusContent:YES 

Oh, and if you {brace} one branch of an if-else if-else, you should {brace} all branches, please.

Comment 10

[Peter Jaros (:peeja)]

Attachment: Fix Mark II

All good points, all addressed.  Take 2.

Comment 11

[Mark Mentovai]

Comment on attachment 297909 [details] [diff] [review]
Fix Mark II

>Index: src/appleevents/ScriptingSupport.mm

> - (void)setCurrentURI:(NSString *)newURI

>+  NSString *scheme = [[NSURL URLWithString:newURI] scheme];
>+  if ([[scheme lowercaseString] isEqualToString:@"javascript"] ||
>+      [[scheme lowercaseString] isEqualToString:@"data"]) {

Instead of taking [scheme lowercaseString] twice, assign it to scheme:

  NSString* scheme = [[[NSURL URLWithString:newURI] scheme] lowercaseString];

OK to fix on checkin.

Comment 12

[Peter Jaros (:peeja)]

Sounds good.  To clarify for checking in:

NSString *scheme = [[[NSURL URLWithString:newURI] scheme] lowercaseString];
if ([scheme isEqualToString:@"javascript"] ||
    [scheme isEqualToString:@"data"]) {

Comment 13

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

I'll land this once I've gotten data for bug 413123.

Comment 14

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

Checked in on the trunk and the MOZILLA_1_8_BRANCH in advance of 1.6b3.

Comment 15

[Samuel Sidler (old account; do not CC)]

Removed security sensitivity now that this is fixed and Camino 1.6b3 is released.