Address PR review feedback: add WOLFSSL_CERT_SIGN_CB guards, input validation, SKID extension, and -signcb example option

Author: jackctj117

.github/workflows/make-test-swtpm.yml

@@ -67,7 +67,7 @@ jobs:
           - name: certsigncb
             wolfssl_config: --enable-wolftpm --enable-pkcallbacks --enable-certsigncb
             wolftpm_config: --enable-swtpm --enable-certgen
-            test_command: "make check && WOLFSSL_PATH=./wolfssl ./examples/run_examples.sh"
+            test_command: "make check && WOLFSSL_PATH=./wolfssl ./examples/run_examples.sh && ./examples/csr/csr -signcb && ./examples/csr/csr -signcb -cert"
 
           # STMicro ST33KTPM2
           - name: st33ktpm2

examples/csr/csr.c

@@ -86,9 +86,9 @@ static const char* gClientCertEccFile = ECC_CERT_PEM;
  *    - Maintains backward compatibility
  *    - Requires crypto callback setup
  * 
- * This example demonstrates the crypto callback approach. To use the new
- * callback-based approach, pass INVALID_DEVID instead of tpmDevId when calling
- * TPM2_CSR_Generate() or wolfTPM2_CSR_MakeAndSign_ex().
+ * This example demonstrates both approaches. By default it uses the crypto
+ * callback approach. Use the -signcb option to use the new callback-based
+ * approach, which passes INVALID_DEVID to wolfTPM2_CSR_MakeAndSign_ex().
  */
 
 static int TPM2_CSR_Generate(WOLFTPM2_DEV* dev, int keyType, WOLFTPM2_KEY* key,
@@ -173,9 +173,11 @@ static int TPM2_CSR_Generate(WOLFTPM2_DEV* dev, int keyType, WOLFTPM2_KEY* key,
 static void usage(void)
 {
     printf("Expected usage:\n");
-    printf("./examples/csr/csr [-cert]\n");
+    printf("./examples/csr/csr [-cert] [-signcb]\n");
     printf("\t-cert: Make self signed certificate instead of "
                     "default CSR (Certificate Signing Request)\n");
+    printf("\t-signcb: Use wc_SignCert_cb callback-based signing "
+                    "(FIPS compliant, requires WOLFSSL_CERT_SIGN_CB)\n");
 }
 
 int TPM2_CSR_Example(void* userCtx)
@@ -192,6 +194,7 @@ int TPM2_CSR_ExampleArgs(void* userCtx, int argc, char *argv[])
     int tpmDevId;
     TPMT_PUBLIC publicTemplate;
     int makeSelfSignedCert = 0;
+    int useSignCb = 0;
 
     printf("TPM2 CSR Example\n");
 
@@ -207,6 +210,9 @@ int TPM2_CSR_ExampleArgs(void* userCtx, int argc, char *argv[])
         if (XSTRCMP(argv[argc-1], "-cert") == 0) {
             makeSelfSignedCert = 1;
         }
+        else if (XSTRCMP(argv[argc-1], "-signcb") == 0) {
+            useSignCb = 1;
+        }
         else {
             printf("Warning: Unrecognized option: %s\n", argv[argc-1]);
         }
@@ -245,7 +251,8 @@ int TPM2_CSR_ExampleArgs(void* userCtx, int argc, char *argv[])
         if (rc == 0) {
             rc = TPM2_CSR_Generate(&dev, RSA_TYPE, &key,
                 makeSelfSignedCert ? gClientCertRsaFile : gClientCsrRsaFile,
-                makeSelfSignedCert, tpmDevId, CTC_SHA256wRSA);
+                makeSelfSignedCert,
+                useSignCb ? INVALID_DEVID : tpmDevId, CTC_SHA256wRSA);
         }
         wolfTPM2_UnloadHandle(&dev, &key.handle);
         wolfTPM2_UnloadHandle(&dev, &storageKey.handle);
@@ -278,7 +285,8 @@ int TPM2_CSR_ExampleArgs(void* userCtx, int argc, char *argv[])
         if (rc == 0) {
             rc = TPM2_CSR_Generate(&dev, ECC_TYPE, &key,
                 makeSelfSignedCert ? gClientCertEccFile : gClientCsrEccFile,
-                makeSelfSignedCert, tpmDevId, sigType);
+                makeSelfSignedCert,
+                useSignCb ? INVALID_DEVID : tpmDevId, sigType);
         }
         wolfTPM2_UnloadHandle(&dev, &key.handle);
         wolfTPM2_UnloadHandle(&dev, &storageKey.handle);

