Addresses @sanjanarajan's review comments

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
+      // See Appendix B here: https://nvlpubs.nist.gov/nistpubs/FIPS/NIST.FIPS.186-4.pdf
-      k = random.getRandomBN(two, q.subn(1)); // returns in [2, q-2]
+      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;

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
+    // See Section 11.5 here: https://crypto.stanford.edu/~dabo/cryptobook/BonehShoup_0_4.pdf
-    const k = random.getRandomBN(two, p.subn(1)); // returns in [2, p-2]
+    const k = random.getRandomBN(zero, p); // returns in [0, p-1]
     return {
       c1: gred.redPow(k).fromRed(),
       c2: yred.redPow(k).redMul(mred).fromRed()

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 {BN}       n    Number to test
- * @param {Integer}  k Optional number of iterations of Miller-Rabin test
+ * @param {Integer}  k    Optional number of iterations of Miller-Rabin test
- * @param {Function} w Optional function to generate potential witnesses
+ * @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;
     }
 

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)
     };
   },

src/crypto/random.js

@@ -108,17 +108,19 @@ export default {
     }
 
     let r;
-    const modulus = max.sub(min);
+    const diff = max.sub(min);
-    const bits = modulus.bitLength();
+    const bits = diff.bitLength();
-    const bytes = modulus.byteLength();
+    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);
   },

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
+    // TODO replace when Promise.forEach is implemented
-    await Promise.all(user.selfCertifications.map(async function(selfCertification) {
+    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 invalid, revoked, or expired
-      // skip if certificates is not valid
+      // eslint-disable-next-line no-await-in-loop
-      if (!(selfCertification.verified || await selfCertification.verify(primaryKey, dataToVerify)) ||
+      if (!(selfCertification.verified || await selfCertification.verify(primaryKey, dataToVerify))) {
-          (selfCertification.revoked || await user.isRevoked(primaryKey, selfCertification)) ||
+        return;
-          selfCertification.isExpired()) {
+      }
+      // 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) :