Add support for Blake2s and Blake2b hash algorithms #867

Open
valeriosetti wants to merge 20 commits from valeriosetti/blake2 into development
valeriosetti commented 2026-08-11 05:57:33 -05:00 (Migrated from github.com)

Description

Blake2 was initially proposed in RFC 7693. It provides:

  • hash and MAC algorithms;
  • 256 or 512 bits of output;
  • single or parallel execution.

PSA Crypto API added Blake2 into the specs in version 1.5:

  • PSA_ALG_BLAKE2_MAC -> MAC algorithm
  • PSA_ALG_BLAKE2S_HASH256 and PSA_ALG_BLAKE2B_HASH512 -> single execution
  • PSA_ALG_BLAKE2SP_HASH256 and PSA_ALG_BLAKE2BP_HASH512 -> parallel execution

Adding support for all of this in a single PR would mean having a very large PR which is difficult to review. As a consequence the scope of this PR is limited to add only hash support (MAC is done in this follow-up PR) and for the single threaded case (i.e. no parallel).

Prerequisite PRs:

  1. https://github.com/Mbed-TLS/mbedtls-framework/pull/316
  2. https://github.com/Mbed-TLS/mbedtls/pull/10921

A follow-up PR (https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/898) will add support for MAC.

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.

## Description Blake2 was initially proposed in [RFC 7693](https://datatracker.ietf.org/doc/html/rfc7693.html). It provides: - hash and MAC algorithms; - 256 or 512 bits of output; - single or parallel execution. PSA Crypto API added Blake2 into the specs in [version 1.5](https://arm-software.github.io/psa-api/crypto/1.5/): - PSA_ALG_BLAKE2_MAC -> MAC algorithm - PSA_ALG_BLAKE2S_HASH256 and PSA_ALG_BLAKE2B_HASH512 -> single execution - PSA_ALG_BLAKE2SP_HASH256 and PSA_ALG_BLAKE2BP_HASH512 -> parallel execution Adding support for all of this in a single PR would mean having a very large PR which is difficult to review. As a consequence the scope of this PR is limited to add only hash support (MAC is done in [this follow-up PR](https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/898)) and for the single threaded case (i.e. no parallel). Prerequisite PRs: 1. https://github.com/Mbed-TLS/mbedtls-framework/pull/316 2. https://github.com/Mbed-TLS/mbedtls/pull/10921 A follow-up PR (https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/898) will add support for MAC. ## 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** provided - [ ] **framework PR** provided: https://github.com/Mbed-TLS/mbedtls-framework/pull/316 - [ ] **TF-PSA-Crypto development PR** provided: this one - [ ] **TF-PSA-Crypto 1.1 PR** not required because: it's an enhancement, so I would say this is not required - [ ] **mbedtls development PR** provided: https://github.com/Mbed-TLS/mbedtls/pull/10921 - [ ] **mbedtls 4.1 PR** not required because: it's an enhancement, so I would say this is not required - [ ] **mbedtls 3.6 PR** not required because: definitely no! - **tests** provided
ronald-cron-arm (Migrated from github.com) reviewed 2026-08-11 05:57:33 -05:00
bjwtaylor (Migrated from github.com) reviewed 2026-08-18 04:23:52 -05:00
bjwtaylor (Migrated from github.com) left a comment

Some comments, questions and suggestions.

Some comments, questions and suggestions.
@ -22,6 +22,8 @@
#define PSA_WANT_ALG_CHACHA20_POLY1305 1
#define PSA_WANT_ALG_CMAC 1
bjwtaylor (Migrated from github.com) commented 2026-08-18 01:49:07 -05:00

Putting this here as I can't comment on the commit messages commits 9d20167eda738062dba18702ba0173ef04f81f3d and 212901fe98d9b298eb0ea88b12a8a1db4685c959 refer blake2p should this be blake2b?

Putting this here as I can't comment on the commit messages commits 9d20167eda738062dba18702ba0173ef04f81f3d and 212901fe98d9b298eb0ea88b12a8a1db4685c959 refer blake2p should this be blake2b?
bjwtaylor (Migrated from github.com) commented 2026-08-18 02:12:21 -05:00

Do the failing tests need investigating or is this expected?

Do the failing tests need investigating or is this expected?
@ -22,6 +22,8 @@
#define PSA_WANT_ALG_CHACHA20_POLY1305 1
#define PSA_WANT_ALG_CMAC 1
#define PSA_WANT_ALG_CTR 1
bjwtaylor (Migrated from github.com) commented 2026-08-18 02:04:26 -05:00

It will also probably need a changelog

It will also probably need a changelog
@ -0,0 +50,4 @@
} tf_psa_crypto_blake2s_context;
#if defined(MBEDTLS_PSA_BUILTIN_ALG_BLAKE2S_HASH256)
bjwtaylor (Migrated from github.com) commented 2026-08-18 02:11:13 -05:00

Is there a reason there is no doxygen for these functions? The whole implementation also lacks comments to explain it's operation and aid future maintainability. Is there a reason for this? I guess it's private, so we don't want the docs in the public documentation. It would still be useful to have some docs though. I've attached a patch of some docs i added to aid the review that might be a good starting point.

documentation.patch

Is there a reason there is no doxygen for these functions? The whole implementation also lacks comments to explain it's operation and aid future maintainability. Is there a reason for this? I guess it's private, so we don't want the docs in the public documentation. It would still be useful to have some docs though. I've attached a patch of some docs i added to aid the review that might be a good starting point. [documentation.patch](https://github.com/user-attachments/files/31173577/documentation.patch)
@ -0,0 +1,389 @@
/**
bjwtaylor (Migrated from github.com) commented 2026-08-18 03:27:38 -05:00

For the benefit other other reviewers this implementation flags as having similarities to nanonext 0.10.4 and PlantUML 1.2023.11 nanonext includes mbedtls and the simularities in PlantUML appear to be related to the fact they are both based RFC 7693 and not indicative of them being related implementations.

For the benefit other other reviewers this implementation flags as having similarities to nanonext 0.10.4 and PlantUML 1.2023.11 nanonext includes mbedtls and the simularities in PlantUML appear to be related to the fact they are both based RFC 7693 and not indicative of them being related implementations.
bjwtaylor (Migrated from github.com) commented 2026-08-18 04:20:52 -05:00

It might be useful to put a scope at the top for future reference, something like

  • This implements the sequential modes from RFC 7693, Sections 2.6 and 2.7.
  • Tree hashing parameters are not supported.
    Maybe also state that tree hashing is not supported due to it not being in the PSA API what do yoiu think?
It might be useful to put a scope at the top for future reference, something like * This implements the sequential modes from RFC 7693, Sections 2.6 and 2.7. * Tree hashing parameters are not supported. Maybe also state that tree hashing is not supported due to it not being in the PSA API what do yoiu think?
@ -0,0 +106,4 @@
static void tf_psa_crypto_blake2s_compress(tf_psa_crypto_blake2s_context *ctx, int last)
{
int i;
uint32_t v[16], m[16];
bjwtaylor (Migrated from github.com) commented 2026-08-18 03:49:13 -05:00

These two variables could contain sensitive data and although they are not available through the API, it probably makes sense to wipe them when your done with them

These two variables could contain sensitive data and although they are not available through the API, it probably makes sense to wipe them when your done with them
@ -0,0 +142,4 @@
mbedtls_platform_zeroize(m, sizeof(m));
}
void tf_psa_crypto_blake2s_update(tf_psa_crypto_blake2s_context *ctx,
bjwtaylor (Migrated from github.com) commented 2026-08-18 04:06:02 -05:00

Is there a reason we copy one byte at a time instead of copying one block at a time?

Is there a reason we copy one byte at a time instead of copying one block at a time?
@ -0,0 +281,4 @@
static void tf_psa_crypto_blake2b_compress(tf_psa_crypto_blake2b_context *ctx, int last)
{
int i;
uint64_t v[16], m[16];
bjwtaylor (Migrated from github.com) commented 2026-08-18 03:49:50 -05:00

These two variables could contain sensitive data and although they are not available through the API, it probably makes sense to wipe them when your done with them

These two variables could contain sensitive data and although they are not available through the API, it probably makes sense to wipe them when your done with them
@ -0,0 +317,4 @@
mbedtls_platform_zeroize(m, sizeof(m));
}
void tf_psa_crypto_blake2b_update(tf_psa_crypto_blake2b_context *ctx,
bjwtaylor (Migrated from github.com) commented 2026-08-18 04:05:41 -05:00

Is there a reason we copy one byte at a time instead of copying one block at a time?

Is there a reason we copy one byte at a time instead of copying one block at a time?
@ -950,0 +958,4 @@
*
* \return 1 if alg is a BLAKE2 hash algorithm, 0 otherwise.
*/
#define PSA_ALG_IS_BLAKE2_HASH(alg) \
bjwtaylor (Migrated from github.com) commented 2026-08-18 01:44:59 -05:00

I think this requires a parentheses around the expression as otherwise it is not safe to negate or compose

I think this requires a parentheses around the expression as otherwise it is not safe to negate or compose
@ -0,0 +1,773 @@
Blake2s RFC 7693 Appendix A ("abc")
blake2b:"":"616263":"BA80A53F981C4D0D6A2797B69F12F6E94C212F14685AC4B74B12BB6FDBFFA2D17D87C5392AAB792DC252D5DE4533CC9518D38AA8DBF1925AB92386EDD4009923"
Blake2b KAT #0 (msg len=0)
bjwtaylor (Migrated from github.com) commented 2026-08-18 02:02:34 -05:00

It might be good to add the RFC 7693 Appendix A: BLAKE2b-512("abc") and RFC 7693 Appendix B: BLAKE2s-256("abc") to exercise unkeyed mode.

It might be good to add the RFC 7693 Appendix A: BLAKE2b-512("abc") and RFC 7693 Appendix B: BLAKE2s-256("abc") to exercise unkeyed mode.
@ -0,0 +1,103 @@
/* BEGIN_HEADER */
bjwtaylor (Migrated from github.com) commented 2026-08-18 02:32:30 -05:00

It might be useful to extend the testing to inclulde:

Severity Area Missing coverage / action
Major Legacy MD API Add expected-output tests for empty, block-1, block, and block+1 inputs; one-shot, multipart, clone divergence, HMAC/reset, and allocation failures. Cover both variants.
Minor PSA hash contracts Add psa_hash_compute() tests, zero/short buffers, output sanitization, invalid expected hashes, invalid state transitions, abort/reuse, active-target cloning, and driver-error propagation.
Minor PSA HMAC Add permanent BLAKE2s/BLAKE2b one-shot, multipart, verify, abort, and reset tests. Include malformed MACs, short buffers, policy/usage failures, and varied key lengths.
Minor PSA signatures Add ECDSA message/pre-hash tests, RSA-PSS where supported, invalid signatures/digest lengths, short buffers, and policy/usage failures. Verify that RSA PKCS#1 v1.5 returns PSA_ERROR_NOT_SUPPORTED.
Minor Private BLAKE2 API Test private one-shot functions, error propagation, shortened output lengths, varied key lengths, retry after a short finish buffer, and seeded counter carry.
Contract decision Invalid pointers Decide whether NULL with a nonzero input/key length and a NULL output are caller preconditions. Document them or add validation and negative tests.
Suggestion Reusable test matrix Instantiate the common PSA hash success and failure tests for every enabled fixed-output hash.
Suggestion Coverage documentation Document that the exhaustive vectors validate digest correctness and boundaries, but not public API error contracts.
It might be useful to extend the testing to inclulde: | Severity | Area | Missing coverage / action | | --- | --- | --- | | Major | Legacy MD API | Add expected-output tests for empty, block-1, block, and block+1 inputs; one-shot, multipart, clone divergence, HMAC/reset, and allocation failures. Cover both variants. | | Minor | PSA hash contracts | Add `psa_hash_compute()` tests, zero/short buffers, output sanitization, invalid expected hashes, invalid state transitions, abort/reuse, active-target cloning, and driver-error propagation. | | Minor | PSA HMAC | Add permanent BLAKE2s/BLAKE2b one-shot, multipart, verify, abort, and reset tests. Include malformed MACs, short buffers, policy/usage failures, and varied key lengths. | | Minor | PSA signatures | Add ECDSA message/pre-hash tests, RSA-PSS where supported, invalid signatures/digest lengths, short buffers, and policy/usage failures. Verify that RSA PKCS#1 v1.5 returns `PSA_ERROR_NOT_SUPPORTED`. | | Minor | Private BLAKE2 API | Test private one-shot functions, error propagation, shortened output lengths, varied key lengths, retry after a short finish buffer, and seeded counter carry. | | Contract decision | Invalid pointers | Decide whether `NULL` with a nonzero input/key length and a `NULL` output are caller preconditions. Document them or add validation and negative tests. | | Suggestion | Reusable test matrix | Instantiate the common PSA hash success and failure tests for every enabled fixed-output hash. | | Suggestion | Coverage documentation | Document that the exhaustive vectors validate digest correctness and boundaries, but not public API error contracts. |
bjwtaylor (Migrated from github.com) commented 2026-08-18 01:46:22 -05:00

Is it intentional to drop SHA3_512 support?

Is it intentional to drop SHA3_512 support?
valeriosetti (Migrated from github.com) reviewed 2026-08-31 09:47:14 -05:00
@ -22,6 +22,8 @@
#define PSA_WANT_ALG_CHACHA20_POLY1305 1
#define PSA_WANT_ALG_CMAC 1
#define PSA_WANT_ALG_CTR 1
valeriosetti (Migrated from github.com) commented 2026-08-31 09:47:14 -05:00

Yeah, that's marked as TODO in the description of the PR indeed :)

Yeah, that's marked as `TODO` in the description of the PR indeed :)
valeriosetti (Migrated from github.com) reviewed 2026-08-31 09:50:25 -05:00
@ -22,6 +22,8 @@
#define PSA_WANT_ALG_CHACHA20_POLY1305 1
#define PSA_WANT_ALG_CMAC 1
valeriosetti (Migrated from github.com) commented 2026-08-31 09:50:25 -05:00

Do the failing tests need investigating

Failing tests are due to missing changes in mbedtls. CI in https://github.com/Mbed-TLS/mbedtls/pull/10921 was green when it was including this PR.

> Do the failing tests need investigating Failing tests are due to missing changes in `mbedtls`. CI in https://github.com/Mbed-TLS/mbedtls/pull/10921 was green when [it was including this PR](https://github.com/Mbed-TLS/mbedtls/pull/10921#issuecomment-5292045443).
valeriosetti (Migrated from github.com) reviewed 2026-09-01 07:29:20 -05:00
valeriosetti (Migrated from github.com) commented 2026-09-01 07:29:19 -05:00

Oops, no, sorry

Oops, no, sorry
valeriosetti (Migrated from github.com) reviewed 2026-09-01 07:36:16 -05:00
@ -22,6 +22,8 @@
#define PSA_WANT_ALG_CHACHA20_POLY1305 1
#define PSA_WANT_ALG_CMAC 1
valeriosetti (Migrated from github.com) commented 2026-09-01 07:36:16 -05:00

ouch, yes, sorry also for this one :/

ouch, yes, sorry also for this one :/
valeriosetti (Migrated from github.com) reviewed 2026-09-01 09:53:31 -05:00
@ -0,0 +50,4 @@
} tf_psa_crypto_blake2s_context;
#if defined(MBEDTLS_PSA_BUILTIN_ALG_BLAKE2S_HASH256)
valeriosetti (Migrated from github.com) commented 2026-09-01 09:53:31 -05:00

