Skip to content
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

Silent Failure in OpenSSL leads to use of NULL Cipher #18970

Closed
rapier1 opened this issue Aug 9, 2022 · 5 comments
Closed

Silent Failure in OpenSSL leads to use of NULL Cipher #18970

rapier1 opened this issue Aug 9, 2022 · 5 comments
Labels
triaged: question The issue contains a question

Comments

@rapier1
Copy link

rapier1 commented Aug 9, 2022

Hello,

I know this is a corner case and one that has been warned about but I thought it would be good to report this anyway.

Under 1.1.1 I was using a custom EVP so that we could implement a multithreaded AES_CTR in our code (hpnssh, a variant of openssh). This was defined as follows:

const EVP_CIPHER * evp_aes_ctr_mt(void) { static EVP_CIPHER *aes_ctr; aes_ctr = EVP_CIPHER_meth_new(NID_undef, 16/*block*/, 16/*key*/); EVP_CIPHER_meth_set_iv_length(aes_ctr, AES_BLOCK_SIZE); EVP_CIPHER_meth_set_init(aes_ctr, ssh_aes_ctr_init); EVP_CIPHER_meth_set_cleanup(aes_ctr, ssh_aes_ctr_cleanup); EVP_CIPHER_meth_set_do_cipher(aes_ctr, ssh_aes_ctr); EVP_CIPHER_meth_set_flags(aes_ctr, EVP_CIPH_CBC_MODE | EVP_CIPH_VARIABLE_LENGTH | EVP_CIPH_ALWAYS_CALL_INIT | EVP_CIPH_CUSTOM_IV); return (aes_ctr); }

This compiles with deprecated warnings under OpenSSL 3 but it does compile. However, interesting things happen when we try to use it. I am not entirely sure but what believe what is happening is that when we try to init this type (evp_aes_ctr_mt) the OpenSSL code looks for a provider for this, can't find one, and possibly returns NULL. This doesn't cause EVP_CipherInit to fail though. Instead it returns 1 and it looks like everything is working. Except that when we move data across this cipher the result is that everything is en clear. I confirmed this via a tcpdump between a server and client both built against OpenSSL 3.0.2 and was able to observe all of the data being transmitted across the wire unencrypted.

Additionally, a connection between a 1.1.1 and 3.0.2 did not work as each side saw the other as being corrupted. Which is what you'd expect if the above is true.

I know that using any of the meth_ functions in OpenSSL 3 is "being deprecated in OpenSSL 3.0, and users of these APIs should know that their use can likely bypass provider selection and configuration, with unintended consequences." However, this seems like a severe unintended consequence.

In any event, I'm not expecting a bug fix but I wanted to let you know in case you needed to know what some of these unintended consequences look like.

@rapier1 rapier1 added the issue: bug report The issue was opened to report a bug label Aug 9, 2022
@mattcaswell
Copy link
Member

This doesn't cause EVP_CipherInit to fail though. Instead it returns 0 and it looks like everything is working.

But a 0 return from EVP_CipherInit is a failure? It should return 1 for success?

@mattcaswell mattcaswell added triaged: question The issue contains a question and removed issue: bug report The issue was opened to report a bug labels Aug 10, 2022
@rapier1
Copy link
Author

rapier1 commented Aug 10, 2022

Apologies for the mistake. It returns 1. I'm working with the OpenSSH code and it gets a little twisty at times. I updated the original question. Anyway, for example,

if (EVP_CipherInit(cc->evp, type, NULL, (u_char *)iv, (do_encrypt == CIPHER_ENCRYPT)) == 0) { ret = SSH_ERR_LIBCRYPTO_ERROR; goto out; }

Does not fail in this circumstance even though 'type' (evp_aes_ctr_mt) seems like it would be unknown under OpenSSL3 as there is no provider for it. Also, it's my assumption the problem is happening in CipherInit rather than elsewhere.

mattcaswell added a commit to mattcaswell/openssl that referenced this issue Sep 29, 2022
If a custom EVP_CIPHER object has been passed to EVP_CipherInit() then it
should be used in preference to a fetched cipher.

We also fix a possible NULL pointer deref in the same code for digests.

If the custom cipher passed to EVP_CipherInit() happens to use NID_undef
(which should be a discouraged practice), then in the previous
implementation this could result in the NULL cipher being fetched and
hence NULL encryption being unexpectedly used.

CVE-2022-3358

Fixes openssl#18970
@mattcaswell
Copy link
Member

mattcaswell commented Sep 29, 2022

This is confirmed as a bug. The fix is in #19300. It has also been assessed as a low severity security issue (CVE-2022-3358). Since it is low severity we are fixing this in public as per our security policy.

The bug here is that the custom cipher is not being properly handled and instead of just using it, we try to fetch an equivalent cipher from a provider. Unfortunately the problem is exacerbated by the use of NID_undef for the cipher NID when you call EVP_CIPHER_meth_new. This is probably a bad idea (you should really use a "proper" NID for this). Unfortunately when we try to find an equivalent cipher to fetch we find the NID_undef which corresponds with the NULL cipher, and that is what ends up being used.

@rapier1
Copy link
Author

rapier1 commented Sep 29, 2022

Matt,

Thanks for the update. Also, thanks again for your help with the providers issue.
Chris

openssl-machine pushed a commit that referenced this issue Oct 3, 2022
If a custom EVP_CIPHER object has been passed to EVP_CipherInit() then it
should be used in preference to a fetched cipher.

We also fix a possible NULL pointer deref in the same code for digests.

If the custom cipher passed to EVP_CipherInit() happens to use NID_undef
(which should be a discouraged practice), then in the previous
implementation this could result in the NULL cipher being fetched and
hence NULL encryption being unexpectedly used.

CVE-2022-3358

Fixes #18970

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #19300)

(cherry picked from commit 25d47cc)
@mattcaswell
Copy link
Member

@rapier1...please can you drop me an email regarding this on matt@openssl.org? I don't seem to have your email address.

@openssl openssl deleted a comment Oct 8, 2022
beldmit pushed a commit to beldmit/openssl that referenced this issue Dec 26, 2022
If a custom EVP_CIPHER object has been passed to EVP_CipherInit() then it
should be used in preference to a fetched cipher.

We also fix a possible NULL pointer deref in the same code for digests.

If the custom cipher passed to EVP_CipherInit() happens to use NID_undef
(which should be a discouraged practice), then in the previous
implementation this could result in the NULL cipher being fetched and
hence NULL encryption being unexpectedly used.

CVE-2022-3358

Fixes openssl#18970

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#19300)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triaged: question The issue contains a question
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants