2fa icon indicating copy to clipboard operation
2fa copied to clipboard

Code comparison needs to be in constant time for security

Open Demuxx opened this issue 1 year ago • 1 comments

https://github.com/simontabor/2fa/blob/f42eef50f566d819a7d9ca06c36db5e4224c1ec2/lib/2FA.js#L66

The string comparison of the hotp code against the user supplied code needs to be secure against timing attacks. There are various methods to addressing this, including those in this stackoverflow:

https://stackoverflow.com/questions/31095905/whats-the-difference-between-a-secure-compare-and-a-simple

Here is a secure implementation with proof code that the timing issue exists.

const crypto = require("crypto");

function constantTimeCompare(a, b) {
  if (a.length !== b.length) {
    return false;
  }

  let diff = 0;
  for (let i = 0; i < a.length; i++) {
    diff |= a.charCodeAt(i) ^ b.charCodeAt(i);
  }

  return diff === 0;
}

function timeComparison(a, b, comparisonFunction) {
  const start = process.hrtime.bigint();
  comparisonFunction(a, b);
  const end = process.hrtime.bigint();

  return end - start;
}

const stringLength = 1000;
const almostSameString1 = "a".repeat(stringLength - 1) + "b";
const almostSameString2 = "a".repeat(stringLength);
const differentString1 = "b" + "a".repeat(stringLength - 1);
const differentString2 = "a".repeat(stringLength);

let constantTimeTotal1 = BigInt(0);
let constantTimeTotal2 = BigInt(0);
let nonConstantTimeTotal1 = BigInt(0);
let nonConstantTimeTotal2 = BigInt(0);
const iterations = 100000;

for (let i = 0; i < iterations; i++) {
  constantTimeTotal1 += timeComparison(
    almostSameString1,
    almostSameString2,
    constantTimeCompare
  );
  constantTimeTotal2 += timeComparison(
    differentString1,
    differentString2,
    constantTimeCompare
  );
  nonConstantTimeTotal1 += timeComparison(
    almostSameString1,
    almostSameString2,
    (a, b) => a === b
  );
  nonConstantTimeTotal2 += timeComparison(
    differentString1,
    differentString2,
    (a, b) => a === b
  );
}

const constantTimeAverage1 = constantTimeTotal1 / BigInt(iterations);
const constantTimeAverage2 = constantTimeTotal2 / BigInt(iterations);
const nonConstantTimeAverage1 = nonConstantTimeTotal1 / BigInt(iterations);
const nonConstantTimeAverage2 = nonConstantTimeTotal2 / BigInt(iterations);

console.log(
  "Average time for constant-time comparison when strings differ at the end:",
  constantTimeAverage1.toString()
);
console.log(
  "Average time for constant-time comparison when strings differ at the start:",
  constantTimeAverage2.toString()
);
console.log(
  "Average time for non-constant-time comparison when strings differ at the end:",
  nonConstantTimeAverage1.toString()
);
console.log(
  "Average time for non-constant-time comparison when strings differ at the start:",
  nonConstantTimeAverage2.toString()
);

Demuxx avatar Oct 27 '23 21:10 Demuxx

Note that this leaks information about the length of the hotp code, but this is OK since all codes are 6 digits always.

Demuxx avatar Oct 27 '23 22:10 Demuxx