Fixing errors with OpenSSL 1.0/1.1 #105
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Issue: there's a slight behavior difference between OpenSSL 1.1 and 3.0 in regards to padding, and how the EVP encryption/decryption functions exactly behave. Our code also incorrectly used these functions until now, which by chance worked correctly with 3.0, but caused decryption issues with 1.0/1.1. This commit: * Fixes the issue * Adds a few related assertions * Adds an Ubuntu 20.04 (OpenSSL 1.1) github runner to verify that everything works correctly with 1.1.
hqakhtar
reviewed
Jan 23, 2024
hqakhtar
reviewed
Jan 24, 2024
src/encryption/enc_aes.c
Outdated
| @@ -102,12 +102,11 @@ AesRun2(EVP_CIPHER_CTX** ctxPtr, int enc, const unsigned char* key, const unsign | |||
|
|
|||
| static void AesRun(int enc, const unsigned char* key, const unsigned char* iv, const unsigned char* in, int in_len, unsigned char* out, int* out_len) | |||
| { | |||
| int out_len2 = 0; | |||
There was a problem hiding this comment.
We need some comments about why out_len2 is required so that people who don't understand SSL can also understand the changes. And perhaps changing the name of out_len2 to something that more self-explanatory.
There was a problem hiding this comment.
I already added a comment around the second assertion
hqakhtar
reviewed
Jan 26, 2024
hqakhtar
reviewed
Jan 26, 2024
src/encryption/enc_aes.c
Outdated
| @@ -141,6 +144,11 @@ static void AesRun(int enc, const unsigned char* key, const unsigned char* iv, c | |||
| goto cleanup; | |||
| } | |||
|
|
|||
| // We encrypt one block (16 bytes) | |||
There was a problem hiding this comment.
Better to use multiline comments /* ... */ instead of single line ones.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Issue: there's a slight behavior difference between OpenSSL 1.1 and 3.0 in regards to padding, and how the EVP encryption/decryption functions exactly behave.
Our code also incorrectly used these functions until now, which by chance worked correctly with 3.0, but caused decryption issues with 1.0/1.1.
This commit: