-
Notifications
You must be signed in to change notification settings - Fork 3.1k
crypto: Improve AEAD cipher detection logic in cipher_kt_mode_aead()
#774
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Modified cipher_kt_mode_aead() to: 1. Use the same detection approach as cipher_ctx_mode_aead() by checking EVP_CIPH_FLAG_AEAD_CIPHER flag instead of mode comparison 2. Apply identical version handling for ChaCha20-Poly1305: - Special case only for OpenSSL < 3.0.0 - Same NID check condition This change makes AEAD detection more robust and future-proof, especially for OpenSSL 3.0+ compatibility.
{ | ||
isaead = true; | ||
} | ||
|
||
#ifdef NID_chacha20_poly1305 | ||
#if defined(NID_chacha20_poly1305) && OPENSSL_VERSION_NUMBER < 0x30000000L |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have you tested that this doesn't break libreSSL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry, but I didn't check this with LibreSSL since I implemented it in a similar way to cipher_ctx_mode_aead
:
bool
cipher_ctx_mode_aead(const cipher_ctx_t *ctx)
{
if (ctx)
{
int flags = EVP_CIPHER_CTX_flags(ctx);
if (flags & EVP_CIPH_FLAG_AEAD_CIPHER)
{
return true;
}
#if defined(NID_chacha20_poly1305) && OPENSSL_VERSION_NUMBER < 0x30000000L
if (EVP_CIPHER_CTX_nid(ctx) == NID_chacha20_poly1305)
{
return true;
}
#endif
}
return false;
}
I hope that if it works with cipher_ctx_mode_aead
then it will work here. I can check it, but a little later.
AES-CCM is also an AEAD mode but do not support that one. It would be good to least to add a unit test that test the common candiates like to see that AES-GCM variatns, Chacha20-Poly1305 are still recognised but AES-CCM isa not a valid cipher for us.
What mode do these actually return instead? |
The failing unit tests also seem to indicate that whitelisting all AEAD algorithm is too way as we would now allow CCM algorithm but utterly fail later as these need different handling than GCM. |
Thank you for your prompt responses regarding the PR — it's really great! :D You're right that there are many implementations of AEAD algorithms, and not all of them are suitable for the default AEAD mode. However, we’d really prefer not to explicitly restrict AEAD modes that don’t require any special handling.
Speaking of the list above, only the OCB mode is explicitly registered in OpenSSL:
The rest of the modes can be defined by the OpenSSL providers themselves and don’t necessarily have to be explicitly registered (for example provided Kuznyechik-MGM). In such cases, we would just like to check whether the algorithm is AEAD or not.
Given the issues with the CCM mode, could we discuss a convenient interface for registering most AEAD algorithms? |
yes but that needs a white-list of modes that we know and have verifed to not require special handling and are safe in the way we are using them. Otherwise we might end up with catastrophic consequences. E.g. if you have an algorithm that allows IV reuse vs one that breaks under IV reuse. New AEAD modes might introduce other requirement that need to be fulfilled in order to be used in a safe way. So allowing AEAD modes needs to be only be done after we checked that they are safe to allow with the code in OpenVPN that does AEAD encryption. E.g. check the OpenVPN RFC and the whole dance we do with epoch keys to not overuse an AES key. Especially with OpenSSL 3.x and providers there is probably a better API than the OpenSSL 1.1.x style API that does not depend on hardcoded constants. And if that API is missing then I think a discussion on the OpenSSL upstream library to allow providers to specify their AEAD mode should be done to get such an API. Allowing all AEAD algorithm is not something I am willing to accept as we already know there are problems with the approach (CCM) and it seems cryptographically speaking also be a bad idea. |
I agree with you — in that case, the only reasonable approach would indeed be to add specific modes individually, and only when there is a clear need. In our case, we only needed support for Kuznyechik-MGM, but it seems that adding it would require a more thorough and careful analysis. I think we can close the PR. Thank you very much for the clarification and for taking the time to explain everything! |
This PR improves AEAD cipher detection in cipher_kt_mode_aead() by replacing the GCM-only mode check with a more reliable flag-based approach using EVP_CIPH_FLAG_AEAD_CIPHER, consistent with cipher_ctx_mode_aead(). It also refines ChaCha20-Poly1305 handling to apply only on OpenSSL versions prior to 3.0.0.
These changes enable support for a broader range of AEAD algorithms—such as AES-OCB, AES-EAX, Kuznyechik-MGM, and others—that were previously unsupported due to narrow detection logic. This makes AEAD handling more robust and future-proof, improving compatibility with OpenSSL 3.x and beyond.