Is there a reason there is no doxygen for these functions?

Lazyness during initial development followed by oversight when finalizing the work ;)

> Is there a reason there is no doxygen for these functions? Lazyness during initial development followed by oversight when finalizing the work ;)
valeriosetti (Migrated from github.com) reviewed 2026-09-01 10:53:25 -05:00
@ -0,0 +106,4 @@
static void tf_psa_crypto_blake2s_compress(tf_psa_crypto_blake2s_context *ctx, int last)
{
int i;
uint32_t v[16], m[16];
valeriosetti (Migrated from github.com) commented 2026-09-01 10:53:25 -05:00

Not sure about this. m contains the input data that has been passed to tf_psa_crypto_blake2s_update so IMO that's not really secret material. As for v that's used to compute the new internal state that's stored in ctx->state so I'm not sure this is really leaking any information that's not available anyway in the context.
What exactly are v and m leaking?

I mean, the algorithm is deterministic, so ideally given the key (if any) and the input material anyone knows exactly what the output will look like. Isn't it?

Not sure about this. `m` contains the input data that has been passed to `tf_psa_crypto_blake2s_update` so IMO that's not really secret material. As for `v` that's used to compute the new internal state that's stored in `ctx->state` so I'm not sure this is really leaking any information that's not available anyway in the context. What exactly are `v` and `m` leaking? I mean, the algorithm is deterministic, so ideally given the key (if any) and the input material anyone knows exactly what the output will look like. Isn't it?
valeriosetti (Migrated from github.com) reviewed 2026-09-01 10:53:53 -05:00
@ -0,0 +281,4 @@
static void tf_psa_crypto_blake2b_compress(tf_psa_crypto_blake2b_context *ctx, int last)
{
int i;
uint64_t v[16], m[16];
valeriosetti (Migrated from github.com) commented 2026-09-01 10:53:53 -05:00

Same as above

Same as [above](https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/867#discussion_r3905897541)
valeriosetti (Migrated from github.com) reviewed 2026-09-01 11:36:54 -05:00
@ -0,0 +317,4 @@
mbedtls_platform_zeroize(m, sizeof(m));
}
void tf_psa_crypto_blake2b_update(tf_psa_crypto_blake2b_context *ctx,
valeriosetti (Migrated from github.com) commented 2026-09-01 11:36:54 -05:00

Honestly that was the design proposed in the RFC and I just took that as working solution. I knew it could perhaps be optimized, but I didn't bothered trying that at the time. I can do this now however!

instead of copying one block at a time

On this please note that the input data is not necessarily provided in blocks whose size is multiple of 32 or 64, so some handling has to be done anyway.

Honestly that was the design proposed in the RFC and I just took that as working solution. I knew it could perhaps be optimized, but I didn't bothered trying that at the time. I can do this now however! > instead of copying one block at a time On this please note that the input data is not necessarily provided in blocks whose size is multiple of 32 or 64, so some handling has to be done anyway.
valeriosetti (Migrated from github.com) reviewed 2026-09-01 11:37:06 -05:00
@ -0,0 +142,4 @@
mbedtls_platform_zeroize(m, sizeof(m));
}
void tf_psa_crypto_blake2s_update(tf_psa_crypto_blake2s_context *ctx,
valeriosetti (Migrated from github.com) commented 2026-09-01 11:37:06 -05:00

Same as above

Same as [above](https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/867#discussion_r3906269484)
valeriosetti (Migrated from github.com) reviewed 2026-09-01 17:10:09 -05:00
@ -0,0 +1,389 @@
/**
valeriosetti (Migrated from github.com) commented 2026-09-01 17:10:09 -05:00

Tree hashing parameters are not supported

IIRC that´s not part of the RFC 7963. It's something they implemented in the reference design in https://github.com/BLAKE2/BLAKE2, but it's not part of the RFC. See the end of this section.

> Tree hashing parameters are not supported IIRC that´s not part of the RFC 7963. It's something they implemented in the reference design in https://github.com/BLAKE2/BLAKE2, but it's not part of the RFC. See the end of[ this section](https://www.rfc-editor.org/info/rfc7693/#section-2.5).
valeriosetti (Migrated from github.com) reviewed 2026-09-02 06:22:54 -05:00
@ -0,0 +1,103 @@
/* BEGIN_HEADER */
valeriosetti (Migrated from github.com) commented 2026-09-02 06:22:54 -05:00
  • I added some MD testing. IMO this doesn't need to extensive because Blake2[s|b] already have their own testing. In MD we just need to ensure MD is able to call Blake2 when/if needed.
  • No HMAC test added because HMAC for Blake2 is not supported yet (and it's not the same as other hash algorithms).
  • Added some minimal testing for ECDSA, RSA-PSS and RSA PKCS#1 v1.5 using Blake2[s|p]
- I added some MD testing. IMO this doesn't need to extensive because Blake2[s|b] already have their own testing. In MD we just need to ensure MD is able to call Blake2 when/if needed. - No HMAC test added because HMAC for Blake2 is not supported yet (and it's not the same as other hash algorithms). - Added some minimal testing for ECDSA, RSA-PSS and RSA PKCS#1 v1.5 using Blake2[s|p]
valeriosetti commented 2026-09-10 15:27:51 -05:00 (Migrated from github.com)

Thanks to the merge of all prerequisite PRs now the CI is green and this PR is officially ready for review :)

Thanks to the merge of all prerequisite PRs now the CI is green and this PR is officially ready for review :)
bjwtaylor (Migrated from github.com) reviewed 2026-09-16 08:34:28 -05:00
@ -0,0 +317,4 @@
mbedtls_platform_zeroize(m, sizeof(m));
}
void tf_psa_crypto_blake2b_update(tf_psa_crypto_blake2b_context *ctx,
bjwtaylor (Migrated from github.com) commented 2026-09-16 08:34:28 -05:00

So, I think it's a nice to have. Not sure if it's essential though, I would see what the second reviewer thinks and at least add an additional issue that's it's further work.

So, I think it's a nice to have. Not sure if it's essential though, I would see what the second reviewer thinks and at least add an additional issue that's it's further work.
bjwtaylor (Migrated from github.com) reviewed 2026-09-16 08:39:29 -05:00
@ -0,0 +1,389 @@
/**
bjwtaylor (Migrated from github.com) commented 2026-09-16 08:39:29 -05:00

Maybe something like this then:
/*

  • Implements sequential BLAKE2s and BLAKE2b as specified in RFC 7693.
  • Tree-hashing parameters and the parallel BLAKE2sp/BLAKE2bp variants
  • are not supported by this module.
    */
    ?
Maybe something like this then: /* * Implements sequential BLAKE2s and BLAKE2b as specified in RFC 7693. * Tree-hashing parameters and the parallel BLAKE2sp/BLAKE2bp variants * are not supported by this module. */ ?
bjwtaylor (Migrated from github.com) reviewed 2026-09-16 08:45:25 -05:00
@ -0,0 +1,4 @@
Features
bjwtaylor (Migrated from github.com) commented 2026-09-16 08:45:08 -05:00

Should this be blake2b not 2p?

Should this be blake2b not 2p?
@ -0,0 +1,292 @@
/**
bjwtaylor (Migrated from github.com) commented 2026-09-16 08:43:52 -05:00

Is this HMAC or native keyed BLAKE2? Ditto on 66:133:180:360

Is this HMAC or native keyed BLAKE2? Ditto on 66:133:180:360
valeriosetti (Migrated from github.com) reviewed 2026-09-21 06:05:54 -05:00
@ -0,0 +1,292 @@
/**
valeriosetti (Migrated from github.com) commented 2026-09-21 06:05:54 -05:00

Oops, right, of course HMAC is not correct. OTOH using 'Blake2 MAC' all the times looks a bit too verbose, so I think I will go with just 'MAC'. Please, let me know if that doesn't look like a good idea to you.

Oops, right, of course HMAC is not correct. OTOH using 'Blake2 MAC' all the times looks a bit too verbose, so I think I will go with just 'MAC'. Please, let me know if that doesn't look like a good idea to you.
valeriosetti (Migrated from github.com) reviewed 2026-09-21 06:08:29 -05:00
@ -0,0 +1,4 @@
Features
valeriosetti (Migrated from github.com) commented 2026-09-21 06:08:28 -05:00

Yes, sorry. I know the correct version is 2b, but for some mysterious reason my mind keeps preferring 2p 🤦

Yes, sorry. I know the correct version is `2b`, but for some mysterious reason my mind keeps preferring `2p` :facepalm:
valeriosetti commented 2026-09-21 06:20:01 -05:00 (Migrated from github.com)

I rebased to resolve a conflict on the framework reference. While at this, since current framework being referenced from development is also newer than what this PR needs, I dropped the commit updating the framework reference. This should reduce the potential conflict surface with other changes.

I rebased to resolve a conflict on the `framework` reference. While at this, since current `framework` being referenced from `development` is also newer than what this PR needs, I dropped the commit updating the `framework` reference. This should reduce the potential conflict surface with other changes.
bjwtaylor (Migrated from github.com) reviewed 2026-09-25 05:47:02 -05:00
@ -0,0 +106,4 @@
static void tf_psa_crypto_blake2s_compress(tf_psa_crypto_blake2s_context *ctx, int last)
{
int i;
uint32_t v[16], m[16];
bjwtaylor (Migrated from github.com) commented 2026-09-25 05:47:02 -05:00

So, I think it depends a little on the use case. There is a possible exposure as follows:
tf_psa_crypto_blake2s_init(ctx, outlen, key, keylen)
→ copies the key into ctx->buf and marks the first block full
tf_psa_crypto_blake2s_update(ctx, message, length)
→ compresses that block when message bytes arrive
→ m[] is loaded from ctx->buf, so m[] contains the key bytes
→ the mixing rounds combine m[] with v[]
→ v[] and then ctx->state become key dependent
In some instances you are initialising this to NULL 0 such as drivers/builtin/src/psa_crypto_hash.c:177. So they are not an issue. Though in others such as the test and blake2.c:381 you are. So ultimately I think it depends if this init function is supposed to init with the real values or with NULL 0? Though it may be worth just zeroing them to be certain, what do you think?

So, I think it depends a little on the use case. There is a possible exposure as follows: tf_psa_crypto_blake2s_init(ctx, outlen, key, keylen) → copies the key into ctx->buf and marks the first block full tf_psa_crypto_blake2s_update(ctx, message, length) → compresses that block when message bytes arrive → m[] is loaded from ctx->buf, so m[] contains the key bytes → the mixing rounds combine m[] with v[] → v[] and then ctx->state become key dependent In some instances you are initialising this to NULL 0 such as drivers/builtin/src/psa_crypto_hash.c:177. So they are not an issue. Though in others such as the test and blake2.c:381 you are. So ultimately I think it depends if this init function is supposed to init with the real values or with NULL 0? Though it may be worth just zeroing them to be certain, what do you think?
bjwtaylor (Migrated from github.com) reviewed 2026-09-25 05:47:52 -05:00
bjwtaylor (Migrated from github.com) left a comment

As per further discussion

As per further discussion
valeriosetti (Migrated from github.com) reviewed 2026-09-25 09:05:39 -05:00
@ -0,0 +106,4 @@
static void tf_psa_crypto_blake2s_compress(tf_psa_crypto_blake2s_context *ctx, int last)
{
int i;
uint32_t v[16], m[16];
valeriosetti (Migrated from github.com) commented 2026-09-25 09:05:39 -05:00

Yeah, I get your point. I still think the attack scenario is a bit complex because you need to be able to read the stack of the thread doing Blake2, but at the same time you cannot dump the context (tf_psa_crypto_blake2s_context/tf_psa_crypto_blake2b_context - because the context contains the same material). So it should be like:

  1. let the Blake2 thread execute only one iteration loading the key and stop it there (do not send other data);
  2. read it's stack memory.

As far as I know step (2) should not be allowed in normal OSes like Linux (but I guess also all other ones).

However, since all other SHA algorithms are doing the same and what you proposed look like a safe approach, I implemented it.

Yeah, I get your point. I still think the attack scenario is a bit complex because you need to be able to read the stack of the thread doing Blake2, but at the same time you cannot dump the context (`tf_psa_crypto_blake2s_context`/`tf_psa_crypto_blake2b_context` - because the context contains the same material). So it should be like: 1. let the Blake2 thread execute only one iteration loading the key and stop it there (do not send other data); 2. read it's stack memory. As far as I know step (2) should not be allowed in normal OSes like Linux (but I guess also all other ones). However, since all other SHA algorithms are doing the same and what you proposed look like a safe approach, I [implemented it](https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/867/commits/ac77a1255a38d1be4c3fa25d925bd6428c9bc543).
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 'valeriosetti/blake2:valeriosetti/blake2'
git switch 'valeriosetti/blake2'

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 'valeriosetti/blake2'
git switch 'valeriosetti/blake2'
git rebase 'development'
git switch 'development'
git merge --ff-only 'valeriosetti/blake2'
git switch 'valeriosetti/blake2'
git rebase 'development'
git switch 'development'
git merge --no-ff 'valeriosetti/blake2'
git switch 'development'
git merge --squash 'valeriosetti/blake2'
git switch 'development'
git merge --ff-only 'valeriosetti/blake2'
git switch 'development'
git merge 'valeriosetti/blake2'
git push origin 'development'
Sign in to join this conversation.
No reviewers
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!867
No description provided.