454087 - Don't rely on NSURL to check for javascript: or data: in set URL requests

Status: RESOLVED FIXED

(Camino Graveyard :: OS Integration, defect)

Description

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

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en; rv:1.8.1.16) Gecko/20080803 Camino/1.6.3 (like Firefox/2.0.0.16)
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en; rv:1.8.1.16) Gecko/20080803 Camino/1.6.3 (like Firefox/2.0.0.16)

Chris discovered in his paste reworking that the URL validation fails for many javascript: and data: URLs over in ScriptingSupport.mm.  NSURL apparently bails or fails when it hits a URL that is not "well-formed", which many javascript: and data: URLs are by default.

The end result is that our security hole is wide open.

You can't file security-sensitive bugs except via Bugzilla Helper, so I'll twiddle stuff after filing.

Reproducible: Always

Comment 1

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

Attachment: Sample text applescript

Uncomment each set url command in turn to see the failures in our dangerous-protocol-catching.

Comment 2

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

Chris was working on this...he wanted me to file it while he went on a ride to think more about it.

Comment 3

[Chris Lawson (gone)]

Attachment: fix v1.0, depends on patch in bug 453623

This works with everything in the testcase (except the last example; since I don't have the same bookmarks Smokey does, the script can't find that bookmark anyway) with the patch for bug 453623 installed as well. It obviously won't work at all otherwise :-p

Comment 4

[Chris Lawson (gone)]

Should we get mento to take a look at this too, since he's the one who suggested the NSURL approach in the first place?

Comment 5

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

(In reply to comment #3)
> fix v1.0, depends on patch in bug 453623

Is there a way to fix this on the branch that doesn't rely on bug 453623 (which in turn relies on checkins from three or four other bugs)?

Comment 6

[Chris Lawson (gone)]

(In reply to comment #5)
> Is there a way to fix this on the branch that doesn't rely on bug 453623 (which
> in turn relies on checkins from three or four other bugs)?

Yes. As soon as I can get a branch patch generated, I'll post it.

Comment 7

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

Attachment: MOZILLA_1_8_BRANCH patch

Since Chris doesn't know the wonders of stub trees, he passed me a block of code to generate a patch for him--good thing, apparently, since it didn't compile ;)

The attached patch compiles and works.

Comment 8

[Daniel Veditz [:dveditz]]

Guessing at security priority, assuming local commandline scripting is the only attack vector. If this can be accessed remotely that would up the severity a bit.

Comment 9

[Stuart Morgan]

Yes, local attack only.

Comment 10

[Stuart Morgan]

Comment on attachment 337337 [details] [diff] [review]
fix v1.0, depends on patch in bug 453623

>+  if ([newURI isJavascriptOrDataURIString]) {

Now we see why I don't like this kind of name. This adds a client of this method that has a *completely* different need than the existing client ("dangerous" vs. "can have spaces and the like in it"), but happens to, currently, share an implementation. That feels like a recipe for later mistakes.

Make a new NSString method called something like isPotentiallyDangerousURI, and duplicate the implementation, so that the method is for the concept, rather than the implementation that we've thought of so far and we don't accidentally break one usage when fixing a bug for the other.

>+    NSString *schemeString = [[[newURI componentsSeparatedByString:@":"] objectAtIndex:0] lowercaseString];
>     [[NSScriptCommand currentCommand] setScriptErrorNumber:NSArgumentsWrongScriptError];
>-    [[NSScriptCommand currentCommand] setScriptErrorString:[NSString stringWithFormat:@"Can't set URL of tab to a '%@:' URL.", scheme]];
>+    [[NSScriptCommand currentCommand] setScriptErrorString:[NSString stringWithFormat:@"Can't set URL of tab to a '%@:' URI.", schemeString]];

Stick with scheme, not schemeString; what else would the scheme be?

Comment 11

[Chris Lawson (gone)]

Attachment: fix v1.1

Trunk patch updated for sr comments. Branch patch doesn't need any changes AFAICT.

Comment 12

[Stuart Morgan]

Comment on attachment 337474 [details] [diff] [review]
fix v1.1

>+#import "NSString+Utils.h"

I assume you meant to actually add the NSString+Utils files to the diff?

>+  // Don't allow |javascript:| or |data:| URIs for security reasons.
>+  if ([newURI isPotentiallyDangerousURI]) {

The comment needs to change to that it's not tied to implementation of the other method.

Comment 13

[Chris Lawson (gone)]

Comment on attachment 337474 [details] [diff] [review]
fix v1.1

Obsoleting until the other bug lands.

Comment 14

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 337375 [details] [diff] [review]
MOZILLA_1_8_BRANCH patch

sr=pink

might be nice to have a comment on why we have to do this and can't use NSURL (but again this is just the branch, so i think that's being handled on the trunk).

Comment 15

[Chris Lawson (gone)]

Comment on attachment 337375 [details] [diff] [review]
MOZILLA_1_8_BRANCH patch

Even though this has sr+, I'd like a second set of eyes on it just in case.

Comment 16

[Chris Lawson (gone)]

Attachment: fix v1.2

This should do it. I moved the comments on the methods to the header since they're all public methods.

Comment 17

[Stuart Morgan]

Comment on attachment 337553 [details] [diff] [review]
fix v1.2

sr=smorgan

Comment 18

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

Checked in on 1.8.1 and cvs trunk.  Since mento's already going to have to minibranch to get the relnotes, we should be able to get this in 1.6.4.

We can probably make this public after 1.6.4 is out, or we could wait until 2.0a1 is out just to make sure all 2.0a1pre nightly users have the fix, too.

Comment 19

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

Comment on attachment 337310 [details]
Sample text applescript

Remind me to attach these (as separate script files, perhaps) to bug 453202 when this bug is opened up.

Comment 20

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

Opening this bug up. Camino 1.6.4 has shipped, as has Camino 2.0 alpha 1.