Closed Bug 810551 Opened 7 years ago Closed 7 years ago

Web App launch_path absolute url check bypass using three slashes (///:)

Categories

(Core Graveyard :: DOM: Apps, defect)

x86
macOS
defect
Not set

Tracking

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed, firefox-esr10 unaffected, firefox-esr17 wontfix)

RESOLVED FIXED
mozilla19
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed
firefox-esr10 --- unaffected
firefox-esr17 --- wontfix

People

(Reporter: pauljt, Assigned: fabrice)

Details

(Keywords: sec-low, Whiteboard: [qa-][adv-main18-])

Attachments

(1 file, 2 obsolete files)

The launch_path of a web app manifest should is not allowed to be absolute (see 786710 for background). There is a check, but this check can be subverted by a launch_path of the form "///example.com".

The check in question is here I think:http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/AppsUtils.jsm#160

160     function isAbsolute(uri) {
161       try {
162         Services.io.newURI(uri, null, null);
163       } catch (e if e.result == Cr.NS_ERROR_MALFORMED_URI) {
164         return false;
165       }
166       return true;
167     }

So there is a problem here since  "Services.io.newURI("///somesite.com", null, null)" throws an exception, and is thus allowed. When the launch_path is used, it is actually using nsIURI.resolve(launch_path):
http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/AppsUtils.jsm#332

This issues comes down to:
Services.io.newURI("http://a.com", null, null).resolve("///b.com") // returns 'http://b.com'.

I don't think this specific issue is sensitive, since since the same effect could be achieved with a server-side redirect. However I have some concerns that I haven't been able to test yet, so I have hidden this bug just in case.

My concerns are:
a) does this affect the origin of the app (I think no, but haven't tested yet), ie could the wrong origin get permissions.
b) is this pattern used more widely in gecko ?
c) is this the correct behavior for the resolve() function?

I'll keep testing, but please let me know if anyone can answer the above questions.
PS Matt W. found this - I'm just the messenger. Matt, please add anything I have missed.
blocking-basecamp: --- → ?
Assignee: nobody → fabrice
blocking-basecamp: ? → +
Attached patch patch (obsolete) — Splinter Review
(In reply to Paul Theriault [:pauljt] from comment #0)

> I don't think this specific issue is sensitive, since since the same effect
> could be achieved with a server-side redirect. However I have some concerns
> that I haven't been able to test yet, so I have hidden this bug just in case.

Actually that allows someone to put up a manifest for app that will transparently redirect to another site, without having to use an embedding iframe, and thus bypassing frame-busting. I think this last point is a good argument to fix this issue since the effect is the same as using an absolute URI.
 
> My concerns are:
> a) does this affect the origin of the app (I think no, but haven't tested
> yet), ie could the wrong origin get permissions.

No it doesn't, since the origin of the app is the origin of the manifest, not the origin of the launch_path.

> b) is this pattern used more widely in gecko ?

I don't know.

> c) is this the correct behavior for the resolve() function?

I put up a simple page at http://people.mozilla.org/~fdesre/test_uri.html with a link to "///mozilla.org", and this behaves like what you report here.

This patch special-cases uris starting by "///" but I'd like to know if there are other tricks that could lead to the same result and block them as well.
Attachment #680810 - Flags: review?(myk)
Comment on attachment 680810 [details] [diff] [review]
patch

> This issues comes down to:
> Services.io.newURI("http://a.com", null, null).resolve("///b.com") //
> returns 'http://b.com'.

In my tests it actually returns http:///b.com.  Firefox's location bar does redirect that to http://b.com/, though, and perhaps the app runtime does too.


This patch is ok as far as it goes, but it only takes out this instance of the problem instead of the entire class, which means there may be others lurking out there.

In fact a quick check of "//b.com" shows that Services.io.newURI("//b.com", null, null) also throws, while Services.io.newURI("http://a.com", null, null).resolve("//b.com") returns http://b.com, so there's another instance.

We could fix that instance too by changing the check to .startsWith("//"), but we should implement a more robust solution.

I haven't found a convention for this in the codebase.  We could resolve the URL against the app's origin, just as fullLaunchPath() does, and make sure the resolved URL's origin is the same as the app's origin.  But at the moment isAbsolute() doesn't have access to the app's origin.  And in any case that comparison wouldn't guarantee that the launch path is relative.

However, here's a technique that seems reasonably robust:

  let foo = Services.io.newURI("http://foo", null, null);
  let bar = Services.io.newURI("http://bar", null, null);
  return !(Services.io.newURI(uri, null, foo).prePath == foo.prePath &&
           Services.io.newURI(uri, null, bar).prePath == bar.prePath);

This tests that the URL resolves relatively to (i.e. that the resolved URL acquires the origin of) two different origins, which *should* guarantee that it specifies neither of those origins (nor any other), as otherwise it would resolve absolutely to at least one of them.

