Issue 463393002: Android Chromoting: Initialize SecureRandom generator.

Issue 463393002: Android Chromoting: Initialize SecureRandom generator. (Closed)

Created:
6 years, 4 months ago by Lambros

Modified:
6 years, 4 months ago

Reviewers:
palmer, kelvinp

CC:
chromium-reviews, chromoting-reviews_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Project:
chromium

Visibility:
Public.

More Reviews

Description

Android Chromoting: Initialize SecureRandom generator. BUG=402732 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289519

Patch Set 2 : Pull initialization code into a reusable class #

Total comments: 8

Created: 6 years, 4 months ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+56 lines, -1 line)			Patch
	A	remoting/android/java/src/org/chromium/chromoting/SecureRandomInitializer.java	View	1	1 chunk	+41 lines, -0 lines	6 comments	Download
	M	remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java	View	1	3 chunks	+15 lines, -1 line	2 comments	Download

Messages

Total messages: 10 (0 generated)

Expand Messages | Collapse Messages

Lambros

@kelvinp: PTAL @palmer: Do we actually need a secure random generator, given that we're only ...

6 years, 4 months ago (2014-08-13 00:58:29 UTC) #1

kelvinp

https://codereview.chromium.org/463393002/diff/1/remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java File remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java (right): https://codereview.chromium.org/463393002/diff/1/remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java#newcode57 remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:57: sSecureRandom.setSeed(getRandomBytes(NUM_RANDOM_BYTES)); As discussed, maybe it is better to move ...

6 years, 4 months ago (2014-08-13 02:33:26 UTC) #2

chromium-reviews

Yes, XSRF tokens should be cryptographically strong. LGTM. But maybe this logic should go in ...

6 years, 4 months ago (2014-08-13 06:23:22 UTC) #3

Yes, XSRF tokens should be cryptographically strong.

LGTM. But maybe this logic should go in some reused utility class? It's
been copied and pasted 3 times now...
On Aug 12, 2014 5:58 PM, <lambroslambrou@chromium.org> wrote:

