Potential bypass of Runas user restrictions in sudo | Hacker News Reader

Potential bypass of Runas user restrictions in sudo

sudo.ws

93 pointsFindus2327 comments

Is this the official sudo repository?

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?

Welcome to the world of code older than most people on HN. Sudo is about 30 years old. A lot of code from that era, including the kernels and utilities for most flavors of UNIX, was written in a similar style. The "goto done" idiom was a pretty common form of error handling in Plain Old C, and was more readable/verifiable than some alternatives such as deeply nested functions. Remember, this is C - no objects (so no destructors for RAII), no exceptions, etc. The lack of unit tests is explainable by the fact that tests were often external to the running code and considered proprietary. It's all too easy for people who never had to deal with serious portability issues or even know what threats existed in the tty-handling stack to hold code like this up against a standard that was still decades in the future and find it wanting. Is it useful?

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; also I don't think unit tests are as useful as claimed), but yeah, when you have 600 files and 11MB of code to do something that is supposed to be simple and also is security-critical ... you have a big problem.

What a coincidence. I watched your video on Preventing the Collapse of Civilization just yesterday. (Mostly agree with things said there, with the exception of the part about Smalltalk. Ironically, the video was recommended to me in a Smalltalk chat channel.) Seems relevant.

>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.

It should be very obvious once you think about it that breaking code into more files, and procedures into subprocedures, is pushing complexity around rather than simplifying ... and it’s pushing the complexity from somewhere visible, into an invisible structure that the viewer has to then reconstruct in his mind. I think in a great many cases it’s not a good idea.

There is a reason the OpenBSD folks decided to write doas:)

> I have files that are 10k lines long and think they are better that way

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.

> objectively worse

That's an opinion. There's nothing objective about the assertion that many small files are inherently more secure. However, that many small files and one large file have equivalent power, can express all the same programs, and can be equally secure or insecure... those actually are all objective facts.

I didn't say it was objectively more secure. Obviously it's possible to have code that is of poor quality/maintainability but still happens to be more secure than its alternatives. Maintaining good code quality is not a magic bullet that prevents all bugs.

Then replace secure with "maintainable" or "high quality". It's still a subjective opinion, unless you can prove that large files are inherently less maintainable or lower quality (which you can't, because all the arguments here are subjective and, like almost everything in software development, these opinions are driven by fashion rather than any evidence).

It's popular to use "objectively" for "clearly" and I've been guilty of this myself, but let's try to reserve the term for actual matters of fact, lest it lose any meaning.

Besides being unclear whether one is better or worse, it's not clear there's even a practical difference when a significant number of programmers will be navigating and learning code through an IDE anyway. They'll be jumping from function to function, barely even aware of which file each one is in.

Personally I prefer smaller files, but that's only because of compile times. I started my career with punch cards, so I know what slow compile times look like. For a long time they seemed to be getting ever shorter, until I switched to a "modern C++" project. Now compile times are not quite back to the punch-card days, but back to maybe late 90s, and breaking stuff up into smaller files helps.

There is a BIG difference between "always moves to the end of the function for error handling" use of goto, and general unstructured control flow goto.

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.

No, it is not very good. I remember this one particular example that I was surprised to see years ago, and I actually encountered the bug at work. There is a constant named MAX_UID_T_LEN to denote the maximum number of characters that a UID can have. For whatever reason, LDAP-synced UIDs at work had many digits and sudo would fail to work because the code only worked UIDs that have MAX_UID_T_LEN - 1 characters.

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.

Hopefully a there's a clear separation between parsing the text files and a small section that allows or disallows.

Are the examples in the advisory statement slightly incorrect? The first example seems to have the user name and host reversed:

    myhost alice = (ALL) /usr/bin/id

All the examples I've seen of sudoers files do it this way:

    alice myhost = (ALL) /usr/bin/id

This is important because the host is rarely used; the host field is usually replaced with ALL, meaning the host name is not important for the rule:

    alice ALL = (ALL) /usr/bin/id

I hope this isn't some new sudoers syntax.

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?

I cannot imagine why would one write (ALL, !root) policy -- this seems like a security hole waiting to happen. There are many system users, and I would not be surprised if some of them can be escalated to root.

Some people do try en enumerate all bad conditions, I have seen this with my own eyes. cp /bin/bash myehell, sudo meshell. I have never seen anyone try to enumerate the user part. I'll be waiting with popcorn.

The fix was done in https://www.sudo.ws/repos/sudo/rev/83db8dba09e7

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.

Status in Debian: https://security-tracker.debian.org/tracker/CVE-2019-14287

Ubuntu: https://people.canonical.com/~ubuntu-security/cve/2019/CVE-2...

RHEL: https://access.redhat.com/security/cve/cve-2019-14287

This seems pretty serious, but I wonder if there is a way to measure how many systems are affected by this. Does anyone have telemetry on how many Runas configs are set up this way? How would someone collect this data?

In my experience not many people are aware of the permission model of sudo and I expect a lot of users are merely granted root access. At the same time those deployments which do use sudo's permission model are likely high cost.

Interesting bug; I wonder what else might be affected by setuid(-1). That said, I’d hope this bug doesn’t really affect too many systems - letting someone run commands as any non-root user is pretty hazardous because some users have really high privileges (e.g. any user in the “docker” group is functionally equivalent to root if Docker is installed).

I do love that this command has its own website - and a delightfully on-point XKCD-inspired logo :)

> 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.

At least the alternative (PolicyKit) reined in on the complexity and is just executing Javascript...

Anyone have any idea if someone can just install the old version in their home directory and enjoy the bypass?

Sudo works by being setuid root. You need some way of becoming root to set that in the first place.

Thanks for answering my question.

Reply on news.ycombinator.com