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

doc: add caveats and example to crypto.timingSafeEqual #41837

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lostfictions
Copy link

@lostfictions lostfictions commented Feb 3, 2022

#41507 recently added documentation about an error case for crypto.timingSafeEqual, but even with that addition I think there are a few extra caveats to timingSafeEqual's use that are worth mentioning. I've tried to be as clear as possible in this addition without going outside the scope of documentation for a single function (ie. into the realm of broader crypto best practices). I've also added a short example for the function.

@nodejs-github-bot nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. labels Feb 3, 2022
`crypto.timingSafeEqual` has a number of caveats to its use that are
worth mentioning. While surrounding code isn't strictly the
responsibility of `crypto.timingSafeEqual`, documenting these pitfalls
may help avoid some very common errors.
@Trott
Copy link
Member

Trott commented Feb 4, 2022

@nodejs/crypto

@tniessen
Copy link
Member

tniessen commented Feb 4, 2022

Welcome @lostfictions and thank you for the contribution!

timingSafeEqual is tricky. In my personal experience, when inputs have different lengths, either something is wrong (for example, passwords should never be compared using timingSafeEqual) or the length of the secret is not actually secret (for example, when comparing API keys).

Do you have an example use case that potentially passes inputs to timingSafeEqual without validating their lengths first?

secretBuffer.write(secret);

return timingSafeEqual(inputBuffer, secretBuffer) &&
inputBuffer.length === Buffer.byteLength(secret);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, operations such as Buffer.byteLength(secret) are generally not timing-safe.

@lostfictions
Copy link
Author

lostfictions commented Feb 4, 2022

Fair enough! Maybe this is all extraneous and timingSafeEqual is more of a "this is a low-level primitive and I assume you already know what you're doing if you reach for it" deal. Or maybe it's worth instead amending the documentation with a brief mention of what you said -- raw passwords should never be compared directly, and it's a smell if you're comparing inputs of different lengths.

Do you have an example use case that potentially passes inputs to timingSafeEqual without validating their lengths first?

I actually saw this is in the wild in a package with 240,000 downloads per week:

https://github.com/LionC/express-basic-auth/blob/dd17b4de9fee9558269cdc583310bde5331456e7/index.js#L7-L17

Admittedly maybe the bigger issue here is "never use basic auth"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants