Buffer overruns, license violations, and bad code: FreeBSD 13's close call

[Litle Piggies]

At first glance, Matthew Macy seemed like a perfectly reasonable
choice to port WireGuard into the FreeBSD kernel. WireGuard is
an encrypted point-to-point tunneling protocol, part of what
most people think of as a "VPN." FreeBSD is a Unix-like
operating system that powers everything from Cisco and Juniper
routers to Netflix's network stack, and Macy had plenty of
experience on its dev team, including work on multiple network
drivers.

So when Jim Thompson, the CEO of Netgate, which makes FreeBSD-
powered routers, decided it was time for FreeBSD to enjoy the
same level of in-kernel WireGuard support that Linux does, he
reached out to offer Macy a contract. Macy would port WireGuard
into the FreeBSD kernel, where Netgate could then use it in the
company's popular pfSense router distribution. The contract was
offered without deadlines or milestones; Macy was simply to get
the job done on his own schedule.

With Macy's level of experience—with kernel coding and network
stacks in particular—the project looked like a slam dunk. But
things went awry almost immediately. WireGuard founding
developer Jason Donenfeld didn't hear about the project until it
surfaced on a FreeBSD mailing list, and Macy didn't seem
interested in Donenfeld's assistance when offered. After roughly
nine months of part-time development, Macy committed his
port—largely unreviewed and inadequately tested—directly into
the HEAD section of FreeBSD's code repository, where it was
scheduled for incorporation into FreeBSD 13.0-RELEASE.

This unexpected commit raised the stakes for Donenfeld, whose
project would ultimately be judged on the quality of any
production release under the WireGuard name. Donenfeld
identified numerous problems with Macy's code, but rather than
object to the port's release, Donenfeld decided to fix the
issues. He collaborated with FreeBSD developer Kyle Evans and
with Matt Dunwoodie, an OpenBSD developer who had worked on
WireGuard for that operating system. The three replaced almost
all of Macy's code in a mad week-long sprint.

This went over very poorly with Netgate, which sponsored Macy's
work. Netgate had already taken Macy's beta code from a FreeBSD
13 release candidate and placed it into production in pfSense's
2.5.0 release. The forklift upgrade performed by Donenfeld and
collaborators—along with Donenfeld's sharp characterization of
Macy's code—presented the company with a serious PR problem.

Netgate's public response included accusations of "irrational
bias against mmacy and Netgate" and irresponsible disclosure of
"a number of zero-day exploits"—despite Netgate's near-
simultaneous declaration that no actual vulnerabilities existed.

This combative response from Netgate raised increased scrutiny
from many sources, which uncovered surprising elements of Macy's
own past. He and his wife Nicole had been arrested in 2008 after
two years spent attempting to illegally evict tenants from a
small San Francisco apartment building the pair had bought.

Matthew Garrett
@mjg59
OH MY GOD THE PERSON WHO WROTE THE PFSENSE WIREGUARD CODE IS
THIS GUY
Exclusive: 'Landlord from Hell' Defends Terrorizing Apartment
Tenants
Kip Macy, 38, and his wife, Nicole Macy, 37, were deemed
"landlords of hell" by authorities for menacing the tenants of
their San Francisco apartment building.
abcnews.go.com
4:08 PM · Mar 16, 2021
459
33
Share this Tweet

The Macys' attempts to force their tenants out included sawing
through floor support joists to make the building unfit for
human habitation, sawing holes directly through the floors of
tenants' apartments, and forging extremely threatening emails
appearing to be from the tenants themselves. The couple fled to
Italy to avoid prosecution but were eventually extradited back
to the US—where they pled guilty to a reduced set of felonies
and served four years and four months each.

Macy's history as a landlord, unsurprisingly, dogged him
professionally—which contributed to his own lack of attention to
the doomed WireGuard port.

"I didn't even want to do this work," Macy eventually told us.
"I was burned out, spent many months with post-COVID syndrome...
I'd suffered through years of verbal abuse from non-doers and
semi-non-doers in the project whose one big one up on me is that
they aren't felons. I jumped at the opportunity to leave the
project in December... I just felt a moral obligation to get
[the WireGuard port] over the finish line. So you'll have to
forgive me if my final efforts were a bit half-hearted."

This admission answers why such an experienced, qualified
developer might produce inferior code—but it raises much larger
questions about process and procedure within the FreeBSD core
committee itself.

