Closed Bug 903907 Opened 8 years ago Closed 3 years ago

Audit Android Background Service's use of SecureRandom

Categories

(Android Background Services Graveyard :: Crypto, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED WORKSFORME

People

(Reporter: nalexander, Assigned: nalexander)

References

Details

(Keywords: sec-audit)

In response to the vulnerability discussed at

http://thegenesisblock.com/security-vulnerability-in-all-android-bitcoin-wallets/

we need to verify that we're using SecureRandom in a safe way, and if we are not we need to find a secure random number generator.
Group: mozilla-services-security
Assignee: nobody → nalexander
Group: mozilla-services-security → mozilla-corporation-confidential
Group: mozilla-corporation-confidential → core-security
From http://armoredbarista.blogspot.com.au/2013/03/randomly-failed-weaknesses-in-java.html:

"""
One of the found bugs directly affects the Android platform. Th other bug is only present in Apache Harmony and NOT the Android Source Code.

FIRST - When creating a self seeding SecureRandom instance (by calling the constructor without arguments and subsequent setSeed() call), the code fails to adjust the byte offset (a pointer into the state buffer) after inserting a start value. This causes a counter and the beginning of a padding (a 32 bit word) to overwrite parts of the seed instead of appending.

SECOND - When running under a Unix-like OS a new SecureRandom instance is seeded with 20 bytes from the urandom or random device. If both are unaccessible the implementation provides a fall-back seeding facility. Once the seeding facility has gathered the requested amount of bytes, for unknown reasons the most significant bit is set to zero. As a consequence, the effective seed of a SecureRandom instance is only 7/8 for each requested byte, degrading security (of only 64 bits due to the first bug) by another 8 bits to 56 bits. Even worse, due to another invalid modulo reduction the entropy of a single call to the seeding facility is limited to only 31 bits.
"""

Let us discuss "FIRST" first.

Android Background Services (including, but not limited to, Sync) do use the weak SecureRandom class.  Based on reading the code, we create SecureRandom instances in 2 places:

http://hg.mozilla.org/mozilla-central/annotate/d1597e9d7d89/mobile/android/base/sync/net/BaseResource.java#l174
http://hg.mozilla.org/mozilla-central/annotate/d1597e9d7d89/mobile/android/base/sync/Utils.java#l41

The first definitely does not call setSeed.  The second creates a private static instance that can be accessed via two static public methods:

generateRandomBytes -- http://hg.mozilla.org/mozilla-central/annotate/d1597e9d7d89/mobile/android/base/sync/Utils.java#l57
reseedSharedRandom http://hg.mozilla.org/mozilla-central/annotate/d1597e9d7d89/mobile/android/base/sync/Utils.java#l78

generateRandomBytes and its callers will not call setSeed, so the SecureRandom will be seeded from system entropy.

reseedSharedRandom is more troubling.  We do in fact call setSeed, but with the results of generateSeed *from the SecureRandom itself*.  If the shared SecureRandom is not yet seeded, this will seed it using the system entropy.  If it is in fact seeded already, then it must have been seeded using the system entropy.

In fact, we anticipate this to be the common flow: the only caller of reseedSharedRandom is at the beginning of a Sync

http://hg.mozilla.org/mozilla-central/file/d1597e9d7d89/mobile/android/base/sync/syncadapter/SyncAdapter.java#l305

The static instance will usually be seeded once via this path per process life-cycle, which is expected to be long.

In conclusion, my reading of the code, the above links, and the paper is that we do not fall into the bad case of "FIRST".

In the case of "SECOND", where there is no available system entropy, I'm not sure what better alternative we have.  I'm pretty sure we shouldn't be implementing our own entropy collecter.  We could try to get some entropy from NSS via JNI.  Open to suggestions, here.
rnewman: do you agree with my analysis of the relevant code?
Flags: needinfo?(rnewman)
warner: do you have an opinion on getting entropy?
Flags: needinfo?(warner-bugzilla)
(In reply to Nick Alexander :nalexander from comment #2)

> generateRandomBytes and its callers will not call setSeed, so the
> SecureRandom will be seeded from system entropy.

generateRandomBytes is essentially used in two places: for GUID generation and for J-PAKE.

The former isn't worrying. Even shoddy pseudo-random is enough for those.

The latter isn't too worrying either, from my understanding of J-PAKE, but if there's an issue here that's what it'll be. As you point out, it'll be seeded from system entropy.


> In fact, we anticipate this to be the common flow: the only caller of
> reseedSharedRandom is at the beginning of a Sync

And thus the only thing it affects is GUID generation (and HMAC-based auth, which isn't used).


Note that there's one more place that system entropy presumably enters the system: when we encrypt a record we allow javax.crypto.Cipher to provide a random IV. We don't generate our own.

In summary, yes, I agree with your analysis: the only concern here is whether we need a better source of entropy than Android provides. But I am not a cryptographer!
Flags: needinfo?(rnewman)
Marking sec-audit, but please clear that or give it a security rating if it turns out there's actually a security problem.
Keywords: sec-audit
Response from Google:
http://android-developers.blogspot.com/2013/08/some-securerandom-thoughts.html

extract:

We have now determined that applications which use the Java Cryptography Architecture (JCA) for key generation, signing, or random number generation may not receive cryptographically strong values on Android devices due to improper initialization of the underlying PRNG. Applications that directly invoke the system-provided OpenSSL PRNG without explicit initialization on Android are also affected. Applications that establish TLS/SSL connections using the HttpClient and java.net classes are not affected as those classes do seed the OpenSSL PRNG with values from /dev/urandom.

Developers who use JCA for key generation, signing or random number generation should update their applications to explicitly initialize the PRNG with entropy from /dev/urandom or /dev/random. A suggested implementation is provided at the end of this blog post. Also, developers should evaluate whether to regenerate cryptographic keys or other random values previously generated using JCA APIs such as SecureRandom, KeyGenerator, KeyPairGenerator, KeyAgreement, and Signature.
Duplicate of this bug: 905625
Duplicate of this bug: 905676
As rnewman pointed out, predictable GUIDs aren't a problem.

If you can predict the IVs and also supply plaintext to be encrypted, you can
sometimes use that to get a client to decrypt things for you. But if merely
have a better-than-average chance of guessing the IV, then I believe your
ability to do that is pretty limited. So I'm not to worried about it.

This is stretching my understanding a bit (I'm not a cryptographer either),
but I think that if J-PAKE is given not-so-random numbers, somebody might be
able to take a recorded key-exchange session and brute-force the android
device's random values, derive the shared session key, then decrypt the
exchanged master key.

J-PAKE generates five random numbers per side: two main group elements, and
three for zero-knowledge-proofs. It publishes g^x for all of them, which
gives an eavesdropper their brute-force oracle. The ZKPs don't expose
anything long-term (there might be an online/MitM attack enabled by
weaknesses there, but I'm not too worried about it).

But the two main group elements are used (in combination with data from the
other side) to derive the shared session key. If the entropy were really bad,
like 30 bits, then from the recorded g^x you could figure out x, which would
be bad.

These elements aren't generated until we start the J-PAKE process (well after
app startup). Do we have a sense of what else happens with the RNG before
this point, and whether it'll be correctly seeded by then?

Also, in the intervening months, has the root RNG problem been fixed?
Flags: needinfo?(warner-bugzilla)
Even if the problem has been fixed the vast majority of our users will not get a fix till they buy a new phone with the hypothetical fix.
Group: core-security → cloud-services-security
Group: cloud-services-security → firefox-core-security
Product: Android Background Services → Firefox for Android
Component: Android Sync → Crypto
Product: Firefox for Android → Android Background Services
If this is "graveyard" now I guess we don't need this bug?
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
(checking that with Kevin)
Flags: needinfo?(kbrosnan)
I still see uses of securerandom in mobile/. Though maybe this is not as important since Google/Java have addressed this issue in more modern uses https://tersesystems.com/blog/2015/12/17/the-right-way-to-use-securerandom/ ? 

https://searchfox.org/mozilla-central/search?q=SecureRandom&path= 

Nick is another person to ask but he is off until the end of this month and not accepting NI.
Flags: needinfo?(kbrosnan) → needinfo?(s.kaspari)
(In reply to Kevin Brosnan [:kbrosnan] from comment #15)
> I still see uses of securerandom in mobile/. Though maybe this is not as
> important since Google/Java have addressed this issue in more modern uses
> https://tersesystems.com/blog/2015/12/17/the-right-way-to-use-securerandom/
> ? 
> 
> https://searchfox.org/mozilla-central/search?q=SecureRandom&path= 
> 
> Nick is another person to ask but he is off until the end of this month and
> not accepting NI.

I believe this has been addressed entirely by https://bugzilla.mozilla.org/show_bug.cgi?id=1350196, which moved the targeted fix from https://bugzilla.mozilla.org/show_bug.cgi?id=1173229 very early during application startup.
Status: RESOLVED → VERIFIED
Resolution: WONTFIX → WORKSFORME
Flags: needinfo?(s.kaspari)

Removing employee no longer with company from CC list of private bugs.

Group: firefox-core-security
You need to log in before you can comment on or make changes to this bug.