Don't allow '\0' characters in FilePath.

Don't allow '\0' characters in FilePath.

BUG=169777
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179141

[aedla, 2012-12-20 11:28:56]

Tom, what do you think of this change to FilePath?

[Tom Sepez, 2012-12-20 18:22:10]

On 2012/12/20 11:28:56, aedla wrote:
> Tom, what do you think of this change to FilePath?

I like the idea, but what if there's more than one \0 in the path?  You only
seem to be zapping the first one.

I expect that you could choose to fail hard here and be OK, since I expect only
bad guys would be affected.

[aedla, 2012-12-20 18:41:57]

On 2012/12/20 18:22:10, Tom Sepez wrote:
> On 2012/12/20 11:28:56, aedla wrote:
> > Tom, what do you think of this change to FilePath?
> 
> I like the idea, but what if there's more than one \0 in the path?  You only
> seem to be zapping the first one.
> 
> I expect that you could choose to fail hard here and be OK, since I expect
only
> bad guys would be affected.

I figured it would make sense to zap the whole string from the first \0 onward
because that's what happens with a terminating \0.

The path_.erase(zero_pos) deletes everything starting from zero_pos, doesn't it?

Do you think it would be better to do a hard fail though? I don't really like
the idea of having an empty path for error conditions. Someone might append to
them, resulting in cwd relative paths.

[Tom Sepez, 2012-12-20 18:46:10]

path_.erase() is really std::string::erase()?  If so, I thought it was only the
character at position when invoked in that form. You could write a test with
multiple 0's.

I also added brett to get some additional perspective.

[aedla, 2012-12-20 19:03:56]

On 2012/12/20 18:46:10, Tom Sepez wrote:
> path_.erase() is really std::string::erase()?  If so, I thought it was only
the
> character at position when invoked in that form. You could write a test with
> multiple 0's.
> 
> I also added brett to get some additional perspective.

path_ is indeed std::string (or std::wstring on windows). There are
::erase(iterator position) which deletes one character and ::erase(size_t pos =
0, size_t n = npos) which deletes n characters from pos. ::erase(zero_pos)
should use the second variant because zero_pos is size_t. I also wrote a quick
test to confirm this. But I'm quite new to C++ so if I'm missing something,
please let me know.

[Tom Sepez, 2012-12-20 19:05:34]

ah, nevermind, I assumed you were using the iterator form.

[Tom Sepez, 2012-12-20 21:14:34]

https://codereview.chromium.org/11642041/diff/1/base/file_path.cc
File base/file_path.cc (right):

https://codereview.chromium.org/11642041/diff/1/base/file_path.cc#newcode44
base/file_path.cc:44: 
Should be in namespace { below.

https://codereview.chromium.org/11642041/diff/1/base/file_path.cc#newcode187
base/file_path.cc:187: // Don't allow '\0' characters in path.
nit: not sure we need this comment. Maybe just add a note to file_path.h that
paths can't contain NULs as a precaution against OS usage.

https://codereview.chromium.org/11642041/diff/1/base/file_path.cc#newcode190
base/file_path.cc:190: path_.erase(zero_pos);
nit: maybe write
   path_.erase(zero_pos, StringType::npos)
to make it clearer?

https://codereview.chromium.org/11642041/diff/1/base/file_path.cc#newcode463
base/file_path.cc:463: StringType appended(component);
Would be nice to avoid the copy if the NULs not found.

https://codereview.chromium.org/11642041/diff/1/base/file_path.cc#newcode617
base/file_path.cc:617: return false;
Need the check here, otherwise IPCs bypass your constructors.

[Tom Sepez, 2012-12-20 21:19:39]

https://codereview.chromium.org/11642041/diff/1/base/file_path.cc
File base/file_path.cc (right):

https://codereview.chromium.org/11642041/diff/1/base/file_path.cc#newcode617
base/file_path.cc:617: return false;
On 2012/12/20 21:14:34, Tom Sepez wrote:
> Need the check here, otherwise IPCs bypass your constructors.

Or Not.  Looks like IPC reads the underlying string type and invokes your
modified constructor.  

This would be called to serialize other things then, check may not hurt.

[aedla, 2012-12-21 10:15:50]

On 2012/12/20 21:19:39, Tom Sepez wrote:
> https://codereview.chromium.org/11642041/diff/1/base/file_path.cc
> File base/file_path.cc (right):
> 
> https://codereview.chromium.org/11642041/diff/1/base/file_path.cc#newcode617
> base/file_path.cc:617: return false;
> On 2012/12/20 21:14:34, Tom Sepez wrote:
> > Need the check here, otherwise IPCs bypass your constructors.
> 
> Or Not.  Looks like IPC reads the underlying string type and invokes your
> modified constructor.  
> 
> This would be called to serialize other things then, check may not hurt.

PTAL

[Tom Sepez, 2012-12-21 18:25:24]

I think this is OK, but please get brett to sign off on it.

https://codereview.chromium.org/11642041/diff/9001/base/file_path.cc
File base/file_path.cc (right):

https://codereview.chromium.org/11642041/diff/9001/base/file_path.cc#newcode463
base/file_path.cc:463: StringType without_zeros;
nit: maybe s/zeros/nuls/ in all these names

https://codereview.chromium.org/11642041/diff/9001/base/file_path.h
File base/file_path.h (right):

https://codereview.chromium.org/11642041/diff/9001/base/file_path.h#newcode56
base/file_path.h:56: // Paths can't contain NULs as a precaution agaist OS
usage.
nit: sp. against.  Maybe better comment is .. as a precaution against premature
truncation.

[Chris Evans, 2013-01-02 20:39:57]

An alternate approach is to validate across the IPC layer.
I'm not sure which approach is better.

Index: ipc_message_utils.cc
===================================================================
--- ipc_message_utils.cc	(revision 174204)
+++ ipc_message_utils.cc	(working copy)
@@ -508,6 +508,11 @@
   FilePath::StringType value;
   if (!ParamTraits<FilePath::StringType>::Read(m, iter, &value))
     return false;
+  // Reject embedded NULLs as they can cause security checks to go awry.
+  if (value.find(static_cast<FilePath::CharType>(0)) !=
+      FilePath::StringType::npos) {
+    return false;
+  }
   *r = FilePath(value);
   return true;
 }

[brettw, 2013-01-02 21:58:16]

I think it's reasonable for the FilePath to do this since embedded nulls are a
guarantee of bad things happening later.

I think this code is fine. Can we get some unit tests for the different places
that changed?

[cevans, 2013-01-03 05:58:10]

https://codereview.chromium.org/11642041/diff/13002/base/file_path.cc
File base/file_path.cc (right):

https://codereview.chromium.org/11642041/diff/13002/base/file_path.cc#newcode189
base/file_path.cc:189: path_.erase(nul_pos, StringType::npos);
It should be very easy to add an API-level unit test for this new behaviour to
file_path_unittest.cc

I recommend testing this as well as append.

https://codereview.chromium.org/11642041/diff/13002/base/file_path.cc#newcode624
base/file_path.cc:624: StringType::size_type nul_pos =
path_.find(kStringTerminator);
Eep! The logic here seems duplicated once you sync to my change of
http://src.chromium.org/viewvc/chrome?view=rev&revision=174935 (sorry!)

IMHO, ParamTraits<FilePath>::Read should probably just take the significant
simplification of doing
return r->ReadFromPickle(iter)
... assuming it works. I added a unit test to ipc_tests in my CL so you'll know
if it works or not.

https://codereview.chromium.org/11642041/diff/13002/base/file_path.cc#newcode626
base/file_path.cc:626: path_.erase(nul_pos, StringType::npos);
I think we should just fail the unpickle here, i.e. "return false"

[aedla, 2013-01-07 08:38:36]

This should be good to commit?

[Chris Evans, 2013-01-08 09:41:00]

https://codereview.chromium.org/11642041/diff/19001/base/file_path.cc
File base/file_path.cc (right):

https://codereview.chromium.org/11642041/diff/19001/base/file_path.cc#newcode189
base/file_path.cc:189: path_.erase(nul_pos, StringType::npos);
I'm ok either way, but just a thought to throw out there: what about we
CHECK(path_.find(kS..) == StringType::npos) instead? Both here and in append.
It's simpler code, and also a simpler contract. I worry that calling code might
have precomputed a length expectation and then get surprised when we truncated
the array that was passed in.

Maybe the benefits outweight the drawbacks, maybe not. What do you and Tom
think?

https://codereview.chromium.org/11642041/diff/19001/base/file_path_unittest.cc
File base/file_path_unittest.cc (right):

https://codereview.chromium.org/11642041/diff/19001/base/file_path_unittest.c...
base/file_path_unittest.cc:1124: EXPECT_EQ(path.value(), FPL("a"));
I'm not 100% sure how std::string equality works. If it's similar to a strcmp(),
you might not be testing what you were hoping to test.

To be safe, I think you'll want to check that path.value().length() == 1. You
might also want to throw in a check that FilePath::StringType
s(FPS("a\0b")).length() == 3, to prove you macro is working :)

