Skip to content

Conversation

tstenner
Copy link
Contributor

Issues:

Continuation of #847.

Some applications (e.g. EBICS) still require compatibility with padding schemes like ISO 10126 and ANSI X9.23.

Description of changes:

This PR adds the method StreamDecryptingKey::disable_padding() as a safe wrapper for the existing EVP_CIPHER_CTX_set_padding() function. This allows applications to decrypt data padded with one of these padding schemes and validate the padding without adding complexity to aws-lc-rs.

Testing:

Testing will be added. What kind of coverage is expected?

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

@tstenner tstenner requested a review from a team as a code owner August 29, 2025 15:40
@justsmth
Copy link
Contributor

... This allows applications to ... validate the padding ...

I'm not familiar with this use-case. Why would padding need to be validated?

@justsmth justsmth requested a review from torben-hansen August 29, 2025 19:08
@tstenner
Copy link
Contributor Author

With the padding schemes I need to support, the last byte n is the number of padding bytes (so it needs to be <=16 with a valid key). With other schemes, all the other padding bits are supposed to be 0 (some X9.23 implementations) or n.
The main validation (and necessary step to remove the padding) would be to check that the last byte is at most the block size.

@justsmth
Copy link
Contributor

justsmth commented Sep 2, 2025

... The main validation (and necessary step to remove the padding) would be to check that the last byte is at most the block size.

I believe that validation is already being done here and here.

@justsmth
Copy link
Contributor

justsmth commented Sep 2, 2025

... The main validation (and necessary step to remove the padding) would be to check that the last byte is at most the block size.

I believe that validation is already being done here and here.

Ahh. My links above only apply to PaddedBlockDecryptingKey. However, I believe this validation is occurring already within AWS-LC. I would need to dig a little deeper to find it.

Do you have reason to believe that this validation is not occurring currently?

@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.27%. Comparing base (c358484) to head (4193cbc).
⚠️ Report is 243 commits behind head on main.

Files with missing lines Patch % Lines
aws-lc-rs/src/cipher/streaming.rs 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #869      +/-   ##
==========================================
- Coverage   95.80%   92.27%   -3.53%     
==========================================
  Files          61       71      +10     
  Lines        8143     9279    +1136     
  Branches        0     9279    +9279     
==========================================
+ Hits         7801     8562     +761     
- Misses        342      437      +95     
- Partials        0      280     +280     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tstenner
Copy link
Contributor Author

tstenner commented Sep 4, 2025

The validation happens within evp_decryptfinal_ex which is called from StreamingDecryptingKey::finish(), but it is hardcoded to PKCS padding (and fails with ciphertexts padded with these legacy padding schemes).

While I'm typing this I thought of working around this problem by omitting the call to StreamingDecryptingKey::finish() and pushing a dummy ciphertext block (yielding the last real plaintext block). I'm changing the PR to a draft until I've tested if this works.

@tstenner tstenner marked this pull request as draft September 4, 2025 13:58
@tstenner
Copy link
Contributor Author

tstenner commented Sep 9, 2025

Calling

decryptor.update(&[0u8; 16], &mut self.decrypted);

instead of

decryptor.finish(&[0u8; 16], &mut self.decrypted);

works for my use case.

In case these legacy padding schemes should be supported a bit better, this PR is a relatively simple fix, but it's not strictly needed.

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