franziskuskiefer / hpke-rs

Pure Rust implementation of HPKE (https://www.rfc-editor.org/rfc/rfc9180.html)
28 stars 14 forks source link

Bad comparison cause Kem Error.cryptoError for P256 #15

Closed mefsantos closed 3 years ago

mefsantos commented 3 years ago

File: dh_kem.rs lines: 35~37 Code:

if dh.len() <= 32 {
    return Err(Error::CryptoError);
}

from Section 4. DH-Based KEM: P256 dh secret is 32 bytes long.

franziskuskiefer commented 3 years ago

If true, this again should fail somewhere.

mefsantos commented 3 years ago

Similarly to issue #14 I believe they are not covered by any of the unit tests, since the unit tests only cover KDF and AEAD, not the KEM's key pair generation and encapsulation

franziskuskiefer commented 3 years ago

Ok, so this is actually fine. The result of the DH call here is 64 bytes for P256 (X and Y). But we only care about the X coordinate with 32 bytes. The check here is only a sanity check to make sure that copying 32 bytes in the next line works and doesn't panic.

mefsantos commented 3 years ago

I guess the lack of documentation of ever crypt might mislead me, and since I am not implementing in RUST, I cannot test this easily. However, it is also somewhat confusing that ecdh_derivederives a valid P256 point with x and y coordinates, when in fact the purpose of the DH is to generate a symmetric key, which for P256 should be 32 bytes long, which is what the crypto package I am using does thus raising the error on my implementation.

franziskuskiefer commented 3 years ago

The purpose of DH is not to derive a symmetric key. It generates a common point on the curve that can be used to derive a symmetric key.

mefsantos commented 3 years ago

I guess I was oversimplifying the process and probably misinterpreted the output of my crypto package. It probably already outputs the content to be used for the sym. key derivation. thanks for the clarification.