https://www.sudo.ws/repos/sudo/file/f75f786eddd5
It has more than 10 thousand commits, ~600 files, and close to 11MB of C code. Also, the code seems to have no unit tests, the main file is 1.4K lines long, has quintuple-nested conditionals and liberally uses goto statements.
Am I missing something here?
>Some of the points you raise are matters of style (I have files that are 10k lines long and think they are better that way
It's true that line counts are a matter of style. However, some coding styles tax cognitive bandwidth and immediate memory capacity more than others. This causes people to skim over specifics and miss bugs. Even if you are comfortable with a 10K LOC file you wrote, whomever reviews your code will probably be overwhelmed until they fully comprehend its structure. Grouping related things into smaller files is a way to focus their attention and communicate intended relations between code units.
> also I don't think unit tests are as useful as claimed
In general, I agree. Not a fan of TDD zealotry. But in this particular case some unit tests would be beneficial. There is a lot of stuff going in some methods.
They do seem to run PVS-Studio static analysis, which is somewhat reassuring, but I don't think that's enough in code that is so important, complex and widely used.
Whether you prefer this practice stylistically or not doesn't change the fact that it is objectively worse in terms of code quality and maintainability, even if you are able to compensate for those deficits with your own skill/experience.
You might have no problem successfully navigating that code for which you've already built a complete mental model around, but any future collaborators or inheritors of that code will have to waste hours or days rebuilding that mental model which you could have just codified in the structure of the source code.
The former isn't significantly different than exceptions, except that it's probably less likely than exceptions to cause unexpected errors because its more explicit.
At a very quick glance that's what the goto usage looks like in the codebase. This isn't to say that I think it's all appropriate, but if it was, I don't think your comment would have been any different.
There are a lot of really hot opinions on coding style and various fashions, but very little of it is backed up with research that connects it to defect rate.
I agree it would be good if sudo had an internal test suite (particularly system tests and fuzzing harnesses)-- but you also shouldn't discount the large amount of testing that goes on against it externally through both usage and external efforts.
The bug was fixed in 2012 - https://www.sudo.ws/changes.html: Use MAX_UID_T_LEN + 1 for uid/gid buffers, not MAX_UID_T_LEN to prevent potential truncation. Bug #562.
myhost alice = (ALL) /usr/bin/id
alice myhost = (ALL) /usr/bin/id
alice ALL = (ALL) /usr/bin/id
As I consider whether this bug impacts my company, I see two types of rules in our sudoers files: (1) rules that let already-privileged users do privileged things and (2) rules that let processes with minimal privileges make an exception to normal security rules. This bug doesn't impact rules for highly privileged users because they already have many ways to do whatever they want. This bug doesn't impact the second type of rules either because those rules specify exactly which user to change to; I tested the '-u#-1' trick with one of those rules on an unpatched sudo and sudo didn't allow it.
The behavior I observed seems to match the advisory, which says the exploitable rules are those that don't specify a specific user to run as.
Now I wonder: what kind of well-written rule would be exploitable?
The fix appears to be to reject -1 as invalid.
The article should have included in the fix section a link to the commit and a summary of what the fix was.
I do love that this command has its own website - and a delightfully on-point XKCD-inspired logo :)
sudo.ws belongs to Todd, the author of sudo.