[PATCH 0/3] imap-send: future proofing and two correctness fixes

[PATCH 0/3] imap-send: future proofing and two correctness fixes

[Beat Bolli]

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

[PATCH 1/3] imap-send: prepare for OpenSSL 4.1

[Beat Bolli]

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 <dev+git@drbeat.li>
---
 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

[PATCH 2/3] imap-send: don't expect an ASN1_STRING to be NUL-terminated

[Beat Bolli]

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 <dev+git@drbeat.li>
---
 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

[PATCH 3/3] imap-send: only check the CN if no SAN DNS names are present

[Beat Bolli]

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 <dev+git@drbeat.li>
---
 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

Re: [PATCH 2/3] imap-send: don't expect an ASN1_STRING to be NUL-terminated

[Junio C Hamano]

Beat Bolli <dev+git@drbeat.li> 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)

Re: [PATCH 3/3] imap-send: only check the CN if no SAN DNS names are present

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2810 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 325 bytes --]

Re: [PATCH 1/3] imap-send: prepare for OpenSSL 4.1

[Junio C Hamano]

Beat Bolli <dev+git@drbeat.li> 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 <dev+git@drbeat.li>
> ---
>  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] == '.') {

Re: [PATCH 1/3] imap-send: prepare for OpenSSL 4.1

[Patrick Steinhardt]

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/

Re: [PATCH 2/3] imap-send: don't expect an ASN1_STRING to be NUL-terminated

[Patrick Steinhardt]

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