4
\$\begingroup\$

I have implemented HKDF in Rust, which passes all test vectors from RFC 5869, using HKDF with HMAC-SHA256. I also have a function to compare byte arrays in constant time, which I hope someone could confirm is correctly done. Any type of feedback is welcome.

EDIT: I mainly mean feedback on hkdf_compute(). HMAC in hkdf_extract() is defined in another module and not included here.

HKDF

pub struct Hkdf {
  pub salt: Vec<u8>,
  pub data: Vec<u8>,
  pub info: Vec<u8>,
  pub hmac: usize,
  pub length: usize,
}

impl Drop for Hkdf {
  fn drop(&mut self) {
    //println!("DROPPING");
    self.salt.clear();
    self.data.clear();
    self.info.clear()
  }
}

impl Hkdf {
  /// Return HMAC matching argument passsed to Hkdf.
  pub fn hkdf_extract(&self, data: &[u8], salt: &[u8]) -> Vec<u8> {
    let hmac_res = Hmac {
        secret_key: salt.to_vec(),
        message: data.to_vec(),
        sha2: self.hmac
    };

    hmac_res.hmac_compute()
  }

  /// The HKDF Expand step. Returns an HKDF.
  pub fn hkdf_compute(&self) -> Vec<u8> {
    // Check that the selected key length is within the limit.
    if self.length as f32 > 255_f32 * (self.hmac / 8) as f32 {
        panic!("Derived key length above max. 255 * (HMAC OUTPUT LENGTH IN BYTES)");
    }

    let n_iter = (self.length as f32 / (self.hmac / 8) as f32).ceil() as usize;

    let mut con_step: Vec<u8> = vec![];
    let mut t_step: Vec<u8> = vec![];
    let mut hkdf_final: Vec<u8> = vec![];

    for x in 1..n_iter+1 {
            con_step.append(&mut t_step);
            con_step.extend_from_slice(&self.info);
            con_step.push(x as u8);
            t_step.extend_from_slice(&self.hkdf_extract(
                &con_step,
                &self.hkdf_extract(&self.data, &self.salt))
            );
            con_step.clear();

            hkdf_final.extend_from_slice(&t_step);
    }

    hkdf_final.truncate(self.length);

    hkdf_final
  }
}

Compare constant time:

#[inline(never)]
/// Comparison in constant time.
pub fn compare_ct(x: &[u8], y: &[u8]) -> bool {

  let length = x.len();

  if length != y.len() {
    false;
  }

  let mut result: u8 = 0;

  for n in 0..length {
    result |= x[n] ^ y[n];
  }

  result == 0
}
\$\endgroup\$
  • \$\begingroup\$ The Hkdf isn't complete. It's missing the definition of `Hmac'. \$\endgroup\$ – Zeta Mar 22 '18 at 12:58
  • \$\begingroup\$ I'm sorry, I don't know what you mean by 'definition'? \$\endgroup\$ – user159822 Mar 22 '18 at 13:00
  • \$\begingroup\$ In let hmac_res = Hmac { … }, you use Hmac, which is unknown to us, the readers and reviewers. I guess you're more interested in a review of hkdf_compute only? \$\endgroup\$ – Zeta Mar 22 '18 at 13:02
  • \$\begingroup\$ You're right, post updated! \$\endgroup\$ – user159822 Mar 22 '18 at 13:06
2
\$\begingroup\$

In general you should try and avoid floating point operations for any cryptographically specific code. Floating points have issues with regards to precision, and even I don't see any issue for this specific code, they always require special attention, e.g. during reviewing of the code.

Similarly, using recursive code is strongly discouraged for cryptographic algorithms. If your code can be coded using loops and only local variables you should really do so. I imagine that goes against the idea of Rust (and is thus up for debate), but crypto code generally requires special attention. If you keep to recursive calls make sure you document them as such, both inline and in the function description.

Although you nicely implemented the Drop destructor it makes more sense to create either a class with just a keyInputMaterial (now called data I presume) or simply keep all the data in parameters / local variables. The idea of just keeping the keyInputMaterial is that you may reuse the object to derive other keys using the function but with different salt / info. In that case you obviously need to retrain the Drop to clear the input key material.

Note that the internal key handling (padding, initial hashing) of HMAC is always the same. So a sneaky speed up is to perform pre-calculation on the salt which acts as the HMAC key (mainly useful if there is a lot of output expected, this speedup matters more for e.g. PBKDF2).


Your constant time function looks very much run-of-the-mill constant time, which in this case is great :) I don't think HKDF warrants a function where HMAC(k, x) and HMAC(k, y) are compared using a random key - but now you know about that neat little trick.

Note that you may want to make sure that you document that the comparison needs to be done in constant time or include it in a verify function (beware of truncated values - you did this in your code, but you need to consider it in the calling code as well!).


Note that I'm not a Rust expert, so I won't comment on the vector handling / slicing etc.

\$\endgroup\$
  • \$\begingroup\$ Excuse my overly compressed comment, I have a tough time with character limits. Hope you can see to formatting correctly. \$\endgroup\$ – user159822 May 13 '18 at 15:22
  • \$\begingroup\$ This is amazing feedback, really. I still have some follow-up questions I hope you would be willing to answer. * Do you have an alternative to CEIL func with floats? * For the recursive code I assume you mean where the extract function is called instead of passing a PRK in expand? * You want me to let the IKM be passed as a function paramter, so that it can be reused instead of Dropped? * The sneaky speed up, I'm not 100% sure what you mean here. You don't happen to have an example? * Am I not taking truncated values into account by comparing equal length vectors? \$\endgroup\$ – user159822 May 13 '18 at 15:32
  • \$\begingroup\$ You can perform + 1 before division instead of ceil For recursive, I see a call to hkdf_compute from hkdf_expand and vice versa. Yes, that or keep using it as a field, but pass the other fields as arguments. Please have a look at HMAC where the hash is used twice and the intermediate states of HMAC after hashing ipad and opad can be kept (you need to be able to store intermediate hash results though!). Yes, your compare code is fine, but was talking about using it. If you provide a verify function then the user is less likely to use another compare function. \$\endgroup\$ – Maarten Bodewes May 13 '18 at 16:07
  • \$\begingroup\$ 1. Fantastic! 2. Understood. 3. Got it. 4. I think I see what you're getting at. I tried pre-padding the password in my PBKDF2 impl and immediately got a ~5 sec performance boost. 5. Right, I have taken care of this then. Thank you! \$\endgroup\$ – user159822 May 13 '18 at 17:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy