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

EVP_MD lost 'type' filed if built with no-autoalginit #20221

Open
liyi77 opened this issue Feb 6, 2023 · 13 comments
Open

EVP_MD lost 'type' filed if built with no-autoalginit #20221

liyi77 opened this issue Feb 6, 2023 · 13 comments
Labels
branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 branch: 3.2 Merge to openssl-3.2 triaged: bug The issue/pr is/fixes a bug

Comments

@liyi77
Copy link
Contributor

liyi77 commented Feb 6, 2023

Hi All,

To reduce the size of the generated binary as possible, I tried to build openssl with 'no-autoalginit' and manually added the required digest.

The strange thing is that the value of 'type' will be lost when using EVP_MD_fetch() to retrieve EVP_MD after EVP_add_digest(). Automatic initialization algorithm (without 'no-autoalginit') does not have this problem.

Any hints about it? More details and configure as below, thanks.

  1. build with 'no-autoalginit' and:
    EVP_add_digest (EVP_sha256 ());
    TestMD = EVP_MD_fetch (NULL, "sha256", NULL);
    image

  2. without 'no-autoalginit':
    image

  3. Whole configure list:
    "no-afalgeng",
    "no-aria",
    "no-async",
    "no-autoalginit",
    "no-autoerrinit",
    "no-autoload-config",
    "no-bf",
    "no-blake2",
    "no-camellia",
    "no-capieng",
    "no-cast",
    "no-chacha",
    "no-cmac",
    "no-cmp",
    "no-cms",
    "no-ct",
    "no-deprecated",
    "no-des",
    "no-dgram",
    "no-dsa",
    "no-dso",
    "no-dynamic-engine",
    "no-ec2m",
    "no-engine",
    "no-err",
    "no-filenames",
    "no-gost",
    "no-hw",
    "no-idea",
    "no-makedepend",
    "no-module",
    "no-md4",
    "no-mdc2",
    "no-pic",
    "no-ocb",
    "no-ocsp",
    "no-padlockeng",
    "no-poly1305",
    "no-posix-io",
    "no-rc2",
    "no-rc4",
    "no-rc5",
    "no-rfc3779",
    "no-rmd160",
    "no-scrypt",
    "no-seed",
    "no-shared",
    "no-siphash",
    "no-siv",
    "no-sm4",
    "no-sock",
    "no-srp",
    "no-srtp",
    "no-sse2",
    "no-ssl",
    "no-ssl3-method",
    "no-ssl-trace",
    "no-static-engine",
    "no-stdio",
    "no-threads",
    "no-ts",
    "no-ui",
    "no-whirlpool",
    "disable-legacy",
    # OpenSSL1_1_1b doesn't support default rand-seed-os for UEFI
    # UEFI only support --with-rand-seed=none
    "--with-rand-seed=none",
    "--api=1.1.1"

@liyi77 liyi77 added the issue: bug report The issue was opened to report a bug label Feb 6, 2023
@paulidale
Copy link
Contributor

paulidale commented Feb 6, 2023

Without automatically loading algorithms, the types will be allocated starting from zero instead of being pre-allocated. This is a function of how providers operate -- new algorithms get integer indices starting from zero.

I can't imagine that it will cause problems, but I've never tried it.

@paulidale paulidale added triaged: question The issue contains a question resolved: answered The issue contained a question which has been answered and removed issue: bug report The issue was opened to report a bug labels Feb 6, 2023
@liyi77
Copy link
Contributor Author

liyi77 commented Feb 6, 2023

Without automatically loading algorithms, the types will be allocated starting from zero instead of being pre-allocated. This is a function of how providers operate -- new algorithms get integer indices starting from zero.

I can't imagine that it will cause problems, but I've never tried it.

Hi, thanks for quick feedback.

This problem causes PKCS7_sign() to fail because this function uses EVP_MD_fetch() to get EVP_MD* and then retrieves EVP_MD* based on 'type' filed, I guess this filed should be NID and be fixed?

  1. fetch call stack:
    EVP_MD_fetch(ossl_lib_ctx_st * ctx, const char * algorithm, const char * properties) Line 1071 (openssl\crypto\evp\digest.c:1071)
    pkcs7_bio_add_digest(bio_st * * pbio, X509_algor_st * alg, const PKCS7_CTX_st * ctx) Line 73 (openssl\crypto\pkcs7\pk7_doit.c:73)
    PKCS7_dataInit(pkcs7_st * p7, bio_st * bio) Line 282 (openssl\crypto\pkcs7\pk7_doit.c:282)
    PKCS7_final(pkcs7_st * p7, bio_st * data, int flags) Line 79 (openssl\crypto\pkcs7\pk7_smime.c:79)
    PKCS7_sign_ex(x509_st * signcert, evp_pkey_st * pkey, stack_st_X509 * certs, bio_st * data, int flags, ossl_lib_ctx_st * libctx, const char * propq) Line 59 (openssl\crypto\pkcs7\pk7_smime.c:59)
    PKCS7_sign(x509_st * signcert, evp_pkey_st * pkey, stack_st_X509 * certs, bio_st * data, int flags) Line 71

  2. find call stack:
    EVP_MD_get_type(const evp_md_st * mVd) Line 786 (openssl\crypto\evp\evp_lib.c:786)
    PKCS7_find_digest(evp_md_ctx_st * * pmd, bio_st * bio, int nid) Line 682 (openssl\crypto\pkcs7\pk7_doit.c:682)
    PKCS7_dataFinal(pkcs7_st * p7, bio_st * bio) Line 817 (openssl\crypto\pkcs7\pk7_doit.c:817)
    PKCS7_final(pkcs7_st * p7, bio_st * data, int flags) Line 89 (openssl\crypto\pkcs7\pk7_smime.c:89)
    PKCS7_sign_ex(x509_st * signcert, evp_pkey_st * pkey, stack_st_X509 * certs, bio_st * data, int flags, ossl_lib_ctx_st * libctx, const char * propq) Line 59 (openssl\crypto\pkcs7\pk7_smime.c:59)
    PKCS7_sign(x509_st * signcert, evp_pkey_st * pkey, stack_st_X509 * certs, bio_st * data, int flags) Line 71 (openssl\crypto\pkcs7\pk7_smime.c:71)

