WireGuard bounces off FreeBSD—for now
The WireGuard VPN tunnel is a fast and easy-to-use solution for those who need or want a secure tunnel for their traffic. The project has been around since 2016, but it has had a somewhat circuitous route into Linux; it was merged for the 5.6 kernel, which was released in March 2020. Getting into Linux required WireGuard developer Jason A. Donenfeld to acquiesce to having WireGuard use some of the existing kernel crypto primitives, rather than merging his Zinc crypto library. Some of the same tensions that were seen in that process seem to be cropping up again in the more recent efforts to add WireGuard support to several BSD kernels.
Complaints
A March 15 posting from Donenfeld on the WireGuard mailing list set off something of a chain reaction; in it he described the current status of WireGuard support in FreeBSD 13, which was in the release candidate stage at that point. A week earlier, FreeBSD developer Kyle Evans had asked on the list about supporting FreeBSD's port of the if_wg implementation of WireGuard from OpenBSD in the user-space wireguard-tools. He noted that FreeBSD 13 was soon to be released. That upcoming deadline caused a flurry of activity that Donenfeld relayed in his message:
In the ensuing discussion I mentioned that we really need to get the actual if_wg kernel implementation up to snuff. We took the conversation to IRC, and agreed that we should work on figuring out what to do before the release date. At the same time, Matt Dunwoodie, who worked on the OpenBSD implementation, also took a look at what had become of that implementation in FreeBSD. Over the next week, the three of us dug in and completely reworked the implementation from top to bottom, each one of us pushing commits and taking passes through the code to ensure correctness.
He went on at some length about the problems that were found in the existing port, which had been done at the behest of Netgate, a network security company that makes the FreeBSD-based pfSense router and firewall distribution. Donenfeld's description of the state of the code in FreeBSD before he, Evans, and Dunwoodie did their week-long rework was rather eye-opening:
It was not pretty. I imagined strange Internet voices jeering, “this is what gives C a bad name!” There were random sleeps added to “fix” race conditions, validation functions that just returned true, catastrophic cryptographic vulnerabilities, whole parts of the protocol unimplemented, kernel panics, security bypasses, overflows, random printf statements deep in crypto code, the most spectacular buffer overflows, and the whole litany of awful things that go wrong when people aren't careful when they write C. Or, more simply, it seems typical of what happens when code ships that wasn't meant to. It was essentially an incomplete half-baked implementation – nothing close to something anybody would want on a production machine.
The lengthy post also laid out some of the philosophy behind the WireGuard
project and Donenfeld's approach to compatible implementations, which is
decidedly hands-on. It is "a radical departure from the traditional model, and one surely to
raise some grumbles amongst graybeards
". He concluded the message by saying
that they had set out hoping to pull something together for the upcoming FreeBSD
release, but had fallen short of that goal. Evans committed
the changes they had worked on, but it "looks
exceedingly likely
" that FreeBSD will "just disable the module from the release, and
revisit for 13.1, rather than merging our fixes a few short days before
the release
". The post led to coverage
in the tech media—and a bunch of other effects.
Response
As might be guessed, Netgate was not particularly happy with what Donenfeld had to say—even less with how he said it—and was also less than pleased about the lack of warning regarding the problems. Netgate director of software engineering Scott Long posted a strongly worded response on the company blog; he also sent an email, apparently privately, to Donenfeld, who publicly replied on the WireGuard mailing list. In the blog post, Long pointed out that the WireGuard work had been put out for public review back in August 2020 and he took exception to some of what was posted:
Unfortunately, the public discussion has also veered into vague claims and slanderous attacks. This is where the lack of transparency, the lack of respect, and the inflation of ego is damaging and unproductive. We had hoped for a better collaboration than this, and it makes me doubt the motives of the attackers. And yes, I make deliberate use of the word “attacker” here, because that’s what this is, an attack on Netgate and on the FreeBSD and pfSense communities. Beware of anyone who says that they have all the answers. I also worry about the integrity of those who make vague statements and blanket, over-the-top accusations.[...] By following the normal, well understood security disclosure process this entire incident could have been handled quickly and efficiently through normal channels. We have yet to see a full description of the problems claimed; their choice to do a complete rewrite obscures the evidence of what they believe they were fixing, and they have yet to submit their work through the normal FreeBSD Phabricator process for review. That said, we do look forward to the bug reports and subsequent evaluation of the code through this review process. Code development is an iterative process, and one that we continue to strive to be better at. In the end, we will all benefit.
For his part, Evans was displeased with his role in the uproar and, on March 16, announced that he would be removing WireGuard from FreeBSD until sometime after the upcoming release. He and others planned to keep working on it moving forward.
The problem is that this work, in particular, is a driver with fairly severe security implications. Review by "three developers working and beating on it" is not the higher bar that we should be holding this to.
He followed that up with a message
stepping down from the wireguard-freebsd project entirely on March 18,
as well as a response
to Donenfeld's original post. He said that he did not "appreciate
how you handled this initial communication
" and felt that some of
the complaints were exaggerated, though he did acknowledge
multiple problems in the existing code. Beyond that:
I've additionally recommended, as a developer and not in any kind of official capacity, that we can't include if_wg in any future version of base. We cannot have our users being put at the risk of this kind of publicity if we can't get security advisories issued in time. Ports is a fine place, where security issues can be addressed expeditiously and more in line with how the WireGuard project chooses to handle them.
For now, development will
continue in the wireguard-freebsd Git
repository, but by the sounds, it will not be returning to the FreeBSD
kernel anytime soon. On March 22, Warner Losh put out a message
from the FreeBSD core team on the freebsd-hackers mailing list about
the "WireGuard controversy
":
Core unconditionally values the work of all contributors, and seeks a culture of cooperation, respect, and collaboration. The public discourse over WireGuard in the past week does not meet these standards and is damaging to our community if not checked. As such, WireGuard development for FreeBSD will now proceed outside of the base system. For those who wish to evaluate, test, or experiment with WireGuard, snapshots will be available via the ports and package systems.
NetBSD
As alluded to in Donenfeld's original post, there have also been efforts to
add WireGuard support in NetBSD, but those have not gone entirely smoothly,
seemingly. An August
2020 thread started by Donenfeld on the tech-kern and tech-net mailing
lists for NetBSD was similarly critical of an existing WireGuard
implementation; he asked that the existing code be reverted in favor of an
evaluation of the proper path forward. He offered to work with Taylor R
Campbell, who had picked
up some code written by Ryota Ozaki back in 2018;
"it seemed to be in pretty good
shape when I reviewed it this year, with a few small issues I saw, so
I dusted it off and merged it
".
In that thread, Campbell and others repeatedly asked Donenfeld to describe the problems he said that he found, but that was not how Donenfeld wanted to proceed:
This isn't about some random bug here or there, or a TODO list of little errors and optimizations, or something that one would normally say, "well, the bones are there, so let's throw this in the tree, and we'll clean it up as a gradual evolutionary thing." Rather, the foundation is incomplete, and its current form does not belong in a tree.[...] The right course of action here is to revert the code, and then after we can evaluate which direction we want to go in -- whether it's doing a full teardown of Ozaki-san's code, or whether it's doing a simple port of the OpenBSD code.
It would seem that Campbell and Donenfeld never actually got to that point;
Donenfeld said that he hoped "to be able to lend a hand similarly to the
NetBSD developers soon
" in the original FreeBSD announcement. Part
of the disconnect here may be the type of review that Donenfeld is looking
for; he talked about what he and Dunwoodie had done over a year's time to get the OpenBSD
implementation into shape ("continual code reviews,
knowledge transfers, and he even showed up at my house at some point
to read code together
"). Matt Macy, who was the author of
the FreeBSD version that was so heavily criticized mentioned a similar
requirement in a FreeBSD
bug report:
Jason had insisted that the code needed to be reviewed by him, but he won't actually spend any time on reviewing the code unless I'm actually sitting there engaging with him directly. The OpenBSD dev appears to have spent many dozens of hours with him. However, I simply have no time or inclination for that. I'm not sure who in the FreeBSD community has the crypto background to review the protocol bits as well as the energy to do so.
By all accounts, WireGuard itself is an excellent VPN solution, but it would seem that unlike the usual approach for a network protocol, where multiple implementations are made independently based on a specification, WireGuard needs to be treated differently. Donenfeld is justifiably proud of his accomplishment, but his requirements for other implementations seem far too rigid—at least for some communities. As we have seen in several different operating system projects (Linux, FreeBSD, NetBSD), Donenfeld often expects that the other, much larger projects conform to his exacting standards and methods. In the end, that attitude may discomfit more than just graybeards.
Posted Mar 24, 2021 17:02 UTC (Wed)
by NYKevin (subscriber, #129325)
[Link] (19 responses)
> There were random sleeps added to “fix” race conditions, validation functions that just returned true, catastrophic cryptographic vulnerabilities, whole parts of the protocol unimplemented, kernel panics, security bypasses, overflows, random printf statements deep in crypto code, the most spectacular buffer overflows, and the whole litany of awful things that go wrong when people aren't careful when they write C.
Either Donenfeld made half those claims up, or he didn't. If he didn't, then it sounds pretty damning to me. If he did, then why on Earth would he do that? Anyone can just go look at the code and check, right?
Yes, this probably should have been worded more diplomatically. But I do not understand how Netgate managed to interpret this sentence as "vague claims." It's actually quite specific (although it would've been nice if Donenfeld could've provided line-by-line citations back to the problematic code). And the reference to "slanderous attacks" makes it sound like they've already got their lawyers on speed dial. Maybe it's just me, but I don't think FOSS volunteers should be expected to collaborate with someone who is making a veiled legal threat.
Am I missing something here?
Posted Mar 24, 2021 18:12 UTC (Wed)
by marcH (subscriber, #57642)
[Link] (4 responses)
Diplomacy is one thing, this is another:
> It's actually quite specific (although it would've been nice if Donenfeld could've provided line-by-line citations back to the problematic code)
... which means it was nowhere near specific enough.
This is exactly how the "post-truth" / "alternative facts" world looks like: even when the evidence is open-source it's apparently not required to merely link to it!? "Fake news" is misleading, "News with no or distorted evidence" is more accurate. True of false? No one knows but everyone on Twitter has an opinion.
It didn't have to be exhaustive; a few examples to back up claims like "catastrophic vulnerabilities", "most spectacular buffer overflows" would have made a huge difference.
Now I understand security disclosures should follow some processes but I doubt these processes are compatible with publicly and vaguely claiming that "everything is broken" before there is an alternative available.
(reality show: fiction where "actors" play their own, exaggerated characters)
Posted Mar 26, 2021 14:57 UTC (Fri)
by ofranja (guest, #11084)
[Link] (3 responses)
A line-by-line comparison would not only take a non-trivial amount of work but also distract from the real issue: lack of proper review/quality control. If there was a line-by-line comparison instead of a single paragraph some could also say it was an attempt to publicly humiliate their code/developers, when the issue rests entirely in a different place.
Posted Mar 28, 2021 1:03 UTC (Sun)
by marcH (subscriber, #57642)
[Link] (2 responses)
No, the last link in https://lists.zx2c4.com/pipermail/wireguard/2021-March/00... is a link to the "culmination of about a week of work from three developers to fix a number of functional and security issues". There are no specific references to "spectacular buffer overflows" etc. in the message that "reports" them.
Reading that entire, giant patch goes way beyond "some homework".
> A line-by-line comparison would not only take a non-trivial amount of work but also distract from the real issue: lack of proper review/quality control. If there was a line-by-line comparison instead of a single paragraph some could also say it was an attempt to publicly humiliate their code/developers, when the issue rests entirely in a different place.
As if that message was not humiliating...
It's the exact opposite: whenever some code review has hard evidence of "spectacular buffer overflows" and other "catastrophic vulnerabilities" _without_ the emphatic/clickbait adjectives, then only the maintainers and persons deeply involved with the project notice and the media does not. Happens all the time. Same in the linux kernel: when Linus or some other maintainer swears it most likely makes the headlines. When they show that some code is crap _without_ using the word "crap" and without swearing then only the project members notice.
Posted Mar 29, 2021 23:04 UTC (Mon)
by ofranja (guest, #11084)
[Link] (1 responses)
So you say "no" at the same time you point to the right patch?
If you don't know how commits and patches work, that's fine, but that's usually assumed if you have some minimal C knowledge.
Hint: red means "code removed". That's the code that was there before and where you'll find the buffer overflows, sleeps, etc.
> There are no specific references to "spectacular buffer overflows" etc. in the message that "reports" them.
No, and that has zero relevance since there's a much better source: the code per se.
It's obvious that's the right commit once you look at the diff stat.
> Reading that entire, giant patch goes way beyond "some homework".
You don't need to read the "giant patch", just check the code that was removed - and just the relevant files (ie: the ones where the driver is implemented).
I have never developed a single line of code for FreeBSD and it took me 30 seconds to find the pause() calls + some of the issues reported.
It's not hard once you know what you are looking for. Hint: the email you linked enumerates several issues.
> As if that message was not humiliating...
Exactly. And you want to add a line-by-line "look how s*** your code is" to make it worse? I don't get it.
> [...] _without_ the emphatic/clickbait adjectives, then only the maintainers and persons deeply involved with the project notice and the media does not.
Sure, I'm not debating this. Of course, it doesn't help that the developer who wrote the original driver is also known as "landlord from hell", but we agree nevertheless.
-
Last, but not least: the point in my response is that this has nothing to do with a "post-truth" world: the references are there on the email - commits, paper, previous discussion, you name it. The email is actually *quite the opposite*: a well-grounded summary with all the relevant information attached to it.
The problem I see here, and the reason I replied to your comment, is the same I've been seeing on open source in general since beginning 2010s: everybody expects the information to be extremely chewed-in, ready to consume, and have zero tolerance for any kind of effort. Grepping a commit is understood as a huge undertaking. I think that doesn't make any sense, and one way to put it into perspective is to consider that if someone had the time to rewrite a whole protocol driver you can spare a couple of minutes to understand a few lines of code - if you are interested, of course.
> "News with no or distorted evidence" is more accurate. True of false? No one knows but everyone on Twitter has an opinion.
"Ackchyually".. the people who looked at the code will know.
Of course, many people will have an opinion even without knowing what a buffer overflow is.. welcome to the internet, I guess?
In any case, my point is: you may say that *for you* that email was not informative enough or that you don't have the time to dig the sources - and that's fair. But saying this is very different that saying the information is not there or "distorted" in any way - which is just plain out wrong.
And that's it.
Posted Apr 16, 2021 5:02 UTC (Fri)
by marcH (subscriber, #57642)
[Link]
> So you say "no" at the same time you point to the right patch?
The explanation is in the [...] part that you cut. So you don't mind searching a patch with 70 files changed, 6333 insertions and 43677 deletions but reading my entire post before starting to answer it is too much?
> > Reading that entire, giant patch goes way beyond "some homework".
> > You don't need to read the "giant patch", just check the code that was removed
Most of the patch _is_ deleted code!
> > As if that message was not humiliating...
> Exactly. And you want to add a line-by-line "look how s*** your code is" to make it worse? I don't get it.
What I'm recommending is pretty simple and is usual in code reviews "at the office" (where people tend to have more to lose):
And "that's it".
> Last, but not least: the point in my response is that this has nothing to do with a "post-truth" world: the references are there on the email - commits, paper, previous discussion, you name it. The email is actually *quite the opposite*: a well-grounded summary with all the relevant information attached to it.
Of course this email was not fake news and yes you could find the information in a reasonable amount time _if_ you cared enough. Yet it was still vague enough to allow a refutal like Netgate's, referring to "vague claims and slanderous attacks." This is the very tough, fake news world we unfortunately live in. Sad but... real. I think this refutal and entire show would have been avoided with 1. and 2. above, that's all. On the other hand not everyone hates the spotlight, hey we even have "rock stars" after all.
> > There are no specific references to "spectacular buffer overflows" etc. in the message that "reports" them.
> No, and that has zero relevance since there's a much better source: the code per se.
Good for you. It took a bit longer to the tech press reacting to the click-baity adjectives. Some of it may have trusted Netgate instead, at least for a moment.
Ignoring the media attention for a moment, there is always a shortage of reviewers (as very clearly demonstrated in this case!) and their time is incredibly precious. Sure they can do a grep in less than 30 seconds but is that enough to make the tough decision of deleting an entire driver? I don't think so.
I saw one of the top Linux kernel maintainers misunderstanding a _one-line_ log statement change last week! Can't blame him considering the huge amount of code reviewed and crazy number of context switches total.
There's always the temptation to make a splash to draw the reviewers' limited attention but when replacing an entire feature I don't think that's necessary, "43677 deletions" should be enough to draw attention one way or the other.
> The problem I see here, and the reason I replied to your comment, is the same I've been seeing on open source in general since beginning 2010s: everybody expects the information to be extremely chewed-in, ready to consume, and have zero tolerance for any kind of effort. Grepping a commit is understood as a huge undertaking. I think that doesn't make any sense, and one way to put it into perspective is to consider that if someone had the time to rewrite a whole protocol driver you can spare a couple of minutes to understand a few lines of code - if you are interested, of course.
It depends. Not everything needs to be chewed-in. However for such a massive, bold and obviously controversial change as deleting an entire feature and replacing it with your own implementation then: yes you really want things to be as "chewed-in" as possible to make sure you have all the over busy (FreeBSD) stakeholders on your side ASAP and not just the domain experts who reviewed the first implementation. Especially when the latter group was made of... no one in this example!
Also, code and emails are "written once but read many times" so yes it's more efficient to put all the effort on the writer's side. Nothing new here, that's much older than the 2010's.
Posted Mar 24, 2021 18:13 UTC (Wed)
by bredelings (subscriber, #53082)
[Link] (9 responses)
> We have yet to see a full description of the problems claimed; their choice to do a complete rewrite obscures the evidence of what they believe they were fixing, and they have yet to submit their work through the normal FreeBSD Phabricator process for review.
This does kind of sound like the same issues in the second part of the article, as indicated by the quote from Matt Macy, where "review" means not so much a search for errors, but an insistence that "you have to do it my way".
But without investigating further it seems difficult to tell where the truth lies.
Posted Mar 24, 2021 18:30 UTC (Wed)
by NYKevin (subscriber, #129325)
[Link]
Regardless, I'm very much Not Happy about the use of the term "slanderous." That is a legal threat. You cannot throw around legal threats in the context of FOSS development. Right now, the safest thing for Donenfeld and the FreeBSD folks to do would be to cease all interactions with Netgate (and their code) until they issue a statement retracting the threat. Anything less could expose them to litigation or the threat of litigation.
I know that sounds extreme, but from a legal perspective, anything that anyone says about Netgate becomes evidence. Anyone who makes any sort of statement about this could potentially become a witness and get hauled into court to testify. Imagine you send an ill-advised email to the relevant mailing list at 3 AM. Do you want to have to explain that email to the court?
Sure, a lawsuit is unlikely to materialize. But that's why legal threats are so damaging: You can never be sure that they won't happen. It creates an air of fear and uncertainty, and makes rational, evidence-based engineering practices impossible to follow.
Posted Mar 24, 2021 20:30 UTC (Wed)
by excors (subscriber, #95769)
[Link] (3 responses)
From the imported Netgate code (https://cgit.freebsd.org/src/commit/sys/dev/if_wg?id=2338...), I think the "random sleep" are the 'pause("link_down", hz/4)' etc. And they were explicitly added to avoid race conditions (https://reviews.freebsd.org/D26137#611017). wg_allowedip_valid is a "validation functions that just returned true". __chacha20poly1305_decrypt looks like "random printf statements deep in crypto code". (The commented-out WARN_ON calls are sloppy but probably don't count).
That doesn't look as terrible as Donenfeld implies, but his points seem basically true, and those are all quite obvious issues that suggest the code was merged without being reviewed to a high standard. Those specific issues could have been fixed with some iterations of reviewing and patching, but it seems hard to have much confidence that there aren't a lot of more subtle bugs that will slip past all those reviews, since it doesn't look like the code was developed with a particularly security-oriented mindset from the start.
It may have been developed and reviewed to the same standards as a lot of kernel code, and maybe that's good enough to probably avoid any major security bugs (so Netgate is not wrong in claiming there are no exploitable vulnerabilities), but Donenfeld appears to deliberately set higher quality standards for WireGuard implementations. It seems he views it as a brand, where a bad implementation on one OS will hurt the overall brand and will discourage users from using it on other OSes where it has been implemented well; it's important to ensure every implementation is not just technically compatible with the protocol but is also implemented to a high standard. But few kernel developers have the same standards, and that leads to some stepping on toes.
Posted Mar 25, 2021 0:13 UTC (Thu)
by zx2c4 (subscriber, #82519)
[Link]
There are more bugs than the ones you identified. Copying and pasting from https://cgit.freebsd.org/src/commit/?id=2338da0373f19b511... , for example:
+ out_len = sizeof(struct wg_pkt_data) + plaintext_len + NOISE_MAC_SIZE;
If out_len is greater than out MCLBYTES (2048), that m_copydata overflows a heap buffer.
+ /* A packet with length 0 is a keepalive packet */
It checks for len == 0, but doesn't check for len >= 20 or >= 40, for being able to read the IPv4/IPv6 headers, so you wind overrunning and reading nonsense off the heap. Later this too-short packet is passed onto directly to the rest of the networking stack.
+ aip = malloc(count*sizeof(*aip), M_TEMP, M_WAITOK);
Probably should be using `mallocarray` on that multiplication.
+ if ((m = m_defrag(m0, M_NOWAIT)) == NULL) {
m_defrag organizes the chain into optimally sized fragments. If the data doesn't fit into a single fragment, it winds up just being truncated and then later decrypted?
+void curve25519_generic(u8 out[CURVE25519_KEY_SIZE],
What happened to the call to clamp that scalar? DJB's paper pretty clearly lays out the case for that.
+ if (curthread->td_ucred->cr_uid == 0)
Just like on Linux, where checking current->uid is not the right way and you should use capable or ns_capable, so too here priv_check should be used.
...and so on and so forth.
It really was as bad as I described.
Posted Apr 5, 2021 19:41 UTC (Mon)
by rlhamil (guest, #6472)
[Link] (1 responses)
MAYBE SeL4 gets close.
In the long run, only AI's should code; humans are too defective; whether too sloppy or too egotistical matters not.
Posted Apr 7, 2021 7:10 UTC (Wed)
by jezuch (subscriber, #52988)
[Link]
Posted Mar 24, 2021 21:23 UTC (Wed)
by rodgerd (guest, #58896)
[Link] (3 responses)
I mean I am quite comfortable with the idea that one ought not to place too much trust in Kip Macy: https://www.huffpost.com/entry/landlords-from-hell_n_3468487
Posted Mar 24, 2021 21:54 UTC (Wed)
by karkhaz (subscriber, #99844)
[Link] (2 responses)
Posted Mar 24, 2021 22:10 UTC (Wed)
by excors (subscriber, #95769)
[Link]
Posted Mar 25, 2021 0:04 UTC (Thu)
by ariagolliver (✭ supporter ✭, #85520)
[Link]
Posted Mar 24, 2021 19:09 UTC (Wed)
by Bigos (subscriber, #96807)
[Link]
-static int
If I understand the API [3] correctly, pause() is meant to make the thread sleep for some period of time (I assume the unit is the number of "jiffies" and "hz" is the number of jiffies in a second, similar to Linux's CONFIG_HZ). The thread cannot be awoken explicitly. This suggests these are really some "random" sleeps to synchronize threads implicitly. I cannot really confirm that without going deeper into the code, though.
The new version [4] doesn't use the pause() function at all.
But as you said, this should all be included in the original message that discredited the original code.
[1] https://cgit.freebsd.org/src/commit/?id=74ae3f3e33b810248...
Posted Mar 24, 2021 21:21 UTC (Wed)
by rodgerd (guest, #58896)
[Link] (1 responses)
That the developer hired by Netgate threatened to murder the children of the tenants in a building he used to own, cut through the floors in an effort to collapse part of it while people were still living there, fled the US in an effort to evade justice, and spend several years in prison for his reign of terror?
I can imagine trying to reason with such a person might prove challenging.
Posted Mar 24, 2021 22:11 UTC (Wed)
by flussence (guest, #85566)
[Link]
It's still unclear whether Netgate hired him oblivious to this, or whether there's some nepotism in play. With the ham-fisted rant they posted and swiftly deleted on their official blog regarding the WireGuard situation, I'm not convinced it's not the latter.
Posted Mar 24, 2021 22:49 UTC (Wed)
by zx2c4 (subscriber, #82519)
[Link]
The claims are not made up in the least, and they're all easily backed up by pointing at lines of code. For example, try sending a packet > 2048 bytes. You wind up overflowing the mbuf ("sk_buff") data allocation.
Posted Mar 24, 2021 23:45 UTC (Wed)
by zx2c4 (subscriber, #82519)
[Link] (2 responses)
Thanks for covering this, though I’m not so sure I agree with the editorial thrust of your article. I suppose I’ll take it point by point.
> “Getting into Linux required WireGuard developer Jason A. Donenfeld to acquiesce to having WireGuard use some of the existing kernel crypto primitives, rather than merging his Zinc crypto library.”
That’s really not what happened. Ard Biesheuvel helped take some of the ideas from the Zinc library approach and organize it into the existing directory structure of the crypto API, and in the process we added, changed, and improved some implementations. It was really a terrific compromise, and not something I felt that required “acquiescence” but rather was a good approach to take for making incremental progress. And since that merge we’ve continued to improve it.
> “As might be guessed, Netgate was not particularly happy with what Donenfeld had to say—even less with how he said it—and was also less than pleased about the lack of warning regarding the problems.”
It’s worth noting that the entire point was to supply fixes to the grave issues we found *before* FreeBSD 13.0 was released. The idea was that beta/RC code was nearly out the door, and so our only option was to fix it *before* it was actually released. It’s not like we found these problems and then sat on them for a while. We immediately set up a git repository in the usual place so we could collaborate and were just rapid-fire pushing fix commits, one after another. When we felt like we had written _too much code_ for one chunk, it was sent up to FreeBSD and we took a pause. And most importantly, we sent it up to FreeBSD *before* 13.0 was released. So I’m not sure about the characterization here – we worked in a public repository on pre-release code and sent it up as soon as we could after working tirelessly. The Linux equivalent would be sending patches to fix bugs at -rc4 or -rc5: pretty yikes that there are bad bugs that late, but thank goodness they’re found before Linus releases the final.
As well, let me be clear: I was really very happy that the FreeBSD security team pulled the code from the final release and allowed us to work on it more leisurely outside of base. Trying to fix a lot of bad bugs and rewrite the thing in a weeks time is way, way too rushed for anything reasonable. I was told initially that pulling the code to work on it slowly wasn’t an option, hence our sprint, but the sprint was always a second-best option. So the outcome is really the best possible one: we now have time to stretch out and do it right.
> <quotation from Scott Long blog post>
There’s really a lot of misinformation in that post, which was pretty handedly dismissed by the infosec world as being ridiculous. There were no “attacks”. We volunteered our time to fix bugs. We sent those bug fixes up to FreeBSD. Netgate, rather than embrace the fixes and work with us, decided to go on the offensive (see https://lists.zx2c4.com/pipermail/wireguard/2021-March/00... ).
> “He followed that up with a message stepping down from the wireguard-freebsd project entirely on March 18, as well as a response to Donenfeld's original post. He said that he did not "appreciate how you handled this initial communication" and felt that some of the complaints were exaggerated, though he did acknowledge multiple problems in the existing code.”
All of the press attention about this caused a lot of emotion in the FreeBSD camp, and I think you saw some of that in the emails you linked to. As for claims of exaggeration, though, I unequivocally deny that. Each thing I pointed to is easily backed up with a line (or lines) of code. There’s been no exaggeration at all. I’ve since discussed this with the author of the email you linked to as well.
I think it’s worth re-reading my original post, which has since been minced in all sorts of ways: https://lists.zx2c4.com/pipermail/wireguard/2021-March/00... . The description is, simply, accurate. There are bugs in all of those categories. The code was quite clearly half-baked and incomplete. And when you’re talking about replacing an implementation to the degree to which we did, backing that up with real description and justification is absolutely necessary. So the email is descriptive, not exaggerated.
(The larger issue at hand here is how they possibly let that original code get committed, but that’s not really a matter for me to wade into.)
> “For now, development will continue in the wireguard-freebsd Git repository, but by the sounds, it will not be returning to the FreeBSD kernel anytime soon.”
This is accurate. We’re still finding bugs in the thing, and it’s going to take some time to get this polished. (I’m grateful we’re not running up against a two week deadline anymore.) My hope is that by the time it’s “complete”, all of the media buzz insanity has died down, and we can submit this for a proper code review (something evidently missing for the original submission).
> “Matt Macy, who was the author of the FreeBSD version that was so heavily criticized mentioned a similar requirement in a FreeBSD bug report:”
He discarded the code reviews I did, and ignored messages and emails. He clearly did not want my involvement. His characterization in that bug report seems like a lousy excuse.
> “It would seem that Campbell and Donenfeld never actually got to that point; Donenfeld said that he hoped "to be able to lend a hand similarly to the NetBSD developers soon" in the original FreeBSD announcement. Part of the disconnect here may be the type of review that Donenfeld is looking for”
No, I don’t think that’s the issue. The disconnect here is that I simply have not been able to carve out time for that, and it’s extremely regrettable, and something I fully intend to rectify after the FreeBSD situation is sorted. Campbell _has_ reached out to me several times, and I believe he’ll be receptive to feedback and we’ll be able to finish an implementation. Currently, it’s on me to deliver.
> “Donenfeld is justifiably proud of his accomplishment, but his requirements for other implementations seem far too rigid—at least for some communities. As we have seen in several different operating system projects (Linux, FreeBSD, NetBSD), Donenfeld often expects that the other, much larger projects conform to his exacting standards and methods. In the end, that attitude may discomfit more than just graybeards.”
That might actually be somewhat accurate. I really do not want WireGuard to be crap code. Of course, bugs happen always, anyway, and no code is perfect. But I believe with enough energy put into it, it’s entirely possible to raise the security standards on small isolated codebases like WireGuard. And so the WireGuard project routinely puts in that energy to write and review code so that it does have that bar of scrutiny. As I described in my email, yes, this is different, and yes, this requires a lot of work from the WireGuard project, but ultimately I think it’s worth it.
Thanks,
Posted Mar 25, 2021 10:19 UTC (Thu)
by rsidd (subscriber, #2582)
[Link]
Posted Apr 1, 2021 16:27 UTC (Thu)
by hawk (subscriber, #3195)
[Link]
I think this is where I was more than a little confused when first seeing this shouting match, as there seems to be a pretty significant communication disconnect right there.
My impression was that Netgate were probably mainly upset with how this was disclosed from a Pfsense perspective rather than a FreeBSD 13 one. Essentially the same code had already shipped in their latest release when these problems were disclosed, so they were already in the position that you wanted to avoid for FreeBSD. And from their communication, it seems they thought there should have been more private communication beforehand, but I don't know who knew what about what everyone else had been doing.
Posted Mar 25, 2021 5:43 UTC (Thu)
by rsidd (subscriber, #2582)
[Link] (5 responses)
Posted Mar 25, 2021 6:05 UTC (Thu)
by mjg59 (subscriber, #23239)
[Link] (1 responses)
Posted Mar 25, 2021 6:40 UTC (Thu)
by rsidd (subscriber, #2582)
[Link]
Posted Mar 25, 2021 10:41 UTC (Thu)
by Sesse (subscriber, #53779)
[Link] (2 responses)
Posted Mar 25, 2021 15:21 UTC (Thu)
by ragnar (guest, #139237)
[Link]
Posted Mar 25, 2021 15:51 UTC (Thu)
by perennialmind (guest, #45817)
[Link]
Posted Mar 26, 2021 11:58 UTC (Fri)
by excors (subscriber, #95769)
[Link] (5 responses)
> Those who did speak described [Macy] as a talented, professional coder who produced no more bugs than most and responded well to code reviews and the need to squash bugs when identified. The condition of Macy's if_wg implementation came as a surprise to those who had worked with him in the past—sloppy, half-finished code wasn't simply par for the course when working with him.
> "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."
I can kind of sympathise with that - sometimes you're in a poor mental state and can't achieve the work that you're normally capable of, and you can't even find the energy to really care that you're failing. And it's hard to blame anyone for struggling during 2020 in particular. Ideally there'd be an environment where people would notice your problems, prevent your poor work from damaging the wider project, and give you the support needed to get back into a state where you can perform at your best.
From the article's conclusions, it appears there wasn't an environment that would prevent the bad WireGuard implementation from damaging the project:
> Neither Netgate's responses, FreeBSD Core's, nor the off-record responses we heard from independent FreeBSD community members lead us to believe that there was in fact any process in place that could reasonably have been expected to catch this issue prior to it going out into the world in 13.0-RELEASE.
That's not an easy problem to solve, but it sounds like an important one.
Posted Mar 26, 2021 13:46 UTC (Fri)
by rsidd (subscriber, #2582)
[Link] (1 responses)
Hopefully this episode leads to some changes. As the article says, FreeBSD is still important.
Posted Mar 29, 2021 5:37 UTC (Mon)
by marcH (subscriber, #57642)
[Link]
Every day I'm amazed how so many software projects (open _and_ closed) driven by so many smart developers can stay so retarded with respect to the most basic software development workflows like code reviews, source control, continuous integration, automated testing and scanning, etc. I'm aware colleges don't care and don't teach these either cause they're only useful for products and not for publishing papers but come on: the explanations about why all these are critical and how are all over the Internet. This is really not rocket science and it's not even new any more either.
Posted Mar 30, 2021 4:16 UTC (Tue)
by kaliszad (guest, #125214)
[Link] (2 responses)
If a security product/ supposedly trusted projects code loses some company's secrets or hurts somebody, nobody cares whether it was a single bad day, a mishap if you will or something that could have been forseen. The data is now in bad hands, peoples lives are disturbed. Everything else is basically just talk until _proven_ otherwise. FreeBSD really dodged the ball at the last minute here, Netgate not so much.
A responsible way to handle this would be to be upfront and admit you don't have the energy/ will to finish the work properly.
People very much did notice "problems" with Macy, he robbed his family, endangered other peoples health and was together with his wife sent to prison for years. I guess, that speaks volumes about how dependable such a person is. It is also not the best professional development to sit for years in a prison even if you believed, he is a changed person now. Just for comparison, would you trust H. Reiser to write filesystem code that would get commited directly to something like RC1 in Linux without any sign-offs, if that was possible?
Posted Apr 2, 2021 7:29 UTC (Fri)
by Hi-Angel (guest, #110915)
[Link] (1 responses)
IMO it's orthogonal. Remember some years ago when Microsoft sent their first driver to the kernel, many people were like "oh no, an evil corporation sent some patches, what's devs gonna do?". Torvalds put it back then, paraphrasing: "I don't care who sends the code as long as it's useful and is up to our quality standards". So, I agree with him here: why should I care who is the code author?
> without any sign-offs, if that was possible?
I assume you meant reviewed-bys? My experience shows even the most experienced and good developers make mistakes, so code review is a must have disregarding who you are. Though, sometimes in various projects there's simply no one to review the code, so you have to commit it as is. But that I think is a separate question.
Posted Apr 3, 2021 4:27 UTC (Sat)
by flussence (guest, #85566)
[Link]
Wasn't that the code that had the infamous 0xB16B00B5 in it? Perhaps not the best example of people you'd want to work with in a professional environment.
WireGuard bounces off FreeBSD—for now
WireGuard bounces off FreeBSD—for now
WireGuard bounces off FreeBSD—for now
WireGuard bounces off FreeBSD—for now
WireGuard bounces off FreeBSD—for now
WireGuard bounces off FreeBSD—for now
1. No word like "s***". Again, did you read before answering? Was my message no "chewed-in" enough?
2. Cold-blooded analysis of a small list of the worst examples with specific line numbers for reviewers who are always in a hurry.
3. "I found many other similar issues in this driver, so I'm removing it"
> I have never developed a single line of code for FreeBSD and it took me 30 seconds to find the pause() calls + some of the issues reported.
WireGuard bounces off FreeBSD—for now
WireGuard bounces off FreeBSD—for now
WireGuard bounces off FreeBSD—for now
WireGuard bounces off FreeBSD—for now
+
+ if ((mc = m_getjcl(M_NOWAIT, MT_DATA, M_PKTHDR, MCLBYTES)) == NULL)
+ goto error;
+
+ data = mtod(mc, struct wg_pkt_data *);
+ m_copydata(m, 0, m->m_pkthdr.len, data->data.buf);
+ if (m->m_pkthdr.len == 0) {
+ DPRINTF(peer->p_sc, "Receiving keepalive packet from peer "
+ "%llu\n", (unsigned long long)peer->p_id);
+ goto done;
+ }
+
+ version = mtod(m, struct ip *)->ip_v;
+ if (version != IPVERSION && version != 6) {
+ DPRINTF(peer->p_sc, "Packet is neither ipv4 nor ipv6 from peer "
+ "%llu\n", (unsigned long long)peer->p_id);
+ goto error;
+ }
+
+ routed_peer = wg_route_lookup(&peer->p_sc->sc_routes, m, IN);
+ DPRINTF(sc, "DEFRAG fail\n");
+ return;
+ }
+ data = mtod(m, void *);
+ const u8 scalar[CURVE25519_KEY_SIZE],
+ const u8 point[CURVE25519_KEY_SIZE])
+{
+ fe x1, x2, z2, x3, z3;
+ fe_loose x2l, z2l, x3l;
+ unsigned swap = 0;
+ int pos;
+ u8 e[32];
+
+ memcpy(e, scalar, 32);
+
+ /* The following implementation was
+ nvlist_add_binary(nvl, "private-key", sc->sc_local.l_private, WG_KEY_SIZE);
WireGuard bounces off FreeBSD—for now
WireGuard bounces off FreeBSD—for now
WireGuard bounces off FreeBSD—for now
WireGuard bounces off FreeBSD—for now
WireGuard bounces off FreeBSD—for now
WireGuard bounces off FreeBSD—for now
WireGuard bounces off FreeBSD—for now
-wg_detach(if_ctx_t ctx)
-{
...
- pause("link_down", hz/4);
- wg_peer_remove_all(sc);
- pause("link_down", hz);
...
-}
[2] https://cgit.freebsd.org/src/tree/sys/dev/if_wg/module/mo...
[3] https://www.freebsd.org/cgi/man.cgi?query=sleep&sekti...
[4] https://cgit.freebsd.org/src/tree/sys/dev/if_wg/if_wg.c?i...
WireGuard bounces off FreeBSD—for now
WireGuard bounces off FreeBSD—for now
WireGuard bounces off FreeBSD—for now
Not so sure I agree with the editorial thrust of article.
Jason
It certainly doesn't sound like you were at fault. If you could work with Linux and OpenBSD folks, the problem is not with you! Probably the problem is not with FreeBSD folks at large, either. Hope the storm is blown over now.
Not so sure I agree with the editorial thrust of article.
Not so sure I agree with the editorial thrust of article.
> It’s worth noting that the entire point was to supply fixes to the grave issues we found *before* FreeBSD 13.0 was released.
WireGuard bounces off FreeBSD—for now
WireGuard bounces off FreeBSD—for now
WireGuard bounces off FreeBSD—for now
WireGuard bounces off FreeBSD—for now
WireGuard bounces off FreeBSD—for now
WireGuard bounces off FreeBSD—for now
WireGuard bounces off FreeBSD—for now
WireGuard bounces off FreeBSD—for now
WireGuard bounces off FreeBSD—for now
WireGuard bounces off FreeBSD—for now
WireGuard bounces off FreeBSD—for now
WireGuard bounces off FreeBSD—for now