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

Implement runtime check on libcrypto linkage #186

Merged
merged 10 commits into from
Jun 14, 2024

Conversation

WillChilds-Klein
Copy link
Contributor

@WillChilds-Klein WillChilds-Klein commented May 22, 2024

Description

This PR adds a runtime check to ensure that the preprocess/compile-time libcrypto is of the same "family" as linked libcrypto. With this check, we enforce that aws-c-cal is compiled against supported libcrypto's (unless BYO_CRYPTO has been specified). In the case of compilation against AWS-LC, we also validate that compile/link version is the same. We do not check version in the OpenSSL case as discussed here and here.

Testing

This PR's CI jobs verify the "happy" path for supported libcryptos (AWS-LC, BoringSSL, BYO_CRYPTO, and a few versions of OpenSSL). I've also tested the failure case with aws-c-cal compiled against OpenSSL 1.1.1, then linked by aws-crt-python's build to AWS-LC. Run from aws-crt-python's source root (with debug logging configured in aws-c-cal):

$ python3 -m unittest discover --failfast --verbose
...
Fatal error condition occurred in /tmp/tmp.5xrtKAz4xw/tmp/src/Aws-c-cal/source/unix/openssl_platform_init.c:591: strcmp(expected_version, runtime_version) == 0 && "libcrypto mislink"
Exiting Application
[DEBUG] [2024-05-23T23:53:49Z] [00007f8dc4ba1740] [libcrypto_resolve] - Compiled with libcrypto OpenSSL 1.1.1x  30 Jan 2024, linked to libcrypto AWS-LC FIPS 1.26.0
...

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@WillChilds-Klein WillChilds-Klein marked this pull request as draft May 22, 2024 02:51
source/unix/openssl_platform_init.c Outdated Show resolved Hide resolved
source/unix/openssl_platform_init.c Outdated Show resolved Hide resolved
source/unix/openssl_platform_init.c Show resolved Hide resolved
WillChilds-Klein and others added 5 commits May 22, 2024 11:33
Co-authored-by: Michael Graeb <graebm@amazon.com>
Co-authored-by: Michael Graeb <graebm@amazon.com>
Co-authored-by: Michael Graeb <graebm@amazon.com>
"Compiled with libcrypto %s, linked to libcrypto %s",
expected_version,
runtime_version);
AWS_FATAL_ASSERT(strcmp(expected_version, runtime_version) == 0 && "libcrypto mislink");
Copy link
Contributor Author

@WillChilds-Klein WillChilds-Klein May 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An important call-out is that while the current implementation is very simple, it is quite strict. Any deviation between compile/link version (including patch version and build date for OpenSSL) will cause this check to fail (except for BoringSSL as its version string is constant). For instance, if a user compiles an application on a system with slightly older OpenSSL installation with -DUSE_OPENSSL=ON then deploys the application to a system with a slightly newer OpenSSL installation (i.e. higher patch version, same minor version), we'd expect this check to fail.

The s2n-tls initialization check that inspired this PR is more relaxed. That check only validates libcrypto "family" (AWS-LC, BoringSSL, OpenSSL, etc.) and "major version" (this concept differs by family), allowing OpenSSL minor version to differ.

If we're not OK with the strictness of the current implementation, we could do a more complicated (but more flexible) validation that tests the following:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To better illustrate this, I've implemented the above suggestion here in this PR. Let me know what the team thinks, we can always revert to the stricter validation if that's preferred.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline with @DmitriyMusatkin. We may want to relax this check to only validate the libcrypto "family" matches between compile and link, and allow flexibility for library versions as OpenSSL has relatively stable ABI across minor (and major?) versions.

@WillChilds-Klein WillChilds-Klein marked this pull request as ready for review May 24, 2024 14:04
source/unix/openssl_platform_init.c Show resolved Hide resolved
source/unix/openssl_platform_init.c Show resolved Hide resolved
const unsigned long runtime_openssl_major = (runtime_openssl_version & 0x00000000F0000000L);
const unsigned long runtime_openssl_minor = (runtime_openssl_version & 0x000000000FF00000L);
AWS_FATAL_ASSERT(expected_openssl_major == runtime_openssl_major && "OpenSSL major version mismatch");
AWS_FATAL_ASSERT(expected_openssl_minor == runtime_openssl_minor && "OpenSSL minor version mismatch");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know OpenSSL 1.x broke API/ABI between minor revisions, but they say they won't do it anymore in 3.x

https://www.openssl.org/policies/general/versioning-policy.html

This document describes the release versioning scheme used by the OpenSSL project from version 3.0.0 onwards ... A minor release can, and generally will, introduce new features. However both the API and ABI will be preserved.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thing to note is that we try to workaround that abi breakage in openssl_init.c, but trying to detect at runtime which version we have and then dlsym the correct api signature.
because of that i think we openssl we can have relaxed checking that just matches the family. otherwise we might break people who mix and match compile and runtime versions.
in future, we can consider having a strict minor and major version check and removing that dlsym code altogether

source/unix/openssl_platform_init.c Outdated Show resolved Hide resolved
Comment on lines 592 to 596
AWS_FATAL_ASSERT(strstr("AWS-LC", expected_version) == NULL);
AWS_FATAL_ASSERT(strstr("AWS-LC", runtime_version) == NULL);
/* Validate both expected and runtime versions begin with OpenSSL's version str prefix. */
const char *openssl_prefix = "OpenSSL ";
AWS_FATAL_ASSERT(strncmp(openssl_prefix, expected_version) == 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

debatable: why are we asserting on the expected_version? This seems like overkill, we just set this like 10 lines ago

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i put the AWS_FATAL_ASSERT(strstr(...) == NULL); asserts in there because AWS-LC's OPENSSL_VERSION_TEXT sort of "looks like" OpenSSL's and I wanted to ensure that we're neither compiling nor linking against AWS-LC when we think we're compiling/linking against OpenSSL.

the AWS_FATAL_ASSERT(strncmp(...) == 0); asserts are the first actual check that we are indeed compiling/linking against OpenSSL (or at least something that looks like it; i.e. has a OpenSSL version string prefix).

these assertions should be cheap, and are only run once on startup.

@DmitriyMusatkin DmitriyMusatkin merged commit 0cdbf2b into awslabs:main Jun 14, 2024
38 checks passed
graebm added a commit that referenced this pull request Jul 3, 2024
graebm added a commit that referenced this pull request Jul 3, 2024
**Issue**:
The libcrypto runtime check isn't working with latest [AWS-LC-FIPS-2.0.13](https://github.com/aws/aws-lc/releases/tag/AWS-LC-FIPS-2.0.13) release. It's not returning the expected version string.

AWS-LC is working on a fix here: aws/aws-lc#1689

**Description of changes:**
This reverts commit 0cdbf2b.

We'll bring this check right back once AWS-LC-FIPS has this fix in its latest release (on Github, and internally at Amazon).

It seems simpler to remove the check entirely, and bring it back once things are working. Vs patching it now, and then patching it again, and then we have all these different versions of aws-lc/aws-c-cal that aren't compatible...
WillChilds-Klein added a commit to WillChilds-Klein/aws-c-cal that referenced this pull request Jul 12, 2024
TingDaoK added a commit that referenced this pull request Jul 15, 2024
…195)

Co-authored-by: WillChilds-Klein <willck93@gmail.com>
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