search

LionC / express-basic-auth

Plug & play basic auth middleware for express
325 stars 57 forks source link

:lock::sparkles: Add time safe compare function #21

Closed LionC closed 5 years ago

LionC commented 5 years ago

@hraban With this merged and released, would you mind updating / closing the vulnerability on snyk?

It has been fixed in 1.1.7

feross commented 4 years ago

Why does this PR change a bunch of instances of the boolean && operator to the bitwise & operator?

Is this necessary for correctness? Is this just an optimization for speed?

LionC commented 4 years ago

@feross It is necessary to guarantee that it runs in constant time to protect against timing attacks.

The && operator shortcuts (it stops evaluating if the left side is false), while the & operator always evaluates both sides.

feross commented 4 years ago

That makes sense. A comment to that effect would have been nice. Thank you for explaining.

dcousens commented 3 years ago

staticUsersAuthorizer is still acts as a timing oracle for the number of users.

I don't know if applying an & to the loop, for a timing complexity of O(n), would be preferred to the present O(n) worst case.

Alternatively you could instead use hash(user, 'base64') to key into a user map which should be relatively constant time and eliminate the need to do safeCompare on the username.