gh-Mbed-TLS/TF-PSA-Crypto
Watch
1
0
Fork
You've already forked TF-PSA-Crypto
 0
Code Issues 167 Pull requests 92 Releases 5 Activity

psa: mac: only call memset if key_length is less than block size #403

Open
amjoul01 wants to merge 2 commits from amjoul01/amjoul01/psa-mac-out-of-bound-fix into development
pull from: amjoul01/amjoul01/psa-mac-out-of-bound-fix
merge into: gh-Mbed-TLS:development
Conversation 8 Commits 2 Files changed 3 +56 -2
amjoul01 commented 2025-07-31 13:01:28 -05:00 (Migrated from github.com)

Description

In case key_length is equal to block_size, memset would be called with a destination address outside the boundary of ipad and opad, with a null size. Since memset implementation is compiler-dependent, it is a good practice not to assume that it won't dereference the out-of-bound destination address.

Instead, only call memset if key_length is less than block_size.

PR checklist

Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line.
If the provided content is part of the present PR remove the # symbol.

  • changelog not required.
  • framework PR not required because the fix is for a bug at the PSA MAC level only.
  • mbedtls development PR will be provided as soon as this PR is reviewed.
  • mbedtls 3.6 PR will be provided as soon as this PR is reviewed.
  • tests a relevant test will follow if not already present.
## Description In case key_length is equal to block_size, memset would be called with a destination address outside the boundary of ipad and opad, with a null size. Since memset implementation is compiler-dependent, it is a good practice not to assume that it won't dereference the out-of-bound destination address. Instead, only call memset if key_length is less than block_size. ## PR checklist Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line. If the provided content is part of the present PR remove the # symbol. - [x] **changelog** not required. - [x] **framework PR** not required because the fix is for a bug at the PSA MAC level only. - [ ] **mbedtls development PR** will be provided as soon as this PR is reviewed. - [ ] **mbedtls 3.6 PR** will be provided as soon as this PR is reviewed. - [ ] **tests** a relevant test will follow if not already present.
felixc-arm commented 2025-08-01 04:17:43 -05:00 (Migrated from github.com)

Hiya, just to add the next steps here as well for completeness, please:

Thanks!

Hiya, just to add the next steps here as well for completeness, please: - Open an issue relating to this in https://github.com/Mbed-TLS/mbedtls/issues and link this PR and the eventual `mbedtls-3.6` one - Add a non-regression test(s) to this PR - Check the CI passes (at the moment it's just a code-style issue due to the spaces before opening & closing parentheses) - Backport the change to `mbedtls-3.6` (https://github.com/Mbed-TLS/mbedtls/tree/mbedtls-3.6) - you shouldn't need to create an `mbedtls-development` port. Thanks!
gilles-peskine-arm commented 2025-08-01 06:50:03 -05:00 (Migrated from github.com)

Unfortunately, we merged an incompatible change this morning, which will cause further CI runs to fail. So please rebase on top of the latest head of development.

Unfortunately, we merged an incompatible change this morning, which will cause further CI runs to fail. So please rebase on top of the latest head of `development`.
mpg commented 2025-08-04 03:35:46 -05:00 (Migrated from github.com)

Also, the code style check is unhappy and suggests the following patch (from the CI output):

