From dee3645c4e53b06dbe1ab09f6a14f3271546fb2e Mon Sep 17 00:00:00 2001 From: David Garske Date: Mon, 7 Mar 2016 13:40:25 -0800 Subject: [PATCH 1/4] Fixed bug with ASN.1 X509V3 Certificate Policy extension parsing. Bug had to do with parsing when OID contains multiple items such as example 2 below. The wolfssl.com server key now contains a URL in the certificate policy "https://secure.comodo.com/CPS0", which wasn't being parsed over correctly. Also cleanup to use loop instead of duplicate code. Example 1: 30 12 30 06 06 04 55 1D 20 00 30 08 06 06 67 81 0C 01 02 01 Result: 2.5.29.32.0 2.23.140.1.2.1 Example 2: 30 46 30 3A 06 0B 2B 06 01 04 01 B2 31 01 02 02 07 30 2B 30 29 06 08 2B 06 01 05 05 07 02 01 16 1D 68 74 74 70 73 3A 2F 2F 73 65 63 75 72 65 2E 63 6F 6D 6F 64 6F 2E 63 6F 6D 2F 43 50 53 30 08 06 06 67 81 0C 01 02 01 Result: 1.3.6.1.4.1.6449.1.2.2.7 2.23.140.1.2.1 --- wolfcrypt/src/asn.c | 107 ++++++++++++++++++-------------------------- 1 file changed, 43 insertions(+), 64 deletions(-) diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index 4bb809e23..29ce32f32 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -4596,87 +4596,64 @@ static int DecodePolicyOID(char *out, word32 outSz, byte *in, word32 inSz) #endif /* WOLFSSL_CERT_EXT && !WOLFSSL_SEP */ #if defined(WOLFSSL_SEP) || defined(WOLFSSL_CERT_EXT) + /* Reference: https://tools.ietf.org/html/rfc5280#section-4.2.1.4 */ static int DecodeCertPolicy(byte* input, int sz, DecodedCert* cert) { word32 idx = 0; - int total_length = 0, length = 0; + int total_length = 0, policy_length = 0, length = 0; WOLFSSL_ENTER("DecodeCertPolicy"); - /* Unwrap certificatePolicies */ if (GetSequence(input, &idx, &total_length, sz) < 0) { - WOLFSSL_MSG("\tdeviceType isn't OID"); + WOLFSSL_MSG("\tGet CertPolicy total seq failed"); return ASN_PARSE_E; } - if (GetSequence(input, &idx, &length, sz) < 0) { - WOLFSSL_MSG("\tdeviceType isn't OID"); - return ASN_PARSE_E; - } - total_length -= (length+1); - - if (input[idx++] != ASN_OBJECT_ID) { - WOLFSSL_MSG("\tdeviceType isn't OID"); - return ASN_PARSE_E; - } - total_length--; - - if (GetLength(input, &idx, &length, sz) < 0) { - WOLFSSL_MSG("\tCouldn't read length of deviceType"); - return ASN_PARSE_E; - } - - if (length > 0) { -#if defined(WOLFSSL_SEP) - cert->deviceType = (byte*)XMALLOC(length, cert->heap, - DYNAMIC_TYPE_X509_EXT); - if (cert->deviceType == NULL) { - WOLFSSL_MSG("\tCouldn't alloc memory for deviceType"); - return MEMORY_E; - } - cert->deviceTypeSz = length; - XMEMCPY(cert->deviceType, input + idx, length); -#elif defined(WOLFSSL_CERT_EXT) - /* decode cert policy */ - if (DecodePolicyOID(cert->extCertPolicies[0], MAX_CERTPOL_SZ, - input+idx, length) != 0) { - WOLFSSL_MSG("\tCouldn't read Policy OID 1"); + /* Unwrap certificatePolicies */ + do { + if (GetSequence(input, &idx, &policy_length, sz) < 0) { + WOLFSSL_MSG("\tGet CertPolicy seq failed"); return ASN_PARSE_E; } - cert->extCertPoliciesNb++; - /* check if we have a second value */ - if (total_length) { - idx += length; + if (input[idx++] != ASN_OBJECT_ID) { + WOLFSSL_MSG("\tCertPolicy isn't OID"); + return ASN_PARSE_E; + } + policy_length--; - if (GetSequence(input, &idx, &length, sz) < 0) { - WOLFSSL_MSG("\tdeviceType isn't OID"); - return ASN_PARSE_E; + if (GetLength(input, &idx, &length, sz) < 0) { + WOLFSSL_MSG("\tGet CertPolicy length failed"); + return ASN_PARSE_E; + } + policy_length--; + + if (length > 0) { + #if defined(WOLFSSL_SEP) + cert->deviceType = (byte*)XMALLOC(length, cert->heap, + DYNAMIC_TYPE_X509_EXT); + if (cert->deviceType == NULL) { + WOLFSSL_MSG("\tCouldn't alloc memory for deviceType"); + return MEMORY_E; } - - if (input[idx++] != ASN_OBJECT_ID) { - WOLFSSL_MSG("\tdeviceType isn't OID"); - return ASN_PARSE_E; - } - - if (GetLength(input, &idx, &length, sz) < 0) { - WOLFSSL_MSG("\tCouldn't read length of deviceType"); - return ASN_PARSE_E; - } - + cert->deviceTypeSz = length; + XMEMCPY(cert->deviceType, input + idx, length); + break; + #elif defined(WOLFSSL_CERT_EXT) /* decode cert policy */ - if (DecodePolicyOID(cert->extCertPolicies[1], MAX_CERTPOL_SZ, - input+idx, length) != 0) { - WOLFSSL_MSG("\tCouldn't read Policy OID 2"); + if (DecodePolicyOID(cert->extCertPolicies[cert->extCertPoliciesNb], MAX_CERTPOL_SZ, + input + idx, length) != 0) { + WOLFSSL_MSG("\tCouldn't decode CertPolicy"); return ASN_PARSE_E; } cert->extCertPoliciesNb++; + #else + WOLFSSL_LEAVE("DecodeCertPolicy : unsupported mode", 0); + return 0; + #endif } -#else - WOLFSSL_LEAVE("DecodeCertPolicy : unsupported mode", 0); - return 0; -#endif - } + idx += policy_length; + } while((int)idx < total_length && cert->extCertPoliciesNb < MAX_CERTPOL_NB); WOLFSSL_LEAVE("DecodeCertPolicy", 0); return 0; @@ -4802,7 +4779,6 @@ static int DecodeCertExtensions(DecodedCert* cert) break; case CERT_POLICY_OID: - WOLFSSL_MSG("Certificate Policy extension not supported yet."); #ifdef WOLFSSL_SEP #ifdef OPENSSL_EXTRA cert->extCertPolicySet = 1; @@ -4810,8 +4786,11 @@ static int DecodeCertExtensions(DecodedCert* cert) #endif #endif #if defined(WOLFSSL_SEP) || defined(WOLFSSL_CERT_EXT) - if (DecodeCertPolicy(&input[idx], length, cert) < 0) - return ASN_PARSE_E; + if (DecodeCertPolicy(&input[idx], length, cert) < 0) { + return ASN_PARSE_E; + } + #else + WOLFSSL_MSG("Certificate Policy extension not supported yet."); #endif break; From 9b79d8643e8ca821597aa56c1553c11e9c1a8361 Mon Sep 17 00:00:00 2001 From: David Garske Date: Mon, 7 Mar 2016 14:20:37 -0800 Subject: [PATCH 2/4] Added checks for total length and the cert policy OID len to make sure they don't exceed buffer. --- wolfcrypt/src/asn.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index 29ce32f32..45664d696 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -4608,6 +4608,12 @@ static int DecodePolicyOID(char *out, word32 outSz, byte *in, word32 inSz) WOLFSSL_MSG("\tGet CertPolicy total seq failed"); return ASN_PARSE_E; } + + /* Validate total length (2 is the CERT_POLICY_OID+SEQ) */ + if ((total_length + 2) != sz) { + WOLFSSL_MSG("\tCertPolicy length mismatch"); + return ASN_PARSE_E; + } /* Unwrap certificatePolicies */ do { @@ -4629,6 +4635,12 @@ static int DecodePolicyOID(char *out, word32 outSz, byte *in, word32 inSz) policy_length--; if (length > 0) { + /* Verify length won't overrun buffer */ + if (length > (sz - (int)idx)) { + WOLFSSL_MSG("\tCertPolicy length exceeds input buffer"); + return ASN_PARSE_E; + } + #if defined(WOLFSSL_SEP) cert->deviceType = (byte*)XMALLOC(length, cert->heap, DYNAMIC_TYPE_X509_EXT); From 05fb6487477790f1e3199191f723cfc4e580971e Mon Sep 17 00:00:00 2001 From: David Garske Date: Mon, 7 Mar 2016 14:33:22 -0800 Subject: [PATCH 3/4] Remove white-space. --- wolfcrypt/src/asn.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index 45664d696..879a8adb1 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -4608,7 +4608,7 @@ static int DecodePolicyOID(char *out, word32 outSz, byte *in, word32 inSz) WOLFSSL_MSG("\tGet CertPolicy total seq failed"); return ASN_PARSE_E; } - + /* Validate total length (2 is the CERT_POLICY_OID+SEQ) */ if ((total_length + 2) != sz) { WOLFSSL_MSG("\tCertPolicy length mismatch"); From b549c813377f15b70a61af05d31e32d8c39cdacc Mon Sep 17 00:00:00 2001 From: David Garske Date: Mon, 7 Mar 2016 14:49:24 -0800 Subject: [PATCH 4/4] Fix the WOLFSSL_SEP (--enable-sep) build scenario where extCertPoliciesNb is not available. --- wolfcrypt/src/asn.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index 879a8adb1..d63806f5b 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -4665,7 +4665,11 @@ static int DecodePolicyOID(char *out, word32 outSz, byte *in, word32 inSz) #endif } idx += policy_length; - } while((int)idx < total_length && cert->extCertPoliciesNb < MAX_CERTPOL_NB); + } while((int)idx < total_length + #if defined(WOLFSSL_CERT_EXT) + && cert->extCertPoliciesNb < MAX_CERTPOL_NB + #endif + ); WOLFSSL_LEAVE("DecodeCertPolicy", 0); return 0;