via lint: TrulyRandom: Weak RNG ../../src/main/java/org/mozilla/gecko/browserid/DSACryptoImplementation.java:147: Potentially insecure random numbers on Android 4.3 and older. Read https://android-developers.blogspot.com/2013/08/some-securerandom-thoughts.html for more info. 144 145 public static BrowserIDKeyPair generateKeyPair(int keysize) 146 throws NoSuchAlgorithmException { 147 final KeyPairGenerator keyPairGenerator = KeyPairGenerator.getInstance("DSA"); 148 keyPairGenerator.initialize(keysize); 149 final KeyPair keyPair = keyPairGenerator.generateKeyPair(); Priority: 9 / 10 Category: Security Severity: Warning Explanation: Weak RNG. Key generation, signing, encryption, and random number generation may not receive cryptographically strong values due to improper initialization of the underlying PRNG on Android 4.3 and below. If your application relies on cryptographically secure random number generation you should apply the workaround described in https://android-developers.blogspot.com/2013/08/some-securerandom-thoughts.html . This lint rule is mostly informational; it does not accurately detect whether cryptographically secure RNG is required, or whether the workaround has already been applied. After reading the blog entry and updating your code if necessary, you can disable this lint issue. More info: https://android-developers.blogspot.com/2013/08/some-securerandom-thoughts.html --- Nick, you're on `hg blame` - can you look into this?

Is this related to Bug 903907?