How did so much sub-par code make it so far into a major open
source operating system? Where was the code review which should
have stopped it? And why did both the FreeBSD core team and
Netgate seem more focused on the fact that the code was being
disparaged than its actual quality?

Code quality
The first issue is whether Macy's code actually had significant
problems. Donenfeld said that it did, and he identified a number
of major issues:

Sleep to mitigate race conditions
Validation functions which simply return true
Catastrophic cryptographic vulnerabilities
Pieces of the wg protocol left unimplemented
Kernel panics
Security bypasses
Printf statements deep in crypto code
"Spectacular" buffer overflows
Mazes of Linux?FreeBSD ifdefs

But Netgate argued that Donenfeld had gone overboard with his
negative assessment. The original Macy code, they argued, was
simply not that bad.

Despite not having any kernel developers on-staff, Ars was able
to verify at least some of Donenfeld's claims directly, quickly,
and without external assistance. For instance, finding a
validation function which simply returned true—and printf
statements buried deep in cryptographic loops—required nothing
more complicated than grep.

Empty validation function
In order to confirm or deny the claim of an empty validation
function—one which always "returns true" rather than actually
validating the data passed to it—we searched for instances of
return true or return (true) in Macy's if_wg code, as checked
into FreeBSD 13.0-HEAD.

root@banshee:~/macy-freebsd-wg/sys/dev/if_wg# grep -ir
'return.*true' . | wc -l
21
This is a small enough number of returns to easily hand-audit,
so we then used grep to find the same data but with three lines
of code coming immediately before and after each return true:

root@banshee:~/macy-freebsd-wg/sys/dev/if_wg# grep -ir -A3 -B3
'return.*true' .
Among the valid uses of return true, we discovered one empty
validation function, in module/module.c:

wg_allowedip_valid(const struct wg_allowedip *wip)
{

return (true);
}
It's probably worth mentioning that this empty validation
function is not buried at the bottom of a sprawling mass of
code—module.c as written is only 863 total lines of code.

We did not attempt to chase down the use of this function any
further, but it appears to be intended to check whether a
packet's source and/or destination belongs to WireGuard's
allowed-ips list, which determines what packets may be routed
down a given WireGuard tunnel.

Printf in crypto loops
Some pfSense 2.5.0 users reported strange hexadecimal output
spamming the root console of their router. This matches
Donenfeld's description of printf statements deep in crypto
code, and we were able to easily discover the source in much the
same way we found the empty validation function above.

root@banshee:~/macy-freebsd-wg/sys/dev/if_wg# grep -ir printf .
| wc -l
68
root@banshee:~/macy-freebsd-wg/sys/dev/if_wg# grep -ir printf
module/crypto | wc -l
7
root@banshee:~/macy-freebsd-wg/sys/dev/if_wg# grep -ir printf
module/crypto/zinc/chacha20poly1305.c
printf("src_len too short\n");
printf("calculated: %16D\n", b.mac, "");
printf("sent : %16D\n", src + dst_len, "");

root@banshee:~/macy-freebsd-wg/sys/dev/if_wg# grep -ir -A3 -B3
'printf("calc' module/crypto/zinc/chacha20poly1305.c
if (likely(!ret))
chacha20(&chacha20_state, dst, src, dst_len, simd_context);
else {
printf("calculated: %16D\n", b.mac, "");
printf("sent : %16D\n", src + dst_len, "");
}
memzero_explicit(&chacha20_state, sizeof(chacha20_state));
In mostly backend code like a WireGuard implementation, printf
statements in general are a bit of a red flag. There's obviously
nothing explicitly wrong or unsafe about emitting text to the
console—but it's something that's generally done rarely. One of
the most frequent uses of printf in this scenario is temporary
debugging, when a developer is trying to figure out what's going
wrong during the process of writing code.

Usually, when you're done with a bout of this "printf
debugging," you remove the printf statements you were using to
check data values along the way—and it's not uncommon to
specifically grep for stray instances of printf when a piece of
code is finished and ready for production, just to make certain
you've cleaned up after yourself.

The stray printf statements we found in Macy's
chacha20poly1305.c are clearly just such an example of temporary
debugging code. By themselves, they're a mild issue—indicative
mostly of sloppiness in getting a piece of code ready for prime-
time production use. The fact that those statements are still
being triggered frequently in production use is a more serious
issue—indicative of likely problems in the crypto implementation
itself.

A spectacular buffer overflow

Continued at link.

https://arstechnica.com/gadgets/2021/03/buffer-overruns-license-
violations-and-bad-code-freebsd-13s-close-call/