4 ms·

> Wondering how they will address the current cryptographic scheme though. The only cryptographic weakness Cure53 identified was that a malicious API server co

> Wondering how they will address the current cryptographic scheme though. The only cryptographic weakness Cure53 identified was that a malicious API server could exfiltrate encryption keys. Cure53 deemed it a hard problem to solve. I wrote a proposed strategy for mitigating it: https://github.com/bitwarden/core/issues/392 https://github.com/bitwarden/core/issues/392 Regarding Bitwarden's cryptographic security, a cursory read through their code yields the following: * It's using RSA-OAEP to encrypt AES keys (EDIT: formerly "some data") https://github.com/bitwarden/jslib/blob/b4fad203b94da53d33693f4283d7249e3a8f1afe/src/services/crypto.service.ts#L382-L399 https://github.com/bitwarden/jslib/blob/b4fad203b94da53d3369... * It's using AES-256-CBC https://github.com/bitwarden/jslib/blob/b4fad203b94da53d33693f4283d7249e3a8f1afe/src/services/crypto.service.ts#L345-L380 https://github.com/bitwarden/jslib/blob/b4fad203b94da53d3369... + https://github.com/bitwarden/jslib/blob/b4fad203b94da53d33693f4283d7249e3a8f1afe/src/services/crypto.service.ts#L494-L508 https://github.com/bitwarden/jslib/blob/b4fad203b94da53d3369... + https://github.com/bitwarden/jslib/blob/2045e7047a66599b2c8a92b88cd0d1b8bfc5186f/src/services/nodeCryptoFunction.service.ts#L71-L78 https://github.com/bitwarden/jslib/blob/2045e7047a66599b2c8a... It doesn't appear to be authenticating the AES-CBC-encrypted ciphertexts in all cases, which makes me suspect padding oracles are still in-scope. https://robertheaton.com/2013/07/29/padding-oracle-attack/ https://robertheaton.com/2013/07/29/padding-oracle-attack/ RSA-OAEP is the better RSA mode. (You don't want PKCS1v1.5) In closing: As long as you're not for some reason storing unauthenticated AES-CBC ciphertexts in the server, the encryption is really boring. (Boring is good for encryption.)

tialaramex 8y ago
"to encrypt some data" ? Actually data? You'd usually expect RSA to be protecting a symmetric key in this sort of setup - is that what the data is, or something else?
- CiPHPerCoder 8y ago
  Yes, it's using RSA to encrypt a key, as one would hope. https://github.com/bitwarden/jslib/blob/b4fad203b94da53d33693f4283d7249e3a8f1afe/src/services/crypto.service.ts#L312 https://github.com/bitwarden/jslib/blob/b4fad203b94da53d3369... Usually when I see RSA-OAEP in a casual stroll through something's code, I stop there and move onto looking for other issues. Reason: Very few users of RSA encryption bother to use a secure padding mode. If they're doing that much, the chances of doing something very stupid (a.k.a. "RSA-ECB") is low enough to discount for the purposes of message board discussions. (Obviously, if I'm being paid to review something, I spend a lot more time on it.) When I wrote my post above, all I cared about was the modes being used. That's why I vaguely said "some data". A further analysis (i.e. where rsaEncrypt() is invoked) yields: They're only using RSA for encrypting AES keys, which is a sane design. Hopefully my lazy word choice didn't cause you (or anyone else) any undue alarm.
  - GordonS 8y ago
    When you said 'data' I assumed you meant a hybrid RSA-AES scheme - of course keys are technically 'data', but when talking about data in the context of cryptography, it usually means 'data that isn't a key' :)
  - tialaramex 8y ago
    You clearly deal with more competent people than me. Literally the last piece of code I read that specified RSA OAEP was trying to shove user session data into it. Thanks for replying to put my mind at ease on this.
xxkylexx 8y ago
All AES-CBC data is authenticated with HMAC SHA-256. This was highlighted in the BWN-01-011 issue (which was determined to be a false positive since it was deemed that authentication was properly done).
- CiPHPerCoder 8y ago
  I haven't traced through the app's code to verify that is true. Recommendation: If there is no HMAC tag with a ciphertext, immediately throw an exception. It makes it clearer that a decryption failure occurred (thus avoiding false positives).
  - xxkylexx 8y ago
    It does do this [1], however, it is a little more complex since Bitwarden has to backwards-compat support old data that was AES-CBC encrypted from long ago before auth checks were implemented, while also combating against downgrade attacks. This same discussion was had back in January when you (I assume this is PIE Scott) reported the problem in issue 306171 on HackerOne which was closed out. [1]: https://github.com/bitwarden/jslib/blob/master/src/services/crypto.service.ts#L547 https://github.com/bitwarden/jslib/blob/master/src/services/...
    - CiPHPerCoder 8y ago
      Oh, this did seem familiar! The AES-CBC thing is tied to the key, right? So the downgrade attack isn't possible.
      - xxkylexx 8y ago
        Yes, new account keys are identified (presence of a mac key) and block the downgrade (see code link above).
