freedomlayer/offset

derive_symmeric_key might panic due to invalid public key

Closed this issue · 3 comments

At crypto/dh.rs, inside derive_symmetric_key there is the following code:

let shared_secret_array = match key_material_res {
    Ok(key_material) => key_material,
    _ => unreachable!(),
};

I think that we might be able to reach the unreachable code. (I wrote this code!) This might allow an attacker to acheive DoS, in case an invalid public key is provided during Diffie Hellman.
I think that a reasonable solution would be to change the function signature from:

pub fn derive_symmetric_key(&self, public_key: &DhPublicKey, salt: &Salt) -> SymmetricKey {

to

pub fn derive_symmetric_key(&self, public_key: &DhPublicKey, salt: &Salt) -> Result<SymmetricKey, ???> {

And return an Error on the case where we currently have unreachable!().

@kamyuentse : This could affect current Channeler code, so I assign this to you.
Tell me what you think.

@realcr Make this change is easy, but I'd like to take this opportunity to request us to rethink about the error management system in CSwitch, in the recent days, I'm rethinking about the Channeler design(codebase, not theoretic), I noticed some errors are fatal, some error can be ignore, etc. I know that we should log the ignorable errors, but we need them to fit the type system in Rust. We should allow these errors happen, and deal with the error when it bubbles up to some places.

BTW, the signature of NonceWindow::new(...) also needs to be changed.

but I'd like to take this opportunity to request us to rethink about the error management system in CSwitch

@kamyuentse : Sounds interesting. Can you open a new issue and introduce it, maybe with some example?

Fixed at: #28
Closing.