Add check for file system access to the sandbox.

The current test for sandboxing, trying to retrieve the USER_DATA_DIR
path, is not reliable. Instead, this adds a central check to the sandbox
to see if filesystem access is available (by trying to stat() /proc).

BUG=chromium:630420
Committed: https://crrev.com/33b4324fcc0caf430a5b1012a30176daf6bf4de7
Cr-Commit-Position: refs/heads/master@{#421292}

[Greg K, 2016-09-21 22:27:46]

kerrnel@chromium.org changed reviewers:
+ rickyz@chromium.org

[Greg K, 2016-09-21 22:27:47]

Ricky,

Please review this CL.

- Greg

[rickyz (no longer on Chrome), 2016-09-22 17:31:15]

lgtm with comment

https://codereview.chromium.org/2357393003/diff/1/chrome/common/chrome_conten...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/2357393003/diff/1/chrome/common/chrome_conten...
chrome/common/chrome_content_client.cc:303: // This function tests if
DIR_USER_DATA can be accessed, as a simple check to
Update the comment as well.

[Greg K, 2016-09-22 17:35:41]

The CQ bit was checked by kerrnel@chromium.org

[Greg K, 2016-09-22 17:35:42]

The patchset sent to the CQ was uploaded after l-g-t-m from rickyz@chromium.org
Link to the patchset: https://codereview.chromium.org/2357393003/#ps20001
(title: "Fix IsSandboxed() check when loading flash player.")

[Greg K, 2016-09-22 17:35:47]

The CQ bit was unchecked by kerrnel@chromium.org

[Greg K, 2016-09-22 17:36:08]

kerrnel@chromium.org changed reviewers:
+ thakis@chromium.org

[Greg K, 2016-09-22 17:36:09]

thakis@chromium.org: Please review changes in 

chrome_content_client.cc

Thanks,

Greg

[Nico, 2016-09-22 17:37:19]

in what way is it not reliable?

[Greg K, 2016-09-22 17:41:00]

On 2016/09/22 17:37:19, Nico wrote:
> in what way is it not reliable?

It was always reporting that DIR_USER_DATA was accessible. I believe it was
written under the assumption that if the path_service could not access files in
DIR_USER_DATA, it would return false when trying to return the path. This,
however, is definitely not true today (whether it was ever true I'm not sure).

Regards,

Greg

[Nico, 2016-09-22 18:00:20]

Since this possibly never worked and seems to be important enough to change it,
is it possible to test this?

[Greg K, 2016-09-22 18:06:13]

On 2016/09/22 18:00:20, Nico wrote:
> Since this possibly never worked and seems to be important enough to change
it,
> is it possible to test this?

I could presumably write a test that calls the function, makes sure it returns
false, enables the sandbox, and calls IsSandboxed() again. My only concern there
is that if the test manually switches on the sandbox, it doesn't fully test real
world conditions.

Ricky, what do you think?

Thanks,

Greg

[Greg K, 2016-09-23 17:22:13]

On 2016/09/22 18:06:13, Greg K wrote:
> On 2016/09/22 18:00:20, Nico wrote:
> > Since this possibly never worked and seems to be important enough to change
> it,
> > is it possible to test this?
> 
> I could presumably write a test that calls the function, makes sure it returns
> false, enables the sandbox, and calls IsSandboxed() again. My only concern
there
> is that if the test manually switches on the sandbox, it doesn't fully test
real
> world conditions.
> 
> Ricky, what do you think?
> 
> Thanks,
> 
> Greg

In order to test this properly, I created a central function to do it in the
sandbox code, and called that. 

Ricky, please take another look.

- Greg

[rickyz (no longer on Chrome), 2016-09-23 23:40:20]

https://codereview.chromium.org/2357393003/diff/40001/sandbox/linux/services/...
File sandbox/linux/services/credentials.cc (right):

https://codereview.chromium.org/2357393003/diff/40001/sandbox/linux/services/...
sandbox/linux/services/credentials.cc:318:
CHECK(!base::DirectoryExists(base::FilePath("/proc")));
Mind replacing this line with CHECK(!HasFileSystemAccess())

https://codereview.chromium.org/2357393003/diff/40001/sandbox/linux/services/...
File sandbox/linux/services/credentials_unittest.cc (right):

https://codereview.chromium.org/2357393003/diff/40001/sandbox/linux/services/...
sandbox/linux/services/credentials_unittest.cc:152:
CHECK(!base::DirectoryExists(base::FilePath("/proc")));
Would be fine with replacing this line rather than adding a separate test

[rickyz (no longer on Chrome), 2016-09-23 23:40:46]

(er oops, forgot to include lgtm - just a minor comment or two)

[Greg K, 2016-09-26 20:22:13]

Thanks. Nico, would you please review chrome/common?

Thanks,

Greg

https://codereview.chromium.org/2357393003/diff/40001/sandbox/linux/services/...
File sandbox/linux/services/credentials.cc (right):

https://codereview.chromium.org/2357393003/diff/40001/sandbox/linux/services/...
sandbox/linux/services/credentials.cc:318:
CHECK(!base::DirectoryExists(base::FilePath("/proc")));
On 2016/09/23 23:40:20, rickyz wrote:
> Mind replacing this line with CHECK(!HasFileSystemAccess())

Done.

https://codereview.chromium.org/2357393003/diff/40001/sandbox/linux/services/...
File sandbox/linux/services/credentials_unittest.cc (right):

https://codereview.chromium.org/2357393003/diff/40001/sandbox/linux/services/...
sandbox/linux/services/credentials_unittest.cc:152:
CHECK(!base::DirectoryExists(base::FilePath("/proc")));
On 2016/09/23 23:40:20, rickyz wrote:
> Would be fine with replacing this line rather than adding a separate test

Done.

[Nico, 2016-09-26 20:23:49]

lgtm with updated CL description. thanks!

[Greg K, 2016-09-26 20:25:32]

Description was changed from

==========
Fix IsSandboxed() check when loading flash player.

The current test for sandboxing, trying to retrieve the USER_DATA_DIR
path, is not reliable. Instead, see if the program can access
/proc/self/exe.

BUG=chromium:630420
==========

to

==========
The current test for sandboxing, trying to retrieve the USER_DATA_DIR
path, is not reliable. Instead, this adds a central check to the sandbox to see
if filesystem access is available (by trying to stat() /proc). 

BUG=chromium:630420
==========

[Greg K, 2016-09-26 20:25:51]

Description was changed from

==========
The current test for sandboxing, trying to retrieve the USER_DATA_DIR
path, is not reliable. Instead, this adds a central check to the sandbox to see
if filesystem access is available (by trying to stat() /proc). 

BUG=chromium:630420
==========

to

==========
The current test for sandboxing, trying to retrieve the USER_DATA_DIR
path, is not reliable. Instead, this adds a central check to the sandbox
to see if filesystem access is available (by trying to stat() /proc). 

BUG=chromium:630420
==========

[Greg K, 2016-09-26 21:03:59]

The CQ bit was checked by kerrnel@chromium.org

[Greg K, 2016-09-26 21:04:00]

The patchset sent to the CQ was uploaded after l-g-t-m from rickyz@chromium.org
Link to the patchset: https://codereview.chromium.org/2357393003/#ps60001
(title: "Fix IsSandboxed() check when loading flash player.")

[commit-bot: I haz the power, 2016-09-26 21:04:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 21:10:08]

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

[commit-bot: I haz the power, 2016-09-26 21:10:09]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Greg K, 2016-09-26 21:15:45]

Nico,

Is the change to chrome/common/BUILD.gn to make the dependency explicit OK?

Thanks,

Greg

On 2016/09/26 21:10:09, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
>   chromeos_amd64-generic_chromium_compile_only_ng on
> master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
>   chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
> (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
>   linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
> (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
>   linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
> (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
>   linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
>   linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
>   linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
>   linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nico, 2016-09-27 15:39:55]

https://codereview.chromium.org/2357393003/diff/80001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (right):

https://codereview.chromium.org/2357393003/diff/80001/chrome/common/BUILD.gn#...
chrome/common/BUILD.gn:212: "//sandbox/linux:sandbox_services",
You're not including a generated header in a chrome header, so this doesn't need
to be a public_deps, a regular deps is enough. Also, the dep should probably
only be added if is_linux (?)

[Greg K, 2016-09-27 17:18:28]

https://codereview.chromium.org/2357393003/diff/80001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (right):

https://codereview.chromium.org/2357393003/diff/80001/chrome/common/BUILD.gn#...
chrome/common/BUILD.gn:212: "//sandbox/linux:sandbox_services",
On 2016/09/27 15:39:55, Nico (mostly away until Oct 3) wrote:
> You're not including a generated header in a chrome header, so this doesn't
need
> to be a public_deps, a regular deps is enough. Also, the dep should probably
> only be added if is_linux (?)

Done.

[Nico, 2016-09-27 18:05:27]

lgtm

[Greg K, 2016-09-27 18:06:56]

The CQ bit was checked by kerrnel@chromium.org

[Greg K, 2016-09-27 18:06:56]

The patchset sent to the CQ was uploaded after l-g-t-m from rickyz@chromium.org
Link to the patchset: https://codereview.chromium.org/2357393003/#ps100001
(title: "Fix IsSandboxed() check when loading flash player.")

[commit-bot: I haz the power, 2016-09-27 18:07:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 19:20:26]

Description was changed from

==========
The current test for sandboxing, trying to retrieve the USER_DATA_DIR
path, is not reliable. Instead, this adds a central check to the sandbox
to see if filesystem access is available (by trying to stat() /proc). 

BUG=chromium:630420
==========

to

==========
The current test for sandboxing, trying to retrieve the USER_DATA_DIR
path, is not reliable. Instead, this adds a central check to the sandbox
to see if filesystem access is available (by trying to stat() /proc). 

BUG=chromium:630420
==========

[commit-bot: I haz the power, 2016-09-27 19:20:27]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-27 19:23:32]

Description was changed from

==========
The current test for sandboxing, trying to retrieve the USER_DATA_DIR
path, is not reliable. Instead, this adds a central check to the sandbox
to see if filesystem access is available (by trying to stat() /proc). 

BUG=chromium:630420
==========

to

==========
The current test for sandboxing, trying to retrieve the USER_DATA_DIR
path, is not reliable. Instead, this adds a central check to the sandbox
to see if filesystem access is available (by trying to stat() /proc).

BUG=chromium:630420

Committed: https://crrev.com/33b4324fcc0caf430a5b1012a30176daf6bf4de7
Cr-Commit-Position: refs/heads/master@{#421292}
==========

[commit-bot: I haz the power, 2016-09-27 19:23:33]

Patchset 6 (id:??) landed as
https://crrev.com/33b4324fcc0caf430a5b1012a30176daf6bf4de7
Cr-Commit-Position: refs/heads/master@{#421292}