Closed hraban closed 5 years ago
~Note that this still leaks the length of the password. To also protect against that you must hash both passwords, then use constant time compare.~
upstream has been fixed; length of the password is now not leaked anymore.
hello all, any update on this? is there anything in the way of this getting merged?
@LionC is there anything specific blocking this PR? Anything I can do to help it getting merged? It's a bit of a security issue at the moment, and anyone using this module with a standard comparator is at risk. Would be lovely to get this one step closer to merging, for the greater good :)
Hi @hraban, thanks for pinging me again about this! I had a lot on my mind the last months, keeping me from doing the typescript rewrite, which I planned to include your proposal into.
As I do not know how much time I will have the next weeks, I will look into your proposal until this Sunday and make sure, that we change the comparison to be constant time instead.
hope you're fine and healthy! best of luck with whatever's bugging you.
if you're going to rewrite in typescript you won't be able to use the &
trick on booleans, unfortunately. We ended up just doing:
// explicit full comparison to avoid leaking data about username
let ok = true;
ok = safeCompare(username, credentials.name) && ok;
ok = safeCompare(password, credentials.pass) && ok;
return ok;
I guess it's more readable, in the end.
It is always possible to force TypeScript compiling a bitwise operation on non-numbers by forcefully disabling the type checker with @ts-ignore
:)
// @ts-ignore
const bitwise = true & true;
Fair enough :D
I like the version with the ts compiler annotation @toverux ! I just googled around a bit and noticed that the crypto module also offers a time-safe compare method nowadays, so I might change some parts of the PR @hraban, as I try not to add any dependencies if possible.
yes, the safe-compare dependency is essentially a passthrough for that crypto operation if it exists, otherwise a shim for older node versions. it helps support node releases <6.6.0, probably not a very large part of node installations out there anymore?
@hraban as node 6 support ends this month, I decided not to include the dependency. I also decided to provide the time safe compare method from the module. As that would heavily deviate from the code in this PR, I decided to do a new branch to implement all of this and opened a new PR. I made sure to add credits to you and the original author of the safe comparison algorithm.
Using native string compare for passwords (even when hashed) exposes you to timing attacks. Using a constant time string compare protects against this.