BLAKE2 refactoring of common code bringing variable length support to BLAKE2s #22444

nabijaczleweli · 2023-10-19T22:17:18Z

The first three patches are functional no-ops, and just move the BLAKE2B stuff from being declared directly into a macro.

The fourth patch replaces the manual implementation of BLAKE2s with a call to the macro, thus matching them both in funxionality. This effectively completes 786b9a8.

The fifth patch explicitly documents what I discovered the hard way over three hours yesterday:

openssl has EVP_DigestFinalXOF() which is a shorthand for setting "xoflen" to the argument and then running EVP_DigestFinal()
this matches how SHAKE works (output length has no bearing on the hashing, so you can set "xoflen" at any time)
BLAKE2 puts the output length into the parameter block, which is part of the seed
naturally, you need to pass the output length to the init, so you set "xoflen" in the params block for EVP_DigestInit_ex2()
setting it after init and before the first byte does nothing, but you can still do it
EVP_DigestFinalXOF() has no bearing on it at all, but you can still pass it there. or you can just use EVP_DigestFinal()
if you didn't set it in EVP_DigestInit_ex2() but did pass it to EVP_DigestFinalXOF(), you just get wrong data (full-width hash, but truncated)
support for "xoflen" in BLAKE2b appeared in 786b9a8 in 2021, and is only in openssl-3.2.0-alpha[12] tags
EVP_DigestInit_ex2() doesn't error if it gets a parameter the hash function doesn't understand
I know this because on Bookworm openssl 3.0.11, code with full error-checking and EVP_DigestFinal() produces a truncated hash, and on upstream openssl HEAD the same code produces the correct short hash
you only get an error if you use EVP_DigestFinalXOF() which you shouldn't be using since it does nothing but implies it does

This all could've been avoided with a startlingly small patch that documents it (+20-4, and most of that +20 is in EVP_MD-BLAKE2.pod which is mostly copied from -SHAKE). The first four patches are really in service of not having to differentiate between b2s and b2b in the manual.

This also means that EVP_DigestInit_ex2() blindly accepting whatever it doesn't understand is an oddly dangerous design?

t8m · 2023-10-20T09:53:44Z

@mattcaswell I actually think this should go in 3.2.0

The refactoring is fairly straightforward and the xoflen support in 2b only is quite surprising.

The documentation is clearly needed as the xoflen handling in Blake is very much different from SHAKE.

tmshort

LGTM

paulidale · 2023-10-23T00:41:27Z

@slontis, does this impact your proposal for the digest squeeze function?

doc/man7/EVP_MD-BLAKE2.pod

paulidale · 2023-10-23T00:48:48Z

OTC: should we classify these as XOF and use xoflen for the digest's output size? The alternative is that they are not XOF and use size. They have no capability for the squeeze functionality.

openssl-machine · 2023-10-23T02:00:14Z

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

Tarnum-tst · 2023-10-30T12:13:35Z

This PR conflicts with 6a0ae39
So needs rebasing/fixing.

nabijaczleweli · 2023-10-30T13:56:32Z

rebased

…size support like 786b9a8 Test data obtained with libb2 with the following programs: ==> b2.c <== #include <blake2.h> #include <unistd.h> int main() { char buf[16] = {}; blake2s(buf, 0, 0, 16, 0, 0); write(1, buf, 16); } ==> b3.c <== #include <blake2.h> #include <unistd.h> int main() { char buf[10] = {}; blake2s(buf, "\x61", 0, 10, 1, 0); write(1, buf, 10); }

t8m · 2023-10-30T16:46:16Z

@paulidale AFAIK the OTC hold question is not relevant anymore. Can you please remove the label if you agree?

t8m · 2023-10-30T16:48:04Z

@nabijaczleweli I think you'll also need to modify EVP_MD-Blake2 manpage and I'd kindly ask you to add a CHANGES.md entry about adding the variable size support to Blake2s.

nabijaczleweli · 2023-10-30T22:14:00Z

Well, if we're going that far, I should probably document that BLAKE2b also has it? 786b9a8 doesn't have any docs in it, and so it's not mentioned in CHANGES.md.

paulidale · 2023-10-30T23:00:36Z

Hold removed, we should have removed it after the discussion.

For the record, the OTC decided that this wasn't an XOF digest and that the digest size parameter should be used to specify the length. This change to be made to the pre-existing digest before 3.2 beta series releases (because the variable length change hadn't been in a release, it could be updated so long as it was before the API freeze at the first 3.2 beta release).

