Code review feedback

Author: kareem-wolfssl

doc/dox_comments/header_files/dsa.h

@@ -162,6 +162,56 @@ int wc_DsaSign(const byte* digest, byte* out,
 int wc_DsaVerify(const byte* digest, const byte* sig,
                              DsaKey* key, int* answer);
 
+/*!
+    \ingroup DSA
+
+    \brief This function validates DSA domain parameters and the public
+    key contained in the given DsaKey. It performs the following checks
+    (a subset of the requirements in FIPS 186-4 and NIST SP 800-89):
+    p > 1 and q > 1; q divides (p - 1); 1 < g < p; 1 < y < p; g^q mod p
+    == 1 (so the multiplicative order of g divides q); and y^q mod p == 1
+    (so the multiplicative order of y divides q).  The verify
+    routines (wc_DsaVerify, wc_DsaVerify_ex) call this internally before
+    any signature math runs, but the function is also exposed so callers
+    can validate keys at import or decode time.
+
+    \return 0 Returned when the key and domain parameters pass validation.
+    \return BAD_FUNC_ARG Returned when key is NULL, or when the key fails
+    any of the validation checks listed above.
+    \return MEMORY_E Returned on memory allocation failure (small-stack
+    builds only).
+    \return MP_INIT_E Returned when mp_int initialization fails.
+    \return Other negative MP error codes (e.g. MP_EXPTMOD_E) Returned on
+    internal big-integer failures.
+
+    Note: this function does not run primality tests on p or q. Full
+    FIPS 186-4 domain-parameter validation additionally requires that p
+    and q be prime; callers that need that level of assurance should use
+    wc_DsaImportParamsRawCheck() (which validates p) and/or run
+    mp_prime_is_prime_ex() on q at import time.
+
+    \param key pointer to a DsaKey structure populated with p, q, g, and y
+
+    _Example_
+    \code
+    DsaKey key;
+    int ret;
+    wc_InitDsaKey(&key);
+    // ... import or decode the public key into &key ...
+    ret = wc_DsaCheckPubKey(&key);
+    if (ret != 0) {
+        // domain parameters or public key are invalid; reject
+    }
+    wc_FreeDsaKey(&key);
+    \endcode
+
+    \sa wc_DsaVerify
+    \sa wc_DsaPublicKeyDecode
+    \sa wc_DsaImportParamsRaw
+    \sa wc_DsaImportParamsRawCheck
+*/
+int wc_DsaCheckPubKey(DsaKey* key);
+
 /*!
     \ingroup DSA
 

tests/api/test_dsa.c

@@ -665,8 +665,9 @@ int test_wc_DsaCheckPubKey(void)
     ExpectIntEQ(mp_copy(&key.p, &key.g), 0);
     ExpectIntEQ(wc_DsaCheckPubKey(&key), WC_NO_ERR_TRACE(BAD_FUNC_ARG));
 
-    /* g in range [2, p-1] but NOT in the order-q subgroup.
-     * g = 2 will generate a subgroup of order != q, so 2^q mod p != 1. */
+    /* g in range [2, p-1] but ord(g) does not divide q.
+     * For our FIPS-style p, 2 has order (p-1)/k for small k and (p-1)/q
+     * is not 1, so 2^q mod p != 1 and the check rejects. */
     ExpectIntEQ(mp_set(&key.g, 2), 0);
     ExpectIntEQ(wc_DsaCheckPubKey(&key), WC_NO_ERR_TRACE(BAD_FUNC_ARG));
 
@@ -688,7 +689,7 @@ int test_wc_DsaCheckPubKey(void)
     ExpectIntEQ(mp_copy(&key.p, &key.y), 0);
     ExpectIntEQ(wc_DsaCheckPubKey(&key), WC_NO_ERR_TRACE(BAD_FUNC_ARG));
 
-    /* y in range but NOT in the order-q subgroup: y = 2. */
+    /* y in range but ord(y) does not divide q: y = 2 fails y^q mod p == 1. */
     ExpectIntEQ(mp_set(&key.y, 2), 0);
     ExpectIntEQ(wc_DsaCheckPubKey(&key), WC_NO_ERR_TRACE(BAD_FUNC_ARG));
 

wolfcrypt/src/dsa.c

@@ -101,8 +101,8 @@ void wc_FreeDsaKey(DsaKey* key)
  *   - q divides (p - 1)            (FIPS 186-4 A.1.1.2)
  *   - 1 < g < p
  *   - 1 < y < p
- *   - g^q mod p == 1  (g generates the order-q subgroup)
- *   - y^q mod p == 1  (y is in the order-q subgroup)
+ *   - g^q mod p == 1  (i.e. ord(g) divides q)
+ *   - y^q mod p == 1  (i.e. ord(y) divides q)
  *
  * Note: this routine does not run primality tests on p or q. Full FIPS
  * 186-4 domain-parameter validation additionally requires that p and q be
@@ -1135,6 +1135,12 @@ int wc_DsaVerify_ex(const byte* digest, word32 digestSz, const byte* sig,
         return BAD_LENGTH_E;
     }
 
+    /* Validate domain parameters and public key before doing any
+     * signature math. */
+    ret = wc_DsaCheckPubKey(key);
+    if (ret != 0)
+        return ret;
+
     do {
 #ifdef WOLFSSL_SMALL_STACK
         w = (mp_int *)XMALLOC(sizeof *w, key->heap, DYNAMIC_TYPE_TMP_BUFFER);
@@ -1166,13 +1172,6 @@ int wc_DsaVerify_ex(const byte* digest, word32 digestSz, const byte* sig,
             break;
         }
 
-        /* Validate domain parameters and public key before doing any
-         * signature math. */
-        ret = wc_DsaCheckPubKey(key);
-        if (ret != 0) {
-            break;
-        }
-
         /* set r and s from signature */
         if (mp_read_unsigned_bin(r, sig, (word32)qSz) != MP_OKAY ||
             mp_read_unsigned_bin(s, sig + qSz, (word32)qSz) != MP_OKAY) {