add ECC test case and review items

Author: JacobBarthelmeh

.github/workflows/windows-cert-store-test.yml

@@ -169,16 +169,24 @@ jobs:
         include:
           - server_key_source: file
             client_key_source: x509
+            key_algorithm: rsa
             test_name: "Server-File-Client-X509"
           - server_key_source: store
             client_key_source: x509
+            key_algorithm: rsa
             test_name: "Server-Store-Client-X509"
           - server_key_source: file
             client_key_source: store
+            key_algorithm: rsa
             test_name: "Server-File-Client-Store"
           - server_key_source: store
             client_key_source: store
+            key_algorithm: rsa
             test_name: "Server-Store-Client-Store"
+          - server_key_source: store
+            client_key_source: x509
+            key_algorithm: ecdsa
+            test_name: "Server-Store-Client-X509-ECDSA"
 
     steps:
     - uses: actions/checkout@v4
@@ -269,14 +277,24 @@ jobs:
         # For server: use LocalMachine so service (LocalSystem) can access it
         # For client: use CurrentUser (accessed by testuser)
         if ("${{ matrix.server_key_source }}" -eq "store") {
-          $serverCert = New-SelfSignedCertificate `
-              -Subject "CN=wolfSSH-Test-Server" `
-              -KeyAlgorithm RSA `
-              -KeyLength 2048 `
-              -CertStoreLocation "Cert:\LocalMachine\My" `
-              -KeyExportPolicy Exportable `
-              -NotAfter (Get-Date).AddYears(1) `
-              -KeyUsage DigitalSignature, KeyEncipherment
+          if ("${{ matrix.key_algorithm }}" -eq "ecdsa") {
+            $serverCert = New-SelfSignedCertificate `
+                -Subject "CN=wolfSSH-Test-Server" `
+                -KeyAlgorithm ECDSA_nistP256 `
+                -CertStoreLocation "Cert:\LocalMachine\My" `
+                -KeyExportPolicy Exportable `
+                -NotAfter (Get-Date).AddYears(1) `
+                -KeyUsage DigitalSignature
+          } else {
+            $serverCert = New-SelfSignedCertificate `
+                -Subject "CN=wolfSSH-Test-Server" `
+                -KeyAlgorithm RSA `
+                -KeyLength 2048 `
+                -CertStoreLocation "Cert:\LocalMachine\My" `
+                -KeyExportPolicy Exportable `
+                -NotAfter (Get-Date).AddYears(1) `
+                -KeyUsage DigitalSignature, KeyEncipherment
+          }
           Write-Host "Server cert created in LocalMachine: $($serverCert.Subject)"
 
           # Grant LocalSystem (NT AUTHORITY\SYSTEM) access to the private key.
@@ -289,7 +307,7 @@ jobs:
           # Try multiple methods to get the private key info
           $keyFound = $false
 
-          # Method 1: Try RSACertificateExtensions
+          # Method 1: Try RSACertificateExtensions (RSA keys)
           try {
             Write-Host "Trying RSACertificateExtensions.GetRSAPrivateKey..."
             $rsaKey = [System.Security.Cryptography.X509Certificates.RSACertificateExtensions]::GetRSAPrivateKey($serverCert)
@@ -311,6 +329,23 @@ jobs:
             Write-Host "  RSACertificateExtensions method failed: $_"
           }
 
