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

A Quick Security Audit #3

Closed
defuse opened this issue Apr 5, 2018 · 1 comment
Closed

A Quick Security Audit #3

defuse opened this issue Apr 5, 2018 · 1 comment
Labels
security Security-related issues or improvements

Comments

@defuse
Copy link
Contributor

defuse commented Apr 5, 2018

Here are some issues and recommendations:

util.rs

  • There was a missing return which made compare_ct return the wrong answer when y is longer than x and x and y are identical up to the length of x. PR to fix it: Fix a security bug in compare_ct #1

hkdf.rs

  • The name "data" for the HKDF parameter is confusing (formally, it's called the Initial Key Material, so maybe just calling it key_material or something similar would be more informative).
  • HKDF doesn't validate self.hmac before using it (it relies on the HMAC call later on panicing when it's a bad value). I would use an enum or something instead of an integer so that it's not possible to even express an incorrect value here.
  • The maximum-length check in HKDF doesn't need to convert to f32, since self.hmac/8 will always be an integer.
  • It would be a good idea to add unit tests that test HKDF's maximum-length check.
  • The implementation of HKDF looks correct to me (and the test vector tests are nice!).

hmac.rs

  • It looks correct to me (and it's awesome that the unit tests compare against test vectors).
  • Again, it would be nice if sha2 were an enum or something instead of a usize so that there can't be invalid values.

default.rs

  • hmac_validate would be more useful (and less error-prone) if its signature was: hmac_validate(expected_hmac: &[u8], message: &[u8], key: &[u8]). Currently, it just compares two &[u8]s, taking in the key and message would be more useful since it saves the caller from having to re-compute the HMAC for comparison.

other

@brycx brycx mentioned this issue Apr 7, 2018
@brycx
Copy link
Member

brycx commented Apr 8, 2018

Fantastic! Everything was covered in #4 so I'm closing this issue.

@brycx brycx closed this as completed Apr 8, 2018
@brycx brycx added the security Security-related issues or improvements label May 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security Security-related issues or improvements
Projects
None yet
Development

No branches or pull requests

2 participants