nabijaczleweli · 2023-10-30T23:11:43Z

EVP_MD-BLAKE2(7) was already modified with text much like was in this PR in 6a0ae39#diff-a94d99d3c13153981b6da8a09b5d5fefcb3f8d6016c8cf0752f44fb30addc919.

CHANGES.md entry added.

t8m · 2023-10-31T10:49:05Z

CHANGES.md

+ * The BLAKE2b and BLAKE2s hash algorithms support a configurable output length
+   by setting the "size" parameter.
+
+   *Čestmír Kalina, Tomáš Mráz, and Ahelenia Ziemiańska*
+


Unfortunately this needs two entries. One in ### Changes between 3.1 and 3.2 for BLAKE2b and another in changes between 3.2 and 3.3 for BLAKE2s. Also the first one needs to be in a separate PR so it gets merged to the 3.2 branch as well.

does it? I only see 786b9a8 in openssl-3.2.0-{alpha{1,2},beta1} tags, and it doesn't look backported into openssl-3.1.4 under another SHA either?

No, I am talking about changes that go into 3.2 (Blake2b is variable output length there) and 3.3 (Blake2s gets this support as well by your PR).

yep, misread and misunderstood. fixed

slontis · 2023-11-05T23:22:49Z

providers/implementations/digests/blake2_prov.c

