Volume XXII, number 279Tuesday, October 6, 2026Latest message 1 hour ago

The Git List

News and archive of git@vger.kernel.org, since April 2005

patch, 3 partsimap-send: future proofing and two correctness fixes

11 messages between Sep 7, 2026 and Sep 23, 2026, from Beat Bolli, Junio C Hamano, brian m. carlson, Patrick Steinhardt.

Plain Markdown or JSON for tools and agents. Diffs are folded; open one to read it.

Beat BolliSep 7, 2026, 21:12 UTC on lore
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 BolliSep 7, 2026, 21:12 UTC in reply to Beat Bolli on lore

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

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(-)
Show changes to imap-send.c +9 −4
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 BolliSep 7, 2026, 21:12 UTC in reply to Beat Bolli on lore

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

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(-)
Show changes to imap-send.c +10 −5
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 BolliSep 7, 2026, 21:12 UTC in reply to Beat Bolli on lore

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

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(-)
Show changes to imap-send.c +6 −1
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 HamanoSep 8, 2026, 01:17 UTC in reply to Beat Bolli on lore

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

Beat Bolli <dev+git@drbeat.li> writes:
Show 23 quoted lines
> -	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. carlsonSep 8, 2026, 01:28 UTC in reply to Beat Bolli on lore

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

On 2026-09-07 at 21:12:10, Beat Bolli wrote:
Show 12 quoted lines
> 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.

Show 29 quoted lines
> 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).

Show 7 quoted lines
>  		}
>  		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 HamanoSep 8, 2026, 04:17 UTC in reply to Beat Bolli on lore

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

Beat Bolli <dev+git@drbeat.li> writes:
Show 19 quoted lines
> 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?
Show 13 quoted lines
> +#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 SteinhardtSep 8, 2026, 08:26 UTC in reply to Beat Bolli on lore

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

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 SteinhardtSep 8, 2026, 08:26 UTC in reply to Beat Bolli on lore

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

On Mon, Sep 07, 2026 at 11:12:09PM +0200, Beat Bolli wrote:
Show 33 quoted lines
> 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 BolliSep 14, 2026, 09:10 UTC in reply to Patrick Steinhardt on lore

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

Hi Patrick
On 08.09.2026 10:26, Patrick Steinhardt wrote:
Show 9 quoted lines
> 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 SteinhardtSep 23, 2026, 12:32 UTC in reply to Beat Bolli on lore

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

On Mon, Sep 14, 2026 at 11:10:36AM +0200, Beat Bolli wrote:
Show 27 quoted lines
> 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

Back to recent threads