src/tpm2_wrap.c

@@ -7391,13 +7391,19 @@ static int wolfTPM2_SignCertCb(const byte* in, word32 inLen,
             byte *r, *s;
             word32 rLen, sLen;
 
-            /* Split R and S from concatenated signature */
-            rLen = sLen = rsLen / 2;
-            r = &sigRS[0];
-            s = &sigRS[rLen];
+            /* Validate concatenated R||S length before splitting */
+            if (rsLen <= 0 || (rsLen & 1) != 0) {
+                rc = BAD_FUNC_ARG;
+            }
+            else {
+                /* Split R and S from concatenated signature */
+                rLen = sLen = (word32)(rsLen / 2);
+                r = &sigRS[0];
+                s = &sigRS[rLen];
 
-            /* Encode as DER ECDSA signature */
-            rc = wc_ecc_rs_raw_to_sig(r, rLen, s, sLen, out, outLen);
+                /* Encode as DER ECDSA signature */
+                rc = wc_ecc_rs_raw_to_sig(r, rLen, s, sLen, out, outLen);
+            }
         }
 #else
         rc = NOT_COMPILED_IN;
@@ -7515,6 +7521,7 @@ static int CSR_MakeAndSign_Cb(WOLFTPM2_DEV* dev, WOLFTPM2_CSR* csr,
     int selfSignCert)
 {
     int rc = 0;
+    int keyInited = 0;
     TpmSignCbCtx signCtx;
     union {
     #ifndef NO_RSA
@@ -7529,13 +7536,15 @@ static int CSR_MakeAndSign_Cb(WOLFTPM2_DEV* dev, WOLFTPM2_CSR* csr,
         return BAD_FUNC_ARG;
     }
 
+    XMEMSET(&signCtx, 0, sizeof(signCtx));
     XMEMSET(&wolfKey, 0, sizeof(wolfKey));
 
     /* Extract public key from TPM key into wolfCrypt key structure */
     if (keyType == ECC_TYPE) {
     #ifdef HAVE_ECC
         rc = wc_ecc_init(&wolfKey.ecc);
         if (rc == 0) {
+            keyInited = 1;
             /* load public portion of key into wolf ECC Key */
             rc = wolfTPM2_EccKey_TpmToWolf(dev, key, &wolfKey.ecc);
         }
@@ -7547,6 +7556,7 @@ static int CSR_MakeAndSign_Cb(WOLFTPM2_DEV* dev, WOLFTPM2_CSR* csr,
     #ifndef NO_RSA
         rc = wc_InitRsaKey(&wolfKey.rsa, NULL);
         if (rc == 0) {
+            keyInited = 1;
             /* load public portion of key into wolf RSA Key */
             rc = wolfTPM2_RsaKey_TpmToWolf(dev, key, &wolfKey.rsa);
         }
@@ -7564,6 +7574,13 @@ static int CSR_MakeAndSign_Cb(WOLFTPM2_DEV* dev, WOLFTPM2_CSR* csr,
         signCtx.key = key;
     }
 
+#ifdef WOLFSSL_CERT_EXT
+    /* add SKID from the Public Key */
+    if (rc == 0) {
+        rc = wc_SetSubjectKeyIdFromPublicKey_ex(&csr->req, keyType, &wolfKey);
+    }
+#endif
+
     /* Create certificate body with public key */
     if (rc == 0 && selfSignCert) {
 #ifdef WOLFSSL_CERT_GEN
@@ -7606,15 +7623,17 @@ static int CSR_MakeAndSign_Cb(WOLFTPM2_DEV* dev, WOLFTPM2_CSR* csr,
     }
 
     /* Cleanup wolfCrypt key structure */
-    if (keyType == ECC_TYPE) {
-    #ifdef HAVE_ECC
-        wc_ecc_free(&wolfKey.ecc);
-    #endif
-    }
-    else if (keyType == RSA_TYPE) {
-    #ifndef NO_RSA
-        wc_FreeRsaKey(&wolfKey.rsa);
-    #endif
+    if (keyInited) {
+        if (keyType == ECC_TYPE) {
+        #ifdef HAVE_ECC
+            wc_ecc_free(&wolfKey.ecc);
+        #endif
+        }
+        else if (keyType == RSA_TYPE) {
+        #ifndef NO_RSA
+            wc_FreeRsaKey(&wolfKey.rsa);
+        #endif
+        }
     }
 
     return rc;