Buffer overruns, license violations, and bad code: FreeBSD 13’s close call
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.
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
21This 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.