Allow /dev/urandom from ppapi sandbox.

Allow /dev/urandom from ppapi sandbox.

Bits of Adobe Flash wish to read urandom directly rather than re-using
our cached file descriptor.

BUG=none
TEST=no user-visible changes at this time.


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

[Scott Hess - ex-Googler, 2012-07-12 21:00:07]

Intentionally not including the necessary content/ OWNER, yet, in case either
Justin or Jeremy recoils in horror.

[jeremy, 2012-07-12 21:08:11]

Skimming this it looks sane, but have you checked that the test hits the
ppapi sandbox case?

I'll take a closer look on Monday when I'm back in the office.

On Fri, Jul 13, 2012 at 12:00 AM, <shess@chromium.org> wrote:

> Reviewers: Justin Schuh, jeremy,
>
> Message:
> Intentionally not including the necessary content/ OWNER, yet, in case
> either
> Justin or Jeremy recoils in horror.
>
> Description:
> Allow /dev/urandom from ppapi sandbox.
>
> Bits of Adobe Flash wish to read urandom directly rather than re-using
> our cached file descriptor.
>
> BUG=none
> TEST=no user-visible changes at this time.
>
>
> Please review this at
https://chromiumcodereview.**appspot.com/10695185/<https://chromiumcodereview...
>
> SVN Base:
http://git.chromium.org/**chromium/src.git@master<http://git.chromium.org/chr...
>
> Affected files:
>   M
content/common/sandbox_mac_**system_access_unittest.mm<http://sandbox_mac_system_access_unittest.mm>
>   M content/ppapi_plugin/ppapi.sb
>
>
> Index:
content/common/sandbox_mac_**system_access_unittest.mm<http://sandbox_mac_system_access_unittest.mm>
> diff --git
a/content/common/sandbox_mac_**system_access_unittest.mm<http://sandbox_mac_system_access_unittest.mm>b/content/common/
>
sandbox_mac_**system_access_unittest.mm<http://sandbox_mac_system_access_unittest.mm>
> index d9086ebd6da06ed32b211e630a4965**eaebf4128b..**
> 91b5b4ce000bfd16a9a05711398cad**2a9e71eba8 100644
> ---
a/content/common/sandbox_mac_**system_access_unittest.mm<http://sandbox_mac_system_access_unittest.mm>
> +++
b/content/common/sandbox_mac_**system_access_unittest.mm<http://sandbox_mac_system_access_unittest.mm>
> @@ -96,4 +96,39 @@ TEST_F(MacSandboxTest, FileAccess) {
>    EXPECT_TRUE(**RunTestInAllSandboxTypes("**
> MacSandboxedFileAccessTestCase**", NULL));
>  }
>
> +//--------------------- /dev/urandom Sandboxing ----------------------
> +// /dev/urandom is available to ppapi sandbox only.
> +class MacSandboxedUrandomTestCase : public sandboxtest::**MacSandboxTestCase
> {
> + public:
> +  virtual bool SandboxedTest();
> +};
> +
> +REGISTER_SANDBOX_TEST_CASE(**MacSandboxedUrandomTestCase);
> +
> +bool MacSandboxedUrandomTestCase::**SandboxedTest() {
> +  int fdes = open("/dev/urandom", O_RDONLY);
> +  file_util::ScopedFD file_closer(&fdes);
> +
> +  // Open succeeds under ppapi sandbox, else it is not permitted.
> +  if (test_data_ == "ppapi") {
> +    return fdes != -1;
> +  } else {
> +    return fdes == -1 && errno == EPERM;
> +  }
> +}
> +
> +TEST_F(MacSandboxTest, UrandomAccess) {
> +  // Similar to RunTestInAllSandboxTypes(), except changing
> +  // |test_data| for the ppapi case.  Passing "" in the non-ppapi case
> +  // to overwrite the test data (NULL means not to change it).
> +  for (content::SandboxType i = content::SANDBOX_TYPE_FIRST_**TYPE;
> +       i < content::SANDBOX_TYPE_AFTER_**LAST_TYPE; ++i) {
> +    if (i == content::SANDBOX_TYPE_PPAPI) {
> +      EXPECT_TRUE(RunTestInSandbox(**i, "MacSandboxedUrandomTestCase",
> "ppapi"));
> +    } else {
> +      EXPECT_TRUE(RunTestInSandbox(**i, "MacSandboxedUrandomTestCase",
> ""));
> +    }
> +  }
> +}
> +
>  }  // namespace
> Index: content/ppapi_plugin/ppapi.sb
> diff --git a/content/ppapi_plugin/ppapi.**sb
<http://ppapi.sb>b/content/ppapi_plugin/
> ppapi.**sb <http://ppapi.sb>
> index 6fa3ebf82580bb3a6d177a024dda2a**f1211f5006..**
> 3067565d5bcbf5581e1901894432c4**813de0c601 100644
> --- a/content/ppapi_plugin/ppapi.**sb <http://ppapi.sb>
> +++ b/content/ppapi_plugin/ppapi.**sb <http://ppapi.sb>
> @@ -18,3 +18,7 @@
>
>  ; http://crbug.com/11269
>  ;10.6_OR_ABOVE (allow file-read* (subpath
"@USER_HOMEDIR_AS_LITERAL@/**Library/Fonts"))
>  ; 10.6
> +
> +; Bits of Adobe Flash wish to open /dev/urandom directly rather than
> +; using our cached file descriptor.
> +(allow file-read* (literal "/dev/urandom"))
>
>
>

[Scott Hess - ex-Googler, 2012-07-12 21:25:05]

On 2012/07/12 21:08:11, jeremy wrote:
> Skimming this it looks sane, but have you checked that the test hits the
> ppapi sandbox case?

I wasn't certain what you meant, since the test had to inline
RunTestInAllSandboxTypes() specifically so it could be different in the ppapi
case :-).

