For those not-so-deep inside the sheepfencing Netgate story: Gonzopancho is Jim Thompsonhttps://www.linkedin.com/in/jimthompson7...and btw doktornotor is... special... :-Dhttps://github.com/doktornotor/pfsense-still-closedsource
let us know when it's time and let's test the experimental Wireguard kernel module.Then we will see if everything will be good.
Empty validation functionIn 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 -l21This 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 loopsSome 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.
Dear FreeBSD Community,In light of the recent commentary on FreeBSD's development practices,members of the Core team would like to issue the following statement.Code quality is an essential FreeBSD value: From the 1980s when work onBSD became the de facto standard TCP/IP stack, to our more recent workaround performance scalability on multicore, attention to detail iscritical. The recent concerns regarding the WireGuard patches remind usthat our development processes must always continue to mature. While theproject has historically, and aggressively, led the way in adopting newdevelopment methodologies - from public version control to being earlyadopters of static analysis tools such as Coverity - these events havebrought to light a real gap that needs to be addressed.The high stability and quality of FreeBSD is a testimony to theexperience of our developers. As in any open source project, we rely ondevelopers to exercise good judgement in seeking review and committingnew features, and to follow the guidelines laid out in the Committer'sGuide. We make heavy use of public code review, and FreeBSD developersspend a significant amount of time improving each others' contributions.We were excited to provide a kernel WireGuard implementation in FreeBSD13.0. Before the if_wg(4) rewrite was committed, several FreeBSDdevelopers proactively worked on fixing bugs and writing tests anddocumentation for the original implementation. In other words, we hadspent time during the release's Q/A period looking for problems, andthat unfortunately culminated in if_wg(4) being removed from 13.0 duringthe release cycle. As FreeBSD developers, it is incumbent on each of usto support each other's work by providing code review and helping testand fix the code. This incident highlights the need to do this work moreproactively, and to maintain a robust, multi-layered development processthat can catch problems as they fall through the cracks.Over the next month the FreeBSD Core Team will lead a discussion onappropriate pre-commit testing, static analysis, code review, andintegration policies to avoid a repeat of this situation and to continueimproving FreeBSD's code quality. We know there will be challenges inkey areas, such as third-party device drivers, and components of thesystem where fewer developers have sufficient expertise. The FreeBSDFoundation has full-time staff members participating in significant codereview today, and is committed to supporting the needs identified by theCore team and the developer community for this effort.We look forward to input from the community on our proposals for updatedpolicies as we move forward, maintaining high code quality as a corevalue for FreeBSD.Thanks,-Mark, with core@ hat on
Before the if_wg(4) rewrite was committed, several FreeBSDdevelopers proactively worked on fixing bugs and writing tests anddocumentation for the original implementation. In other words, we hadspent time during the release's Q/A period looking for problems, andthat unfortunately culminated in if_wg(4) being removed from 13.0 duringthe release cycle.
Pulling it from 13.0 release is ok, pulling it out of the development tree for 14.0 or a potential backport of 13.1 is insane if you think about all the work that was done to get it there in the first place.
