Potential bypass of Runas user restrictions in sudo

[gambler]

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?

[notacoward]

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?

[jblow]

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.

[nullc]

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.

[wbkang]

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.

[ape4]

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

[hathawsh]

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?

[theamk]

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.

[6c696e7578]

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.

[compressedgas]

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.

[aargh_aargh]

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

[andrewchoi]

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?

[snagglegaggle]

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.

[nneonneo]

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 :)

[6c696e7578]

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

[JetSpiegel]

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

[CodeWriter23]

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

[daxelrod]

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