Comment by CiPHPerCoder | Hacker News Reader

Comment by CiPHPerCoder | Hacker News Reader

Parent Full thread

> 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

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/b4fad203b94da53d3369...

* It's using AES-256-CBC https://github.com/bitwarden/jslib/blob/b4fad203b94da53d3369... + https://github.com/bitwarden/jslib/blob/b4fad203b94da53d3369... + 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/

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.)

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).

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).

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/...

Oh, this did seem familiar!

The AES-CBC thing is tied to the key, right? So the downgrade attack isn't possible.

Yes, new account keys are identified (presence of a mac key) and block the downgrade (see code link above).

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.

You're thinking signatures, not encryption.

https://paragonie.com/blog/2018/04/protecting-rsa-based-prot...

"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?

Yes, it's using RSA to encrypt a key, as one would hope. 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.

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' :)

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.

Reply on news.ycombinator.com