From: Nathan Moinvaziri Date: Tue, 8 Aug 2023 20:12:19 +0000 (-0700) Subject: schannel: fix ordering of cert chain info X-Git-Tag: curl-8_3_0~17 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=f6700c744b2b11e6d2011705e227d5f89863c789;p=thirdparty%2Fcurl.git schannel: fix ordering of cert chain info - Use CERT_CONTEXT's pbCertEncoded to determine chain order. CERT_CONTEXT from SECPKG_ATTR_REMOTE_CERT_CONTEXT contains end-entity/server certificate in pbCertEncoded. We can use this pointer to determine the order of certificates when enumerating hCertStore using CertEnumCertificatesInStore. This change is to help ensure that the ordering of the certificate chain requested by the user via CURLINFO_CERTINFO has the same ordering on all versions of Windows. Prior to this change Schannel certificate order was reversed in 8986df80 but that was later reverted in f540a39b when it was discovered that Windows 11 22H2 does the reversal on its own. Ref: https://github.com/curl/curl/issues/9706 Closes https://github.com/curl/curl/pull/11632 --- diff --git a/lib/vtls/schannel.c b/lib/vtls/schannel.c index 4f8b6f96dc..f6a5d441a9 100644 --- a/lib/vtls/schannel.c +++ b/lib/vtls/schannel.c @@ -1656,7 +1656,8 @@ valid_cert_encoding(const CERT_CONTEXT *cert_context) (cert_context->cbCertEncoded > 0); } -typedef bool(*Read_crt_func)(const CERT_CONTEXT *ccert_context, void *arg); +typedef bool(*Read_crt_func)(const CERT_CONTEXT *ccert_context, + bool reverse_order, void *arg); static void traverse_cert_store(const CERT_CONTEXT *context, Read_crt_func func, @@ -1664,19 +1665,32 @@ traverse_cert_store(const CERT_CONTEXT *context, Read_crt_func func, { const CERT_CONTEXT *current_context = NULL; bool should_continue = true; + bool first = true; + bool reverse_order = false; while(should_continue && (current_context = CertEnumCertificatesInStore( context->hCertStore, - current_context)) != NULL) - should_continue = func(current_context, arg); + current_context)) != NULL) { + /* Windows 11 22H2 OS Build 22621.674 or higher enumerates certificates in + leaf-to-root order while all previous versions of Windows enumerate + certificates in root-to-leaf order. Determine the order of enumeration + by comparing SECPKG_ATTR_REMOTE_CERT_CONTEXT's pbCertContext with the + first certificate's pbCertContext. */ + if(first && context->pbCertEncoded != current_context->pbCertEncoded) + reverse_order = true; + should_continue = func(current_context, reverse_order, arg); + first = false; + } if(current_context) CertFreeCertificateContext(current_context); } static bool -cert_counter_callback(const CERT_CONTEXT *ccert_context, void *certs_count) +cert_counter_callback(const CERT_CONTEXT *ccert_context, bool reverse_order, + void *certs_count) { + (void)reverse_order; /* unused */ if(valid_cert_encoding(ccert_context)) (*(int *)certs_count)++; return true; @@ -1687,17 +1701,23 @@ struct Adder_args struct Curl_easy *data; CURLcode result; int idx; + int certs_count; }; static bool -add_cert_to_certinfo(const CERT_CONTEXT *ccert_context, void *raw_arg) +add_cert_to_certinfo(const CERT_CONTEXT *ccert_context, bool reverse_order, + void *raw_arg) { struct Adder_args *args = (struct Adder_args*)raw_arg; args->result = CURLE_OK; if(valid_cert_encoding(ccert_context)) { const char *beg = (const char *) ccert_context->pbCertEncoded; const char *end = beg + ccert_context->cbCertEncoded; - args->result = Curl_extract_certinfo(args->data, (args->idx)++, beg, end); + int insert_index = reverse_order ? (args->certs_count - 1) - args->idx : + args->idx; + args->result = Curl_extract_certinfo(args->data, insert_index, + beg, end); + args->idx++; } return args->result == CURLE_OK; } @@ -1831,6 +1851,7 @@ schannel_connect_step3(struct Curl_cfilter *cf, struct Curl_easy *data) struct Adder_args args; args.data = data; args.idx = 0; + args.certs_count = certs_count; traverse_cert_store(ccert_context, add_cert_to_certinfo, &args); result = args.result; } diff --git a/tests/data/Makefile.inc b/tests/data/Makefile.inc index 49c678f844..2ec4936ec1 100644 --- a/tests/data/Makefile.inc +++ b/tests/data/Makefile.inc @@ -256,6 +256,6 @@ test3008 test3009 test3010 test3011 test3012 test3013 test3014 test3015 \ test3016 test3017 test3018 test3019 test3020 test3021 test3022 test3023 \ test3024 test3025 test3026 test3027 test3028 test3029 test3030 \ \ -test3100 test3101 \ +test3100 test3101 test3102 \ test3200 \ test3201 test3202 diff --git a/tests/data/test3102 b/tests/data/test3102 new file mode 100644 index 0000000000..5d9bcf3854 --- /dev/null +++ b/tests/data/test3102 @@ -0,0 +1,51 @@ + + + +HTTPS +HTTP GET + + + +# +# Server-side + + + + + +# +# Client-side + +# SSL with libraries supporting CURLOPT_CERTINFO + +SSL +!bearssl +!mbedtls +!rustls +!wolfssl + + +https + + +lib%TESTNUMBER + + +verify certificate chain order with simple HTTPS GET + + +https://%HOSTIP:%HTTPSPORT/%TESTNUMBER + + + +# +# Verify data after the test has been "shot" + + +GET /%TESTNUMBER HTTP/1.1 +Host: %HOSTIP:%HTTPSPORT +Accept: */* + + + + diff --git a/tests/libtest/Makefile.inc b/tests/libtest/Makefile.inc index f13e017b07..df44f1be1b 100644 --- a/tests/libtest/Makefile.inc +++ b/tests/libtest/Makefile.inc @@ -75,7 +75,7 @@ noinst_PROGRAMS = chkhostname libauthretry libntlmconnect libprereq \ lib2402 lib2404 \ lib2502 \ lib3010 lib3025 lib3026 lib3027 \ - lib3100 lib3101 + lib3100 lib3101 lib3102 chkhostname_SOURCES = chkhostname.c ../../lib/curl_gethostname.c chkhostname_LDADD = @CURL_NETWORK_LIBS@ @@ -686,3 +686,6 @@ lib3100_LDADD = $(TESTUTIL_LIBS) lib3101_SOURCES = lib3101.c $(SUPPORTFILES) $(TESTUTIL) $(WARNLESS) lib3101_LDADD = $(TESTUTIL_LIBS) + +lib3102_SOURCES = lib3102.c $(SUPPORTFILES) $(TESTUTIL) $(WARNLESS) +lib3102_LDADD = $(TESTUTIL_LIBS) diff --git a/tests/libtest/lib3102.c b/tests/libtest/lib3102.c new file mode 100644 index 0000000000..abc0a27eab --- /dev/null +++ b/tests/libtest/lib3102.c @@ -0,0 +1,141 @@ +/*************************************************************************** + * _ _ ____ _ + * Project ___| | | | _ \| | + * / __| | | | |_) | | + * | (__| |_| | _ <| |___ + * \___|\___/|_| \_\_____| + * + * Copyright (C) Daniel Stenberg, , et al. + * + * This software is licensed as described in the file COPYING, which + * you should have received as part of this distribution. The terms + * are also available at https://curl.se/docs/copyright.html. + * + * You may opt to use, copy, modify, merge, publish, distribute and/or sell + * copies of the Software, and permit persons to whom the Software is + * furnished to do so, under the terms of the COPYING file. + * + * This software is distributed on an "AS IS" basis, WITHOUT WARRANTY OF ANY + * KIND, either express or implied. + * + * SPDX-License-Identifier: curl + * + ***************************************************************************/ +#include "test.h" + +#include "memdebug.h" + +/* + * Verify correct order of certificates in the chain by comparing the + * subject and issuer attributes of each certificate. + */ +static bool is_chain_in_order(struct curl_certinfo *cert_info) +{ + char *last_issuer = NULL; + int cert; + + /* Chains with only a single certificate are always in order */ + if(cert_info->num_of_certs <= 1) + return 1; + + /* Enumerate each certificate in the chain */ + for(cert = 0; cert < cert_info->num_of_certs; cert++) { + struct curl_slist *slist = cert_info->certinfo[cert]; + char *issuer = NULL; + char *subject = NULL; + + /* Find the certificate issuer and subject by enumerating each field */ + for(; slist && (!issuer || !subject); slist = slist->next) { + const char issuer_prefix[] = "Issuer:"; + const char subject_prefix[] = "Subject:"; + + if(!strncmp(slist->data, issuer_prefix, sizeof(issuer_prefix)-1)) { + issuer = slist->data + sizeof(issuer_prefix)-1; + } + if(!strncmp(slist->data, subject_prefix, sizeof(subject_prefix)-1)) { + subject = slist->data + sizeof(subject_prefix)-1; + } + } + + if(subject && issuer) { + printf("cert %d\n", cert); + printf(" subject: %s\n", subject); + printf(" issuer: %s\n", issuer); + + if(last_issuer) { + /* If the last certificate's issuer matches the current certificate's + * subject, then the chain is in order */ + if(strcmp(last_issuer, subject) != 0) { + fprintf(stderr, "cert %d issuer does not match cert %d subject\n", + cert - 1, cert); + fprintf(stderr, "certificate chain is not in order\n"); + return false; + } + } + } + + last_issuer = issuer; + } + + printf("certificate chain is in order\n"); + return true; +} + +static size_t wrfu(void *ptr, size_t size, size_t nmemb, void *stream) +{ + (void)stream; + (void)ptr; + return size * nmemb; +} + +int test(char *URL) +{ + CURL *curl; + CURLcode res = CURLE_OK; + + if(curl_global_init(CURL_GLOBAL_ALL) != CURLE_OK) { + fprintf(stderr, "curl_global_init() failed\n"); + return TEST_ERR_MAJOR_BAD; + } + + curl = curl_easy_init(); + if(!curl) { + fprintf(stderr, "curl_easy_init() failed\n"); + curl_global_cleanup(); + return TEST_ERR_MAJOR_BAD; + } + + /* Set the HTTPS url to retrieve. */ + test_setopt(curl, CURLOPT_URL, URL); + + /* Capture certificate information */ + test_setopt(curl, CURLOPT_CERTINFO, 1L); + + /* Ignore output */ + test_setopt(curl, CURLOPT_WRITEFUNCTION, wrfu); + + /* No peer verify */ + test_setopt(curl, CURLOPT_SSL_VERIFYPEER, 0L); + test_setopt(curl, CURLOPT_SSL_VERIFYHOST, 0L); + + /* Perform the request, res will get the return code */ + res = curl_easy_perform(curl); + if(!res || res == CURLE_GOT_NOTHING) { + struct curl_certinfo *cert_info = NULL; + /* Get the certificate information */ + res = curl_easy_getinfo(curl, CURLINFO_CERTINFO, &cert_info); + if(!res) { + /* Check to see if the certificate chain is ordered correctly */ + if(!is_chain_in_order(cert_info)) + res = TEST_ERR_FAILURE; + } + } + +test_cleanup: + + /* always cleanup */ + curl_easy_cleanup(curl); + curl_global_cleanup(); + + return res; +}