Remove ia32cap overrides alternate #16693

bernd-edlinger · 2021-09-27T19:32:38Z

alternate implementation idea for #11933 (master-does not work)

The removed override was: OPENSSL_ia32cap=~0x200000200000000 which disables AESNI codepaths and PCLMULQDQ (useful for ghash). It is unclear why this was done, but it probably just hides bugs. [extended tests]

This replaces the AES-128-CBC-HMAC-SHA1 cipher with a non-encrypting version for use the test suite. [extended tests]

bernd-edlinger · 2021-09-27T20:41:22Z

interesting fact:
the checks start to pass if I do this:
OPENSSL_ia32cap=0 make test TESTS=70

But it is completely strange that it does even pass if I do this:

diff --git a/engines/e_ossltest.c b/engines/e_ossltest.c
index 58421ef..5cdcda7 100644
--- a/engines/e_ossltest.c
+++ b/engines/e_ossltest.c
@@ -846,7 +846,7 @@ int ossltest_aes128_cbc_hmac_sha1_cipher(EVP_CIPHER_CTX *ctx,
         }
     }
 
-    return 1;
+    return 0;
 }
 
 static int ossltest_aes128_cbc_hmac_sha1_ctrl(EVP_CIPHER_CTX *ctx, int type,

This should be completely impossible, since this makes the cipher FAIL in every case!

bernd-edlinger · 2021-09-28T19:34:04Z

Hmm, this looks like a deep crack in your new design....
if the test is run with OPENSSL_ia32cap=0 things start to go wrong about here:

openssl/ssl/tls_depr.c

Lines 37 to 40 in 398ae82

    
           eng = ENGINE_get_cipher_engine(nid); 
        
           if (eng != NULL) { 
        
               ENGINE_finish(eng); 
        
               return EVP_get_cipherbynid(nid);

here if nid == NID_aes_128_cbc_hmac_sha1 ENGINE_get_cipher_engine finds a non-zero engine,
but EVP_get_cipherbynid(nid) returns NULL, but ENGINE_get_cipher(eng, nid) would return the cipher
from tne ossltest engine.

bernd-edlinger · 2021-09-28T19:45:01Z

... and after this fails, the last chance to find the cipher is here:

openssl/ssl/ssl_lib.c

Lines 5885 to 5895 in 398ae82

    
               ciph = tls_get_cipher_from_engine(nid); 
        
               if (ciph != NULL) 
        
                   return ciph; 
        
               /* 
        
                * If there is no engine cipher then we do an explicit fetch. This may fail 
        
                * and that could be ok 
        
                */ 
        
               ERR_set_mark(); 
        
               ciph = EVP_CIPHER_fetch(libctx, OBJ_nid2sn(nid), properties); 
        
               ERR_pop_to_mark();

but EVP_CIPHER_fetch fill not find anything since the provider does not have the cipher since it is disabled by
OPENSSL_ia32cap=0, so the ssl uses the default implementation using separete AES-128-CBC and HMAC-SHA1.

bernd-edlinger · 2021-09-30T16:01:08Z

ssl/record/tls_pad.c

@@ -138,8 +138,6 @@ int tls1_cbc_remove_padding_and_mac(size_t *reclen,
        if (aead) {
            /* padding is already verified and we don't need to check the MAC */
            *reclen -= padding_length + 1 + mac_size;
-            *mac = NULL;
-            *alloced = 0;


No longer work in progress. This actually works now.
I had to fix a bug in the ssl engine to make that happen,
in deed any engine exporting the aes_128_cbc_hmac_sha1 cipher
would have crashed here, but the only other engine "dasync" with that cipher
is so broken in the RSA cipher, that I can't make it up to this point.

t8m

In general this looks good to me but perhaps @mattcaswell should look at the libssl changes to verify.

t8m · 2021-10-01T06:51:22Z

engines/e_ossltest.c

+int ossltest_aes128_cbc_hmac_sha1_init_key(EVP_CIPHER_CTX *ctx,
+                                           const unsigned char *key,
+                                           const unsigned char *iv, int enc);
+int ossltest_aes128_cbc_hmac_sha1_cipher(EVP_CIPHER_CTX *ctx,
+                                         unsigned char *out,
+                                         const unsigned char *in, size_t inl);


Should these functions be static as well?

yeah, maybe when I'm already there, I should also do the same with AES-128-CBC and AES-128-GCM ciphers.

t8m

LGTM. I am not sure whether this should be merged to 3.0 branch as well.

openssl-machine · 2021-10-05T07:00:16Z

This pull request is ready to merge

The removed override was: OPENSSL_ia32cap=~0x200000200000000 which disables AESNI codepaths and PCLMULQDQ (useful for ghash). It is unclear why this was done, but it probably just hides bugs. [extended tests] Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #16693)

This replaces the AES-128-CBC-HMAC-SHA1 cipher with a non-encrypting version for use the test suite. [extended tests] Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #16693)

bernd-edlinger · 2021-10-06T13:23:34Z

I am not sure whether this should be merged to 3.0 branch as well.

Yes, I would recommend that, because of the better test coverage of the legacy code path,
and because this fixes a potential null pointer dereference:

--- a/ssl/record/tls_pad.c
+++ b/ssl/record/tls_pad.c
@@ -138,8 +138,6 @@ int tls1_cbc_remove_padding_and_mac(size_t *reclen,
         if (aead) {
             /* padding is already verified and we don't need to check the MAC *
             *reclen -= padding_length + 1 + mac_size;
-            *mac = NULL;
-            *alloced = 0;
             return 1;
         }

bernd-edlinger · 2021-10-06T13:24:32Z

Merged to master so far...

bernd-edlinger · 2021-10-09T08:58:50Z

I found a way to trigger the expected crash the 3.0 branch, similar to #16724:

OPENSSL_ENGINES=../engines
OPENSSL_CONF=test.conf

$ cat test.conf 
openssl_conf = openssl_init

[openssl_init]
ssl_conf = ssl_config

[ssl_config]
server = server_conf
client = client_conf

[server_conf]
Options = -EncryptThenMac

[client_conf]
Options = -EncryptThenMac

../util/shlib_wrap.sh ./openssl s_server -engine dasync -ssl_config server -cipher "ECDHE-ECDSA-AES128-SHA:@SECLEVEL=0" -tls1 -cert ../test/certs/p256-server-cert.pem -key ../test/certs/p256-server-key.pem

../util/shlib_wrap.sh ./openssl s_client -engine dasync -ssl_config client -cipher "ECDHE-ECDSA-AES128-SHA:@SECLEVEL=0" -tls1

server will crash here:
Program received signal SIGSEGV, Segmentation fault.
0x00007ffff7b7bd59 in tls1_cbc_remove_padding_and_mac (reclen=0x7c5860, 
    origreclen=48, recdata=0x7cb8d8 "\024", mac=0x0, alloced=0x0, 
    block_size=16, mac_size=0, aead=1, libctx=0x0) at ssl/record/tls_pad.c:141
141	            *mac = NULL;
(gdb) bt
#0  0x00007ffff7b7bd59 in tls1_cbc_remove_padding_and_mac (reclen=0x7c5860, 
    origreclen=48, recdata=0x7cb8d8 "\024", mac=0x0, alloced=0x0, 
    block_size=16, mac_size=0, aead=1, libctx=0x0) at ssl/record/tls_pad.c:141
#1  0x00007ffff7b7944f in tls1_enc (s=0x7c45b0, recs=0x7c5858, n_recs=1, 
    sending=0, macs=0x0, macsize=0) at ssl/record/ssl3_record.c:1250
#2  0x00007ffff7b76a2c in ssl3_get_record (s=0x7c45b0)
    at ssl/record/ssl3_record.c:575
#3  0x00007ffff7b739fa in ssl3_read_bytes (s=0x7c45b0, type=22, 
    recvd_type=0x7fffffffd47c, buf=0x7c6370 "\001", len=4, peek=0, 
    readbytes=0x7fffffffd488) at ssl/record/rec_layer_s3.c:1348
#4  0x00007ffff7b9f05c in tls_get_message_header (s=0x7c45b0, 
    mt=0x7fffffffd4c0) at ssl/statem/statem_lib.c:1168
#5  0x00007ffff7b8dbf6 in read_state_machine (s=0x7c45b0)
    at ssl/statem/statem.c:587
#6  0x00007ffff7b8d866 in state_machine (s=0x7c45b0, server=1)
    at ssl/statem/statem.c:442
#7  0x00007ffff7b8d318 in ossl_statem_accept (s=0x7c45b0)
    at ssl/statem/statem.c:270
#8  0x00007ffff7b50150 in SSL_do_handshake (s=0x7c45b0) at ssl/ssl_lib.c:3893
#9  0x00007ffff7b4b9b9 in SSL_accept (s=0x7c45b0) at ssl/ssl_lib.c:1735
#10 0x000000000046c8fa in init_ssl_connection (con=0x7c45b0)
    at apps/s_server.c:2820
#11 0x000000000046c3dd in sv_body (s=4, stype=1, prot=0, context=0x0)

this does no longer reproduce on master, after this PR was merged.

beldmit · 2021-10-09T09:11:58Z

Looks like a separate problem, would you mind a separate issue for it?

bernd-edlinger · 2021-10-09T09:29:03Z

Okay, #16795

beldmit · 2021-10-09T09:32:39Z

Thanks! Could this be closed now?

bernd-edlinger · 2021-10-09T09:39:03Z

yes, however, a back-port to 3.0 would be suggested.

bernd-edlinger · 2021-10-09T09:43:04Z

Actually I only stumbled over #16724 because I knew this crash can happen, but it is really hard to avoid all the other bugs
on the way.

t8m · 2021-10-11T08:43:41Z

I am OK with merging this to 3.0. Will you do that @bernd-edlinger ?

This replaces the AES-128-CBC-HMAC-SHA1 cipher with a non-encrypting version for use the test suite. [extended tests] Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #16693) (cherry picked from commit 64da15c)

bernd-edlinger · 2021-10-11T14:26:17Z

Merged to 3.0 branch as 14fd5a0. Thanks!

bernd-edlinger added 2 commits September 26, 2021 08:58

Remove OPENSSL_ia32cap overrides in various test scripts

caf0eca

The removed override was: OPENSSL_ia32cap=~0x200000200000000 which disables AESNI codepaths and PCLMULQDQ (useful for ghash). It is unclear why this was done, but it probably just hides bugs. [extended tests]

Replace the AES-128-CBC-HMAC-SHA1 cipher in e_ossltest.c

26664cf

This replaces the AES-128-CBC-HMAC-SHA1 cipher with a non-encrypting version for use the test suite. [extended tests]

This was referenced Sep 27, 2021

[WIP] Remove ia32cap overrides alternate for 1.1.1 #16694

Closed

Remove OPENSSL_ia32cap overrides in various test scripts [1.1.1] #11934

Closed

t8m added branch: master Merge to master branch triaged: OTC evaluated This issue/pr was triaged by OTC triaged: refactor The issue/pr requests/implements refactoring and removed triaged: OTC evaluated This issue/pr was triaged by OTC labels Sep 28, 2021

github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Sep 29, 2021

fixup! Replace the AES-128-CBC-HMAC-SHA1 cipher in e_ossltest.c

68dfccc

bernd-edlinger force-pushed the remove_ia32cap_overrides_alt branch from 60dd28a to 68dfccc Compare September 29, 2021 14:44

bernd-edlinger changed the title ~~[WIP] Remove ia32cap overrides alternate~~ Remove ia32cap overrides alternate Sep 30, 2021

bernd-edlinger commented Sep 30, 2021

View reviewed changes

t8m added the triaged: bug The issue/pr is/fixes a bug label Oct 1, 2021

t8m reviewed Oct 1, 2021

View reviewed changes

fixup! Replace the AES-128-CBC-HMAC-SHA1 cipher in e_ossltest.c

8b72b1e

t8m approved these changes Oct 4, 2021

View reviewed changes

t8m added the approval: done This pull request has the required number of approvals label Oct 4, 2021

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 Oct 5, 2021

bernd-edlinger closed this Oct 9, 2021

bernd-edlinger reopened this Oct 9, 2021

github-actions bot removed the severity: fips change The pull request changes FIPS provider sources label Oct 9, 2021

bernd-edlinger mentioned this pull request Oct 9, 2021

crash in tls1_cbc_remove_padding_and_mac with aes-cbc-hmac-sha cipher #16795

Closed

bernd-edlinger closed this Oct 9, 2021

t8m added branch: 3.0 Merge to openssl-3.0 branch and removed triaged: refactor The issue/pr requests/implements refactoring labels Oct 11, 2021

t8m reopened this Oct 11, 2021

bernd-edlinger closed this Oct 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove ia32cap overrides alternate #16693

Remove ia32cap overrides alternate #16693

bernd-edlinger commented Sep 27, 2021

bernd-edlinger commented Sep 27, 2021

bernd-edlinger commented Sep 28, 2021

bernd-edlinger commented Sep 28, 2021

bernd-edlinger Sep 30, 2021

t8m left a comment

t8m Oct 1, 2021

bernd-edlinger Oct 1, 2021

t8m left a comment

openssl-machine commented Oct 5, 2021

bernd-edlinger commented Oct 6, 2021

bernd-edlinger commented Oct 6, 2021

bernd-edlinger commented Oct 9, 2021

beldmit commented Oct 9, 2021

bernd-edlinger commented Oct 9, 2021

beldmit commented Oct 9, 2021

bernd-edlinger commented Oct 9, 2021

bernd-edlinger commented Oct 9, 2021

t8m commented Oct 11, 2021

bernd-edlinger commented Oct 11, 2021

Remove ia32cap overrides alternate #16693

Remove ia32cap overrides alternate #16693

Conversation

bernd-edlinger commented Sep 27, 2021

bernd-edlinger commented Sep 27, 2021

bernd-edlinger commented Sep 28, 2021

bernd-edlinger commented Sep 28, 2021

bernd-edlinger Sep 30, 2021

Choose a reason for hiding this comment

t8m left a comment

Choose a reason for hiding this comment

t8m Oct 1, 2021

Choose a reason for hiding this comment

bernd-edlinger Oct 1, 2021

Choose a reason for hiding this comment

t8m left a comment

Choose a reason for hiding this comment

openssl-machine commented Oct 5, 2021

bernd-edlinger commented Oct 6, 2021

bernd-edlinger commented Oct 6, 2021

bernd-edlinger commented Oct 9, 2021

beldmit commented Oct 9, 2021

bernd-edlinger commented Oct 9, 2021

beldmit commented Oct 9, 2021

bernd-edlinger commented Oct 9, 2021

bernd-edlinger commented Oct 9, 2021

t8m commented Oct 11, 2021

bernd-edlinger commented Oct 11, 2021