Re: "wireguard" implementation improperly merged and needs revert

["Jason A. Donenfeld" <Jason%zx2c4.com@localhost>]

Hi Maxime,

On Mon, Aug 24, 2020 at 4:37 PM Maxime Villard <max%m00nbsd.net@localhost> wrote:
>
> 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.

Thanks, I'll pass that onto the OpenBSD networking guys if you haven't
already done so.

It wouldn't surprise me if there are kernel programming bugs of that
sort; that code is new, and stabilization is happening in the OpenBSD
tree (I hope?). Bugs like that are squarely within the realm of issues
that are incrementally fixed with successive code reviews. Were
NetBSD's "wireguard" implementation in that situation, I'd feel fine
about it, and just put my time into doing usual security audits and
submitting boring patches and such. But that's not my concern here
with the code. But rather:

>
> > [...]
> > 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.

But rather, this is my concern -- that the issue isn't just a TODO
list of little bugs here and there. I really don't want to get drawn
into that kind of mailing list rabbit hole. And I don't think that
anybody here seriously considers me "uninformed" on the particulars of
WireGuard. These mailing list discussions are windy and thorny, and
asking people to revert stuff is already a huge party foul. And as you
can see, I got baited into responding to totally irrelevant discussion
on "Mozilla's lawyers" or something? Rather, I'm trying to keep this
really focused: I think the code was merged prematurely, but if you're
serious about having a WireGuard implementation, I'm serious about
helping, so let's move this to a development branch and revert it from
the main one, and then we can go head first into that.

> 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.

Cool. Whatever the SCM particulars wind up being is fine with me. I
was mostly just curious at the mention of branches in this context
because I know a lot of cvs-based projects avoid them.

Anyway, it seems like this thread is at risk of veering dangerously
off course into something a lot more disruptive and provocative than
initially planned. I hope we can just throw this in a branch, and then
Taylor and I can dive into it heads-down come September, and deliver
something that works great for everyone.

Jason