Was reading through common/crypto/Encryption.java and noticed the symmetric cipher is just:
public static final String SYM_CIPHER = "AES";
...
Cipher cipher = Cipher.getInstance(SYM_CIPHER);
Cipher.getInstance("AES") resolves to the JCE default, which is AES/ECB/PKCS5Padding. So everything that goes through encrypt() / encryptPayloadWithHmac() is ECB: no IV, deterministic, and identical 16-byte plaintext blocks produce identical ciphertext blocks. That's a problem because this path isn't just used for ephemeral message keys — it also encrypts longer-lived stuff: the identity keys in KeyStorage, the persisted app data in PersistenceManager, EncryptedConnectionList, and the payment-account payload exchanged during a trade. Structured protobuf records under a long-lived key are pretty much the worst case for ECB pattern leakage.
The other thing in the same code: encryptPayloadWithHmac() uses the same SecretKey for the AES cipher and for the HMAC. Reusing one key across two primitives breaks key separation. It's not catastrophic with HMAC-SHA256 + AES specifically, but there's no reason to do it.
Suggested fix:
- Move to an AEAD so confidentiality + integrity come from one vetted construction:
AES/GCM/NoPadding with a fresh 96-bit nonce per message (or ChaCha20-Poly1305).
- Derive separate enc/MAC keys with HKDF instead of reusing one key.
- Version the on-disk/on-wire format byte so old data can be migrated.
Happy to put up a PR with a versioned Encryption v2 + a migration path if that's wanted. The main care-point is the persisted formats (KeyStorage / PersistenceManager) so existing installs keep opening.
Was reading through
common/crypto/Encryption.javaand noticed the symmetric cipher is just:Cipher.getInstance("AES")resolves to the JCE default, which isAES/ECB/PKCS5Padding. So everything that goes throughencrypt()/encryptPayloadWithHmac()is ECB: no IV, deterministic, and identical 16-byte plaintext blocks produce identical ciphertext blocks. That's a problem because this path isn't just used for ephemeral message keys — it also encrypts longer-lived stuff: the identity keys inKeyStorage, the persisted app data inPersistenceManager,EncryptedConnectionList, and the payment-account payload exchanged during a trade. Structured protobuf records under a long-lived key are pretty much the worst case for ECB pattern leakage.The other thing in the same code:
encryptPayloadWithHmac()uses the sameSecretKeyfor the AES cipher and for the HMAC. Reusing one key across two primitives breaks key separation. It's not catastrophic with HMAC-SHA256 + AES specifically, but there's no reason to do it.Suggested fix:
AES/GCM/NoPaddingwith a fresh 96-bit nonce per message (or ChaCha20-Poly1305).Happy to put up a PR with a versioned
Encryptionv2 + a migration path if that's wanted. The main care-point is the persisted formats (KeyStorage / PersistenceManager) so existing installs keep opening.