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

Fix PSK in rev32 #36

Open
wants to merge 2 commits into
base: rev32
Choose a base branch
from
Open

Conversation

pallas
Copy link
Contributor

@pallas pallas commented Apr 18, 2019

Addresses Issue #28.

pallas added 2 commits April 18, 2019 00:04
According to §9.2, when `psk` appears in a pattern, the e operation changes
from
	`MixHash(e.public_key)`
to
	`MixHash(e.public_key), MixKey(e.public_key)`

This commit makes that change, causing the cacophony vectors to pass.
According to §5.2, in MixKey and MixKeyAndHash, temp_k should be truncated
to 32 bytes if HASHLEN is 64.
@@ -281,6 +281,10 @@ int noise_symmetricstate_mix_key
(state->hash, state->ck, hash_len, input, size,
state->ck, hash_len, temp_k, key_len);

/* Truncate temp_k */
if (hash_len == 64 && key_len > 32)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would seem to prevent the use of symmetric ciphers with more than 256 bits. While the noise specification may have that restriction, I tried to avoid putting that restriction into the code to support future ciphers with larger keys. Why is this needed?

@pallas
Copy link
Contributor Author

pallas commented Apr 24, 2019

Ah, got it. I added it because it was in the spec; if the omission is intentional, I have no problem with skipping.

bcspragu added a commit to bcspragu/noise-c that referenced this pull request Jul 20, 2023
It isn't called that, which I think has something to do with rweather#36, but preshared keys do appear to work correctly.
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.

2 participants