Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement runtime check on libcrypto linkage #186
Changes from 6 commits
7d9c4a7
fa68f92
6658659
8089294
ded5d64
0e60174
8cbf6d7
4a6553e
0ac6143
51a4a84
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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:
OPENSSL_VERSION_NUMBER
at compile time andSSLeay
(ifdef referent) at runtime, extract and compare major and minor version per this formatting ofOPENSSL_VERSION_NUMBER
. ignore other ordinals.AWSLC_VERSION_NUMBER_STRING
at compile time andSSLeay_version
orOPENSSL_version
at runtime.OPENSSL_VERSION_NUMBER
and anAWSLC_API_VERSION
, neither are updated regularly and are not particularly useful for this check.OPENSSL_VERSION_NUMBER
, but it hasn't been updated in 4 years. BoringSSL does expose aBORINGSSL_API_VERSION
, but not at runtime.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.