Only take basename of default_file_name when starting a File Chooser.

Only take basename of default_file_name when starting a File Chooser.

Validate no ViewHostMsg_RunFileChooser messages received in the browser contain no path elements.

BUG=444198
TEST=content_browsertests

Committed: https://crrev.com/815c487fa677cf5413ed22e181437c1107300d49
Cr-Commit-Position: refs/heads/master@{#318111}

[Will Harris, 2014-12-20 00:48:41]

wfh@chromium.org changed reviewers:
+ thestig@chromium.org

[Lei Zhang, 2014-12-20 01:08:58]

Can we change the renderer to only send the base name and sanity check that on
the browser side when we first receive the IPC?

I haven't touched the code in a while, so I'm trying to remember how this all
works.

https://codereview.chromium.org/817103002/diff/1/chrome/browser/file_select_h...
File chrome/browser/file_select_helper.cc (right):

https://codereview.chromium.org/817103002/diff/1/chrome/browser/file_select_h...
chrome/browser/file_select_helper.cc:462: // default_file_name is untrusted from
renderer so only take basename.
nit: |default_file_name|

[Will Harris, 2014-12-20 01:17:41]

On 2014/12/20 01:08:58, Lei Zhang wrote:
> Can we change the renderer to only send the base name and sanity check that on
> the browser side when we first receive the IPC?
> 
> I haven't touched the code in a while, so I'm trying to remember how this all
> works.
> 
>
https://codereview.chromium.org/817103002/diff/1/chrome/browser/file_select_h...
> File chrome/browser/file_select_helper.cc (right):
> 
>
https://codereview.chromium.org/817103002/diff/1/chrome/browser/file_select_h...
> chrome/browser/file_select_helper.cc:462: // default_file_name is untrusted
from
> renderer so only take basename.
> nit: |default_file_name|

Yes, good point. I haven't actually found any legitimate cases where the
renderer sends an absolute path, so I could just terminate the renderer if I get
an IPC where params.default_file_name.IsAbsolute()

[Lei Zhang, 2014-12-20 02:03:34]

On 2014/12/20 01:17:41, Will Harris wrote:
> On 2014/12/20 01:08:58, Lei Zhang wrote:
> > Can we change the renderer to only send the base name and sanity check that
on
> > the browser side when we first receive the IPC?
> > 
> > I haven't touched the code in a while, so I'm trying to remember how this
all
> > works.
> > 
> >
>
https://codereview.chromium.org/817103002/diff/1/chrome/browser/file_select_h...
> > File chrome/browser/file_select_helper.cc (right):
> > 
> >
>
https://codereview.chromium.org/817103002/diff/1/chrome/browser/file_select_h...
> > chrome/browser/file_select_helper.cc:462: // default_file_name is untrusted
> from
> > renderer so only take basename.
> > nit: |default_file_name|
> 
> Yes, good point. I haven't actually found any legitimate cases where the
> renderer sends an absolute path, so I could just terminate the renderer if I
get
> an IPC where params.default_file_name.IsAbsolute()

If that's the case, then ya, let's just sanity check in the
ViewHostMsg_RunFileChooser IPC handler.

[Will Harris, 2014-12-23 20:32:51]

PTAL

[Will Harris, 2014-12-30 11:05:03]

On 2014/12/23 20:32:51, Will Harris wrote:
> PTAL

festive ping

[Will Harris, 2014-12-30 15:28:23]

wfh@chromium.org changed reviewers:
+ nasko@chromium.org
- thestig@chromium.org

[Will Harris, 2014-12-30 15:28:23]

nasko@chromium.org: Please review changes when you get back from vacation!

[nasko, 2015-01-02 16:48:14]

The code looks good, but it will be nice to have a test for this to avoid
regressing.

[Will Harris, 2015-01-02 16:51:52]

On 2015/01/02 16:48:14, nasko (Out till 2015) wrote:
> The code looks good, but it will be nice to have a test for this to avoid
> regressing.

I certainly wanted to add a test but I had no idea where to add it.  Do you have
any suggestions?

[nasko, 2015-01-02 17:02:59]

There are a couple of options - you can write a test and fake the IPC message
sent by the renderer and verify the child process is killed. An example of how
to work with the FileChooser can be found in 
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
And tests that try to fake IPCs are in security_exploit_browsertest.cc.

[Lei Zhang, 2015-01-05 22:43:55]

thestig@chromium.org changed reviewers:
+ thestig@chromium.org

[Lei Zhang, 2015-01-05 22:43:55]

Happy new year pong!

https://codereview.chromium.org/817103002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/817103002/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_view_host_impl.cc:1451: if
(params.default_file_name.IsAbsolute()) {
Should we check and make sure the file name does not contain any path separators
instead? i.e. No ../../ ... /../path/to/file

[Will Harris, 2015-02-24 21:05:11]

On 2015/01/05 22:43:55, Lei Zhang wrote:
> Happy new year pong!
> 
>
https://codereview.chromium.org/817103002/diff/20001/content/browser/renderer...
> File content/browser/renderer_host/render_view_host_impl.cc (right):
> 
>
https://codereview.chromium.org/817103002/diff/20001/content/browser/renderer...
> content/browser/renderer_host/render_view_host_impl.cc:1451: if
> (params.default_file_name.IsAbsolute()) {
> Should we check and make sure the file name does not contain any path
separators
> instead? i.e. No ../../ ... /../path/to/file

I have this aching feeling if I don't finish this CL someone will exploit it in
pwn2own... so I'll get this done this week!

[Will Harris, 2015-02-25 01:21:21]

PTAL - added some tests.

[Lei Zhang, 2015-02-25 01:27:58]

lgtm

[nasko, 2015-02-25 15:14:45]

Looks good, but I have a question about the test.

https://codereview.chromium.org/817103002/diff/60001/content/browser/security...
File content/browser/security_exploit_browsertest.cc (right):

https://codereview.chromium.org/817103002/diff/60001/content/browser/security...
content/browser/security_exploit_browsertest.cc:120: GURL
foo("http://foo.com/simple_page.html");
This seems like simple enough test that it should be doable in unit tests, which
are much cheaper and more efficient. Have you tried going that route?

[Will Harris, 2015-02-25 17:24:39]

On 2015/02/25 15:14:45, nasko wrote:
> Looks good, but I have a question about the test.
> 
>
https://codereview.chromium.org/817103002/diff/60001/content/browser/security...
> File content/browser/security_exploit_browsertest.cc (right):
> 
>
https://codereview.chromium.org/817103002/diff/60001/content/browser/security...
> content/browser/security_exploit_browsertest.cc:120: GURL
> foo("http://foo.com/simple_page.html");
> This seems like simple enough test that it should be doable in unit tests,
which
> are much cheaper and more efficient. Have you tried going that route?

How strongly do you feel about this - comment #12 suggested this was the best
way so I went ahead and did it.  Can you point me at some code in unit_tests
that I can try and copy that instead - I need to verify the renderer is killed
though, is that still possible from unit tests?

[Will Harris, 2015-02-25 17:35:19]

On 2015/02/25 17:24:39, Will Harris wrote:
> On 2015/02/25 15:14:45, nasko wrote:
> > Looks good, but I have a question about the test.
> > 
> >
>
https://codereview.chromium.org/817103002/diff/60001/content/browser/security...
> > File content/browser/security_exploit_browsertest.cc (right):
> > 
> >
>
https://codereview.chromium.org/817103002/diff/60001/content/browser/security...
> > content/browser/security_exploit_browsertest.cc:120: GURL
> > foo("http://foo.com/simple_page.html");
> > This seems like simple enough test that it should be doable in unit tests,
> which
> > are much cheaper and more efficient. Have you tried going that route?
> 
> How strongly do you feel about this - comment #12 suggested this was the best
> way so I went ahead and did it.  Can you point me at some code in unit_tests
> that I can try and copy that instead - I need to verify the renderer is killed
> though, is that still possible from unit tests?

Sorry, I found the other example you gave.  I have no way to get the renderer to
generate the bad IPC because the renderer sanitises the path before sending it,
so I need to fake the IPC anyway - it sounds like leaving this in
security_exploit_browsertest.cc is a good place since it already does fake IPCs.
 Thoughts?

[nasko, 2015-02-25 18:08:44]

LGTM

[Will Harris, 2015-02-25 20:57:02]

The CQ bit was checked by wfh@chromium.org

[commit-bot: I haz the power, 2015-02-25 20:57:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/817103002/60001

[commit-bot: I haz the power, 2015-02-25 21:01:46]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-25 21:02:50]

Patchset 4 (id:??) landed as
https://crrev.com/815c487fa677cf5413ed22e181437c1107300d49
Cr-Commit-Position: refs/heads/master@{#318111}