Tentative fix for the async engine by beldmit · Pull Request #16734 · openssl/openssl

beldmit · 2021-10-03T18:31:50Z

Related to #16724

It fixes the crashes, but I think it's incorrect fix

As a result of the proposed changes, s_server fails with output

Using default temp DH parameters
ACCEPT
ERROR
C0F18AF7FF7F0000:error:02000090:rsa routines:pkey_rsa_ctrl:illegal or unsupported padding mode:crypto/rsa/rsa_pmeth.c:460:
C0F18AF7FF7F0000:error:03000093:digital envelope routines:evp_pkey_ctx_ctrl_int:command not supported:crypto/evp/pmeth_lib.c:1308:
C0F18AF7FF7F0000:error:0A000093:SSL routines:tls_process_cke_rsa:decryption failed:ssl/statem/statem_srvr.c:2911:

BTW, the problem is we've lost the pattern 'call EVP_PKEY_encrypt with NULL out ptr - get enclen - allocate memory and call EVP_PKEY_encrypt again':

openssl/ssl/statem/statem_clnt.c

Lines 2860 to 2870 in 19e277d

    
           pctx = EVP_PKEY_CTX_new_from_pkey(s->ctx->libctx, pkey, s->ctx->propq); 
        
           if (pctx == NULL || EVP_PKEY_encrypt_init(pctx) <= 0 
        
               || EVP_PKEY_encrypt(pctx, NULL, &enclen, pms, pmslen) <= 0) { 
        
               SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_EVP_LIB); 
        
               goto err; 
        
           } 
        
           if (!WPACKET_allocate_bytes(pkt, enclen, &encdata) 
        
                   || EVP_PKEY_encrypt(pctx, encdata, &enclen, pms, pmslen) <= 0) { 
        
               SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_R_BAD_RSA_ENCRYPT); 
        
               goto err; 
        
           }

If we don't change pkey_rsa_encrypt and just fix the dasync engine, we get a SEGFAULT with the following stack trace:

bn2binpad (endianess=big, tolen=256, to=0xff <error: Cannot access memory at address 0xff>, a=0x5b8658) at crypto/bn/bn_lib.c:522
522	            *--to = val;
(gdb) bt
#0  bn2binpad (endianess=big, tolen=256, to=0xff <error: Cannot access memory at address 0xff>, a=0x5b8658) at crypto/bn/bn_lib.c:522
#1  BN_bn2binpad (a=a@entry=0x5b8658, to=to@entry=0x0, tolen=tolen@entry=256) at crypto/bn/bn_lib.c:535
#2  0x00007ffff7d19ad3 in rsa_ossl_public_encrypt (flen=48, from=0x5b7fc0 "\003\001\247\337\335\027\260Ǿ\200\370\066\354.\t\346\322{\271\264\251\341S\272Mh\346&ے\273\316Ԋ\034T\024\260P\270\223[\372-\254\272̑", to=0x0, rsa=0x58e330, 
    padding=<optimized out>) at crypto/rsa/rsa_ossl.c:154
#3  0x00007ffff7d1c3df in pkey_rsa_encrypt (ctx=0x5b84f0, out=0x0, outlen=0x7fffffffd4e8, 
    in=0x5b7fc0 "\003\001\247\337\335\027\260Ǿ\200\370\066\354.\t\346\322{\271\264\251\341S\272Mh\346&ے\273\316Ԋ\034T\024\260P\270\223[\372-\254\272̑", inlen=48) at crypto/rsa/rsa_pmeth.c:337
