base::CopyFile can copy *from* Android's content scheme.

base::CopyFile can copy *from* Android's content scheme.

Some other select file functions can deal with Android's content scheme (i.e.
content://...), but not CopyFile.

BUG=416424
Committed: https://crrev.com/620ace8dc5084fd162a98b90ee78846980879e6f
Cr-Commit-Position: refs/heads/master@{#304446}

[cmumford, 2014-09-25 00:16:35]

cmumford@chromium.org changed reviewers:
+ jsbell@chromium.org, rvargas@chromium.org

[cmumford, 2014-09-25 00:16:35]

rvargas for review, jsbell for FYI

I'm not 100% sure this is the correct way to fix 416424, but wanted to put up
the change for review & discussion.

The prior version created the file copy with mode 0666, which I think is a bit
wide open (writable by others!). This change uses the default mask (usually
0644). Also, on the topic of security, I'm not sure if it is less secure to
allow copying from the content scheme, but I suspect it's no different since the
networking code can read from there today.

[jsbell, 2014-09-25 17:24:15]

Thanks for the FYI.

I'd keep the file permissions the same; I would avoid introducing any behavior
change in the general case. File a bug/use separate CL for changing the
permissions if you feel strongly about it.

[rvargas (doing something else), 2014-09-25 20:28:39]

I think it is reasonable to have base methods that understand special path
encodings for a given platform. The expectation is that file related methods
should work with anything that provides a standard FS interface, and given that
OpenContentURIForRead returns a standard base::File, it seems fine to update
CopyFile to deal with it.

As for the flags, we should have consistent behavior between a file created with
CopyFile and files created with other functions from base. I just created bug
417830 for it.

In other words, this change looks fine to me in that at least reduces one place
of deciding what to use; not sure if we want to land it before agreeing about
flags on 417830.

[cmumford, 2014-09-26 16:55:54]

On 2014/09/25 20:28:39, rvargas wrote:
> I think it is reasonable to have base methods that understand special path
> encodings for a given platform. The expectation is that file related methods
> should work with anything that provides a standard FS interface, and given
that
> OpenContentURIForRead returns a standard base::File, it seems fine to update
> CopyFile to deal with it.
> 
> As for the flags, we should have consistent behavior between a file created
with
> CopyFile and files created with other functions from base. I just created bug
> 417830 for it.
> 
> In other words, this change looks fine to me in that at least reduces one
place
> of deciding what to use; not sure if we want to land it before agreeing about
> flags on 417830.

Thanks rvargas@. I was pointed at this information from another developer:

http://developer.android.com/guide/topics/providers/content-provider-basics.h...

which basically confirms my suspicion that content:// scheme's don't always
refer to files, so it isn't possible to strip off the scheme and do a standard
file copy. So I believe this is the correct fix to make, but I do still want to
address the umask question. Given that I'm restricting the umask I want to at
least have one of the security guys weigh in on 417830 before moving ahead with
this change.

[rvargas (doing something else), 2014-09-26 19:24:57]

On 2014/09/26 16:55:54, cmumford wrote:
> On 2014/09/25 20:28:39, rvargas wrote:
> > I think it is reasonable to have base methods that understand special path
> > encodings for a given platform. The expectation is that file related methods
> > should work with anything that provides a standard FS interface, and given
> that
> > OpenContentURIForRead returns a standard base::File, it seems fine to update
> > CopyFile to deal with it.
> > 
> > As for the flags, we should have consistent behavior between a file created
> with
> > CopyFile and files created with other functions from base. I just created
bug
> > 417830 for it.
> > 
> > In other words, this change looks fine to me in that at least reduces one
> place
> > of deciding what to use; not sure if we want to land it before agreeing
about
> > flags on 417830.
> 
> Thanks rvargas@. I was pointed at this information from another developer:
> 
>
http://developer.android.com/guide/topics/providers/content-provider-basics.h...
> 
> which basically confirms my suspicion that content:// scheme's don't always
> refer to files, so it isn't possible to strip off the scheme and do a standard
> file copy. So I believe this is the correct fix to make, but I do still want
to
> address the umask question. Given that I'm restricting the umask I want to at
> least have one of the security guys weigh in on 417830 before moving ahead
with
> this change.

Let me rephrase what I said before: if it is possible to go from [random object
type with some special name] to [file descriptor / handle that implements a
standard FS interface] then it's fine to handle the special case in base code
that is meant to deal with files. If the returned object is close to a standard
fd, but with some quirks that prevent some functions to work properly, it makes
more sense to write code somewhere else that is specifically tailored to deal
with that particular type of object.

I really never considered the case of code stripping the scheme and working on
the rest of the name as if it is a standard file path.

[cmumford, 2014-09-30 20:06:44]

On 2014/09/26 19:24:57, rvargas wrote:
> On 2014/09/26 16:55:54, cmumford wrote:
> > On 2014/09/25 20:28:39, rvargas wrote:
> > > I think it is reasonable to have base methods that understand special path
> > > encodings for a given platform. The expectation is that file related
methods
> > > should work with anything that provides a standard FS interface, and given
> > that
> > > OpenContentURIForRead returns a standard base::File, it seems fine to
update
> > > CopyFile to deal with it.
> > > 
> > > As for the flags, we should have consistent behavior between a file
created
> > with
> > > CopyFile and files created with other functions from base. I just created
> bug
> > > 417830 for it.
> > > 
> > > In other words, this change looks fine to me in that at least reduces one
> > place
> > > of deciding what to use; not sure if we want to land it before agreeing
> about
> > > flags on 417830.
> > 
> > Thanks rvargas@. I was pointed at this information from another developer:
> > 
> >
>
http://developer.android.com/guide/topics/providers/content-provider-basics.h...
> > 
> > which basically confirms my suspicion that content:// scheme's don't always
> > refer to files, so it isn't possible to strip off the scheme and do a
standard
> > file copy. So I believe this is the correct fix to make, but I do still want
> to
> > address the umask question. Given that I'm restricting the umask I want to
at
> > least have one of the security guys weigh in on 417830 before moving ahead
> with
> > this change.
> 
> Let me rephrase what I said before: if it is possible to go from [random
object
> type with some special name] to [file descriptor / handle that implements a
> standard FS interface] then it's fine to handle the special case in base code
> that is meant to deal with files. If the returned object is close to a
standard
> fd, but with some quirks that prevent some functions to work properly, it
makes
> more sense to write code somewhere else that is specifically tailored to deal
> with that particular type of object.
> 
> I really never considered the case of code stripping the scheme and working on
> the rest of the name as if it is a standard file path.

Yes, I agree with your rephrasing. In this case there is no way to extract a
file descriptor/handle from a content:// reference. The best that could be done
would be to use OpenContentUriForRead to first copy the file to a temporary file
location, to perform the task, and then to delete the temp file. I'll try to
track down another reviewer that can make a definitive ruling on this.

[cmumford, 2014-09-30 20:19:02]

cmumford@chromium.org changed reviewers:
+ brettw@chromium.org

[cmumford, 2014-09-30 20:19:35]

On 2014/09/30 20:06:44, cmumford wrote:
> On 2014/09/26 19:24:57, rvargas wrote:
> > On 2014/09/26 16:55:54, cmumford wrote:
> > > On 2014/09/25 20:28:39, rvargas wrote:
> > > > I think it is reasonable to have base methods that understand special
path
> > > > encodings for a given platform. The expectation is that file related
> methods
> > > > should work with anything that provides a standard FS interface, and
given
> > > that
> > > > OpenContentURIForRead returns a standard base::File, it seems fine to
> update
> > > > CopyFile to deal with it.
> > > > 
> > > > As for the flags, we should have consistent behavior between a file
> created
> > > with
> > > > CopyFile and files created with other functions from base. I just
created
> > bug
> > > > 417830 for it.
> > > > 
> > > > In other words, this change looks fine to me in that at least reduces
one
> > > place
> > > > of deciding what to use; not sure if we want to land it before agreeing
> > about
> > > > flags on 417830.
> > > 
> > > Thanks rvargas@. I was pointed at this information from another developer:
> > > 
> > >
> >
>
http://developer.android.com/guide/topics/providers/content-provider-basics.h...
> > > 
> > > which basically confirms my suspicion that content:// scheme's don't
always
> > > refer to files, so it isn't possible to strip off the scheme and do a
> standard
> > > file copy. So I believe this is the correct fix to make, but I do still
want
> > to
> > > address the umask question. Given that I'm restricting the umask I want to
> at
> > > least have one of the security guys weigh in on 417830 before moving ahead
> > with
> > > this change.
> > 
> > Let me rephrase what I said before: if it is possible to go from [random
> object
> > type with some special name] to [file descriptor / handle that implements a
> > standard FS interface] then it's fine to handle the special case in base
code
> > that is meant to deal with files. If the returned object is close to a
> standard
> > fd, but with some quirks that prevent some functions to work properly, it
> makes
> > more sense to write code somewhere else that is specifically tailored to
deal
> > with that particular type of object.
> > 
> > I really never considered the case of code stripping the scheme and working
on
> > the rest of the name as if it is a standard file path.
> 
> Yes, I agree with your rephrasing. In this case there is no way to extract a
> file descriptor/handle from a content:// reference. The best that could be
done
> would be to use OpenContentUriForRead to first copy the file to a temporary
file
> location, to perform the task, and then to delete the temp file. I'll try to
> track down another reviewer that can make a definitive ruling on this.

+brettw for review.

[brettw, 2014-10-01 17:36:59]

lgtm

[jsbell, 2014-10-27 23:15:14]

ping on this - cmumford, are you waiting on anyone?

Would be nice to have the fix in for 40.

[cmumford, 2014-10-27 23:19:05]

On 2014/10/27 23:15:14, jsbell (TPAC Oct 27-28) wrote:
> ping on this - cmumford, are you waiting on anyone?
> 
> Would be nice to have the fix in for 40.

Was waiting for somebody to chime in on the security bug rvargas opened, but so
far no input on that. I'm going to push this through.

[cmumford, 2014-10-27 23:19:09]

The CQ bit was checked by cmumford@chromium.org

[commit-bot: I haz the power, 2014-10-27 23:24:15]

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

[commit-bot: I haz the power, 2014-10-28 01:56:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-28 01:56:07]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[cmumford, 2014-10-28 16:56:00]

On 2014/10/28 01:56:07, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_chromium_rel on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

The FileUtilTest.CopyFile test is failing because it is asserting the ability to
copy a file to a path with a ".." in it. File::Initialize() checks that the name
doesn't reference a parent (via ReferencesParent()) for security purposes.
MakeAbsoluteFilePath can't be used on the destination file path because that
function only works for files that exist.

So... one solution is to use the old file I/O except when the source path
contains the content scheme. However, this begs the question, "If File is
designed to fail for paths that reference their parent for security purposes
then shouldn't base::CopyFile also follow this rule"?

[brettw, 2014-10-29 17:13:55]

On 2014/10/28 16:56:00, cmumford wrote:
> On 2014/10/28 01:56:07, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   linux_chromium_rel on tryserver.chromium.linux
> >
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> 
> The FileUtilTest.CopyFile test is failing because it is asserting the ability
to
> copy a file to a path with a ".." in it. File::Initialize() checks that the
name
> doesn't reference a parent (via ReferencesParent()) for security purposes.
> MakeAbsoluteFilePath can't be used on the destination file path because that
> function only works for files that exist.
> 
> So... one solution is to use the old file I/O except when the source path
> contains the content scheme. However, this begs the question, "If File is
> designed to fail for paths that reference their parent for security purposes
> then shouldn't base::CopyFile also follow this rule"?

It seems so, yes.

[cmumford, 2014-11-11 16:01:58]

The CQ bit was checked by cmumford@chromium.org

[commit-bot: I haz the power, 2014-11-11 16:02:51]

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

[commit-bot: I haz the power, 2014-11-11 16:31:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-11 16:31:20]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[cmumford, 2014-11-11 23:08:27]

On 2014/11/11 16:31:20, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   ios_dbg_simulator on tryserver.chromium.mac
>
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

The tests were failing on some platforms because only the Linux POSIX
CopyFileUnsafe uses base::File and therefore fails for directory navigation
paths. The other platforms (Windows, OSX, iOS) will still allow copies including
"..". I decided to modify the test and therefore CopyFileUnsafe isn't guaranteed
to be able to copy a file path including "..". This function is only used by
base::CopyFile so I don't see this as a significant change.

Just wanted to pass it by one of you for one more look before pressing the CL
button.

[rvargas (doing something else), 2014-11-11 23:40:50]

https://codereview.chromium.org/603683005/diff/20001/base/files/file_util_uni...
File base/files/file_util_unittest.cc (right):

https://codereview.chromium.org/603683005/diff/20001/base/files/file_util_uni...
base/files/file_util_unittest.cc:1479:
ASSERT_FALSE(internal::CopyFileUnsafe(file_name_from, dest_file2));
Aha!. So the code was already there :)

What we need is a dedicated test for internal::CopyFileUnsafe that passes on all
platforms (if we keep the function), or better yet, remove the function as it
appears to have no users.

[cmumford, 2014-11-13 21:27:41]

I removed CopyFileUnsafe as rvargas suggested. PTAL.

[rvargas (doing something else), 2014-11-14 00:23:10]

PS4 lgtm

[cmumford, 2014-11-17 17:54:17]

The CQ bit was checked by cmumford@chromium.org

[commit-bot: I haz the power, 2014-11-17 17:55:23]

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

[commit-bot: I haz the power, 2014-11-17 18:58:33]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-11-17 18:59:06]

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