Segfault on Fedora 36 due to vendored openssl

[thomasjfox]

New crev user here. I was following the "Getting started" guide and instantly got a segfault on cargo crev verify as soon as it started to download any crate. Strangely downloading the trust data worked fine like in

cargo crev trust --level high https://github.com/dpc/crev-proofs
cargo crev repo fetch all

Platform is Fedora 36 Linux.

$ rpm -q rust openssl openssl-devel
rust-1.63.0-1.fc36.x86_64
openssl-3.0.5-1.fc36.x86_64
openssl-devel-3.0.5-1.fc36.x86_64

So I went down the rabbit hole and got this nice backtrace (skip to the end for a solution):

Thread 1 "cargo-crev" received signal SIGSEGV, Segmentation fault.
0x00007ffff781c5f8 in OPENSSL_sk_value (st=0x5555575bfa50 <ssl_session_hash>, i=0) at crypto/stack/stack.c:427
427         return (void *)st->data[i];
(gdb) bt
#0  0x00007ffff781c5f8 in OPENSSL_sk_value (st=0x5555575bfa50 <ssl_session_hash>, i=0) at crypto/stack/stack.c:427
#1  0x00007ffff783ea1a in X509_STORE_add_lookup (v=v@entry=0x55555beec4e0, m=0x7ffff7a414a0 <x509_file_lookup>) at crypto/x509/x509_lu.c:272
#2  0x00007ffff78452ff in X509_STORE_load_file_ex (ctx=0x55555beec4e0, file=file@entry=0x555559a81de0 "/etc/pki/tls/certs/ca-bundle.crt", 
    libctx=0x555558655c00 <TLS_client_method_data.14>, propq=0x555559152f70 "alloc\177") at crypto/x509/x509_d2.c:51
#3  0x00007ffff7a7a397 in SSL_CTX_load_verify_file (ctx=<optimized out>, CAfile=CAfile@entry=0x555559a81de0 "/etc/pki/tls/certs/ca-bundle.crt")
    at ssl/ssl_lib.c:4411
#4  0x00007ffff7f463b7 in ossl_connect_step1 (data=data@entry=0x55555c036c90, conn=conn@entry=0x55555a6e7ba0, sockindex=sockindex@entry=0)
    at ../../lib/vtls/openssl.c:3085
#5  0x00007ffff7f4c1c7 in ossl_connect_common (data=0x55555c036c90, conn=0x55555a6e7ba0, sockindex=0, nonblocking=true, done=0x7ffffffdeb52)
    at ../../lib/vtls/openssl.c:4118
#6  0x00007ffff7f48bd9 in Curl_ssl_connect_nonblocking (done=0x7ffffffdeb52, sockindex=0, isproxy=false, conn=0x55555a6e7ba0, data=0x55555c036c90)
    at ../../lib/vtls/vtls.c:375
#7  Curl_ssl_connect_nonblocking (data=0x55555c036c90, conn=0x55555a6e7ba0, isproxy=<optimized out>, sockindex=0, done=0x7ffffffdeb52)
    at ../../lib/vtls/vtls.c:358
#8  0x00007ffff7eff6d6 in https_connecting (done=<optimized out>, data=<optimized out>) at ../../lib/http.c:1595
#9  Curl_http_connect (data=0x55555c036c90, done=0x7ffffffdeb52) at ../../lib/http.c:1521
#10 0x00007ffff7f1f006 in protocol_connect (protocol_done=0x7ffffffdeb52, data=0x55555c036c90) at ../../lib/multi.c:1728
#11 multi_runsingle (multi=multi@entry=0x55555a84ec80, nowp=nowp@entry=0x7ffffffdec10, data=data@entry=0x55555c036c90) at ../../lib/multi.c:2044
#12 0x00007ffff7f2157e in curl_multi_perform (multi=0x55555a84ec80, running_handles=0x7ffffffded34) at ../../lib/multi.c:2637
#13 0x0000555556d2d5af in curl::multi::Multi::perform (self=0x7ffffffe27c8) at src/multi.rs:728
#14 0x00005555564fad6a in cargo::core::package::{impl#9}::wait_for_curl::{closure#0} () at src/cargo/core/package.rs:974
#15 0x00005555566df1ac in cargo::core::package::tls::set::{closure#0}<core::result::Result<u32, anyhow::Error>, cargo::core::package::{impl#9}::wait_for_curl::{closure_env#0}> (p=0x7ffff70b5a78) at src/cargo/core/package.rs:1184
#16 0x000055555694e28f in std::thread::local::LocalKey<core::cell::Cell<usize>>::try_with<core::cell::Cell<usize>, cargo::core::package::tls::set::{closure_env#0}<core::result::Result<u32, anyhow::Error>, cargo::core::package::{impl#9}::wait_for_curl::{closure_env#0}>, core::result::Result<u32, anyhow::Error>> (
    self=0x5555585f8a58, f=...) at /builddir/build/BUILD/rustc-1.63.0-src/library/std/src/thread/local.rs:445