#4  0x00007ffff7f8aac1 in tls_construct_cke_rsa (pkt=0x7fffffffd550, s=0x59bda0) at ssl/statem/statem_clnt.c:2862
#5  tls_construct_client_key_exchange (s=0x59bda0, pkt=0x7fffffffd550) at ssl/statem/statem_clnt.c:3287
#6  0x00007ffff7f85a35 in write_state_machine (s=0x59bda0) at ssl/statem/statem.c:852
#7  state_machine (s=0x59bda0, server=0) at ssl/statem/statem.c:451
#8  0x00007ffff7f73b64 in ssl3_write_bytes (s=0x59bda0, type=23, buf_=0x4e6270, len=0, written=0x7fffffffd768) at ssl/record/rec_layer_s3.c:403
#9  0x00007ffff7f55465 in SSL_write (s=s@entry=0x59bda0, buf=buf@entry=0x4e6270, num=num@entry=0) at ssl/ssl_lib.c:2123
#10 0x0000000000457b8c in s_client_main (argc=<optimized out>, argv=<optimized out>) at apps/s_client.c:2793
#11 0x0000000000445086 in do_cmd (prog=prog@entry=0x4e4390, argc=argc@entry=8, argv=argv@entry=0x7fffffffde40) at apps/openssl.c:415
#12 0x0000000000422507 in main (argc=8, argv=0x7fffffffde40) at apps/openssl.c:295

mattcaswell · 2021-10-04T10:13:00Z

The "unsupported padding mode" error is actually caused by #16738.

mattcaswell · 2021-10-04T10:15:29Z

we've lost the pattern 'call EVP_PKEY_encrypt with NULL out ptr - get enclen - allocate memory and call EVP_PKEY_encrypt again'

This is because daysnc's custom EVP_PKEY_METHOD does not have the EVP_PKEY_FLAG_AUTOARGLEN flag. Simple fix for that is:

diff --git a/engines/e_dasync.c b/engines/e_dasync.c
index e2e587d839..d7f45a046c 100644
--- a/engines/e_dasync.c
+++ b/engines/e_dasync.c
@@ -211,7 +211,8 @@ static int bind_dasync(ENGINE *e)
     /* Setup RSA */
     ;
     if ((dasync_rsa_orig = EVP_PKEY_meth_find(EVP_PKEY_RSA)) == NULL
-        || (dasync_rsa = EVP_PKEY_meth_new(EVP_PKEY_RSA, 0)) == NULL)
+        || (dasync_rsa = EVP_PKEY_meth_new(EVP_PKEY_RSA,
+                                           EVP_PKEY_FLAG_AUTOARGLEN)) == NULL)
         return 0;
     EVP_PKEY_meth_set_init(dasync_rsa, dasync_rsa_init);
     EVP_PKEY_meth_set_cleanup(dasync_rsa, dasync_rsa_cleanup);

mattcaswell · 2021-10-04T10:17:14Z

It fixes the crashes, but I think it's incorrect fix

Actually I think the fixes in e_dasync.c are fine - although I would extend the same change to all the pkey init functions.

beldmit · 2021-10-04T10:22:01Z

Actually I think the fixes in e_dasync.c are fine - although I would extend the same change to all the pkey init functions.

Will do. Should I revert the rsa_pmeth changes?

mattcaswell · 2021-10-04T10:25:54Z

Will do. Should I revert the rsa_pmeth changes?

Yes. With the suggested EVP_PKEY_FLAG_AUTOARGLEN change it should be unnecessary.

mattcaswell · 2021-10-04T10:28:41Z

While you are making changes to e_dasync.c you might also want to address #16735. See my comments there as to the cause.

Something like this should do it:

 static void destroy_pkey(void)
 {
-    EVP_PKEY_meth_free(dasync_rsa);
+    /*
+     * We don't actually need to free the dasync_rsa method since this is
+     * automatically freed for us by libcrypto.
+     */
     dasync_rsa_orig = NULL;
     dasync_rsa = NULL;
 }

beldmit · 2021-10-04T10:29:41Z

Will do later. Thanks!

levitte · 2021-10-04T12:38:36Z

engines/e_dasync.c

return pparamgen_init != NULL ? pparamgen_init(ctx) : 1;

In all other case too

Fixed. Thanks!

Did you push your fixes? I don't see them?

