cipher: swith from ECB to CBC mode for AES lookups

[xnox]

newEVPCipher function returns AES cipher of any supported modes. It
does so by loading an AES cipher with ECB mode.

Pure stand-alone ECB mode (but not as a primitive in other modes) is
deprecated and will be retired by upcoming NIST SP 800-131A Rev. 3.

Separately geomys module upstream blocks ECB mode completely in FIPS
mode.

I think this is a minimal change, which shouldn't affect any existing
matrix of any providers, as far as I can tell CBC is available
everywhere ECB is.

But this change fixes using OpenSSL FIPS providers that make ECB mode
private.

[sil2100]

Looks good to me.

[qmuntal]

aes.NewCipher returns a ECB cipher, and that function is implemented by newAESCipher, so we can't unilateraly switch to CBC.

I guess you just want to call aes.NewCipher to then pass it tocipher.NewCBCDecrypter, or friends, and then discard the ECB cipher. But with the current implementation, if the provider fail to allocate an ECB cipher, aes.NewCipher errors.

If that's the case, then I would instead update newAESCipher to not allocate an OpenSSL ECBcipher at all, bur defer it until the Encrypt/Decryot is first called (if it ever does). This will also supose a nice perf improvement in the common case of only interested on the CBC cipher.

[xnox]

aes.NewCipher returns a ECB cipher, and that function is implemented by newAESCipher, so we can't unilateraly switch to CBC.

Does it? It seems to return a new struct of kind aes without any mode reference at all.

Would you agree to add mode argument to this API call and make all callers set that?

I guess you just want to call aes.NewCipher to then pass it tocipher.NewCBCDecrypter, or friends, and then discard the ECB cipher. But with the current implementation, if the provider fail to allocate an ECB cipher, aes.NewCipher errors.

If that's the case, then I would instead update newAESCipher to not allocate an OpenSSL ECBcipher at all, bur defer it until the Encrypt/Decryot is first called (if it ever does). This will also supose a nice perf improvement in the common case of only interested on the CBC cipher.

I think all of the above already happens.

[qmuntal]

Forget my previous comment, you are right, we already lazy-init the ECB OpenSSL cipher in Encrypt/Decrypt.

In fact, we only instantiate the ECB cipher in newEVPCipher to cache the block size, and that is not affected by the mode, so this PR is correct.

I'm still don't like the solution, though. We only the the block size in case the caller uses evpCipher.encrypt or evpCipher.decrypt, and that does use ECB mode. Why don't we remove the loadCipher and the EVP_CIPHER_get_block_size call from newEVPCipher, and call EVP_CIPHER_CTX_get_block_size instead in encrypt/decrypt once we have the cipher context? I don't think that calling EVP_CIPHER_CTX_get_block_size every time will have a noticeable perf impact, we can cache it if it does. This way we decouple the AES constructor from any mode, which is less confusing and more future-proof.

[xnox]

Forget my previous comment, you are right, we already lazy-init the ECB OpenSSL cipher in Encrypt/Decrypt.

In fact, we only instantiate the ECB cipher in newEVPCipher to cache the block size, and that is not affected by the mode, so this PR is correct.

ack

I'm still don't like the solution, though. We only the the block size in case the caller uses evpCipher.encrypt or evpCipher.decrypt, and that does use ECB mode. Why don't we remove the loadCipher and the EVP_CIPHER_get_block_size call from newEVPCipher, and call EVP_CIPHER_CTX_get_block_size instead in encrypt/decrypt once we have the cipher context? I don't think that calling EVP_CIPHER_CTX_get_block_size every time will have a noticeable perf impact, we can cache it if it does. This way we decouple the AES constructor from any mode, which is less confusing and more future-proof.

Maybe, but there is a lot of code and tests that expects blockSize to be already set, and asserts on exact errors that happen before that.

This code change adds compatibility with a FIPS module under submission that removes ECB mode as available, whilst being backwards compatible with all other implementations as well.

Also I agree that blocksize checks are miss-placed as it can be 16, 12, 1 => and yet many tests expect it as always 16.

If this passes all CI for all implementation, I would prefer to merge this as is; and then work on redoing things.

[qmuntal]

If this passes all CI for all implementation, I would prefer to merge this as is; and then work on redoing things.

I don't think the refactor is that big. Current code has been working well for years with ECB, except for the case you report. I don't know if by switching to CBC we will hit a corner case elsewhere. So I would rather do the right thing in this same PR.