10 ms·

Potential bypass of Runas user restrictions in sudo

93 points by Findus23 7y ago

andrewchoi 7y ago
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 7y ago
  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 7y ago
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 7y ago
  > 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.
aargh_aargh 7y ago
Status in Debian: https://security-tracker.debian.org/tracker/CVE-2019-14287 https://security-tracker.debian.org/tracker/CVE-2019-14287 Ubuntu: https://people.canonical.com/~ubuntu-security/cve/2019/CVE-2019-14287.html https://people.canonical.com/~ubuntu-security/cve/2019/CVE-2... RHEL: https://access.redhat.com/security/cve/cve-2019-14287 https://access.redhat.com/security/cve/cve-2019-14287
hathawsh 7y ago
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?
compressedgas 7y ago
The fix was done in https://www.sudo.ws/repos/sudo/rev/83db8dba09e7 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.
gambler 7y ago
Is this the official sudo repository? https://www.sudo.ws/repos/sudo/file/f75f786eddd5 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?
- jblow 7y ago
  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.
  - gambler 7y ago
    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.
    - jblow 7y ago
CodeWriter23 7y ago
Anyone have any idea if someone can just install the old version in their home directory and enjoy the bypass?
- daxelrod 7y ago
  Sudo works by being setuid root. You need some way of becoming root to set that in the first place.
  - CodeWriter23 7y ago
    Thanks for answering my question.
theamk 7y ago
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 7y ago
  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.
JetSpiegel 7y ago
At least the alternative (PolicyKit) reined in on the complexity and is just executing Javascript...

93 points by Findus23 7y ago

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 7y ago
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 7y ago

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

aargh_aargh 7y ago

Status in Debian: https://security-tracker.debian.org/tracker/CVE-2019-14287 https://security-tracker.debian.org/tracker/CVE-2019-14287 Ubuntu: https://people.canonical.com/~ubuntu-security/cve/2019/CVE-2019-14287.html https://people.canonical.com/~ubuntu-security/cve/2019/CVE-2... RHEL: https://access.redhat.com/security/cve/cve-2019-14287 https://access.redhat.com/security/cve/cve-2019-14287

hathawsh 7y ago

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?

compressedgas 7y ago

The fix was done in https://www.sudo.ws/repos/sudo/rev/83db8dba09e7 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.

gambler 7y ago

Is this the official sudo repository? https://www.sudo.ws/repos/sudo/file/f75f786eddd5 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?

jblow 7y ago
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.
- gambler 7y ago
  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.
  - jblow 7y ago

CodeWriter23 7y ago

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

daxelrod 7y ago
Sudo works by being setuid root. You need some way of becoming root to set that in the first place.
- CodeWriter23 7y ago
  Thanks for answering my question.

theamk 7y ago

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

JetSpiegel 7y ago

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