Kill a renderer process if ViewHostMsg_AsyncOpenFile comes with a...

Kill a renderer process if ViewHostMsg_AsyncOpenFile comes with a
request to write a file, as the renderer is most probably compromised.

BUG=56725
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=60709

[darin (slow to review), 2010-09-24 00:03:44]

http://codereview.chromium.org/3502001/diff/1/2
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/3502001/diff/1/2#newcode1715
chrome/browser/renderer_host/resource_message_filter.cc:1715: if (flags &
base::PLATFORM_FILE_WRITE) {
should we check other flags?  maybe this should just be a whitelist of allowed
flags?

http://codereview.chromium.org/3502001/diff/1/2#newcode1718
chrome/browser/renderer_host/resource_message_filter.cc:1718:
ViewHostMsg_AsyncOpenFile::ID, handle());
should probably return at this point.

[dumi, 2010-09-24 00:43:20]

On 2010/09/24 00:03:44, darin wrote:
> http://codereview.chromium.org/3502001/diff/1/2
> File chrome/browser/renderer_host/resource_message_filter.cc (right):
> 
> http://codereview.chromium.org/3502001/diff/1/2#newcode1715
> chrome/browser/renderer_host/resource_message_filter.cc:1715: if (flags &
> base::PLATFORM_FILE_WRITE) {
> should we check other flags?  maybe this should just be a whitelist of allowed
> flags?
> 
> http://codereview.chromium.org/3502001/diff/1/2#newcode1718
> chrome/browser/renderer_host/resource_message_filter.cc:1718:
> ViewHostMsg_AsyncOpenFile::ID, handle());
> should probably return at this point.

hmm, now that i think about this, i'm not sure this is right... if we never
allow write requests, then what's the point of having FileIO::Write() or
Truncate()?

[darin (slow to review), 2010-09-24 04:19:33]

On Thu, Sep 23, 2010 at 5:43 PM, <dumi@chromium.org> wrote:

> On 2010/09/24 00:03:44, darin wrote:
>
>> http://codereview.chromium.org/3502001/diff/1/2
>> File chrome/browser/renderer_host/resource_message_filter.cc (right):
>>
>
>  http://codereview.chromium.org/3502001/diff/1/2#newcode1715
>> chrome/browser/renderer_host/resource_message_filter.cc:1715: if (flags &
>> base::PLATFORM_FILE_WRITE) {
>> should we check other flags?  maybe this should just be a whitelist of
>> allowed
>> flags?
>>
>
>  http://codereview.chromium.org/3502001/diff/1/2#newcode1718
>> chrome/browser/renderer_host/resource_message_filter.cc:1718:
>> ViewHostMsg_AsyncOpenFile::ID, handle());
>> should probably return at this point.
>>
>
> hmm, now that i think about this, i'm not sure this is right... if we never
> allow write requests, then what's the point of having FileIO::Write() or
> Truncate()?


We need to extend ChildProcessSecurityPolicy to do more than just check
CanUploadFile.  That is only indicating when a file may be read.  We need a
different security attribute to indicate files that may be written.

This will involve teaching the system about the root directory of where the
file systems are stored.  Note: for file paths created through pepper's
FileRef interface, the paths would currently fail the CanUploadFile check.

I view this current patch as a temporary security fix until we have support
for pepper file system access ironed out.

-Darin




>
>
> http://codereview.chromium.org/3502001/show
>

[dumi, 2010-09-24 21:56:25]

On 2010/09/24 04:19:33, darin wrote:
> On Thu, Sep 23, 2010 at 5:43 PM, <mailto:dumi@chromium.org> wrote:
> 
> > On 2010/09/24 00:03:44, darin wrote:
> >
> >> http://codereview.chromium.org/3502001/diff/1/2
> >> File chrome/browser/renderer_host/resource_message_filter.cc (right):
> >>
> >
> >  http://codereview.chromium.org/3502001/diff/1/2#newcode1715
> >> chrome/browser/renderer_host/resource_message_filter.cc:1715: if (flags &
> >> base::PLATFORM_FILE_WRITE) {
> >> should we check other flags?  maybe this should just be a whitelist of
> >> allowed
> >> flags?
> >>
> >
> >  http://codereview.chromium.org/3502001/diff/1/2#newcode1718
> >> chrome/browser/renderer_host/resource_message_filter.cc:1718:
> >> ViewHostMsg_AsyncOpenFile::ID, handle());
> >> should probably return at this point.
> >>
> >
> > hmm, now that i think about this, i'm not sure this is right... if we never
> > allow write requests, then what's the point of having FileIO::Write() or
> > Truncate()?
> 
> 
> We need to extend ChildProcessSecurityPolicy to do more than just check
> CanUploadFile.  That is only indicating when a file may be read.  We need a
> different security attribute to indicate files that may be written.
> 
> This will involve teaching the system about the root directory of where the
> file systems are stored.  Note: for file paths created through pepper's
> FileRef interface, the paths would currently fail the CanUploadFile check.
> 
> I view this current patch as a temporary security fix until we have support
> for pepper file system access ironed out.
> 
> -Darin
> 
> 
> 
> 
> >
> >
> > http://codereview.chromium.org/3502001/show
> >
> 

i see. updated the patch to only allow OPEN_EXISTING and READ operations.

[darin (slow to review), 2010-09-25 04:45:19]

LGTM

http://codereview.chromium.org/3502001/diff/2002/6001
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/3502001/diff/2002/6001#newcode1722
chrome/browser/renderer_host/resource_message_filter.cc:1722: if (flags &
~allowed_flags) {
how about adding a DLOG(ERROR) here?

[dumi, 2010-09-27 18:58:49]

http://codereview.chromium.org/3502001/diff/2002/6001
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/3502001/diff/2002/6001#newcode1722
chrome/browser/renderer_host/resource_message_filter.cc:1722: if (flags &
~allowed_flags) {
On 2010/09/25 04:45:19, darin wrote:
> how about adding a DLOG(ERROR) here?

done. added DLOG(ERROR) << "Bad flags in ViewMsgHost_AsyncOpenFile message: " <<
flags;

[darin (slow to review), 2010-09-27 20:23:28]

> done. added DLOG(ERROR) << "Bad flags in ViewMsgHost_AsyncOpenFile message: "
<<
> flags;

LGTM