#17 0x000055555694ded7 in std::thread::local::LocalKey<core::cell::Cell<usize>>::with<core::cell::Cell<usize>, cargo::core::package::tls::set::{closure_env#0}<core::result::Result<u32, anyhow::Error>, cargo::core::package::{impl#9}::wait_for_curl::{closure_env#0}>, core::result::Result<u32, anyhow::Error>> (
    self=0x5555585f8a58, f=...) at /builddir/build/BUILD/rustc-1.63.0-src/library/std/src/thread/local.rs:421
#18 0x00005555566df0c6 in cargo::core::package::tls::set<core::result::Result<u32, anyhow::Error>, cargo::core::package::{impl#9}::wait_for_curl::{closure_env#0}> (dl=0x7ffffffe0010, f=...) at src/cargo/core/package.rs:1181
#19 0x00005555564fa863 in cargo::core::package::Downloads::wait_for_curl (self=0x7ffffffe0010) at src/cargo/core/package.rs:973
#20 0x00005555564f8017 in cargo::core::package::Downloads::wait (self=0x7ffffffe0010) at src/cargo/core/package.rs:825
#21 0x00005555564f5205 in cargo::core::package::PackageSet::get_many<core::option::Option<cargo::core::package_id::PackageId>> (self=0x7ffffffe2758, 
    ids=...) at src/cargo/core/package.rs:479
#22 0x00005555564f4cd5 in cargo::core::package::PackageSet::get_one (self=0x7ffffffe2758, id=...) at src/cargo/core/package.rs:469
#23 0x0000555555a7ea08 in cargo_crev::repo::build_graph<alloc::vec::into_iter::IntoIter<cargo::core::package_id::PackageId, alloc::alloc::Global>> (
    resolve=0x7ffffffe27e0, packages=0x7ffffffe2758, roots=..., target=..., cfgs=...) at cargo-crev/src/repo.rs:215
#24 0x0000555555938913 in cargo_crev::repo::Repo::get_dependency_graph (self=0x7ffffffe5180, roots=...) at cargo-crev/src/repo.rs:395
#25 0x000055555595afe0 in cargo_crev::deps::scan::Scanner::new (root_crate=..., args=0x7fffffffb8a8) at cargo-crev/src/deps/scan.rs:128
#26 0x0000555555a6063e in cargo_crev::deps::verify_deps (crate_=..., args=...) at cargo-crev/src/deps.rs:321
#27 0x0000555555a25b61 in cargo_crev::run_command (command=...) at cargo-crev/src/main.rs:670
#28 0x0000555555a1406e in cargo_crev::main::{closure#1} () at cargo-crev/src/main.rs:989
#29 0x0000555555a14668 in cargo_crev::handle_command_result_and_panics::{closure#1}<cargo_crev::main::{closure_env#1}> () at cargo-crev/src/main.rs:1017
#30 0x00005555559f2be0 in std::panicking::try::do_call<cargo_crev::handle_command_result_and_panics::{closure_env#1}<cargo_crev::main::{closure_env#1}>, ()>
    (data=0x7fffffffc408) at /builddir/build/BUILD/rustc-1.63.0-src/library/std/src/panicking.rs:492
#31 0x00005555559f345b in __rust_try ()
#32 0x00005555559f1fe7 in std::panicking::try<(), cargo_crev::handle_command_result_and_panics::{closure_env#1}<cargo_crev::main::{closure_env#1}>> (f=...)
    at /builddir/build/BUILD/rustc-1.63.0-src/library/std/src/panicking.rs:456
#33 0x0000555555a37a03 in std::panic::catch_unwind<cargo_crev::handle_command_result_and_panics::{closure_env#1}<cargo_crev::main::{closure_env#1}>, ()> (
    f=...) at /builddir/build/BUILD/rustc-1.63.0-src/library/std/src/panic.rs:137
#34 0x0000555555a14216 in cargo_crev::handle_command_result_and_panics<cargo_crev::main::{closure_env#1}> (f=...) at cargo-crev/src/main.rs:1017
#35 0x0000555555a3618d in cargo_crev::main () at cargo-crev/src/main.rs:989

It looks like this in valgrind:

==112589== Invalid read of size 8
==112589==    at 0x4D60384: SSL_CTX_load_verify_file (ssl_lib.c:4411)
==112589==    by 0x490F3B6: ossl_connect_step1 (openssl.c:3085)
==112589==    by 0x49151C6: ossl_connect_common (openssl.c:4118)
==112589==    by 0x4911BD8: UnknownInlinedFun (vtls.c:375)
==112589==    by 0x4911BD8: Curl_ssl_connect_nonblocking (vtls.c:358)
==112589==    by 0x48C86D5: UnknownInlinedFun (http.c:1595)
==112589==    by 0x48C86D5: Curl_http_connect (http.c:1521)
==112589==    by 0x48E8005: UnknownInlinedFun (multi.c:1728)
==112589==    by 0x48E8005: multi_runsingle (multi.c:2044)
==112589==    by 0x48EA57D: curl_multi_perform (multi.c:2637)
==112589==    by 0x18E15AE: curl::multi::Multi::perform (multi.rs:728)
==112589==    by 0x10AED69: cargo::core::package::Downloads::wait_for_curl::{{closure}} (package.rs:974)
==112589==    by 0x12931AB: cargo::core::package::tls::set::{{closure}} (package.rs:1184)
==112589==    by 0x150228E: std::thread::local::LocalKey<T>::try_with (local.rs:445)
==112589==    by 0x1501ED6: std::thread::local::LocalKey<T>::with (local.rs:421)
==112589==  Address 0x815e130 is 0 bytes inside an unallocated block of size 1,008 in arena "client"
==112589== 
==112589== Invalid read of size 8
==112589==    at 0x4FCD5F8: OPENSSL_sk_value (stack.c:427)
==112589==    by 0x4FEFA19: X509_STORE_add_lookup (x509_lu.c:272)
==112589==    by 0x4FF62FE: X509_STORE_load_file_ex (x509_d2.c:51)
==112589==    by 0x490F3B6: ossl_connect_step1 (openssl.c:3085)
==112589==    by 0x49151C6: ossl_connect_common (openssl.c:4118)
==112589==    by 0x4911BD8: UnknownInlinedFun (vtls.c:375)
==112589==    by 0x4911BD8: Curl_ssl_connect_nonblocking (vtls.c:358)
==112589==    by 0x48C86D5: UnknownInlinedFun (http.c:1595)
==112589==    by 0x48C86D5: Curl_http_connect (http.c:1521)
==112589==    by 0x48E8005: UnknownInlinedFun (multi.c:1728)
==112589==    by 0x48E8005: multi_runsingle (multi.c:2044)
==112589==    by 0x48EA57D: curl_multi_perform (multi.c:2637)
==112589==    by 0x18E15AE: curl::multi::Multi::perform (multi.rs:728)
==112589==    by 0x10AED69: cargo::core::package::Downloads::wait_for_curl::{{closure}} (package.rs:974)
==112589==    by 0x12931AB: cargo::core::package::tls::set::{{closure}} (package.rs:1184)
==112589==  Address 0x834800000158b78d is not stack'd, malloc'd or (recently) free'd
==112589== 
==112589== 
==112589== Process terminating with default action of signal 11 (SIGSEGV): dumping core
==112589==  Access not within mapped region at address 0x50978B48
==112589==    at 0x4FCD5F8: OPENSSL_sk_value (stack.c:427)
==112589==    by 0x4FEFA19: X509_STORE_add_lookup (x509_lu.c:272)
==112589==    by 0x4FF62FE: X509_STORE_load_file_ex (x509_d2.c:51)
==112589==    by 0x490F3B6: ossl_connect_step1 (openssl.c:3085)
==112589==    by 0x49151C6: ossl_connect_common (openssl.c:4118)
==112589==    by 0x4911BD8: UnknownInlinedFun (vtls.c:375)
==112589==    by 0x4911BD8: Curl_ssl_connect_nonblocking (vtls.c:358)
==112589==    by 0x48C86D5: UnknownInlinedFun (http.c:1595)
==112589==    by 0x48C86D5: Curl_http_connect (http.c:1521)
==112589==    by 0x48E8005: UnknownInlinedFun (multi.c:1728)
==112589==    by 0x48E8005: multi_runsingle (multi.c:2044)
==112589==    by 0x48EA57D: curl_multi_perform (multi.c:2637)
==112589==    by 0x18E15AE: curl::multi::Multi::perform (multi.rs:728)
==112589==    by 0x10AED69: cargo::core::package::Downloads::wait_for_curl::{{closure}} (package.rs:974)
==112589==    by 0x12931AB: cargo::core::package::tls::set::{{closure}} (package.rs:1184)

This made me dig into the openssl code (I'm familiar with it a bit), but nothing obviously suspicious turned up.

While inspecting Cargo.toml I noticed it includes a vendored openssl build. Oh my.

So I disabled that one and voila, crev verify finally works by magic several hours later.

The problem is that the vendored openssl is version 1.1.1 while F36 ships openssl 3.0.0. This doesn't mix well at all if other crates link the system wide openssl. Openssl loads further "engines" shared libraries dynamically at runtime.

Compiled with the vendored openssl feature:

$ strings target/debug/cargo-crev |grep -i openssl |grep 1.1.1
OpenSSL 1.1.1q  5 Jul 2022

Proposal: Disable the vendored openssl builds on Linux platforms by default. Security updates from the distro will come for free, too.

[dpc]

Fine with me. Can we do the default per platform? Relevant: #401

[thomasjfox]

According to rust-lang/cargo#1197 it seems possible now:
Example:

[target."cfg(target_os=linux)".dependencies]
dep1 = {version = x, optional = true}

A quick grep shows it's in use by the Rust compiler itself f.e. in rustc/src/tools/miri/Cargo.toml:

[target."cfg(unix)".dependencies]
libc = "0.2"

I'll experiment with it tomorrow. Might need Rust 2021 edition to enable "cargo resolver v2" by default.

[thomasjfox]

Unfortunately cargo will merge all features from target specific dependencies into one big graph.

I tried two different approaches:

@@ -51,7 +51,6 @@ structopt = "0.3.25"
 time = "0.3.6"
 tokei = "12.1.2"
 walkdir = "2.3.2"
-openssl-sys = "0.9.65"
 git2 = ">=0.13, <15"
 tempfile = "3.3.0"
 rprompt = "1.0.5"
@@ -61,8 +60,14 @@ term = "0.7.0"
 syn-inline-mod = "0.5.0"
 quote = "1.0.20"
 
+[target."cfg(windows)".dependencies.openssl-sys]
+version = "0.9.65"
+features = ["vendored"]
+
+[target."cfg(unix)".dependencies.openssl-sys]
+version = "0.9.65"
+
 [features]
-default = ["openssl-sys/vendored"]

As this didn't work, I thought may be I can outsmart it by moving the "vendored" feature flag into an own dependency inside cargo-crev:

--- a/cargo-crev/Cargo.toml
+++ b/cargo-crev/Cargo.toml
@@ -61,8 +61,10 @@ term = "0.7.0"
 syn-inline-mod = "0.5.0"
 quote = "1.0.20"
 
+[target.'cfg(target_os = "windows")'.dependencies]
+vendorize-openssl = { version = "0.0.1", path = "vendorize-openssl" }
+
 [features]
-default = ["openssl-sys/vendored"]

$ cat vendorize-openssl/Cargo.toml 
[package]
name = "vendorize-openssl"
version = "0.0.1"
edition = "2021"

[lib]

[dependencies]
openssl-sys = { version = "0.9.65", features = ["vendored"] }

One can see in the cargo tree output that as soon as the vendorize-openssl is mentioned, the openssl-src crate gets included in all dependencies using openssl-sys:

│   │   ├── crypto-hash v0.3.4
│   │   │   ├── hex v0.3.2
│   │   │   └── openssl v0.10.41
│   │   │       ├── bitflags v1.3.2
│   │   │       ├── cfg-if v1.0.0
│   │   │       ├── foreign-types v0.3.2
│   │   │       │   └── foreign-types-shared v0.1.1
│   │   │       ├── libc v0.2.131
│   │   │       ├── once_cell v1.13.0
│   │   │       ├── openssl-macros v0.1.0 (proc-macro)
│   │   │       │   ├── proc-macro2 v1.0.43 (*)
│   │   │       │   ├── quote v1.0.21 (*)
│   │   │       │   └── syn v1.0.99 (*)
│   │   │       └── openssl-sys v0.9.75
│   │   │           └── libc v0.2.131
│   │   │           [build-dependencies]
│   │   │           ├── autocfg v1.1.0
│   │   │           ├── cc v1.0.73
│   │   │           │   └── jobserver v0.1.24
│   │   │           │       └── libc v0.2.131
│   │   │           ├── openssl-src v111.22.0+1.1.1q
..

With the current cargo feature set this seems unsolvable. There are hacks using build.rs and stuff, but that's rather unclean.

The solution I can think of right now is to disable openssl vendoring by default and enable it for the official Windows builds explicitly.

Other ideas?

[Kixunil]

Oh, so it turns out my suggestion in #488 is already implemented. 🤔

This doesn't mix well at all if other crates link the system wide openssl.

How could this happen? Cargo doesn't allow linking same library multiple times and if multiple crates depend on openssl-sys then turning on vendored should turn it on for all of them so all should link statically.