But it'd be worth getting an expert opinion on this.  biesi or bz?
Attachment #680810 - Flags: review?(myk) → review-
One more potential problem. I don't know if this exists on the phone, but I saw it when testing via Aurora desktop on the Mac.

The resolved domain - from launch_path - is used there to create the folder in which to store an app's settings. If your app can craft a launch_path that resolves to another's domain, you might be able to overwrite their preferences.

Example:
"launch_path" : "///mozilla.com"

App data:
Users/username/Library/Application Support/mozilla.com;http;-1

It seems strange to me that this folder would be created with the resolved domain (from launch_path) rather than the domain from which the manifest was requested, but I'm sure there is a reason.
(In reply to Matt Wobensmith from comment #4)
> One more potential problem. I don't know if this exists on the phone, but I
> saw it when testing via Aurora desktop on the Mac.
> 
> The resolved domain - from launch_path - is used there to create the folder
> in which to store an app's settings. If your app can craft a launch_path
> that resolves to another's domain, you might be able to overwrite their
> preferences.
> 
> Example:
> "launch_path" : "///mozilla.com"
> 
> App data:
> Users/username/Library/Application Support/mozilla.com;http;-1
> 
> It seems strange to me that this folder would be created with the resolved
> domain (from launch_path) rather than the domain from which the manifest was
> requested, but I'm sure there is a reason.

This behavior is specific to the desktop WebRT. Can you file another bug for this?
Fabrice - filed #811405 for the above. Thanks.
bz: we're looking for a robust way to determine that a URI string is relative, i.e. that it specifies only the Path portion of a URI (f.e. /foo/bar#baz), and resolving it against a base URL (f.e. http://example.com) will not result in the resolved URL having a different PrePath (i.e. origin) from the base URL.

Is there platform support for making this determination?  And if not, is the following technique a robust way of doing so?

function isRelative(uri) {
  let foo = Services.io.newURI("http://foo", null, null);
  let bar = Services.io.newURI("http://bar", null, null);
  return Services.io.newURI(uri, null, foo).prePath == foo.prePath &&
         Services.io.newURI(uri, null, bar).prePath == bar.prePath;
}

This tests that the URL resolves relatively to two different base URLs, which seems like it should guarantee that it specifies neither of those origins (nor any other), as otherwise it would resolve absolutely to at least one of them.
I don't think there's platform support for that; it's never really come up before.  Normally what we'd do is create a new URI object for the thing we plan to load, and then if there's a same-origin check to be done or something do it on that URI object.

If that's not an option, the technique from comment 7 seems reasonable.
Attached patch patch v2 (obsolete) — Splinter Review
In this patch a first build a URI for the manifest URL, then resolve the launch path against it and test if both origin are similar.
Attachment #680810 - Attachment is obsolete: true
Attachment #681674 - Flags: review?(myk)
Comment on attachment 681674 [details] [diff] [review]
patch v2

>-  checkManifest: function(aManifest, aInstallOrigin) {
>+  checkManifest: function(aManifest, aInstallURI) {
...
>-        if (!AppsUtils.checkManifest(manifest, installOrigin)) {
>+        if (!AppsUtils.checkManifest(manifest, aURL)) {

These changes conflate the app URI with the install URI, since aInstallURI is now the app URI, yet checkManifest still treats it as the install URI in some cases.

In particular, cbCheckAllowedOrigin checks that it is in installs_allowed_from, which means it now checks that the app origin is allowed to install the app, when it should be checking that the install origin is allowed to install the app.

You can still implement this approach, however.  You just need to pass the app URI as a third argument.

Just note that this approach doesn't actually verify that the launch path is relative, it only verifies that it is either relative or an absolute URL with the same origin as the app.  That plugs the class of security holes, so it fixes this bug.  But it doesn't quite check that the launch path conforms to the requirements of the spec.
Attachment #681674 - Flags: review?(myk) → review-
Attached patch patch v3Splinter Review
Attachment #681674 - Attachment is obsolete: true
Attachment #682201 - Flags: review?(myk)
Comment on attachment 682201 [details] [diff] [review]
patch v3

Looks good, r=myk!
Attachment #682201 - Flags: review?(myk) → review+
https://hg.mozilla.org/mozilla-central/rev/2c2c6ddb6fe6
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Flags: in-testsuite+
Whiteboard: [qa-]
Can I get a rating on this? Right now, since it is an internally reported, unrated security issue, I'm not going to write an advisory for it.

Is this a sec-high or sec-critical?
I think this probably rates as sec-low at the most, and probably doesn't need to be hidden at all.
Keywords: sec-low
Whiteboard: [qa-] → [qa-][adv-main18-]
Group: core-security
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.