OpenSSL "double free" error

Bug #1921518 reported by Mahantesh Salimath
276
This bug affects 1 person
Affects Status Importance Assigned to Milestone
wget (Ubuntu)
Fix Released
Undecided
Unassigned
Focal
Fix Released
Undecided
Unassigned

Bug Description

[Impact]
openssl config file is being loaded twice, causing engines to be loaded twice if specified therein, causing double free errors and other strange behavior.

[Test plan]
Run the command of the package being tested in

gdb -ex "break CONF_modules_load_file" -ex "run" --args

and make sure it only breaks one.

Regression test:

In default Ubuntu configuration, either no openssl configuration is provided, or it contains no settings that affect wget. This code path changes how/when openssl configuration is loaded and used by openssl. One should verify that:
1) wget continues to work without openssl.cnf
2) wget continues to work with stock ubuntu unmodified openssl.cnf
3) wget continue to honor and use custom TLS settings that one may have specified in openssl.cnf (for example custom engine)

[Where problems could occur]

wget: This is an upstream change that changes initialization and is in use in later releases. Since it mostly removes an unneeded call to the load file function, a regression could be a config file being ignored, but it seems unlikely given the use in later releases

[Original bug report]
"double free" error is seen when using curl utility. Error is from libcrypto.so which is part of the OpenSSL package. This happens only when OpenSSL is configured to use a dynamic engine.

OpenSSL version is 1.1.1f

The issue is not encountered if http://www.openssl.org/source/openssl-1.1.1f.tar.gz is used instead.

OpenSSL can be configured to use a dynamic engine by editing the default openssl config file which is located at '/etc/ssl/openssl.cnf' on Ubuntu systems.

On Bluefield systems, config diff to enable PKA dynamic engine, is as below:

+openssl_conf = conf_section
+
 # Extra OBJECT IDENTIFIER info:
 #oid_file = $ENV::HOME/.oid
 oid_section = new_oids

+[ conf_section ]
+engines = engine_section
+
+[ engine_section ]
+bf = bf_section
+
+[ bf_section ]
+engine_id=pka
+dynamic_path=/usr/lib/aarch64-linux-gnu/engines-1.1/pka.so
+init=0
+

engine_id above refers to dynamic engine name/identifier.
dynamic_path points to the .so file for the dynamic engine.

# curl -O https://tpo.pe/pathogen.vim

double free or corruption (out)

Aborted (core dumped)

CVE References

description: updated
Revision history for this message
Mahantesh Salimath (mahantesh92) wrote :
information type: Public → Private
information type: Private → Private Security
Revision history for this message
Dimitri John Ledkov (xnox) wrote :

@ Mahantesh Salimath

You have opened a public bug report against community supported Ubuntu distribution only, which is not monitored by the dedicated Canonical engineers for the engagement project you are possibly part of.

If this issue is related to the chelmsford project, could you please open a bug report in the chelmsford project?

I do not have a way to move this ticket there, and I have for now marked this bug report private.

information type: Private Security → Private
information type: Private → Public
information type: Public → Private Security
Revision history for this message
Dimitri John Ledkov (xnox) wrote :

If this is intended as a public bug report, please double check if the bug description and the coredump disclose private information, before marking it public again.

Revision history for this message
Alex Kaluzhny (alex-kaluzhny) wrote :

@Dimitri,

Mahantesh first approached Canonical Delivery team for Nvidia (Mellonax) Bluefield SmartNIC with this issue, and we suggested to file a bug against OpenSSL project in Launchpad, and that is exactly what he did.
The bug was filed 2 months ago and he saw no traction with it.
I am not sure if that was his way to attract attention by making it private, but it, certainly, would be good if this bug is triaged and assigned. Thank you for your help!

Changed in openssl (Ubuntu):
importance: Undecided → Critical
information type: Private Security → Public Security
Revision history for this message
Dimitri John Ledkov (xnox) wrote :

It appears that engine is destroyed multiple times.

Please see https://github.com/Mellanox/pka/pull/37 which can help to guard against that.

Meanwhile I'm continuing to research as to why engine is destroyed multiple times.

Revision history for this message
Dimitri John Ledkov (xnox) wrote :

Cannot reproduce the issue when using `openssl s_client -connect` or when using `wget` so it is specific to curl + openssl + engine at the moment.

Revision history for this message
Dimitri John Ledkov (xnox) wrote :

Found curl missuse of openssl api; Found missing use-after-free fixes in openssl; in addition to the pka engine fixes that are possible.

Imho all three should be fixed.

Revision history for this message
Dimitri John Ledkov (xnox) wrote :
Changed in openssl (Ubuntu Focal):
importance: Undecided → Critical
Revision history for this message
Dimitri John Ledkov (xnox) wrote :
Changed in openssl (Ubuntu):
status: New → Incomplete
Changed in openssl (Ubuntu Focal):
status: New → Incomplete
importance: Critical → Undecided
Changed in openssl (Ubuntu):
importance: Critical → Undecided
Revision history for this message
Dimitri John Ledkov (xnox) wrote :

Whilst I have identified broken/racy/incomplete behaviours in both curl and openssl in ubuntu focal 20.04 and created SRUs for them in the above mentioned bug reports; these do not fix crashes of the old PKA 1.0.0 engine.

Also PKA 1.0.0 does not appear to be compatible with 20.04 userspace anymore.

The fix I have proposed for the PKA engine https://github.com/Mellanox/pka/pull/37/files must be shipped on all systems / any distribution.

