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

Add RFC 8439 ChaCha20 test vectors test #245

Merged
merged 1 commit into from May 25, 2021

Conversation

philipr-za
Copy link
Contributor

While playing with your ChaCha20 implementation I noticed in my tests that I was not able to generate the correct result using the test vectors given in RFC 8439. So I thought I would write the tests to reproduce my problem in this PR.

However, while writing them I stumbled on this section of the XChaCha tests:

        // The test vectors omit the first 64-bytes of the keystream
        let mut prefix = [0u8; 64];
        cipher.apply_keystream(&mut prefix);

Adding this to my tests produced the correct results with the test vectors and I am not sure why this is needed. I don't see it mentioned in the RFC. So I wanted to ask why this prefix is needed for the test vectors to produce the correct cipher text? I thought I would contribute the RFC test vectors test during the process of asking.

Regards,
Philip

@tarcieri
Copy link
Member

As mentioned on #244, some of the test vectors are in the chacha20.blb file, but it's encoded in an opaque binary format which doesn't have good tooling for introspection or a way of keeping comments about what vectors it includes.

At this point I think we should just merge some PRs like this and #244 even if they duplicate test vectors in the chacha20.blb file. Given there are two PRs about this, I think it's pretty unclear to everyone involved which of the RFC test vectors are already covered. Then whenever someone wants to reconcile which vectors are or are not in the .blb file they can do that.

It generally seems problematic that .blb files are opaque, lack introspection tooling, and do not in any document what test vectors they contain, leading to this sort of confusion /cc @newpavlov

To answer your question:

So I wanted to ask why this prefix is needed for the test vectors to produce the correct cipher text?

Within the context of the ChaCha20Poly1305, the first 64-bytes of the ChaCha20 keystream are used as the Poly1305 key.

@tarcieri
Copy link
Member

Going back through the repo history, here's one of the commits that touches the .blb files: 39726d3

But offhand (as in #244) I'm still not sure what vectors they contain specifically.

@philipr-za
Copy link
Contributor Author

To answer your question:

So I wanted to ask why this prefix is needed for the test vectors to produce the correct cipher text?

Within the context of the ChaCha20Poly1305, the first 64-bytes of the ChaCha20 keystream are used as the Poly1305 key.

Thanks, I appreciate the clarification.

@seanybaggins
Copy link

@philipr-za I believe the

        // The test vectors omit the first 64-bytes of the keystream
        let mut prefix = [0u8; 64];
        cipher.apply_keystream(&mut prefix)

is needed because when you create the ChaCha Cypher, It is initialized with its counter equal to zero.

Consuming the first 64 bytes produced by the Cypher causes the counter to be incremented by one, thus causing the state of the Cypher to match the tests in the rfc.

@philipr-za
Copy link
Contributor Author

Consuming the first 64 bytes produced by the Cypher causes the counter to be incremented by one, thus causing the state of the Cypher to match the tests in the rfc.

Thanks, that makes perfect sense. I will leave it up you guys to decide whether this PR is worthwhile or not. By all means close it if those vectors are covered or you would like to handle it in a different way.

@tarcieri
Copy link
Member

I had to write a tool to dump the test vectors from blobby. I'm not sure one existed before? (cc @newpavlov)

Anyway, here's what's in chacha20.blb:

vector #0
key: 0000000000000000000000000000000000000000000000000000000000000000
iv: 000000000000000000000000
pt: 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
ct: 76b8e0ada0f13d90405d6ae55386bd28bdd219b8a08ded1aa836efcc8b770dc7da41597c5157488d7724e03fb8d84a376a43b8f41518a11cc387b669b2ee6586

vector #1
key: 0000000000000000000000000000000000000000000000000000000000000000
iv: 000000000000000000000002
pt: 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
ct: c2c64d378cd536374ae204b9ef933fcd1a8b2288b3dfa49672ab765b54ee27c78a970e0e955c14f3a88e741b97c286f75f8fc299e8148362fa198a39531bed6d1a91288c874ec254f322c2a197340c55bb3e9b3998f7de2309486a0bb494abd20c9c5ef99c1370d61e77f408ac5514f49202bcc6828d45409d2d1416f8ae106b06ebd2541256264fa415bd54cb12e1d4449ed85299a1b7a249b75ff6c89b2e3f2e0a13881b024b80658fc2b69eaf2da024815374d867b1051af9b108a712cafdd18cdc6cfc921d76e5700e33f09af911ce8c6ca89c4d6cfc83ac1eb02ac16da387bb5348b20e7ddbb05f7dff3bbbc69630357f7697da8e76c2eabdf5502f66dc

vector #2
key: 0000000000000000000000000000000000000000000000000000000000000001
iv: 000000000000000000000000
pt: 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
ct: 4540f05a9f1fb296d7736e7b208e3c96eb4fe1834688d2604f450952ed432d41bbe2a0b6ea7566d2a5d1e7e20d42af2c53d792b1c43fea817e9ad275ae546963

So I think it's worth merging this and I guess then we can look at regenerating the .blb file.

@tarcieri tarcieri merged commit 926bd31 into RustCrypto:master May 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants