softprops / afterparty

rust github webhook server
MIT License
55 stars 13 forks source link

Possible timing attack vulnerability #21

Closed Philipp91 closed 7 years ago

Philipp91 commented 7 years ago

Just a quick heads-up from a passer-by: https://github.com/softprops/afterparty/blob/master/src/hook.rs#L45 This comparison might be vulnerable to timing attacks because it's not constant-time. Not sure if that's relevant here at all. I'm reporting such issues to a couple of repositories, please just ignore/close if it isn't applicable here.

softprops commented 7 years ago

I'm not super familiar with the attack but you sparked my interest (in a phone). Would tjis be remedies by somethink like https://github.com/briansmith/ring/blob/master/src/constant_time.rs a quick gooGlenn for rust hmac constant time led me here? If there are other suggestions let me know and I'll look into it

Philipp91 commented 7 years ago

Sorry, I should have included this in my initial post already.

Yes, the function you found would work. rust-crypto also has this function, it's called crypto::util::fixed_time_eq(). Alternatively, a cleaner solution would be to parse the hex signature you received and call MacResult::new() with it and then compare the two MacResults, as this will that same constant-time comparison function: https://github.com/DaGenix/rust-crypto/blob/master/src/mac.rs#L88

softprops commented 7 years ago

Sweet. thanks for the examples. I'll take a look