I'm a bit concerned that their random number generator might produce biased output. This is usually a red flag that there are other issues in the code that haven't been examined by a crypto person.
Just a word of caution from a casual glance. For all I know the rest of the code is fine. For all I know, the rest of the code is clunky swiss cheese.
There doesn't appear to be much crypto in this project; it's a small application built on SpiderOak's Crypton.io. I'm not a fan of Crypton, but it's not clownshoes crypto.
Just to be clear to everyone on the thread: it's very unlikely that there's anything practical an attacker can do with the modulus bias in a situation like this.
> Just to be clear to everyone on the thread: it's very unlikely that there's anything practical an attacker can do with the modulus bias in a situation like this.
Correct. This was just the first thing I saw in a cursory glance through their app.js file.
I haven't reviewed Crypton.io and can't say whether I like it or not. What don't you like about it in particular?
Yes, I understand. I was wondering why tptacek wasn't a fan though, and the only reason I could think of was that it might encourage this usage?? even though it looks like they explicitly discourage this[0]. It's the first time I hear of crypton.io - so just trying to learn more.
Encryptr's security bits are likely all implemented in the crypton framework. If you search the repo for that function (randomString), you'll see that it's only used in one place: to propose a new password (https://github.com/devgeeks/Encryptr/search?utf8=%E2%9C%93&q...). While it'd be best to not have any bias at all, this low bias only assists people trying to brute force passwords created by encryptr.
FWIW, the audit happened directly in the middle of a dev cycle, without any of the normal internal review of that code first. The audit report describes those circumstances in the first few pages. Scheduling audits is hard work!
Biases caused by modulo reduction are dangerous when the source integer is relatively close to the target distribution. This bias is unlikely to be useful for attackers. The probability difference between the most likely output, 0, and the less likely outputs is 2^-32. For comparison, Cryptocat's bug had a much larger distance of 1/251 ~ 2^-8, which greatly reduces the amount of samples needed for distinguishing the output from random. EdDSA itself uses modulo reduction for the nonce, using a large enough source (~2^512 modulo ~2^252) value that renders the bias irrelevant.
This is using getRandomValues which is cryptographically secure. The account/login/privacy crypto is using SJCL and the entire app is a signed desktop or mobile application - (no remote code)
Nothing in principle; it's the % operator that can biased the output.
Also, outside the scope of Cordova apps, Node.js uses OpenSSL rather than /dev/urandom for their crypto.getRandomBytes() implementation, so I don't really trust it in that context. ;)
Entropy is maximized when the distribution probability is uniform. A biased random sequence has less entropy than the one of the same length which has uniform distribution.
"For example, assume that your random number source gives numbers from 0 to 99 (as was the case for Fisher and Yates' original tables), and that you wish to obtain an unbiased random number from 0 to 15. If you simply divide the numbers by 16 and take the remainder, you'll find that the numbers 0–3 occur about 17% more often than others. This is because 16 does not evenly divide 100: the largest multiple of 16 less than or equal to 100 is 6×16 = 96, and it is the numbers in the incomplete range 96–99 that cause the bias. The simplest way to fix the problem is to discard those numbers before taking the remainder and to keep trying again until a number in the suitable range comes up. While in principle this could, in the worst case, take forever, the expected number of retries will always be less than one."
If you generate 1000 random integers, but we know there's a 28% chance it will be 0 and a 14% chance it will be 1,2,3,4, or 5, a clever attacker can more accurately predict/brute force outputs.
When you're working with cryptography, you want a uniform distribution of possible values as well as unpredictable randomness.
For 0..max random integer it depends on max and length. If max is 2^32 and 2^32 modulo charset.length = 0 it should be fine. Am I right?
In our case length is 85 and it is indeed biased. Needs a pull request.
Correct, that's the edge case where it actually falls together neatly.
If you're starting with a random byte and charset.length is an even power of 2, you end up with no bias.
It's better to design functions like this to discard values outside of an acceptable range and try again until they generate a safe value (also, apply a & bit mask to reduce the number of retries). This allows you to accept any arbitrary charset size without being concerned about security.
In theory, haha. In practice it gives me some numbers twice more. [2001954,
1000322,
998546,
1001551,
999105,
2000886,
998760,
998705,
1000001,
998424,
1000978,
2000907,
1002097,
998786,
1000101,
998818,
1000381,
1999818,
999662,
1001260,
999531,
1000076,
I thought instead of cutting off out of range results we could num*85/4294967296 and round it. But now realized buckets for rounding won't be of equal size and it's wrong.
I'm a bit concerned that their random number generator might produce biased output. This is usually a red flag that there are other issues in the code that haven't been examined by a crypto person.
Just a word of caution from a casual glance. For all I know the rest of the code is fine. For all I know, the rest of the code is clunky swiss cheese.
Further reading on biased RNGs, with a visual: https://stackoverflow.com/a/31374501/2224584