Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix mtr-packet cap_net_raw turn on and off #311

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kerolasa
Copy link
Contributor

@kerolasa kerolasa commented Aug 1, 2019

This change allows effective capability bit to be dropped. From now on
mtr-package will internally switch on capabilities for time the command
needs them and no longer. Earlier capabilities were held from start up to
point when they were dropped.

@rewolff
Copy link
Collaborator

rewolff commented Aug 1, 2019

What we need to protect against is that some malicious actor manages say to buffer-overflow something in mtr so that he can then execute code with the permissions of mtr.

What extra protection does it give us if instead of being able to directly exploit the fact that mtr has cap_ the exploit has to "request to enable and THEN exploit?
That would stump just those script kiddies that found a script that worked before your change and requires a slight modification after your change.

What's wrong with my reasoning?

@kerolasa
Copy link
Contributor Author

kerolasa commented Aug 2, 2019

Exploit you are describing would work already because mtr-packet has both e and p bits. It sounds like you want p bit to be removed. Is that correct?

@kerolasa
Copy link
Contributor Author

kerolasa commented Aug 2, 2019

How about keeping capabilities permitted but not effective until sockets are created, and then calling prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0); described in https://www.kernel.org/doc/html/latest/userspace-api/no_new_privs.html

This change allows effective capability bit to be dropped.  From now on
mtr-package will internally switch on capabilities for time the command
needs them and no longer.  Earlier capabilities were held from start up to
point when they were dropped.

After review discussion prctl PR_SET_NO_NEW_PRIVS was added to this change.

Reference: traviscross#311
@kerolasa
Copy link
Contributor Author

kerolasa commented Aug 2, 2019

Use of prctl PR_SET_NO_NEW_PRIVS is implemented in 0e819a6.

@rewolff
Copy link
Collaborator

rewolff commented Aug 3, 2019

A typical privilege escalation bug works as follows:

A program with elevated privileges interacts with the outside world and has a bug. By crafting the interaction with the outside world, other people than the authors of the program get control of code execution in the "elevated privileges" section and therefore get access to the elevated privileges that they shouldn't have had access to.

Mtr needs access to the privilege "access raw sockets". But in the old days mtr this privilege would also come with extra stuff like "can write all files". That would make it very easy to escalate a bug in mtr to full system access. Nowadays once a hacker gets control over mtr, he'll be able to "access raw sockets" but not "write all files". This just takes a bit more effort to escalate further to the "full system access", but you should expect that everything that required "root" in the old situation can be escalated to "root" access. Even when you can't think of a way how to do that. Expect others to be smarter than you in this field.

Now if it is the case that after dropping the privs after using them, the program COULD request them back, then that was wrong all along. That's the whole idea: we drop privs to prevent bugs in the further code from being able to use those privs.

IMHO, dropping the privs for the small part from start to use is unnecessary. When mtr can request to use them, so can a hacker who has control over the control flow in mtr. The code up to the point where they get permanently dropped is the code that needs close scrutiny to prevent bugs from happening.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants