Re: Cert validation in pkg_add

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

> Date: Sun, 17 Dec 2023 13:00:57 -0500
> From: Greg Troxel <gdt%lexort.com@localhost>
> 
> I read again.  My issues are:

Cool, thanks for the input!

>   This, while I know you see it as a bug fix, is a huge change in
>   existing practice.

Is this a huge change in existing practice?

Before:

- `pkg_add http://...' is quietly vulnerable to MITM attacks
- `pkg_add ftp://...' is quietly vulnerable to MITM attacks
- `pkg_add https://...' is quietly vulnerable to MITM attacks

After:

- `pkg_add http://...' is quietly vulnerable to MITM attacks
- `pkg_add ftp://...' is quietly vulnerable to MITM attacks
- `pkg_add https://...' rejects MITM attacks

In other words, there is no change when you write `pkg_add http://...'
(or use PKG_PATH=http://...).  So this won't affect, e.g., NetBSD 9
installations where the suggested PKG_PATH in /root/.profile is an
http:// URL.

The change affects only uses that _explicitly ask_ for secure
transport by writing https:// URLs, which, currently, pkg_add silently
fails to verify.

I think it is extremely surprising for `pkg_add https://...' to
silently skip any validation, to the point that it is correct to call
it a security vulnerability.  Do you disagree with this?

(Note: The first draft of the change rejected `pkg_add http://...'
unless you also set the new INSECURE_TRANSPORT=1 option.  The latest
draft does not affect `pkg_add http://...'.)

>   We have had one positive comment.  One person told me they would test
>   and comment, but haven't yet.  Another person in private mail was
>   vaguely positive but unconvinced about the rush.

The rush is to ensure that NetBSD 10.0 ships with working transport
validation for binary packages, probably the biggest source of
embarrassment for NetBSD security right now.

>   This seems to make libfetch reject ftp/http if V is on, but we
>   discussed changing semantics to only affecting https.  If a program
>   that uses libfetch wants pkix-validated methods only, it should filter
>   them.  Really I see this as wanting to change https to default to pkix
>   validation, and doing that via V because it would be an incompat
>   chagne.  This is blurring that flip with "reject ftp even though it
>   was asked for".  I expected in this round to have the fail-ftp code to
>   just vanish.

With this change, if you do `PKG_PATH=https://... pkg_add whatever',
you are guaranteed that the transport is all over validated HTTPS.

The reason the change still requires logic in libfetch ftp code is
that an HTTPS response could redirect to an ftp:// URL, and the
redirection logic is all inside libfetch.  Without that logic in
libfetch, pkg_add may still silently be vulnerable to insecure
transport.

>   I still do not like INSECURE_TRANSPORT as it seeks to frighten rather
>   than inform.  I think TLS_VALIDATE_CERTIFICTES=no should be settable,
>   with it defaulting to yes.  (Or really, the default being yes on
>   NetBSD 10 and no elsewhere, for now.)  That leads the reader to the
>   correct impression without docs.  I also think the variable should
>   just being about TLS validation.  A decision to reject http/ftp is
>   another thing, and if implemented at all (which I 95% think is bad)
>   should get another variable.  To me, disallowing http/ftp is a further
>   step beyond tls validation, and much more than a bug fix.

The change does not reject http:// and ftp:// URLs.

It only makes `pkg_add https://...' require valid certs and reject
_redirects_ to http:// and ftp:// URLs.

The new INSECURE_TRANSPORT option is only needed to allow:

- https:// URLs with invalid certs
- https:// URLs that redirect to http:// or ftp:// URLs

I think `insecure transport' is a more accurate description of this
group of scenarios than `TLS certificate validation disabled', since
it is about bypassing a general security guarantee that I'm proposing
`pkg_add https://...' should give by default about validating all data
received over the network.

I'm open to other names, but I would like it to emphasize that the
point is to bypass security guarantees, not just twiddle TLS knobs.

>   I don't see bumping the required pkg_install version in the patch.  I
>   think we might need that to get "pkg_add will be the same".  That
>   makes this more complicated.

Do you mean PKGTOOLS_REQD, as used for BOOTSTRAP_DEPENDS, in
mk/pkgformat/pkg/pkgformat-vars.mk?

Generally we advise against mixing TNF-built binary packages with
self-built pkgsrc, so I don't see the motivation for changing that.
Nothing _inside_ pkgsrc for building packages relies on the newer
pkg_install, as far as I can imagine.  Do you see a reason to change
this?

>   Do we have agreement from netbsd releng that this change is going to
>   be pulled up to netbsd-10 before release?  If not, then there's risk
>   to pkgsrc without gain.  I haven't see any comments from any of them
>   on this list.

I've asked martin@ to comment.

> I'll note that we could pull this up the branch later, after things have
> really settled and been tested.  I am really uncomfortable doing things
> last-minute and under time pressure, and that's what this feels like.
> Not your fault, that's what happens with time-based releases, a bit of a
> minus.  On the plus side, they actually happen!
> 
> 
> So I wonder about landing it in netbsd-10 and after it all works ok then
> I think it would be easy to get consensus to bring that change
> (conditioned on netbsd-10) into pkgsrc.

Landing it in netbsd-10 now and pkgsrc later is problematic because if
you use pkg_add on 10 to install a newer pkg_install binary package,
it will have the effect of downgrading and reintroducing the security
vulnerability.