From d529edfddaf9db022d59f93d002d9517dd017dde Mon Sep 17 00:00:00 2001 From: Mahrud Sayrafi Date: Tue, 27 Feb 2018 13:04:45 -0800 Subject: [PATCH] Addresses @sanjanarajan's review comments --- src/crypto/public_key/dsa.js | 6 +++--- src/crypto/public_key/elgamal.js | 6 +++--- src/crypto/public_key/prime.js | 17 ++++++++--------- src/crypto/public_key/rsa.js | 5 +++-- src/crypto/random.js | 10 ++++++---- src/key.js | 22 ++++++++++++++-------- 6 files changed, 37 insertions(+), 29 deletions(-) diff --git a/src/crypto/public_key/dsa.js b/src/crypto/public_key/dsa.js index b6455e45..efffd842 100644 --- a/src/crypto/public_key/dsa.js +++ b/src/crypto/public_key/dsa.js @@ -32,7 +32,7 @@ import random from '../random'; import config from '../../config'; import util from '../../util'; -const two = new BN(2); +const one = new BN(1); const zero = new BN(0); /* @@ -72,8 +72,8 @@ export default { // signature shall be recalculated. It is extremely unlikely that r = 0 // or s = 0 if signatures are generated properly. while (true) { - // TODO confirm this range - k = random.getRandomBN(two, q.subn(1)); // returns in [2, q-2] + // See Appendix B here: https://nvlpubs.nist.gov/nistpubs/FIPS/NIST.FIPS.186-4.pdf + k = random.getRandomBN(one, q); // returns in [1, q-1] r = gred.redPow(k).fromRed().toRed(redq); // (g**k mod p) mod q if (zero.cmp(r) === 0) { continue; diff --git a/src/crypto/public_key/elgamal.js b/src/crypto/public_key/elgamal.js index 2279620f..b48402e6 100644 --- a/src/crypto/public_key/elgamal.js +++ b/src/crypto/public_key/elgamal.js @@ -26,7 +26,7 @@ import BN from 'bn.js'; import random from '../random'; -const two = new BN(2); +const zero = new BN(0); export default { /* @@ -38,8 +38,8 @@ export default { const mred = m.toRed(redp); const gred = g.toRed(redp); const yred = y.toRed(redp); - // TODO confirm this range - const k = random.getRandomBN(two, p.subn(1)); // returns in [2, p-2] + // See Section 11.5 here: https://crypto.stanford.edu/~dabo/cryptobook/BonehShoup_0_4.pdf + const k = random.getRandomBN(zero, p); // returns in [0, p-1] return { c1: gred.redPow(k).fromRed(), c2: yred.redPow(k).redMul(mred).fromRed() diff --git a/src/crypto/public_key/prime.js b/src/crypto/public_key/prime.js index ae94402e..53339f1a 100644 --- a/src/crypto/public_key/prime.js +++ b/src/crypto/public_key/prime.js @@ -75,6 +75,8 @@ function isProbablePrime(n, e, k) { if (!millerRabin(n, k)) { return false; } + // TODO implement the Lucas test + // See Section C.3.3 here: https://nvlpubs.nist.gov/nistpubs/FIPS/NIST.FIPS.186-4.pdf return true; } @@ -127,16 +129,13 @@ const lowprimes = [ // USE OR OTHER DEALINGS IN THE SOFTWARE. // Adapted on Jan 2018 from version 4.0.1 at https://github.com/indutny/miller-rabin -// TODO check this against jsbn's bnpMillerRabin -// TODO implement fixed base Miller-Rabin; for instance by writing a function that -// picks a number within the given range from a precomputed list of primes. /** * Tests whether n is probably prime or not using the Miller-Rabin test. * See HAC Remark 4.28. - * @param {BN} n Number to test - * @param {Integer} k Optional number of iterations of Miller-Rabin test - * @param {Function} w Optional function to generate potential witnesses + * @param {BN} n Number to test + * @param {Integer} k Optional number of iterations of Miller-Rabin test + * @param {Function} rand Optional function to generate potential witnesses * @return {boolean} */ function millerRabin(n, k, rand) { @@ -159,16 +158,16 @@ function millerRabin(n, k, rand) { let a = rand ? rand() : random.getRandomBN(new BN(2), n1); let x = a.toRed(red).redPow(d); - if (x.cmp(rone) === 0 || x.cmp(rn1) === 0) + if (x.eq(rone) || x.eq(rn1)) continue; let i; for (i = 1; i < s; i++) { x = x.redSqr(); - if (x.cmp(rone) === 0) + if (x.eq(rone)) return false; - if (x.cmp(rn1) === 0) + if (x.eq(rn1)) break; } diff --git a/src/crypto/public_key/rsa.js b/src/crypto/public_key/rsa.js index 48da90e7..51d9a95a 100644 --- a/src/crypto/public_key/rsa.js +++ b/src/crypto/public_key/rsa.js @@ -213,6 +213,7 @@ export default { // RSA keygen fallback using 40 iterations of the Miller-Rabin test // See https://stackoverflow.com/a/6330138 for justification + // Also see section C.3 here: https://nvlpubs.nist.gov/nistpubs/FIPS/NIST let p = prime.randomProbablePrime(B - (B >> 1), E, 40); let q = prime.randomProbablePrime(B >> 1, E, 40); @@ -225,10 +226,10 @@ export default { n: p.mul(q), e: E, d: E.invm(phi), - q: q, p: p, - // dq: d.mod(q.subn(1)), + q: q, // dp: d.mod(p.subn(1)), + // dq: d.mod(q.subn(1)), u: p.invm(q) }; }, diff --git a/src/crypto/random.js b/src/crypto/random.js index 8ce6dce4..99fb4089 100644 --- a/src/crypto/random.js +++ b/src/crypto/random.js @@ -108,17 +108,19 @@ export default { } let r; - const modulus = max.sub(min); - const bits = modulus.bitLength(); - const bytes = modulus.byteLength(); + const diff = max.sub(min); + const bits = diff.bitLength(); + const bytes = diff.byteLength(); // Using a while loop is necessary to avoid bias + // TODO consider using 64 extra random bits and taking mod + // Section B.1.1 here: https://nvlpubs.nist.gov/nistpubs/FIPS/NIST.FIPS.186-4.pdf do { r = new BN(this.getRandomBytes(bytes)); if (r.bitLength() > bits) { r.ishrn(r.bitLength() - bits); } - } while (r.cmp(modulus) >= 0); + } while (r.cmp(diff) >= 0); return r.iadd(min); }, diff --git a/src/key.js b/src/key.js index 5bf3de9a..57c8b8b4 100644 --- a/src/key.js +++ b/src/key.js @@ -696,25 +696,31 @@ Key.prototype.verifyPrimaryUser = async function(keys) { return; } const dataToVerify = { userid: user.userId || user.userAttribute, key: primaryKey }; - // TODO: fix the race condition, this should be a forEach - await Promise.all(user.selfCertifications.map(async function(selfCertification) { + // TODO replace when Promise.forEach is implemented + for (let i = 0; i < user.selfCertifications.length; i++) { + const selfCertification = user.selfCertifications[i]; // skip if certificate is not the most recent if ((selfCertification.isPrimaryUserID && selfCertification.isPrimaryUserID < lastPrimaryUserID) || (!lastPrimaryUserID && selfCertification.created < lastCreated)) { return; } - // TODO break apart the .verify/isRevoked/isExpired checks - // skip if certificates is not valid - if (!(selfCertification.verified || await selfCertification.verify(primaryKey, dataToVerify)) || - (selfCertification.revoked || await user.isRevoked(primaryKey, selfCertification)) || - selfCertification.isExpired()) { + // skip if certificates is invalid, revoked, or expired + // eslint-disable-next-line no-await-in-loop + if (!(selfCertification.verified || await selfCertification.verify(primaryKey, dataToVerify))) { + return; + } + // eslint-disable-next-line no-await-in-loop + if (selfCertification.revoked || await user.isRevoked(primaryKey, selfCertification)) { + return; + } + if (selfCertification.isExpired()) { return; } lastPrimaryUserID = selfCertification.isPrimaryUserID; lastCreated = selfCertification.created; primaryUsers.push(user); - })); + } })); const user = primaryUsers.pop(); const results = !user ? [] : keys ? await user.verifyAllCertifications(primaryKey, keys) :