Replace OPENSSL_NO_SSL3_METHODs with dummies

[cem]

Summary

There are three symbols removed with OPENSSL_NO_SSL3_METHOD:

SSLv3_client_method
SSLv3_method
SSLv3_server_method

SSLv3 has been deprecated since 2015 (and broken since 2014: "POODLE"); it
should not have shipped in FreeBSD 11 (2016) or 12 (2018). No one should
use it, and if they must, they can use some implementation outside of base.

These symbols exist to request an explicit SSLv3 connection to a server.
There is no good reason for an application to link or invoke these symbols
instead of TLS_method(), et al (née SSLv23_method, et al). Applications
that do so have broken cryptography.

Define these symbols for some trite definition of ABI stability, but remove
the functionality again -- restore r361392.

Test Plan

I think there is a real chance OpenSSL 3.0.0 will slip too late for FreeBSD
13.0 and we should not wait on it to kill unsafe protocols in base to
protect our users anyway. TLS 1.0, 1.1, and 1.2 have been available for
decades. TLS 1.3 for a few years. There is no good excuse for exposing
users -- out of the box -- to a broken, 1996 algorithm design that has been
exposed as broken for six years.

readelf --syms shows the symbols are still present in versioned ELF ABI
sense (foo@OPENSSL...), but no longer visible to new linked programs
(foo@@OPENSSL...).

[gordon]

This feels heavy handed (abort and KILL). Would we be better to mirror the actual API and return NULL to indicate this doesn't work?

[jhb]

It's probably worth seeing what other distros do. I'd probably be inclined to just return NULL as well. Some old software might call all of these methods even if it ends up using TLS 1.x instead, so the abort might be overkill. (I ran into things like this with the deprecation warnings for old crypto since OpenSSL in stable/10 would just open /dev/crypto and try to create sessions for all algos even if it didn't end up using any of them).

FWIW, my Google skills were only able to find that Debian just removed the symbols outright in 2016. Perhaps @bjk has insight into what sort of stubs other distros have used?

[cem]

If you know of an example of some software that explicitly invokes every version it knows about (and tries SSLv3 before a TLS version), I've love to see an example.

I'd take returning NULL over the status quo, where we ship SSLv3 in 2020.

[gordon]

In D25493#563618, @cem wrote:

If you know of an example of some software that explicitly invokes every version it knows about (and tries SSLv3 before a TLS version), I've love to see an example.

I'd take returning NULL over the status quo, where we ship SSLv3 in 2020.

It's probably somewhat unfair, but I wrote an SSL scanner a while ago that uses this behavior:
https://github.com/tetlowgm/sslscan

Also this change would, depending on the arguments, cause the openssl commandline to abort.

[cem]

In D25493#563646, @gordon wrote:

It's probably somewhat unfair, but I wrote an SSL scanner a while ago that uses this behavior:
https://github.com/tetlowgm/sslscan

It is nearly ifdef'd out, if we had shipped OPENSSL_NO_SSL3 earlier (r361392): https://github.com/tetlowgm/sslscan/blob/master/sslscan.c#L248

#if defined(SSL_TXT_SSLV3) && !defined(OPENSSL_NO_SSL3)
	SCAN_PROTO(SSLv3);
#endif

Do you intend to MFC OPENSSL_NO_SSL3?

Also this change would, depending on the arguments, cause the openssl commandline to abort.

I do not see any direct invocations of SSL3_method/SSL3_client_method/SSL3_server_method in the CLI, which is all that is impacted by this change. The internal SSL3 machinery is not changed, and OPENSSL_NO_SSL3 already disables some portions of the CLI.

Anyway, it is fine by me to just return NULL if that is preferred.

[gordon]

In D25493#563650, @cem wrote:

Do you intend to MFC OPENSSL_NO_SSL3?

This is a good question. I hadn't really planned on it, because at the time, I broke the ABI (with the OPENSSL_NO_SSL3_METHOD change in there as well). I suppose could shovel just the OPENSSL_NO_SSL3 into stable/12 without too much issue.

Also this change would, depending on the arguments, cause the openssl commandline to abort.

I do not see any direct invocations of SSL3_method/SSL3_client_method/SSL3_server_method in the CLI, which is all that is impacted by this change. The internal SSL3 machinery is not changed, and OPENSSL_NO_SSL3 already disables some portions of the CLI.

Anyway, it is fine by me to just return NULL if that is preferred.

Let's go that direction.

I'm also a newbie when it comes to symbol versioning like is being done here. With the __sym_compat calls being done here, it sounds like existing applications would have the ABI available, but new versions would be unable to link using this implementation. If that's the case, could we just have the actual symbols exposed for the existing implementation to keep it working for old clients, but prevent linking new clients. This would allow stable/12 binaries to continue to work, but all of the HEAD and 13.x would get the benefit of the implementation. It may not be preferable to use that scenario, but I was trying to see how we could accomplish that with symbol versioning and I wasn't able to work it out.

[bjk]

I don't have particular insight into what various distros are doing for SSLv3_method/etc. specifically (my main exposure is into Debian), but I think we can learn quite a bit from the analogous handling of SSLv2_method() and its removal. Specifically, upstream OpenSSL had removed the methods outright around the 1.0.2g/1.0.2h timeframe but had to add them back as stubs that return NULL, to improve ABI compatibility. (This was, IIRC, before the timeframe when separate no-foo and no-foo-method configure options were available, so there wasn't much of a middlle ground available.) This predates the use of github and open pre-commit review, so we just have the commit messages for https://github.com/openssl/openssl/commit/13313856 and https://github.com/openssl/openssl/commit/fcedd2d69d to go from for understanding the motivations of the upstream changes.

Regardless, it seems clear that having stub method functions around that return NULL is useful for ABI-compat purposes and is otherwise reasonably safe. I note that virtually all callers are not going to check the return value of the method functions and just pass them to SSL_CTX_new() directly, but SSL_CTX_new() explicitly checks for a NULL method; there are enough reasons that SSL_CTX_new() can fail that we can reasonably expect errors to be caught at that stage by the application.

[cem]

Don't abort/kill, just warn once and NULL stub per consensus.

[cem]

In D25493#563769, @gordon wrote:

Let's go that direction.

Done!

I'm also a newbie when it comes to symbol versioning like is being done here. With the __sym_compat calls being done here, it sounds like existing applications would have the ABI available, but new versions would be unable to link using this implementation.

Yes, I think that's so:

$ cat c.c
extern void *SSLv3_method(void);
int main(void) {
        void *p = SSLv3_method();
        return (int)p;
}
$ cc -Wall -Wextra -O2 -g c.c  -L $(make -V .OBJDIR -C secure/lib/libssl/) -lssl
ld: error: undefined symbol: SSLv3_method
>>> referenced by c.c:5
>>>               /tmp/c-8ff8ec.o:(main)
>>> did you mean: SSLv3_method
>>> defined in: /obj/usr/home/conrad/src/freebsd/amd64.amd64/secure/lib/libssl/libssl.so

If that's the case, could we just have the actual symbols exposed for the existing implementation to keep it working for old clients, but prevent linking new clients.

Yeah, that's the idea.

$ cc -Wall -Wextra -O2 -g c.c -lssl
$ ./a.out ; echo $?
240
$ LD_LIBRARY_PATH=$(make -V .OBJDIR -C secure/lib/libssl/) ./a.out ; echo $?
SSLv3 use is deprecated.
0

This would allow stable/12 binaries to continue to work, but all of the HEAD and 13.x would get the benefit of the implementation. It may not be preferable to use that scenario, but I was trying to see how we could accomplish that with symbol versioning and I wasn't able to work it out.

I think this works ok. If there are better ideas, great! But I think we've ticked off most of the boxes already:

1. Nominal ABI stability with 12.x binaries (i.e., rtld does not reject)
2. SSLv3_*_methods functions return an unusable NULL pointer
3. New programs can't (easily) link SSLv3_*_methods, despite the ABI compat shims.

[cem]

• Don't print only sizeof(void*) bytes of the warning.

[gordon]

I think this looks good to me. @kib, can you weigh in on the symbol versioning being used here? Should we also update the Symbol.map file as well? What's the protocol around that look like?

[jhb]

I believe this is correct. I think you only put things in Symbol.map if you want them public for when you link new binaries. We use __sym_compat without any corresponding Symbol.map entries for other compat shims in libc (e.g. fts* symbols in lib/libc/gen/Symbol.map are only the latest FBSD1.5 version and don't include the legacy versions for FBSD1.0 and FBSD1.1.)

[kib]

In D25493#563935, @jhb wrote:

I believe this is correct. I think you only put things in Symbol.map if you want them public for when you link new binaries. We use __sym_compat without any corresponding Symbol.map entries for other compat shims in libc (e.g. fts* symbols in lib/libc/gen/Symbol.map are only the latest FBSD1.5 version and don't include the legacy versions for FBSD1.0 and FBSD1.1.)

You can check readelf -Ds output to verify that there are symbols with name@OPENSSL_1_1_0 version but not name@@OPENSSL_1_1_0. @@ denotes default version used by static linker to record version into the consumer. If there is no '@@' definition, ld(1) refuses to link with the symbol if reference does not specify wanted version.

Interaction between version script and .symver asm directive seems to be undocumented (or undefined, go figure). We are usually cautious and always remove symbol from Symbol.map if code manually specifies the version.

secure/lib/libssl/dummy_abi.c

4 ↗

(On Diff #73897)

Is there such legal entity ?

40 ↗

(On Diff #73897)

static const char []

[cem]

In D25493#564116, @kib wrote:

In D25493#563935, @jhb wrote:

I believe this is correct. I think you only put things in Symbol.map if you want them public for when you link new binaries. We use __sym_compat without any corresponding Symbol.map entries for other compat shims in libc (e.g. fts* symbols in lib/libc/gen/Symbol.map are only the latest FBSD1.5 version and don't include the legacy versions for FBSD1.0 and FBSD1.1.)

You can check readelf -Ds output to verify that there are symbols with name@OPENSSL_1_1_0 version but not name@@OPENSSL_1_1_0. @@ denotes default version used by static linker to record version into the consumer. If there is no '@@' definition, ld(1) refuses to link with the symbol if reference does not specify wanted version.

Yep, that is what I found. Excerpts are included in the Testing Notes section.

Interaction between version script and .symver asm directive seems to be undocumented (or undefined, go figure). We are usually cautious and always remove symbol from Symbol.map if code manually specifies the version.

I think Symbol.map may only apply if a symbol of the specific name actually exists. With this revision, the unversioned public symbols are __SSLv3_*_method_fbsd12 rather than SSLv3_*_method.

I would be happy to remove them from Symbol.map as well, but as it did not seem necessary, I took the option of less churn to the contrib source tree.

secure/lib/libssl/dummy_abi.c

4 ↗

(On Diff #73897)

*shrug*

40 ↗

(On Diff #73897)

will fix

[cem]

• Drop copyright/license header
• static string