Legrandin / pycryptodome

A self-contained cryptographic library for Python
https://www.pycryptodome.org
Other
2.74k stars 492 forks source link

XChaCha20 Inconsistencies #811

Open Michael-Coveware opened 1 month ago

Michael-Coveware commented 1 month ago

The implementations for XChaCha20 and its documentation is all over the place when it comes to Draft 02 vs Draft 03. There is a very minor, but critical difference between the two revisions... the counter value used (0 vs. 1 respectively).

The documentation and code comments for XChaCha20 currently link to Draft 03.

https://github.com/Legrandin/pycryptodome/blob/a6b6ecd8959d155eeec46db664c6817359d50ac7/Doc/src/cipher/chacha20.rst?plain=1#L15-L17

However, when using XChaCha20, the ChaCha20 class actually internally implements Draft 02 (despite the comment referencing Draft 03). It does not increment the counter to 1 when using the ChaCha20 stream.

https://github.com/Legrandin/pycryptodome/blob/a6b6ecd8959d155eeec46db664c6817359d50ac7/lib/Crypto/Cipher/ChaCha20.py#L99-L107

The unit test vectors even refer to Draft 02 as well.

https://github.com/Legrandin/pycryptodome/blob/a6b6ecd8959d155eeec46db664c6817359d50ac7/test_vectors/pycryptodome_test_vectors/Cipher/wycheproof/xchacha20_poly1305_test.json#L22

However, the XChaCha20-Poly1305 implementation in the class ChaCha20_Poly1305 does increment the counter, as per Draft 03.

https://github.com/Legrandin/pycryptodome/blob/a6b6ecd8959d155eeec46db664c6817359d50ac7/lib/Crypto/Cipher/ChaCha20_Poly1305.py#L69-L72

As does this unit test manually?

https://github.com/Legrandin/pycryptodome/blob/a6b6ecd8959d155eeec46db664c6817359d50ac7/lib/Crypto/SelfTest/Cipher/test_ChaCha20.py#L317-L319

It appears it is up to the user of ChaCha20 to increment the counter themselves in order to adhere to Draft 03. This can be confusing when using the class and its output does not match that of other libraries.

I would make a PR, but I'm unsure the direction the maintainers would wish to take:

  1. Change all implementations to adhere to Draft 03, and increment the counter internally (inside ChaCha20)
  2. Update the documentation to state the base behavior is Draft 02 (updating the link), and how to adhere to Draft 03 by incrementing the counter manually

Option 1 would possibly break user's code, so I would believe Option 2 to be better. However, the unit tests would need updated as well, as they are lying.