Don't allow path traversal paths on the base file helpers

Don't allow path traversal paths on the base file helpers

This forces explicit normalization of paths and make path escaping security bugs much harder to exploit. See for example bug 167122

BUG=168890
TEST=included tests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175642

[brettw, 2013-01-07 20:50:26]

lgtm

https://codereview.chromium.org/11782005/diff/11002/base/file_util.cc
File base/file_util.cc (right):

https://codereview.chromium.org/11782005/diff/11002/base/file_util.cc#newcode155
base/file_util.cc:155: if (path.ReferencesParent())
Maybe this warrants a mention in the header as well?

https://codereview.chromium.org/11782005/diff/11002/base/platform_file.h
File base/platform_file.h (right):

https://codereview.chromium.org/11782005/diff/11002/base/platform_file.h#newc...
base/platform_file.h:134: const FilePath& name, int flags, bool* created,
Can you horizontally align the params on different lines like the other one
does?

[jschuh, 2013-01-07 20:57:25]

lgtm on the security side.

[cpu_(ooo_6.6-7.5), 2013-01-08 00:18:13]

changes made, given the column limit I had to change the name of the function to
CreatePlatformFileUnsafe.

please take a look again.

[cpu_(ooo_6.6-7.5), 2013-01-08 19:06:59]

Nico, owners check for the skia/ext file please.

[Nico, 2013-01-08 19:20:03]

skia owners lgtm

https://codereview.chromium.org/11782005/diff/8010/base/file_util.cc
File base/file_util.cc (right):

https://codereview.chromium.org/11782005/diff/8010/base/file_util.cc#newcode155
base/file_util.cc:155: if (path.ReferencesParent())
Should this DCHECK(), so that devs trying to do this in a test don't waste 20
minutes trying to understand what's going on?

https://codereview.chromium.org/11782005/diff/8010/base/file_util.h
File base/file_util.h (right):

https://codereview.chromium.org/11782005/diff/8010/base/file_util.h#newcode179
base/file_util.h:179: // This function fails if the |path| contains path
traversal components.
I would read this and think "I wonder what a path traversal component is". Does
this mean '..'? If so, can you say '..' instead? If it means '..' and '.', can
you add "(., ..)" to the end of the sentence?

[dglazkov, 2013-01-09 16:36:18]

Could this change have caused massive fails on WebKit canaries? There are tons
of tests failing like this:

[1785:-1598589632:1136796049667:FATAL:platform_support_mac.mm(231)] Failed
reading:
/Volumes/data/b/build/slave/WebKit_Mac10_6/build/src/xcodebuild/Release/DumpRenderTree.app/Contents/MacOS/../Resources/missingImage.png

[Nico, 2013-01-09 19:21:45]

On 2013/01/09 16:36:18, Dimitri Glazkov wrote:
> Could this change have caused massive fails on WebKit canaries? There are tons
> of tests failing like this:
> 
> [1785:-1598589632:1136796049667:FATAL:platform_support_mac.mm(231)] Failed
> reading:
>
/Volumes/data/b/build/slave/WebKit_Mac10_6/build/src/xcodebuild/Release/DumpRenderTree.app/Contents/MacOS/../Resources/missingImage.png

I'd say that's well possible. Revert, see if it helps I suppose.

Carlos: When you reland, please do so with dcheck.

[cpu_(ooo_6.6-7.5), 2013-01-09 22:57:56]

On 2013/01/09 19:21:45, Nico wrote:
> On 2013/01/09 16:36:18, Dimitri Glazkov wrote:
> > Could this change have caused massive fails on WebKit canaries? There are
tons
> > of tests failing like this:
> > 
> > [1785:-1598589632:1136796049667:FATAL:platform_support_mac.mm(231)] Failed
> > reading:
> >
>
/Volumes/data/b/build/slave/WebKit_Mac10_6/build/src/xcodebuild/Release/DumpRenderTree.app/Contents/MacOS/../Resources/missingImage.png
> 
> I'd say that's well possible. Revert, see if it helps I suppose.
> 
> Carlos: When you reland, please do so with dcheck.

There was no need to reland. The code failed in an understandable way anyways.

Actually I am even less inclined to the dcheck since in all the cases so far the
the test asserts which is much better than trying to decode stack traces on mac.
Also already had another person affected (some fyi GPU bots) and the person was
able to figure it out without my help.

But you feel strongly, convince brettw and he can sudo me into fixing it.

[Noel Gordon, 2013-01-18 14:15:51]

This change caused https://bugs.webkit.org/show_bug.cgi?id=106631

Refer to
http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/tests/WebImageTes...
readFile()

static PassRefPtr<SharedBuffer> readFile(const char* fileName)
{
    String filePath = webkit_support::GetWebKitRootDir();
    filePath.append("/Source/WebKit/chromium/tests/data/");
    filePath.append(fileName);

    long long fileSize;
    if (!getFileSize(filePath, fileSize))
        return 0;

    PlatformFileHandle handle = openFile(filePath, OpenForRead);
    // FAILS right here on the webkit win mac bots: the handle is invalid.

Synopsis:

GetWebKitRootDir() calls GetWebKitRootDirFilePath() in 

https://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/support/webkit_sup...

which returns a path containing "../.." on win and mac.

We can read the length (and other metadata) of a file with such a path via
WebFileUtilitiesImpl::getFileInfo(path)

https://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/glue/webfileutilit...

(and information exposure) hence getFileSize() works.  But due to this change,
we are prevented from opening the file 

https://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/glue/webfileutilit...

because that uses base::CreatePlatformFile().

[Nico, 2013-01-18 17:56:05]

On Fri, Jan 18, 2013 at 6:15 AM,  <noel@chromium.org> wrote:
> This change caused https://bugs.webkit.org/show_bug.cgi?id=106631
>
> Refer to
>
http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/tests/WebImageTes...
> readFile()
>
> static PassRefPtr<SharedBuffer> readFile(const char* fileName)
> {
>     String filePath = webkit_support::GetWebKitRootDir();
>     filePath.append("/Source/WebKit/chromium/tests/data/");
>     filePath.append(fileName);
>
>     long long fileSize;
>     if (!getFileSize(filePath, fileSize))
>         return 0;
>
>     PlatformFileHandle handle = openFile(filePath, OpenForRead);
>     // FAILS right here on the webkit win mac bots: the handle is invalid.
>
> Synopsis:
>
> GetWebKitRootDir() calls GetWebKitRootDirFilePath() in
>
>
https://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/support/webkit_sup...
>
> which returns a path containing "../.." on win and mac.
>
> We can read the length (and other metadata) of a file with such a path via
> WebFileUtilitiesImpl::getFileInfo(path)
>
>
https://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/glue/webfileutilit...
>
> (and information exposure) hence getFileSize() works.  But due to this
> change,
> we are prevented from opening the file

I think the fix is to convert the paths to an absolute path first. See
https://codereview.chromium.org/11817018 and
https://codereview.chromium.org/11829014 for an example.

>
>
https://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/glue/webfileutilit...
>
> because that uses base::CreatePlatformFile().
>
>
> https://codereview.chromium.org/11782005/

[Noel Gordon, 2013-01-21 01:19:00]

On 2013/01/18 17:56:05, Nico wrote:
> 
> I think the fix is to convert the paths to an absolute path first. See
> https://codereview.chromium.org/11817018 and
> https://codereview.chromium.org/11829014 for an example.

Yeap, https://codereview.chromium.org/12038003