HR / Crypter

🔓✨🔒 An innovative, convenient and secure encryption app
https://git.io/Crypter
MIT License
454 stars 70 forks source link

MasterPass.check is vulnerable to timing attacks. #28

Closed EdOverflow closed 7 years ago

EdOverflow commented 7 years ago

Summary

Timing attacks are a type of side channel attack where one can discover valuable information by recording the time it takes for a cryptographic algorithm to execute.

_.isEqual() does a byte-by-byte comparison of two values and as soon as the two differentiate it terminates. This means the longer it takes until the operation returns, the more correct characters the attacker has guessed.

const match = _.isEqual(global.creds.mpkhash, mpk.hash)

Link to source code: https://github.com/HR/Crypter/blob/master/app/src/MasterPass.js#L23

How can this be fixed?

The following code does not terminate as soon as two bytes are not the same:

var result = 0;
for (var i = 0; i < a.length; ++i) {
  result |= (a.charCodeAt(i) ^ b.charCodeAt(i));
}
return result;
HR commented 7 years ago

@EdOverflow Thanks for reporting this vulnerability. I believe that this vulnerability can be fixed by using the crypto.timingSafeEqual(a, b) method of the node crypto lib instead. The fix will be released in the imminent v3.0 release of Crypter.

If you discover any other vulnerability or have any feedback then please report it. Also feel free to contribute!

EdOverflow commented 7 years ago

I can confirm that crypto.timingSafeEqual(a, b) would solve this issue.

HR commented 7 years ago

@EdOverflow So turns out crypto.timingSafeEqual(a, b) is not available in the node version which Electron ships with (i.e. node v6.5.0). Hence, we can't make use of it 😭 .

So instead I'm implementing the function myself like so

exports.timingSafeEqual = (a, b) => {
  // convert args to buffers if not already
  a = (Buffer.isBuffer(a)) ? a : new Buffer(a)
  b = (Buffer.isBuffer(b)) ? b : new Buffer(b)
  var result = 0
  var l = a.length
  while (l--) {
    // bitwise comparison
    result |= a[l] ^ b[l]
  }
  return result === 0
}

Link in source code: https://github.com/HR/Crypter/blob/dev/app/core/crypto.js#L301

This can be used to compare any data type (strings, numbers,...). Have tested it and works as expected.

EdOverflow commented 7 years ago

Your implementation solves this issue perfectly.

HR commented 7 years ago

Issue fixed!