Re: Cert validation in pkg_add

[Greg Troxel <gdt%lexort.com@localhost>]

Taylor R Campbell <riastradh%NetBSD.org@localhost> writes:

> We can do that, but I just want to be clear that this means we might
> break existing setups that never asked for https in the first place --
> that this can change the behaviour of _any_ installation, not just
> depending on its local configuration, but depending on the behaviour
> of remote servers on the internet.

Sorry, I somehow missed this in trying to sort out dealing with the
branch and Things Other than NetBSD, leaving messages left unread when I
had to focus on branch issues.

> The attached patch is much smaller, but
>
> (a) it fails to guarantee that it has authenticated every byte over
>     the network even if you ask for `pkg_add https://...' without any
>     security overrides; and
>
> (b) it is more likely to incur wider fallout because it changes the
>     semantics of the library, and the conditions that make it behave
>     differently depend not just on the local configuration but also on
>     the behaviour of remote servers.

(True, but as I have said it fixes the bug, and I think it's ok for
people who are relying on bugs having to adapt when they are fixed.)

> (For this patch, I used the environment variable SSL_NO_VERIFY_PEER to
> match what FreeBSD libfetch does, rather than a pkg_install.conf
> variable, in order to reduce the number of variations floating around
> out there.  Should maybe also support SSL_CA_CERT_FILE/PATH and
> SSL_CRL_FILE for compatibility but that can be future work, not a high
> priority.)

Totally happy to match FreeBSD's paint color on the environment variable
name.

Seeing that FreeBSD has done this makes me feel even more in favor of
it.  We're not just fixing a bug, we're stealing a bugfix from the
original upstream.

> From ee1977b2a9d6a3e0f9a78f9fea50523df2079a6d Mon Sep 17 00:00:00 2001
> From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
> Date: Sat, 9 Dec 2023 03:39:31 +0000
> Subject: [PATCH] net/libfetch: update to 2.40
>
> Validate HTTPS by default, unless environment variable
> SSL_NO_VERIFY_PEER is set.
>
> NOTE: This changes the semantics of the library in ways that may
> break the functionality of existing callers, even callers that don't
> ask to fetch HTTPS URLs.
> ---
>  net/libfetch/Makefile       | 3 +--
>  net/libfetch/files/common.c | 4 ++++
>  net/libfetch/files/fetch.3  | 6 +++++-
>  3 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/net/libfetch/Makefile b/net/libfetch/Makefile
> index e5cea0f4b830..66d034e66a9c 100644
> --- a/net/libfetch/Makefile
> +++ b/net/libfetch/Makefile
> @@ -1,7 +1,6 @@
>  # $NetBSD: Makefile,v 1.64 2023/10/24 22:10:22 wiz Exp $
>  
> -DISTNAME=	libfetch-2.39
> -PKGREVISION=	2
> +DISTNAME=	libfetch-2.40
>  CATEGORIES=	net
>  MASTER_SITES=	# empty
>  DISTFILES=	# empty
> diff --git a/net/libfetch/files/common.c b/net/libfetch/files/common.c
> index c1e158711636..bcb418d29918 100644
> --- a/net/libfetch/files/common.c
> +++ b/net/libfetch/files/common.c
> @@ -451,6 +451,10 @@ fetch_ssl(conn_t *conn, const struct url *URL, int verbose)
>  	conn->ssl_meth = SSLv23_client_method();
>  	conn->ssl_ctx = SSL_CTX_new(conn->ssl_meth);
>  	SSL_CTX_set_mode(conn->ssl_ctx, SSL_MODE_AUTO_RETRY);
> +	if (getenv("SSL_NO_VERIFY_PEER") == NULL) {
> +		SSL_CTX_set_default_verify_paths(conn->ssl_ctx);
> +		SSL_CTX_set_verify(conn->ssl_ctx, SSL_VERIFY_PEER, NULL);
> +	}
>  
>  	conn->ssl = SSL_new(conn->ssl_ctx);
>  	if (conn->ssl == NULL){
> diff --git a/net/libfetch/files/fetch.3 b/net/libfetch/files/fetch.3
> index bb58071dd088..45899eb3ee19 100644
> --- a/net/libfetch/files/fetch.3
> +++ b/net/libfetch/files/fetch.3
> @@ -27,7 +27,7 @@
>  .\" $FreeBSD: fetch.3,v 1.64 2007/12/18 11:03:26 des Exp $
>  .\" $NetBSD: fetch.3,v 1.17 2016/05/31 18:02:36 abhinav Exp $
>  .\"
> -.Dd January 22, 2010
> +.Dd December 22, 2023
>  .Dt FETCH 3
>  .Os
>  .Sh NAME
> @@ -638,6 +638,10 @@ which proxies should not be used.
>  Same as
>  .Ev NO_PROXY ,
>  for compatibility.
> +.It Ev SSL_NO_VERIFY_PEER
> +If defined,
> +.Nm
> +will skip validating certificates when fetching HTTPS URLs.
>  .El
>  .Sh EXAMPLES
>  To access a proxy server on

I am 100% happy with this being committed to pkgsrc-current.

I will float the concept on pkgsrc-users of pulling this up for all
systems.


I am also open to discussing prevent https->http downgrades unless
SSL_NO_VERIFY_PEER (for all libfetch uses, regardless of why), looking
into standards and other programs for context (but not absolute
guidance).  But I would prefer to defer that until this is dealt with
since it's more complicated.

Given that I've been the only real objection, and there is broad
agreement in concept, I'm fine with you commiting this right now.

Sorry for the confusion on my end; it has not been my best week.