From 03e6f7cca3367082cb381b8071387f2ebc271dbb Mon Sep 17 00:00:00 2001 From: John Safranek Date: Wed, 18 May 2016 10:39:18 -0700 Subject: [PATCH 1/3] RFC 5280 Appendix A.1 states that the Country Name in a certificate shall have a size of 2 octets. Restrict country name length to 2 or 0. --- wolfcrypt/src/asn.c | 8 ++++++++ wolfssl/wolfcrypt/asn_public.h | 1 + 2 files changed, 9 insertions(+) diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index 17d3aa7eb..84ea1358e 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -6712,6 +6712,14 @@ static int SetName(byte* output, word32 outputSz, CertName* name) int thisLen = strLen; int firstSz, secondSz, seqSz, setSz; + /* Restrict country code size */ + if (i == 0) { + if (strLen >= CTC_COUNTRY_SIZE) + strLen = CTC_COUNTRY_SIZE; + else + strLen = 0; + } + if (strLen == 0) { /* no user data for this item */ names[i].used = 0; continue; diff --git a/wolfssl/wolfcrypt/asn_public.h b/wolfssl/wolfcrypt/asn_public.h index 0e719ddb2..83140e674 100644 --- a/wolfssl/wolfcrypt/asn_public.h +++ b/wolfssl/wolfcrypt/asn_public.h @@ -77,6 +77,7 @@ enum Ctc_Encoding { }; enum Ctc_Misc { + CTC_COUNTRY_SIZE = 2, CTC_NAME_SIZE = 64, CTC_DATE_SIZE = 32, CTC_MAX_ALT_SIZE = 16384, /* may be huge */ From 5c8daa0ac654ff565d2bf08c2f64a40b581f3370 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Wed, 18 May 2016 15:04:40 -0700 Subject: [PATCH 2/3] 1. SetName() should return error if country code isn't 2 bytes. 2. MakeCert() was not checking return codes correctly for the SetFoo() functions. 3. Added error code for invalid country code length. --- wolfcrypt/src/asn.c | 50 ++++++++++++++++----------------- wolfcrypt/src/error.c | 3 ++ wolfssl/wolfcrypt/error-crypt.h | 1 + 3 files changed, 29 insertions(+), 25 deletions(-) diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index 84ea1358e..4862f68b6 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -6712,19 +6712,19 @@ static int SetName(byte* output, word32 outputSz, CertName* name) int thisLen = strLen; int firstSz, secondSz, seqSz, setSz; - /* Restrict country code size */ - if (i == 0) { - if (strLen >= CTC_COUNTRY_SIZE) - strLen = CTC_COUNTRY_SIZE; - else - strLen = 0; - } - if (strLen == 0) { /* no user data for this item */ names[i].used = 0; continue; } + /* Restrict country code size */ + if (i == 0 && strLen != CTC_COUNTRY_SIZE) { +#ifdef WOLFSSL_SMALL_STACK + XFREE(names, NULL, DYNAMIC_TYPE_TMP_BUFFER); +#endif + return ASN_COUNTRY_SIZE_E; + } + secondSz = SetLength(strLen, secondLen); thisLen += secondSz; if (email) { @@ -6858,7 +6858,7 @@ static int EncodeCert(Cert* cert, DerCert* der, RsaKey* rsaKey, ecc_key* eccKey, /* signature algo */ der->sigAlgoSz = SetAlgoID(cert->sigType, der->sigAlgo, oidSigType, 0); - if (der->sigAlgoSz == 0) + if (der->sigAlgoSz <= 0) return ALGO_ID_E; /* public key */ @@ -6907,7 +6907,7 @@ static int EncodeCert(Cert* cert, DerCert* der, RsaKey* rsaKey, ecc_key* eccKey, /* date validity copy ? */ if (cert->beforeDateSz && cert->afterDateSz) { der->validitySz = CopyValidity(der->validity, cert); - if (der->validitySz == 0) + if (der->validitySz <= 0) return DATE_E; } #endif @@ -6915,19 +6915,19 @@ static int EncodeCert(Cert* cert, DerCert* der, RsaKey* rsaKey, ecc_key* eccKey, /* date validity */ if (der->validitySz == 0) { der->validitySz = SetValidity(der->validity, cert->daysValid); - if (der->validitySz == 0) + if (der->validitySz <= 0) return DATE_E; } /* subject name */ der->subjectSz = SetName(der->subject, sizeof(der->subject), &cert->subject); - if (der->subjectSz == 0) + if (der->subjectSz <= 0) return SUBJECT_E; /* issuer name */ der->issuerSz = SetName(der->issuer, sizeof(der->issuer), cert->selfSigned ? &cert->subject : &cert->issuer); - if (der->issuerSz == 0) + if (der->issuerSz <= 0) return ISSUER_E; /* set the extensions */ @@ -6936,7 +6936,7 @@ static int EncodeCert(Cert* cert, DerCert* der, RsaKey* rsaKey, ecc_key* eccKey, /* CA */ if (cert->isCA) { der->caSz = SetCa(der->ca, sizeof(der->ca)); - if (der->caSz == 0) + if (der->caSz <= 0) return CA_TRUE_E; der->extensionsSz += der->caSz; @@ -6949,7 +6949,7 @@ static int EncodeCert(Cert* cert, DerCert* der, RsaKey* rsaKey, ecc_key* eccKey, if (cert->altNamesSz) { der->altNamesSz = SetAltNames(der->altNames, sizeof(der->altNames), cert->altNames, cert->altNamesSz); - if (der->altNamesSz == 0) + if (der->altNamesSz <= 0) return ALT_NAME_E; der->extensionsSz += der->altNamesSz; @@ -6967,7 +6967,7 @@ static int EncodeCert(Cert* cert, DerCert* der, RsaKey* rsaKey, ecc_key* eccKey, der->skidSz = SetSKID(der->skid, sizeof(der->skid), cert->skid, cert->skidSz); - if (der->skidSz == 0) + if (der->skidSz <= 0) return SKID_E; der->extensionsSz += der->skidSz; @@ -6983,7 +6983,7 @@ static int EncodeCert(Cert* cert, DerCert* der, RsaKey* rsaKey, ecc_key* eccKey, der->akidSz = SetAKID(der->akid, sizeof(der->akid), cert->akid, cert->akidSz); - if (der->akidSz == 0) + if (der->akidSz <= 0) return AKID_E; der->extensionsSz += der->akidSz; @@ -6995,7 +6995,7 @@ static int EncodeCert(Cert* cert, DerCert* der, RsaKey* rsaKey, ecc_key* eccKey, if (cert->keyUsage != 0){ der->keyUsageSz = SetKeyUsage(der->keyUsage, sizeof(der->keyUsage), cert->keyUsage); - if (der->keyUsageSz == 0) + if (der->keyUsageSz <= 0) return KEYUSAGE_E; der->extensionsSz += der->keyUsageSz; @@ -7009,7 +7009,7 @@ static int EncodeCert(Cert* cert, DerCert* der, RsaKey* rsaKey, ecc_key* eccKey, sizeof(der->certPolicies), cert->certPolicies, cert->certPoliciesNb); - if (der->certPoliciesSz == 0) + if (der->certPoliciesSz <= 0) return CERTPOLICIES_E; der->extensionsSz += der->certPoliciesSz; @@ -7025,7 +7025,7 @@ static int EncodeCert(Cert* cert, DerCert* der, RsaKey* rsaKey, ecc_key* eccKey, der->extensionsSz = SetExtensionsHeader(der->extensions, sizeof(der->extensions), der->extensionsSz); - if (der->extensionsSz == 0) + if (der->extensionsSz <= 0) return EXTENSIONS_E; /* put CA */ @@ -7043,7 +7043,7 @@ static int EncodeCert(Cert* cert, DerCert* der, RsaKey* rsaKey, ecc_key* eccKey, ret = SetExtensions(der->extensions, sizeof(der->extensions), &der->extensionsSz, der->altNames, der->altNamesSz); - if (ret == 0) + if (ret <= 0) return EXTENSIONS_E; } #endif @@ -7054,7 +7054,7 @@ static int EncodeCert(Cert* cert, DerCert* der, RsaKey* rsaKey, ecc_key* eccKey, ret = SetExtensions(der->extensions, sizeof(der->extensions), &der->extensionsSz, der->skid, der->skidSz); - if (ret == 0) + if (ret <= 0) return EXTENSIONS_E; } @@ -7063,7 +7063,7 @@ static int EncodeCert(Cert* cert, DerCert* der, RsaKey* rsaKey, ecc_key* eccKey, ret = SetExtensions(der->extensions, sizeof(der->extensions), &der->extensionsSz, der->akid, der->akidSz); - if (ret == 0) + if (ret <= 0) return EXTENSIONS_E; } @@ -7072,7 +7072,7 @@ static int EncodeCert(Cert* cert, DerCert* der, RsaKey* rsaKey, ecc_key* eccKey, ret = SetExtensions(der->extensions, sizeof(der->extensions), &der->extensionsSz, der->keyUsage, der->keyUsageSz); - if (ret == 0) + if (ret <= 0) return EXTENSIONS_E; } @@ -7081,7 +7081,7 @@ static int EncodeCert(Cert* cert, DerCert* der, RsaKey* rsaKey, ecc_key* eccKey, ret = SetExtensions(der->extensions, sizeof(der->extensions), &der->extensionsSz, der->certPolicies, der->certPoliciesSz); - if (ret == 0) + if (ret <= 0) return EXTENSIONS_E; } #endif /* WOLFSSL_CERT_EXT */ diff --git a/wolfcrypt/src/error.c b/wolfcrypt/src/error.c index 5ad5d77a5..f38ee38e8 100644 --- a/wolfcrypt/src/error.c +++ b/wolfcrypt/src/error.c @@ -380,6 +380,9 @@ const char* wc_GetErrorString(int error) case WC_KEY_SIZE_E: return "Key size error, either too small or large"; + case ASN_COUNTRY_SIZE_E: + return "Country code size error, either too small or large"; + default: return "unknown error number"; diff --git a/wolfssl/wolfcrypt/error-crypt.h b/wolfssl/wolfcrypt/error-crypt.h index fe1aef3e5..0be438d3c 100644 --- a/wolfssl/wolfcrypt/error-crypt.h +++ b/wolfssl/wolfcrypt/error-crypt.h @@ -170,6 +170,7 @@ enum { WC_PENDING_E = -233, /* wolfCrypt operation pending (would block) */ WC_KEY_SIZE_E = -234, /* Key size error, either too small or large */ + ASN_COUNTRY_SIZE_E = -235, /* ASN Cert Gen, invalid country code size */ MIN_CODE_E = -300 /* errors -101 - -299 */ From bae0fe9b63d3027a47f519ba78eb5a087a6c3e9c Mon Sep 17 00:00:00 2001 From: John Safranek Date: Wed, 18 May 2016 15:14:23 -0700 Subject: [PATCH 3/3] MakeCertReq() was not checking return codes correctly for the SetFoo() functions. --- wolfcrypt/src/asn.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index 4862f68b6..e64f56f86 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -7413,7 +7413,7 @@ static int EncodeCertReq(Cert* cert, DerCert* der, /* subject name */ der->subjectSz = SetName(der->subject, sizeof(der->subject), &cert->subject); - if (der->subjectSz == 0) + if (der->subjectSz <= 0) return SUBJECT_E; /* public key */ @@ -7442,7 +7442,7 @@ static int EncodeCertReq(Cert* cert, DerCert* der, /* CA */ if (cert->isCA) { der->caSz = SetCa(der->ca, sizeof(der->ca)); - if (der->caSz == 0) + if (der->caSz <= 0) return CA_TRUE_E; der->extensionsSz += der->caSz; @@ -7459,7 +7459,7 @@ static int EncodeCertReq(Cert* cert, DerCert* der, der->skidSz = SetSKID(der->skid, sizeof(der->skid), cert->skid, cert->skidSz); - if (der->skidSz == 0) + if (der->skidSz <= 0) return SKID_E; der->extensionsSz += der->skidSz; @@ -7471,7 +7471,7 @@ static int EncodeCertReq(Cert* cert, DerCert* der, if (cert->keyUsage != 0){ der->keyUsageSz = SetKeyUsage(der->keyUsage, sizeof(der->keyUsage), cert->keyUsage); - if (der->keyUsageSz == 0) + if (der->keyUsageSz <= 0) return KEYUSAGE_E; der->extensionsSz += der->keyUsageSz; @@ -7486,7 +7486,7 @@ static int EncodeCertReq(Cert* cert, DerCert* der, /* put the start of sequence (ID, Size) */ der->extensionsSz = SetSequence(der->extensionsSz, der->extensions); - if (der->extensionsSz == 0) + if (der->extensionsSz <= 0) return EXTENSIONS_E; /* put CA */ @@ -7494,7 +7494,7 @@ static int EncodeCertReq(Cert* cert, DerCert* der, ret = SetExtensions(der->extensions, sizeof(der->extensions), &der->extensionsSz, der->ca, der->caSz); - if (ret == 0) + if (ret <= 0) return EXTENSIONS_E; } @@ -7504,7 +7504,7 @@ static int EncodeCertReq(Cert* cert, DerCert* der, ret = SetExtensions(der->extensions, sizeof(der->extensions), &der->extensionsSz, der->skid, der->skidSz); - if (ret == 0) + if (ret <= 0) return EXTENSIONS_E; } @@ -7513,7 +7513,7 @@ static int EncodeCertReq(Cert* cert, DerCert* der, ret = SetExtensions(der->extensions, sizeof(der->extensions), &der->extensionsSz, der->akid, der->akidSz); - if (ret == 0) + if (ret <= 0) return EXTENSIONS_E; } @@ -7522,7 +7522,7 @@ static int EncodeCertReq(Cert* cert, DerCert* der, ret = SetExtensions(der->extensions, sizeof(der->extensions), &der->extensionsSz, der->keyUsage, der->keyUsageSz); - if (ret == 0) + if (ret <= 0) return EXTENSIONS_E; } @@ -7531,7 +7531,7 @@ static int EncodeCertReq(Cert* cert, DerCert* der, der->attribSz = SetReqAttrib(der->attrib, cert->challengePw, der->extensionsSz); - if (der->attribSz == 0) + if (der->attribSz <= 0) return REQ_ATTRIBUTE_E; der->total = der->versionSz + der->subjectSz + der->publicKeySz +