From 071852bbc9619342ce2409ce3241124526a3f0a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Milkovi=C4=8D?= Date: Wed, 9 Mar 2022 16:51:50 +0100 Subject: [PATCH 1/2] Updated authenticode parser to the newest version --- .../authenticode-parser/authenticode.h | 8 +++ deps/authenticode-parser/src/authenticode.c | 18 ++++--- deps/authenticode-parser/src/certificate.c | 36 ++++++++++++- .../src/countersignature.c | 50 +++++++++---------- deps/authenticode-parser/src/helper.c | 6 +++ src/fileformat/file_format/pe/pe_format.cpp | 1 + 6 files changed, 86 insertions(+), 33 deletions(-) diff --git a/deps/authenticode-parser/include/authenticode-parser/authenticode.h b/deps/authenticode-parser/include/authenticode-parser/authenticode.h index 98fbeede9..2a3841146 100644 --- a/deps/authenticode-parser/include/authenticode-parser/authenticode.h +++ b/deps/authenticode-parser/include/authenticode-parser/authenticode.h @@ -105,6 +105,7 @@ typedef struct { ByteArray sha256; /* SHA256 of the DER representation of the cert */ char* key_alg; /* Name of the key algorithm */ char* sig_alg; /* Name of the signature algorithm */ + char* sig_alg_oid; /* OID of the signature algorithm */ time_t not_before; /* NotBefore validity */ time_t not_after; /* NotAfter validity */ char* key; /* PEM encoded public key */ @@ -154,6 +155,13 @@ typedef struct { size_t count; } AuthenticodeArray; +/** + * @brief Initializes all globals OpenSSl objects we need for parsing, this is not thread-safe and + * needs to be called only once, before any multithreading environment + * https://github.com/openssl/openssl/issues/13524 + */ +void initialize_authenticode_parser(); + /** * @brief Constructs AuthenticodeArray from PE file data. Authenticode can * contains nested Authenticode signatures as its unsigned attribute, diff --git a/deps/authenticode-parser/src/authenticode.c b/deps/authenticode-parser/src/authenticode.c index 724b64f9a..1a857da80 100644 --- a/deps/authenticode-parser/src/authenticode.c +++ b/deps/authenticode-parser/src/authenticode.c @@ -273,8 +273,9 @@ static bool authenticode_verify(PKCS7* p7, PKCS7_SIGNER_INFO* si, X509* signCert return isValid; } -/* Creates all the Authenticode objects so we can parse them with OpenSSL */ -static void initialize_openssl() +/* Creates all the Authenticode objects so we can parse them with OpenSSL, is not thread-safe, needs + * to be called once before any multi-threading environmentt - https://github.com/openssl/openssl/issues/13524 */ +void initialize_authenticode_parser() { OBJ_create("1.3.6.1.4.1.311.2.1.12", "spcSpOpusInfo", "SPC_SP_OPUS_INFO_OBJID"); OBJ_create("1.3.6.1.4.1.311.3.3.1", "spcMsCountersignature", "SPC_MICROSOFT_COUNTERSIGNATURE"); @@ -289,9 +290,6 @@ AuthenticodeArray* authenticode_new(const uint8_t* data, long len) if (!data || len == 0) return NULL; - /* We need to initialize all the custom objects for further parsing */ - initialize_openssl(); - AuthenticodeArray* result = (AuthenticodeArray*)calloc(1, sizeof(*result)); if (!result) return NULL; @@ -531,6 +529,11 @@ AuthenticodeArray* parse_authenticode(const uint8_t* pe_data, long pe_len) if (pe_len < dos_hdr_size) return NULL; + /* Check if it has DOS signature, so we don't parse random gibberish */ + uint8_t dos_prefix[] = {0x4d, 0x5a}; + if (memcmp(pe_data, dos_prefix, sizeof(dos_prefix)) != 0) + return NULL; + /* offset to pointer in DOS header, that points to PE header */ const int pe_hdr_ptr_offset = 0x3c; /* Read the PE offset */ @@ -553,8 +556,9 @@ AuthenticodeArray* parse_authenticode(const uint8_t* pe_data, long pe_len) if (pe_len < pe_cert_table_addr + 2 * sizeof(uint32_t)) return NULL; - uint32_t cert_addr = letoh32(*(uint32_t*)(pe_data + pe_cert_table_addr)); - uint32_t cert_len = letoh32(*(uint32_t*)(pe_data + pe_cert_table_addr + 4)); + /* Use 64bit type due to the potential overflow in crafted binaries */ + uint64_t cert_addr = letoh32(*(uint32_t*)(pe_data + pe_cert_table_addr)); + uint64_t cert_len = letoh32(*(uint32_t*)(pe_data + pe_cert_table_addr + 4)); /* we need atleast 8 bytes to read dwLength, revision and certType */ if (cert_len < 8 || pe_len < cert_addr + cert_len) diff --git a/deps/authenticode-parser/src/certificate.c b/deps/authenticode-parser/src/certificate.c index 2f1a4f0ca..337b92e87 100644 --- a/deps/authenticode-parser/src/certificate.c +++ b/deps/authenticode-parser/src/certificate.c @@ -30,6 +30,27 @@ SOFTWARE. #include "helper.h" +#if OPENSSL_VERSION_NUMBER >= 0x3000000fL +/* Removes any escaping \/ -> / that is happening with oneline() functions + from OpenSSL 3.0 */ +static void parse_oneline_string(char* string) +{ + size_t len = strlen(string); + char* tmp = string; + while (true) { + char* ptr = strstr(tmp, "\\/"); + if (!ptr) + break; + + memmove(ptr, ptr + 1, strlen(ptr + 1)); + tmp = ptr + 1; + len--; + } + + string[len] = 0; +} +#endif + static void parse_name_attributes(X509_NAME* raw, Attributes* attr) { if (!raw || !attr) @@ -269,11 +290,19 @@ Certificate* certificate_new(X509* x509) * but we want to comply with existing YARA code */ X509_NAME* issuerName = X509_get_issuer_name(x509); X509_NAME_oneline(issuerName, buffer, sizeof(buffer)); + result->issuer = strdup(buffer); + /* This is a little ugly hack for 3.0 compatibility */ +#if OPENSSL_VERSION_NUMBER >= 0x3000000fL + parse_oneline_string(result->issuer); +#endif X509_NAME* subjectName = X509_get_subject_name(x509); X509_NAME_oneline(subjectName, buffer, sizeof(buffer)); result->subject = strdup(buffer); +#if OPENSSL_VERSION_NUMBER >= 0x3000000fL + parse_oneline_string(result->subject); +#endif parse_name_attributes(issuerName, &result->issuer_attrs); parse_name_attributes(subjectName, &result->subject_attrs); @@ -282,7 +311,11 @@ Certificate* certificate_new(X509* x509) result->serial = integer_to_serial(X509_get_serialNumber(x509)); result->not_after = ASN1_TIME_to_time_t(X509_get0_notAfter(x509)); result->not_before = ASN1_TIME_to_time_t(X509_get0_notBefore(x509)); - result->sig_alg = strdup(OBJ_nid2ln(X509_get_signature_nid(x509))); + int sig_nid = X509_get_signature_nid(x509); + result->sig_alg = strdup(OBJ_nid2ln(sig_nid)); + + OBJ_obj2txt(buffer, sizeof(buffer), OBJ_nid2obj(sig_nid), 1); + result->sig_alg_oid = strdup(buffer); EVP_PKEY* pkey = X509_get0_pubkey(x509); if (pkey) { @@ -364,6 +397,7 @@ void certificate_free(Certificate* cert) free(cert->issuer); free(cert->subject); free(cert->sig_alg); + free(cert->sig_alg_oid); free(cert->key_alg); free(cert->key); free(cert->sha1.data); diff --git a/deps/authenticode-parser/src/countersignature.c b/deps/authenticode-parser/src/countersignature.c index 59ca8038a..36d0c12ab 100644 --- a/deps/authenticode-parser/src/countersignature.c +++ b/deps/authenticode-parser/src/countersignature.c @@ -52,25 +52,26 @@ Countersignature* pkcs9_countersig_new( int digestnid = OBJ_obj2nid(si->digest_alg->algorithm); result->digest_alg = strdup(OBJ_nid2ln(digestnid)); - /* Get digest that corresponds to decrypted encrypted digest in signature */ - ASN1_TYPE* messageDigest = PKCS7_get_signed_attribute(si, NID_pkcs9_messageDigest); - const ASN1_TYPE* sign_time = PKCS7_get_signed_attribute(si, NID_pkcs9_signingTime); - result->sign_time = ASN1_TIME_to_time_t(sign_time->value.utctime); - - X509* signCert = X509_find_by_issuer_and_serial( - certs, si->issuer_and_serial->issuer, si->issuer_and_serial->serial); - /* PKCS9 stores certificates in the corresponding PKCS7 it countersigns */ - result->chain = parse_signer_chain(signCert, certs); - if (!sign_time) { result->verify_flags = COUNTERSIGNATURE_VFY_TIME_MISSING; goto end; } + + result->sign_time = ASN1_TIME_to_time_t(sign_time->value.utctime); + + X509* signCert = X509_find_by_issuer_and_serial( + certs, si->issuer_and_serial->issuer, si->issuer_and_serial->serial); if (!signCert) { result->verify_flags = COUNTERSIGNATURE_VFY_NO_SIGNER_CERT; goto end; } + + /* PKCS9 stores certificates in the corresponding PKCS7 it countersigns */ + result->chain = parse_signer_chain(signCert, certs); + + /* Get digest that corresponds to decrypted encrypted digest in signature */ + ASN1_TYPE* messageDigest = PKCS7_get_signed_attribute(si, NID_pkcs9_messageDigest); if (!messageDigest) { result->verify_flags = COUNTERSIGNATURE_VFY_DIGEST_MISSING; goto end; @@ -196,23 +197,24 @@ Countersignature* ms_countersig_new(const uint8_t* data, long size, ASN1_STRING* } const ASN1_TIME* rawTime = TS_TST_INFO_get_time(ts); - result->sign_time = ASN1_TIME_to_time_t(rawTime); - - STACK_OF(X509)* sigs = PKCS7_get0_signers(p7, p7->d.sign->cert, 0); - X509* signCert = sk_X509_value(sigs, 0); - result->chain = parse_signer_chain(signCert, p7->d.sign->cert); - - /* Imprint == digest */ - TS_MSG_IMPRINT* imprint = TS_TST_INFO_get_msg_imprint(ts); - if (!rawTime) { result->verify_flags = COUNTERSIGNATURE_VFY_TIME_MISSING; goto end; } + + result->sign_time = ASN1_TIME_to_time_t(rawTime); + + STACK_OF(X509)* sigs = PKCS7_get0_signers(p7, p7->d.sign->cert, 0); + X509* signCert = sk_X509_value(sigs, 0); if (!signCert) { result->verify_flags = COUNTERSIGNATURE_VFY_NO_SIGNER_CERT; goto end; } + + result->chain = parse_signer_chain(signCert, p7->d.sign->cert); + + /* Imprint == digest */ + TS_MSG_IMPRINT* imprint = TS_TST_INFO_get_msg_imprint(ts); if (!imprint) { result->verify_flags = COUNTERSIGNATURE_VFY_DIGEST_MISSING; goto end; @@ -242,6 +244,7 @@ Countersignature* ms_countersig_new(const uint8_t* data, long size, ASN1_STRING* uint8_t calc_digest[EVP_MAX_MD_SIZE]; calculate_digest(md, enc_digest->data, enc_digest->length, calc_digest); + #if OPENSSL_VERSION_NUMBER >= 0x3000000fL int mdLen = EVP_MD_get_size(md); #else @@ -260,7 +263,7 @@ Countersignature* ms_countersig_new(const uint8_t* data, long size, ASN1_STRING* TS_VERIFY_CTX_set_flags(ctx, TS_VFY_VERSION | TS_VFY_IMPRINT); TS_VERIFY_CTX_set_store(ctx, store); #if OPENSSL_VERSION_NUMBER >= 0x3000000fL - TS_VERIFY_CTX_set_store(ctx, p7->d.sign->cert); + TS_VERIFY_CTX_set_certs(ctx, p7->d.sign->cert); #else TS_VERIFY_CTS_set_certs(ctx, p7->d.sign->cert); #endif @@ -268,11 +271,8 @@ Countersignature* ms_countersig_new(const uint8_t* data, long size, ASN1_STRING* bool isValid = TS_RESP_verify_token(ctx, p7) == 1; - /* VERIFY_CTX_free tries to free these, we don't want that */ - TS_VERIFY_CTX_set_imprint(ctx, NULL, 0); - TS_VERIFY_CTS_set_certs(ctx, NULL); - - TS_VERIFY_CTX_free(ctx); + X509_STORE_free(store); + OPENSSL_free(ctx); if (!isValid) { result->verify_flags = COUNTERSIGNATURE_VFY_INVALID; diff --git a/deps/authenticode-parser/src/helper.c b/deps/authenticode-parser/src/helper.c index be846b827..f65a39179 100644 --- a/deps/authenticode-parser/src/helper.c +++ b/deps/authenticode-parser/src/helper.c @@ -58,6 +58,12 @@ int calculate_digest(const EVP_MD* md, const uint8_t* data, size_t len, uint8_t* int byte_array_init(ByteArray* arr, const uint8_t* data, int len) { + if (len == 0) { + arr->data = NULL; + arr->len = 0; + return 0; + } + arr->data = (uint8_t*)malloc(len); if (!arr->data) return -1; diff --git a/src/fileformat/file_format/pe/pe_format.cpp b/src/fileformat/file_format/pe/pe_format.cpp index 7ed7598d1..470b8b6d7 100644 --- a/src/fileformat/file_format/pe/pe_format.cpp +++ b/src/fileformat/file_format/pe/pe_format.cpp @@ -2030,6 +2030,7 @@ void PeFormat::loadCertificates() std::vector sections = getSections(); + initialize_authenticode_parser(); AuthenticodeArray* auth = parse_authenticode(this->getBytesData(), this->getFileLength()); std::vector sigs = authenticodeToSignatures(auth, this); authenticode_array_free(auth); From 4a6291b64cb37d5d791b3437ff5477b88b797813 Mon Sep 17 00:00:00 2001 From: houndthe Date: Wed, 9 Mar 2022 20:27:06 +0100 Subject: [PATCH 2/2] Fix uninitialize free, use finer sanity checks in auth. parser --- deps/authenticode-parser/src/authenticode.c | 6 +++--- deps/authenticode-parser/src/countersignature.c | 15 +++++++++------ 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/deps/authenticode-parser/src/authenticode.c b/deps/authenticode-parser/src/authenticode.c index 1a857da80..65141228c 100644 --- a/deps/authenticode-parser/src/authenticode.c +++ b/deps/authenticode-parser/src/authenticode.c @@ -561,14 +561,14 @@ AuthenticodeArray* parse_authenticode(const uint8_t* pe_data, long pe_len) uint64_t cert_len = letoh32(*(uint32_t*)(pe_data + pe_cert_table_addr + 4)); /* we need atleast 8 bytes to read dwLength, revision and certType */ - if (cert_len < 8 || pe_len < cert_addr + cert_len) + if (cert_len < 8 || pe_len < cert_addr + 8) return NULL; uint32_t dwLength = letoh32(*(uint32_t*)(pe_data + cert_addr)); if (pe_len < cert_addr + dwLength) return NULL; - - AuthenticodeArray* auth_array = authenticode_new(pe_data + cert_addr + 0x8, dwLength); + /* dwLength = offsetof(WIN_CERTIFICATE, bCertificate) + (size of the variable-length binary array contained within bCertificate) */ + AuthenticodeArray* auth_array = authenticode_new(pe_data + cert_addr + 0x8, dwLength - 0x8); if (!auth_array) return NULL; diff --git a/deps/authenticode-parser/src/countersignature.c b/deps/authenticode-parser/src/countersignature.c index 36d0c12ab..d905e239b 100644 --- a/deps/authenticode-parser/src/countersignature.c +++ b/deps/authenticode-parser/src/countersignature.c @@ -133,16 +133,17 @@ Countersignature* pkcs9_countersig_new( /* compare the encrypted digest and calculated digest */ bool isValid = false; - /* Sometimes signed data contains DER encoded DigestInfo structure which contains hash of - * authenticated attributes (39c9d136f026a9ad18fb9f41a64f76dd8418e8de625dce5d3a372bd242fc5edd) - * but other times it is just purely and I didn't find another way to distinguish it but only - * based on the length of data we get. Found mention of this in openssl mailing list: - * https://mta.openssl.org/pipermail/openssl-users/2015-September/002054.html */ + #if OPENSSL_VERSION_NUMBER >= 0x3000000fL size_t mdLen = EVP_MD_get_size(md); #else size_t mdLen = EVP_MD_size(md); #endif + /* Sometimes signed data contains DER encoded DigestInfo structure which contains hash of + * authenticated attributes (39c9d136f026a9ad18fb9f41a64f76dd8418e8de625dce5d3a372bd242fc5edd) + * but other times it is just purely and I didn't find another way to distinguish it but only + * based on the length of data we get. Found mention of this in openssl mailing list: + * https://mta.openssl.org/pipermail/openssl-users/2015-September/002054.html */ if (mdLen == decLen) { isValid = !memcmp(calc_digest, decData, mdLen); } else { @@ -199,7 +200,9 @@ Countersignature* ms_countersig_new(const uint8_t* data, long size, ASN1_STRING* const ASN1_TIME* rawTime = TS_TST_INFO_get_time(ts); if (!rawTime) { result->verify_flags = COUNTERSIGNATURE_VFY_TIME_MISSING; - goto end; + TS_TST_INFO_free(ts); + PKCS7_free(p7); + return result; } result->sign_time = ASN1_TIME_to_time_t(rawTime);