Skip to content

certman: require keyCertSign on self-issued peer intermediates #1294

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
yosuke-wolfssl wants to merge 1 commit into
base: master
Choose a base branch
from
Open

certman: require keyCertSign on self-issued peer intermediates #1294

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/certman.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand All @@ -336,7 +337,7 @@ 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. */
Expand Down
76 changes: 63 additions & 13 deletions tests/unit.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17766,7 +17766,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
Expand Down Expand Up @@ -17795,12 +17796,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
Expand Down Expand Up @@ -17985,6 +17991,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.
Expand All @@ -17997,15 +18022,21 @@ static int test_CertMan_NoPromoteNonCaIntermediate(void)
* "digitalSignature" has KeyUsage but lacks keyCertSign: intermediate
* CA must be demoted (keyCertSign-rejection branch).
*
* 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 (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)
static int certmanCheckIntermediate(const char* interKeyUsage, int selfIssued,
int appendRoot, int expectPromote)
{
int result = 0;
int ret;
Expand Down Expand Up @@ -18066,12 +18097,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,
Expand All @@ -18095,7 +18131,12 @@ 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 (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_CERT_NO_SIGNER_E) {
printf("CertMan: valid intermediate CA not promoted, ret=%d\n", ret);
result = -933; goto done;
Expand Down Expand Up @@ -18127,16 +18168,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;
Expand Down
Loading