no-engine: fix signing with legacy app method based keys #22163

mspncp · 2023-09-21T15:39:44Z

Signing with an app method based key (i.e. an EVP_PKEY which wraps an RSA key with an application defined RSA_METHOD) used to work in 1.1.1. That feature was broken in commit 60488d2, but later on fixed by @t8m in commit b247113 (see #14859).

This commit corrects a minor flaw of the fix, which affects only no-engine builds: the special treatment for foreign keys is guarded by an OPENSSL_NO_ENGINE check.

Checklist

documentation is added or updated
tests are added or updated

mspncp · 2023-09-21T15:44:31Z

Some background information

After switching from 1.1.1 to 3.0, our TLS server application failed with a tls handshake failure (alert 40). It turned out that the server correctly attempted to select the cipher suite ECDHE-RSA-AES256-GCM-SHA384, but the check_cert_usable test failed with an EVP_R_UNSUPPORTED_ALGORITHM for mysterious reasons:

#0  ERR_new () at crypto/err/err_blocks.c:20
#1  0x00007ffff74d8cdc in int_ctx_new (libctx=0x0, pkey=0x555555637330, e=0x0, keytype=0x0, propquery=0x0, id=6) at crypto/evp/pmeth_lib.c:316
#2  0x00007ffff74d8f43 in EVP_PKEY_CTX_new_from_pkey (libctx=0x0, pkey=0x555555637330, propquery=0x0) at crypto/evp/pmeth_lib.c:374
#3  0x00007ffff74caf15 in do_sigver_init (ctx=0x7fffe802adb0, pctx=0x0, type=0x0, mdname=0x7ffff773d78f "SHA256", libctx=0x0, props=0x0, e=0x0, pkey=0x555555637330, ver=0, params=0x0) at crypto/evp/m_sigver.c:60
#4  0x00007ffff74cbd10 in EVP_DigestSignInit_ex (ctx=0x7fffe802adb0, pctx=0x0, mdname=0x7ffff773d78f "SHA256", libctx=0x0, props=0x0, pkey=0x555555637330, params=0x0) at crypto/evp/m_sigver.c:372
#5  0x00007ffff74d33fd in EVP_PKEY_digestsign_supports_digest (pkey=0x555555637330, libctx=0x0, name=0x7ffff773d78f "SHA256", propq=0x0) at crypto/evp/p_lib.c:1369
#6  0x00007ffff721eff2 in check_cert_usable (s=0x7fffe8000d90, sig=0x555555606498, x=0x5555556361a0, pkey=0x555555637330) at ssl/t1_lib.c:3102
#7  0x00007ffff721f185 in has_usable_cert (s=0x7fffe8000d90, sig=0x555555606498, idx=0) at ssl/t1_lib.c:3154
#8  0x00007ffff721f6c4 in tls_choose_sigalg (s=0x7fffe8000d90, fatalerrs=1) at ssl/t1_lib.c:3291
#9  0x00007ffff7264c54 in tls_post_process_client_hello (s=0x7fffe8000d90, wst=WORK_MORE_B) at ssl/statem/statem_srvr.c:2227
#10 0x00007ffff7261e47 in ossl_statem_server_post_process_message (s=0x7fffe8000d90, wst=WORK_MORE_A) at ssl/statem/statem_srvr.c:1236
#11 0x00007ffff724ab2f in read_state_machine (s=0x7fffe8000d90) at ssl/statem/statem.c:675
#12 0x00007ffff724a3c5 in state_machine (s=0x7fffe8000d90, server=1) at ssl/statem/statem.c:442
#13 0x00007ffff7249e4b in ossl_statem_accept (s=0x7fffe8000d90) at ssl/statem/statem.c:270
#14 0x00007ffff7208ea5 in SSL_do_handshake (s=0x7fffe8000d90) at ssl/ssl_lib.c:3938
#15 0x00007ffff72040db in SSL_accept (s=0x7fffe8000d90) at ssl/ssl_lib.c:1750
#16 0x00007ffff7ec850b in TcpSrvSession::openSession (this=0x7ffff0000bb0) at ncp/source/ncpinet/src/tcpsrv.cpp:271
#17 0x00007ffff7ec86ee in TcpSrvSession::work (this=0x7ffff0000bb0) at ncp/source/ncpinet/src/tcpsrv.cpp:314
#18 0x00007ffff7ec83ea in TcpSrvSession::init (aData=0x7ffff0000bb0) at ncp/source/ncpinet/src/tcpsrv.cpp:241
#19 0x00007ffff6e8dea7 in start_thread (arg=<optimized out>) at pthread_create.c:477
#20 0x00007ffff6dada2f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

After some debugging I found out that the problem is caused by the problem which is fixed by this pull request.

Regression Test

@t8m I considered adding a regression test for this. I found the test_redirect test case, and adding a few extra lines for the app method based keys would be the natural place where I would put it.

Unfortunately, none of the engine tests is executed in the no-engine case. If you believe that such a test would be valuable, I'd suggest to move the test_redirect test case from the engine test suite into the evp test suite (evp_test, evp_extra_test?) and add the necessary OPENSSL_NO_ENGINE guards.

mspncp · 2023-09-21T15:55:18Z

This commit cherry-picks to 3.1 and 3.0 without conflicts.

crypto/evp/pmeth_lib.c

@t8m

Signing with an app method based key (i.e. an `EVP_PKEY` which wraps an `RSA` key with an application defined `RSA_METHOD`) used to work in 1.1.1. That feature was broken in commit 60488d2, but later on fixed by @t8m in commit b247113 (see openssl#14859). This commit corrects a minor flaw of the fix, which affects only `no-engine` builds: the special treatment for foreign keys is guarded by an `OPENSSL_NO_ENGINE` check.

beldmit

I'd like to have a test for that but not immediately.

LGTM

openssl-machine · 2023-09-22T19:00:17Z

This pull request is ready to merge

@t8m

Signing with an app method based key (i.e. an `EVP_PKEY` which wraps an `RSA` key with an application defined `RSA_METHOD`) used to work in 1.1.1. That feature was broken in commit 60488d2, but later on fixed by @t8m in commit b247113 (see #14859). This commit corrects a minor flaw of the fix, which affects only `no-engine` builds: the special treatment for foreign keys is guarded by an `OPENSSL_NO_ENGINE` check. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Todd Short <todd.short@me.com> (Merged from #22163)

@t8m

Signing with an app method based key (i.e. an `EVP_PKEY` which wraps an `RSA` key with an application defined `RSA_METHOD`) used to work in 1.1.1. That feature was broken in commit 60488d2, but later on fixed by @t8m in commit b247113 (see #14859). This commit corrects a minor flaw of the fix, which affects only `no-engine` builds: the special treatment for foreign keys is guarded by an `OPENSSL_NO_ENGINE` check. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Todd Short <todd.short@me.com> (Merged from #22163) (cherry picked from commit 1acc3e8)

tmshort · 2023-09-22T19:51:32Z

Merged to master, openssl-3.1 and openssl-3.0. Thank you!

@t8m

Signing with an app method based key (i.e. an `EVP_PKEY` which wraps an `RSA` key with an application defined `RSA_METHOD`) used to work in 1.1.1. That feature was broken in commit 60488d2, but later on fixed by @t8m in commit b247113 (see #14859). This commit corrects a minor flaw of the fix, which affects only `no-engine` builds: the special treatment for foreign keys is guarded by an `OPENSSL_NO_ENGINE` check. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Todd Short <todd.short@me.com> (Merged from #22163) (cherry picked from commit 1acc3e8) (cherry picked from commit c67a198)

@t8m

Signing with an app method based key (i.e. an `EVP_PKEY` which wraps an `RSA` key with an application defined `RSA_METHOD`) used to work in 1.1.1. That feature was broken in commit 60488d2, but later on fixed by @t8m in commit b247113 (see #14859). This commit corrects a minor flaw of the fix, which affects only `no-engine` builds: the special treatment for foreign keys is guarded by an `OPENSSL_NO_ENGINE` check. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Todd Short <todd.short@me.com> (Merged from openssl/openssl#22163) (cherry picked from commit 1acc3e8) (cherry picked from commit c67a1988fcf8fe34b1d31e29849f2528d553dd66) Signed-off-by: fly2x <fly2x@hitls.org>

@t8m

Signing with an app method based key (i.e. an `EVP_PKEY` which wraps an `RSA` key with an application defined `RSA_METHOD`) used to work in 1.1.1. That feature was broken in commit 60488d2, but later on fixed by @t8m in commit b247113 (see #14859). This commit corrects a minor flaw of the fix, which affects only `no-engine` builds: the special treatment for foreign keys is guarded by an `OPENSSL_NO_ENGINE` check. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Todd Short <todd.short@me.com> (Merged from openssl/openssl#22163) Signed-off-by: fly2x <fly2x@hitls.org>

@t8m

Signing with an app method based key (i.e. an `EVP_PKEY` which wraps an `RSA` key with an application defined `RSA_METHOD`) used to work in 1.1.1. That feature was broken in commit 60488d2, but later on fixed by @t8m in commit b247113 (see #14859). This commit corrects a minor flaw of the fix, which affects only `no-engine` builds: the special treatment for foreign keys is guarded by an `OPENSSL_NO_ENGINE` check. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Todd Short <todd.short@me.com> (Merged from openssl/openssl#22163) (cherry picked from commit 1acc3e8) Signed-off-by: fly2x <fly2x@hitls.org>

This commit adds `test_EVP_PKEY_sign_with_app_method`, a regression test for the bug fix in commit 1acc3e8 (pull request openssl#22163). It is analogous to `test_EVP_PKEY_sign`, only with a fake app method based key. (The EC key test case was omitted, because there is no `EC_KEY_METHOD_dup` method.)

This commit adds `test_EVP_PKEY_sign_with_app_method`, a regression test for the bug fix in commit 1acc3e8 (pull request #22163). It is analogous to `test_EVP_PKEY_sign`, only with a fake app method based key. (The EC key test case was omitted, because there is no `EC_KEY_METHOD_dup` method.) Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #22185)

This commit adds `test_EVP_PKEY_sign_with_app_method`, a regression test for the bug fix in commit 1acc3e8 (pull request #22163). It is analogous to `test_EVP_PKEY_sign`, only with a fake app method based key. (The EC key test case was omitted, because there is no `EC_KEY_METHOD_dup` method.) Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #22185) (cherry picked from commit 860e36d)

mspncp added branch: master Merge to master branch triaged: bug The issue/pr is/fixes a bug branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 labels Sep 21, 2023

mspncp requested a review from t8m September 21, 2023 15:39

mspncp added the severity: regression The issue/pr is a regression from previous released version label Sep 21, 2023

mattcaswell requested changes Sep 21, 2023

View reviewed changes

crypto/evp/pmeth_lib.c Show resolved Hide resolved

mspncp force-pushed the fix-legacy-app-method-keys branch from 0c01497 to 6de9e91 Compare September 21, 2023 16:33

mspncp requested a review from mattcaswell September 21, 2023 16:36

mattcaswell approved these changes Sep 21, 2023

View reviewed changes

mattcaswell added the approval: review pending This pull request needs review by a committer label Sep 21, 2023

t8m approved these changes Sep 21, 2023

View reviewed changes

t8m added approval: done This pull request has the required number of approvals tests: exempted The PR is exempt from requirements for testing and removed approval: review pending This pull request needs review by a committer labels Sep 21, 2023

beldmit approved these changes Sep 21, 2023

View reviewed changes

openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Sep 22, 2023

tmshort self-assigned this Sep 22, 2023

tmshort closed this Sep 22, 2023

mspncp deleted the fix-legacy-app-method-keys branch September 23, 2023 20:57

mspncp mentioned this pull request Sep 25, 2023

test: evp_extra: test signing with legacy app method based keys #22185

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

no-engine: fix signing with legacy app method based keys #22163

no-engine: fix signing with legacy app method based keys #22163

mspncp commented Sep 21, 2023

mspncp commented Sep 21, 2023 •

edited

mspncp commented Sep 21, 2023

beldmit left a comment

openssl-machine commented Sep 22, 2023

tmshort commented Sep 22, 2023

no-engine: fix signing with legacy app method based keys #22163

no-engine: fix signing with legacy app method based keys #22163

Conversation

mspncp commented Sep 21, 2023

Checklist

mspncp commented Sep 21, 2023 • edited

Some background information

Regression Test

mspncp commented Sep 21, 2023

beldmit left a comment

Choose a reason for hiding this comment

openssl-machine commented Sep 22, 2023

tmshort commented Sep 22, 2023

mspncp commented Sep 21, 2023 •

edited