From dfe4eea31e725010336894be5c7bf21fa3764166 Mon Sep 17 00:00:00 2001 From: Julius Pfrommer Date: Mon, 30 Sep 2024 00:27:51 +0200 Subject: [PATCH 1/3] fix(core): Fix type access when comparing ExtensionObjects --- src/ua_types.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ua_types.c b/src/ua_types.c index c2c14cccac0..2fff32e18bb 100644 --- a/src/ua_types.c +++ b/src/ua_types.c @@ -1436,7 +1436,7 @@ extensionObjectOrder(const UA_ExtensionObject *p1, const UA_ExtensionObject *p2, case UA_EXTENSIONOBJECT_DECODED: default: { const UA_DataType *type1 = p1->content.decoded.type; - const UA_DataType *type2 = p1->content.decoded.type; + const UA_DataType *type2 = p2->content.decoded.type; if(type1 != type2) return ((uintptr_t)type1 < (uintptr_t)type2) ? UA_ORDER_LESS : UA_ORDER_MORE; if(!type1) From f9d4c1631032f1d1232cb75ab38d74081e87882f Mon Sep 17 00:00:00 2001 From: Julius Pfrommer Date: Fri, 27 Sep 2024 22:54:10 +0200 Subject: [PATCH 2/3] fix(plugins): More thorough validation that the issuer is a valid CA --- plugins/crypto/mbedtls/ua_pki_mbedtls.c | 94 +++++++++++++++++-------- plugins/crypto/openssl/ua_pki_openssl.c | 28 +++++--- 2 files changed, 83 insertions(+), 39 deletions(-) diff --git a/plugins/crypto/mbedtls/ua_pki_mbedtls.c b/plugins/crypto/mbedtls/ua_pki_mbedtls.c index ac3bf38814e..2abc1bf4323 100644 --- a/plugins/crypto/mbedtls/ua_pki_mbedtls.c +++ b/plugins/crypto/mbedtls/ua_pki_mbedtls.c @@ -13,6 +13,7 @@ #ifdef UA_ENABLE_ENCRYPTION_MBEDTLS #include +#include #include #include #include @@ -232,31 +233,63 @@ reloadCertificates(CertInfo *ci) { #endif +#define UA_MBEDTLS_MAX_CHAIN_LENGTH 10 +#define UA_MBEDTLS_MAX_DN_LENGTH 256 + /* We need to access some private fields below */ #ifndef MBEDTLS_PRIVATE #define MBEDTLS_PRIVATE(x) x #endif -/* Return the first matching issuer candidate AFTER prev */ +/* Is the certificate a CA? */ +static UA_Boolean +mbedtlsCheckCA(mbedtls_x509_crt *cert) { + /* The Basic Constraints extension must be set and the cert acts as CA */ + if(!(cert->MBEDTLS_PRIVATE(ext_types) & MBEDTLS_X509_EXT_BASIC_CONSTRAINTS) || + !cert->MBEDTLS_PRIVATE(ca_istrue)) + return false; + + /* The Key Usage extension must be set to cert signing and CRL issuing */ + if(!(cert->MBEDTLS_PRIVATE(ext_types) & MBEDTLS_X509_EXT_KEY_USAGE) || + mbedtls_x509_crt_check_key_usage(cert, MBEDTLS_X509_KU_KEY_CERT_SIGN) != 0 || + mbedtls_x509_crt_check_key_usage(cert, MBEDTLS_X509_KU_CRL_SIGN) != 0) + return false; + + return true; +} + +static UA_Boolean +mbedtlsSameName(UA_String name, const mbedtls_x509_name *name2) { + char buf[UA_MBEDTLS_MAX_DN_LENGTH]; + int len = mbedtls_x509_dn_gets(buf, UA_MBEDTLS_MAX_DN_LENGTH, name2); + if(len < 0) + return false; + UA_String nameString = {(size_t)len, (UA_Byte*)buf}; + return UA_String_equal(&name, &nameString); +} + +/* Return the first matching issuer candidate AFTER prev. + * This can return the cert itself if self-signed. */ static mbedtls_x509_crt * mbedtlsFindNextIssuer(CertInfo *ci, mbedtls_x509_crt *stack, mbedtls_x509_crt *cert, mbedtls_x509_crt *prev) { - char inbuf[512], snbuf[512]; - if(mbedtls_x509_dn_gets(inbuf, 512, &cert->issuer) < 0) + char inbuf[UA_MBEDTLS_MAX_DN_LENGTH]; + int nameLen = mbedtls_x509_dn_gets(inbuf, UA_MBEDTLS_MAX_DN_LENGTH, &cert->issuer); + if(nameLen < 0) return NULL; + UA_String issuerName = {(size_t)nameLen, (UA_Byte*)inbuf}; do { for(mbedtls_x509_crt *i = stack; i; i = i->next) { - /* Compare issuer name and subject name */ - if(mbedtls_x509_dn_gets(snbuf, 512, &i->subject) < 0) - continue; - if(strncmp(inbuf, snbuf, 512) != 0) - continue; - /* Skip when the key does not match the signature */ - if(!mbedtls_pk_can_do(&i->pk, cert->MBEDTLS_PRIVATE(sig_pk))) + if(prev) { + if(prev == i) + prev = NULL; /* This was the last issuer we tried to verify */ continue; - if(prev == NULL) + } + /* Compare issuer name and subject name. + * Skip when the key does not match the signature. */ + if(mbedtlsSameName(issuerName, &i->subject) && + mbedtls_pk_can_do(&i->pk, cert->MBEDTLS_PRIVATE(sig_pk))) return i; - prev = NULL; /* This was the last issuer we tried to verify */ } /* Switch from the stack that came with the cert to the ctx->skIssue list */ stack = (stack != &ci->certificateIssuerList) ? &ci->certificateIssuerList : NULL; @@ -266,17 +299,16 @@ mbedtlsFindNextIssuer(CertInfo *ci, mbedtls_x509_crt *stack, static UA_Boolean mbedtlsCheckRevoked(CertInfo *ci, mbedtls_x509_crt *cert) { - char inbuf[512], inbuf2[512]; - if(mbedtls_x509_dn_gets(inbuf, 512, &cert->issuer) < 0) + char inbuf[UA_MBEDTLS_MAX_DN_LENGTH]; + int nameLen = mbedtls_x509_dn_gets(inbuf, UA_MBEDTLS_MAX_DN_LENGTH, &cert->issuer); + if(nameLen < 0) return true; + UA_String issuerName = {(size_t)nameLen, (UA_Byte*)inbuf}; for(mbedtls_x509_crl *crl = &ci->certificateRevocationList; crl; crl = crl->next) { - /* Is the CRL for the issuer of the certificate? */ - if(mbedtls_x509_dn_gets(inbuf2, 512, &crl->issuer) < 0) - return true; - if(strncmp(inbuf, inbuf2, 512) != 0) - continue; - /* Is the serial number of the certificate contained in the CRL? */ - if(mbedtls_x509_crt_is_revoked(cert, crl) != 0) + /* Is the CRL for certificates from the cert issuer? + * Is the serial number of the certificate contained in the CRL? */ + if(mbedtlsSameName(issuerName, &crl->issuer) && + mbedtls_x509_crt_is_revoked(cert, crl) != 0) return true; } return false; @@ -305,8 +337,6 @@ mbedtlsCheckSignature(const mbedtls_x509_crt *cert, mbedtls_x509_crt *issuer) { hash, hash_len, sig->p, sig->len) == 0); } -#define UA_MBEDTLS_MAX_CHAIN_LENGTH 10 - static UA_StatusCode mbedtlsVerifyChain(CertInfo *ci, mbedtls_x509_crt *stack, mbedtls_x509_crt **old_issuers, mbedtls_x509_crt *cert, int depth) { @@ -337,6 +367,13 @@ mbedtlsVerifyChain(CertInfo *ci, mbedtls_x509_crt *stack, mbedtls_x509_crt **old if(!issuer) break; + /* Verification Step: Certificate Usage + * Can the issuer act as CA? Omit for self-signed leaf certificates. */ + if((depth > 0 || issuer != cert) && !mbedtlsCheckCA(issuer)) { + ret = UA_STATUSCODE_BADCERTIFICATEISSUERUSENOTALLOWED; + continue; + } + /* Verification Step: Signature */ if(!mbedtlsCheckSignature(cert, issuer)) { ret = UA_STATUSCODE_BADCERTIFICATEINVALID; /* Wrong issuer, try again */ @@ -346,7 +383,7 @@ mbedtlsVerifyChain(CertInfo *ci, mbedtls_x509_crt *stack, mbedtls_x509_crt **old /* The certificate is self-signed. We have arrived at the top of the * chain. We check whether the certificate is trusted below. This is the * only place where we return UA_STATUSCODE_BADCERTIFICATEUNTRUSTED. - * This sinals that the chain is complete (but can be still + * This signals that the chain is complete (but can be still * untrusted). */ if(issuer == cert || (cert->tbs.len == issuer->tbs.len && memcmp(cert->tbs.p, issuer->tbs.p, cert->tbs.len) == 0)) { @@ -408,12 +445,9 @@ certificateVerification_verify(void *verificationContext, /* Verification Step: Certificate Usage * Check whether the certificate is a User certificate or a CA certificate. - * If the KU_KEY_CERT_SIGN and KU_CRL_SIGN of key_usage are set, then the - * certificate shall be condidered as CA Certificate and cannot be used to - * establish a connection. Refer the test case CTT/Security/Security - * Certificate Validation/029.js for more details */ - unsigned int ca_flags = MBEDTLS_X509_KU_KEY_CERT_SIGN | MBEDTLS_X509_KU_CRL_SIGN; - if(mbedtls_x509_crt_check_key_usage(&cert, ca_flags)) { + * Refer the test case CTT/Security/Security Certificate Validation/029.js + * for more details. */ + if(mbedtlsCheckCA(&cert)) { mbedtls_x509_crt_free(&cert); return UA_STATUSCODE_BADCERTIFICATEUSENOTALLOWED; } diff --git a/plugins/crypto/openssl/ua_pki_openssl.c b/plugins/crypto/openssl/ua_pki_openssl.c index 3c05aaa2777..d7bb1297e5d 100644 --- a/plugins/crypto/openssl/ua_pki_openssl.c +++ b/plugins/crypto/openssl/ua_pki_openssl.c @@ -454,11 +454,16 @@ openSSLFindNextIssuer(CertContext *ctx, STACK_OF(X509) *stack, X509 *x509, X509 int size = sk_X509_num(stack); for(int i = 0; i < size; i++) { X509 *candidate = sk_X509_value(stack, i); - if(X509_check_issued(candidate, x509) != 0) + if(prev) { + if(prev == candidate) + prev = NULL; /* This was the last issuer we tried to verify */ continue; - if(prev == NULL) + } + /* This checks subject/issuer name and the key usage of the issuer. + * It does not verify the validity period and if the issuer key was + * used for the signature. We check that afterwards. */ + if(X509_check_issued(candidate, x509) == 0) return candidate; - prev = NULL; } /* Switch to search in the ctx->skIssue list */ stack = (stack != ctx->skIssue) ? ctx->skIssue : NULL; @@ -519,6 +524,13 @@ openSSL_verifyChain(CertContext *ctx, STACK_OF(X509) *stack, X509 **old_issuers, if(!issuer) break; + /* Verification Step: Certificate Usage + * Can the issuer act as CA? Omit for self-signed leaf certificates. */ + if((depth > 0 || issuer != cert) && !X509_check_ca(issuer)) { + ret = UA_STATUSCODE_BADCERTIFICATEISSUERUSENOTALLOWED; + continue; + } + /* Verification Step: Signature */ int opensslRet = X509_verify(cert, X509_get0_pubkey(issuer)); if(opensslRet == -1) { @@ -531,7 +543,7 @@ openSSL_verifyChain(CertContext *ctx, STACK_OF(X509) *stack, X509 **old_issuers, /* The certificate is self-signed. We have arrived at the top of the * chain. We check whether the certificate is trusted below. This is the * only place where we return UA_STATUSCODE_BADCERTIFICATEUNTRUSTED. - * This sinals that the chain is complete (but can be still + * This signals that the chain is complete (but can be still * untrusted). */ if(cert == issuer || X509_cmp(cert, issuer) == 0) { ret = UA_STATUSCODE_BADCERTIFICATEUNTRUSTED; @@ -589,12 +601,10 @@ UA_CertificateVerification_Verify(void *verificationContext, /* Verification Step: Certificate Usage * Check whether the certificate is a User certificate or a CA certificate. - * If the KU_KEY_CERT_SIGN and KU_CRL_SIGN of key_usage are set, then the - * certificate shall be condidered as CA Certificate and cannot be used to - * establish a connection. Refer the test case CTT/Security/Security - * Certificate Validation/029.js for more details */ + * Refer the test case CTT/Security/Security Certificate Validation/029.js + * for more details. */ X509 *leaf = sk_X509_value(stack, 0); - if(X509_check_purpose(leaf, X509_PURPOSE_CRL_SIGN, 0) && X509_check_ca(leaf)) { + if(X509_check_ca(leaf)) { sk_X509_pop_free(stack, X509_free); return UA_STATUSCODE_BADCERTIFICATEUSENOTALLOWED; } From 47336a788068960d8171a29ded8698ee81ba5703 Mon Sep 17 00:00:00 2001 From: Julius Pfrommer Date: Wed, 2 Oct 2024 21:47:05 +0200 Subject: [PATCH 3/3] refactor(build): Bump version to v1.3.13 --- CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index fbced8259ea..3ff203aeee4 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -43,7 +43,7 @@ set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin) # overwritten with more detailed information if git is available. set(OPEN62541_VER_MAJOR 1) set(OPEN62541_VER_MINOR 3) -set(OPEN62541_VER_PATCH 12) +set(OPEN62541_VER_PATCH 13) set(OPEN62541_VER_LABEL "-undefined") # like "-rc1" or "-g4538abcd" or "-g4538abcd-dirty" set(OPEN62541_VER_COMMIT "unknown-commit")