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

Bring sslscan into a OpenSSL 3.x world #326

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

tetlowgm
Copy link
Contributor

This set of code changes drags sslscan kicking and screaming into the modern era with OpenSSL. There isn't any functional code changes, it's mostly purging old ifdef's and dropping old code that has runtime detection for ancient versions of OpenSSL.

This also moves over several calls that have drop-in modern methods.

Still outstanding deprecated feature usage will require deeper code changes to deal with. Specifically protocol versioned SSL_METHOD (which is sprinkled absolutely everywhere in the codebase) and moving away from key type specific object (EC_KEY, DH, RSA, etc) to the generic EVP_PKEY framework.

@jtesta
Copy link
Contributor

jtesta commented Jan 23, 2025

I attempted to compile the changes introduced in this PR, but failed:

sslscan.c: In function ‘testHost’:
sslscan.c:3487:17: error: expected expression before ‘[’ token
 3487 |                 [[fallthrough]];
      |                 ^
sslscan.c:3487:19: error: ‘fallthrough’ undeclared (first use in this function)
 3487 |                 [[fallthrough]];
      |                   ^~~~~~~~~~~

Once this is fixed, bonus points if you can run ./docker_test.sh without any failures!

Remove ifdefs for old versions of OpenSSL.
Remove ifdefs for SSL_MODE_SEND_FALLBACK_SCSV. It's always there.
Remove ifdefs for OPENSSL_NO_TLSEXT. It's always there.
Cleanup a bit of whitespace and cruft in adjacent blocks.
This is a runtime detection logic for old versions of OpenSSL.
Per the comment, this is a belt and suspenders methodology that
is unlikely to work in practice. It's now deprecated anyway, so
remove it entirely.
Remove library init functions. They were deprecated in 1.1.0.
Several functions have been const returns. Prefer those.
@tetlowgm
Copy link
Contributor Author

tetlowgm commented Jan 23, 2025

Sorry, that was a c23-ism that snuck in. I've fixed the commit for that.

I don't have a docker setup; I'm a FreeBSD/MacOS guy myself. Can I trouble you to run the docker test for me? I will see what it would take to get a functional setup, but that's going to be a bit.

@jtesta
Copy link
Contributor

jtesta commented Jan 23, 2025 via email

@rbsec
Copy link
Owner

rbsec commented Jan 27, 2025

Thanks for submitting this (and @jtesta for testing).

The main hesitation that I have with requiring OpenSSL 3.x is that it's only been out for three years, which means that there are still several widely used and supported distros (such as Ubuntu 20.04 LTS and RHEL 8, although that Ubuntu release seems to have dropped the package) that are shipping OpenSSL 1.x with backported security patches - so requiring 3.x means breaking their builds, as they generally don't seem keen on statically compiling packages.

But the distro packaged versions of sslscan are often pretty outdated and built against versions of OpenSSL that don't support everything we'd want them to - and with the ease of both static builds and Docker builds I'm not sure that's a significant issue. And most of the people using sslscan won't be doing so from those older distros.

So I guess the question would be whether we gain much from dropping support for OpenSSL 1.x? If it's the first step of a wider cleanup and refactoring process, or that legacy support is causing issues elsewhere then that makes perfect sense - but I'm always a little hesitant to drop backwards compatibility without a good reason.

@jtesta what're your thoughts?

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.

3 participants