gh-Mbed-TLS/TF-PSA-Crypto

Watch

Fork 0

Fix for incorrect macro behavior on a whole number of blocks #680

Open

bjwtaylor wants to merge 25 commits from bjwtaylor/cipher-update-incorrect-error into development

bjwtaylor commented

2026-02-02 08:39:47 -06:00

(Migrated from github.com)

Description

Fix for incorrect macro behavior on a whole number of blocks. resolves https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/592. Depends on https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/692

PR checklist

changelog provided | not required because: TBC
framework PR not required
mbedtls development PR not required because: No changes
mbedtls 3.6 PR not required because: To be provided
tests provided

## Description Fix for incorrect macro behavior on a whole number of blocks. resolves https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/592. Depends on https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/692 ## PR checklist - [ ] **changelog** provided | not required because: TBC - [ ] **framework PR** not required - [ ] **mbedtls development PR** not required because: No changes - [ ] **mbedtls 3.6 PR** not required because: To be provided - **tests** provided

gilles-peskine-arm (Migrated from github.com) reviewed

2026-02-02 08:39:47 -06:00

valeriosetti (Migrated from github.com) reviewed

2026-02-02 08:39:47 -06:00

valeriosetti (Migrated from github.com) requested changes

2026-02-04 06:33:40 -06:00

valeriosetti (Migrated from github.com) left a comment

Instead of adding a new test function I think we can use the already existing cipher_verify_output_multipart and just add this patch:

diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function
index 87ce86cda..fe2640e93 100644
--- a/tests/suites/test_suite_psa_crypto.function
+++ b/tests/suites/test_suite_psa_crypto.function
@@ -5007,6 +5007,8 @@ void cipher_verify_output_multipart(int alg_arg,
               PSA_CIPHER_DECRYPT_OUTPUT_SIZE(key_type, alg, output1_length));
     TEST_LE_U(output2_buffer_size,
               PSA_CIPHER_DECRYPT_OUTPUT_MAX_SIZE(output1_length));
