Skip to content
This repository has been archived by the owner on Apr 7, 2021. It is now read-only.

Use a time based comparison #135

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

johnbeynon
Copy link

Use a constant time comparison rather than HMAC comparison

@rmm5t
Copy link
Contributor

rmm5t commented Dec 11, 2017

I like the sentiment of this change, but crypto.timingSafeEqual throws a TypeError if the two buffer lengths are not equal. Personally, I think this is a failure of the design of this function, because it allows for timing attacks to potentially be able to determine the exact length of a secret, but that's a tangential discussion.

> Crypto = require('crypto')

> Crypto.timingSafeEqual(Buffer.from("1234"), Buffer.from("1234"))
true

> Crypto.timingSafeEqual(Buffer.from("4321"), Buffer.from("1234"))
false

> Crypto.timingSafeEqual(Buffer.from("12345"), Buffer.from("1234"))
TypeError: Input buffers must have the same length
    at repl:1:8
    at ContextifyScript.Script.runInThisContext (vm.js:50:33)
    at REPLServer.defaultEval (repl.js:240:29)
    at bound (domain.js:301:14)
    at REPLServer.runBound [as eval] (domain.js:314:12)
    ...

Should we catch this TypeError to ensure a "four_oh_four" response or just allow a 50x error to bubble up?

@dpuga1
Copy link

dpuga1 commented Dec 19, 2017

I don't even know what that means

@neilmiddleton
Copy link

"four_oh_four" would make more sense to me, otherwise we're potentially revealing information about the existence of an asset.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants