File manager: support for zipping selected files.

File manager: support for zipping selected files.

Adds a new command "Zip selection" in the file manager context menu that creates a zip file for the selected files and folders.

TODO: zipping on Drive is not yet supported. Tracked under bug 158690.

BUG=138359
TEST=manual, CQ


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

[hshi1, 2012-11-02 18:27:38]

Hi could you review this draft CL (particularly the JS / fileBrowserPrivateApi
part)? It is not complete yet but I've already added a bunch of code here and
I'm not really sure if it is the best approach.

(1) I've added a separate serviceZipTask_ function to FileCopyManager, even
though it is largely similar to the copy task. The difference is that it must
handle all entries in one call, instead of each entry at a time.

(2) It has yet to address the issue of progress updates. I wonder if I can
re-use the existing progress update event (that is used for gdata transfers)? Or
should I create a new event?

[hshi1, 2012-11-02 22:50:53]

P.S. I have decided that this CL is becoming too large to get landed in one
shot. 

The per-entry zip progress update itself is probably going to touch another
bunch of files (define new event for the new zip-entry-added status,
fileBrowserEventRouter changes, IPC messages between the utility process that
carries out zipping back to fileBrowserPrivateApi, etc.).

Also, I have not yet renamed the FileCopyManager to something more generic. I
think it should be renamed, but not sure if we want to do it first or later.

For now I'll just set both pendingBytes and completeBytes to 0 and let butterBar
display a scrolling progress bar. At least it looks good in the UI, and is
functional.

Please let me know your comments, thanks!

[Oleg Eterevsky, 2012-11-06 10:07:42]

Changes to FileManager js code lgtm.

http://codereview.chromium.org/11309014/diff/41009/chrome/browser/resources/f...
File chrome/browser/resources/file_manager/js/file_copy_manager.js (right):

http://codereview.chromium.org/11309014/diff/41009/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/file_copy_manager.js:954:
errorCallback) {
Indentation.

[hshi1, 2012-11-06 18:01:02]

Fixed indentation in file_copy_manager.js. I'll add other owners for review.

http://codereview.chromium.org/11309014/diff/41009/chrome/browser/resources/f...
File chrome/browser/resources/file_manager/js/file_copy_manager.js (right):

http://codereview.chromium.org/11309014/diff/41009/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/file_copy_manager.js:954:
errorCallback) {
On 2012/11/06 10:07:42, Oleg wrote:
> Indentation.

Done.

[hshi1, 2012-11-06 19:00:38]

Adding Nico and James for the changes in the common/ and utility/:

M	 chrome/common/chrome_utility_messages.h
M	 chrome/common/extensions/api/file_browser_private.json
M	 chrome/common/zip.h
M	 chrome/common/zip.cc
M	 chrome/utility/chrome_content_utility_client.h
M	 chrome/utility/chrome_content_utility_client.cc

Also Toni - please review the change in chrome/browser/chromeos/extensions/,
thanks!

[tbarzic, 2012-11-06 19:41:25]

http://codereview.chromium.org/11309014/diff/41009/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/zip_file_creator.h (right):

http://codereview.chromium.org/11309014/diff/41009/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/zip_file_creator.h:35: class Delegate {
Looks like this class reacts on events from the ZipFileCreator, maybe |Observer|
would be a better name for it.

http://codereview.chromium.org/11309014/diff/41009/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/zip_file_creator.h:75: // Our delegate's
thread. This is the thread we respond on.
"The delegate's thread" sounds better to me, but you decide..

http://codereview.chromium.org/11309014/diff/43005/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

http://codereview.chromium.org/11309014/diff/43005/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:2941: std::string
virtual_path;
virtual path is a bit confusing given that selecteion_urls actually contains
urls.

http://codereview.chromium.org/11309014/diff/31030/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

http://codereview.chromium.org/11309014/diff/31030/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:2968: for
(++iter; iter != files.end(); ++iter) {
should we check that files are not on drive (this doesn't work for drive,
right?)

http://codereview.chromium.org/11309014/diff/31030/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/zip_file_creator.cc (right):

http://codereview.chromium.org/11309014/diff/31030/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/zip_file_creator.cc:43: // We assume that we
are started on the thread that the client wants us to do
maybe it would be easier to use the class if this can be any thread and we make
sure the Delegate methods are called on that thread.
We would just use BrowserThread::FILE when creating process host in
StartProcessOnIOThread (afaik, we can't really use any other thread).

http://codereview.chromium.org/11309014/diff/31030/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/zip_file_creator.cc:84: //
host->SetExposedDir(temp_crx_path.DirName());
Is this commented out intentionally? If so, it would be better if the line is
just removed.

http://codereview.chromium.org/11309014/diff/31030/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/zip_file_creator.h (right):

http://codereview.chromium.org/11309014/diff/31030/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/zip_file_creator.h:28: // Additionally, we
hold a reference to our own client so that it lives at least
is this true?
It seems that the Delegate makes sure it doesn't die before zip file creation
returns result.

http://codereview.chromium.org/11309014/diff/31030/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/zip_file_creator.h:37: virtual void
OnZipSuccess() = 0;
what do you think about having OnZipDone(bool success)

http://codereview.chromium.org/11309014/diff/31030/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/zip_file_creator.h:57: friend class
ZipFileCreatorTest;
I don't see new tests :)

[hshi1, 2012-11-06 20:42:59]

Thanks Toni for the feedback. I've addressed a few easy ones. Will work on the
tests and then update.

http://codereview.chromium.org/11309014/diff/43005/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

http://codereview.chromium.org/11309014/diff/43005/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:2941: std::string
virtual_path;
On 2012/11/06 19:41:25, tbarzic wrote:
> virtual path is a bit confusing given that selecteion_urls actually contains
> urls.

Done.

http://codereview.chromium.org/11309014/diff/31030/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

http://codereview.chromium.org/11309014/diff/31030/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:2968: for
(++iter; iter != files.end(); ++iter) {
On 2012/11/06 19:41:25, tbarzic wrote:
> should we check that files are not on drive (this doesn't work for drive,
> right?)

Done. I'm adding a TODO item here for drive support. Also I'm checking the
|dir_path| to make sure the dir is not on Drive, plus checking that each file on
the list is under the specified dir.

http://codereview.chromium.org/11309014/diff/31030/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/zip_file_creator.h (right):

http://codereview.chromium.org/11309014/diff/31030/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/zip_file_creator.h:28: // Additionally, we
hold a reference to our own client so that it lives at least
On 2012/11/06 19:41:26, tbarzic wrote:
> is this true?
> It seems that the Delegate makes sure it doesn't die before zip file creation
> returns result.

Done.

http://codereview.chromium.org/11309014/diff/31030/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/zip_file_creator.h:37: virtual void
OnZipSuccess() = 0;
On 2012/11/06 19:41:26, tbarzic wrote:
> what do you think about having OnZipDone(bool success)

Done.

[satorux1, 2012-11-07 04:58:00]

I'd suggest to split the changes in chrome/common/zip into a separate patch. I'd
be happy to review.

[hshi1, 2012-11-07 05:13:17]

On 2012/11/07 04:58:00, satorux1 wrote:
> I'd suggest to split the changes in chrome/common/zip into a separate patch.
I'd
> be happy to review.

Great suggestion! I'll work on a separate CL for the zip::ZipFileList() related
change and ask you for review. (And I need to write unit test for it too).
Thanks.

[hshi1, 2012-11-10 02:29:11]

http://codereview.chromium.org/11309014/diff/31030/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/zip_file_creator.cc (right):

http://codereview.chromium.org/11309014/diff/31030/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/zip_file_creator.cc:43: // We assume that we
are started on the thread that the client wants us to do
Maybe the comments aren't clear, but this function (ZipFileCreator::Start()) can
be any thread. The CHECK below is for whether we succeeded to get the thread
identifier. Later in OnCreateZipFileSucceeded/Failed we're checking if it is the
same thread.

On 2012/11/06 19:41:25, tbarzic wrote:
> maybe it would be easier to use the class if this can be any thread and we
make
> sure the Delegate methods are called on that thread.
> We would just use BrowserThread::FILE when creating process host in
> StartProcessOnIOThread (afaik, we can't really use any other thread).

http://codereview.chromium.org/11309014/diff/31030/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/zip_file_creator.cc:84: //
host->SetExposedDir(temp_crx_path.DirName());
On 2012/11/06 19:41:25, tbarzic wrote:
> Is this commented out intentionally? If so, it would be better if the line is
> just removed.

Done.

[tbarzic, 2012-11-10 03:40:56]

http://codereview.chromium.org/11309014/diff/31030/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/zip_file_creator.cc (right):

http://codereview.chromium.org/11309014/diff/31030/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/zip_file_creator.cc:43: // We assume that we
are started on the thread that the client wants us to do
On 2012/11/10 02:29:11, hshi1 wrote:
> Maybe the comments aren't clear, but this function (ZipFileCreator::Start())
can
> be any thread. The CHECK below is for whether we succeeded to get the thread
> identifier. Later in OnCreateZipFileSucceeded/Failed we're checking if it is
the
> same thread.
> 
> On 2012/11/06 19:41:25, tbarzic wrote:
> > maybe it would be easier to use the class if this can be any thread and we
> make
> > sure the Delegate methods are called on that thread.
> > We would just use BrowserThread::FILE when creating process host in
> > StartProcessOnIOThread (afaik, we can't really use any other thread).
> 

ah, ok..
Is there a reason this is called on file thread in file_browser_private_api.cc
also, if Start can be called on any thread, you should probably remove this
comment.

http://codereview.chromium.org/11309014/diff/43011/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

http://codereview.chromium.org/11309014/diff/43011/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:2985: if
(src_dir.AppendRelativePath(file_path, &relative_path))
I'd prefer it this failed in case there is a file that's not under |src_dir|
than silently ignore the file (as a user, I'd be surprised if a file I selected
is not successfully zipped file).

also, all the source files passed to the function should be relative to src_dir,
so |src_dir| not being |file_path| is an error in the extension code (so we
should let the extension know it's doing something wrong)

http://codereview.chromium.org/11309014/diff/43011/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:3004:
SendResponse(success);
how about

SetResult(new base::FundamentalValue(success));
SendResponse(true);


and add bool argument to zipSelection callback?

http://codereview.chromium.org/11309014/diff/43011/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/zip_file_creator.h (right):

http://codereview.chromium.org/11309014/diff/43011/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/zip_file_creator.h:32: // NOTE: This class
should only be used on the file thread.
Is this still true?

http://codereview.chromium.org/11309014/diff/43011/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/zip_file_creator.h:71: void ReportFailure();
similar to delegate methods,
these could be merged in something like ReportResult (but try to came up with a
better name)

http://codereview.chromium.org/11309014/diff/43011/chrome/browser/resources/f...
File chrome/browser/resources/file_manager/js/file_copy_manager.js (right):

http://codereview.chromium.org/11309014/diff/43011/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/file_copy_manager.js:1022: function
onZipSelectionComplete() {
what if zip failed? we should probably show an error in butter bar..

http://codereview.chromium.org/11309014/diff/43011/chrome/common/extensions/a...
File chrome/common/extensions/api/file_browser_private.json (right):

http://codereview.chromium.org/11309014/diff/43011/chrome/common/extensions/a...
chrome/common/extensions/api/file_browser_private.json:1043: "description":
"URLs of the selected files. Must be under the directory specified by dirURL.",
The files must be under the...

[hshi1, 2012-11-12 20:12:11]

http://codereview.chromium.org/11309014/diff/41009/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/zip_file_creator.h (right):

http://codereview.chromium.org/11309014/diff/41009/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/zip_file_creator.h:35: class Delegate {
On 2012/11/06 19:41:25, tbarzic wrote:
> Looks like this class reacts on events from the ZipFileCreator, maybe
|Observer|
> would be a better name for it.

Done.

http://codereview.chromium.org/11309014/diff/41009/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/zip_file_creator.h:75: // Our delegate's
thread. This is the thread we respond on.
On 2012/11/06 19:41:25, tbarzic wrote:
> "The delegate's thread" sounds better to me, but you decide..

Done.

http://codereview.chromium.org/11309014/diff/31030/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/zip_file_creator.cc (right):

http://codereview.chromium.org/11309014/diff/31030/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/zip_file_creator.cc:43: // We assume that we
are started on the thread that the client wants us to do
On 2012/11/10 03:40:56, tbarzic wrote:
> On 2012/11/10 02:29:11, hshi1 wrote:
> > Maybe the comments aren't clear, but this function (ZipFileCreator::Start())
> can
> > be any thread. The CHECK below is for whether we succeeded to get the thread
> > identifier. Later in OnCreateZipFileSucceeded/Failed we're checking if it is
> the
> > same thread.
> > 
> > On 2012/11/06 19:41:25, tbarzic wrote:
> > > maybe it would be easier to use the class if this can be any thread and we
> > make
> > > sure the Delegate methods are called on that thread.
> > > We would just use BrowserThread::FILE when creating process host in
> > > StartProcessOnIOThread (afaik, we can't really use any other thread).
> > 
> 
> ah, ok..
> Is there a reason this is called on file thread in file_browser_private_api.cc
> also, if Start can be called on any thread, you should probably remove this
> comment.

Done.

http://codereview.chromium.org/11309014/diff/31030/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/zip_file_creator.h (right):

http://codereview.chromium.org/11309014/diff/31030/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/zip_file_creator.h:57: friend class
ZipFileCreatorTest;
On 2012/11/06 19:41:26, tbarzic wrote:
> I don't see new tests :)

After landing the separate CL for zip::ZipFiles and its accompanying unit tests,
I think it is not really very necesssary to have a ZipFileCreatorTest class
(this class is essentially a wrapper). So removing the line.

http://codereview.chromium.org/11309014/diff/43011/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

http://codereview.chromium.org/11309014/diff/43011/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:2985: if
(src_dir.AppendRelativePath(file_path, &relative_path))
On 2012/11/10 03:40:56, tbarzic wrote:
> I'd prefer it this failed in case there is a file that's not under |src_dir|
> than silently ignore the file (as a user, I'd be surprised if a file I
selected
> is not successfully zipped file).
> 
> also, all the source files passed to the function should be relative to
src_dir,
> so |src_dir| not being |file_path| is an error in the extension code (so we
> should let the extension know it's doing something wrong)

Done.

http://codereview.chromium.org/11309014/diff/43011/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:3004:
SendResponse(success);
On 2012/11/10 03:40:56, tbarzic wrote:
> how about
> 
> SetResult(new base::FundamentalValue(success));
> SendResponse(true);
> 
> 
> and add bool argument to zipSelection callback?

Done.

http://codereview.chromium.org/11309014/diff/43011/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/zip_file_creator.h (right):

http://codereview.chromium.org/11309014/diff/43011/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/zip_file_creator.h:32: // NOTE: This class
should only be used on the file thread.
On 2012/11/10 03:40:56, tbarzic wrote:
> Is this still true?

You're right, removed.

http://codereview.chromium.org/11309014/diff/43011/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/zip_file_creator.h:71: void ReportFailure();
On 2012/11/10 03:40:56, tbarzic wrote:
> similar to delegate methods,
> these could be merged in something like ReportResult (but try to came up with
a
> better name)

Done.

http://codereview.chromium.org/11309014/diff/43011/chrome/browser/resources/f...
File chrome/browser/resources/file_manager/js/file_copy_manager.js (right):

http://codereview.chromium.org/11309014/diff/43011/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/file_copy_manager.js:1022: function
onZipSelectionComplete() {
On 2012/11/10 03:40:56, tbarzic wrote:
> what if zip failed? we should probably show an error in butter bar..

Done.

http://codereview.chromium.org/11309014/diff/43011/chrome/common/extensions/a...
File chrome/common/extensions/api/file_browser_private.json (right):

http://codereview.chromium.org/11309014/diff/43011/chrome/common/extensions/a...
chrome/common/extensions/api/file_browser_private.json:1043: "description":
"URLs of the selected files. Must be under the directory specified by dirURL.",
On 2012/11/10 03:40:56, tbarzic wrote:
> The files must be under the...

Done.

[tbarzic, 2012-11-12 20:29:37]

http://codereview.chromium.org/11309014/diff/31054/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

http://codereview.chromium.org/11309014/diff/31054/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:2995:
BrowserThread::FILE, FROM_HERE,
it should be save to call zip_file_creator_->Start() on UI thread, right?

http://codereview.chromium.org/11309014/diff/31054/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/zip_file_creator.cc (right):

http://codereview.chromium.org/11309014/diff/31054/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/zip_file_creator.cc:91: void
ZipFileCreator::OnCreateZipFileFailed() {
should there be
if (thread_identifier_ != BrowserThread::ID_COUNT)

like we have in OnCreateZipSucceeded?

[hshi1, 2012-11-12 21:42:17]

http://codereview.chromium.org/11309014/diff/31054/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

http://codereview.chromium.org/11309014/diff/31054/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:2995:
BrowserThread::FILE, FROM_HERE,
On 2012/11/12 20:29:37, tbarzic wrote:
> it should be save to call zip_file_creator_->Start() on UI thread, right?

You're right! Done.

http://codereview.chromium.org/11309014/diff/31054/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/zip_file_creator.cc (right):

http://codereview.chromium.org/11309014/diff/31054/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/zip_file_creator.cc:91: void
ZipFileCreator::OnCreateZipFileFailed() {
On 2012/11/12 20:29:37, tbarzic wrote:
> should there be
> if (thread_identifier_ != BrowserThread::ID_COUNT)
> 
> like we have in OnCreateZipSucceeded?
> 

Done.

[tbarzic, 2012-11-12 21:43:31]

lgtm

[hshi1, 2012-11-13 18:47:05]

Ping again for review for changes under the chrome common directories.

Adding jln@ and palmer@ for security review regarding the new IPC messages
introduced.

[James Hawkins, 2012-11-13 18:48:17]

On 2012/11/13 18:47:05, hshi1 wrote:
> Ping again for review for changes under the chrome common directories.
> 

Who, specifically, are you pinging?  It helps if you call people out so they
know what they're supposed to review.

> Adding jln@ and palmer@ for security review regarding the new IPC messages
> introduced.

[hshi1, 2012-11-13 18:53:55]

Need owner review for the following:

mihaip@: 
  chrome/browser/extensions/extension_function_registry.cc

jhawkins@, thakis@:  
  chrome/chrome_browser_chromeos.gypi
  chrome/common/extensions/api/file_browser_private.json
  chrome/utility/chrome_content_utility_client.h
  chrome/utility/chrome_content_utility_client.cc

jln@, palmer@: (security review for IPC message addition)
  chrome/common/chrome_utility_messages.h

Already reviewed:

  chrome/browser/chromeos/extensions/*
  chrome/browser/resources/file_manager/js/*

[James Hawkins, 2012-11-13 18:56:58]

On 2012/11/13 18:53:55, hshi1 wrote:
> Need owner review for the following:
> 
> mihaip@: 
>   chrome/browser/extensions/extension_function_registry.cc
> 
> jhawkins@, thakis@:  
>   chrome/chrome_browser_chromeos.gypi
>   chrome/common/extensions/api/file_browser_private.json
>   chrome/utility/chrome_content_utility_client.h
>   chrome/utility/chrome_content_utility_client.cc
> 

Why do we both need to review these files (perhaps I'm missing some context)?

> jln@, palmer@: (security review for IPC message addition)
>   chrome/common/chrome_utility_messages.h
> 
> Already reviewed:
> 
>   chrome/browser/chromeos/extensions/*
>   chrome/browser/resources/file_manager/js/*

[hshi1, 2012-11-13 18:58:35]

Sorry about that. Removing thakis@ and jln@ -- just keep one owner for each set
of files, to avoid dilution of responsibility.

[Mihai Parparita -not on Chrome, 2012-11-13 18:58:38]

s/mihaip/miket/

[James Hawkins, 2012-11-13 19:05:07]

lgtm

[palmer, 2012-11-13 20:43:40]

Re-adding jln for another pair of eyes. We need to make sure this IPC does not
become another arbitrary file read/write primitive under the control of a
compromised renderer (or other sandboxed process).

https://codereview.chromium.org/11309014/diff/34058/chrome/utility/chrome_con...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/11309014/diff/34058/chrome/utility/chrome_con...
chrome/utility/chrome_content_utility_client.cc:195: if (zip::ZipFiles(src_dir,
src_relative_paths, dest_file))
As it is, this function looks very dangerous.

zip::ZipFiles does no validation of the pathnames. As you say in
chrome/common/zip.cc,

178   // TODO(hshi): check that |src_relative_paths| does not contain
|dest_file|.

I'd also add that you should enforce that src_relative_paths are, in fact,
relative to src_dir.

Additionally, we need to make sure this is not an arbitrary file-write primitive
(e.g. one that overwrites existing, believed-trustworthy zip files, or any other
files).

This message-receiving code should determine |dest_file| itself; or,
alternately, validate |dest_file| strictly and then incorporate it into the
final pathanme as a suggestion from the caller. For example (in pseudocode):

    dest_file = strip_non_ascii(dest_file);
    dest_file = basename(dest_file);
    dest_file = strip_extension(dest_file);
    dest_file = strip_spaces(dest_file);
    dest_file = kZipFilesParent + kDirectorySeparator + dest_file + ".zip";

It should probably pass zip::ZipFiles an open HANDLE or file descriptor, rather
than a pathname, to avoid race conditions and name collisions. That probably
requires a new function overload for zip::ZipFiles.

    dest_descriptor = open(dest_file, O_CREAT|O_EXCL|O_TRUNC|O_WRONLY, 0644); 
// And whatever is appropriate for Windows

Similarly, there should be something like:

    foreach (pathname in src_relative_paths) {
        pathname = normalize_pathname(pathname);  // No "../" tricks, et c.
        if (is_absolute_path(pathname))
            fail();
        validated_src_paths.push_back(pathname);
    }

    zip::ZipFiles(src_dir, validated_src_paths, dest_descriptor);

[Jorge Lucangeli Obes, 2012-11-13 21:03:35]

https://codereview.chromium.org/11309014/diff/34058/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/zip_file_creator.h (right):

https://codereview.chromium.org/11309014/diff/34058/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/zip_file_creator.h:21: // from untrusted
sources.
How is the subprocess sandboxed? The utility process is not sandboxed by
default. Moreover, you cannot sandbox a process that needs to access the file
system, which the implementation of zip::ZipFiles does.

The safe way to do this is to pass the utility process a set of file
descriptors, not a set of file names.

[hshi1, 2012-11-13 22:07:49]

>> The utility process is not sandboxed by
default.

For this case, is it necessary to sandbox the utility process? The intention is
just to separate the utility process from the main browser process so that a
crash in there does not affect the browser itself. It seems a rather futile
exercise to obtain file descriptors in the browser process and then pass them to
another separate utility process, which is created by the browser process
anyway.

[hshi1, 2012-11-13 22:18:08]

Chris,

I understand the part about path validation. But is it really necessary to
change zip::ZipFiles to use file descriptors? As long as the proper path
validation logic is in we should be able to ensure that the dest path does not
collide with any other source files.

On 2012/11/13 20:43:40, Chris P. wrote:
> Re-adding jln for another pair of eyes. We need to make sure this IPC does not
> become another arbitrary file read/write primitive under the control of a
> compromised renderer (or other sandboxed process).
> 
>
https://codereview.chromium.org/11309014/diff/34058/chrome/utility/chrome_con...
> File chrome/utility/chrome_content_utility_client.cc (right):
> 
>
https://codereview.chromium.org/11309014/diff/34058/chrome/utility/chrome_con...
> chrome/utility/chrome_content_utility_client.cc:195: if
(zip::ZipFiles(src_dir,
> src_relative_paths, dest_file))
> As it is, this function looks very dangerous.
> 
> zip::ZipFiles does no validation of the pathnames. As you say in
> chrome/common/zip.cc,
> 
> 178   // TODO(hshi): check that |src_relative_paths| does not contain
> |dest_file|.
> 
> I'd also add that you should enforce that src_relative_paths are, in fact,
> relative to src_dir.
> 
> Additionally, we need to make sure this is not an arbitrary file-write
primitive
> (e.g. one that overwrites existing, believed-trustworthy zip files, or any
other
> files).
> 
> This message-receiving code should determine |dest_file| itself; or,
> alternately, validate |dest_file| strictly and then incorporate it into the
> final pathanme as a suggestion from the caller. For example (in pseudocode):
> 
>     dest_file = strip_non_ascii(dest_file);
>     dest_file = basename(dest_file);
>     dest_file = strip_extension(dest_file);
>     dest_file = strip_spaces(dest_file);
>     dest_file = kZipFilesParent + kDirectorySeparator + dest_file + ".zip";
> 
> It should probably pass zip::ZipFiles an open HANDLE or file descriptor,
rather
> than a pathname, to avoid race conditions and name collisions. That probably
> requires a new function overload for zip::ZipFiles.
> 
>     dest_descriptor = open(dest_file, O_CREAT|O_EXCL|O_TRUNC|O_WRONLY, 0644); 
> // And whatever is appropriate for Windows
> 
> Similarly, there should be something like:
> 
>     foreach (pathname in src_relative_paths) {
>         pathname = normalize_pathname(pathname);  // No "../" tricks, et c.
>         if (is_absolute_path(pathname))
>             fail();
>         validated_src_paths.push_back(pathname);
>     }
> 
>     zip::ZipFiles(src_dir, validated_src_paths, dest_descriptor);

[Jorge Lucangeli Obes, 2012-11-13 22:20:34]

On 2012/11/13 22:07:49, hshi1 wrote:
> >> The utility process is not sandboxed by
> default.
> 
> For this case, is it necessary to sandbox the utility process? The intention
is
> just to separate the utility process from the main browser process so that a
> crash in there does not affect the browser itself. It seems a rather futile
> exercise to obtain file descriptors in the browser process and then pass them
to
> another separate utility process, which is created by the browser process
> anyway.

Running as a separate, unsandboxed process provides very little added security.
The unsandboxed utility process can access/do the same things that the browser
process can.

In this case, since this thing is zipping and not unzipping, there's less risk
that malicious files might exploit a bug in the zipping algorithm, but it's not
impossible.

In any case, the comment on that file is inaccurate, the process is *not*
sandboxed, and thus provides *no* added security, and no protection against
untrusted data.

If this was reimplemented using file descriptors, then the utility process could
run sandboxed, without FS access, and it *would* provide protection against
untrusted data.

Chris's comments, which are centered on IPC, are of course still valid.

[miket_OOO, 2012-11-13 22:37:09]

chrome/browser/extensions/extension_function_registry.cc LGTM

[hshi1, 2012-11-13 22:39:19]

I see. I guess the question is whether it is acceptable for the zip-file utility
process to be installed?

If you look at the existing utility IPC messages and their implementation (for
example, JSON parser, extension unpacker, etc.) you'll notice that they are all
passing FilePath, not file descriptors. I guess that means these are not secure
either?

On 2012/11/13 22:20:34, Jorge Lucangeli Obes wrote:
> On 2012/11/13 22:07:49, hshi1 wrote:
> > >> The utility process is not sandboxed by
> > default.
> > 
> > For this case, is it necessary to sandbox the utility process? The intention
> is
> > just to separate the utility process from the main browser process so that a
> > crash in there does not affect the browser itself. It seems a rather futile
> > exercise to obtain file descriptors in the browser process and then pass
them
> to
> > another separate utility process, which is created by the browser process
> > anyway.
> 
> Running as a separate, unsandboxed process provides very little added
security.
> The unsandboxed utility process can access/do the same things that the browser
> process can.
> 
> In this case, since this thing is zipping and not unzipping, there's less risk
> that malicious files might exploit a bug in the zipping algorithm, but it's
not
> impossible.
> 
> In any case, the comment on that file is inaccurate, the process is *not*
> sandboxed, and thus provides *no* added security, and no protection against
> untrusted data.
> 
> If this was reimplemented using file descriptors, then the utility process
could
> run sandboxed, without FS access, and it *would* provide protection against
> untrusted data.
> 
> Chris's comments, which are centered on IPC, are of course still valid.

[hshi1, 2012-11-13 22:40:06]

Sorry for the typo. I meant "for the zip-file utility process to be not
sandboxed".

On 2012/11/13 22:39:19, hshi1 wrote:
> I see. I guess the question is whether it is acceptable for the zip-file
utility
> process to be installed?
> 
> If you look at the existing utility IPC messages and their implementation (for
> example, JSON parser, extension unpacker, etc.) you'll notice that they are
all
> passing FilePath, not file descriptors. I guess that means these are not
secure
> either?
> 
> On 2012/11/13 22:20:34, Jorge Lucangeli Obes wrote:
> > On 2012/11/13 22:07:49, hshi1 wrote:
> > > >> The utility process is not sandboxed by
> > > default.
> > > 
> > > For this case, is it necessary to sandbox the utility process? The
intention
> > is
> > > just to separate the utility process from the main browser process so that
a
> > > crash in there does not affect the browser itself. It seems a rather
futile
> > > exercise to obtain file descriptors in the browser process and then pass
> them
> > to
> > > another separate utility process, which is created by the browser process
> > > anyway.
> > 
> > Running as a separate, unsandboxed process provides very little added
> security.
> > The unsandboxed utility process can access/do the same things that the
browser
> > process can.
> > 
> > In this case, since this thing is zipping and not unzipping, there's less
risk
> > that malicious files might exploit a bug in the zipping algorithm, but it's
> not
> > impossible.
> > 
> > In any case, the comment on that file is inaccurate, the process is *not*
> > sandboxed, and thus provides *no* added security, and no protection against
> > untrusted data.
> > 
> > If this was reimplemented using file descriptors, then the utility process
> could
> > run sandboxed, without FS access, and it *would* provide protection against
> > untrusted data.
> > 
> > Chris's comments, which are centered on IPC, are of course still valid.

[Jorge Lucangeli Obes, 2012-11-13 22:42:00]

On 2012/11/13 22:18:08, hshi1 wrote:
> Chris,
> 
> I understand the part about path validation. But is it really necessary to
> change zip::ZipFiles to use file descriptors? As long as the proper path
> validation logic is in we should be able to ensure that the dest path does not
> collide with any other source files.

Or any other file on the system.

> On 2012/11/13 20:43:40, Chris P. wrote:
> > Re-adding jln for another pair of eyes. We need to make sure this IPC does
not
> > become another arbitrary file read/write primitive under the control of a
> > compromised renderer (or other sandboxed process).
> > 
> >
>
https://codereview.chromium.org/11309014/diff/34058/chrome/utility/chrome_con...
> > File chrome/utility/chrome_content_utility_client.cc (right):
> > 
> >
>
https://codereview.chromium.org/11309014/diff/34058/chrome/utility/chrome_con...
> > chrome/utility/chrome_content_utility_client.cc:195: if
> (zip::ZipFiles(src_dir,
> > src_relative_paths, dest_file))
> > As it is, this function looks very dangerous.
> > 
> > zip::ZipFiles does no validation of the pathnames. As you say in
> > chrome/common/zip.cc,
> > 
> > 178   // TODO(hshi): check that |src_relative_paths| does not contain
> > |dest_file|.
> > 
> > I'd also add that you should enforce that src_relative_paths are, in fact,
> > relative to src_dir.
> > 
> > Additionally, we need to make sure this is not an arbitrary file-write
> primitive
> > (e.g. one that overwrites existing, believed-trustworthy zip files, or any
> other
> > files).
> > 
> > This message-receiving code should determine |dest_file| itself; or,
> > alternately, validate |dest_file| strictly and then incorporate it into the
> > final pathanme as a suggestion from the caller. For example (in pseudocode):
> > 
> >     dest_file = strip_non_ascii(dest_file);
> >     dest_file = basename(dest_file);
> >     dest_file = strip_extension(dest_file);
> >     dest_file = strip_spaces(dest_file);
> >     dest_file = kZipFilesParent + kDirectorySeparator + dest_file + ".zip";
> > 
> > It should probably pass zip::ZipFiles an open HANDLE or file descriptor,
> rather
> > than a pathname, to avoid race conditions and name collisions. That probably
> > requires a new function overload for zip::ZipFiles.
> > 
> >     dest_descriptor = open(dest_file, O_CREAT|O_EXCL|O_TRUNC|O_WRONLY,
0644); 
> > // And whatever is appropriate for Windows
> > 
> > Similarly, there should be something like:
> > 
> >     foreach (pathname in src_relative_paths) {
> >         pathname = normalize_pathname(pathname);  // No "../" tricks, et c.
> >         if (is_absolute_path(pathname))
> >             fail();
> >         validated_src_paths.push_back(pathname);
> >     }
> > 
> >     zip::ZipFiles(src_dir, validated_src_paths, dest_descriptor);

[Jorge Lucangeli Obes, 2012-11-13 22:43:39]

On 2012/11/13 22:39:19, hshi1 wrote:
> I see. I guess the question is whether it is acceptable for the zip-file
utility
> process to be installed?
> 
> If you look at the existing utility IPC messages and their implementation (for
> example, JSON parser, extension unpacker, etc.) you'll notice that they are
all
> passing FilePath, not file descriptors. I guess that means these are not
secure
> either?

That is not correct. The only ChromeUtilityMsg_* that includes a FilePath is
extension unpacking. This is an exception, because it's not trivial to unpack
extensions in memory.

JSON parsing takes a string:

// Tell the utility process to parse a JSON string into a Value object.
IPC_MESSAGE_CONTROL1(ChromeUtilityMsg_ParseJSON,
                     std::string /* JSON to parse */)

> On 2012/11/13 22:20:34, Jorge Lucangeli Obes wrote:
> > On 2012/11/13 22:07:49, hshi1 wrote:
> > > >> The utility process is not sandboxed by
> > > default.
> > > 
> > > For this case, is it necessary to sandbox the utility process? The
intention
> > is
> > > just to separate the utility process from the main browser process so that
a
> > > crash in there does not affect the browser itself. It seems a rather
futile
> > > exercise to obtain file descriptors in the browser process and then pass
> them
> > to
> > > another separate utility process, which is created by the browser process
> > > anyway.
> > 
> > Running as a separate, unsandboxed process provides very little added
> security.
> > The unsandboxed utility process can access/do the same things that the
browser
> > process can.
> > 
> > In this case, since this thing is zipping and not unzipping, there's less
risk
> > that malicious files might exploit a bug in the zipping algorithm, but it's
> not
> > impossible.
> > 
> > In any case, the comment on that file is inaccurate, the process is *not*
> > sandboxed, and thus provides *no* added security, and no protection against
> > untrusted data.
> > 
> > If this was reimplemented using file descriptors, then the utility process
> could
> > run sandboxed, without FS access, and it *would* provide protection against
> > untrusted data.
> > 
> > Chris's comments, which are centered on IPC, are of course still valid.

[jln (very slow on Chromium), 2012-11-13 22:45:09]

On Tue, Nov 13, 2012 at 2:42 PM,  <jorgelo@chromium.org> wrote:
> On 2012/11/13 22:18:08, hshi1 wrote:
>>
>> Chris,
>
>
>> I understand the part about path validation. But is it really necessary to
>> change zip::ZipFiles to use file descriptors? As long as the proper path
>> validation logic is in we should be able to ensure that the dest path does
>> not
>> collide with any other source files.
>
>
> Or any other file on the system.

Or any sensitive file path on the system, existing or not. Really this
boils down to making sure that nothing untrusted can send an IPC to
the utility process.

If that's not the case, we probably have security issues already. If
that's the case, we should add a comment explaining this, and somehow
make sure that it does not regress.

[hshi1, 2012-11-13 22:56:16]

Jorge, would it be ok if I just clarify in the comments that the utility process
is not really sandboxed? It wouldn't be very difficult to validate the file
paths (only need to ensure that the src/dst directories are in permitted
locations, that the relative paths are normalized, and the output file does not
already exist).

It will not be feasible to create the zip file in memory, as the underlying zip
tool is based on files.

Also, the ZipFiles call may add thousands of files (recursively under selected
files/folders), so I think it will be quite messy to require the caller to open
up all the file descriptors first, then pass them in a giant array to the
utility process. Rather, the ZipFiles opens the source files one by one, and
close them after they're read.

[palmer, 2012-11-13 22:59:55]

> I understand the part about path validation. But is it really necessary to
> change zip::ZipFiles to use file descriptors? As long as the proper path
> validation logic is in we should be able to ensure that the dest path does not
> collide with any other source files.

My immediate concern is that we not create any unsafe IPC receivers. Even though
we intend for the browser process (already fully trusted) to be sending these
pathnames to the utility process (currently the browser process' fully trusted
child, but that is sub-optimal), attackers (such as attackers who have
compromised a renderer) might very well be able to invoke these IPC endpoints
for nefarious purposes. We suffered just such an attack in the recent Pwnium 2
contest, and in fact that is why chrome-security team members are now OWNERs of
these IPC endpoints files. :)

Now, a render can't open files. But with this IPC API, they could. The utility
is a very confused deputy right now.
(http://en.wikipedia.org/wiki/Confused_deputy_problem)

So, although we can mitigate our immediate-term concerns by doing very careful
pathname validation, I'd like to argue that the better approach is definitely to
use file descriptors rather than pathnames — and that such a fix is also more
robust in the short term, too.

In general, not just for security reasons, it is better to define APIs in terms
of file descriptors:

* they are simpler — no complex internal structure; they are just opaque
references to objects that have read/write/seek/stat methods (and maybe not even
all of those)
** Compare with the surprisingly complex semantics of pathname parsing — does
implementing those pathname validation rules I described sound like fun?
Probably not. :)

* they are transferable, yet unforgeable
** "Here is a thing. It is exactly the thing I want you to have, and will never
be misinterpreted (by you or anyone else) as another thing."

* the callee does not have to handle errors related to acquiring the descriptor
they will eventually need anyway
** it is often very hard to bubble up errors in a meaningful way, back up to the
caller. Often the code path back up to the caller crosses many modules, and in
this case a process boundary. How does the utility process tell the browser that
it couldn't find some of the files to zip, for example?
** "Here is a thing. It is exactly the thing I want you to have. It works, I
just checked."

* there are no race conditions
** "Here is a thing. It is the very thing I want you have right now. It is not a
weak reference that may mean something else in a moment or two."

For these reasons, APIs using file descriptors are sandboxable, robust, and
afford user-friendly error handling, which is a huge win not just for security
but also for reliability and usability generally.

Ultimately we should seek to sandbox that utility process; if its IPCs use
descriptors, we have a decent chance of success without having to do a large
refactor first. As Jorge notes, we want to sandbox the utility process sooner
rather than later. Archiving and compression have historically been wonderful
(i.e. terrible) attack surfaces.

In general, pathnames are an anti-pattern in API design; doubly so across a
privilege boundary. open(2) and CreateFile, invoked directly by the original
caller, should be the sole means of converting pathnames (weak references) into
objects (well, opaque/unforgeable/transferable references to objects).

[hshi1, 2012-11-13 23:16:05]

Jorge, Chris -- thanks for all the feedback. I agree that it would be the better
approach to use file descriptors in the API, rather than land this first then
fix it later.

[Jorge Lucangeli Obes, 2012-11-13 23:18:22]

On 2012/11/13 22:56:16, hshi1 wrote:
> Jorge, would it be ok if I just clarify in the comments that the utility
process
> is not really sandboxed? It wouldn't be very difficult to validate the file
> paths (only need to ensure that the src/dst directories are in permitted
> locations, that the relative paths are normalized, and the output file does
not
> already exist).
> 
> It will not be feasible to create the zip file in memory, as the underlying
zip
> tool is based on files.
> 
> Also, the ZipFiles call may add thousands of files (recursively under selected
> files/folders), so I think it will be quite messy to require the caller to
open
> up all the file descriptors first, then pass them in a giant array to the
> utility process. Rather, the ZipFiles opens the source files one by one, and
> close them after they're read.

It's not necessary to open all the fd's at once. You can use the fd for a
directory together with openat() to get to all the fd's you need.

However, it starts to get weird because we don't allow sending dir fd's from the
browser to other processes, because this would allow them to escape the setuid
sandbox.

Chris's "no unsafe IPC receivers" is a very good rule to avoid regressions
turning into security issues.

As a minimum, the dest file should be passed as an fd to the utility process.
Plumbing fd's into the zip file code is not hard:
https://chromiumcodereview.appspot.com/8873039/

[hshi1, 2012-11-13 23:20:29]

>> As a minimum, the dest file should be passed as an fd to the utility process.

Sounds good and this is pretty actionable. Thanks.

[Jorge Lucangeli Obes, 2012-11-13 23:28:09]

On 2012/11/13 23:20:29, hshi1 wrote:
> >> As a minimum, the dest file should be passed as an fd to the utility
process.
> 
> Sounds good and this is pretty actionable. Thanks.

Since you have src_dir and src_files in your IPC message, you should use
openat() in the utility process/zip file handling code to open the files to be
compressed. That way, the code will be easy to convert to something that
receives no absolute paths.

I.e.:

absolute_src_dir = absolute(src_dir)
check(absolute_src_dir) // make sure we're not reading from wherever

dir_fd = open(absolute_src_dir)

for rel_src_file in src_files:
  check(rel_src_file) // i.e. they should not have .. or .
  openat(dir_fd, rel_src_file)

We have utility functions that turn paths into absolute paths.

[hshi1, 2012-11-15 18:21:54]

>> for rel_src_file in src_files:
  check(rel_src_file) // i.e. they should not have .. or .
  openat(dir_fd, rel_src_file)

This will require a larger change in the zip/zip_internal.cc. 

I'd like to opt to passing the dest file as fd to the utility process, and
implement the path validation as suggested by you and Chris.

[Jorge Lucangeli Obes, 2012-11-15 20:08:33]

On 2012/11/15 18:21:54, hshi1 wrote:
> >> for rel_src_file in src_files:
>   check(rel_src_file) // i.e. they should not have .. or .
>   openat(dir_fd, rel_src_file)
> 
> This will require a larger change in the zip/zip_internal.cc. 
> 
> I'd like to opt to passing the dest file as fd to the utility process, and
> implement the path validation as suggested by you and Chris.

OK, so to summarize:

The IPC message will contain src_dir, src_files, dst_fd. That's what the utility
process will receive.

Path validation will need to happen both in the browser process before opening
the dest fd that will be passed in the IPC message, and in the utility process
before opening the src files for zipping.

Is this going to be a Chrome OS thing? In that case, the IPC message could be
#ifdef'd.

Moreover, if this is only a Chrome OS thing, it would be *really* good to use
openat(). I haven't worked with zip_internal.cc in a while, is it problematic to
plumb the fd's?

[hshi1, 2012-11-20 19:10:07]

>> is it problematic to plumb the fd's?

Hi Jorge, sorry for the delayed reply. I have no problem to put all the zip file
stuff in #if defined(OS_CHROMEOS).

There is indeed some problem with plumbing the fd's for the source files. In
fact we open and read the source files into buffers in zip.cc using
net::FileStream class and supply the data to zlib. If we want to pass a
src_dir_fd and use openat() then we can't use net::FileStream, which means a
different path needs to be created for reading from fd's. Of course this is
doable but I just wonder if this added complexity is really necessary...? As you
mentioned in comment #37, we can't really pass the src_dir_fd in IPC messages,
so it won't really improve security.

[Jorge Lucangeli Obes, 2012-11-20 19:23:10]

On 2012/11/20 19:10:07, hshi1 wrote:
> >> is it problematic to plumb the fd's?
> 
> Hi Jorge, sorry for the delayed reply. I have no problem to put all the zip
file
> stuff in #if defined(OS_CHROMEOS).
> 
> There is indeed some problem with plumbing the fd's for the source files. In
> fact we open and read the source files into buffers in zip.cc using
> net::FileStream class and supply the data to zlib. If we want to pass a
> src_dir_fd and use openat() then we can't use net::FileStream, which means a
> different path needs to be created for reading from fd's. Of course this is
> doable but I just wonder if this added complexity is really necessary...? As
you
> mentioned in comment #37, we can't really pass the src_dir_fd in IPC messages,
> so it won't really improve security.

I wanted to be clear on the difficulty of implementing it. It is correct that
right now we cannot pass a dir fd to the utility process, and we'll need to make
major changes to allow that.

So for now I think we can settle on passing an fd for the destination file, and
paths for the source dir and source files.

Make sure to check the dst file in the browser process before opening the fd;
check the src dir to make sure it's inside the FS that's exposed to the Chrome
OS user, and make sure that the src files are inside the src dir, and be careful
with directory traversals using '..'.

Moreover, if we don't need this IPC outside of Chrome OS, we don't need to add
the attack surface to other platforms.

Thanks!

[palmer, 2012-11-20 19:30:02]

> So for now I think we can settle on passing an fd for the destination file,
and
> paths for the source dir and source files.

I agree. We can do the fd thing later — it's a big enough change that it
deserves to be its own CL. We will get an improvement in safety with this
dest_fd and source pathname validation, so I feel ok for now.

Thank you for all your work, hshi1!

[hshi1, 2012-11-21 23:04:54]

Jorge/Chris, please review Patch Set #30.
What I did in patches 29,30:
(1) use base::PlatformFile to handle the POSIX-style file creation. Set creation
flags so that we fail if the dest file already exists (solves the overwrite
problem).
(2) add sanitization logic in the ChromeContentUtilityClient::OnCreateZipFile()
to reject absolute paths or any paths that contain attempts to reference parent
("../" tricks, etc.)

What I did NOT do is to add an additional verification step in browser process
to check that user have permission to access source and dest path. This is
unnecessary, because in file_browser_private_api.cc, all the file paths are
converted from the file browser extension URLs (see
FileBrowserFunction::GetLocalPathsOnFileThreadAndRunCallbackOnUIThread()). Only
paths from registered mount points are accepted.

[palmer, 2012-11-22 00:27:21]

LGTM. Thank you!

[Jorge Lucangeli Obes, 2012-11-22 00:28:20]

lgtm, but please fix the comment if you can. Chris P needs to ACK for OWNERS.

https://codereview.chromium.org/11309014/diff/57028/chrome/utility/chrome_con...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/11309014/diff/57028/chrome/utility/chrome_con...
chrome/utility/chrome_content_utility_client.cc:202: // Check sanity for source
relative paths. Reject if path is absolute or
"Check sanity of" or "Sanity check for".

[hshi1, 2012-11-22 00:35:45]

https://codereview.chromium.org/11309014/diff/57028/chrome/utility/chrome_con...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/11309014/diff/57028/chrome/utility/chrome_con...
chrome/utility/chrome_content_utility_client.cc:202: // Check sanity for source
relative paths. Reject if path is absolute or
On 2012/11/22 00:28:20, Jorge Lucangeli Obes wrote:
> "Check sanity of" or "Sanity check for".

Done.

[commit-bot: I haz the power, 2012-11-22 00:40:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/11309014/57033

[commit-bot: I haz the power, 2012-11-22 01:02:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/11309014/61014

[commit-bot: I haz the power, 2012-11-22 03:49:36]

Change committed as 169212