+    TEST_LE_U(output2_buffer_size,
+              PSA_CIPHER_UPDATE_OUTPUT_MAX_SIZE(input->len));
     TEST_CALLOC(output2, output2_buffer_size);
 
     if (iv_length > 0) {

With out your fix to crypto_sizes.h it fails on:
PSA symmetric encrypt/decrypt multipart: AES-CBC-PKCS#7, 16 bytes
PSA symmetric encrypt/decrypt multipart: AES-CBC-PKCS#7 padding, 4

If instead you apply the fix in crypto_sizes.h all tests are fine.
Wdyt?

Instead of adding a new test function I think we can use the already existing `cipher_verify_output_multipart` and just add this patch: ``` diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index 87ce86cda..fe2640e93 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -5007,6 +5007,8 @@ void cipher_verify_output_multipart(int alg_arg, PSA_CIPHER_DECRYPT_OUTPUT_SIZE(key_type, alg, output1_length)); TEST_LE_U(output2_buffer_size, PSA_CIPHER_DECRYPT_OUTPUT_MAX_SIZE(output1_length)); + TEST_LE_U(output2_buffer_size, + PSA_CIPHER_UPDATE_OUTPUT_MAX_SIZE(input->len)); TEST_CALLOC(output2, output2_buffer_size); if (iv_length > 0) { ``` With out your fix to `crypto_sizes.h` it fails on: `PSA symmetric encrypt/decrypt multipart: AES-CBC-PKCS#7, 16 bytes` `PSA symmetric encrypt/decrypt multipart: AES-CBC-PKCS#7 padding, 4` If instead you apply the fix in `crypto_sizes.h` all tests are fine. Wdyt?

tests/suites/test_suite_psa_crypto.function

I'm not sure that this test function is OK. IIUC from the original issue the problem arises on the decryption phase, not the encryption one.
I tried testing only the 1st commit of this PR (without the patch) and failures appear on psa_cipher_finish during encryption and IMO this is due to the fact that you are trying to store the encrypted data on a buffer sized with PSA_CIPHER_UPDATE_OUTPUT_MAX_SIZE() which should instead be sized with PSA_CIPHER_FINISH_OUTPUT_MAX_SIZE. This is on line 5121 at that commit.

I'm not sure that this test function is OK. IIUC from the [original issue](https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/592#issuecomment-3647306655) the problem arises on the decryption phase, not the encryption one. I tried testing only the 1st commit of this PR (without the patch) and failures appear on `psa_cipher_finish` during encryption and IMO this is due to the fact that you are trying to store the encrypted data on a buffer sized with `PSA_CIPHER_UPDATE_OUTPUT_MAX_SIZE()` which should instead be sized with `PSA_CIPHER_FINISH_OUTPUT_MAX_SIZE`. This is on line 5121 at that commit.

bjwtaylor (Migrated from github.com) reviewed

2026-02-04 07:38:16 -06:00

tests/suites/test_suite_psa_crypto.function

Yeah, this makes sense. I've updated the PR, let's see what happens with the ci.

valeriosetti (Migrated from github.com) reviewed

2026-02-05 02:02:57 -06:00

gilles-peskine-arm commented

2026-02-09 08:58:10 -06:00

(Migrated from github.com)

not required because: No backports

Oh? Have you confirmed that the bug is not present in 3.6, and that there is a test for it? I thought that this had never been tested, and the offending code doesn't seem to have changed much since well before 3.6.0.

> not required because: No backports Oh? Have you confirmed that the bug is not present in 3.6, and that there is a test for it? I thought that this had never been tested, and the offending code doesn't seem to have changed much since well before 3.6.0.

bjwtaylor commented

2026-02-09 09:20:22 -06:00

(Migrated from github.com)

@gilles-peskine-arm, good spot. I'll add a backport.

gilles-peskine-arm (Migrated from github.com) requested changes

2026-02-09 10:28:41 -06:00

gilles-peskine-arm (Migrated from github.com) left a comment

The fix is at least incomplete and unexplained, and I'm not sure if it's correct.

ChangeLog.d/fix-bug-in-update-max-size-calcuation.txt

     when a whole number of blocks is used with PKCS7 padding. Fixes #592.

```suggestion when a whole number of blocks is used with PKCS7 padding. Fixes #592. ```

include/psa/crypto_sizes.h

Adding 1 in PSA_CIPHER_UPDATE_OUTPUT_MAX_SIZE but not in PSA_CIPHER_UPDATE_OUTPUT_SIZE doesn't make sense. (And it would fail if you add the corresponding test assertion.)

Adding 1 in `PSA_CIPHER_UPDATE_OUTPUT_MAX_SIZE` but not in `PSA_CIPHER_UPDATE_OUTPUT_SIZE` doesn't make sense. (And it would fail if you add the corresponding test assertion.)

Adding 1 here makes this case work, but I don't understand where it comes from. At the very least, it needs an explanation. But I don't think it's actually correct.

tests/suites/test_suite_psa_crypto.function

Although this assertion does fail before the bug fix, it's not asserting the actual property in the function's specification. Given that output2_buffer_size = output1_length;, this is asserting that PSA_CIPHER_UPDATE_OUTPUT_MAX_SIZE is at least as large as the output from encryption, not that PSA_CIPHER_UPDATE_OUTPUT_MAX_SIZE is a sufficient size for the decryption step.

I don't really understand what this function is asserting. Matching the assertions with expected properties of the API would be useful, and I think it's necessary for this task (both in the sense of adding comments to explain, and quite possibly in the sense of changing the assertions to correspond to the expected properties).

Although this assertion does fail before the bug fix, it's not asserting the actual property in the function's specification. Given that `output2_buffer_size = output1_length;`, this is asserting that `PSA_CIPHER_UPDATE_OUTPUT_MAX_SIZE` is at least as large as the output from encryption, not that `PSA_CIPHER_UPDATE_OUTPUT_MAX_SIZE` is a sufficient size for the decryption step. I don't really understand what this function is asserting. Matching the assertions with expected properties of the API would be useful, and I think it's necessary for this task (both in the sense of adding comments to explain, and quite possibly in the sense of changing the assertions to correspond to the expected properties).

gilles-peskine-arm (Migrated from github.com) requested changes

2026-02-09 10:50:40 -06:00

gilles-peskine-arm (Migrated from github.com) left a comment

The fix is at least incomplete and unexplained, and I'm not sure if it's correct.

bjwtaylor commented

2026-02-10 02:39:08 -06:00

(Migrated from github.com)

The issue in the formula in the code, was that in the scenario when PT size was equal to a whole number of blocks it did not account for the additional block being added. This is because a PT size, that is a whole number of blocks will be rounded to the current number of blocks and therefore the CT size outputted by the macro will equal the PT size. This fix resolves this by adding 1 to the size of the PT prior to performing the rounding. This then means any value of PT that is a whole number of blocks then becomes PT + 1 and when rounded becomes PT + block_size as intended. All other values are unaffected, as (N * block_size) -1 becomes N * block_size and is therefore rounded to N*block_size. In all other cases the PT will also still be rounded to the nearest whole number of blocks.

Hopefully this answers the unexplained part, as for whether it is incomplete or incorrect if you can point to specific scenarios where this is the case I am happy to look into them. I can think that this would potentially over allocate when PKCS7 is not used. I'm not sure if this is an issue and is what you are referring to?

I agree that adding 1 in PSA_CIPHER_UPDATE_OUTPUT_MAX_SIZE but not in PSA_CIPHER_UPDATE_OUTPUT_SIZE, doesn't make sense. So if we're happy with this solution I'll modify this macro to match. As for the test, the original strategy was to do a full multi stage encrypt/decrypt to recreate the bug and confirm it has been resolved. However this was changed in response to comments, we can always go back to this if we think it will be more effective though.

Apologies for not adding a full explanation of the fix to the changelog and if you're happy this solution works, I'll update it with the above explanation to avoid further confusion. I'll also add a backport, as you've correctly pointed out this will also probably effect 3,6 also. Let me know what you think to the current implementation in the meantime though.

Morning @gilles-peskine-arm, thanks for your comments. So in order to resolve the unexplained part. The issue identified in the bug is that when using PKCS7, the macro does not account for the size of the padding. In this standard my understanding is that the padding will depend on the PT size, relative to the block size. If the PT size is equal to a whole number of blocks then the CT size, will be equal to PT + block_size. If it is not a whole number of blocks the CT size will be equal to the PT size rounded up to the nearest number of whole blocks. The issue in the formula in the code, was that in the scenario when PT size was equal to a whole number of blocks it did not account for the additional block being added. This is because a PT size, that is a whole number of blocks will be rounded to the current number of blocks and therefore the CT size outputted by the macro will equal the PT size. This fix resolves this by adding 1 to the size of the PT prior to performing the rounding. This then means any value of PT that is a whole number of blocks then becomes PT + 1 and when rounded becomes PT + block_size as intended. All other values are unaffected, as (N * block_size) -1 becomes N * block_size and is therefore rounded to N*block_size. In all other cases the PT will also still be rounded to the nearest whole number of blocks. Hopefully this answers the unexplained part, as for whether it is incomplete or incorrect if you can point to specific scenarios where this is the case I am happy to look into them. I can think that this would potentially over allocate when PKCS7 is not used. I'm not sure if this is an issue and is what you are referring to? I agree that adding 1 in PSA_CIPHER_UPDATE_OUTPUT_MAX_SIZE but not in PSA_CIPHER_UPDATE_OUTPUT_SIZE, doesn't make sense. So if we're happy with this solution I'll modify this macro to match. As for the test, the original strategy was to do a full multi stage encrypt/decrypt to recreate the bug and confirm it has been resolved. However this was changed in response to comments, we can always go back to this if we think it will be more effective though. Apologies for not adding a full explanation of the fix to the changelog and if you're happy this solution works, I'll update it with the above explanation to avoid further confusion. I'll also add a backport, as you've correctly pointed out this will also probably effect 3,6 also. Let me know what you think to the current implementation in the meantime though.

gilles-peskine-arm commented

2026-02-10 03:06:46 -06:00

(Migrated from github.com)

The issue identified in the bug is that when using PKCS7, the macro does not account for the size of the padding.

No, that's not the problem. The macro does account for the padding, by adding the block size. Adding 1 more in the macro makes this particular test case pass, but the test isn't testing the API contract and I'm not even sure it fully fixes the actual bug.

There's a bug in the library in the size calculation, where it overestimates the size of the output.

Regarding the test strategy, please do test the actual API contract (psa_cipher_update must work if given an output buffer that is exactly PSA_CIPHER_UPDATE_OUTPUT_SIZE(…) bytes, which must be tested for a relevant selection of sizes and alignments, for all modes that aren't stream ciphers, in both directions). That may mean writing a new function, or better changing the current function. I'm not sure what the current function is testing. Figuring out what it's testing and deciding what to keep and what to change is part of this task.

> The issue identified in the bug is that when using PKCS7, the macro does not account for the size of the padding. No, that's not the problem. The macro _does_ account for the padding, by adding the block size. Adding 1 more in the macro makes this particular test case pass, but the test isn't testing the API contract and I'm not even sure it fully fixes the actual bug. There's a bug in the library in the size calculation, where it overestimates the size of the output. Regarding the test strategy, please do test the actual API contract (`psa_cipher_update` must work if given an output buffer that is exactly `PSA_CIPHER_UPDATE_OUTPUT_SIZE(…)` bytes, which must be tested for a relevant selection of sizes and alignments, for all modes that aren't stream ciphers, in both directions). That may mean writing a new function, or better _changing_ the current function. I'm not sure what the current function is testing. Figuring out what it's testing and deciding what to keep and what to change is part of this task.

bjwtaylor commented

2026-02-10 03:17:56 -06:00

(Migrated from github.com)

OK, sounds like a good way forward. I'll add further testing to the PR as described and hopefully that'll give us a better idea if the fix has any issues. Many thanks for your advice.

bjwtaylor (Migrated from github.com) reviewed

2026-02-23 01:51:12 -06:00

ChangeLog.d/fix-bug-in-update-max-size-calcuation.txt

Now Updated.

bjwtaylor (Migrated from github.com) reviewed

2026-02-23 01:52:13 -06:00

include/psa/crypto_sizes.h

As discussed I have now changed the size calculation in the update to match the expected design.

bjwtaylor (Migrated from github.com) reviewed

2026-02-23 01:53:32 -06:00

tests/suites/test_suite_psa_crypto.function

I've now modified the cipher_decrypt_multipart to demonstrate the error in the update.

bjwtaylor (Migrated from github.com) reviewed

2026-02-23 01:53:51 -06:00

include/psa/crypto_sizes.h

As discussed I have now changed the size calculation in the update to match the expected design.

This pull request is broken due to missing fork information.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin 'bjwtaylor/cipher-update-incorrect-error:bjwtaylor/cipher-update-incorrect-error'

git switch 'bjwtaylor/cipher-update-incorrect-error'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'development'

git merge --no-ff 'bjwtaylor/cipher-update-incorrect-error'

git switch 'bjwtaylor/cipher-update-incorrect-error'

git rebase 'development'

git switch 'development'

git merge --ff-only 'bjwtaylor/cipher-update-incorrect-error'

git switch 'bjwtaylor/cipher-update-incorrect-error'

git rebase 'development'

git switch 'development'

git merge --no-ff 'bjwtaylor/cipher-update-incorrect-error'

git switch 'development'

git merge --squash 'bjwtaylor/cipher-update-incorrect-error'

git switch 'development'

git merge --ff-only 'bjwtaylor/cipher-update-incorrect-error'

git switch 'development'

git merge 'bjwtaylor/cipher-update-incorrect-error'

git push origin 'development'

No reviewers

gilles-peskine-arm

valeriosetti

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gh-Mbed-TLS/TF-PSA-Crypto!680

No description provided.

Rows
Columns