From d49e35392370b8d89f008561464087f29f456456 Mon Sep 17 00:00:00 2001 From: tanjiro Kamado Date: Wed, 21 Oct 2026 13:57:26 +0530 Subject: [PATCH 1/2] check request common name in cupsSignCredentialsRequest --- CHANGES.md | 2 ++ cups/testcreds.c | 18 ++++++++++++++++++ cups/tls-gnutls.c | 15 +++++++++++++++ cups/tls-openssl.c | 17 +++++++++++++++++ 4 files changed, 52 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index ec7350113..d3eab5bbd 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -30,6 +30,8 @@ v3.0.4 - YYYY-MM-DD - Fixed a potential string overrun when loading .strings files (Issue #171) - Fixed `cupsSignCredentialsRequest` to reject requests with a bad signature (Issue 172) +- Fixed `cupsSignCredentialsRequest` to reject requests whose subject common + name does not match the common name being signed. - Fixed `httpPeek` return value with compressed message bodies (Issue #175) - Fixed a potential integer overflow with `pwgMediaForPWG` (Issue #176) - Fixed some character testing that wasn't safe on platforms with a signed diff --git a/cups/testcreds.c b/cups/testcreds.c index 176d3aeeb..537a2ca25 100644 --- a/cups/testcreds.c +++ b/cups/testcreds.c @@ -424,6 +424,17 @@ do_unit_tests(void) "ecdsa-p384", "ecdsa-p521" }; + static const char * const badcn_request = + // CSR for CN=badprinter, DNS:altprinter + "-----BEGIN CERTIFICATE REQUEST-----\n" + "MIIBHjCBxAIBADAVMRMwEQYDVQQDDApiYWRwcmludGVyMFkwEwYHKoZIzj0CAQYI\n" + "KoZIzj0DAQcDQgAE4DPBo9+csGwv9kthG3ZrrKg2GkxaKFsgJZa29RhM13vYa4jO\n" + "fpcT5aGad0spacNRwqX6GdiQjlQqp6YFdt4YFKBNMEsGCSqGSIb3DQEJDjE+MDww\n" + "FQYDVR0RBA4wDIIKYWx0cHJpbnRlcjAOBgNVHQ8BAf8EBAMCBaAwEwYDVR0lBAww\n" + "CgYIKwYBBQUHAwEwCgYIKoZIzj0EAwIDSQAwRgIhAM+MG8suwyzesdEF+33wU7CG\n" + "qVydDfe8KEn/S5FenVMlAiEAzHItj++UctdqVHD4c8Uv2Du0exSxQ4RvKUoNb7At\n" + "a4I=\n" + "-----END CERTIFICATE REQUEST-----\n"; for (type = CUPS_CREDTYPE_DEFAULT; type <= CUPS_CREDTYPE_ECDSA_P521_SHA256; type ++) @@ -604,6 +615,13 @@ do_unit_tests(void) } } + // A request whose subject CN names another host must not be signed... + testBegin("cupsSignCredentialsRequest(altprinter w/CN=badprinter)"); + if (cupsSignCredentialsRequest(TEST_CERT_PATH, "altprinter", badcn_request, "_site_", CUPS_CREDPURPOSE_ALL, CUPS_CREDUSAGE_ALL, /*cb*/NULL, /*cb_data*/NULL, time(NULL) + 30 * 86400)) + testEndMessage(false, "Expected a failure"); + else + testEndMessage(true, "%s", cupsGetErrorString()); + return (testsPassed ? 0 : 1); } diff --git a/cups/tls-gnutls.c b/cups/tls-gnutls.c index 3939068f2..1e8c99577 100644 --- a/cups/tls-gnutls.c +++ b/cups/tls-gnutls.c @@ -1163,6 +1163,21 @@ cupsSignCredentialsRequest( goto done; } + // The subject is copied to the certificate, so any commonName values must + // match the common name we are signing for... + for (i = 0; i < 100; i ++) + { + tempsize = sizeof(temp) - 1; + if ((err = gnutls_x509_crq_get_dn_by_oid(crq, GNUTLS_OID_X520_COMMON_NAME, (unsigned)i, 0, temp, &tempsize)) == GNUTLS_E_REQUESTED_DATA_NOT_AVAILABLE) + break; + + if (err < 0 || tempsize != strlen(common_name) || _cups_strncasecmp(temp, common_name, tempsize)) + { + _cupsSetError(IPP_STATUS_ERROR_INTERNAL, _("Validation of commonName in X.509 certificate request failed."), true); + goto done; + } + } + // Create the certificate... DEBUG_puts("1cupsSignCredentialsRequest: Generating X.509 certificate."); diff --git a/cups/tls-openssl.c b/cups/tls-openssl.c index 7bc994d34..1f17154b7 100644 --- a/cups/tls-openssl.c +++ b/cups/tls-openssl.c @@ -1086,6 +1086,7 @@ cupsSignCredentialsRequest( bool result = false; // Return value X509 *cert = NULL; // Certificate X509_REQ *crq = NULL; // Certificate request + const X509_NAME *subject; // Subject name of request X509 *root_cert = NULL; // Root certificate, if any EVP_PKEY *root_key = NULL; // Root private key, if any char defpath[1024], // Default path @@ -1144,6 +1145,22 @@ cupsSignCredentialsRequest( goto done; } + // The subject is copied to the certificate, so any commonName values must + // match the common name we are signing for... + subject = X509_REQ_get_subject_name(crq); + + for (i = -1; (i = X509_NAME_get_index_by_NID(subject, NID_commonName, i)) >= 0;) + { + const ASN1_STRING *cn = X509_NAME_ENTRY_get_data(X509_NAME_get_entry(subject, i)); + // commonName value + + if ((size_t)ASN1_STRING_length(cn) != strlen(common_name) || _cups_strncasecmp((const char *)ASN1_STRING_get0_data(cn), common_name, strlen(common_name))) + { + _cupsSetError(IPP_STATUS_ERROR_INTERNAL, _("Validation of commonName in X.509 certificate request failed."), true); + goto done; + } + } + // Create the X.509 certificate... DEBUG_puts("1cupsSignCredentialsRequest: Generating X.509 certificate."); From 7f67c42f720fb9435fb680930426168e1fe9cab9 Mon Sep 17 00:00:00 2001 From: tanjiro Kamado Date: Fri, 23 Oct 2026 12:58:33 +0530 Subject: [PATCH 2/2] bound the commonName compare by the asn.1 length and drop changelog hunk --- CHANGES.md | 2 -- cups/tls-openssl.c | 6 +++++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index d3eab5bbd..ec7350113 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -30,8 +30,6 @@ v3.0.4 - YYYY-MM-DD - Fixed a potential string overrun when loading .strings files (Issue #171) - Fixed `cupsSignCredentialsRequest` to reject requests with a bad signature (Issue 172) -- Fixed `cupsSignCredentialsRequest` to reject requests whose subject common - name does not match the common name being signed. - Fixed `httpPeek` return value with compressed message bodies (Issue #175) - Fixed a potential integer overflow with `pwgMediaForPWG` (Issue #176) - Fixed some character testing that wasn't safe on platforms with a signed diff --git a/cups/tls-openssl.c b/cups/tls-openssl.c index 1f17154b7..ab6164bc1 100644 --- a/cups/tls-openssl.c +++ b/cups/tls-openssl.c @@ -1153,8 +1153,12 @@ cupsSignCredentialsRequest( { const ASN1_STRING *cn = X509_NAME_ENTRY_get_data(X509_NAME_get_entry(subject, i)); // commonName value + size_t cnlen = (size_t)ASN1_STRING_length(cn); + // Length of commonName value - if ((size_t)ASN1_STRING_length(cn) != strlen(common_name) || _cups_strncasecmp((const char *)ASN1_STRING_get0_data(cn), common_name, strlen(common_name))) + // The ASN.1 data is not necessarily nul-terminated, so compare only the + // encoded length and never read past it... + if (cnlen != strlen(common_name) || _cups_strncasecmp((const char *)ASN1_STRING_get0_data(cn), common_name, cnlen)) { _cupsSetError(IPP_STATUS_ERROR_INTERNAL, _("Validation of commonName in X.509 certificate request failed."), true); goto done;