How to properly use crypto.timingSafeEqual(a, b)?

[niftylettuce]

It would be nice to pass an option rawBuffer: true or something to get the raw buffers returned as user and pass instead of String's via toString(), that way we can use crypto.timingSafeEqual(a, b) for comparison?

References:
https://nodejs.org/api/crypto.html#crypto_crypto_timingsafeequal_a_b
nodejs/node#17178

[dougwilson]

The example in the README shows how to compare safely: https://github.com/jshttp/basic-auth#with-vanilla-nodejs-http-server

[dougwilson]

You can replace the use of the tsscmp lib in the example with timeSafeEqual, of course:

function check (name, pass) {
  var valid = true

  // Simple method to prevent short-circut and use timing-safe compare
  valid = crypto.timingSafeEqual(Buffer.from(name), Buffer.from('john')) && valid
  valid = crypto.timingSafeEqual(Buffer.from(pass), Buffer.from('secret')) && valid

  return valid
}

The example just has the username / password you want to compare to in the code itself from simplicity, but you'd just inject it from where ever those are stored (like a database or something).

[niftylettuce]

Wouldn't it be more performant to not call Buffer.from again though?

[niftylettuce]

(since we already call it here https://github.com/jshttp/basic-auth/blob/master/index.js#L78)

[dougwilson]

I'm not sure. A benchmark would be able to answer that question, though. Also it looks like crypto.timingSafeEqual is a very hard-to-use API, because you have to validate lengths yourself and everything. My example above is wrong because of this.

Why not just use the tsscmp lib? This would make it much simpler and less error prone.

[niftylettuce]

I saw this comment here https://github.com/suryagh/tsscmp/blob/master/lib/index.js#L5 and figured there should be a more modern solution using native Node methods is the only reason.

[dougwilson]

The docs at https://nodejs.org/api/crypto.html#crypto_crypto_timingsafeequal_a_b have

This is suitable for comparing HMAC digests or secret values like authentication cookies or capability urls.

Which doesn't mention passwords. This may be why it's a hard-to-use interface for variable-length data like passwords, while easy for fixed-length things like HMAC digests, etc. as the examples?

Ideally when you're comparing passwords you wouldn't always want to directly compare the raw buffers, but make sure to run the password through a UTF normalization function. Because if the password the user has is 'Åwesome' they shouldn't have to care if their browser sent it in a different normalization than what is stored in your database.

[dougwilson]

I think another reason the Node.js docs for crypto.timingSafeEqual doesn't mention comparing passwords using it is because of the length equality requirement. Comparing lengths and bailing early would make it pretty easy with completely invalid passwords to guess the length of the target password with a timing attack. Getting the password length then reduces your search window to combinations that are exactly that length.

[dougwilson]

So that would meant to keep an attacker from reducing their attack complexity (by revealing the length of the password), you'd need to do something like hash the password before passing it to crypto.timingSafeEqual, which would likely overshadow the overheard of Buffer -> string -> Buffer I think (though I didn't benchmark it).

It would basically need to do everything the tsscmp module is doing, but you can replace the https://github.com/suryagh/tsscmp/blob/master/lib/index.js#L11 function with crypto.timingSafeEqual.

[niftylettuce]

@dougwilson does this look good to you? https://github.com/niftylettuce/tsscmp/commit/0664f5be593e037ece61e3037943ea9d67765ba3#diff-6d186b954a58d5bb740f73d84fe39073R15

[dougwilson]

Yea, that looks good! Though if you're intending to make a PR to tsscmp than I'll need to find a different module to start advertising and using -- this module supports Node.js 0.8+ and the docs here will need to reflect that fact.

[dougwilson]

And the several npm modules I have published will also need to move over to something else.

[niftylettuce]

@dougwilson what do you think about adjusting tsscmp to use semver and do a comparison of the Node version? https://www.npmjs.com/package/semver

I'm not sure what version of Node actually was published with the crypto.timingSafeEqual fix (the reason it wasn't used by tsscmp in the first place).

[dougwilson]

Well, I'm not the author of that module, so you'll probably want to ask the author :) But that sounds like how I would normally do it, as a polyfill that uses it when there otherwise falls back for old versions.

[niftylettuce]

Looks like the method was added in v6.6.0 according to the docs. I'll submit a PR.

[dougwilson]

Cool. If you're going to polyfill it, you don't even need to know when it was added -- check check if crypto.timingSafeEqual exists. Node.js core has broken version sniffing in the past and just said that folks should not be version sniffing in the first place. At least, that's what I do now after arguing with the core devs a few times :)

[niftylettuce]

@dougwilson see suryagh/tsscmp@865e4d7 - any changes lmk!

[dougwilson]

Cool. You may want to put a block around it to match the styling and move it after the length check (since timingSafeEqual requires you do the length check before passing the two buffers, per the Node.js issue linked at the top of this issue).

[niftylettuce]

done @dougwilson

[niftylettuce]

@dougwilson hey I'm curious if you had any security concerns/improvements to share here https://github.com/ladjs/policies/blob/master/index.js#L34-L48 - I'm not using tsscmp since I have to query a DB to check if API token matches credentials.

[dougwilson]

So I don't see any use of the .pass property -- I assume that this means what you're doing is using Basic auth but just providing your API key as the username and no password? If so, that can be bad as there is always the risk that the token can get logged, because the username part is not considered sensitive and even historically automatically added to access logs (like in Apache HTTPD). This is the same kind of concern with passing an API key in a query string parameter -- URLS are typically included in access logs.

As for the comparison quality, as long as the database is using some kind of timing-safe comparison then it doesn't matter if the db is doing the comparison or the code is.

I hope that helps!

[niftylettuce]

It does help @dougwilson - thank you. I noticed that Stripe also does this approach https://stripe.com/docs/api#authentication and so I wonder why they chose to use user instead of pass. Maybe for simplicity.

[dougwilson]

It's certainly possible. Of course, if just a single token is being provided, using Basic auth at all as the carrier seems overly complicated -- that's just Bearer at that point where your token has been passed through base64 for additional overhead (in both the need to encode / decode to / from base64 and it's going to be some more bytes on the wire).

[dougwilson]

Looks like tsscmp is updated. I'm going to close this. Let me know if I misunderstood the action item here and I can always reopen ❤️ !

[niftylettuce]

Good to close, thank you

[tranvansang]

Some people recommend tsscmp package. However, this package uses deprecated pseudoRandomBytes

https://github.com/suryagh/tsscmp/blob/6f2a3dd83a8bd449ced3b7d400e034d39123e4e9/lib/index.js#L31

And redundant for-loop check

https://github.com/suryagh/tsscmp/blob/6f2a3dd83a8bd449ced3b7d400e034d39123e4e9/lib/index.js#L20

Here is my recommended version

const crypto = require('crypto')

// only accept string values
function safeCompare(a, b) {
	if (typeof a !== 'string' || typeof b !== 'string') return false
	const key = crypto.randomBytes(32)
	const ha = crypto.createHmac('sha256', key).update(a).digest()
	const hb = crypto.createHmac('sha256', key).update(b).digest()
	return ha.length === hb.length && crypto.timingSafeEqual(Buffer.from(ha), Buffer.from(hb)) && a === b
}

There is also a good to know implementation of node-cookie package

tj/node-cookie-signature@5fb33f0

[dougwilson]

Hi @tranvansang I believe you opened this issue in the wrong location. Please file an issue (or PR) with tsscmp if there is an issue there to fix -- many others besides this module use that package. The idea of small packages vs copy-pasting code everywhere is there is a central location to fix an issue and everyone will benefit from it.