(In reply to Richard Newman [:rnewman] from comment #1) > Is this related to Bug 903907? I believe it is the same underlying issue, yes -- but we have grown code where it actually needs to be addressed. In the FxA process, we generate certain relatively-long-lived DSA keys. The DSA key generator initialization itself is impacted by the lack of true randomness :( We should apply the work-around described in https://android-developers.blogspot.com/2013/08/some-securerandom-thoughts.html for safety. We also never revisited our choice of key-length. At the moment we use 1024-bit DSA keys, which is definitely not a conservative choice, and was essentially me saying "good enough for now". This is relevant because we should cycle all user keys, so that existing users get the benefit of this fix. (This is possible because these DSA keys are only used to sign per-device assertions; they are client specific and private to a single client.)

ckarlof: you should be aware of this client weakness. markh: can you direct me to Desktop's Browser ID key generation parameters? We may want to match Desktop.

Thanks. It looks like we use DS160 (https://dxr.mozilla.org/mozilla-central/source/services/fxaccounts/FxAccounts.jsm#282), which are 1024 bit keys I believe (https://dxr.mozilla.org/mozilla-central/source/toolkit/identity/IdentityCryptoService.cpp#385).

Hi Nick, has there been any movement on this?

(In reply to Matt Wobensmith from comment #5) > Hi Nick, has there been any movement on this? No, sorry. I have a lot on my plate, and this just isn't that high a priority. I could be convinced otherwise, I guess.

(In reply to Nick Alexander :nalexander from comment #6) > (In reply to Matt Wobensmith from comment #5) > > Hi Nick, has there been any movement on this? > > No, sorry. I have a lot on my plate, and this just isn't that high a > priority. I could be convinced otherwise, I guess. This is the only open sec-high bug in the Firefox for Android product. If you don't think this is an accurate rating, we could ask the security team to re-triage it, but otherwise we should make a plan for fixing this.

I think it's worth testing whether the proposed fix in that blog post dramatically impacts main-thread startup. The main concern with use of javax.crypto stuff is Bug 959652 -- that getting a digest function or other use will result in fortress.Services.initServiceInfo being called. If that's not a side effect, then swizzling in a better RNG at the beginning of startup on affected devices seems like a fine idea to me.

Here's a startup method trace with the patch attached.

700+ milliseconds applying these PRNG fixes in onCreate. This is with a Samsung Galaxy S3 runnin gAndroid 4.3. Tragic.

Here's a startup trace with install and verify separated. Still 250ms to install, but clearly shows 2/3 of time (500ms) in verification. We could delay verification.

(In reply to Nick Alexander :nalexander from comment #12) > Created attachment 8691091 [details] > startup-1448321999341.trace > > Here's a startup trace with install and verify separated. Still 250ms to > install, but clearly shows 2/3 of time (500ms) in verification. We could > delay verification. margaret: mfinkle: rnewman: I claim that 250ms on the main thread during Application onCreate is too much, but I have no particular reference point. Please confirm or deny. I think we will have to be more circumspect here. We will delay installing the fix until later. We will block the only known consumer (https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/browserid/DSACryptoImplementation.java#145) on the fixes being installed (and possibly verifying, although if we can't verify we shouldn't fail anyways...). I would like it to be as late as Gecko:DelayedStartup (https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#707), but if we do this we need to be careful to handle this smoothly in background services. That leaves a window for a future consumer to fall afoul of this, but it's a small-ish window, most likely to be hit by background services that run without Gecko running.

(In reply to Nick Alexander :nalexander from comment #13) > (In reply to Nick Alexander :nalexander from comment #12) > > Created attachment 8691091 [details] > > startup-1448321999341.trace > > > > Here's a startup trace with install and verify separated. Still 250ms to > > install, but clearly shows 2/3 of time (500ms) in verification. We could > > delay verification. > > margaret: mfinkle: rnewman: I claim that 250ms on the main thread during > Application onCreate is too much, but I have no particular reference point. > Please confirm or deny. > > I think we will have to be more circumspect here. We will delay installing > the fix until later. We will block the only known consumer > (https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/ > browserid/DSACryptoImplementation.java#145) on the fixes being installed > (and possibly verifying, although if we can't verify we shouldn't fail > anyways...). > > I would like it to be as late as Gecko:DelayedStartup > (https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp. > java#707), but if we do this we need to be careful to handle this smoothly > in background services. > > That leaves a window for a future consumer to fall afoul of this, but it's a > small-ish window, most likely to be hit by background services that run > without Gecko running. rnewman: mfinkle: margaret: please opine.

250ms blocking startup is more than we'd like to deal with. With our use of background threads, we might see this compound in unknown ways to create a larger impact. I assume this usage needs to be cryptographically secure. Is it possible to only install the fixes in DSAVerifyingPublicKey.generateKeyPair ? Is it possible to switch to exposing GenerateDSAKeyPair (from IdentityCryptoService.cpp) via JNI?

Concur; 250msec is too much. Nick: my hypothesis is that this is front-loading the initialization cost for Fortress, probably due to this line: // Install a Linux PRNG-based SecureRandom implementation as the // default, if not yet installed. Provider[] secureRandomProviders = Security.getProviders("SecureRandom.SHA1PRNG"); We are _currently_ paying that cost at point of first use: FHR constructing its environment, FxA doing DSA, or the favicon fetcher or other Java network component doing TLS/SSL. If that hypothesis is true -- that is, we're just moving this time around, not adding much more to it -- then I would suggest adding a lazy thread-safe run-once wrapper and doing this prior to first use. If it's false (and perhaps even then!) I'd prefer for us to use native code. A little bit of profiling will show where this time is going.

(In reply to Mark Finkle (:mfinkle) from comment #15) > 250ms blocking startup is more than we'd like to deal with. With our use of > background threads, we might see this compound in unknown ways to create a > larger impact. > > I assume this usage needs to be cryptographically secure. It does. > Is it possible to only install the fixes in > DSAVerifyingPublicKey.generateKeyPair ? Yes, absolutely. The reason to do so earlier is to prevent this issue arising in future. > Is it possible to switch to exposing GenerateDSAKeyPair (from > IdentityCryptoService.cpp) via JNI? A quick read suggests yes, it is. The existing code is a thin XPCOM wrapper around the core NSS functionality. It's my understanding that there's no magic there, and that we can access NSS via the JNI without issue. It's a straight-forward but time-consuming task to do so with minimal duplication: the existing code is clear but tied to XPCOM. Extracting shared functionality so that there's a layer cake (NSS, wrapper, {XPCOM,JNI}) is probably not a great fit, since the wrapper in the middle provides little value. So we'd end up duplicating the core functionality and exposing ourselves to future security issues as the two implementations diverge. Such is life.

(In reply to Richard Newman [:rnewman] from comment #16) > Concur; 250msec is too much. > > Nick: my hypothesis is that this is front-loading the initialization cost > for Fortress, probably due to this line: > > // Install a Linux PRNG-based SecureRandom implementation as the > // default, if not yet installed. > Provider[] secureRandomProviders = > Security.getProviders("SecureRandom.SHA1PRNG"); Correct. > We are _currently_ paying that cost at point of first use: FHR constructing > its environment, FxA doing DSA, or the favicon fetcher or other Java network > component doing TLS/SSL. Correct -- the piper must be paid. > If that hypothesis is true -- that is, we're just moving this time around, > not adding much more to it -- then I would suggest adding a lazy thread-safe > run-once wrapper and doing this prior to first use. The issue with doing this is that it's easy to err in the future. > If it's false (and perhaps even then!) I'd prefer for us to use native code. > > A little bit of profiling will show where this time is going. I believe your analysis is correct -- I attached the screencap earlier to demonstrate this. I am not against doing this in native code, although it's not trivial. Reading further, I'm particularly concerned about blocking NSS shutdown while we consume it.

Margaret, has there been any further discussion on this?

(In reply to Matt Wobensmith [:mwobensmith][:matt] from comment #19) > Margaret, has there been any further discussion on this? I did a good deal of work on this and never got it over the line. I'm pretty sure my work would handle the immediate issue but wouldn't re-generate keys that already had been generated for existing users. We could split that part out into a separate (not necessarily confidential) ticket.

Comment on attachment 8715559 [details] [diff] [review] dsa.patch Review of attachment 8715559 [details] [diff] [review]: ----------------------------------------------------------------- I think you also need a small patch to disable that particular lint warning for that particular file… then we can leave it on and catch other uses elsewhere.

Nick, follow-up action for you: request sec-approval.

(In reply to Richard Newman [:rnewman] from comment #22) > Comment on attachment 8715559 [details] [diff] [review] > dsa.patch > > Review of attachment 8715559 [details] [diff] [review]: > ----------------------------------------------------------------- > > I think you also need a small patch to disable that particular lint warning > for that particular file… then we can leave it on and catch other uses > elsewhere. Mmm, perhaps. I suppressed the warning at the call-site in question, but perhaps the linter catches PRNGFixes.java as well. Follow-up when I finally land Bug 1233882. Thanks!

Comment on attachment 8715559 [details] [diff] [review] dsa.patch [Security approval request comment] How easily could an exploit be constructed based on the patch? Not easily; this is a prophylactic. We do, however, have something like one million weak keys in the wild. See Bug 1227342 to try to rotate those keys. These keys are used as the public/private key pair in a browser ID authentication loop. We ask the upstream FxA auth server to sign the public key. The certificate from the FxA server (== signed public key) is then signed with the corresponding private key and exchanged to allow access to a Sync server. All that means: it's not enough to just have a weak key (some how!), you have to be able to authenticate to the FxA auth server to get it signed. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Maybe? Not really. Which older supported branches are affected by this flaw? All. If not all supported branches, which bug introduced the flaw? None. Android be broke, yo. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? It will be easy to backport as far as needed. How likely is this patch to cause regressions; how much testing does it need? Very unlikely to cause regressions. We may see some bad devices in the wild, but we'll just fail to apply and then move on. Too bad; you're mildly weaker than you could be. But I expect Google will have gotten it reasonably right. In addition, only some Android APIs are impacted, so the number of impacted devices is dropping over time.

Comment on attachment 8715559 [details] [diff] [review] dsa.patch sec-approval+. Let's get Aurora and Beta patch nominations as well.

Comment on attachment 8715559 [details] [diff] [review] dsa.patch Approval Request Comment [Feature/regressing bug #]: Some Android versions be buggy, yo! [User impact if declined]: internal keys are not full cryptographic strength. [Describe test coverage new/current, TreeHerder]: very little manual testing. We should bake this on Nightly for a few days to ferret out issues. [Risks and why]: I doubt there are too many show stoppers. Google has this code out there and has addressed some issues seen in the wild already. It only applies to a small and shrinking set of devices, because newer Android versions are not weak in this way. And finally we catch exceptions and log them, and accept weaker keys at worst. [String/UUID change made/needed]: none.

Comment on attachment 8715559 [details] [diff] [review] dsa.patch Approved for aurora, improves security.

Comment on attachment 8715559 [details] [diff] [review] dsa.patch Actually looks like the plan is to land this on nightly first.

https://hg.mozilla.org/integration/fx-team/rev/9c8bdf34b06654ef4c84854d2a49726c918febb9 Bug 1173229 - Apply Google-recommended PRNG fixes before generating DSA keys. r=rnewman

Almost positive this is already in m-c, just didn't get marked in here: https://hg.mozilla.org/mozilla-central/rev/9c8bdf34b06654ef4c84854d2a49726c918febb9

OK, so this landed on Feb. 10th. No obvious issues have been connected to the landing. I think we are ok to uplift this to aurora and beta now.

Comment on attachment 8715559 [details] [diff] [review] dsa.patch Approved for aurora uplift since this has had a week on m-c without issues.

Comment on attachment 8715559 [details] [diff] [review] dsa.patch Just like Liz, taking it. Should be in 45 beta 8