4 ms·

https://github.com/devgeeks/Encryptr/blob/64223f0cb4adba80c8d89f23762bba0c8dc7fa3c/src/app.js#L285-L298 https://github.com/devgeeks/Encryptr/blob/64223f0cb4adba

https://github.com/devgeeks/Encryptr/blob/64223f0cb4adba80c8d89f23762bba0c8dc7fa3c/src/app.js#L285-L298 https://github.com/devgeeks/Encryptr/blob/64223f0cb4adba80c8... 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 https://stackoverflow.com/a/31374501/2224584

tptacek 11y ago
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.
- sarciszewski 11y ago
  > 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?
- gingerlime 11y ago
  > I'm not a fan of Crypton Just curious - is this to do with using javascript crypto[0]? or something that goes beyond that? [0] https://www.nccgroup.trust/us/about-us/newsroom-and-events/blog/2011/august/javascript-cryptography-considered-harmful/ https://www.nccgroup.trust/us/about-us/newsroom-and-events/b...
  - sarciszewski 11y ago
    JavaScript is only a real issue when you have an insecure code delivery mechanism. The article spells that out pretty well.
    - gingerlime 11y ago
      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. [0] https://crypton.io/docs/ https://crypton.io/docs/
homakov 11y ago
What's wrong with window.crypto.getRandomValues?
- sarciszewski 11y ago
  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. ;)
  - homakov 11y ago
    % shouldn't decrease the entropy, modulo of random is still random.
    - sarciszewski 11y ago
      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.
      - homakov 11y ago
        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.
        sarciszewski 11y ago
        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. See also: https://github.com/paragonie/random_compat/blob/5aa6689651a58a1f97e87d9e1188e5d90c871cc3/lib/random.php#L393-L447 https://github.com/paragonie/random_compat/blob/5aa6689651a5... I'll open a PR tonight if nobody beats me to it.
shuzchen 11y ago
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=randomString 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. Crypton itself has been audited a couple times by crypto persons, see https://crypton.io/docs/security/audits.html https://crypton.io/docs/security/audits.html (disclaimer: i used to work at spideroak, but neither on crypton nor encryptr. i still think they're all awesome though)
- cmrx64 11y ago
  The contents of the second audit in particular deeply troubled me. Lots of simple but grevious mistakes. It's nice that they got the audit though.
  - daviddahl 11y ago
    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!
    - sarciszewski 11y ago
      Yeah, especially when there are no clearly defined "sprints".
daviddahl 11y ago
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)
- sarciszewski 11y ago
  Yes, but see the other comments about how the % operator can introduce bias into an RNG's output.
pbsd 11y ago
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.

tptacek 11y ago

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.

sarciszewski 11y ago
> 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?
gingerlime 11y ago
> I'm not a fan of Crypton Just curious - is this to do with using javascript crypto[0]? or something that goes beyond that? [0] https://www.nccgroup.trust/us/about-us/newsroom-and-events/blog/2011/august/javascript-cryptography-considered-harmful/ https://www.nccgroup.trust/us/about-us/newsroom-and-events/b...
- sarciszewski 11y ago
  JavaScript is only a real issue when you have an insecure code delivery mechanism. The article spells that out pretty well.
  - gingerlime 11y ago
    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. [0] https://crypton.io/docs/ https://crypton.io/docs/

homakov 11y ago

What's wrong with window.crypto.getRandomValues?

sarciszewski 11y ago
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. ;)
- homakov 11y ago
  % shouldn't decrease the entropy, modulo of random is still random.
  - sarciszewski 11y ago
    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.
    - homakov 11y ago
      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.
      - sarciszewski 11y ago
        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. See also: https://github.com/paragonie/random_compat/blob/5aa6689651a58a1f97e87d9e1188e5d90c871cc3/lib/random.php#L393-L447 https://github.com/paragonie/random_compat/blob/5aa6689651a5... I'll open a PR tonight if nobody beats me to it.

shuzchen 11y ago

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=randomString 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. Crypton itself has been audited a couple times by crypto persons, see https://crypton.io/docs/security/audits.html https://crypton.io/docs/security/audits.html (disclaimer: i used to work at spideroak, but neither on crypton nor encryptr. i still think they're all awesome though)

cmrx64 11y ago
The contents of the second audit in particular deeply troubled me. Lots of simple but grevious mistakes. It's nice that they got the audit though.
- daviddahl 11y ago
  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!
  - sarciszewski 11y ago
    Yeah, especially when there are no clearly defined "sprints".

daviddahl 11y ago

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)

sarciszewski 11y ago
Yes, but see the other comments about how the % operator can introduce bias into an RNG's output.

pbsd 11y ago

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.