+            ERR_raise(ERR_LIB_PROV, PROV_R_FAILED_TO_GET_PARAMETER); \
+            return 0; \
+        } \
+        if (size < 1 || size > UINT8_MAX) { \


Is this upper constraint correct.. RFC 7693 seems to have 1 <= outlen <=64

libb2 also refuses lengths longer than the hash, but the paper says "put 0xFFFFFFFF in there if you don't know" () – I daren't have a take on this, I'm just adding backslashes.

slontis · 2023-11-05T23:32:38Z

test/recipes/30-test_evp_data/evpmd_blake.txt

+Input = 61
+OutputSize = 10
+Output = b60d322755eebca92b5e
+


If there is an upper bound on the output length there should be failure checks here..

Similarly, this just mimicks the blake2b tests at the end of the file (but obviously with blake2s outputs). If the limit in the formerly-blake2b code is appropriate, then maybe. No limit is documented afaict

openssl-machine · 2023-11-06T22:00:21Z

This pull request is ready to merge

t8m · 2023-11-08T08:38:40Z

I am merging this as @slontis concerns should IMO be resolved in a separate PR as they are already relevant for Blake2b.

t8m · 2023-11-08T08:44:31Z

Merged to the master branch with partially squashing the commits. Thank you for your contribution.

Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #22444)

This avoids code duplication and provides variable-size support for BLAKE2s like 786b9a8 Test data obtained with libb2 with the following programs: ==> b2.c <== #include <blake2.h> #include <unistd.h> int main() { char buf[16] = {}; blake2s(buf, 0, 0, 16, 0, 0); write(1, buf, 16); } ==> b3.c <== #include <blake2.h> #include <unistd.h> int main() { char buf[10] = {}; blake2s(buf, "\x61", 0, 10, 1, 0); write(1, buf, 10); } Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #22444)

Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #22444)

Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl/openssl#22444) Signed-off-by: fly2x <fly2x@hitls.org>

This avoids code duplication and provides variable-size support for BLAKE2s like 786b9a8 Test data obtained with libb2 with the following programs: ==> b2.c <== #include <blake2.h> #include <unistd.h> int main() { char buf[16] = {}; blake2s(buf, 0, 0, 16, 0, 0); write(1, buf, 16); } ==> b3.c <== #include <blake2.h> #include <unistd.h> int main() { char buf[10] = {}; blake2s(buf, "\x61", 0, 10, 1, 0); write(1, buf, 10); } Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl/openssl#22444) Signed-off-by: fly2x <fly2x@hitls.org>

Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl/openssl#22444) Signed-off-by: fly2x <fly2x@hitls.org>

mattcaswell added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member labels Oct 20, 2023

mattcaswell added this to the Post 3.2.0 milestone Oct 20, 2023

t8m added triaged: documentation The issue/pr deals with documentation (errors) triaged: refactor The issue/pr requests/implements refactoring tests: present The PR has suitable tests present labels Oct 20, 2023

t8m previously approved these changes Oct 20, 2023

View reviewed changes

t8m removed the approval: otc review pending This pull request needs review by an OTC member label Oct 20, 2023

tmshort previously approved these changes Oct 22, 2023

View reviewed changes

tmshort added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Oct 22, 2023

paulidale requested changes Oct 23, 2023

View reviewed changes

doc/man7/EVP_MD-BLAKE2.pod Outdated Show resolved Hide resolved

paulidale added the hold: need otc decision The OTC needs to make a decision label Oct 23, 2023

nabijaczleweli added 3 commits October 30, 2023 14:43

Lift BLAKE2B provider definition into a macro

ac7e140

Lift ossl_blake2b512_init into the macro too

d5f4ff0

Move the BLAKE2B XOF support stuff from 786b9a8 to the macro as well

9f13572

nabijaczleweli added 2 commits October 30, 2023 15:23

Document BLAKE2's variable size

9bc9da8

t8m added approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member and removed approval: done This pull request has the required number of approvals labels Oct 30, 2023

t8m added the triaged: feature The issue/pr requests/adds a feature label Oct 30, 2023

paulidale removed the hold: need otc decision The OTC needs to make a decision label Oct 30, 2023

t8m reviewed Oct 31, 2023

View reviewed changes

CHANGES.md: note BLAKE2s supports BLAKE2b-like "size" setting

e621c43

nabijaczleweli mentioned this pull request Oct 31, 2023

CHANGES.md: document BLAKE2b's "size"-setting support #22583

Closed

1 task

t8m approved these changes Nov 3, 2023

View reviewed changes

t8m removed the approval: otc review pending This pull request needs review by an OTC member label Nov 3, 2023

paulidale approved these changes Nov 5, 2023

View reviewed changes

paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Nov 5, 2023

slontis reviewed Nov 5, 2023

View reviewed changes

openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Nov 6, 2023

t8m closed this Nov 8, 2023

openssl-machine pushed a commit that referenced this pull request Nov 8, 2023

Lift BLAKE2B provider definition into a macro

8349c02

Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #22444)

openssl-machine pushed a commit that referenced this pull request Nov 8, 2023

Document BLAKE2's variable size

7966592

Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #22444)

openssl-machine pushed a commit that referenced this pull request Nov 8, 2023

CHANGES.md: note BLAKE2s supports BLAKE2b-like "size" setting

7cf75e5

Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #22444)

guidovranken mentioned this pull request Nov 12, 2023

HKDF + BLAKE2S256 outputs uninitialized memory #22708

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BLAKE2 refactoring of common code bringing variable length support to BLAKE2s #22444

BLAKE2 refactoring of common code bringing variable length support to BLAKE2s #22444

nabijaczleweli commented Oct 19, 2023 •

edited

t8m commented Oct 20, 2023

tmshort left a comment

paulidale commented Oct 23, 2023

paulidale commented Oct 23, 2023 •

edited

openssl-machine commented Oct 23, 2023

Tarnum-tst commented Oct 30, 2023

nabijaczleweli commented Oct 30, 2023

t8m commented Oct 30, 2023

t8m commented Oct 30, 2023

nabijaczleweli commented Oct 30, 2023 •

edited

paulidale commented Oct 30, 2023

nabijaczleweli commented Oct 30, 2023

t8m Oct 31, 2023

nabijaczleweli Oct 31, 2023

t8m Oct 31, 2023

nabijaczleweli Oct 31, 2023

slontis Nov 5, 2023

nabijaczleweli Nov 5, 2023

slontis Nov 5, 2023

nabijaczleweli Nov 6, 2023

openssl-machine commented Nov 6, 2023

t8m commented Nov 8, 2023

t8m commented Nov 8, 2023

BLAKE2 refactoring of common code bringing variable length support to BLAKE2s #22444

BLAKE2 refactoring of common code bringing variable length support to BLAKE2s #22444

Conversation

nabijaczleweli commented Oct 19, 2023 • edited

t8m commented Oct 20, 2023

tmshort left a comment

Choose a reason for hiding this comment

paulidale commented Oct 23, 2023

paulidale commented Oct 23, 2023 • edited

openssl-machine commented Oct 23, 2023

Tarnum-tst commented Oct 30, 2023

nabijaczleweli commented Oct 30, 2023

t8m commented Oct 30, 2023

t8m commented Oct 30, 2023

nabijaczleweli commented Oct 30, 2023 • edited

paulidale commented Oct 30, 2023

nabijaczleweli commented Oct 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openssl-machine commented Nov 6, 2023

t8m commented Nov 8, 2023

t8m commented Nov 8, 2023

nabijaczleweli commented Oct 19, 2023 •

edited

paulidale commented Oct 23, 2023 •

edited

nabijaczleweli commented Oct 30, 2023 •

edited