Re: [PATCH] imap-send: use ASN1_STRING_length

[Junio C Hamano <gitster@pobox.com>]

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.