search

stef / libopaque

c implementation of the OPAQUE protocol with bindings for python, php, ruby, lua, zig, java, erlang, golang, js and SASL.
GNU Lesser General Public License v3.0
69 stars 10 forks source link

Fix compatibility with specification in regards to `client_mac` #25

Closed falko17 closed 2 years ago

falko17 commented 2 years ago

Summary

This pull request fixes two bugs which lead to an incorrect client_mac being calculated: the first bug was about reusing a state parameter which has already been "used up", the second was using an incorrect key length.

Details

State parameter reuse

The function calc_preamble ends by calling crypto_hash_sha512_final on it. After this (according to the libsodium documentation) the passed-in state parameter must not be re-used. However, in our case the parameter was re-used, because the preamble_state was used not only for the server_mac, but also for the client_mac—hence, the SHA512 sum that was calculated before this commit was incorrect.

This bug has been fixed by copying the state parameter before passing it to crypto_hash_sha512_final. That way, the caller of calc_preamble can still use the state and the correct SHA512 sum will be calculated.

Incorrect key length

The function crypto_auth_hmacsha512 was used in RecoverCredentials and CreateCredentialResponse. This was a problem because the function (i.e., libsodium) uses 32-byte keys by default, while OPAQUE specifies a key length of 64 bytes.

The problem has been fixed by replacing this function call with opaque_hmacsha512, which uses the correct key length.

Testing procedure / Background

I noticed these two bugs in the first place because I was using opaque-ke on one side and libopaque on the other. Trying to make these two libraries work together was a little difficult due to the relative complexity of pinning down why a certain step failed, but when using the version of libopaque from this pull request and the latest commit of opaque-ke[^1], the login flow finally completes successfully.

In other words, it should now be possible to use libopaque and opaque-ke together without any problems.

(I've also run the tests generated by make tests to make sure I didn't break anything.)

[^1]: Before this commit (which is not yet released in the crate itself), opaque-ke used a non-standard salt length, which also made it impossible to combine it with libopaque.

stef commented 2 years ago

huh. great detective work! thanks, just came back from afk time. will have a look at this asap. btw any idea why did the test-vectors not catch this?

falko17 commented 2 years ago

btw any idea why did the test-vectors not catch this?

I'm not sure, actually. I didn't have to modify them either to pass the tests, so they don't seem to catch any changes here, which is weird—the only thing I can think of is that the first problem (state parameter re-use) isn't necessarily easily reproducible, and perhaps doesn't occur in the test environment for some reason? As for the second problem (key length), maybe the HMAC itself is not something that's covered by the test vectors? This is pure speculation, though.

stef commented 2 years ago

thank you very much for this great contribution!