Constant time to protect against timing attacks

[hraban]

Using native string compare for passwords (even when hashed) exposes you to timing attacks. Using a constant time string compare protects against this.

[hraban]

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.

[lirantal]

Looks like there's a typo in this code. I believe @hraban meant a &&

Suggested change

	return compare("admin", username) & compare("admin-password", password);
	return compare("admin", username) && compare("admin-password", password);

[hraban]

I initially made the same mistake, but someone pointed out to me that using a short-circuiting conjunction operator (&&) will leak information about the username. Using an eager operator (&) will be actually constant time.

I added a comment to the readme to elaborate on it.

[lirantal]

🤦‍♂️
yes let's document to prevent same mistakes in the future! ;-)

[hraban]

hello all, any update on this? is there anything in the way of this getting merged?

[hraban]

@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 :)

[LionC]

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.

[hraban]

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.

[toverux]

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;

[hraban]

Fair enough :D

[LionC]

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.

[hraban]

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?

[LionC]

@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.

Would you like to review it?