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

engine: guard against multiple calls to engine_pka_destroy #37

Merged
merged 1 commit into from Aug 20, 2021

Conversation

xnox
Copy link
Contributor

@xnox xnox commented Aug 18, 2021

When engine is missused, engine_pka_destroy may be called multiple
times. When this happens, memory is freed which no longer was
allocated by the engine, leading to stack corruption and sigaborts.

Whilst engines should not be missused like that, it's best to guard
against such scenarios. It is not the first engine that has been
tripped by other software missusing openssl engine api's.

Sample gdb debuggin session with this patch applied:

    $ gdb curl
    (gdb) break engine_pka_destroy
    Function "engine_pka_destroy" not defined.
    Make breakpoint pending on future shared library load? (y or [n]) y
    Breakpoint 1 (engine_pka_destroy) pending.
    (gdb) run -O https://tpo.pe/pathogen.vim
    Thread 1 "curl" hit Breakpoint 1, engine_pka_destroy (e=0xaaaaaab59010) at e_bluefield.c:594
    warning: Source file is more recent than executable.
    594	    if (pka_rsa_meth) {
    (gdb) p pka_rsa_meth
    $1 = (RSA_METHOD *) 0xaaaaaab58d40
    (gdb) p pka_rsa_meth*
    A syntax error in expression, near `'.
    (gdb) p *pka_rsa_meth
    $2 = {name = 0xaaaaaab592c0 "BlueField RSA method",
      rsa_pub_enc = 0xfffff7a8c3a0 <rsa_ossl_public_encrypt>,
      rsa_pub_dec = 0xfffff7a8c040 <rsa_ossl_public_decrypt>,
      rsa_priv_enc = 0xfffff7a8cbb0 <rsa_ossl_private_encrypt>,
      rsa_priv_dec = 0xfffff7a8c728 <rsa_ossl_private_decrypt>,
      rsa_mod_exp = 0xfffff6af78c0 <engine_pka_rsa_mod_exp>,
      bn_mod_exp = 0xfffff6af76f0 <engine_pka_bn_mod_exp>, init = 0x0, finish = 0x0, flags = 0,
      app_data = 0x0, rsa_sign = 0x0, rsa_verify = 0x0, rsa_keygen = 0x0,
      rsa_multi_prime_keygen = 0x0}
    (gdb) n
    595		RSA_meth_free(pka_rsa_meth);
    (gdb) n
    596		pka_rsa_meth = NULL;
    (gdb) n
    600	    if (pka_dsa_meth) {
    (gdb) p pka_dsa_meth
    $3 = (DSA_METHOD *) 0xaaaaaab5e360
    (gdb) p *pka_dsa_meth
    $4 = {name = 0xaaaaaab592e0 "BlueField DSA method",
      dsa_do_sign = 0xfffff79f58a0 <dsa_do_sign>,
      dsa_sign_setup = 0xfffff79f5890 <dsa_sign_setup_no_digest>,
      dsa_do_verify = 0xfffff79f5190 <dsa_do_verify>,
      dsa_mod_exp = 0xfffff6af77f8 <engine_pka_dsa_mod_exp>,
      bn_mod_exp = 0xfffff6af8a40 <engine_pka_dsa_bn_mod_exp>, init = 0x0, finish = 0x0,
      flags = 0, app_data = 0x0, dsa_paramgen = 0x0, dsa_keygen = 0x0}
    (gdb) n
    601		    DSA_meth_free(pka_dsa_meth);
    (gdb) n
    602		pka_dsa_meth = NULL;
    (gdb) n
    606	    if (pka_dh_meth) {
    (gdb) p pka_dh_meth
    $5 = (DH_METHOD *) 0xaaaaaab53a90
    (gdb) p *pk_dh_meth
    No symbol "pk_dh_meth" in current context.
    (gdb) p *pka_dh_meth
    $6 = {name = 0xaaaaaab59300 "BlueField DH method",
      generate_key = 0xfffff79f02f8 <generate_key>, compute_key = 0xfffff79f00f8 <compute_key>,
      bn_mod_exp = 0xfffff6af77d8 <engine_pka_dh_bn_mod_exp>, init = 0x0, finish = 0x0,
      flags = 0, app_data = 0x0, generate_params = 0x0}
    (gdb) n
    607		    DH_meth_free(pka_dh_meth);
    (gdb) n
    608		pka_dh_meth = NULL;
    (gdb) p *pka_dh_meth
    $7 = {name = 0x0, generate_key = 0xaaaaaaaf3010,
      compute_key = 0xfffff79f00f8 <compute_key>,
      bn_mod_exp = 0xfffff6af77d8 <engine_pka_dh_bn_mod_exp>, init = 0x0, finish = 0x0,
      flags = 0, app_data = 0x0, generate_params = 0x0}
    (gdb) p pka_dh_meth
    $8 = (DH_METHOD *) 0xaaaaaab53a90
    (gdb) n
    611	if (pka_ec_key_meth) {
    (gdb) p pka_dh_meth
    $9 = (DH_METHOD *) 0x0
    (gdb) n
    612	    EC_KEY_METHOD_free(pka_ec_key_meth);
    (gdb) n
    613	    pka_ec_key_meth = NULL;
    (gdb) n
    618	    return 1;
    (gdb) continue
    Continuing.
    PKA_ENGINE: PKA instance is invalid
    PKA_ENGINE: failed to retrieve valid instance
    PKA_ENGINE: PKA instance is invalid
    PKA_ENGINE: failed to retrieve valid instance
    PKA_ENGINE: PKA instance is invalid
    PKA_ENGINE: failed to retrieve valid instance
    PKA_ENGINE: PKA instance is invalid
    PKA_ENGINE: failed to retrieve valid instance
    100   338  100   338    0     0      3      0  0:01:52  0:01:28  0:00:24     3

    Thread 1 "curl" hit Breakpoint 1, engine_pka_destroy (e=0xaaaaaab24320) at e_bluefield.c:594
    594	    if (pka_rsa_meth) {
    (gdb) p pka_rsa_meth
    $10 = (RSA_METHOD *) 0x0
    (gdb) n
    600	    if (pka_dsa_meth) {
    (gdb) p pka_dsa_meth
    $11 = (DSA_METHOD *) 0x0
    (gdb) n
    606	    if (pka_dh_meth) {
    (gdb) p pka_dh_meth
    $12 = (DH_METHOD *) 0x0
    (gdb) n
    611	if (pka_ec_key_meth) {
    (gdb) p pka_ec_key_meth
    $13 = (EC_KEY_METHOD *) 0x0
    (gdb) n
    618	    return 1;
    (gdb) continue
    Continuing.
    warning: Temporarily disabling breakpoints for unloaded shared library "/usr/lib/aarch64-linux-gnu/engines-1.1/pka.so"
    [Inferior 1 (process 73661) exited normally]

When engine is missused, engine_pka_destroy may be called multiple
times. When this happens, memory is freed which no longer was
allocated by the engine, leading to stack corruption and sigaborts.

Whilst engines should not be missused like that, it's best to guard
against such scenarios. It is not the first engine that has been
tripped by other software missusing openssl engine api's.

Sample gdb debuggin session with this patch applied:

    $ gdb curl
    (gdb) break engine_pka_destroy
    Function "engine_pka_destroy" not defined.
    Make breakpoint pending on future shared library load? (y or [n]) y
    Breakpoint 1 (engine_pka_destroy) pending.
    (gdb) run -O https://tpo.pe/pathogen.vim
    Thread 1 "curl" hit Breakpoint 1, engine_pka_destroy (e=0xaaaaaab59010) at e_bluefield.c:594
    warning: Source file is more recent than executable.
    594	    if (pka_rsa_meth) {
    (gdb) p pka_rsa_meth
    $1 = (RSA_METHOD *) 0xaaaaaab58d40
    (gdb) p pka_rsa_meth*
    A syntax error in expression, near `'.
    (gdb) p *pka_rsa_meth
    $2 = {name = 0xaaaaaab592c0 "BlueField RSA method",
      rsa_pub_enc = 0xfffff7a8c3a0 <rsa_ossl_public_encrypt>,
      rsa_pub_dec = 0xfffff7a8c040 <rsa_ossl_public_decrypt>,
      rsa_priv_enc = 0xfffff7a8cbb0 <rsa_ossl_private_encrypt>,
      rsa_priv_dec = 0xfffff7a8c728 <rsa_ossl_private_decrypt>,
      rsa_mod_exp = 0xfffff6af78c0 <engine_pka_rsa_mod_exp>,
      bn_mod_exp = 0xfffff6af76f0 <engine_pka_bn_mod_exp>, init = 0x0, finish = 0x0, flags = 0,
      app_data = 0x0, rsa_sign = 0x0, rsa_verify = 0x0, rsa_keygen = 0x0,
      rsa_multi_prime_keygen = 0x0}
    (gdb) n
    595		RSA_meth_free(pka_rsa_meth);
    (gdb) n
    596		pka_rsa_meth = NULL;
    (gdb) n
    600	    if (pka_dsa_meth) {
    (gdb) p pka_dsa_meth
    $3 = (DSA_METHOD *) 0xaaaaaab5e360
    (gdb) p *pka_dsa_meth
    $4 = {name = 0xaaaaaab592e0 "BlueField DSA method",
      dsa_do_sign = 0xfffff79f58a0 <dsa_do_sign>,
      dsa_sign_setup = 0xfffff79f5890 <dsa_sign_setup_no_digest>,
      dsa_do_verify = 0xfffff79f5190 <dsa_do_verify>,
      dsa_mod_exp = 0xfffff6af77f8 <engine_pka_dsa_mod_exp>,
      bn_mod_exp = 0xfffff6af8a40 <engine_pka_dsa_bn_mod_exp>, init = 0x0, finish = 0x0,
      flags = 0, app_data = 0x0, dsa_paramgen = 0x0, dsa_keygen = 0x0}
    (gdb) n
    601		    DSA_meth_free(pka_dsa_meth);
    (gdb) n
    602		pka_dsa_meth = NULL;
    (gdb) n
    606	    if (pka_dh_meth) {
    (gdb) p pka_dh_meth
    $5 = (DH_METHOD *) 0xaaaaaab53a90
    (gdb) p *pk_dh_meth
    No symbol "pk_dh_meth" in current context.
    (gdb) p *pka_dh_meth
    $6 = {name = 0xaaaaaab59300 "BlueField DH method",
      generate_key = 0xfffff79f02f8 <generate_key>, compute_key = 0xfffff79f00f8 <compute_key>,
      bn_mod_exp = 0xfffff6af77d8 <engine_pka_dh_bn_mod_exp>, init = 0x0, finish = 0x0,
      flags = 0, app_data = 0x0, generate_params = 0x0}
    (gdb) n
    607		    DH_meth_free(pka_dh_meth);
    (gdb) n
    608		pka_dh_meth = NULL;
    (gdb) p *pka_dh_meth
    $7 = {name = 0x0, generate_key = 0xaaaaaaaf3010,
      compute_key = 0xfffff79f00f8 <compute_key>,
      bn_mod_exp = 0xfffff6af77d8 <engine_pka_dh_bn_mod_exp>, init = 0x0, finish = 0x0,
      flags = 0, app_data = 0x0, generate_params = 0x0}
    (gdb) p pka_dh_meth
    $8 = (DH_METHOD *) 0xaaaaaab53a90
    (gdb) n
    611	if (pka_ec_key_meth) {
    (gdb) p pka_dh_meth
    $9 = (DH_METHOD *) 0x0
    (gdb) n
    612	    EC_KEY_METHOD_free(pka_ec_key_meth);
    (gdb) n
    613	    pka_ec_key_meth = NULL;
    (gdb) n
    618	    return 1;
    (gdb) continue
    Continuing.
    PKA_ENGINE: PKA instance is invalid
    PKA_ENGINE: failed to retrieve valid instance
    PKA_ENGINE: PKA instance is invalid
    PKA_ENGINE: failed to retrieve valid instance
    PKA_ENGINE: PKA instance is invalid
    PKA_ENGINE: failed to retrieve valid instance
    PKA_ENGINE: PKA instance is invalid
    PKA_ENGINE: failed to retrieve valid instance
    100   338  100   338    0     0      3      0  0:01:52  0:01:28  0:00:24     3

    Thread 1 "curl" hit Breakpoint 1, engine_pka_destroy (e=0xaaaaaab24320) at e_bluefield.c:594
    594	    if (pka_rsa_meth) {
    (gdb) p pka_rsa_meth
    $10 = (RSA_METHOD *) 0x0
    (gdb) n
    600	    if (pka_dsa_meth) {
    (gdb) p pka_dsa_meth
    $11 = (DSA_METHOD *) 0x0
    (gdb) n
    606	    if (pka_dh_meth) {
    (gdb) p pka_dh_meth
    $12 = (DH_METHOD *) 0x0
    (gdb) n
    611	if (pka_ec_key_meth) {
    (gdb) p pka_ec_key_meth
    $13 = (EC_KEY_METHOD *) 0x0
    (gdb) n
    618	    return 1;
    (gdb) continue
    Continuing.
    warning: Temporarily disabling breakpoints for unloaded shared library "/usr/lib/aarch64-linux-gnu/engines-1.1/pka.so"
    [Inferior 1 (process 73661) exited normally]

Signed-off-by: Dimitri John Ledkov <dimitri.ledkov@canonical.com>
Copy link
Contributor

@kblaiech kblaiech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mahantesh-nvidia mahantesh-nvidia merged commit 7fd1396 into Mellanox:master Aug 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants