Tarsnap / kivaloo

Kivaloo is a collection of utilities which together form a data store associating keys of up to 255 bytes with values of up to 255 bytes.
http://www.tarsnap.com/kivaloo.html
Other
201 stars 17 forks source link

network_ssl_compat.h needs updating for LibreSSL 2.9+ (I think?) #236

Closed cperciva closed 3 years ago

cperciva commented 3 years ago

The function SSL_set1_host is defined in LibreSSL 2.9 and later. I'm not 100% certain but I think we need to turn off some of the compatibility code to avoid symbol conflicts there?

gperciva commented 3 years ago

Urgh, there's a whole host of iffiness with libressl, freebsd, and compilers. For example, did you know that the clang linker doesn't obey LIBRARY_PATH, whereas gcc's linker does?

With libressl-2.9.0 and what I believe to be a properly configured ~/.profile, I can't compile with

make

but it's ok with

LDFLAGS="-L$HOME/.local/lib/" make

with only the warning

cc  -D_POSIX_C_SOURCE=200809L -D_XOPEN_SOURCE=700 -DCPUSUPPORT_CONFIG_FILE=\"cpusupport-config.h\"  -I../.. -I ../../libcperciva/datastruct -I ../../libcperciva/util -I ../../libcperciva/events -I ../../libcperciva/network -I ../../lib/network_ssl  -O2 -pipe -c ../../lib/network_ssl/network_ssl.c -o network_ssl.o
../../lib/network_ssl/network_ssl.c:25:9: warning: 
      'SSL_CTX_set_min_proto_version' macro redefined [-Wmacro-redefined]
#define SSL_CTX_set_min_proto_version network_ssl_compat_CTL_set_min_pro...
        ^
/home/td/.local/include/openssl/ssl.h:1224:9: note: previous definition is here
#define SSL_CTX_set_min_proto_version   SSL_CTX_set_min_proto_version

which I'll patch in a perhaps-unrelated PR.

If you installed it into /usr/local/lib via ports, then may the flying spaghetti monster have mercy on your soul. I could work on that scenario (inside virtualbox) if you want, or test kivaloo on a system which has no openssl at all (like openbsd).

What's the actual scenario that we're trying to support?

cperciva commented 3 years ago

I was inspired by

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=253846

which I fixed in

https://github.com/cperciva/bsdec2-image-upload/commit/8cd268bdda57428ae0da5a8a7d50c1ba665be091

The code in bsdec2-image-upload isn't exactly the same but given the fix which it needed I assumed that we would have issues here as well.

If it's not straightforward, don't worry about it; I'm not using libressl anywhere and if/when someone does use kivaloo on a system with libressl they can help us reproduce any issues.

gperciva commented 3 years ago

Looks like our compatibility layer takes care of it. Between the various headers and source files, the relevant sections are:

/*
 * LibreSSL claims to be OpenSSL 2.0, but (currently) has APIs compatible with
 * OpenSSL 1.0.1g.
 */
#ifdef LIBRESSL_VERSION_NUMBER
#undef OPENSSL_VERSION_NUMBER
#define OPENSSL_VERSION_NUMBER 0x1000107fL
#endif

/* Compatibility for OpenSSL pre-1.1.0 */
#if OPENSSL_VERSION_NUMBER < 0x10100000L
#define NETWORK_SSL_COMPAT_CHECK_HOSTNAME
#endif

#ifdef NETWORK_SSL_COMPAT_CHECK_HOSTNAME
#define SSL_set1_host network_ssl_compat_set1_host
#endif

#ifdef NETWORK_SSL_COMPAT_CHECK_HOSTNAME
/**
 * network_ssl_compat_set1_host(ssl, hostname):
 * Set expected name for hostname verification.
 *
 * COMPATIBILITY: Behave like SSL_set1_host().
 */
int
network_ssl_compat_set1_host(SSL * ssl, const char * hostname)
{
        X509_VERIFY_PARAM * param;

        param = SSL_get0_param(ssl);
        return (X509_VERIFY_PARAM_set1_host(param, hostname, strlen(hostname)));
}
#endif

libressl-2.9.0 defines it as:

        return X509_VERIFY_PARAM_set1_host(s->param, hostname, 0);

It's odd to be passing 0 as the string length, but x509_param_set_hosts_internal() does the strlen() if the length is 0. I don't know why this did it that way, and there's no code comments to explain it. :(

Anyway, at the moment our openssl compat code handles:

If we need to add extra steps for various libressl releases, we can do that, but of course that complicates things further.

cperciva commented 3 years ago

Ah, we're probably fine then -- the problem for bsdec2-image-upload was that SSL_set1_host was being defined twice, but since we preprocessor it to a different name there's no linkage issues there.