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

Using EVP_md_null results in crash in OpenSSL 3 #16660

Closed
kaniini opened this issue Sep 22, 2021 · 9 comments
Closed

Using EVP_md_null results in crash in OpenSSL 3 #16660

kaniini opened this issue Sep 22, 2021 · 9 comments
Assignees
Labels
branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch triaged: bug The issue/pr is/fixes a bug

Comments

@kaniini
Copy link

kaniini commented Sep 22, 2021

Consider the following code:

#include <openssl/evp.h>

int main(int argc, const char *argv[]) {
        OpenSSL_add_all_algorithms();

        EVP_MD_CTX *mdctx = EVP_MD_CTX_new();
        const EVP_MD *mdnull = EVP_md_null();

        EVP_DigestInit_ex(mdctx, mdnull, NULL);
        EVP_DigestUpdate(mdctx, "test", 4);

        unsigned char md_value[EVP_MAX_MD_SIZE];
        int md_len;
        EVP_DigestFinal_ex(mdctx, md_value, &md_len);
        EVP_MD_CTX_free(mdctx);

        return 0;
}

When run under OpenSSL 1.1, this code will finish successfully. Under OpenSSL 3, it crashes.

The reason why is because the null EVP_MD object has atypical behavior for a legacy module, so EVP_DigestInit_ex exits and returns 0 at crypto/evp/digest.c:233, because EVP_MD_fetch does not support md_null.

One option might be to make EVP_MD_fetch return md_null, when the NID_undef is passed in. Another option might be to update md_null to the new API, making it no longer a "legacy" module.

I'm willing to investigate doing either, but would like some guidance first.

@kaniini kaniini added the issue: bug report The issue was opened to report a bug label Sep 22, 2021
@kaniini
Copy link
Author

kaniini commented Sep 22, 2021

I suspected a possible workaround might be to use

EVP_MD_CTX_set_flags(mdctx, EVP_MD_CTX_FLAG_NO_INIT);

to allow the MD_CTX to be properly initialized.

While this allows for EVP_DigestInit_ex to succeed with ctx->digest being set correctly, it still does not result in ctx->update being set to ctx->digest->update. This is because the code path at crypto/evp/digest.c:306 does not get run as ctx->digest does equal type there, so ctx->update is never set.

@paulidale paulidale added branch: 3.0 Merge to openssl-3.0 branch triaged: bug The issue/pr is/fixes a bug and removed issue: bug report The issue was opened to report a bug labels Sep 22, 2021
@t8m t8m added the branch: master Merge to master branch label Sep 23, 2021
@paulidale
Copy link
Contributor

I think things are more complicated. NID_undef is being used for two purposes which are conflicting. It means no algorithm but here it also means null algorithm.

I don't think we should allocate the null algorithm it's own NID because this will likely cause all manner of issues.

@levitte any ideas?

@kaniini
Copy link
Author

kaniini commented Sep 25, 2021

Yeah, I'm not quite sure what is going on with all of that code. I stared at it for hours, and concluded that it would probably be best if OpenSSL upstream figured this one out.

However, this is breaking a few real-world applications, like apk-tools, which sometimes uses the null algorithm.

@mattcaswell
Copy link
Member

Using NID_undef for md_null was probably a mistake in <=1.1.1.

However there is clearly a discrepancy here. EVP_enc_null is much the same concept but for ciphers (also using NID_undef). However there is an implementation for EVP_enc_null in the providers. It seems strange that we should have one for ciphers but not for digests.

We have some "special" handling for EVP_enc_null in the fetch:

EVP_CIPHER *provciph =
EVP_CIPHER_fetch(NULL,
cipher->nid == NID_undef ? "NULL"
: OBJ_nid2sn(cipher->nid),
"");

@mspncp
Copy link
Contributor

mspncp commented Nov 9, 2021

@kaniini I stumbled over your post on alpinelinux.org and I'm sorry to hear that the stalled state of this issue seems to create some frustration in the alpine Linux community. Mind telling us why you did not just ping us here and tell us about it?

cc @openssl/otc

@t8m t8m assigned t8m and unassigned levitte Nov 11, 2021
t8m added a commit to t8m/openssl that referenced this issue Nov 12, 2021
This is necessary to keep compatibility with 1.1.1.

Fixes openssl#16660
@t8m
Copy link
Member

t8m commented Nov 12, 2021

Fix in #17016

@mspncp
Copy link
Contributor

mspncp commented Nov 12, 2021

@kaniini
Copy link
Author

kaniini commented Nov 12, 2021

Mind telling us why you did not just ping us here and tell us about it?

We already had a workaround for apk-tools, e.g. just not using EVP_md_null on OpenSSL 3, so this specific regression was no longer our primary concern. However, it seems to me that fixing a crash regression in a non-deprecated OpenSSL 1.1 API should be high priority.

On top of that, we were quite busy reverting back to OpenSSL 1.1 due to other regressions caused by the providers modularization (this bug, also programs expecting the legacy provider to be available but not loading it themselves) and lack of downstream preparedness. No amount of pinging OpenSSL developers would resolve the fact that MariaDB is not ready.

As the Alpine OpenSSL 3 transition was something I owned responsibility for, I had to explain why it got aborted to the TSC regardless, as that is our policy whenever a contingency plan is triggered. As for the frustration, it is not just this issue, but all of the issues I outlined in my report. Given that the OpenSSL 3 migration had an outcome where our contingency plan came into effect, I believe it to be the most prudent course of action to evaluate all possible options before committing to trying the OpenSSL 3 migration again, such an evaluation would be required by the TSC anyway.

@kroeckx
Copy link
Member

kroeckx commented Nov 12, 2021 via email

openssl-machine pushed a commit that referenced this issue Nov 15, 2021
This is necessary to keep compatibility with 1.1.1.

Fixes #16660

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #17016)

(cherry picked from commit bef9b48)
ramikhaldi pushed a commit to ramikhaldi/openssl that referenced this issue Nov 21, 2021
This is necessary to keep compatibility with 1.1.1.

Fixes openssl#16660

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl#17016)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants