tech-pkg archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Cert validation in pkg_add
Apparently pkg_add doesn't do cert validation for remote PKG_PATH.
This is embarrassing.
tl;dr: I propose to enable cert validation in pkg_add by default.
Objections?
DETAILS
One can argue that (a) packages should be signed at the origin rather
than in the transport, (b) you should be using pkgin rather than
pkg_add, (c) you can just make your own packages and install them via
file:// PKG_PATHs mounted with psshfs, (d) etc.
But there's a lot of man pages and docs and copypasta out there to
just use https://cdn.NetBSD.org/... or similar and I expect most users
would be astonished (if not disgusted) that this doesn't do
validation, not even when they have trust anchor certs installed.
Certainly it came as a surprise to me when I discovered this today.
And while the CDN isn't the TNF origin, transport authentication goes
a long way toward plugging the gaping security hole here, with much
less effort because almost all the infrastructure is already there.
Plus even if you authenticate the origin with package signatures,
there's a huge attack surface of complicated data transports and
formats to get at the package signatures in the first place, which
transport authentication helps to avoid.
So I propose the attached patch to implement the following:
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.
2. pkg_add does cert validation by default, by setting `V' flag in
libfetch. New option INSECURE_TRANSPORT=1 in pkg_install.conf to
allow insecure transports.
This is an opt-out because the reasonable modern expectation for
applications is to do cert validation out of the box -- and while
pkg_add has a bit of a dual purpose as a library subroutine and a
user interface, its nature as a user interface is baked into a lot
of documentation and fingers.
The option name is intentionally scary-sounding so you're tempted
to fix whatever symptom leads you to try it first before you are
tempted to enable the insecure option.
I realize it's late in the branch cycle, but it would be embarrassing
for this not to be fixed in NetBSD 10 (whose release is imminent) now
that we have certs set up by default in base.
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.
Thoughts?
P.S. Separate patch will be coming for pkgin to do the same thing as
pkg_add here, using the same libfetch flag. As above, it would
be good to do origin authentication, but pkgin currently does a
loooot of exciting parsing of things like pkg_summary.bz2 which
is currently totally unauthenticated before it can even look at
package signatures.
And I would like to see that attack surface reduced by having a
way to authenticate pkg_summary.bz2 at the origin too, with other
advantages like preventing selective package rollback attacks --
but this takes more planning and design and implementation and
it's not going to get done in the near future.
>From 98dae898223dfbc38cfd95f991cd798e7e5495e5 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 | 9 ++++++++-
net/libfetch/files/http.c | 11 +++++++++--
6 files changed, 33 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..7387e701b113 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,18 @@ 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;
+ return (NULL);
+ }
+
if (direct)
purl = NULL;
diff --git a/net/libfetch/files/http.c b/net/libfetch/files/http.c
index c2133dbd38f6..0991a851015a 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,12 @@ 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;
+ return (NULL);
+ }
+
if ((conn = fetch_cache_get(URL, af)) != NULL) {
*cached = 1;
return (conn);
@@ -783,7 +790,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 f699a30be625ad2a13aee923741148f5250a5473 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
Require secure transport by default -- HTTPS with cert validation
only, no FTP/HTTP. New pkg_install.conf option INSECURE_TRANSPORT=1
allows insecure transports.
---
pkgtools/pkg_install/files/lib/parse-config.c | 5 ++++-
pkgtools/pkg_install/files/lib/version.h | 2 +-
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/pkgtools/pkg_install/files/lib/parse-config.c b/pkgtools/pkg_install/files/lib/parse-config.c
index 107466a1b4ca..9b674aa8085c 100644
--- a/pkgtools/pkg_install/files/lib/parse-config.c
+++ b/pkgtools/pkg_install/files/lib/parse-config.c
@@ -84,6 +84,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 +112,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,7 +246,8 @@ pkg_install_config(void)
fetchConnectionCacheInit(cache_connections, cache_connections_host);
#endif
- snprintf(fetch_flags, sizeof(fetch_flags), "%s%s%s%s",
+ snprintf(fetch_flags, sizeof(fetch_flags), "%s%s%s%s%s",
+ (insecure_transport && *insecure_transport) ? "" : "V",
(do_cache_index) ? "c" : "",
(verbose_netio && *verbose_netio) ? "v" : "",
(active_ftp && *active_ftp) ? "a" : "",
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