> Reviewers: kelvinp, Chromium Palmer,
>
> Message:
> @kelvinp: PTAL
> @palmer: Do we actually need a secure random generator, given that we're
> only
> using it for XSRF detection?
>
>
> Description:
> Android Chromoting: Initialize SecureRandom generator.
>
> BUG=402732
>
> Please review this at https://codereview.chromium.org/463393002/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files (+39, -1 lines):
>   M remoting/android/java/src/org/chromium/chromoting/
> ThirdPartyTokenFetcher.java
>
>
> Index: remoting/android/java/src/org/chromium/chromoting/
> ThirdPartyTokenFetcher.java
> diff --git
a/remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java
> b/remoting/android/java/src/org/chromium/chromoting/
> ThirdPartyTokenFetcher.java
> index ac195cf617acbc1cbb598000b5dd1fa87c2dd55c..
> 71a23f50bd8f70e7ec29ea0dbe42ffa86c2d221b 100644
> --- a/remoting/android/java/src/org/chromium/chromoting/
> ThirdPartyTokenFetcher.java
> +++ b/remoting/android/java/src/org/chromium/chromoting/
> ThirdPartyTokenFetcher.java
> @@ -4,6 +4,7 @@
>
>  package org.chromium.chromoting;
>
> +import android.annotation.SuppressLint;
>  import android.app.Activity;
>  import android.content.ActivityNotFoundException;
>  import android.content.ComponentName;
> @@ -14,6 +15,8 @@ import android.text.TextUtils;
>  import android.util.Base64;
>  import android.util.Log;
>
> +import java.io.FileInputStream;
> +import java.io.IOException;
>  import java.security.SecureRandom;
>  import java.util.ArrayList;
>
> @@ -38,8 +41,21 @@ public class ThirdPartyTokenFetcher {
>       */
>      private static final String RESPONSE_TYPE = "code token";
>
> +    /** Used to initialize the random number generator. */
> +    private static final int NUM_RANDOM_BYTES = 16;
> +
>      /** This is used to securely generate an opaque 128 bit for the
> |mState| variable. */
> -    private static SecureRandom sSecureRandom = new SecureRandom();
> +    @SuppressLint("TrulyRandom")
> +    private static SecureRandom sSecureRandom;
> +
> +    static {
> +        // Versions of SecureRandom from Android <= 4.3 do not seed
> themselves as
> +        // securely as possible. This workaround should suffice until the
> fixed version
> +        // is deployed to all users. It reads random data from
> /dev/urandom, which is as good as
> +        // the platform can get.
> +        sSecureRandom = new SecureRandom();
> +        sSecureRandom.setSeed(getRandomBytes(NUM_RANDOM_BYTES));
> +    }
>
>      /** This is used to launch the third party login page in the browser.
> */
>      private Activity mContext;
> @@ -61,6 +77,28 @@ public class ThirdPartyTokenFetcher {
>
>      private final String mRedirectUri;
>
> +    private static byte[] getRandomBytes(int numBytes) {
> +        FileInputStream fis = null;
> +        try {
> +            fis = new FileInputStream("/dev/urandom");
> +            byte[] bytes = new byte[numBytes];
> +            if (bytes.length != fis.read(bytes)) {
> +                throw new SecurityException("Failed to get enough random
> data.");
> +            }
> +            return bytes;
> +        } catch (IOException e) {
> +            throw new SecurityException("Error reading random data.");
> +        } finally {
> +            try {
> +                if (fis != null) {
> +                    fis.close();
> +                }
> +            } catch (IOException e) {
> +                // Ignore exception closing the device.
> +            }
> +        }
> +    }
> +
>      public ThirdPartyTokenFetcher(Activity context,
>                                    ArrayList<String> tokenUrlPatterns,
>                                    Callback callback) {
>
>
>

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

Lambros

PTAL. I've moved the initialization code into a separate class. This will make it easier ...

6 years, 4 months ago (2014-08-13 23:48:51 UTC) #4

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/463393002/20001

6 years, 4 months ago (2014-08-14 01:11:47 UTC) #7

commit-bot: I haz the power

Committed patchset #2 (20001) as 289519

6 years, 4 months ago (2014-08-14 11:02:03 UTC) #8

palmer

https://codereview.chromium.org/463393002/diff/20001/remoting/android/java/src/org/chromium/chromoting/SecureRandomInitializer.java File remoting/android/java/src/org/chromium/chromoting/SecureRandomInitializer.java (right): https://codereview.chromium.org/463393002/diff/20001/remoting/android/java/src/org/chromium/chromoting/SecureRandomInitializer.java#newcode5 remoting/android/java/src/org/chromium/chromoting/SecureRandomInitializer.java:5: package org.chromium.chromoting; Is there some more general place we ...

6 years, 4 months ago (2014-08-18 17:47:52 UTC) #9

Lambros

6 years, 4 months ago (2014-08-18 20:31:31 UTC) #10

Message was sent while issue was closed.

Thanks @Palmer for the comments. I'm going away for 3 weeks so I'll take another
look when I get back (if someone else hasn't moved it already!)

https://codereview.chromium.org/463393002/diff/20001/remoting/android/java/sr...
File
remoting/android/java/src/org/chromium/chromoting/SecureRandomInitializer.java
(right):

https://codereview.chromium.org/463393002/diff/20001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/SecureRandomInitializer.java:5:
package org.chromium.chromoting;
On 2014/08/18 17:47:52, Chromium Palmer wrote:
> Is there some more general place we can put this so that all the code can use
> it?

Probably base/android, to be accessible to our app. We don't use content/ or
chrome/. We do use net/ and ui/ but this code probably doesn't belong there.

https://codereview.chromium.org/463393002/diff/20001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/SecureRandomInitializer.java:23:
FileInputStream fis = null;
On 2014/08/18 17:47:52, Chromium Palmer wrote:
> Nit: Reference types are always initialized to null by default.

Acknowledged.

https://codereview.chromium.org/463393002/diff/20001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/SecureRandomInitializer.java:26:
byte[] bytes = new byte[NUM_RANDOM_BYTES];
On 2014/08/18 17:47:52, Chromium Palmer wrote:
> Nit: You could optimize a bit by making the array be a field in this class,
> rather than allocating on each call.

Acknowledged.

https://codereview.chromium.org/463393002/diff/20001/remoting/android/java/sr...
File
remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java
(right):

https://codereview.chromium.org/463393002/diff/20001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/ThirdPartyTokenFetcher.java:52:
SecureRandomInitializer.initialize(sSecureRandom);
On 2014/08/18 17:47:52, Chromium Palmer wrote:
> I almost wonder if the interface should instead be:
> 
>     sSecureRandom = SecureRandomInitializer.getInstance();

I thought about that, but there are a lot of SecureRandom ctors and factory
methods to override. I'm open to persuasion though - maybe there should be One
True Way for all of Chromium code to get a SecureRandom object?

Expand Messages | Collapse Messages