But then it occurred to me to test more thoroughly anyhow.  So I commented out
the ppapi.sb change, and that test failed.  Then I copied the ppapi.sb line to
common.sb, and all the other ones failed.  So I think the test tests failures in
both directions adequately.

Thanks,
scott

[jschuh, 2012-07-12 22:59:10]

lgtm

[jeremy, 2012-07-16 11:36:40]

https://chromiumcodereview.appspot.com/10695185/diff/4/content/ppapi_plugin/p...
File content/ppapi_plugin/ppapi.sb (right):

https://chromiumcodereview.appspot.com/10695185/diff/4/content/ppapi_plugin/p...
content/ppapi_plugin/ppapi.sb:24: (allow file-read* (literal "/dev/urandom"))
Any chance that just file-read-data is enough here?

[jeremy, 2012-07-16 11:37:01]

also, lgtm :)

[Scott Hess - ex-Googler, 2012-07-16 16:36:42]

I'll wait a few hours to push the button, in case anyone wants to complain about
my change to read-only.

https://chromiumcodereview.appspot.com/10695185/diff/4/content/ppapi_plugin/p...
File content/ppapi_plugin/ppapi.sb (right):

https://chromiumcodereview.appspot.com/10695185/diff/4/content/ppapi_plugin/p...
content/ppapi_plugin/ppapi.sb:24: (allow file-read* (literal "/dev/urandom"))
On 2012/07/16 11:36:40, jeremy wrote:
> Any chance that just file-read-data is enough here?

Done.  Writing to the file can be used to add entropy, and AFAICT it would be
safe to allow, but I suspect that any sandboxed plugin which was doing this
would probably be doing the wrong thing.

[commit-bot: I haz the power, 2012-07-16 19:21:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/10695185/9001

[commit-bot: I haz the power, 2012-07-16 19:21:18]

Presubmit check for 10695185-9001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/ppapi_plugin

[Scott Hess - ex-Googler, 2012-07-16 19:26:51]

Brett for the OWNERS rubber stamp.

[brettw, 2012-07-16 19:37:45]

lgtm

[commit-bot: I haz the power, 2012-07-16 19:50:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/10695185/9001

[commit-bot: I haz the power, 2012-07-16 21:02:47]

Change committed as 146879