--- drivers/builtin/src/psa_crypto_mac.c	2025-07-31 19:55:40.000000000 +0000
+++ drivers/builtin/src/psa_crypto_mac.c.uncrustify	2025-07-31 20:41:37.447975783 +0000
@@ -80,7 +80,7 @@
     }
 
     if (key_length < block_size) {
-        memset( ipad + key_length, 0x36, block_size - key_length );
+        memset(ipad + key_length, 0x36, block_size - key_length);
     }
 
     /* Copy the key material from ipad to opad, flipping the requisite bits,
@@ -90,7 +90,7 @@
     }
 
     if (key_length < block_size) {
-        memset( hmac->opad + key_length, 0x5C, block_size - key_length );
+        memset(hmac->opad + key_length, 0x5C, block_size - key_length);
     }
 
     status = psa_hash_setup(&hmac->hash_ctx, hash_alg);
Also, the code style check is unhappy and suggests the following patch (from the CI output): ```diff --- drivers/builtin/src/psa_crypto_mac.c 2025-07-31 19:55:40.000000000 +0000 +++ drivers/builtin/src/psa_crypto_mac.c.uncrustify 2025-07-31 20:41:37.447975783 +0000 @@ -80,7 +80,7 @@ } if (key_length < block_size) { - memset( ipad + key_length, 0x36, block_size - key_length ); + memset(ipad + key_length, 0x36, block_size - key_length); } /* Copy the key material from ipad to opad, flipping the requisite bits, @@ -90,7 +90,7 @@ } if (key_length < block_size) { - memset( hmac->opad + key_length, 0x5C, block_size - key_length ); + memset(hmac->opad + key_length, 0x5C, block_size - key_length); } status = psa_hash_setup(&hmac->hash_ctx, hash_alg); ```
amjoul01 commented 2025-08-04 03:57:27 -05:00 (Migrated from github.com)

Style check issues are addressed, changes are rebased on latest development. This PR is linked to the corresponding issue: https://github.com/Mbed-TLS/mbedtls/issues/10342

Style check issues are addressed, changes are rebased on latest development. This PR is linked to the corresponding issue: https://github.com/Mbed-TLS/mbedtls/issues/10342
👍 1
mpg (Migrated from github.com) reviewed 2025-08-04 03:59:54 -05:00
mpg (Migrated from github.com) left a comment
No description provided.
LGTM, thanks!
amjoul01 commented 2025-08-04 12:15:19 -05:00 (Migrated from github.com)

@mpg @gilles-peskine-arm I have added a non-regression test to this PR. It does not cover all hasing algorithms but it's a start.

@mpg @gilles-peskine-arm I have added a non-regression test to this PR. It does not cover all hasing algorithms but it's a start.
gilles-peskine-arm (Migrated from github.com) requested changes 2025-08-05 14:21:30 -05:00
gilles-peskine-arm (Migrated from github.com) left a comment

This doesn't actually fix a bug, as far as I can tell.

This doesn't actually fix a bug, as far as I can tell.
drivers/builtin/src/psa_crypto_mac.c
@ -81,1 +81,3 @@
memset(ipad + key_length, 0x36, block_size - key_length);
if (key_length < block_size) {
memset(ipad + key_length, 0x36, block_size - key_length);
gilles-peskine-arm (Migrated from github.com) commented 2025-08-05 14:15:55 -05:00

If key_length > block_size, above, we replace key_length by the length of the hash. For all hashes, the length of the hash is less than the block size. Thus, at this point, we know that key_length <= block_size, but it relies on a high-level argument.

Based on this high-level argument, key_length < block_size is false only if key_length == block_size. In that case, the call to memset is still correct, so there's no need to skip it. (It's correct because it's calling memset with the size 0 and a pointer that's a valid end-of-array pointer. This is perfectly well defined, unlike memset with a null pointer.)

So I don't think this patch makes sense.

What we could do is to add a sanity check on the hash length vs the block length: at the end of the if (key_length > block_size) block above:

if (key_length  > block_size) {
    /* This shouldn't happen: a hash's block size is always larger than the length of the hash. */
    return PSA_ERROR_CORRUPTION_DETECTED;
}
If `key_length > block_size`, above, we replace `key_length` by the length of the hash. For all hashes, the length of the hash is less than the block size. Thus, at this point, we know that `key_length <= block_size`, but it relies on a high-level argument. Based on this high-level argument, `key_length < block_size` is false only if `key_length == block_size`. In that case, the call to `memset` is still correct, so there's no need to skip it. (It's correct because it's calling `memset` with the size 0 and a pointer that's a valid end-of-array pointer. This is perfectly well defined, unlike `memset` with a null pointer.) So I don't think this patch makes sense. What we could do is to add a sanity check on the hash length vs the block length: at the end of the `if (key_length > block_size)` block above: ``` if (key_length > block_size) { /* This shouldn't happen: a hash's block size is always larger than the length of the hash. */ return PSA_ERROR_CORRUPTION_DETECTED; } ```
tests/suites/test_suite_psa_crypto_hash.data
@ -671,0 +671,4 @@
PSA HMAC SHA-256 Block-Size-Key
depends_on:PSA_WANT_ALG_SHA_256
hmac_block_size_key:PSA_ALG_SHA_256:"abc":"2f8cff867f2668ca93d3c5b03ba9f816746742eda349b3bc4bb35aa27816754c"
gilles-peskine-arm (Migrated from github.com) commented 2025-08-05 14:19:11 -05:00

The file is for hash tests only. A MAC test doesn't belong here.

The file is for hash tests only. A MAC test doesn't belong here.
tests/suites/test_suite_psa_crypto_hash.function
@ -82,2 +82,4 @@
/* END_CASE */
/* BEGIN_CASE depends_on: MBEDTLS_PSA_CRYPTO_C */
void hmac_block_size_key(int alg_arg, char *input, data_t *expected_mac)
gilles-peskine-arm (Migrated from github.com) commented 2025-08-05 14:20:56 -05:00

