tech-pkg archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: Cert validation in pkg_add
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 564abbb4eb07f9253d914bb8cdf2bd6ff9a55cc1 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://..., 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.
---
pkgtools/pkg_install/Makefile | 1 -
pkgtools/pkg_install/files/admin/audit.c | 4 +-
pkgtools/pkg_install/files/lib/lib.h | 3 +-
pkgtools/pkg_install/files/lib/parse-config.c | 41 ++++++++++++++++++-
.../files/lib/pkg_install.conf.5.in | 9 ++++
pkgtools/pkg_install/files/lib/pkg_io.c | 6 +--
pkgtools/pkg_install/files/lib/version.h | 2 +-
7 files changed, 56 insertions(+), 10 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..63c7e978a173 100644
--- a/pkgtools/pkg_install/files/admin/audit.c
+++ b/pkgtools/pkg_install/files/admin/audit.c
@@ -317,13 +317,13 @@ fetch_pkg_vulnerabilities(int argc, char **argv)
"Could not parse location of pkg_vulnerabilities: %s",
fetchLastErrString);
- flags = fetch_flags;
+ flags = pkgvuln_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);
+ pkgvuln_fetch_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..94268fa0c6fb 100644
--- a/pkgtools/pkg_install/files/lib/lib.h
+++ b/pkgtools/pkg_install/files/lib/lib.h
@@ -460,7 +460,8 @@ extern const char *gpg_keyring_pkgvuln;
extern const char *gpg_keyring_sign;
extern const char *gpg_keyring_verify;
extern const char *gpg_sign_as;
-extern char fetch_flags[];
+extern const char *pkg_fetch_flags;
+extern const char *pkgvuln_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..9e6228cb20ce 100644
--- a/pkgtools/pkg_install/files/lib/parse-config.c
+++ b/pkgtools/pkg_install/files/lib/parse-config.c
@@ -57,7 +57,10 @@ 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 */
+static char fetch_flags[10] = ""; /* Workaround Mac OS X linker issues with BSS */
+static char insecure_fetch_flags[10] = "";
+const char *pkg_fetch_flags;
+const char *pkgvuln_fetch_flags;
static const char *active_ftp;
static const char *verbose_netio;
static const char *ignore_proxy;
@@ -84,6 +87,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 +115,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 },
@@ -175,6 +180,7 @@ void
pkg_install_config(void)
{
int do_cache_index;
+ int verify_transport = 1;
char *value;
parse_pkg_install_conf();
@@ -244,11 +250,42 @@ 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" : "");
+ snprintf(fetch_flags, sizeof(fetch_flags), "%s%s",
+ insecure_fetch_flags,
+ (insecure_transport && *insecure_transport) ? "" : "V");
+
+ /*
+ * If the user specified an ftp:// or http:// URL in PKG_PATH,
+ * allow insecure transport to download packages. Otherwise,
+ * require valid HTTPS certificates to download packages.
+ */
+ if (config_pkg_path != NULL &&
+ (strncasecmp(config_pkg_path, "ftp://";, strlen("ftp://";)) == 0 ||
+ strncasecmp(config_pkg_path, "http://";,
+ strlen("http://";)) == 0))
+ pkg_fetch_flags = insecure_fetch_flags;
+ else
+ pkg_fetch_flags = fetch_flags;
+
+ /*
+ * If the user specified an ftp:// or http:// URL in
+ * PKGVULNURL, allow insecure transport to download
+ * pkg-vulnerabilities. Otherwise, require valid HTTPS
+ * certificates to download pkg-vulnerabilities.
+ */
+ if (strncasecmp(pkg_vulnerabilities_url, "ftp://";,
+ strlen("ftp://";)) == 0 ||
+ strncasecmp(pkg_vulnerabilities_url, "http://";,
+ strlen("http://";)) == 0)
+ pkgvuln_fetch_flags = insecure_fetch_flags;
+ else
+ pkgvuln_fetch_flags = fetch_flags;
}
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..7f0a2e04d9b7 100644
--- a/pkgtools/pkg_install/files/lib/pkg_io.c
+++ b/pkgtools/pkg_install/files/lib/pkg_io.c
@@ -80,7 +80,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);
if (f->fetch == NULL)
return ENOENT;
f->size = us.size;
@@ -118,7 +118,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);
if (f->fetch == NULL)
return -1;
if (us.size != f->size)
@@ -255,7 +255,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)) {
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_ */
Home |
Main Index |
Thread Index |
Old Index