If you can reproduce any new issues whilst using openssl & curl from https://launchpad.net/~ci-train-ppa-service/+archive/ubuntu/4654/+packages and whilst using up to date PKA with my pull request merged, please provide further details.

As it stands, I don't see any crashes on any systems, once _all_ of the above are applied.

Revision history for this message
Vladimir Sokolovsky (vlad-a) wrote :

Hi Dimitri,
I tested new openssl, curl and your patch for the pka engine and it fixed the issue.
Please push the new curl and openssl to updates repository.

Thanks a lot.

Revision history for this message
Dimitri John Ledkov (xnox) wrote :

New curl & openssl will take some time to appear in focal-updates, as focal-updates are frozen for 20.04.3 release on 26th of August at the moment.

See https://discourse.ubuntu.com/t/focal-fossa-20-04-3-lts-point-release-status-tracking/22948

Revision history for this message
Vladimir Sokolovsky (vlad-a) wrote :

It is fine. For now I updated my local base OS image to include your fixes.

Thanks again for the fast response and providing the fixes.

Revision history for this message
Vladimir Sokolovsky (vlad-a) wrote :

Note that there is a conflict between libcrypto and openssl:

# dpkg -i libcrypto1.1-udeb_1.1.1f-1ubuntu2.6_arm64.udeb
(Reading database ... 160240 files and directories currently installed.)
Preparing to unpack libcrypto1.1-udeb_1.1.1f-1ubuntu2.6_arm64.udeb ...
Unpacking libcrypto1.1-udeb (1.1.1f-1ubuntu2.6) ...
dpkg: error processing archive libcrypto1.1-udeb_1.1.1f-1ubuntu2.6_arm64.udeb (--install):
 trying to overwrite '/usr/lib/ssl/openssl.cnf', which is also in package openssl 1.1.1f-1ubuntu2.6
Errors were encountered while processing:
 libcrypto1.1-udeb_1.1.1f-1ubuntu2.6_arm64.udeb

Revision history for this message
Vladimir Sokolovsky (vlad-a) wrote :

Also libssl1.1-udeb depends on libc6-udeb which is not available:

# dpkg -i libssl1.1-udeb_1.1.1f-1ubuntu2.6_arm64.udeb
(Reading database ... 128018 files and directories currently installed.)
Preparing to unpack libssl1.1-udeb_1.1.1f-1ubuntu2.6_arm64.udeb ...
Unpacking libssl1.1-udeb (1.1.1f-1ubuntu2.6) over (1.1.1f-1ubuntu2.6) ...
dpkg: dependency problems prevent configuration of libssl1.1-udeb:
 libssl1.1-udeb depends on libc6-udeb (>= 2.31); however:
  Package libc6-udeb is not installed.

dpkg: error processing package libssl1.1-udeb (--install):
 dependency problems - leaving unconfigured
Errors were encountered while processing:
 libssl1.1-udeb

Revision history for this message
Steve Langasek (vorlon) wrote :

You should not be installing use packages on an Ubuntu system. These packages are only for use in the debian-installer environment.

Revision history for this message
Steve Langasek (vorlon) wrote :

*udeb, not use, thanks autocorrect

Revision history for this message
Vladimir Sokolovsky (vlad-a) wrote :

without libcrypto1.1-udeb_1.1.1f-1ubuntu2.6_arm64.udeb the issue still reproduced. So, how can I get this fix?

Revision history for this message
Vladimir Sokolovsky (vlad-a) wrote :

Actually, I see that the issue disappear only after /usr/lib/ssl/openssl.cnf was changed by libcrypto1.1-udeb_1.1.1f-1ubuntu2.6_arm64.udeb.

So, it may be the case that the bug is still there.

What is the proper way to install the fix?

Revision history for this message
Dimitri John Ledkov (xnox) wrote (last edit ):

@vladimir sokolovsky

Note, that the proposed PPA is built for all architectures, and all configurations of the packages in questions as used in Ubuntu. Meaning, they are all compiled in multiple configurations, which are mutually incompatible. To ensure one installs the upgraded packages suitable for ones own system, one should not download all/any/individual debs from the archive, but instead one should only download and upgrade the packages that are already present on ones own system only.

For example, your comments indicate that incompatible packages were downloaded and attempted to be forcefully installed, breaking your system.

Please revert the system to stock configuration.

Enable the ppa as mentioned in https://launchpad.net/~ci-train-ppa-service/+archive/ubuntu/4654 "Adding this PPA to your system" I.e. specifically `sudo add-apt-repository ppa:ci-train-ppa-service/4654` followed by `sudo apt update; sudo apt full-upgrade`

This should only install relevant packages for your system from the proposed ppa, specifically things like "libssl1.1 openssl cur libcurl4" in the deb format for the arm64 server architecture only.

Do not install any packages that have udeb in the name, they are not used on servers and have incompatible configuration.

The above is preferred.

If you do not have direct access to launchpad PPA on your system under test, you can use pull-pkg utility from ubuntu-dev-tools to download packages you require, but then you must be extra careful to upgrade only the matching set of packages. For example

$ pull-pkg -D ppa --ppa ci-train-ppa-service/4654 --arch arm64 --pull debs openssl focal
$ pull-pkg -D ppa --ppa ci-train-ppa-service/4654 --arch arm64 --pull debs curl focal

Transfer the debs to your system under test.
Check packages that are already installed, and upgrade only the ones that are already on your system.
I.e.
$ dpkg -l | grep -e 7.68.0 -e 1.1.1f
$ sudo apt install ./curl_*.deb ./libssl1.1_*.deb ./openssl_*.deb ./libcurl4_*.deb