Why add a new test function? Yes, it allows us to calculate the block size parametrically on the chosen hash, but I don't think that's really useful, since there has to be specific test case data (expected_mac) for each hash algorithm anyway. It would be much simpler to just use mac_sign in test_suite_psa_crypto.

Why add a new test function? Yes, it allows us to calculate the block size parametrically on the chosen hash, but I don't think that's really useful, since there has to be specific test case data (`expected_mac`) for each hash algorithm anyway. It would be much simpler to just use `mac_sign` in `test_suite_psa_crypto`.
gilles-peskine-arm (Migrated from github.com) reviewed 2025-08-05 15:03:40 -05:00
drivers/builtin/src/psa_crypto_mac.c
@ -81,1 +81,3 @@
memset(ipad + key_length, 0x36, block_size - key_length);
if (key_length < block_size) {
memset(ipad + key_length, 0x36, block_size - key_length);
gilles-peskine-arm (Migrated from github.com) commented 2025-08-05 15:03:40 -05:00

I've added test cases in https://github.com/Mbed-TLS/mbedtls/pull/10345. UBSan is happy about them.

I've added test cases in https://github.com/Mbed-TLS/mbedtls/pull/10345. UBSan is happy about them.
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 'amjoul01/amjoul01/psa-mac-out-of-bound-fix:amjoul01/amjoul01/psa-mac-out-of-bound-fix'
git switch 'amjoul01/amjoul01/psa-mac-out-of-bound-fix'

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 'amjoul01/amjoul01/psa-mac-out-of-bound-fix'
git switch 'amjoul01/amjoul01/psa-mac-out-of-bound-fix'
git rebase 'development'
git switch 'development'
git merge --ff-only 'amjoul01/amjoul01/psa-mac-out-of-bound-fix'
git switch 'amjoul01/amjoul01/psa-mac-out-of-bound-fix'
git rebase 'development'
git switch 'development'
git merge --no-ff 'amjoul01/amjoul01/psa-mac-out-of-bound-fix'
git switch 'development'
git merge --squash 'amjoul01/amjoul01/psa-mac-out-of-bound-fix'
git switch 'development'
git merge --ff-only 'amjoul01/amjoul01/psa-mac-out-of-bound-fix'
git switch 'development'
git merge 'amjoul01/amjoul01/psa-mac-out-of-bound-fix'
git push origin 'development'
Sign in to join this conversation.
Reviewers
No reviewers
gilles-peskine-arm
Labels
Clear labels   
api-break
This issue/PR breaks the API and must wait for a new major version

  
approved
Design and code approved - may be waiting for CI or backports

  
bug
Something isn't working

  
component-docs
Docs / website issues filed here for tracking

  
component-platform
Portability layer and build scripts

  
component-test
Test framework and CI scripts

  
dependencies
Pull requests that update a dependency file

  
deprecation
needs deprecation announcement

  
DO-NOT-MERGE

   
enhancement
New feature or request

  
good first issue
Good for newcomers

  
help wanted
This issue is not being actively worked on, but PRs welcome.

  
needs-adoption
stalled PR that someone should pick up and complete

  
needs-backports
Backports are missing or are pending review and approval.

  
needs-ci
Needs to pass CI tests

  
needs-design-approval
Needs design discussion / approval

  
needs-info
An issue or PR which needs further info from the reporter / author

  
needs-preceding-pr
Requires another PR to be merged first

  
needs-review
Every commit must be reviewed by at least two team members

  
needs-reviewer
This PR needs someone to pick it up for review

  
needs-work

   
priority-high
High priority - will be reviewed soon

  
priority-low
Low priority - this may not receive review soon

  
priority-medium
Medium priority - this can be reviewed as time permits

  
priority-scheduled
This PR is big - it will require time to be scheduled for review

  
priority-very-high
Highest priority - prioritise this over other review work

  
resolved-after-1.1
The issue is resolved in development, but still present in 1.1 LTS.

  
single-reviewer
This PR qualifies for having only one reviewer

  
size-l
Estimated task size: large (2w+)

  
size-m
Estimated task size: medium (~1w)

  
size-optimisation

   
size-s
Estimated task size: small (~2d)

  
size-xs
Estimated task size: extra small (a few hours at most)

No labels
api-break
approved
bug
component-docs
component-platform
component-test
dependencies
deprecation
DO-NOT-MERGE
enhancement
good first issue
help wanted
needs-adoption
needs-backports
needs-ci
needs-design-approval
needs-info
needs-preceding-pr
needs-review
needs-reviewer
needs-work
priority-high
priority-low
priority-medium
priority-scheduled
priority-very-high
resolved-after-1.1
single-reviewer
size-l
size-m
size-optimisation
size-s
size-xs
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
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!403
No description provided.