Re: "wireguard" implementation improperly merged and needs revert

[Maxime Villard <max%m00nbsd.net@localhost>]

Le 24/08/2020 à 13:47, Jason A. Donenfeld a écrit :
> > (4) Considering the overall poor quality of code coming out of OpenBSD, I
> >     wouldn't feel confident with us importing their code, whether it be wg,
> >     or anything else at large.
>
> Again, I'm not sure this is a relevant technical point, and I *really*
> don't want to wade into BSD political rivalries on a mailing list like
> that. But I will say that neither Matt nor myself are actual OpenBSD
> developers, so maybe you don't need to really have concern about the
> pedigree.

I've given a 5-minute look at OpenBSD's if_wg.c file considered "production-
ready". 40 lines of code into the wg_input() entry point it looks like there
is already a vuln:

-	if (m_defrag(m, M_NOWAIT) != 0)
+	if (m_defrag(m, M_NOWAIT) != 0) {
+		m_freem(m);
		return NULL;
+	}

M_NOWAIT means the allocation can fail, if it fails 'm' needs to be freed,
otherwise it is leaked. The leak here means remote DoS of the kernel,
triggerable without authentication.

> [...]
> If you haven't noticed, I'm trying very hard not to get bogged down in
> classic mailing list errors of secumbing to, "oh, there's a problem?
> make a list of bugs for us and we'll prioritize them and then fix them
> one by one," because that _entirely_ misses the point here, and the
> current issues won't be coherently addressed in that manner.
> [...]

Sure, I understand. However, it is good to put some substance in the
criticism, at least as a clue, for people not to dismiss your remarks
as uninformed rant.

> [...]
> Similarly, "but the code works for me for years, it sends packets"
> also misses the point entirely. The right thing to do is back up the
> truck a bit and reapproach the problem space more carefully.
> [...]

I understand your concern.

> [...]
> Actually, yes, sure. If you want to put this in a "wgdev" branch, or
> some such name, and move it out of the master development branch,
> that'd be productive and also give us some more room to unpack things
> -- specifically, a wg-specific feature branch, rather than a "next
> version" branch or something. When I asked that you revert the code, I
> didn't mean, "revert it and place it in the dumpster", but was more
> thinking of moving it to a development tree somewhere, because with
> Linux development, things usually work with individual developer
> trees. If NetBSD has feature-development branches within its main
> repository, that works well.I wouldn't want to totally discard
> Taylor's improvements on Ozaki-san's code, especially as that might
> wind up being the basis of our work on this moving forward. In other
> words, wherever it makes sense to develop it, let's do that, so long
> as we merge it to the master tree *not before* it's ready. (With that
> said, is a cvs branch what you had in mind? Or does NetBSD have a
> pretty established flow for git/hg feature development branches
> instead?)

Our main tree is cvs, but we are migrating to hg (anonhg.netbsd.org), and
also mirror our tree on github. So three options exist, but Taylor will
probably know better than me what's the best option.

As Manuel said, generally, for subsystems that are small and self-contained,
CVS-HEAD is the preferred development branch, which is why wg went there in
the first place.

But I don't think there is difficulty in moving the code to a separate
branch if you like it better there. What do Taylor/Ryota think?

Maxime