https://codereview.chromium.org/11642041/diff/19001/base/file_path_unittest.c...
base/file_path_unittest.cc:1134: EXPECT_EQ(path.value(), "a/b");
Again, maybe check length() to be sure.

[aedla, 2013-01-08 15:24:07]

PTAL

On 2013/01/08 09:41:00, Chris Evans wrote:
> I'm ok either way, but just a thought to throw out there: what about we
> CHECK(path_.find(kS..) == StringType::npos) instead? Both here and in append.
> It's simpler code, and also a simpler contract. I worry that calling code
might
> have precomputed a length expectation and then get surprised when we truncated
> the array that was passed in.
> 
> Maybe the benefits outweight the drawbacks, maybe not. What do you and Tom
> think?

Yeah, the length change is not very nice. CHECK() is not very nice either,
because if the renderer should happen to pass a string over IPC to be appended,
then the browser crashes. So I don't know, it's worth considering though.

[Tom Sepez, 2013-01-08 19:19:11]

I'm on the erase the NUL and continue side of the fence but I could be convinced
otherwise.

[Chris Evans, 2013-01-23 10:34:06]

https://codereview.chromium.org/11642041/diff/26001/base/file_path.cc
File base/file_path.cc (right):

https://codereview.chromium.org/11642041/diff/26001/base/file_path.cc#newcode625
base/file_path.cc:625: return false;
Cleanup: the logic in here is now the same as ParamTraits<FilePath>::Read in
ipc_message_utils.cc

Is it possible to just do

return r->ReadFromPickle(iter) in that function?

[Chris Evans, 2013-01-25 21:48:12]

On 2013/01/23 10:34:06, Chris Evans wrote:
> https://codereview.chromium.org/11642041/diff/26001/base/file_path.cc
> File base/file_path.cc (right):
> 
>
https://codereview.chromium.org/11642041/diff/26001/base/file_path.cc#newcode625
> base/file_path.cc:625: return false;
> Cleanup: the logic in here is now the same as ParamTraits<FilePath>::Read in
> ipc_message_utils.cc
> 
> Is it possible to just do
> 
> return r->ReadFromPickle(iter) in that function?

LGTM

You'll need OWNERS for base/ and ipc/

[Chris Evans, 2013-01-25 21:49:53]

On 2013/01/25 21:48:12, Chris Evans wrote:
> On 2013/01/23 10:34:06, Chris Evans wrote:
> > https://codereview.chromium.org/11642041/diff/26001/base/file_path.cc
> > File base/file_path.cc (right):
> > 
> >
>
https://codereview.chromium.org/11642041/diff/26001/base/file_path.cc#newcode625
> > base/file_path.cc:625: return false;
> > Cleanup: the logic in here is now the same as ParamTraits<FilePath>::Read in
> > ipc_message_utils.cc
> > 
> > Is it possible to just do
> > 
> > return r->ReadFromPickle(iter) in that function?
> 
> LGTM
> 
> You'll need OWNERS for base/ and ipc/

@cpu: for ipc/* OWNERS please.
@brettw: for base/* OWNERS please.

[Tom Sepez, 2013-01-25 21:51:22]

I can give you an LGTM for ipc/

[brettw, 2013-01-25 22:06:03]

lgtm

https://codereview.chromium.org/11642041/diff/33001/ipc/ipc_message_utils_uni...
File ipc/ipc_message_utils_unittest.cc (right):

https://codereview.chromium.org/11642041/diff/33001/ipc/ipc_message_utils_uni...
ipc/ipc_message_utils_unittest.cc:67: FilePath okPath;
Style nit: no camelCase, use lowercase_with_underscores. Can you fix the other
stuff in this test?

[commit-bot: I haz the power, 2013-01-28 09:36:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aedla@chromium.org/11642041/42001

[commit-bot: I haz the power, 2013-01-28 10:12:09]

Retried try job too often on linux_aura for step(s) content_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[aedla, 2013-01-28 11:40:15]

https://codereview.chromium.org/11642041/diff/48003/content/browser/download/...
File content/browser/download/save_package_unittest.cc (left):

https://codereview.chromium.org/11642041/diff/48003/content/browser/download/...
content/browser/download/save_package_unittest.cc:112:
long_file_name.resize(kMaxFilePathLength + long_file_name.length());
Looks like reserve was intended here. Because otherwise a long string of NULs is
created and the while loop won't be executed. When appended to FilePath, all
NULs are stripped and the long path becomes a short path, causing
TestUnSuccessfullyGenerateSavePackageFilename to fail.

[commit-bot: I haz the power, 2013-01-28 11:41:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aedla@chromium.org/11642041/48003

[commit-bot: I haz the power, 2013-01-28 13:47:57]

Change committed as 179141