diff --git a/src/certman.c b/src/certman.c index 11c711138..6a09c0884 100644 --- a/src/certman.c +++ b/src/certman.c @@ -313,8 +313,9 @@ enum { #define MAX_CHAIN_DEPTH 9 #endif -/* Returns 1 if der is a CA: isCA set and, unless self-signed, keyCertSign - * set. Already signature-verified by the caller, so parse NO_VERIFY. */ +/* Returns 1 if der is a CA: isCA set and, when a KeyUsage extension is + * present, keyCertSign set. Already signature-verified by the caller, so + * parse NO_VERIFY. */ static int CertManIntermediateIsCA(WOLFSSH_CERTMAN* cm, const unsigned char* der, word32 derSz) { @@ -336,10 +337,11 @@ static int CertManIntermediateIsCA(WOLFSSH_CERTMAN* cm, if (wc_ParseCert(decoded, WOLFSSL_FILETYPE_ASN1, NO_VERIFY, NULL) == 0) { isCA = decoded->isCA; #ifndef ALLOW_INVALID_CERTSIGN - if (isCA && !decoded->selfSigned && decoded->extKeyUsageSet && + if (isCA && decoded->extKeyUsageSet && (decoded->extKeyUsage & KEYUSE_KEY_CERT_SIGN) == 0) { - /* If a KeyUsage extension is present, an intermediate CA must - * assert the keyCertSign bit. */ + /* A KeyUsage extension must assert keyCertSign. This also + * rejects a trusted root the peer appends without the bit. + * TODO: rework after wolfSSL selfSigned handling is improved. */ isCA = 0; } #endif diff --git a/tests/unit.c b/tests/unit.c index cbd00701f..17705c616 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -60,6 +60,7 @@ !defined(NO_FILESYSTEM) #define WOLFSSH_TEST_CERTMAN_PROMOTE #include + #include #endif /* Cert manager tests that read a test cert off disk. A superset of the @@ -17984,7 +17985,8 @@ static int certmanForgeChild(const byte* issuerCert, word32 issuerCertSz, return ret; } -/* Forge a cert with the given subject CN. When isCA is set the cert asserts +/* Forge a cert with the given subject CN, or with issuerCert's subject name + * when cn is NULL (a self-issued cert). When isCA is set the cert asserts * basicConstraints CA=TRUE. When keyUsage is non-NULL the cert carries a * KeyUsage extension with the named usage(s) (e.g. "keyCertSign" or * "digitalSignature"); NULL omits the extension entirely. extKeyUsage does @@ -18013,12 +18015,17 @@ static int certmanForgeCert(const char* cn, int isCA, const char* keyUsage, /* wc_InitCert zeroed the struct, so leaving the final byte untouched * keeps the name NUL-terminated and avoids a strncpy truncation * warning on the runtime cn pointer. */ - WSTRNCPY(cert.subject.country, "US", CTC_NAME_SIZE - 1); - WSTRNCPY(cert.subject.commonName, cn, CTC_NAME_SIZE - 1); + if (cn != NULL) { + WSTRNCPY(cert.subject.country, "US", CTC_NAME_SIZE - 1); + WSTRNCPY(cert.subject.commonName, cn, CTC_NAME_SIZE - 1); + } + else { + ret = wc_SetSubjectBuffer(&cert, issuerCert, (int)issuerCertSz); + } cert.sigType = CTC_SHA256wECDSA; cert.daysValid = 365; cert.isCA = isCA; - if (keyUsage != NULL) { + if (ret == 0 && keyUsage != NULL) { #ifdef WOLFSSL_CERT_EXT ret = wc_SetKeyUsage(&cert, keyUsage); #else @@ -18045,6 +18052,14 @@ static int certmanForgeCert(const char* cn, int isCA, const char* keyUsage, } if (ret == 0) ret = wc_SetIssuerBuffer(&cert, issuerCert, (int)issuerCertSz); +#ifdef WOLFSSL_CERT_EXT + /* Key ids make the signer lookup go by key, not by the issuer name, + * which a self-issued intermediate shares with the root. */ + if (ret == 0) + ret = wc_SetSubjectKeyIdFromPublicKey_ex(&cert, ECC_TYPE, subjectKey); + if (ret == 0) + ret = wc_SetAuthKeyIdFromCert(&cert, issuerCert, (int)issuerCertSz); +#endif if (ret == 0) { sz = wc_MakeCert(&cert, der, *derSz, NULL, subjectKey, &rng); if (sz < 0) @@ -18203,6 +18218,25 @@ static int test_CertMan_NoPromoteNonCaIntermediate(void) return result; } +/* Returns 1 when der's issuer name equals its subject name. Compares the name + * hashes, since DecodedCert.selfSigned also requires a self-signature in + * some wolfSSL versions. */ +static int certmanIsSelfIssued(const byte* der, word32 derSz) +{ + DecodedCert decoded; + int ret = 0; + + wc_InitDecodedCert(&decoded, der, derSz, NULL); + if (wc_ParseCert(&decoded, CERT_TYPE, NO_VERIFY, NULL) == 0 && + WMEMCMP(decoded.issuerHash, decoded.subjectHash, + sizeof(decoded.subjectHash)) == 0) { + ret = 1; + } + wc_FreeDecodedCert(&decoded); + + return ret; +} + /* Drives CertManIntermediateIsCA through a forged [leaf <- intermediate] chain * with only the root trusted, so the intermediate CA must be promoted for the * leaf to find a signer. @@ -18215,15 +18249,19 @@ static int test_CertMan_NoPromoteNonCaIntermediate(void) * "digitalSignature" has KeyUsage but lacks keyCertSign: intermediate * CA must be demoted (keyCertSign-rejection branch). * - * expectPromote==1 asserts the intermediate was promoted (verify returns - * anything but WS_CERT_NO_SIGNER_E); ==0 asserts it was not (verify returns - * WS_CERT_NO_SIGNER_E). Promotion is the unit under test, not full chain - * success: with FPKI profile enforcement (--enable-all) a promoted chain's - * synthetic leaf is rejected later with WS_CERT_PROFILE_E, which is - * orthogonal -- hence the "anything but WS_CERT_NO_SIGNER_E" success criterion - * mirroring the negative test's sanity check. */ -static int certmanCheckIntermediate(const char* interKeyUsage, - int expectPromote) + * selfIssued gives the intermediate the root's subject name while it keeps its + * own key and the root's signature, the shape of a CA key rollover cert. + * + * appendRoot also sends the trusted root at the end of the chain, so it goes + * through CertManIntermediateIsCA too and must be promoted as well. + * + * expectPromote==1 asserts the intermediate was promoted and the leaf + * verified through it: verify returns WS_SUCCESS, or WS_CERT_PROFILE_E when + * FPKI profile enforcement (--enable-all) rejects the synthetic leaf after the + * chain verified. ==0 asserts it was not promoted (verify returns + * WS_CERT_NO_SIGNER_E). */ +static int certmanCheckIntermediate(const char* interKeyUsage, int selfIssued, + int appendRoot, int expectPromote) { int result = 0; int ret; @@ -18284,12 +18322,17 @@ static int certmanCheckIntermediate(const char* interKeyUsage, } /* Intermediate CA signed by the root. */ - ret = certmanForgeCert("IntermediateCA", 1, interKeyUsage, NULL, - root, rootSz, &rootKey, &interKey, inter, &interSz); + ret = certmanForgeCert(selfIssued ? NULL : "IntermediateCA", 1, + interKeyUsage, NULL, root, rootSz, &rootKey, &interKey, inter, + &interSz); if (ret != 0) { printf("CertMan: forge intermediate failed, ret=%d\n", ret); result = -929; goto done; } + if (selfIssued && !certmanIsSelfIssued(inter, interSz)) { + printf("CertMan: forged intermediate is not self-issued\n"); + result = -935; goto done; + } /* Leaf signed by the intermediate. */ ret = certmanForgeCert("ValidLeaf", 0, NULL, NULL, inter, interSz, @@ -18313,9 +18356,15 @@ static int certmanCheckIntermediate(const char* interKeyUsage, leaf, leafSz); chainSz = certmanAppendCert(chain, (word32)sizeof(chain), chainSz, inter, interSz); - ret = wolfSSH_CERTMAN_VerifyCerts_buffer(cm, chain, chainSz, 2); - if (expectPromote && ret == WS_CERT_NO_SIGNER_E) { - printf("CertMan: valid intermediate CA not promoted, ret=%d\n", ret); + if (appendRoot) { + chainSz = certmanAppendCert(chain, (word32)sizeof(chain), chainSz, + root, rootSz); + } + ret = wolfSSH_CERTMAN_VerifyCerts_buffer(cm, chain, chainSz, + appendRoot ? 3 : 2); + if (expectPromote && ret != WS_SUCCESS && ret != WS_CERT_PROFILE_E) { + printf("CertMan: chain did not verify through the intermediate CA, " + "ret=%d\n", ret); result = -933; goto done; } if (!expectPromote && ret != WS_CERT_NO_SIGNER_E) { @@ -18345,16 +18394,25 @@ static int test_CertMan_PromoteValidCaIntermediate(void) int result; /* A CA intermediate with no KeyUsage extension must still be promoted. */ - result = certmanCheckIntermediate(NULL, 1); + result = certmanCheckIntermediate(NULL, 0, 0, 1); + /* A peer may send the trusted root too; it asserts keyCertSign, so it is + * promoted like any other CA. */ + if (result == 0) + result = certmanCheckIntermediate(NULL, 0, 1, 1); #ifdef WOLFSSL_CERT_EXT /* As must a CA intermediate that explicitly asserts keyCertSign. */ if (result == 0) - result = certmanCheckIntermediate("keyCertSign", 1); + result = certmanCheckIntermediate("keyCertSign", 0, 0, 1); + if (result == 0) + result = certmanCheckIntermediate("keyCertSign", 1, 0, 1); #ifndef ALLOW_INVALID_CERTSIGN /* But a CA intermediate carrying a KeyUsage extension that omits - * keyCertSign must be rejected (the keyCertSign-rejection branch). */ + * keyCertSign must be rejected (the keyCertSign-rejection branch), + * self-issued or not. */ + if (result == 0) + result = certmanCheckIntermediate("digitalSignature", 0, 0, 0); if (result == 0) - result = certmanCheckIntermediate("digitalSignature", 0); + result = certmanCheckIntermediate("digitalSignature", 1, 0, 0); #endif #endif return result;