If needed, you can also use pull-pkg tool to download debug symbols packages to assist in debugging. And again only install debug symbols packages for the libraries and packages already present on your system; as again there are debug symbols provided for all configurations, which are incompatible with each other and cannot be all installed simultaneously.

Revision history for this message
Mahantesh Salimath (mahantesh92) wrote :

The updated OpenSSL package is not behaving as expected, openssl config file (/etc/ssl/openssl.cnf) has PKA dynamic engine enabled. But execution of `openssl engine` doesn't show (PKA) engine as one of the listings. And also, offloading to PKA doesn't happen by default. Ex: Executing speed test of PKA supported algorithms would by default offload to PKA engine (openssl speed rsa512), this is not the case now. Hence it seems updated OpenSSL package just provided a workaround by not offloading to PKA by default. The fix expected should offload to PKA by default and have no issues when used with curl and wget.

Revision history for this message
Dimitri John Ledkov (xnox) wrote :

The updated openssl package does not change any behaviour w.r.t. config or engine use. It only has three patches applied to prevent potential use-after-free errors. It also relies on installing the new PKA engine with patches from github.

Has the new PKA engine been recompiled and installed correctly?

When installed on my system, the engines configured continued to be used, and I can observe PKA debug output when using `openssl s_client -connect` or `curl`.

Note, that `speed` command, by default does not use engines, even when configured in /etc/ssl/openssl.cnf. One must pass `-engine ID` option to it to use an engine.

Revision history for this message
Vladimir Sokolovsky (vlad-a) wrote :

Ubuntu 20.04 updates repository was updated with:
libssl1.1/focal-updates,focal-security 1.1.1f-1ubuntu2.8 arm64 [upgradable from: 1.1.1f-1ubuntu2.5]
openssl/focal-updates,focal-security 1.1.1f-1ubuntu2.8 arm64 [upgradable from: 1.1.1f-1ubuntu2.5]

Do these versions include the fixes above?

Revision history for this message
Marc Deslauriers (mdeslaur) wrote :

No, they do not include the fixes from this bug.

Revision history for this message
Dimitri John Ledkov (xnox) wrote :

1.1.1f-1ubuntu2.8 is security-only update to address CVE-2021-3711 & CVE-2021-3712

The fixes from this bug report have been rebased on top of the security-only update in the PPA provided earlier. It has been carrying 1.1.1f-1ubuntu2.9 since yesterday.

Revision history for this message
Vladimir Sokolovsky (vlad-a) wrote :

Dimitri, can you provide debs with your fixes and higher version, so apt update will not remove your fix?

Revision history for this message
Dimitri John Ledkov (xnox) wrote :

Vladimir, I did this in the same location as before - https://launchpad.net/~ci-train-ppa-service/+archive/ubuntu/4654

Revision history for this message
Vladimir Sokolovsky (vlad-a) wrote :

The issue is still reproduced:
# dpkg --list | grep 1.1.1f
ii libssl-dev:arm64 1.1.1f-1ubuntu2.9 arm64 Secure Sockets Layer toolkit - development files
ii libssl1.1:arm64 1.1.1f-1ubuntu2.9 arm64 Secure Sockets Layer toolkit - shared libraries
ii openssl 1.1.1f-1ubuntu2.9 arm64 Secure Sockets Layer toolkit - cryptographic utility

# dpkg --list | grep curl
ii curl 7.68.0-1ubuntu2.7 arm64 command line tool for transferring data with URL syntax
ii libcurl3-gnutls:arm64 7.68.0-1ubuntu2.7 arm64 easy-to-use client-side URL transfer library (GnuTLS flavour)
ii libcurl4:arm64 7.68.0-1ubuntu2.7 arm64 easy-to-use client-side URL transfer library (OpenSSL flavour)

# curl -O https://tpo.pe/pathogen.vim
  % Total % Received % Xferd Average Speed Time Time Time Current
                                 Dload Upload Total Spent Left Speed
  0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0
Bus error (core dumped)

Revision history for this message
Dimitri John Ledkov (xnox) wrote :

@Vladimir

This is an improvement.

Previously we were getting: double free or corruption (out)
But now it is: Bus error
So some progress has been made.

Can you please install debug symbols, and generate a complete traceback with debug symbols? or a core dump with debug symbols? (libcurl4-dbgsym curl-dbgsym libssl1.1-dbgsym)

Also which libpka are you using? Were debug symbols compiled for it, and can you share it? Is it just the latest build from github?

I have not previously seen Bus error when debugging this issue => on arm64 it is usually non-aligned memory access coming from somewhere.

Revision history for this message
Vladimir Sokolovsky (vlad-a) wrote :

Dimitri, we use the latest https://github.com/Mellanox/pka/tree/releases that includes the fix proposed by you.
Aren't you able to reproduce this issue on your side anymore?

Revision history for this message
Dimitri John Ledkov (xnox) wrote :

No I'm not able to reproduce the issues anymore. Hence I need detailed logs from you. Including tracebacks with debug symbols installed, and strace too. Because I have never seen "bus error" on my side.

Revision history for this message
Dmitrii Shcherbakov (dmitriis) wrote :
Download full text (3.6 KiB)

Vladimir,

stracing reveals that si_code is set to BUS_ADRALN so there is a problem with address alignment.

strace curl https://example.com

--- SIGBUS {si_signo=SIGBUS, si_code=BUS_ADRALN, si_addr=0x3efd151115865b} ---
+++ killed by SIGBUS (core dumped) +++
Bus error (core dumped)

The fault is raised by the CPU in response to a misaligned address and the respective handler in the kernel is being invoked to assert a signal to the process:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=52d7523d84d534c241ebac5ac89f5c0a6cb51e41

