Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EVP_chacha20 does not match RFC 7539 or documentation in nonce/counter split #21095

Closed
davidben opened this issue May 31, 2023 · 3 comments
Closed
Labels
branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 help wanted triaged: bug The issue/pr is/fixes a bug triaged: documentation The issue/pr deals with documentation (errors)

Comments

@davidben
Copy link
Contributor

RFC 7539 specified ChaCha20 with a 96-bit nonce and a 32-bit counter. Only 32 bits of the input is incremented across each block.
https://www.rfc-editor.org/rfc/rfc7539.html#section-2.4

The original paper didn't specify a split but, prior to the RFC, 64-bit nonce with 64-bit counter was also common. A 96/32 and 64/64 split are very similar primitives, but not quite the same. OpenSSL's documentation says it implements the RFC formulation:

The ChaCha20 stream cipher. The key length is 256 bits, the IV is 128 bits long. The first 32 bits consists of a counter in little-endian order followed by a 96 bit nonce.

https://www.openssl.org/docs/manmaster/man3/EVP_chacha20.html

However, the implementation doesn't match this and uses a 64-bit counter.

key->counter[0] = ctr32;
if (ctr32 == 0) key->counter[1]++;

See also C2SP/wycheproof#90

@davidben davidben added the issue: bug report The issue was opened to report a bug label May 31, 2023
@t8m
Copy link
Member

t8m commented May 31, 2023

I assume we should just document the current implementation. A new feature would be to add a parameter to change to the 32/96 bit split implementation.

@t8m t8m added triaged: documentation The issue/pr deals with documentation (errors) branch: master Merge to master branch triaged: bug The issue/pr is/fixes a bug branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 and removed issue: bug report The issue was opened to report a bug labels May 31, 2023
paulidale added a commit to paulidale/openssl that referenced this issue May 31, 2023
@bernd-edlinger
Copy link
Member

But this does not really affect ChaCha20_Poly1305, since the counter[0] starts always at 0,
and an overflow is therefore (almost) impossible. Right?

@paulidale
Copy link
Contributor

paulidale commented Jun 1, 2023

That's my understanding.

From what's discussed in C2SP/wycheproof#90, it seems like any overflow is extremely unlikely unless someone is trying to shoot themselves.

I don't think that precludes us from implementing it in a standards compliant manner.

openssl-machine pushed a commit that referenced this issue Jun 6, 2023
Fixes #21095

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #21098)

(cherry picked from commit c69756e)
openssl-machine pushed a commit that referenced this issue Jun 6, 2023
Fixes #21095

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #21098)

(cherry picked from commit c69756e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 help wanted triaged: bug The issue/pr is/fixes a bug triaged: documentation The issue/pr deals with documentation (errors)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants