Re: Cert validation in pkg_add

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

Take 3:

- For http/ftp URL of a package on command-line, or of package
  repository in semicolon-separated PKG_PATH entry, no change.

- When pkg_add fetches via https URL of package on command-line, or of
  package repository in PKG_PATH, it requires secure transport --
  i.e., requires valid TLS certs and refuses redirect to http/ftp.

- Same deal with pkg_admin audit and PKGVULNURL.

(Change from previous: The rule applies to each semicolon-separated
entry in PKG_PATH, and to the command line, not just to the first
entry in PKG_PATH.)

I also added a build-time conditional on NetBSD>=10 so that this
doesn't affect any other platforms for now -- we can easily remove
that after the 2023Q4 branch.


> Date: Sat, 9 Dec 2023 16:29:46 +0000
> From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
> 
> New proposal:
> 
> - If you set PKG_PATH=ftp://... or http://..., no change to pkg_add.
> 
>   So existing use cases of _explicitly requested_ FTP or HTTP for a
>   package repository continue to work.
> 
> - If you set PKG_PATH=https://... (or file://..., though it doesn't
>   matter here), pkg_add requires secure transport, i.e., requires
>   valid TLS certs and refuses redirect to http/ftp.
> 
>   So https does what it says on the box.  But you can restore the
>   previous non-validating bug by setting INSECURE_TRANSPORT=1.
> 
> (Same applies to PKGVULNURL and pkg_admin audit in new patch.  This
> covers all cases of calls to libfetch in pkg_install.  Will update
> pkgin patch to work similarly.)
> 
> The cases this will break are:
> 1. self-signed certs (solution: Let's Encrypt or custom trust anchors)
> 2. intentional redirect from authenticated https to http/ftp
> 3. asking for https:// on systems without trust anchors
> I think these cases are obscure and not worth worrying about, and if
> you really need them to work you can set INSECURE_TRANSPORT=1.
> 
> Better?
> 
> 
> Some small points below:
> 
> > Date: Sat, 09 Dec 2023 07:34:12 -0500
> > From: Greg Troxel <gdt%lexort.com@localhost>
> > 
> > Taylor R Campbell <riastradh%NetBSD.org@localhost> writes:
> > 
> > > 1. New libfetch flag `V' to enable validation.
> > >
> > >    This is an opt-in so that existing clients of the _library_ don't
> > >    break -- no change to library semantics.
> > 
> > opt-in seems fine.  But it looks like the patch changes the signature of
> > an existing public function, so I'd expect this requires a shlib major
> > bump of libfetch, and accomodation in all users, and I don't understand
> > either how that's being done or why it isn't necessary.
> 
> This isn't a public function -- it's an internal function that is used
> between different .c files inside libfetch.  It may be accidentally
> exported by the .so because nobody has gotten around to sprinkling
> visibility attributes.  But it's not declared in any public header
> file so if any applications are linked against it, they get what they
> deserve.
> 
> As a side note, this function has already diverged between NetBSD base
> and pkgsrc -- it has an extra argument in pkgsrc to pass through the
> URL (although I don't think that's necessary since it's available
> through conn->cache_url).
> 
> > > We could, of course, apply the changes to NetBSD first, and then
> > > pkgsrc, rather than the other way around, if pkgsrc-releng considers
> > > this change too risky for pkgsrc so close to branch-time.
> > 
> > How are you proposing to deal with this for systems that do and don't
> > have trust anchors configured?  I get it that on NetBSD 10, trust
> > anchors are configured in base.  But on older NetBSD -- and who knows
> > about the other ~20 platforms, it's hard to tell.
> 
> 1. Use PKG_PATH=http:// or ftp:// instead.
> 2. Install trust anchors.
> 3. Set INSECURE_TRANSPORT=1 in pkg_install.conf.
> 
> It's a severe bug, meaning a mismatch between reasonable expectations
> and actual implementation leading to user astonishment of exploitable
> security flaws, for pkg_add to quietly accept https:// URLs without
> any cert validation.
> 
> And for _automation_ to rely on that is worse.  So it's _good_ if this
> has the effect of requiring manual intervention for systems where you
> _thought_ you were getting the security of https but were not.
> 
> > (A nit but stability heading to branch is pkgsrc-pmc.  Acceptability of
> > pullups is pkgsrc-releng, in theory with pmc oversight, but I have no
> > memory of *ever* needing to discuss a decision that was made my
> > pkgsrc-releng.)
> 
> Oops, yes, that's what I meant.
> 
> > I don't think it's wise to have different code in NetBSD base
> > pkg_install and in pkgsrc pkg_install, as that will cause behavior
> > changes when flipping between them, which could happen due to unrelated
> > fixes in pkg_install.
> 
> Yes, I guess it would be bad if upgrading pkg_install from pkgsrc had
> the effect of downgrading security, so let's not do that.
> 
> > My quick reaction is that the default to validate TLS is easy to revert
> > if trouble.  When guarded by an ifdef on NetBSD 10, then it addresses
> > what I think you most care about, while not impacting the rest of the
> > systems where we don't have a good handle on the impacts.
> 
> That seems fine to me as a band-aid that we rip off next branch.  Can
> put some #if NetBSD 10 conditionals in the next round of the patch.
> 
> > Not quite on topic, but related to why this is perceived as urgent: I'm
> > not really clear on why we don't have  signed packages.  I see it as
> > "key management is hard" and we don't have a story for supercession and
> > revocation, distribution of trust anchors, etc., for the keys used or
> > signing.
> 
> That is accurate.  Also the package signing system we have has design
> flaws that weaken its security properties (DoS, selective rollback)
> and make key management harder (no thresholds so migration is harder).
> 
> I have some draft work in progress to address all of this but my tuit
> supply is low so I'm prioritizing low-hanging fruit first.

From 217a0b07cc4a7b362fd0d44fd2be429e9a012a68 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 1/2] net/libfetch: update to 2.40

New flag `V' to require verification of transport.  This blocks HTTPS
without a valid certificate, and blocks HTTP and FTP.

No change to semantics for existing clients of libfetch.
---
 net/libfetch/Makefile       |  3 +--
 net/libfetch/files/common.c |  6 +++++-
 net/libfetch/files/common.h |  2 +-
 net/libfetch/files/fetch.3  | 10 +++++++++-
 net/libfetch/files/ftp.c    | 11 ++++++++++-
 net/libfetch/files/http.c   | 13 +++++++++++--
 6 files changed, 37 insertions(+), 8 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..507e2ee81404 100644
--- a/net/libfetch/files/common.c
+++ b/net/libfetch/files/common.c
@@ -436,7 +436,7 @@ fetch_cache_put(conn_t *conn, int (*closecb)(conn_t *))
  * Enable SSL on a connection.
  */
 int
-fetch_ssl(conn_t *conn, const struct url *URL, int verbose)
+fetch_ssl(conn_t *conn, const struct url *URL, int verbose, int verify)
 {
 
 #ifdef WITH_SSL
@@ -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 (verify) {
+		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/common.h b/net/libfetch/files/common.h
index 9a07e3560c8e..43c92e1f2061 100644
--- a/net/libfetch/files/common.h
+++ b/net/libfetch/files/common.h
@@ -106,7 +106,7 @@ conn_t		*fetch_cache_get(const struct url *, int);
 void		 fetch_cache_put(conn_t *, int (*)(conn_t *));
 conn_t		*fetch_connect(struct url *, int, int);
 conn_t		*fetch_reopen(int);
-int		 fetch_ssl(conn_t *, const struct url *, int);
+int		 fetch_ssl(conn_t *, const struct url *, int, int);
 ssize_t		 fetch_read(conn_t *, char *, size_t);
 int		 fetch_getln(conn_t *);
 ssize_t		 fetch_write(conn_t *, const void *, size_t);
diff --git a/net/libfetch/files/fetch.3 b/net/libfetch/files/fetch.3
index bb58071dd088..35e214d223bf 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 8, 2023
 .Dt FETCH 3
 .Os
 .Sh NAME
@@ -357,6 +357,14 @@ functions is read-only, and that a stream returned by one of the
 functions is write-only.
 .Sh PROTOCOL INDEPENDENT FLAGS
 If the
+.Ql V
+(verify) flag is specified, the library will refuse to fetch from URLs
+that can't be cryptographically verified.
+This limits
+.Nm
+to HTTPS URLs fetched from servers with valid certificates.
+.Pp
+If the
 .Ql i
 (if-modified-since) flag is specified, the library will try to fetch
 the content only if it is newer than
diff --git a/net/libfetch/files/ftp.c b/net/libfetch/files/ftp.c
index 8f3d576526d6..099b4a6826bd 100644
--- a/net/libfetch/files/ftp.c
+++ b/net/libfetch/files/ftp.c
@@ -1034,7 +1034,7 @@ static conn_t *
 ftp_connect(struct url *url, struct url *purl, const char *flags)
 {
 	conn_t *conn;
-	int e, direct, verbose;
+	int e, direct, verbose, verify;
 #ifdef INET6
 	int af = AF_UNSPEC;
 #else
@@ -1043,11 +1043,20 @@ ftp_connect(struct url *url, struct url *purl, const char *flags)
 
 	direct = CHECK_FLAG('d');
 	verbose = CHECK_FLAG('v');
+	verify = CHECK_FLAG('V');
 	if (CHECK_FLAG('4'))
 		af = AF_INET;
 	else if (CHECK_FLAG('6'))
 		af = AF_INET6;
 
+	if (verify) {
+		/* can't verify FTP */
+		fetchLastErrCode = FETCH_AUTH;
+		snprintf(fetchLastErrString, MAXERRSTRING,
+		    "unable to authenticate %s:// URLs", url->scheme);
+		return (NULL);
+	}
+
 	if (direct)
 		purl = NULL;
 
diff --git a/net/libfetch/files/http.c b/net/libfetch/files/http.c
index c2133dbd38f6..e562056f18e7 100644
--- a/net/libfetch/files/http.c
+++ b/net/libfetch/files/http.c
@@ -723,7 +723,7 @@ http_connect(struct url *URL, struct url *purl, const char *flags, int *cached)
 	conn_t *conn;
 	hdr_t h;
 	const char *p;
-	int af, verbose;
+	int af, verbose, verify;
 #if defined(TCP_NOPUSH) && !defined(__APPLE__)
 	int val;
 #endif
@@ -737,6 +737,7 @@ http_connect(struct url *URL, struct url *purl, const char *flags, int *cached)
 #endif
 
 	verbose = CHECK_FLAG('v');
+	verify = CHECK_FLAG('V');
 	if (CHECK_FLAG('4'))
 		af = AF_INET;
 #ifdef INET6
@@ -746,6 +747,14 @@ http_connect(struct url *URL, struct url *purl, const char *flags, int *cached)
 
 	curl = (purl != NULL) ? purl : URL;
 
+	if (verify && strcasecmp(URL->scheme, SCHEME_HTTPS) != 0) {
+		/* can't verify non-HTTPS */
+		fetchLastErrCode = FETCH_AUTH;
+		snprintf(fetchLastErrString, MAXERRSTRING,
+		    "unable to authenticate %s:// URLs", URL->scheme);
+		return (NULL);
+	}
+
 	if ((conn = fetch_cache_get(URL, af)) != NULL) {
 		*cached = 1;
 		return (conn);
@@ -783,7 +792,7 @@ http_connect(struct url *URL, struct url *purl, const char *flags, int *cached)
 		} while (h < hdr_end);
 	}
 	if (strcasecmp(URL->scheme, SCHEME_HTTPS) == 0 &&
-	    fetch_ssl(conn, URL, verbose) == -1) {
+	    fetch_ssl(conn, URL, verbose, verify) == -1) {
 		/* grrr */
 #ifdef EAUTH
 		errno = EAUTH;

From e6520a55c995521e4f2fdf6b61f472247054b1aa Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Sat, 9 Dec 2023 03:43:14 +0000
Subject: [PATCH 2/2] pkg_install: update to 20231208

If you use PKG_PATH=https://..., or you pass an https://... URL on
the command line to identify a package, require secure transport by
default when downloading packages (i.e., validate certs and refuse
redirect to ftp/http) unless you set the new pkg_install.conf option
INSECURE_TRANSPORT=1.  Same with PKGVULNURL and downloading
pkg-vulnerabilities.

If you use PKG_PATH=http://... or ftp://..., or same for PKGVULNURL,
no change.  (This applies to each entry in the semicolon-separated
PKG_PATH list independently: if you use `http://foo;https://bar',
then validation is only applied to https://bar.)
---
 pkgtools/pkg_install/Makefile                 |  1 -
 pkgtools/pkg_install/files/admin/audit.c      | 15 ++++++++++-
 pkgtools/pkg_install/files/lib/lib.h          |  1 +
 pkgtools/pkg_install/files/lib/parse-config.c | 18 ++++++++++++-
 .../files/lib/pkg_install.conf.5.in           |  9 +++++++
 pkgtools/pkg_install/files/lib/pkg_io.c       | 26 ++++++++++++++++---
 pkgtools/pkg_install/files/lib/version.h      |  2 +-
 7 files changed, 65 insertions(+), 7 deletions(-)

diff --git a/pkgtools/pkg_install/Makefile b/pkgtools/pkg_install/Makefile
index 1a2b975bf37f..2d6812433c29 100644
--- a/pkgtools/pkg_install/Makefile
+++ b/pkgtools/pkg_install/Makefile
@@ -7,7 +7,6 @@
 # change in the pkg_* tools that pkgsrc relies on for proper operation.
 
 PKGNAME=		pkg_install-${VERSION}
-PKGREVISION=		1
 CATEGORIES=		pkgtools
 
 MAINTAINER=		agc%NetBSD.org@localhost
diff --git a/pkgtools/pkg_install/files/admin/audit.c b/pkgtools/pkg_install/files/admin/audit.c
index a3b445ef5703..333b782b9f41 100644
--- a/pkgtools/pkg_install/files/admin/audit.c
+++ b/pkgtools/pkg_install/files/admin/audit.c
@@ -318,12 +318,25 @@ fetch_pkg_vulnerabilities(int argc, char **argv)
 		    fetchLastErrString);
 
 	flags = fetch_flags;
+
+	/*
+	 * If the user specified PKGVULNURL=http://... or ftp://... (or
+	 * if that is the default), enable insecure transport to
+	 * download it -- this way we don't break existing setups that
+	 * never expected secure transport in the first place.
+	 *
+	 * If you want secure transport, use https or file URLs.
+	 */
+	if (strcasecmp(url->scheme, SCHEME_HTTP) == 0 ||
+	    strcasecmp(url->scheme, SCHEME_FTP))
+		flags = insecure_fetch_flags;
+
 	if (update_pkg_vuln) {
 		fd = open(pkg_vulnerabilities_file, O_RDONLY);
 		if (fd != -1 && fstat(fd, &sb) != -1) {
 			url->last_modified = sb.st_mtime;
 			snprintf(my_flags, sizeof(my_flags), "%si",
-			    fetch_flags);
+			    flags);
 			flags = my_flags;
 		} else
 			update_pkg_vuln = 0;
diff --git a/pkgtools/pkg_install/files/lib/lib.h b/pkgtools/pkg_install/files/lib/lib.h
index 6a825d4b5834..b55dd6d258bb 100644
--- a/pkgtools/pkg_install/files/lib/lib.h
+++ b/pkgtools/pkg_install/files/lib/lib.h
@@ -461,6 +461,7 @@ extern const char *gpg_keyring_sign;
 extern const char *gpg_keyring_verify;
 extern const char *gpg_sign_as;
 extern char fetch_flags[];
+extern char insecure_fetch_flags[];
 
 extern const char *pkg_vulnerabilities_dir;
 extern const char *pkg_vulnerabilities_file;
diff --git a/pkgtools/pkg_install/files/lib/parse-config.c b/pkgtools/pkg_install/files/lib/parse-config.c
index 107466a1b4ca..7900cc422a1f 100644
--- a/pkgtools/pkg_install/files/lib/parse-config.c
+++ b/pkgtools/pkg_install/files/lib/parse-config.c
@@ -58,6 +58,7 @@ static int cache_connections_host = 4;
 const char     *config_file = SYSCONFDIR"/pkg_install.conf";
 
 char fetch_flags[10] = ""; /* Workaround Mac OS X linker issues with BSS */
+char insecure_fetch_flags[10] = "";
 static const char *active_ftp;
 static const char *verbose_netio;
 static const char *ignore_proxy;
@@ -84,6 +85,7 @@ const char *pkg_vulnerabilities_dir;
 const char *pkg_vulnerabilities_file;
 const char *pkg_vulnerabilities_url;
 const char *ignore_advisories = NULL;
+const char *insecure_transport = NULL;
 const char tnf_vulnerability_base[] = "http://cdn.NetBSD.org/pub/NetBSD/packages/vulns";;
 const char *acceptable_licenses = NULL;
 
@@ -111,6 +113,7 @@ static struct config_variable {
 	{ "GPG_SIGN_AS", &gpg_sign_as },
 	{ "IGNORE_PROXY", &ignore_proxy },
 	{ "IGNORE_URL", &ignore_advisories },
+	{ "INSECURE_TRANSPORT", &insecure_transport },
 	{ "PKG_DBDIR", &config_pkg_dbdir },
 	{ "PKG_PATH", &config_pkg_path },
 	{ "PKG_REFCOUNT_DBDIR", &config_pkg_refcount_dbdir },
@@ -244,11 +247,24 @@ pkg_install_config(void)
 	fetchConnectionCacheInit(cache_connections, cache_connections_host);
 #endif
 
-	snprintf(fetch_flags, sizeof(fetch_flags), "%s%s%s%s",
+	snprintf(insecure_fetch_flags, sizeof(insecure_fetch_flags),
+	    "%s%s%s%s",
 	    (do_cache_index) ? "c" : "",
 	    (verbose_netio && *verbose_netio) ? "v" : "",
 	    (active_ftp && *active_ftp) ? "a" : "",
 	    (ignore_proxy && *ignore_proxy) ? "d" : "");
+#if defined(__NetBSD__) && __NetBSD_Version__ >= 1000000000
+	snprintf(fetch_flags, sizeof(fetch_flags), "%s%s",
+	    insecure_fetch_flags,
+	    (insecure_transport && *insecure_transport) ? "" : "V");
+#else
+	/*
+	 * Provisional until after pkgsrc-2023Q4 is branched -- don't
+	 * require secure transports by default except on NetBSD 10.
+	 */
+	snprintf(fetch_flags, sizeof(fetch_flags), "%s",
+	    insecure_fetch_flags);
+#endif
 }
 
 void
diff --git a/pkgtools/pkg_install/files/lib/pkg_install.conf.5.in b/pkgtools/pkg_install/files/lib/pkg_install.conf.5.in
index fb9d2cb70fde..ff74f0d03379 100644
--- a/pkgtools/pkg_install/files/lib/pkg_install.conf.5.in
+++ b/pkgtools/pkg_install/files/lib/pkg_install.conf.5.in
@@ -159,6 +159,15 @@ One line per advisory which should be ignored when running
 The URL from the
 .Pa pkg-vulnerabilities
 file should be used as value.
+.It Dv INSECURE_TRANSPORT
+Allow downloads over insecure transport, bypassing certificate
+validation, even if
+.Dv PKG_PATH
+or
+.Dv PKGVULNURL
+is set to an
+.Ql https://
+URL.
 .It Dv PKG_DBDIR (*)
 Location of the packages database.
 This option is always overriden by the argument of the
diff --git a/pkgtools/pkg_install/files/lib/pkg_io.c b/pkgtools/pkg_install/files/lib/pkg_io.c
index e961e8147420..4ac83a5b8ada 100644
--- a/pkgtools/pkg_install/files/lib/pkg_io.c
+++ b/pkgtools/pkg_install/files/lib/pkg_io.c
@@ -65,6 +65,26 @@ struct pkg_path {
 static char *orig_cwd, *last_toplevel;
 static TAILQ_HEAD(, pkg_path) pkg_path = TAILQ_HEAD_INITIALIZER(pkg_path);
 
+static const char *
+pkg_fetch_flags(const struct url *url)
+{
+
+	/*
+	 * If the user specified PKG_PATH=http://... or ftp://..., or
+	 * passed an http/ftp URL on the command line of a package to
+	 * install, enable insecure transport to download it -- this
+	 * way we don't break existing setups that never expected
+	 * secure transport in the first place.
+	 *
+	 * If you want secure transport, use https or file URLs.
+	 */
+	if (strcasecmp(url->scheme, SCHEME_HTTP) == 0 ||
+	    strcasecmp(url->scheme, SCHEME_FTP) == 0)
+		return insecure_fetch_flags;
+
+	return fetch_flags;
+}
+
 #ifndef BOOTSTRAP
 struct fetch_archive {
 	struct url *url;
@@ -80,7 +100,7 @@ fetch_archive_open(struct archive *a, void *client_data)
 	struct fetch_archive *f = client_data;
 	struct url_stat us;
 
-	f->fetch = fetchXGet(f->url, &us, fetch_flags);
+	f->fetch = fetchXGet(f->url, &us, pkg_fetch_flags(f->url));
 	if (f->fetch == NULL)
 		return ENOENT;
 	f->size = us.size;
@@ -118,7 +138,7 @@ fetch_archive_read(struct archive *a, void *client_data,
 		free(url);
 	}
 	fetchIO_close(f->fetch);
-	f->fetch = fetchXGet(f->url, &us, fetch_flags);
+	f->fetch = fetchXGet(f->url, &us, pkg_fetch_flags(f->url));
 	if (f->fetch == NULL)
 		return -1;
 	if (us.size != f->size)
@@ -255,7 +275,7 @@ find_best_package_int(struct url *url, const char *pattern,
 	url_pattern = xasprintf("%*.*s*", (int)i, (int)i, pattern);
 
 	fetchInitURLList(&ue);
-	if (fetchList(&ue, url, url_pattern, fetch_flags)) {
+	if (fetchList(&ue, url, url_pattern, pkg_fetch_flags(url))) {
 		char *base_url;
 		base_url = fetchStringifyURL(url);
 		warnx("Can't process %s/%s: %s", base_url, url_pattern,
diff --git a/pkgtools/pkg_install/files/lib/version.h b/pkgtools/pkg_install/files/lib/version.h
index 9bd3ab0a0def..c445d83d26c6 100644
--- a/pkgtools/pkg_install/files/lib/version.h
+++ b/pkgtools/pkg_install/files/lib/version.h
@@ -27,6 +27,6 @@
 #ifndef _INST_LIB_VERSION_H_
 #define _INST_LIB_VERSION_H_
 
-#define PKGTOOLS_VERSION 20211115
+#define PKGTOOLS_VERSION 20231208
 
 #endif /* _INST_LIB_VERSION_H_ */