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

pass padding of 0 to EVP_CIPHER_CTX_set_padding #3450

Merged
merged 1 commit into from
Aug 25, 2022

Conversation

toidiu
Copy link
Contributor

@toidiu toidiu commented Aug 16, 2022

Security concerns:

Padding currently being enabled in main branch is not a security issue since we apply padding manually and provide the encrypt function with data that is a multiple of the block size. This essentially mean a noop when openssl tries to apply padding.

Description of changes:

This is part of the effort to integrate s2n-tls with openssl3.

The error originates from the usage of the function EVP_CIPHER_CTX_set_padding. This function controls padding when performing encrypt/decrypt operations on a block of data. Padding is enabled by default and can be disabled if desired.

If the pad parameter is zero then no padding is performed, the total amount of data encrypted or decrypted must then be a multiple of the block size or an error will occur.

Based on the documentation and source code this doesnt seem to have been used correctly. The EVP_CIPHER_CTX_set_padding fn expects 1 or 0 while the value of EVP_CIPH_NO_PADDING is 0x100 and seems to be used to toggle the flag on the internally used context.

FIX:
The fix here should be to call EVP_CIPHER_CTX_set_padding with 0 instead of EVP_CIPH_NO_PADDING.
https://github.com/toidiu/s2n-tls/compare/ak-openssl3_padding?expand=1

Links:
EVP_CIPHER_CTX_set_padding
https://github.com/openssl/openssl/blob/openssl-3.0.5/crypto/evp/evp_enc.c#L1024
https://github.com/openssl/openssl/blob/OpenSSL_1_1_0/crypto/evp/evp_enc.c#L561

EVP_CIPH_NO_PADDING 0x100
https://github.com/openssl/openssl/blob/openssl-3.0.5/include/openssl/evp.h#L321
https://github.com/openssl/openssl/blob/OpenSSL_1_1_0/include/openssl/evp.h#L243

Testing:

Unit tests for openssl1.1.1 and openssl3 passed.

$ ldd build/bin/s2nd | grep libcrypto

-------libcrypto
        libcrypto.so.3 => /home/ubuntu/projects/rsync/s2n-tls/test-deps/openssl-3.0/lib/libcrypto.so.3 (0x00007f33d81e7000)
-------libcrypto

Running /home/ubuntu/projects/rsync/s2n-tls/tests/unit/s2n_3des_test.c ... PASSED     153692 tests
Running /home/ubuntu/projects/rsync/s2n-tls/tests/unit/s2n_aes_test.c ... PASSED     307382 tests

Is this a refactor change? If so, how have you proved that the intended behavior hasn't changed?

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

@toidiu toidiu requested a review from a team as a code owner August 16, 2022 18:08
@github-actions github-actions bot added the s2n-core team label Aug 16, 2022
@toidiu toidiu closed this Aug 16, 2022
@toidiu toidiu deleted the ak-openssl3_padding branch August 16, 2022 19:08
@toidiu toidiu restored the ak-openssl3_padding branch August 24, 2022 21:24
@toidiu toidiu reopened this Aug 24, 2022
@toidiu toidiu force-pushed the ak-openssl3_padding branch from af890de to b7a55d0 Compare August 24, 2022 21:24
@toidiu toidiu requested a review from lrstewart August 24, 2022 21:27
@toidiu toidiu enabled auto-merge (squash) August 25, 2022 00:28
@toidiu toidiu requested a review from maddeleine August 25, 2022 00:37
@toidiu toidiu merged commit 85bcb26 into aws:main Aug 25, 2022
@toidiu toidiu deleted the ak-openssl3_padding branch August 30, 2022 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants