Add assertions that the empty Platform::cryptographicallyRandomValues() overrides are not being use…

Add assertions that the empty Platform::cryptographicallyRandomValues() overrides are not being used.

These implementations are not safe and look scary if not accompanied by an assertion. Also one of the comments was incorrect.

BUG=552749
Committed: https://crrev.com/0d151e09e13a704e9738ea913d117df7282e6c7d
Cr-Commit-Position: refs/heads/master@{#359229}

[eroman, 2015-11-09 22:50:54]

eroman@chromium.org changed reviewers:
+ jww@chromium.org

[eroman, 2015-11-09 22:50:54]

(This is a cleanup CL as part of my work on crbug.com/552749)

[eroman, 2015-11-10 00:24:06]

+cc davidben as FYI

[jww, 2015-11-10 18:43:17]

lgtm % nits

https://codereview.chromium.org/1419293005/diff/1/components/test_runner/test...
File components/test_runner/test_common.cc (right):

https://codereview.chromium.org/1419293005/diff/1/components/test_runner/test...
components/test_runner/test_common.cc:35: NOTREACHED();
Why isn't this an ASSERT_NOT_REACHED()? Wouldn't we be really sad if this was
reached in a Release build, to the point that all security guarantees are lost?

https://codereview.chromium.org/1419293005/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/ImageDecodeBench.cpp (right):

https://codereview.chromium.org/1419293005/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/ImageDecodeBench.cpp:370: void
cryptographicallyRandomValues(unsigned char*, size_t) override
There was a comment in one of the other classes explaining that this needs to be
overridden to be useful/secure; maybe copy that comment here, too? Or at least
have a comment explaining the security concern.

[eroman, 2015-11-10 19:26:14]

https://codereview.chromium.org/1419293005/diff/1/components/test_runner/test...
File components/test_runner/test_common.cc (right):

https://codereview.chromium.org/1419293005/diff/1/components/test_runner/test...
components/test_runner/test_common.cc:35: NOTREACHED();
On 2015/11/10 18:43:16, jww wrote:
> Why isn't this an ASSERT_NOT_REACHED()? Wouldn't we be really sad if this was
> reached in a Release build, to the point that all security guarantees are
lost?

 (1) ASSERT_NOT_REACHED() is compiled out of Release builds, so if we wanted to
test in release I believe it would need to be "RELEASE_ASSERT_NOT_REACHED". I am
happy to change the existing ones to RELEASE_ASSERT_NOT_REACHED() if you think
that is good, I was just matching the existing pattern.

 (2) In this case since it is Chromium code, I wasn't sure if it was legit to
include files from wtf/, since the rest of this file is just using
WebKit/public. For that reason I used the Chromium equivalent "NOTREACHED()".

https://codereview.chromium.org/1419293005/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/ImageDecodeBench.cpp (right):

https://codereview.chromium.org/1419293005/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/ImageDecodeBench.cpp:370: void
cryptographicallyRandomValues(unsigned char*, size_t) override
On 2015/11/10 18:43:16, jww wrote:
> There was a comment in one of the other classes explaining that this needs to
be
> overridden to be useful/secure; maybe copy that comment here, too? Or at least
> have a comment explaining the security concern.

The existing comment here was wrong here which is why I deleted it (there is no
"default" that gets used if this function does nothing).

I could add a comment explaining that this needs to be implemented if it is
used?

[jww, 2015-11-10 19:32:26]

https://codereview.chromium.org/1419293005/diff/1/components/test_runner/test...
File components/test_runner/test_common.cc (right):

https://codereview.chromium.org/1419293005/diff/1/components/test_runner/test...
components/test_runner/test_common.cc:35: NOTREACHED();
On 2015/11/10 19:26:14, eroman wrote:
> On 2015/11/10 18:43:16, jww wrote:
> > Why isn't this an ASSERT_NOT_REACHED()? Wouldn't we be really sad if this
was
> > reached in a Release build, to the point that all security guarantees are
> lost?
> 
>  (1) ASSERT_NOT_REACHED() is compiled out of Release builds, so if we wanted
to
> test in release I believe it would need to be "RELEASE_ASSERT_NOT_REACHED". I
am
> happy to change the existing ones to RELEASE_ASSERT_NOT_REACHED() if you think
> that is good, I was just matching the existing pattern.
> 
>  (2) In this case since it is Chromium code, I wasn't sure if it was legit to
> include files from wtf/, since the rest of this file is just using
> WebKit/public. For that reason I used the Chromium equivalent "NOTREACHED()".

Ah, got it! Good call all around.

(1) Yes, I think all of those call sites should probably be
RELEASE_ASSERT_NOT_REACHED(). It seems pretty critical to me that these should
not be reached in a Release build.

(2) Great call. You're right that it shouldn't use wtf/. I'm not sure if there's
a Release equivalent of NOTREACHED, so maybe just CHECK(false)?

[eroman, 2015-11-10 19:45:56]

https://codereview.chromium.org/1419293005/diff/1/components/test_runner/test...
File components/test_runner/test_common.cc (right):

https://codereview.chromium.org/1419293005/diff/1/components/test_runner/test...
components/test_runner/test_common.cc:35: NOTREACHED();
On 2015/11/10 19:32:26, jww wrote:
> On 2015/11/10 19:26:14, eroman wrote:
> > On 2015/11/10 18:43:16, jww wrote:
> > > Why isn't this an ASSERT_NOT_REACHED()? Wouldn't we be really sad if this
> was
> > > reached in a Release build, to the point that all security guarantees are
> > lost?
> > 
> >  (1) ASSERT_NOT_REACHED() is compiled out of Release builds, so if we wanted
> to
> > test in release I believe it would need to be "RELEASE_ASSERT_NOT_REACHED".
I
> am
> > happy to change the existing ones to RELEASE_ASSERT_NOT_REACHED() if you
think
> > that is good, I was just matching the existing pattern.
> > 
> >  (2) In this case since it is Chromium code, I wasn't sure if it was legit
to
> > include files from wtf/, since the rest of this file is just using
> > WebKit/public. For that reason I used the Chromium equivalent
"NOTREACHED()".
> 
> Ah, got it! Good call all around.
> 
> (1) Yes, I think all of those call sites should probably be
> RELEASE_ASSERT_NOT_REACHED(). It seems pretty critical to me that these should
> not be reached in a Release build.
> 
> (2) Great call. You're right that it shouldn't use wtf/. I'm not sure if
there's
> a Release equivalent of NOTREACHED, so maybe just CHECK(false)?

Done:

 * I changed to RELEASE_ASSERT_NOT_REACHED() everywhere. I agree this is better
and will guard against any future copy-pastes.

 * In the case of the Chromium code, rather than adding a CHECK(false) I opted
to instead just implement with a call to base::RandBytes(). (Note that
base::RandBytes() is in fact a cryptographically secure implementation despite
the comments in header. Following up on this ambiguity in
https://code.google.com/p/chromium/issues/detail?id=553857).

[jww, 2015-11-10 20:07:52]

Still lgtm. Thanks!

[eroman, 2015-11-10 20:38:36]

eroman@chromium.org changed reviewers:
+ jrummell@chromium.org, mkwst@chromium.org

[eroman, 2015-11-10 20:38:37]

+mkwst for OWNERS approval (on all files except
media/blink/run_all_unittests.cc).

+jrummell for OWNERS approval on media/blink/run_all_unittests.cc

Thanks!

[Mike West, 2015-11-11 06:12:43]

LGTM.

[jrummell, 2015-11-11 23:42:37]

media LGTM

[eroman, 2015-11-12 00:08:25]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2015-11-12 00:11:08]

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

[commit-bot: I haz the power, 2015-11-12 03:18:16]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-11-12 20:03:47]

Patchset 2 (id:??) landed as
https://crrev.com/0d151e09e13a704e9738ea913d117df7282e6c7d
Cr-Commit-Position: refs/heads/master@{#359229}