Heap use after free in ENGINE_remove

[bernd-edlinger]

when I compile the test case from #7950 with address sanitizer there is a heap use after free:
#7950 (comment)

I think this is a different issue than #16724 since it happens with every engine
not only dasync, and also here the test case is okay in 1.1.1 but broken in master.

#include <openssl/engine.h>
#include <openssl/evp.h>

int main()
{
  ENGINE *e;
  const EVP_CIPHER * cipher;
  EVP_CIPHER_CTX *ctx;

  OPENSSL_init_crypto(OPENSSL_INIT_ENGINE_DYNAMIC, NULL);
  e = ENGINE_by_id("dasync");
  if (e == NULL)
    return 1;
  ENGINE_init(e);
  cipher = ENGINE_get_cipher(e, NID_aes_128_cbc_hmac_sha1);
  ctx = EVP_CIPHER_CTX_new();
  if (cipher != NULL && ctx != NULL)
    {
       int rv = EVP_EncryptInit_ex(ctx, cipher, e, NULL, NULL);
       printf("EncryptInit=%d\n", rv);
    }
  EVP_CIPHER_CTX_free(ctx);
  ENGINE_finish(e);
  ENGINE_remove(e);
  ENGINE_free(e);
  return 0;
}

s ./a.out 
EncryptInit=1
=================================================================
==6080==ERROR: AddressSanitizer: heap-use-after-free on address 0x611000000404 at pc 0x7f07868a55ed bp 0x7fffd03f61d0 sp 0x7fffd03f61c8
READ of size 4 at 0x611000000404 thread T0
    #0 0x7f07868a55ec in EVP_PKEY_meth_free crypto/evp/pmeth_lib.c:445
    #1 0x7f078783d7e4 in destroy_pkey engines/e_dasync.c:315
    #2 0x7f078783d7e4 in dasync_destroy engines/e_dasync.c:382
    #3 0x7f078680d5bd in engine_free_util crypto/engine/eng_lib.c:92
    #4 0x4012e9 in main /home/ed/OPC/openssl/test.c:25
    #5 0x7f0786071f44 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21f44)
    #6 0x401168  (/home/ed/OPC/openssl/a.out+0x401168)

0x611000000404 is located 4 bytes inside of 256-byte region [0x611000000400,0x611000000500)
freed by thread T0 here:
    #0 0x7f0786eaa2b7 in __interceptor_free ../../../../gcc-trunk/libsanitizer/asan/asan_malloc_linux.cpp:111
    #1 0x7f0786813bb4 in engine_pkey_meths_free crypto/engine/tb_pkmeth.c:113
    #2 0x7f078680d591 in engine_free_util crypto/engine/eng_lib.c:85
    #3 0x4012e9 in main /home/ed/OPC/openssl/test.c:25
    #4 0x7f0786071f44 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21f44)

previously allocated by thread T0 here:
    #0 0x7f0786eaa577 in __interceptor_malloc ../../../../gcc-trunk/libsanitizer/asan/asan_malloc_linux.cpp:129
    #1 0x7f07868d8881 in CRYPTO_zalloc crypto/mem.c:191
    #2 0x7f07868a5978 in EVP_PKEY_meth_new crypto/evp/pmeth_lib.c:130
    #3 0x7f078783df32 in bind_dasync engines/e_dasync.c:214
    #4 0x7f078783ea32 in bind_helper engines/e_dasync.c:325
    #5 0x7f078783ea32 in bind_engine engines/e_dasync.c:331
    #6 0x7f078680bc98 in dynamic_load crypto/engine/eng_dyn.c:487
    #7 0x7f078680bc98 in dynamic_ctrl crypto/engine/eng_dyn.c:344
    #8 0x7f078680aafa in ENGINE_ctrl_cmd_string crypto/engine/eng_ctrl.c:324
    #9 0x7f078680ee51 in ENGINE_by_id crypto/engine/eng_list.c:337
    #10 0x401236 in main /home/ed/OPC/openssl/test.c:11
    #11 0x7f0786071f44 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21f44)

SUMMARY: AddressSanitizer: heap-use-after-free crypto/evp/pmeth_lib.c:445 in EVP_PKEY_meth_free
Shadow bytes around the buggy address:
  0x0c227fff8030: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c227fff8040: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c227fff8050: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c227fff8060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c227fff8070: 00 fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c227fff8080:[fd]fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c227fff8090: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c227fff80a0: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c227fff80b0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c227fff80c0: fd fd fd fd fd fd fa fa fa fa fa fa fa fa fa fa
  0x0c227fff80d0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==6080==ABORTING

[bernd-edlinger]

One additional questionable thing is, when I remove the line
OPENSSL_init_crypto(OPENSSL_INIT_ENGINE_DYNAMIC, NULL);
from the test case, the function ENGINE_by_id("dasync") does still
return the dasync engine in master, while 1.1.1 does not return the dynamic
engine, because the engine named "dynamic" is only loaded thru
the above call. I wonder if that change in behavior is also an error.

[mattcaswell]

since it happens with every engine

I can't explain this part since there is a problem in daysnc engine. How have you tested other engines?

The problem is here:

openssl/engines/e_dasync.c

Lines 313 to 318 in 657d192

	static void destroy_pkey(void)
	{
	EVP_PKEY_meth_free(dasync_rsa);
	dasync_rsa_orig = NULL;
	dasync_rsa = NULL;
	}

The dasync engine free the EVP_PKEY_METHOD it previously created and set here:

openssl/engines/e_dasync.c

Line 234 in 657d192

|| !ENGINE_set_pkey_meths(e, dasync_pkey)

But actually this is incorrect since libcrypto automatically frees all the EVP_PKEY_METHODs for you when it removes the ENGINE:

openssl/crypto/engine/tb_pkmeth.c

Lines 98 to 117 in 657d192

	/*
	* Internal function to free up EVP_PKEY_METHOD structures before an ENGINE
	* is destroyed
	*/
	void engine_pkey_meths_free(ENGINE *e)
	{
	int i;
	EVP_PKEY_METHOD *pkm;
	if (e->pkey_meths) {
	const int *pknids;
	int npknids;
	npknids = e->pkey_meths(e, NULL, &pknids, 0);
	for (i = 0; i < npknids; i++) {
	if (e->pkey_meths(e, &pkm, NULL, pknids[i])) {
	EVP_PKEY_meth_free(pkm);
	}
	}
	}
	}

here the test case is okay in 1.1.1 but broken in master.

This is because the 1.1.1 dasync engine does not support an EVP_PKEY_METHOD but it does in master/3.0.

[bernd-edlinger]

okay, my fault. I thought I had seen that also with ossltest / aes_128_cbc, but when I tried it again,
it was good, maybe I had one too many context switches.

[bernd-edlinger]

And what do you think about the OPENSSL_INIT_ENGINE_DYNAMIC no longer needed, is that on purpose?

[mattcaswell]

And what do you think about the OPENSSL_INIT_ENGINE_DYNAMIC no longer needed, is that on purpose?

This looks like a bug in 1.1.1 to me. You should not need to call OPENSSL_init_crypto for this. The only time you should need to call that function directly is where you want some non-standard initialisation to take place (such as if you want to suppress automatic loading of the config file).

In this case it looks like that bug was fixed by #11543 (a5c864c), and it should be backported to 1.1.1.

[mattcaswell]

it should be backported to 1.1.1.

#16741