382558 - Input validation flaw in nsResProtocolHandler.cpp

Status: RESOLVED INCOMPLETE

(Core :: Networking, defect, P5)

Description

[Thor Larholm]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.4) Gecko/20070515 Firefox/2.0.0.4
Build Identifier: 

The patch from bug 367428 - ressource:// directory traversal - opens up a related input validation flaw in the resource protocol.

Before the patch the input to nsResProtocolHandler::ResolveURI was already checked to prevent any : characters from sneaking in, which can allow absolute URI references such as "res:C:boot.ini". This check is still in place in lines 334 to 336.

    // Don't misinterpret the filepath as an absolute URI.
    if (filepath.FindChar(':') != -1)
        return NS_ERROR_MALFORMED_URI;

Since the previous directory traversal vulnerability depended on URL-escaped characters such as %5C to work the patch added the currently present lines 338 to 340.

    NS_UnescapeURL(filepath);
    if (filepath.FindChar('\\') != -1)
        return NS_ERROR_MALFORMED_URI;

Since the filepath is now unescaped after the check for a : character has occurred, it is possible to inject : characters with the URL-escaped version %3A.

A non-malicious example request that can be used for verification is the following link include, which passes on unfiltered : characters to the host file system.

<link rel="stylesheet" href="resource://gre/browserconfig%3A.properties" />

It is also possible to pass on @ characters to the host file system, which at least on the Windows platform can be used to Basic Authentication style URI's.


Reproducible: Always

Comment 1

[Thor Larholm]

Attachment: Filemon log to demonstrate : and @ being passed to the filesystem

The first line comes from <link rel="stylesheet" href="resource://gre/browserconfig%3A.properties" />, the second line comes from <link rel="stylesheet" href="resource://gre/browserconfig%40.properties" />

Comment 2

[:Gavin Sharp [email: [e-mail]]]

As far as I can tell, the check for a colon was added in bug 337744 to prevent URIs like "resource:///http://www.google.com/" from being interpreted as "http://www.google.com/", not to avoid passing ":" to the file system. In other words, the fact that ":" or "@" are being sent to the file system was not the problem, the problem was that those characters could potentially cause our URI parsing code to output absolute URIs when resolving the latter part of the resource: URI.

Is there any evidence that this is an exploitable flaw?

Comment 3

[Thor Larholm]

You would have to re-run some modified versions of the unit tests from bug 337744 to validate whether this is an immediately exploitable flaw.

Since this does circumvent the input validation checks in nsResProtocolHandler::ResolveURI I am sure that there will be some exploitable condition for this, provided that someone is up for the task of single stepping through the resolver logic.

Comment 4

[Daniel Veditz [:dveditz]]

Gavin is correct in comment 2 about why the colon check is there, it's not intended to prevent colons in file paths. Some might argue, in fact, that doing so is a bug, except we wouldn't really care because ':' doesn't work in Windows file names so we'd never use them in Firefox resources, and resource: is intended for internal use.

It's _probably_ OK since escaped colon can't validly delimit a scheme, but given code that apparently could read a second scheme out of a URL in the first place I'm not sure I want to bank on that.

We might as well open this up given the blog post at http://larholm.com/2007/06/04/unpatched-input-validation-flaw-in-firefox-2004/

Comment 5

[Boris Zbarsky [:bzbarsky]]

I think using a check for ':' to tell whether something is an absolute URI is a little odd to start with.  Ideally we'd either use the same code as Resolve() or just go ahead and use nsIIOService::extractScheme on the exact string we plan to pass to Resolve(), and bail out if it finds something, right?

Comment 6

[Window Snyder]

Thor's blog post says:

There are some odd URI resolver logic in nsIStandard::Resolve that I will still have to look into. Under some circumstances on Windows, I can get / characters translated into \ characters and have triple dots translated into double dots, which will once again allow the full directory traversal vulnerability. However, the output is flaky at best and I will have to do some tedious single stepping through the URI resolver logic code to determine at what point the input is unescaped twice :)

So does this allow full directory traverse or not?

Comment 7

[Benjamin Smedberg]

The full-traversal is covered by bug 380994. I'm unclear on whether the colons or slashes issue is a problem: I can't see how passing either to the filesystem is a problem, but I may not be inventive enough.

Comment 8

[Benjamin Smedberg]

This blocks if it's valid... need more data somehow.

Comment 9

[Daniel Veditz [:dveditz]]

haven't found an immediate problem, and the colon check is not intended to keep ':' out of filenames but just to keep deeper layers of nsStandardURL from doing it's "is this a relative or absolute URI" check twice.

Comment 10

[Robert Sayre]

This was filed in June. How do we make sure this gets addressed promptly?

Comment 11

[Robert Sayre]

OK, nothing confirmed here I guess. Pushing down priority.