https://paste.ubuntu.com/p/yHJrJW2gSF/ (package & distro details)

----

By the looks of it the alignment fault is caused by just trying to call the public key method init function in the PKA engine.

Below we have:

1) pmeth->init is at 0xc82028bf65604647

When it is attempted to be called, si_addr has the same value:

2) _sigfault = {si_addr = 0x2028bf65604647}

(gdb) r
The program being debugged has been started already.
Start it from the beginning? (y or n) y
Starting program: /usr/bin/curl https://example.com
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/aarch64-linux-gnu/libthread_db.so.1".
[New Thread 0xfffff6372f90 (LWP 2486506)]
[Thread 0xfffff6372f90 (LWP 2486506) exited]

Thread 1 "curl" hit Breakpoint 1, int_ctx_new (pkey=pkey@entry=0x0, e=e@entry=0x0, id=1034) at ../crypto/evp/pmeth_lib.c:113
113 if (id == -1) {
(gdb) n
119 if (e == NULL && pkey != NULL)
(gdb)
122 if (e) {
(gdb)
128 e = ENGINE_get_pkey_meth_engine(id);
(gdb)
135 if (e)
(gdb)
136 pmeth = ENGINE_get_pkey_meth(e, id);
(gdb)
141 if (pmeth == NULL) {
(gdb)
149 ret = OPENSSL_zalloc(sizeof(*ret));
(gdb)
150 if (ret == NULL) {
(gdb)
157 ret->engine = e;
(gdb)
159 ret->operation = EVP_PKEY_OP_UNDEFINED;
(gdb)
161 if (pkey != NULL)
(gdb)
164 if (pmeth->init) {
(gdb)
165 if (pmeth->init(ret) <= 0) {
(gdb) print *pmeth
$10 = {pkey_id = -1784943492, flags = -364887078, init = 0xc82028bf65604647, copy = 0x9c17b192eb068c0b, cleanup = 0xedbe7dcdf413f1c0, paramgen_init = 0xc28e015828ce4282, paramgen = 0x6fce6fa0a7ee471f,
  keygen_init = 0xdf9a9579438d24eb, keygen = 0xc63719742b8964b9, sign_init = 0x78f4d90cba7ad854, sign = 0xb0d4f1b3df1a9e13, verify_init = 0x7b5f10ffa4c58586, verify = 0x96e16d3250d67446,
  verify_recover_init = 0xe11ef96099ea206c, verify_recover = 0x8ed096c03e046773, signctx_init = 0xc6ea05c3bdb5153c, signctx = 0xdd1cb7963c7185, verifyctx_init = 0xd19718983089e1f8,
  verifyctx = 0x6143e92bef937feb, encrypt_init = 0x94450e0e52af0bcd, encrypt = 0x2a4633c02797f8b, decrypt_init = 0xa69b08bdbfea813, decrypt = 0x84b9264be5facf60, derive_init = 0x99bcf2700df9fc7e,
  derive = 0x9961eec79bc58dfb, ctrl = 0x1779f7901d10471b, ctrl_str = 0x763a1ebbf28338f0, digestsign = 0xacc57ce435798e94, digestverify = 0xae611fd83700f11f, check = 0x6b8d5f0b7cf4a89b,
  public_check = 0xef347940990e67fb, param_check = 0xe, digest_custom = 0xfffff797ec60 <aes_v8_encrypt>}

(gdb) print pmeth->init
$11 = (int (*)(EVP_PKEY_CTX *)) 0xc82028bf65604647

(gdb) n

Thread 1 "curl" received signal SIGBUS, Bus error.
0x002028bf65604647 in ?? ()

(gdb) p $_siginfo
$12 = {si_signo = 7, si_errno = 0, si_code = 1, _...

Read more...

Revision history for this message
Dmitrii Shcherbakov (dmitriis) wrote :

Also I seem to be getting a SIGBUS signal only when function addresses are not 4-byte aligned:

Thread 1 "curl" received signal SIGBUS, Bus error.
0x006358f58d277bf1 in ?? ()

Thread 1 "curl" received signal SIGBUS, Bus error.
0xb1b30b5cc1eb2dda in ?? ()

Thread 1 "curl" received signal SIGBUS, Bus error.
0x006358f58d277bf1 in ?? ()

Thread 1 "curl" received signal SIGBUS, Bus error.
0x4fc81bc04dc15a0d in ?? ()

When they are 4-byte aligned, I get SIGSEGV instead:

Thread 1 "curl" received signal SIGSEGV, Segmentation fault.
0x0023c42279cdf718 in ?? ()

Thread 1 "curl" received signal SIGSEGV, Segmentation fault.
0x0078cdaa17700aa4 in ?? ()

Revision history for this message
Eyal Itkin (eitkin) wrote :
Download full text (4.2 KiB)

Hi,

Sorry for interrupting your thread, this bug has been prioritized on our end (I work at NVIDIA) so I joined the triaging effort and I believe I found the root cause for the crash. For the record, I used wget, but it behaves the same to curl.

First of all, it seems that it isn't that far off from an old case - https://openssl-dev.openssl.narkive.com/97zf5qT2/what-is-the-really-proper-way-to-use-an-engine. The reason being that the configuration file is loaded twice:

#0 0x0000fffff7c80304 in CONF_modules_load_file () from /lib/aarch64-linux-gnu/libcrypto.so.1.1
#1 0x0000fffff7c805e8 in ?? () from /lib/aarch64-linux-gnu/libcrypto.so.1.1
#2 0x0000fffff7cfa03c in ?? () from /lib/aarch64-linux-gnu/libcrypto.so.1.1
#3 0x0000fffff79dadd4 in __pthread_once_slow (once_control=0xfffff7e44550, init_routine=0xfffff7cfa020) at pthread_once.c:116
#4 0x0000fffff7d434c4 in CRYPTO_THREAD_run_once () from /lib/aarch64-linux-gnu/libcrypto.so.1.1
#5 0x0000fffff7cfa608 in OPENSSL_init_crypto () from /lib/aarch64-linux-gnu/libcrypto.so.1.1
#6 0x0000fffff7c80578 in OPENSSL_config () from /lib/aarch64-linux-gnu/libcrypto.so.1.1
#7 0x0000aaaaaaae9e24 in ?? ()
#8 0x0000aaaaaaacfbe0 in ?? ()
#9 0x0000aaaaaaad29cc in ?? ()
#10 0x0000aaaaaaadde5c in ?? ()
#11 0x0000aaaaaaab8874 in ?? ()

And later again (practically in the same function):

#0 0x0000fffff7c80304 in CONF_modules_load_file () from /lib/aarch64-linux-gnu/libcrypto.so.1.1
#1 0x0000aaaaaaae9e68 in ?? ()
#2 0x0000aaaaaaacfbe0 in ?? ()
#3 0x0000aaaaaaad29cc in ?? ()
#4 0x0000aaaaaaadde5c in ?? ()
#5 0x0000aaaaaaab8874 in ?? ()

This means that the engine parsing and creation flow is done twice, which breaks several assumptions of both OpenSSL and the loaded engines:

1. Parsing the name "pka"
2. Creating a dynamic engine
3. Cloning the dynamic engine inside ENGINE_by_id()
3. Using the dynamic path to call the "bind" method of the loaded .so file (pka.so)
4. Populating the fields of the cloned dynamic engine on top of what the user still holds as ("e"):
        else if (strcmp(ctrlname, "dynamic_path") == 0) {
            e = ENGINE_by_id("dynamic");
5. dynamic_load() will try to once again ENGINE_add() the engine, but it is already in the engine list
6. The Add will fail, and the reference count won't be incremented, hence will stay at 1
7. The error will propagate and ENGINE_free() will get called because the load failed:
            if (!ENGINE_ctrl_cmd_string(e, "LOAD", NULL, 0))
                goto err;
8. The free()ed engine is actually the PKA engine which got created twice, and the destruction of the engine (which assumes it only get created/destroyed once) will free memory pointed by static variables inside of the PKA module.
9. The first PKA engine is still "alive" and when used it will access free()ed memory pointed by the static variables of PKA
10. We get a BUS error when invoking a function using a garbled fptr which got overridden because it is stored in a free()ed memory buffer. On ARM, BUS error is very common because the CPU can't execute commands from misaligned addresses, and with high probability the garbled value isn't divisible by 4.

It seems that several fixes could...

Read more...

Revision history for this message
Eyal Itkin (eitkin) wrote :

While trying to understand why a fix in PKA that guards against multiple destroys (https://github.com/Mellanox/pka/pull/37/files) didn't bypass this issue, I found the following.

bind() operation of engines is expected to populate the pmeths and ameths of an existing engine (https://github.com/gost-engine/engine/blob/df3ead272bd2019f98d16e6787f5df51556c0603/gost_eng.c#L375, https://github.com/Mellanox/pka/blob/master/engine/e_bluefield.c#L1615). This means that the engine uses EVP_PKEY_meth_new (for instance) as part of this registration.

However, on teardown, OpenSSL's engine_free_util() is invoking engine_pkey_meths_free() and engine_pkey_asn1_meths_free(). Both of which iterate the list of registered methods, and invoke EVP_PKEY_meth_free() on each on of them. Only after OpenSSL freed these methods it calls the engine's destroy() method which allows the registered engine to do its own cleanup.

As long as this design is used, an engine using pkey methods can't protect itself against multiple destroy operations, because OpenSSL is the one freeing it's methods and there isn't much the engine can do about it. For future versions, it might be recommended to update this API and grant the engine the ownership on clearing up the memory that it allocated on the first place.

Revision history for this message
Vladimir Sokolovsky (vlad-a) wrote :

Dmitrii,
Any update?

tags: added: fr-1852
Revision history for this message
Julian Andres Klode (juliank) wrote :

So my understanding from #34 and #35 is that this is an upstream OpenSSL issue, that should be discussed with the OpenSSL people.

The feedback in #34 suggests that this problem can be solved by not parsing the configuration file twice, I have not investigated that as of yet.

The feedback in #35 suggests a solution for duplicate destruction handling that requires an API change which is again a discussion best had with OpenSSL upstream, and something that won't be applicable to existing releases, only a future OpenSSL version that breaks API.

With regards to possible workarounds with the OpenSSL API as is:

1. I don't fully understand if we get two different ENGINE* pointers, if that is the case, the PKA engine could work around this by avoid static singletons and instead store all globals in a struct and do something like

struct engine_private {
 ENGINE *engine;
 EVP_PKEY_METHOD *engine_pka_pmeth_X25519;
 EVP_PKEY_METHOD *engine_pka_pmeth_X448;
 EVP_PKEY_ASN1_METHOD *engine_pka_ameth_X25519;
 EVP_PKEY_ASN1_METHOD *engine_pka_ameth_X448;
}

keeping a dynamic array or linked list of that, and then looking up the one for the correct engine when we get asked to return by OpenSSL in engine_pka_pkey_meths and engine_pka_X25519_keygen().

2. It might also be worth to see if those methods could be allocated statically instead of using _new, from what I see, the deletion then becomes a no-op, leaving only the variables in destroy() to worry about. Since they are not really linked to the ENGINE AFAICT, it seems one could implement a reference count for them.

3. There certainly are ways we could "hack" around the issue by setting an external setting somewhere when retrieving the pmeth and ameth to say that the value returned will be invalidated, currently openssl loops over all pkey meths and does

if (e->pkey_meths(e, &pkm, NULL, pknids[i]))
{
   EVP_PKEY_meth_free(pkm);
}

It could be hacked into something like

setenv("UBUNTU_PKEY_METH_TO_BE_INVALIDATED", "1");
if (e->pkey_meths(e, &pkm, NULL, pknids[i]))
{
   unsetenv("UBUNTU_PKEY_METH_TO_BE_INVALIDATED");
   EVP_PKEY_meth_free(pkm);
}

But this is arguably the worst of all possible workarounds.

Revision history for this message
Eyal Itkin (eitkin) wrote (last edit ):

Loading the configuration only once will resolve this issue, and is the recommended code fix.

On top of this bug fix, and as mentioned above, we recommend that future versions will incorporate an API change that will shift the ownership on releasing the pointers to the engine that allocated them on the first place. We understand this is an API change and that it isn't relevant in the near future.

Regarding workarounds that will apply till OpenSSL fix their double loading of the configuration, as I mentioned in #35, we are at a loss here because OpenSSL is actively freeing the memory that the engine allocated. The struct ENGINE is an OpenSSL implementation detail, and is allocated in an OpenSSL function ENGINE_new(). I fail to see how a given engine can add fields to it / save its own context on top of it.

1. The engine library could maintain some Data structure with all "known" engine instances of itself, but OpenSSL guaranteed developers a singleton invocation. This is a drastic implementation change on behalf of the engine developers, and I doubt developers will go to this effort when they know it is temporary until OpenSSL fixes their code and restore back the singleton guarantee.

2. It is a bit trickier, because OpenSSL expect the memory to be initialized using EVP_PKEY_meth_new() and then the pointer to be set using EVP_PKEY_meth_set_derive(). Hence, they've taken the liberty to invoke EVP_PKEY_meth_free() and directly free the instance that was given to them. If all engine instances use the same static field, it will get freed by OpenSSL. If there will be an instance per engine, it throws us back to point (1) of maintaining a data structure of the additional engine metadata per instance.

3. In our deployment environments any environment variable based hack is out of the question.

As such, any temporary fix will be a hack, and will need to bypass the break of the singleton engine guarantee that was given to engine developers. Disabling the double parsing of the configuration, and double load of the engine is the code fix that should be done, and it should be done on OpenSSL's side.

Revision history for this message
Julian Andres Klode (juliank) wrote :

The original bug report stated

> OpenSSL version is 1.1.1f
>
> The issue is not encountered if http://www.openssl.org/source/openssl-1.1.1f.tar.gz is used instead.

Does that mean you believe the double parsing issue is in one of the patches in debian/patches in the Ubuntu package, and not an upstream bug? If so, could you bisect the patches and see which one causes the crash?

If not, please open an issue with OpenSSL upstream and link it here for tracking.

Revision history for this message
Julian Andres Klode (juliank) wrote :
Download full text (8.0 KiB)

Further analysis of why config files are being loaded twice shows that these are bugs in curl and wget, both call CONF_modules_load_file directly during their initialization functions, while it is also being called from OPENSSL_init_crypto and similar top-level functions:

For wget:

(gdb) bt
#0 CONF_modules_load_file (filename=filename@entry=0x0, appname=appname@entry=0x0, flags=flags@entry=50) at ../crypto/conf/conf_mod.c:114
#1 0x00007ffff7c7da10 in openssl_config_int (settings=<optimized out>) at ../crypto/conf/conf_sap.c:69
#2 0x00007ffff7d14234 in ossl_init_config () at ../crypto/init.c:293
#3 ossl_init_config_ossl_ () at ../crypto/init.c:291
#4 0x00007ffff796947f in __pthread_once_slow (once_control=0x7ffff7e717e0 <config>, init_routine=0x7ffff7d14220 <ossl_init_config_ossl_>) at pthread_once.c:116
#5 0x00007ffff7969535 in __GI___pthread_once (once_control=once_control@entry=0x7ffff7e717e0 <config>, init_routine=init_routine@entry=0x7ffff7d14220 <ossl_init_config_ossl_>) at pthread_once.c:143
#6 0x00007ffff7d7f74d in CRYPTO_THREAD_run_once (once=once@entry=0x7ffff7e717e0 <config>, init=init@entry=0x7ffff7d14220 <ossl_init_config_ossl_>) at ../crypto/threads_pthread.c:118
#7 0x00007ffff7d148b8 in OPENSSL_init_crypto (settings=0x7fffffffdc50, opts=64) at ../crypto/init.c:701
#8 OPENSSL_init_crypto (opts=opts@entry=64, settings=settings@entry=0x7fffffffdc50) at ../crypto/init.c:620
#9 0x00007ffff7c7d9ae in OPENSSL_config (appname=appname@entry=0x0) at ../crypto/conf/conf_sap.c:39
#10 0x0000555555598237 in ssl_init () at ../../src/openssl.c:178
#11 0x000055555557cbc5 in gethttp (u=u@entry=0x5555555e63e0, original_url=original_url@entry=0x5555555e63e0, hs=hs@entry=0x7fffffffe110, dt=dt@entry=0x7fffffffe4f0, proxy=proxy@entry=0x0, iri=iri@entry=0x5555555e63b0, count=1) at ../../src/http.c:3209
#12 0x00005555555808f3 in http_loop (u=u@entry=0x5555555e63e0, original_url=original_url@entry=0x5555555e63e0, newloc=newloc@entry=0x7fffffffe408, local_file=local_file@entry=0x7fffffffe410, referer=referer@entry=0x0, dt=dt@entry=0x7fffffffe4f0, proxy=0x0, iri=0x5555555e63b0)
    at ../../src/http.c:4356
#13 0x000055555558c594 in retrieve_url (orig_parsed=0x5555555e63e0, origurl=0x5555555e7600 "https://google.de/", file=0x7fffffffe4f8, newloc=0x7fffffffe500, refurl=0x0, dt=0x7fffffffe4f0, recursive=false, iri=0x5555555e63b0, register_status=true) at ../../src/retr.c:973
#14 0x00005555555644bb in main (argc=<optimized out>, argv=<optimized out>) at ../../src/main.c:2165
(gdb) c
Continuing.

Breakpoint 1, CONF_modules_load_file (filename=filename@entry=0x0, appname=appname@entry=0x0, flags=flags@entry=48) at ../crypto/conf/conf_mod.c:114
114 in ../crypto/conf/conf_mod.c
(gdb) bt
#0 CONF_modules_load_file (filename=filename@entry=0x0, appname=appname@entry=0x0, flags=flags@entry=48) at ../crypto/conf/conf_mod.c:114
#1 0x00005555555982c0 in ssl_init () at ../../src/openssl.c:202
#2 0x000055555557cbc5 in gethttp (u=u@entry=0x5555555e63e0, original_url=original_url@entry=0x5555555e63e0, hs=hs@entry=0x7fffffffe110, dt=dt@entry=0x7fffffffe4f0, proxy=proxy@entry=0x0, iri=iri@entry=0x5555555e63b0, count=1) at ../../src/http.c:3209
#3 0x00005555...

Read more...

Changed in curl (Ubuntu):
status: New → Fix Released
Changed in wget (Ubuntu):
status: New → Fix Released
Changed in wget (Ubuntu Focal):
status: New → Triaged
Revision history for this message
Julian Andres Klode (juliank) wrote :

In wget, this was fixed upstream in

commit 14e3712b8c39165219fa227bd11f6feae7b09a33
Author: Eneas U de Queiroz <email address hidden>
Date: Mon Apr 22 11:03:25 2019 -0300

    * src/openssl.c: fix ssl_init for openssl 1.1.1

    ssl_init fails with openssl 1.1.1 when openssl.cnf is not found.
    Redundant calls to intialization functions were removed as
    OPENSSL_config takes care of them for openssl versions < 1.1.0.
    For versions > 1.1.0, OPENSSL_init_ssl is preferred.

    Signed-off-by: Eneas U de Queiroz <email address hidden>
    Copyright-paperwork-exempt: Yes

diff --git a/src/openssl.c b/src/openssl.c
index a1502173..03737d7a 100644
--- a/src/openssl.c
+++ b/src/openssl.c
@@ -174,7 +174,9 @@ ssl_init (void)
 #if OPENSSL_VERSION_NUMBER >= 0x00907000
   if (ssl_true_initialized == 0)
     {
-#if OPENSSL_API_COMPAT < 0x10100000L
+#if !defined(LIBRESSL_VERSION_NUMBER) && (OPENSSL_VERSION_NUMBER >= 0x10100000L)
+ OPENSSL_init_ssl (OPENSSL_INIT_LOAD_CONFIG | OPENSSL_INIT_ENGINE_ALL_BUILTIN, NULL);
+#else
       OPENSSL_config (NULL);
 #endif
       ssl_true_initialized = 1;
@@ -194,21 +196,9 @@ ssl_init (void)
       goto error;
     }

-#if OPENSSL_VERSION_NUMBER >= 0x00907000
- OPENSSL_load_builtin_modules();
-#ifndef OPENSSL_NO_ENGINE
- ENGINE_load_builtin_engines();
-#endif
- CONF_modules_load_file(NULL, NULL,
- CONF_MFLAGS_DEFAULT_SECTION|CONF_MFLAGS_IGNORE_MISSING_FILE);
-#endif
-#if OPENSSL_API_COMPAT >= 0x10100000L
- OPENSSL_init_ssl(0, NULL);
-#else
+#if defined(LIBRESSL_VERSION_NUMBER) || (OPENSSL_VERSION_NUMBER < 0x10100000L)
   SSL_library_init ();
   SSL_load_error_strings ();
-#endif
-#if OPENSSL_VERSION_NUMBER < 0x10100000L
   SSLeay_add_all_algorithms ();
   SSLeay_add_ssl_algorithms ();
 #endif

Changed in curl (Ubuntu Focal):
status: New → Triaged
Revision history for this message
Julian Andres Klode (juliank) wrote :

I have uploaded a fixed wget for focal, verified that it only loads the config file once.

description: updated
Changed in wget (Ubuntu Focal):
status: Triaged → In Progress
Revision history for this message
Julian Andres Klode (juliank) wrote :

There are possibly more applications with broken initialization that need fixes, we will run a search for CONF_modules_load_file in all Ubuntu packages to hopefully "catch them all".

Revision history for this message
Julian Andres Klode (juliank) wrote :

The fix for curl is being tracked in bug 1940528

no longer affects: curl (Ubuntu)
no longer affects: curl (Ubuntu Focal)
Revision history for this message
Steve Langasek (vorlon) wrote :

How will you test that the change does not regress any wget behavior?

Changed in wget (Ubuntu Focal):
status: In Progress → Incomplete
Revision history for this message
Dimitri John Ledkov (xnox) wrote :

> How will you test that the change does not regress any wget behavior?

In default Ubuntu configuration, either no openssl configuration is provided, or it contains no settings that affect wget. This code path changes how/when openssl configuration is loaded and used by openssl. One should verify that:
1) wget continues to work without openssl.cnf
2) wget continues to work with stock ubuntu unmodified openssl.cnf
3) wget continue to honor and use custom TLS settings that one may have specified in openssl.cnf (for example custom engine)

The above three test scenarios cover the changes or regressions that may occur from applying the proposed patch.

(ps. similar for the curl SRU as well)

description: updated
Changed in wget (Ubuntu Focal):
status: Incomplete → In Progress
Revision history for this message
Łukasz Zemczak (sil2100) wrote : Please test proposed package

Hello Mahantesh, or anyone else affected,

Accepted wget into focal-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/wget/1.20.3-1ubuntu2 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-focal to verification-done-focal. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-focal. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

Changed in wget (Ubuntu Focal):
status: In Progress → Fix Committed
tags: added: verification-needed verification-needed-focal
Revision history for this message
Seth Arnold (seth-arnold) wrote :
Revision history for this message
Julian Andres Klode (juliank) wrote :

I have done preliminary analysis of the grep and filed additional bugs for

- bind9 and bind9-libs (LP: #1951097)
- freeradius (LP: #1951099)
- librabbitmq (LP: #1951102)

Revision history for this message
Vladimir Sokolovsky (vlad-a) wrote :

The fix was verified for wget and curl.
Thanks a lot.

When these new packages will be added to "updates" repository?

Revision history for this message
Brian Murray (brian-murray) wrote : Re: [Bug 1921518] Re: OpenSSL "double free" error

On Tue, Nov 16, 2021 at 03:24:21PM -0000, Vladimir Sokolovsky wrote:
> The fix was verified for wget and curl.
> Thanks a lot.
>
> When these new packages will be added to "updates" repository?

There is a minimum aging period of 7 days in -proposed and shortly after
that they will migrate to -updates. (Provided that day isn't a Friday.)

--
Brian Murray

Mathew Hodson (mhodson)
no longer affects: openssl (Ubuntu Focal)
no longer affects: openssl (Ubuntu)
Revision history for this message
Eyal Itkin (eitkin) wrote :

following my request, OpenSSL just integrated a fix to avoid loading an engine twice even if the configuration is parsed more than once: https://github.com/openssl/openssl/commit/9b06ebb1edfddffea083ba36090af7eb7cad207b

Integrating this patch in the existing OpenSSL 1.1.1 package (or at least packaging the relevant OpenSSL 1.1.1 version that will include it) will ensure that no additional project will crash if it uses an engine (such as PKA) and the configuration is parsed twice.

In the long term, this aims to be a robust solution to this double-load issue, so that instead of playing whack-a-mole on all 3rd party projects that might load the config twice, the issue will be resolved at OpenSSL itself.

Revision history for this message
Dimitri John Ledkov (xnox) wrote :

Thank you for working with OpenSSL upstream, explaining the issue at hand, for everyone to eventually understand what is going on, and finally coming up with a solution on the OpenSSL side of the APIs that is accepted by upstream into development v3 branch and stable 1.1.1 branch.

I have started paperwork to pick up these changes at https://bugs.launchpad.net/ubuntu/+source/openssl/+bug/1951943

As far as I can tell it would be desirable to ship in 5 current Ubuntu stable series, hence using a new bug to track landing those updates.

Revision history for this message
Julian Andres Klode (juliank) wrote (last edit ):

for 1.20.3-1ubuntu2 in focal:

I have verified the configuration file is only loaded once, and 1) and 2) but 3) I did not manage to do. I tried this before the SRU with like setting min TLS to 1.3 and check it's respected, but that did nothing, and I don't have a custom engine handy that I could check is working wrt wget.

Maybe someone else can verify that or point me at a guide. The "The fix was verified for wget and curl." statement is unfortunately not sufficient, it does not mention the version tested nor the testing procedure.

Revision history for this message
Eyal Itkin (eitkin) wrote :

The wget package that was tested and approved on our setup (using PKA 1.3 engine) is the one you declared above - 1.20.3-1ubuntu2. The tests were basic functionality tests for wget, including debugging to verify that the engine is loaded exactly once.

Same for curl (exactly the same procedure).

Revision history for this message
Julian Andres Klode (juliank) wrote :

Marking as verification-done, I'm happy with the described test procedure to fulfill 3) (arguably all of it :D)

tags: added: verification-done verification-done-focal
removed: verification-needed verification-needed-focal
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package wget - 1.20.3-1ubuntu2

---------------
wget (1.20.3-1ubuntu2) focal; urgency=medium

  * Cherry-pick upstream fix fix-ssl_init-for-openssl-1.1.1.patch:
    - Fix initialization of openssl for 1.1.1 (LP: #1921518)

 -- Julian Andres Klode <email address hidden> Fri, 12 Nov 2021 18:09:16 +0100

Changed in wget (Ubuntu Focal):
status: Fix Committed → Fix Released
Revision history for this message
Łukasz Zemczak (sil2100) wrote : Update Released

The verification of the Stable Release Update for wget has completed successfully and the package is now being released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

To post a comment you must log in.
This report contains Public Security information  
Everyone can see this security related information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.