[PATCH] imap-send: use ASN1_STRING_length

[PATCH] imap-send: use ASN1_STRING_length

[Xiangfei Ding]

From: xfding <xfding@google.com>

X.509 has never stipulated that ASN1_STRING shall be null-terminated in general.
In OpenSSL, `ASN1_STRING_get0_data()` returns an internal pointer to
the raw string payload, which is lucky for us null-terminated. This is probably
about to change as OpenSSL derivatives like BoringSSL would adopt changes so
that the data buffer would not be null-terminated anymore.

Strictly speaking, printing or accessing `ASN1_STRING` data should always query
`ASN1_STRING_length`. This means that we should adapt `host_matches` and
`verify_hostname` to use `strncasecmp` and print strings with designated length.

Signed-off-by: Xiangfei Ding <dingxiangfei2009@protonmail.ch>
Signed-off-by: Xiangfei Ding <xfding@google.com>
---
 imap-send.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index bf1d2cf74d..bd306b8d0a 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -225,19 +225,21 @@ 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 pat_len = ASN1_STRING_length(asn1_str);
 
 	/* embedded NUL characters may open a security hole */
-	if (memchr(pattern, '\0', ASN1_STRING_length(asn1_str)))
-	    return 0;
+	if (memchr(pattern, '\0', pat_len))
+		return 0;
 
-	if (pattern[0] == '*' && pattern[1] == '.') {
+	if (pat_len >= 2 && pattern[0] == '*' && pattern[1] == '.') {
 		pattern += 2;
+		pat_len -= 2;
 		if (!(host = strchr(host, '.')))
 			return 0;
 		host++;
 	}
 
-	return *host && *pattern && !strcasecmp(host, pattern);
+	return *host && strlen(host) == (size_t)pat_len && !strncasecmp(host, pattern, pat_len);
 }
 
 static int verify_hostname(X509 *cert, const char *hostname)
@@ -278,8 +280,9 @@ static int verify_hostname(X509 *cert, const char *hostname)
 		return error("cannot get certificate common name");
 	if (host_matches(hostname, cname))
 		return 0;
-	return error("certificate owner '%s' does not match hostname '%s'",
-		     ASN1_STRING_get0_data(cname), hostname);
+	return error("certificate owner '%.*s' does not match hostname '%s'",
+		     (int)ASN1_STRING_length(cname),
+		     (const char *)ASN1_STRING_get0_data(cname), hostname);
 }
 
 static int ssl_socket_connect(struct imap_socket *sock,
-- 
2.56.0.385.gd3acb90ef8-goog

Re: [PATCH] imap-send: use ASN1_STRING_length

[Junio C Hamano]

Xiangfei Ding <xfding@google.com> writes:

> From: xfding <xfding@google.com>

This line is called in-body From: that is used to override the "From:"
header your MUA will add.  But in this case, I think you do not want
to override "Xiangfei Ding <xfding@google.com>", which is what your
MUA sent the message as.  In any case, either MUA From or in-body From
is used as the author identity when this patch e-mail is committed, so
it should match your sign-off we see later.

> X.509 has never stipulated that ASN1_STRING shall be null-terminated in general.
> In OpenSSL, `ASN1_STRING_get0_data()` returns an internal pointer to
> the raw string payload, which is lucky for us null-terminated.

OK.

> This is probably
> about to change as OpenSSL derivatives like BoringSSL would adopt changes so
> that the data buffer would not be null-terminated anymore.

It is unclear from reading the above if you mean only derivatives
will adopt changes not to NUL-terminate, or OpenSSL will change
ASN1_STRING_get0_data() to return a string that is not
NUL-terminated, and derivatives will follow suit.

> Strictly speaking, printing or accessing `ASN1_STRING` data should always query
> `ASN1_STRING_length`. This means that we should adapt `host_matches` and
> `verify_hostname` to use `strncasecmp` and print strings with designated length.

Makes sense.  I take that the second paragraph to mean "not using
ASN1_STRING_length happened to be OK, but that is not true anymore
with various implementations of OpenSSL like libraries",
illustrating the need and urgency to make this change?

In our log message, usually all of the above (i.e., observation of
the current situation and pointing out problems in the status quo)
are followed by what to do to solve the problem by giving an order
to hypothetical somebody to update the code to "make it so", e.g.,

    Update code that assumes ASN1_STRING data are NUL-terminated to
    use ASN1_STRING_length and treat the data as <pointer, length>
    pair.

but the above description you gave is sufficiently detailed and left
no need to make a separate description of the solution.

> Signed-off-by: Xiangfei Ding <dingxiangfei2009@protonmail.ch>
> Signed-off-by: Xiangfei Ding <xfding@google.com>

What are these two separate sign-off lines doing?

Unless rules have changed recently, I think Google OSPO tells you as
a Google engineer [*] to use your @google.com address when making
contributions to open source projects, "unless you have a history of
contributing to the repo under a different email before your
employment at Google".  So only the second one would be what you
want to use, I presume?  Again, as I mentioned earlier, it matches
the address your MUA sent the message as, so you do not need/want
the in-body From: line.

[Reference]

 * https://opensource.google/documentation/reference/patching

> diff --git a/imap-send.c b/imap-send.c
> index bf1d2cf74d..bd306b8d0a 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -225,19 +225,21 @@ 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 pat_len = ASN1_STRING_length(asn1_str);
>  
>  	/* embedded NUL characters may open a security hole */
> -	if (memchr(pattern, '\0', ASN1_STRING_length(asn1_str)))
> -	    return 0;
> +	if (memchr(pattern, '\0', pat_len))
> +		return 0;

OK, the above is essentially a no-op.  But we have the length in a
variable so we can reuse it in the rest of the function, which is
good.

> -	if (pattern[0] == '*' && pattern[1] == '.') {
> +	if (pat_len >= 2 && pattern[0] == '*' && pattern[1] == '.') {
>  		pattern += 2;
> +		pat_len -= 2;
>  		if (!(host = strchr(host, '.')))
>  			return 0;
>  		host++;
>  	}

OK, we check for "*." only when we have at least two bytes, and
consume these two bytes if they match.

> -	return *host && *pattern && !strcasecmp(host, pattern);
> +	return *host && strlen(host) == (size_t)pat_len && !strncasecmp(host, pattern, pat_len);

LIne-wrap this overly long line.

	return *host && strlen(host) == (size_t)pat_len &&
               !strncasecmp(host, pattern, pat_len);

Curious.  If we declare pat_len to be of type size_t, we would no
longer need to cast it here.  Even though we passed pat_len to
memchr() earlier, that call implicitly promoted int to size_t, which
would also become unnecessary.

>  }
>  
>  static int verify_hostname(X509 *cert, const char *hostname)
> @@ -278,8 +280,9 @@ static int verify_hostname(X509 *cert, const char *hostname)
>  		return error("cannot get certificate common name");
>  	if (host_matches(hostname, cname))
>  		return 0;
> -	return error("certificate owner '%s' does not match hostname '%s'",
> -		     ASN1_STRING_get0_data(cname), hostname);
> +	return error("certificate owner '%.*s' does not match hostname '%s'",
> +		     (int)ASN1_STRING_length(cname),

Because ASN1_STRING_length() is declared to return "int", this cast
is not strictly needed, but it is good to be explicit in a case like
this, I think.

> +		     (const char *)ASN1_STRING_get0_data(cname), hostname);

Great.  

Re: [PATCH] imap-send: use ASN1_STRING_length

[Aniket Gupta]

Hello Xiangfei,

> +	int pat_len = ASN1_STRING_length(asn1_str);

It seems that with OpenSSL 4.1.0-beta2 this is deprecated (ASN1_STRING_get_length() is the replacement), so it warns, and fails with DEVELOPER=1. The patch adds a second use at the error message. Beat Bolli has a series from 7 Sept ("imap-send: future proofing and two correctness fixes") that also touches host_matches(), maybe we can check how the two can fit together?

Thanks,
Aniket

Re: [PATCH] imap-send: use ASN1_STRING_length

[Junio C Hamano]

Aniket Gupta <aniketgupta3001@gmail.com> writes:

> Hello Xiangfei,
>
>> +	int pat_len = ASN1_STRING_length(asn1_str);
>
> It seems that with OpenSSL 4.1.0-beta2 this is deprecated
> (ASN1_STRING_get_length() is the replacement), so it warns, and
> fails with DEVELOPER=1. The patch adds a second use at the error
> message. Beat Bolli has a series from 7 Sept ("imap-send: future
> proofing and two correctness fixes") that also touches
> host_matches(), maybe we can check how the two can fit together?
>
> Thanks,
> Aniket

[jch: rewrapped long unreadable lines]

That topic also needs rework, unfortunately.

  https://lore.kernel.org/git/20260907211210.2621693-1-dev+git@drbeat.li/

I guess we can salvage the patches 1 and 2 with fairly trivial
changes, namely with the style fix in [1/3], and pointer arithmetic
that breaks later free() fix in [2/3].  I am not sure how involved
it would be to fix [3/3] in such a way suggested by brian in the
thread.

Thanks for reminding us of the old thread.

Re: [PATCH] imap-send: use ASN1_STRING_length

[Beat Bolli]

On 12.10.2026 19:18, Junio C Hamano wrote:
> Aniket Gupta <aniketgupta3001@gmail.com> writes:
> 
>> Hello Xiangfei,
>>
>>> +	int pat_len = ASN1_STRING_length(asn1_str);
>>
>> It seems that with OpenSSL 4.1.0-beta2 this is deprecated
>> (ASN1_STRING_get_length() is the replacement), so it warns, and
>> fails with DEVELOPER=1. The patch adds a second use at the error
>> message. Beat Bolli has a series from 7 Sept ("imap-send: future
>> proofing and two correctness fixes") that also touches
>> host_matches(), maybe we can check how the two can fit together?
>>
>> Thanks,
>> Aniket
> 
> [jch: rewrapped long unreadable lines]
> 
> That topic also needs rework, unfortunately.
> 
>    https://lore.kernel.org/git/20260907211210.2621693-1-dev+git@drbeat.li/
> 
> I guess we can salvage the patches 1 and 2 with fairly trivial
> changes, namely with the style fix in [1/3], and pointer arithmetic
> that breaks later free() fix in [2/3].  I am not sure how involved
> it would be to fix [3/3] in such a way suggested by brian in the
> thread.
> 
> Thanks for reminding us of the old thread.
> 

I'm working on a new branch that uses OpenSSL's built-in hostname
verification. All this low level ASN.1 code will disappear as a result.

Best regards, Beat