Cleanup: Consolidate initialization of Android's SecureRandom class.

Cleanup: Consolidate initialization of Android's SecureRandom class.

This replaces some copy-pasted snippets of code with a centralized
utility for securely initializing a SecureRandom instance (needed for
Android versions <= 4.3).

BUG=402732
Committed: https://crrev.com/df0de36454a6c58757517238c977334b1b421bbb
Cr-Commit-Position: refs/heads/master@{#299788}

[Lambros, 2014-10-03 23:20:40]

lambroslambrou@chromium.org changed reviewers:
+ palmer@chromium.org, yfriedman@chromium.org

[Lambros, 2014-10-03 23:20:41]

palmer: please review changes
yfriedman: OWNERS approval

This also addresses some late comments from @palmer in
https://codereview.chromium.org/463393002

[palmer, 2014-10-06 19:23:46]

Much cleaner, thank you! LGTM, with optional philosophizing.

https://codereview.chromium.org/624183002/diff/1/base/android/java/src/org/ch...
File base/android/java/src/org/chromium/base/SecureRandomInitializer.java
(right):

https://codereview.chromium.org/624183002/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/SecureRandomInitializer.java:19: private
static byte[] sSeedBytes = new byte[NUM_RANDOM_BYTES];
I suspect a method-scoped heap array has a greater chance of getting freed,
re-used, and overwritten than a static field.

(Attempts to clear crypto-relevant memory are often cited as "best practice" to
defend against high-capability forensic attackers. I don't personally believe
much that such attacks can be defeated, and even less so in memory-managed
languages. But, if you want to give a nod to such things, leaving the seed bytes
array as a stack-local might be nice. It costs 1 new, of course. So.)

[Lambros, 2014-10-08 17:19:13]

Friendly ping, need OWNERS approval.

[Yaron, 2014-10-08 18:08:00]

https://codereview.chromium.org/624183002/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/crypto/CipherFactory.java
(left):

https://codereview.chromium.org/624183002/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/crypto/CipherFactory.java:155:
seed = mRandomNumberProvider.getBytes(NUM_BYTES);
Looks like you should update CipherFactoryTest. It still thinks the seed is
coming from the random number provider

[Lambros, 2014-10-14 17:00:21]

On 2014/10/08 18:08:00, Yaron wrote:
>
https://codereview.chromium.org/624183002/diff/1/content/public/android/java/...
> File
>
content/public/android/java/src/org/chromium/content/browser/crypto/CipherFactory.java
> (left):
> 
>
https://codereview.chromium.org/624183002/diff/1/content/public/android/java/...
>
content/public/android/java/src/org/chromium/content/browser/crypto/CipherFactory.java:155:
> seed = mRandomNumberProvider.getBytes(NUM_BYTES);
> Looks like you should update CipherFactoryTest. It still thinks the seed is
> coming from the random number provider

I ran the CipherFactoryTests with this patch, and they still pass:
$ build/android/test_runner.py instrumentation --test-apk=ContentShellTest -vvv
-f '*CipherFactoryTest*'
...
C   18.768s Main  [==========] 7 tests ran.
C   18.769s Main  [  PASSED  ] 7 tests.

[Yaron, 2014-10-14 19:51:24]

On 2014/10/14 17:00:21, Lambros wrote:
> On 2014/10/08 18:08:00, Yaron wrote:
> >
>
https://codereview.chromium.org/624183002/diff/1/content/public/android/java/...
> > File
> >
>
content/public/android/java/src/org/chromium/content/browser/crypto/CipherFactory.java
> > (left):
> > 
> >
>
https://codereview.chromium.org/624183002/diff/1/content/public/android/java/...
> >
>
content/public/android/java/src/org/chromium/content/browser/crypto/CipherFactory.java:155:
> > seed = mRandomNumberProvider.getBytes(NUM_BYTES);
> > Looks like you should update CipherFactoryTest. It still thinks the seed is
> > coming from the random number provider
> 
> I ran the CipherFactoryTests with this patch, and they still pass:
> $ build/android/test_runner.py instrumentation --test-apk=ContentShellTest
-vvv
> -f '*CipherFactoryTest*'
> ...
> C   18.768s Main  [==========] 7 tests ran.
> C   18.769s Main  [  PASSED  ] 7 tests.


Hmm. Ok it passes but it seems to be mocking things unnecessarily (i.e.
inserting two randoms) or am I misreading it?

[Lambros, 2014-10-14 20:09:18]

lambroslambrou@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[Lambros, 2014-10-14 20:17:36]

On 2014/10/14 19:51:24, Yaron wrote:
> On 2014/10/14 17:00:21, Lambros wrote:
> > On 2014/10/08 18:08:00, Yaron wrote:
> > >
> >
>
https://codereview.chromium.org/624183002/diff/1/content/public/android/java/...
> > > File
> > >
> >
>
content/public/android/java/src/org/chromium/content/browser/crypto/CipherFactory.java
> > > (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/624183002/diff/1/content/public/android/java/...
> > >
> >
>
content/public/android/java/src/org/chromium/content/browser/crypto/CipherFactory.java:155:
> > > seed = mRandomNumberProvider.getBytes(NUM_BYTES);
> > > Looks like you should update CipherFactoryTest. It still thinks the seed
is
> > > coming from the random number provider
> > 
> > I ran the CipherFactoryTests with this patch, and they still pass:
> > $ build/android/test_runner.py instrumentation --test-apk=ContentShellTest
> -vvv
> > -f '*CipherFactoryTest*'
> > ...
> > C   18.768s Main  [==========] 7 tests ran.
> > C   18.769s Main  [  PASSED  ] 7 tests.
> 
> 
> Hmm. Ok it passes but it seems to be mocking things unnecessarily (i.e.
> inserting two randoms) or am I misreading it?

Adding @dfalcantara, another OWNER of CipherFactoryTest.

@yfriedman: which part of CipherFactoryTest.java do you think you might be
misreading?
@dfalcantara or @palmer: Can you answer yfriedman's question?

Is the objection that CipherFactoryTest *should* be broken by this CL (because
it should be testing that SecureRandom is initialized by a particular
RandomNumberProvider, which is no longer the case - it's now initialized by the
factored-out code in SecureRandomInitializer.java). And the fact that the tests
are still passing indicates a problem with the tests?

[Yaron, 2014-10-15 19:01:17]

On 2014/10/14 20:17:36, Lambros wrote:
> On 2014/10/14 19:51:24, Yaron wrote:
> > On 2014/10/14 17:00:21, Lambros wrote:
> > > On 2014/10/08 18:08:00, Yaron wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/624183002/diff/1/content/public/android/java/...
> > > > File
> > > >
> > >
> >
>
content/public/android/java/src/org/chromium/content/browser/crypto/CipherFactory.java
> > > > (left):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/624183002/diff/1/content/public/android/java/...
> > > >
> > >
> >
>
content/public/android/java/src/org/chromium/content/browser/crypto/CipherFactory.java:155:
> > > > seed = mRandomNumberProvider.getBytes(NUM_BYTES);
> > > > Looks like you should update CipherFactoryTest. It still thinks the seed
> is
> > > > coming from the random number provider
> > > 
> > > I ran the CipherFactoryTests with this patch, and they still pass:
> > > $ build/android/test_runner.py instrumentation --test-apk=ContentShellTest
> > -vvv
> > > -f '*CipherFactoryTest*'
> > > ...
> > > C   18.768s Main  [==========] 7 tests ran.
> > > C   18.769s Main  [  PASSED  ] 7 tests.
> > 
> > 
> > Hmm. Ok it passes but it seems to be mocking things unnecessarily (i.e.
> > inserting two randoms) or am I misreading it?
> 
> Adding @dfalcantara, another OWNER of CipherFactoryTest.
> 
> @yfriedman: which part of CipherFactoryTest.java do you think you might be
> misreading?
> @dfalcantara or @palmer: Can you answer yfriedman's question?
> 
> Is the objection that CipherFactoryTest *should* be broken by this CL (because
> it should be testing that SecureRandom is initialized by a particular
> RandomNumberProvider, which is no longer the case - it's now initialized by
the
> factored-out code in SecureRandomInitializer.java). And the fact that the
tests
> are still passing indicates a problem with the tests?

Right, I expected that would break it since now something that was intentionally
mocked to be deterministic is random.
Looking at the tests though, they seem to be written in a way where this is ok,
because they don't try to assert about exact values.

lgtm

[palmer, 2014-10-15 19:12:34]

> Right, I expected that would break it since now something that was
intentionally
> mocked to be deterministic is random.
> Looking at the tests though, they seem to be written in a way where this is
ok,
> because they don't try to assert about exact values.

I think that is an OK test. The thing that should be deterministic is that
plaintext == decrypt(encrypt(key, plaintext)), not that the key itself
necessarily be deterministic. (In fact, a deterministic key generation function
might conceivably *hide* enc/dec problems.)

Still LGTM.

[Yaron, 2014-10-15 20:38:37]

On Wed Oct 15 2014 at 12:12:35 PM <palmer@chromium.org> wrote:

> > Right, I expected that would break it since now something that was
> intentionally
> > mocked to be deterministic is random.
> > Looking at the tests though, they seem to be written in a way where this
> > is
> ok,
> > because they don't try to assert about exact values.
>
> I think that is an OK test. The thing that should be deterministic is that
> plaintext == decrypt(encrypt(key, plaintext)), not that the key itself
> necessarily be deterministic. (In fact, a deterministic key generation
> function
> might conceivably *hide* enc/dec problems.)
>
> Still LGTM.
>

agreed lgtm. I had a naive assumption of the test

>
> https://codereview.chromium.org/624183002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Lambros, 2014-10-15 21:13:24]

The CQ bit was checked by lambroslambrou@chromium.org

[commit-bot: I haz the power, 2014-10-15 21:14:20]

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

[commit-bot: I haz the power, 2014-10-16 00:21:42]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2014-10-16 00:22:23]

Patchset 1 (id:??) landed as
https://crrev.com/df0de36454a6c58757517238c977334b1b421bbb
Cr-Commit-Position: refs/heads/master@{#299788}