rocqua 8y ago
Last I heard on RSA-OAEP vs RSA-PKCS is that, since RSA-PKCS got a 'security proof' it is actually favored. Reason being that it gives similar guarantees but is easier to compute. I don't know the details of the security proof for RSA-PKCS though, just that there is one.
- CiPHPerCoder 8y ago
  You're thinking signatures, not encryption. https://paragonie.com/blog/2018/04/protecting-rsa-based-protocols-against-adaptive-chosen-ciphertext-attacks https://paragonie.com/blog/2018/04/protecting-rsa-based-prot...

tialaramex 8y ago

"to encrypt some data" ? Actually data? You'd usually expect RSA to be protecting a symmetric key in this sort of setup - is that what the data is, or something else?

CiPHPerCoder 8y ago
Yes, it's using RSA to encrypt a key, as one would hope. https://github.com/bitwarden/jslib/blob/b4fad203b94da53d33693f4283d7249e3a8f1afe/src/services/crypto.service.ts#L312 https://github.com/bitwarden/jslib/blob/b4fad203b94da53d3369... Usually when I see RSA-OAEP in a casual stroll through something's code, I stop there and move onto looking for other issues. Reason: Very few users of RSA encryption bother to use a secure padding mode. If they're doing that much, the chances of doing something very stupid (a.k.a. "RSA-ECB") is low enough to discount for the purposes of message board discussions. (Obviously, if I'm being paid to review something, I spend a lot more time on it.) When I wrote my post above, all I cared about was the modes being used. That's why I vaguely said "some data". A further analysis (i.e. where rsaEncrypt() is invoked) yields: They're only using RSA for encrypting AES keys, which is a sane design. Hopefully my lazy word choice didn't cause you (or anyone else) any undue alarm.
- GordonS 8y ago
  When you said 'data' I assumed you meant a hybrid RSA-AES scheme - of course keys are technically 'data', but when talking about data in the context of cryptography, it usually means 'data that isn't a key' :)
- tialaramex 8y ago
  You clearly deal with more competent people than me. Literally the last piece of code I read that specified RSA OAEP was trying to shove user session data into it. Thanks for replying to put my mind at ease on this.

xxkylexx 8y ago

All AES-CBC data is authenticated with HMAC SHA-256. This was highlighted in the BWN-01-011 issue (which was determined to be a false positive since it was deemed that authentication was properly done).

CiPHPerCoder 8y ago
I haven't traced through the app's code to verify that is true. Recommendation: If there is no HMAC tag with a ciphertext, immediately throw an exception. It makes it clearer that a decryption failure occurred (thus avoiding false positives).
- xxkylexx 8y ago
  It does do this [1], however, it is a little more complex since Bitwarden has to backwards-compat support old data that was AES-CBC encrypted from long ago before auth checks were implemented, while also combating against downgrade attacks. This same discussion was had back in January when you (I assume this is PIE Scott) reported the problem in issue 306171 on HackerOne which was closed out. [1]: https://github.com/bitwarden/jslib/blob/master/src/services/crypto.service.ts#L547 https://github.com/bitwarden/jslib/blob/master/src/services/...
  - CiPHPerCoder 8y ago
    Oh, this did seem familiar! The AES-CBC thing is tied to the key, right? So the downgrade attack isn't possible.
    - xxkylexx 8y ago
      Yes, new account keys are identified (presence of a mac key) and block the downgrade (see code link above).

rocqua 8y ago

Last I heard on RSA-OAEP vs RSA-PKCS is that, since RSA-PKCS got a 'security proof' it is actually favored. Reason being that it gives similar guarantees but is easier to compute. I don't know the details of the security proof for RSA-PKCS though, just that there is one.

CiPHPerCoder 8y ago
You're thinking signatures, not encryption. https://paragonie.com/blog/2018/04/protecting-rsa-based-protocols-against-adaptive-chosen-ciphertext-attacks https://paragonie.com/blog/2018/04/protecting-rsa-based-prot...