@paulidale paulidale added triaged: bug The issue/pr is/fixes a bug and removed triaged: question The issue contains a question resolved: answered The issue contained a question which has been answered labels Feb 6, 2023
@paulidale
Copy link
Contributor

Yeah, that would be a bug. I'm not sure how to support no-autoalginit. The name/NID mappings won't exist without initing the algorithsm and without that no lookup by NID or OID will work.

liyi77 added a commit to liyi77/openssl that referenced this issue Apr 19, 2023
FIX: openssl#20221

The value of 'type' will be lost if using EVP_MD_fetch() to retrieve
EVP_MD when building with OPENSSL_NO_AUTOALGINIT.
Manually add it back here.

Signed-off-by: Yi Li <yi1.li@intel.com>
liyi77 added a commit to liyi77/openssl that referenced this issue Apr 20, 2023
FIX: openssl#20221

The value of 'type' will be lost if using EVP_MD_fetch() to retrieve
EVP_MD when building with OPENSSL_NO_AUTOALGINIT.
Manually add it back here.

Signed-off-by: Yi Li <yi1.li@intel.com>
@levitte
Copy link
Member

levitte commented Apr 20, 2023

The earlier answers aren't quite right.

Fetched algorithms aren't guaranteed to have an associated OBJ or NID, because provider based implementation aren't designed for it. We do place the NID into the type field if there is a registered legacy implementation as well.

Unfortunately, there are a few subsystems that weren't fully upgraded to deal with a provider world. PKCS#7 and CMS are among those, there's definitely a bit of work to do there.

@paulidale
Copy link
Contributor

@levitte, create an issue for this perhaps?

@levitte
Copy link
Member

levitte commented Apr 20, 2023

There is a collection of issues to raise, and that'll be quite a write-up. I actually thought I already had raised this before we released 3.0.0... but maybe that got lost.

@liyi77
Copy link
Contributor Author

liyi77 commented Apr 20, 2023

There is a collection of issues to raise, and that'll be quite a write-up. I actually thought I already had raised this before we released 3.0.0... but maybe that got lost.

It would be nice to know the root cause. Thanks, I closed my PR and look forward to your issues.

@t8m t8m changed the title EVP_MD lost 'type' filed if manually add digest EVP_MD lost 'type' filed if built with no-autoalginit Apr 20, 2023
@liyi77
Copy link
Contributor Author

liyi77 commented May 16, 2023

There is a collection of issues to raise, and that'll be quite a write-up. I actually thought I already had raised this before we released 3.0.0... but maybe that got lost.

Hi @levitte , any update here, do we have new issue about this bug?

@levitte
Copy link
Member

levitte commented May 16, 2023

Sorry for the delay, I've had a few other and higher priority things on my table, and they still preoccupy me. But, I do plan on taking on the bigger issue (lack of provider support in some sections of libcrypto) next (that's still a week or so away, mind you)

@liyi77
Copy link
Contributor Author

liyi77 commented May 16, 2023

Sorry for the delay, I've had a few other and higher priority things on my table, and they still preoccupy me. But, I do plan on taking on the bigger issue (lack of provider support in some sections of libcrypto) next (that's still a week or so away, mind you)

Oh i see, thank you!

@ereisch
Copy link

ereisch commented May 31, 2023

This bug impacts PEM_write_bio_PrivateKey as well if the enc field has a non-NULL algorithm defined.

PKCS5_pbe2_set_iv_ex():
if (alg_nid == NID_undef) { ...

@liyi77
Copy link
Contributor Author

liyi77 commented Jun 7, 2023

++visibility
Hello, any update here?

@liyi77
Copy link
Contributor Author

liyi77 commented Aug 2, 2023

This issue impacts EDK2 http boot (based on openssl TLS/SSL implementation) as well, but I don't have root cause it yet..

@t8m t8m added branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 labels Oct 9, 2023
@t8m t8m added the branch: 3.2 Merge to openssl-3.2 label Oct 26, 2023
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 branch: 3.1 Merge to openssl-3.1 branch: 3.2 Merge to openssl-3.2 triaged: bug The issue/pr is/fixes a bug
Projects
None yet
5 participants