Release proposal: 2.0.0 #10
Conversation
This is a BREAKING CHANGE
- Removed dependency tsscmp
Co-authored-by: Íñigo Marquínez Prado <25435858+inigomarquinez@users.noreply.github.com>
UlisesGascon
force-pushed
the
next-major
branch
from
f3c017f to
8ff3362
Compare
Co-authored-by: Íñigo Marquínez Prado <25435858+inigomarquinez@users.noreply.github.com>
| callback = (user, pass) => { | ||
| const buffers = [ | ||
| Buffer.from(user), | ||
| Buffer.from(pass), | ||
| Buffer.from(username), | ||
| Buffer.from(password) | ||
| ]; | ||
|
|
||
| // Determine the maximum length among all buffers | ||
| const maxLength = Math.max(...buffers.map(buf => buf.length)); | ||
|
|
||
| // Pad each buffer to the maximum length | ||
| const paddedBuffers = buffers.map(buf => Buffer.concat([buf, Buffer.alloc(maxLength - buf.length)])); | ||
|
|
||
| const usernameValid = crypto.timingSafeEqual(paddedBuffers[0], paddedBuffers[2]) | ||
| const passwordValid = crypto.timingSafeEqual(paddedBuffers[1], paddedBuffers[3]) | ||
| return usernameValid && passwordValid; |
There was a problem hiding this comment.
@lirantal any chance you can review this? 🙏
There was a problem hiding this comment.
Using timingSafeEqual is indeed the best practice to follow for a timing safe comparison to avoid TOCTOU and side channel attacks that leak information about the username and password being used.
However, secure timing comparison is useful when you are comparing hashes which always produce the same length, regardless of the input string length. Here, you are padding it, which could also end up with false positives. Buffer.alloc() will pad with zero length strings (`\x00') and if the input provided to you includes that byte sequence you'd have a false positive. Here's an example based on the above:
const res = callbackComparison("user", "pass", "user\x00", "pass");
console.log("Zero Padding Result:", res);You'd expect res to be false but it is actually true.
You could fix it by filling with another byte like ... Buffer.alloc(maxLength - buf.length, 0x01)])); but my recommendation would be to completely drop the padding altogether and keep it simple and rely on the timingSafeEqual to either throw if the length is different (which is ok and not a timing leak) or perform secure comparison if the length is the same.
Main Changes
Breaking changes
Other changes