camo icon indicating copy to clipboard operation
camo copied to clipboard

Use a time based comparison

Open johnbeynon opened this issue 7 years ago • 3 comments

Use a constant time comparison rather than HMAC comparison

johnbeynon avatar Dec 11 '17 15:12 johnbeynon

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?

rmm5t avatar Dec 11 '17 17:12 rmm5t

I don't even know what that means

dpuga1 avatar Dec 19 '17 18:12 dpuga1

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

neilmiddleton avatar Dec 20 '17 16:12 neilmiddleton