Comments (27)
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.
from basic-auth.
The example in the README shows how to compare safely: https://github.com/jshttp/basic-auth#with-vanilla-nodejs-http-server
from basic-auth.
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).
from basic-auth.
Wouldn't it be more performant to not call Buffer.from
again though?
from basic-auth.
(since we already call it here https://github.com/jshttp/basic-auth/blob/master/index.js#L78)
from basic-auth.
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.
from basic-auth.
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.
from basic-auth.
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.
from basic-auth.
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
.
from basic-auth.
@dougwilson does this look good to you? https://github.com/niftylettuce/tsscmp/commit/0664f5be593e037ece61e3037943ea9d67765ba3#diff-6d186b954a58d5bb740f73d84fe39073R15
from basic-auth.
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.
from basic-auth.
And the several npm modules I have published will also need to move over to something else.
from basic-auth.
@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).
from basic-auth.
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.
from basic-auth.
Looks like the method was added in v6.6.0
according to the docs. I'll submit a PR.
from basic-auth.
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 :)
from basic-auth.
@dougwilson see suryagh/tsscmp@865e4d7 - any changes lmk!
from basic-auth.
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).
from basic-auth.
done @dougwilson
from basic-auth.
@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.
from basic-auth.
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!
from basic-auth.
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.
from basic-auth.
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).
from basic-auth.
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 ❤️ !
from basic-auth.
Good to close, thank you
from basic-auth.
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
from basic-auth.
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.
from basic-auth.
Related Issues (20)
- Logout HOT 1
- Cannot read property 'nodemon' of undefined HOT 1
- Not able to install Basic-auth on windows 10. HOT 2
- Add support for es6 default import HOT 2
- Add support for URL encoding HOT 3
- Use constant-time comparison to prevent timing attacks. HOT 2
- Is the example w/ express? and example w/ express routes? (eom) HOT 5
- Questions about realms HOT 3
- Special character in password using IE HOT 5
- Does not work when missing colon HOT 6
- Example in documentation is vulnerable to timing attack HOT 1
- Passing basic http credentials in URI HOT 3
- Vulnerabilities found HOT 3
- Question: Any breaking change from 1.1.0 -> 2.0.1 ? Can't find any CHANGELOG HOT 1
- Header is case sensitive HOT 5
- Security Vulnerability pick up by Fortify Scan (basic-auth) HOT 1
- How do i remove the pop up and make a custom html login page with basic-auth? HOT 1
- Value of validation token HOT 1
- Disallow control characters in `user-id` and `password` to be RFC-compliant. HOT 6
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from basic-auth.