Re: [PATCH v2 2/2] imap-send: let OpenSSL do the hostname verification

[Patrick Steinhardt <ps@pks.im>]

On Mon, Oct 12, 2026 at 10:30:57PM +0200, Beat Bolli wrote:
> According to the OpenSSL Wiki[1], OpenSSL knows how to verify the peer's
> hostname since version 1.0.2. A simpler interface is available since
> 1.1.0, which is what is implemented here.

What is implicit here is that Git performs its own hostname
verification. A short introduction explaining the status quo and why
the status quo is insufficient would help the reviewer.

What's also missing is a quick note on the minimum supported version of
OpenSSL.

> As a side-effect, we get all present and future improvements done by
> OpenSSL for free. E.g., the old code did not handle IP addresses in the
> subject alternative names, while the new code does.

That sounds like it's win-win: less code and more supported scenarios!

> If the verification fails, print the error message as returned by
> OpenSSL.
> 
> [1]: https://wiki.openssl.org/index.php/Hostname_validation
> 
> Suggested-by: brian m. carlson <sandals@crustytoothpaste.net>
> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
> ---
>  imap-send.c | 86 +++++++++--------------------------------------------
>  1 file changed, 14 insertions(+), 72 deletions(-)

Don't we have any tests for this? If not, can we maybe add some to make
sure that we indeed still verify the hostname?

> diff --git a/imap-send.c b/imap-send.c
> index 9f21705d77..cd9eae15d0 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -325,6 +261,15 @@ static int ssl_socket_connect(struct imap_socket *sock,
>  		ssl_socket_perror("SSL_new");
>  		return -1;
>  	}
> +	if (cfg->ssl_verify) {
> +		SSL_set_hostflags(sock->ssl, X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS);

This matches the documentation, but it also points out the
X509_CHECK_FLAG_NO_WILDCARDS as a second commonly used flag. Why do we
pick one over the other?

> @@ -352,14 +297,11 @@ static int ssl_socket_connect(struct imap_socket *sock,
>  	}
>  
>  	if (cfg->ssl_verify) {
> -		/* make sure the hostname matches that of the certificate */
> -		cert = SSL_get_peer_certificate(sock->ssl);
> -		if (!cert)
> -			return error("unable to get peer certificate.");
> -		if (SSL_get_verify_result(sock->ssl) != X509_V_OK)
> -			return error("unable to verify peer certificate");
> -		if (verify_hostname(cert, cfg->host) < 0)
> -			return -1;
> +		/* make sure the certificate was verified */
> +		long res = SSL_get_verify_result(sock->ssl);
> +		if (res != X509_V_OK)
> +			return error("unable to verify peer certificate: %s",
> +				     X509_verify_cert_error_string(res));

Wait, this feels fishy to me. Take the following from [1]:

  In other cases where no peer certificate or raw public key was
  presented, the returned result can still be X509_V_OK, for example
  when a server did not request a client certificate during the initial
  handshake. X509_V_OK alone does not establish how or whether the peer
  was authenticated. A suitably provisioned external PSK can
  authenticate a peer without a certificate or raw public key. When
  certificate or raw public key authentication is expected, applications
  must also check that the expected credential is present using
  SSL_get_peer_certificate(3) or SSL_get0_peer_rpk(3).

So this here basically allows for a MITM attack in case the intercepting
adversary simply strips the certificate altogether if I understand
correctly. And to fix that we must retain the call to
`SSL_get_peer_certificate()`.

That makes me want to have tests even more now.

[1]: https://docs.openssl.org/4.1/man3/SSL_get_verify_result/#bugs

Patrick