+          # Method 1b: Try ECDsaCertificateExtensions (ECDSA keys)
+          if (-not $keyName) {
+            try {
+              Write-Host "Trying ECDsaCertificateExtensions.GetECDsaPrivateKey..."
+              $ecdsaKey = [System.Security.Cryptography.X509Certificates.ECDsaCertificateExtensions]::GetECDsaPrivateKey($serverCert)
+              if ($ecdsaKey) {
+                Write-Host "  Got ECDSA key object: $($ecdsaKey.GetType().FullName)"
+                if ($ecdsaKey.Key) {
+                  $keyName = $ecdsaKey.Key.UniqueName
+                  Write-Host "  ECDSA CNG key unique name: $keyName"
+                }
+              }
+            } catch {
+              Write-Host "  ECDsaCertificateExtensions method failed: $_"
+            }
+          }
+
           # Method 2: Try PrivateKey property (older API)
           if (-not $keyName) {
             try {

src/certman.c

@@ -103,10 +103,13 @@ int wolfSSH_SetCertManager(WOLFSSH_CTX* ctx, WOLFSSL_CERT_MANAGER* cm)
     }
 
     /* free up existing cm if present */
+    if (ctx->certMan->cm == cm) {
+        return WS_SUCCESS;
+    }
+    wolfSSL_CertManager_up_ref(cm);
     if (ctx->certMan->cm != NULL) {
         wolfSSL_CertManagerFree(ctx->certMan->cm);
     }
-    wolfSSL_CertManager_up_ref(cm);
     ctx->certMan->cm = cm;
 
     return WS_SUCCESS;
@@ -638,6 +641,11 @@ int wolfSSH_ParseCertStoreSpec(const char* spec,
     wSubjectNameLen = MultiByteToWideChar(CP_UTF8, 0, subjectName, -1,
             NULL, 0);
 
+    if (wStoreNameLen == 0 || wSubjectNameLen == 0) {
+        WFREE(specCopy, heap, DYNTYPE_TEMP);
+        return WS_FATAL_ERROR;
+    }
+
     *wStoreName = (wchar_t*)WMALLOC(wStoreNameLen * sizeof(wchar_t),
             heap, DYNTYPE_TEMP);
     *wSubjectName = (wchar_t*)WMALLOC(wSubjectNameLen * sizeof(wchar_t),

src/internal.c

@@ -1127,11 +1127,11 @@ void CtxResourceFree(WOLFSSH_CTX* ctx)
                     ctx->privateKey[i].certStoreContext = NULL;
                 }
                 if (ctx->privateKey[i].storeName != NULL) {
-                    WFREE((void*)ctx->privateKey[i].storeName, ctx->heap, DYNTYPE_STRING);
+                    WFREE(ctx->privateKey[i].storeName, ctx->heap, DYNTYPE_STRING);
                     ctx->privateKey[i].storeName = NULL;
                 }
                 if (ctx->privateKey[i].subjectName != NULL) {
-                    WFREE((void*)ctx->privateKey[i].subjectName, ctx->heap, DYNTYPE_STRING);
+                    WFREE(ctx->privateKey[i].subjectName, ctx->heap, DYNTYPE_STRING);
                     ctx->privateKey[i].subjectName = NULL;
                 }
                 ctx->privateKey[i].useCertStore = 0;
@@ -11369,6 +11369,47 @@ static int SendKexGetSigningKey(WOLFSSH* ssh,
             ret = wc_ecc_init_ex(&sigKeyBlock_ptr->sk.ecc.key, heap,
                     INVALID_DEVID);
             scratch = 0;
+#ifdef USE_WINDOWS_API
+#ifdef WOLFSSH_CERTS
+            if (ret == 0 && ssh->ctx->privateKey[keyIdx].useCertStore) {
+                /* For cert store keys, extract the ECC public key from the
+                 * DER certificate.  Signing uses the cert store handle via
+                 * SignHEcdsa's cert-store branch. */
+                const byte* certDer =
+                        ssh->ctx->privateKey[keyIdx].cert;
+                word32 certDerSz =
+                        ssh->ctx->privateKey[keyIdx].certSz;
+
+                if (certDer != NULL && certDerSz > 0) {
+                    byte*  pubKeyDer = NULL;
+                    word32 pubKeyDerSz = 0;
+
+                    ret = ExtractPubKeyDerFromCert(certDer, certDerSz,
+                            &pubKeyDer, &pubKeyDerSz, heap);
+                    if (ret == 0) {
+                        word32 idx2 = 0;
+                        ret = wc_EccPublicKeyDecode(pubKeyDer, &idx2,
+                                &sigKeyBlock_ptr->sk.ecc.key, pubKeyDerSz);
+                    }
+                    if (pubKeyDer != NULL)
+                        WFREE(pubKeyDer, heap, DYNTYPE_PUBKEY);
+
+                    if (ret != 0) {
+                        WLOG(WS_LOG_DEBUG,
+                            "SendKexDhReply: cert store ECC pubkey "
+                            "decode failed %d", ret);
+                        ret = WS_CRYPTO_FAILED;
+                    }
+                }
+                else {
+                    WLOG(WS_LOG_DEBUG,
+                        "SendKexDhReply: cert store key has no cert DER");
+                    ret = WS_BAD_ARGUMENT;
+                }
+            }
+            else
+#endif /* WOLFSSH_CERTS */
+#endif /* USE_WINDOWS_API */
             if (ret == 0)
                 ret = wc_EccPrivateKeyDecode(ssh->ctx->privateKey[keyIdx].key,
                         &scratch, &sigKeyBlock_ptr->sk.ecc.key,
@@ -12655,8 +12696,19 @@ static int SignHEcdsa(WOLFSSH* ssh, byte* sig, word32* sigSz,
         /* NCryptSignHash for ECDSA returns raw r||s (each half of sigSz),
          * NOT DER-encoded.  Split directly. */
         if (sigKey->pvtKey != NULL && sigKey->pvtKey->useCertStore) {
-            word32 halfSz = *sigSz / 2;
+            word32 halfSz;
             word32 rOff = 0, sOff = 0;
+            if (*sigSz < 2 || (*sigSz & 1) != 0) {
+                ret = WS_ECC_E;
+            }
+            halfSz = *sigSz / 2;
+            if (ret == WS_SUCCESS && (halfSz > rSz || halfSz > sSz)) {
+                ret = WS_ECC_E;
+            }
+            if (ret != WS_SUCCESS) {
+                /* skip the copy/trim below */
+            }
+            else {
             WMEMCPY(r, sig, halfSz);
             WMEMCPY(s, sig + halfSz, halfSz);
             /* Trim leading zeroes (use offset to preserve base pointer
@@ -12671,6 +12723,7 @@ static int SignHEcdsa(WOLFSSH* ssh, byte* sig, word32* sigSz,
             if (sOff > 0)
                 WMEMMOVE(s, s + sOff, halfSz - sOff);
             sSz = halfSz - sOff;
+            }
         } else
 #endif /* WOLFSSH_CERTS */
 #endif /* USE_WINDOWS_API */

src/ssh.c

@@ -2501,6 +2501,7 @@ int wolfSSH_CTX_UsePrivateKey_fromStore(WOLFSSH_CTX* ctx,
     PCCERT_CONTEXT pCertContext = NULL;
     word32 keyIdx = 0;
     byte keyId = ID_NONE;
+    byte addedNewSlot = 0;
     void* heap = NULL;
 
     WLOG(WS_LOG_DEBUG, "Entering wolfSSH_CTX_UsePrivateKey_fromStore()");
@@ -2624,6 +2625,7 @@ int wolfSSH_CTX_UsePrivateKey_fromStore(WOLFSSH_CTX* ctx,
         if (keyIdx == WOLFSSH_MAX_PVT_KEYS && ctx->privateKeyCount < WOLFSSH_MAX_PVT_KEYS) {
             keyIdx = ctx->privateKeyCount;
             ctx->privateKeyCount++;
+            addedNewSlot = 1;
         }
     }
 
@@ -2634,6 +2636,19 @@ int wolfSSH_CTX_UsePrivateKey_fromStore(WOLFSSH_CTX* ctx,
         return WS_MEMORY_E;
     }
 
+    /* Free existing resources if replacing an existing slot */
+    if (ctx->privateKey[keyIdx].useCertStore) {
+        if (ctx->privateKey[keyIdx].certStoreContext != NULL)
+            CertFreeCertificateContext(
+                (PCCERT_CONTEXT)ctx->privateKey[keyIdx].certStoreContext);
+        if (ctx->privateKey[keyIdx].storeName != NULL)
+            WFREE(ctx->privateKey[keyIdx].storeName, heap, DYNTYPE_STRING);
+        if (ctx->privateKey[keyIdx].subjectName != NULL)
+            WFREE(ctx->privateKey[keyIdx].subjectName, heap, DYNTYPE_STRING);
+        if (ctx->privateKey[keyIdx].cert != NULL)
+            WFREE(ctx->privateKey[keyIdx].cert, heap, DYNTYPE_CERT);
+    }
+
     /* Set up the private key structure */
     ctx->privateKey[keyIdx].publicKeyFmt = keyId;
     ctx->privateKey[keyIdx].useCertStore = 1;
@@ -2668,8 +2683,8 @@ int wolfSSH_CTX_UsePrivateKey_fromStore(WOLFSSH_CTX* ctx,
         byte* certBuf = (byte*)WMALLOC(certSz, heap, DYNTYPE_CERT);
         if (certBuf == NULL) {
             /* Cleanup */
-            WFREE((void*)ctx->privateKey[keyIdx].storeName, heap, DYNTYPE_STRING);
-            WFREE((void*)ctx->privateKey[keyIdx].subjectName, heap, DYNTYPE_STRING);
+            WFREE(ctx->privateKey[keyIdx].storeName, heap, DYNTYPE_STRING);
+            WFREE(ctx->privateKey[keyIdx].subjectName, heap, DYNTYPE_STRING);
             CertFreeCertificateContext(pCertContext);
             CertCloseStore(hStore, 0);
             WLOG(WS_LOG_DEBUG, "wolfSSH_CTX_UsePrivateKey_fromStore: Certificate buffer allocation failed");
@@ -2697,8 +2712,8 @@ int wolfSSH_CTX_UsePrivateKey_fromStore(WOLFSSH_CTX* ctx,
                  "access private key, error: %lu. Check that the current user "
                  "or service account has permission to access the key.", dwErr);
             /* Cleanup already stored data */
-            WFREE((void*)ctx->privateKey[keyIdx].storeName, heap, DYNTYPE_STRING);
-            WFREE((void*)ctx->privateKey[keyIdx].subjectName, heap, DYNTYPE_STRING);
+            WFREE(ctx->privateKey[keyIdx].storeName, heap, DYNTYPE_STRING);
+            WFREE(ctx->privateKey[keyIdx].subjectName, heap, DYNTYPE_STRING);
             WFREE(ctx->privateKey[keyIdx].cert, heap, DYNTYPE_CERT);
             ctx->privateKey[keyIdx].useCertStore = 0;
             CertFreeCertificateContext(pCertContext);
@@ -2707,7 +2722,8 @@ int wolfSSH_CTX_UsePrivateKey_fromStore(WOLFSSH_CTX* ctx,
             ctx->privateKey[keyIdx].subjectName = NULL;
             ctx->privateKey[keyIdx].cert = NULL;
             ctx->privateKey[keyIdx].certSz = 0;
-            ctx->privateKeyCount--;
+            if (addedNewSlot)
+                ctx->privateKeyCount--;
             CertCloseStore(hStore, 0);
             return WS_CRYPTO_FAILED;
         }

wolfssh/internal.h

@@ -546,9 +546,9 @@ typedef struct WOLFSSH_PVT_KEY {
     void* certStoreContext;
         /* Windows certificate context (PCCERT_CONTEXT) for MS Certificate Store.
          * Owned by CTX, must be freed with CertFreeCertificateContext. */
-    const wchar_t* storeName;
+    wchar_t* storeName;
         /* Certificate store name (e.g., "My", "Root"). Owned by CTX. */
-    const wchar_t* subjectName;
+    wchar_t* subjectName;
         /* Certificate subject name or thumbprint for lookup. Owned by CTX. */
     DWORD dwFlags;
         /* Certificate store flags (e.g., CERT_SYSTEM_STORE_CURRENT_USER). */