Oh :(
Sorry

Fixes: openssl#16724 Fixes: openssl#16735

engines/e_dasync.c

openssl-machine · 2021-10-06T08:00:17Z

This pull request is ready to merge

Fixes: #16724 Fixes: #16735 Reviewed-by: Matt Caswell <[email protected]> (Merged from #16734)

Fixes: #16724 Fixes: #16735 Reviewed-by: Matt Caswell <[email protected]> (Merged from #16734) (cherry picked from commit 59cd0bc)

beldmit · 2021-10-06T12:28:02Z

Merged both to master and 3.0. Thanks!

beldmit added branch: 3.0 Applies to openssl-3.0 branch branch: master Applies to master branch triaged: bug The issue/pr is/fixes a bug labels Oct 3, 2021

mattcaswell mentioned this pull request Oct 4, 2021

Legacy RSA EVP_PKEY_METHOD does not support RSA_PKCS1_WITH_TLS_PADDING #16738

Open

beldmit force-pushed the fix_16724 branch from 982f649 to 30758ab Compare October 4, 2021 11:13

beldmit added the approval: review pending This pull request needs review by a committer label Oct 4, 2021

levitte reviewed Oct 4, 2021

View reviewed changes

Fix for the dasync engine

c766c5b

Fixes: openssl#16724 Fixes: openssl#16735

beldmit force-pushed the fix_16724 branch from 30758ab to c766c5b Compare October 4, 2021 15:20

mattcaswell reviewed Oct 4, 2021

View reviewed changes

engines/e_dasync.c Show resolved Hide resolved

mattcaswell approved these changes Oct 5, 2021

View reviewed changes

mattcaswell added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Oct 5, 2021

This was referenced Oct 5, 2021

Fix double-free in e_dasync.c #16751

Closed

[WIP] Test with various crypto engines #16749

Closed

openssl-machine removed the approval: done This pull request has the required number of approvals label Oct 6, 2021

openssl-machine added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Oct 6, 2021

openssl-machine pushed a commit that referenced this pull request Oct 6, 2021

Fix for the dasync engine

59cd0bc

Fixes: #16724 Fixes: #16735 Reviewed-by: Matt Caswell <[email protected]> (Merged from #16734)

openssl-machine pushed a commit that referenced this pull request Oct 6, 2021

Fix for the dasync engine

d62c5d6

Fixes: #16724 Fixes: #16735 Reviewed-by: Matt Caswell <[email protected]> (Merged from #16734) (cherry picked from commit 59cd0bc)

beldmit closed this Oct 6, 2021

beldmit deleted the fix_16724 branch October 6, 2021 12:28

	pctx = EVP_PKEY_CTX_new_from_pkey(s->ctx->libctx, pkey, s->ctx->propq);
	if (pctx == NULL \|\| EVP_PKEY_encrypt_init(pctx) <= 0
	\|\| EVP_PKEY_encrypt(pctx, NULL, &enclen, pms, pmslen) <= 0) {
	SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_EVP_LIB);
	goto err;
	}
	if (!WPACKET_allocate_bytes(pkt, enclen, &encdata)
	\|\| EVP_PKEY_encrypt(pctx, encdata, &enclen, pms, pmslen) <= 0) {
	SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_R_BAD_RSA_ENCRYPT);
	goto err;
	}

Uh oh!

Conversation

beldmit commented Oct 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattcaswell commented Oct 4, 2021

Uh oh!

mattcaswell commented Oct 4, 2021

Uh oh!

mattcaswell commented Oct 4, 2021

Uh oh!

beldmit commented Oct 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattcaswell commented Oct 4, 2021

Uh oh!

mattcaswell commented Oct 4, 2021

Uh oh!

beldmit commented Oct 4, 2021

Uh oh!

levitte Oct 4, 2021

Choose a reason for hiding this comment

Uh oh!

levitte Oct 4, 2021

Choose a reason for hiding this comment

Uh oh!

beldmit Oct 4, 2021

Choose a reason for hiding this comment

Uh oh!

mattcaswell Oct 4, 2021

Choose a reason for hiding this comment

Uh oh!

beldmit Oct 4, 2021

Choose a reason for hiding this comment

Uh oh!

beldmit Oct 4, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

openssl-machine commented Oct 6, 2021

Uh oh!

beldmit commented Oct 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

beldmit commented Oct 3, 2021 •

edited

Loading

beldmit commented Oct 4, 2021 •

edited

Loading