# [PATCH 0/3] imap-send: future proofing and two correctness fixes 11 messages from 2026-09-07 to 2026-09-23. Participants: Beat Bolli, Junio C Hamano, brian m. carlson, Patrick Steinhardt. Thread: https://gitlist.dev/t/66288 ## Beat Bolli, 2026-09-07 21:12 Subject: [PATCH 0/3] imap-send: future proofing and two correctness fixes Message-ID: <20260907211210.2621693-1-dev+git@drbeat.li> ``` Hi! Patch 1 future-proofs against a renamed ASN1_STRING function. Patch 2 fixes an incorrect assumption about NUL-termination of ASN1_STRINGs. Patch 3 only checks the certificate subject common name if no DNS subject alternative names are available, as defined by RFC 6125. Beat Bolli (3): imap-send: prepare for OpenSSL 4.1 imap-send: don't expect an ASN1_STRING to be NUL-terminated imap-send: only check the CN if no SAN DNS names are present imap-send.c | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-) -- 2.53.0 ``` ## Beat Bolli, 2026-09-07 21:12 Subject: [PATCH 3/3] imap-send: only check the CN if no SAN DNS names are present Message-ID: <20260907211210.2621693-4-dev+git@drbeat.li> In-Reply-To: <20260907211210.2621693-1-dev+git@drbeat.li> ``` Checking the certificate subject's common name may only be done if the subjectAltNames extension contains no DNS entries. If no SAN DNS name matches, there's no match. Per RFC 6125 section 6.4.4[1]: As noted, a client MUST NOT seek a match for a reference identifier of CN-ID if the presented identifiers include a DNS-ID, SRV-ID, URI-ID, or any application-specific identifier types supported by the client. This change was inspired by a similar commit in the HAProxy project[2]. [1]: https://datatracker.ietf.org/doc/html/rfc6125#section-6.4.4 [2]: https://github.com/haproxy/haproxy/commit/75129aaacb7a7b172f4e5334db71d6c1c50a3dbf Signed-off-by: Beat Bolli --- imap-send.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/imap-send.c b/imap-send.c index 9a807cdde8..66d3dbfaa5 100644 --- a/imap-send.c +++ b/imap-send.c @@ -256,11 +256,11 @@ static int verify_hostname(X509 *cert, const char *hostname) #endif const X509_NAME_ENTRY *cname_entry; const ASN1_STRING *cname; - int i, found; + int i, found, has_san_dns; STACK_OF(GENERAL_NAME) *subj_alt_names; /* try the DNS subjectAltNames */ - found = 0; + found = has_san_dns = 0; if ((subj_alt_names = X509_get_ext_d2i(cert, NID_subject_alt_name, NULL, NULL))) { int num_subj_alt_names = sk_GENERAL_NAME_num(subj_alt_names); for (i = 0; !found && i < num_subj_alt_names; i++) { @@ -268,13 +268,18 @@ static int verify_hostname(X509 *cert, const char *hostname) GENERAL_NAME *subj_alt_name = sk_GENERAL_NAME_value(subj_alt_names, i); ASN1_STRING *subj_alt_str = GENERAL_NAME_get0_value(subj_alt_name, &ntype); - if (ntype == GEN_DNS && host_matches(hostname, subj_alt_str)) - found = 1; + if (ntype == GEN_DNS) { + has_san_dns = 1; + if (host_matches(hostname, subj_alt_str)) + found = 1; + } } sk_GENERAL_NAME_pop_free(subj_alt_names, GENERAL_NAME_free); } if (found) return 0; + if (has_san_dns) + return error("none of the subjectAltNames matches hostname '%s'", hostname); /* try the common name */ if (!(subj = X509_get_subject_name(cert))) -- 2.53.0 ``` ## Beat Bolli, 2026-09-07 21:12 Subject: [PATCH 2/3] imap-send: don't expect an ASN1_STRING to be NUL-terminated Message-ID: <20260907211210.2621693-3-dev+git@drbeat.li> In-Reply-To: <20260907211210.2621693-1-dev+git@drbeat.li> ``` As highlighted by a recent OpenSSL commit[1], ASN1_STRINGs were never documented to be terminated by a NUL byte, but our code treats the pattern as such in the strcasecmp() call. Make a NUL-terminated copy to avoid Undefined Behavior. [1]: https://github.com/openssl/openssl/commit/4b581a4666c3e470a01a7323801b2ba8ccfa478c (Add a migration entry for ASN1_STRINGs, 2026-08-06) Signed-off-by: Beat Bolli --- imap-send.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/imap-send.c b/imap-send.c index 977d78005c..9a807cdde8 100644 --- a/imap-send.c +++ b/imap-send.c @@ -226,20 +226,25 @@ static int ssl_socket_connect(struct imap_socket *sock UNUSED, static int host_matches(const char *host, const ASN1_STRING *asn1_str) { - const char *pattern = (const char *)ASN1_STRING_get0_data(asn1_str); + int ret = 0; + size_t len = ASN1_STRING_get_length(asn1_str); + char *pattern = xmemdupz(ASN1_STRING_get0_data(asn1_str), len); /* embedded NUL characters may open a security hole */ - if (memchr(pattern, '\0', ASN1_STRING_get_length(asn1_str))) - return 0; + if (memchr(pattern, '\0', len)) + goto out; if (pattern[0] == '*' && pattern[1] == '.') { pattern += 2; if (!(host = strchr(host, '.'))) - return 0; + goto out; host++; } - return *host && *pattern && !strcasecmp(host, pattern); + ret = *host && *pattern && !strcasecmp(host, pattern); +out: + free(pattern); + return ret; } static int verify_hostname(X509 *cert, const char *hostname) -- 2.53.0 ``` ## Beat Bolli, 2026-09-07 21:12 Subject: [PATCH 1/3] imap-send: prepare for OpenSSL 4.1 Message-ID: <20260907211210.2621693-2-dev+git@drbeat.li> In-Reply-To: <20260907211210.2621693-1-dev+git@drbeat.li> ``` OpenSSL master (to be v4.1 after the release) renamed the function ASN1_STRING_length() to ASN1_STRING_get_length(). Map the new name to the old one if we're compiling with a pre-4.1 version. Signed-off-by: Beat Bolli --- imap-send.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/imap-send.c b/imap-send.c index 0d16d02029..977d78005c 100644 --- a/imap-send.c +++ b/imap-send.c @@ -219,12 +219,17 @@ static int ssl_socket_connect(struct imap_socket *sock UNUSED, #else +#if (OPENSSL_VERSION_NUMBER < 0x40100000L) +// map to the pre-4.1 name +#define ASN1_STRING_get_length(s) ASN1_STRING_length(s) +#endif + static int host_matches(const char *host, const ASN1_STRING *asn1_str) { const char *pattern = (const char *)ASN1_STRING_get0_data(asn1_str); /* embedded NUL characters may open a security hole */ - if (memchr(pattern, '\0', ASN1_STRING_length(asn1_str))) + if (memchr(pattern, '\0', ASN1_STRING_get_length(asn1_str))) return 0; if (pattern[0] == '*' && pattern[1] == '.') { -- 2.53.0 ``` ## Junio C Hamano, 2026-09-08 01:17 Subject: Re: [PATCH 2/3] imap-send: don't expect an ASN1_STRING to be NUL-terminated Message-ID: In-Reply-To: <20260907211210.2621693-3-dev+git@drbeat.li> ``` Beat Bolli writes: > - const char *pattern = (const char *)ASN1_STRING_get0_data(asn1_str); > + int ret = 0; > + size_t len = ASN1_STRING_get_length(asn1_str); > + char *pattern = xmemdupz(ASN1_STRING_get0_data(asn1_str), len); > > /* embedded NUL characters may open a security hole */ > - if (memchr(pattern, '\0', ASN1_STRING_get_length(asn1_str))) > - return 0; > + if (memchr(pattern, '\0', len)) > + goto out; > > if (pattern[0] == '*' && pattern[1] == '.') { > pattern += 2; > if (!(host = strchr(host, '.'))) > - return 0; > + goto out; > host++; > } > > - return *host && *pattern && !strcasecmp(host, pattern); > + ret = *host && *pattern && !strcasecmp(host, pattern); > +out: > + free(pattern); There is a code path that increments the "pattern" variable by 2. Running free() on it would not have a pleasant outcome. The pattern we often employ in our codebase is to have a separate variable "char *pattern_to_free" and have it used only for a call to free(). > + return ret; > } > > static int verify_hostname(X509 *cert, const char *hostname) ``` ## brian m. carlson, 2026-09-08 01:28 Subject: Re: [PATCH 3/3] imap-send: only check the CN if no SAN DNS names are present Message-ID: In-Reply-To: <20260907211210.2621693-4-dev+git@drbeat.li> ``` On 2026-09-07 at 21:12:10, Beat Bolli wrote: > Checking the certificate subject's common name may only be done if the > subjectAltNames extension contains no DNS entries. If no SAN DNS name > matches, there's no match. > > Per RFC 6125 section 6.4.4[1]: > > As noted, a client MUST NOT seek a match for a reference identifier > of CN-ID if the presented identifiers include a DNS-ID, SRV-ID, > URI-ID, or any application-specific identifier types supported by the > client. > > This change was inspired by a similar commit in the HAProxy project[2]. TLS is not supposed to use the CN at all these days and Go's implementation completely ignores it. subjectAltName is supposed to be used in all cases. > diff --git a/imap-send.c b/imap-send.c > index 9a807cdde8..66d3dbfaa5 100644 > --- a/imap-send.c > +++ b/imap-send.c > @@ -256,11 +256,11 @@ static int verify_hostname(X509 *cert, const char *hostname) > #endif > const X509_NAME_ENTRY *cname_entry; > const ASN1_STRING *cname; > - int i, found; > + int i, found, has_san_dns; > STACK_OF(GENERAL_NAME) *subj_alt_names; > > /* try the DNS subjectAltNames */ > - found = 0; > + found = has_san_dns = 0; > if ((subj_alt_names = X509_get_ext_d2i(cert, NID_subject_alt_name, NULL, NULL))) { > int num_subj_alt_names = sk_GENERAL_NAME_num(subj_alt_names); > for (i = 0; !found && i < num_subj_alt_names; i++) { > @@ -268,13 +268,18 @@ static int verify_hostname(X509 *cert, const char *hostname) > GENERAL_NAME *subj_alt_name = sk_GENERAL_NAME_value(subj_alt_names, i); > ASN1_STRING *subj_alt_str = GENERAL_NAME_get0_value(subj_alt_name, &ntype); > > - if (ntype == GEN_DNS && host_matches(hostname, subj_alt_str)) > - found = 1; > + if (ntype == GEN_DNS) { > + has_san_dns = 1; > + if (host_matches(hostname, subj_alt_str)) > + found = 1; > + } This handles certificates with DNS names but not IP addresses. So, for instance, this match wouldn't work for the certificates for 1.1.1.1 (assuming they had public IMAP service). > } > sk_GENERAL_NAME_pop_free(subj_alt_names, GENERAL_NAME_free); > } > if (found) > return 0; > + if (has_san_dns) > + return error("none of the subjectAltNames matches hostname '%s'", hostname); I know OpenSSL has built-in hostname verification that can be used as of OpenSSL 1.0.2[0]. Is there a reason we're still doing this by hand? Relying on OpenSSL's verification would mean that (a) we would not have to worry about getting verification wrong in a security-sensitive way and (b) OpenSSL would handle the policy and standards compliance functionality. [0] https://wiki.openssl.org/index.php/Hostname_validation -- brian m. carlson (they/them) Toronto, Ontario, CA ``` ## Junio C Hamano, 2026-09-08 04:17 Subject: Re: [PATCH 1/3] imap-send: prepare for OpenSSL 4.1 Message-ID: In-Reply-To: <20260907211210.2621693-2-dev+git@drbeat.li> ``` Beat Bolli writes: > OpenSSL master (to be v4.1 after the release) renamed the function > ASN1_STRING_length() to ASN1_STRING_get_length(). Map the new name to > the old one if we're compiling with a pre-4.1 version. > > Signed-off-by: Beat Bolli > --- > imap-send.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/imap-send.c b/imap-send.c > index 0d16d02029..977d78005c 100644 > --- a/imap-send.c > +++ b/imap-send.c > @@ -219,12 +219,17 @@ static int ssl_socket_connect(struct imap_socket *sock UNUSED, > > #else > > +#if (OPENSSL_VERSION_NUMBER < 0x40100000L) > +// map to the pre-4.1 name Style? > +#define ASN1_STRING_get_length(s) ASN1_STRING_length(s) > +#endif > + > static int host_matches(const char *host, const ASN1_STRING *asn1_str) > { > const char *pattern = (const char *)ASN1_STRING_get0_data(asn1_str); > > /* embedded NUL characters may open a security hole */ > - if (memchr(pattern, '\0', ASN1_STRING_length(asn1_str))) > + if (memchr(pattern, '\0', ASN1_STRING_get_length(asn1_str))) > return 0; > > if (pattern[0] == '*' && pattern[1] == '.') { ``` ## Patrick Steinhardt, 2026-09-08 08:26 Subject: Re: [PATCH 1/3] imap-send: prepare for OpenSSL 4.1 Message-ID: In-Reply-To: <20260907211210.2621693-2-dev+git@drbeat.li> ``` On Mon, Sep 07, 2026 at 11:12:08PM +0200, Beat Bolli wrote: > OpenSSL master (to be v4.1 after the release) renamed the function > ASN1_STRING_length() to ASN1_STRING_get_length(). Map the new name to > the old one if we're compiling with a pre-4.1 version. I can see [1] that the new functions indeed exist now. But it doesn't say anything about the old functions, they still exist and don't seem to be deprecated. So why do we even have to switch to the new function? Patrick [1]: https://docs.openssl.org/master/man3/ASN1_STRING_length/ ``` ## Patrick Steinhardt, 2026-09-08 08:26 Subject: Re: [PATCH 2/3] imap-send: don't expect an ASN1_STRING to be NUL-terminated Message-ID: In-Reply-To: <20260907211210.2621693-3-dev+git@drbeat.li> ``` On Mon, Sep 07, 2026 at 11:12:09PM +0200, Beat Bolli wrote: > diff --git a/imap-send.c b/imap-send.c > index 977d78005c..9a807cdde8 100644 > --- a/imap-send.c > +++ b/imap-send.c > @@ -226,20 +226,25 @@ static int ssl_socket_connect(struct imap_socket *sock UNUSED, > > static int host_matches(const char *host, const ASN1_STRING *asn1_str) > { > - const char *pattern = (const char *)ASN1_STRING_get0_data(asn1_str); > + int ret = 0; > + size_t len = ASN1_STRING_get_length(asn1_str); > + char *pattern = xmemdupz(ASN1_STRING_get0_data(asn1_str), len); > > /* embedded NUL characters may open a security hole */ > - if (memchr(pattern, '\0', ASN1_STRING_get_length(asn1_str))) > - return 0; > + if (memchr(pattern, '\0', len)) > + goto out; > > if (pattern[0] == '*' && pattern[1] == '.') { > pattern += 2; > if (!(host = strchr(host, '.'))) > - return 0; > + goto out; > host++; > } > > - return *host && *pattern && !strcasecmp(host, pattern); > + ret = *host && *pattern && !strcasecmp(host, pattern); > +out: > + free(pattern); > + return ret; > } I don't quite see a reason why we even have to memdup the string. We already use memchr, which is bounded by the length of the string. We do have two other sites though: - We use strchr, but that can be adapted to use memchr. - Likewise, we use strcasecmp, but that can be adapted to use strncasecmp. So with that, all calls that inspect the string would be bounded by the length of the encoded string, and that means we don't have to copy the string first, do we? Patrick ``` ## Beat Bolli, 2026-09-14 09:10 Subject: Re: [PATCH 1/3] imap-send: prepare for OpenSSL 4.1 Message-ID: <872286d0-7786-46cc-b26f-16f6d487e608@drbeat.li> In-Reply-To: ``` Hi Patrick On 08.09.2026 10:26, Patrick Steinhardt wrote: > On Mon, Sep 07, 2026 at 11:12:08PM +0200, Beat Bolli wrote: >> OpenSSL master (to be v4.1 after the release) renamed the function >> ASN1_STRING_length() to ASN1_STRING_get_length(). Map the new name to >> the old one if we're compiling with a pre-4.1 version. > > I can see [1] that the new functions indeed exist now. But it doesn't > say anything about the old functions, they still exist and don't seem to > be deprecated. So why do we even have to switch to the new function? > The very page you give below contains this text: ---- 8< ---- The following functions have been deprecated since OpenSSL 4.1, and can be hidden entirely by defining OPENSSL_API_COMPAT with a suitable version value, see openssl_user_macros(7): int ASN1_STRING_set(ASN1_STRING *str, const void *data, int len); int ASN1_STRING_length(ASN1_STRING *x); ---- 8< ---- We also don't define any compatibility macros for OpenSSL, and the build failed because of the deprecation warning that was turned into an error because of DEVELOPER=1. So I still think this patch is needed. Beat > [1]: https://docs.openssl.org/master/man3/ASN1_STRING_length/ ``` ## Patrick Steinhardt, 2026-09-23 12:32 Subject: Re: [PATCH 1/3] imap-send: prepare for OpenSSL 4.1 Message-ID: In-Reply-To: <872286d0-7786-46cc-b26f-16f6d487e608@drbeat.li> ``` On Mon, Sep 14, 2026 at 11:10:36AM +0200, Beat Bolli wrote: > Hi Patrick > > On 08.09.2026 10:26, Patrick Steinhardt wrote: > > On Mon, Sep 07, 2026 at 11:12:08PM +0200, Beat Bolli wrote: > > > OpenSSL master (to be v4.1 after the release) renamed the function > > > ASN1_STRING_length() to ASN1_STRING_get_length(). Map the new name to > > > the old one if we're compiling with a pre-4.1 version. > > > > I can see [1] that the new functions indeed exist now. But it doesn't > > say anything about the old functions, they still exist and don't seem to > > be deprecated. So why do we even have to switch to the new function? > > > > The very page you give below contains this text: > > ---- 8< ---- > The following functions have been deprecated since OpenSSL 4.1, and can be > hidden entirely by defining OPENSSL_API_COMPAT with a suitable version > value, see openssl_user_macros(7): > > int ASN1_STRING_set(ASN1_STRING *str, const void *data, int len); > int ASN1_STRING_length(ASN1_STRING *x); > ---- 8< ---- > > We also don't define any compatibility macros for OpenSSL, and the build > failed because of the deprecation warning that was turned into an error > because of DEVELOPER=1. Ah, I missed the part about OPENSSL_API_COMPAT. I think it would make sense to explicitly point that out in the commit message. Thanks! Patrick ```