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

Tenshi should create its PID file before dropping privileges (CVE-2017-11746) #6

Closed
orlitzky opened this Issue Jul 28, 2017 · 11 comments

Comments

Projects
None yet
2 participants
@orlitzky
Contributor

orlitzky commented Jul 28, 2017

As part of the recent OpenRC init script changes, I found the following vulnerability regarding signal handling. Basically, tenshi should creates its PID file before it drops privileges to a restricted user (say, the tenshi user).

Why? Well, imagine what you might like to do with the PID of the tenshi process: send a signal to the daemon! But if the PID file is created as the tenshi user, then the tenshi user can replace what's in the PID file with e.g. 1. Now, any attempt to stop tenshi will instead stop PID 1 and reboot the system. To prevent that, the PID file should be created as root.

After this change is made, I'll need to fix the init script once more to make sure that the restricted user can't write to the directory containing its PID file (for the same reasons). From a quick glance, it looks like the other init scripts have the same problem.

@abarisani

This comment has been minimized.

Show comment
Hide comment
@abarisani

abarisani Jul 29, 2017

Contributor

I would hardly consider this a vulnerability as the tenshi user is not a generic user controlled process, but a system one.

I also think that saving the PID before dropping privileges would add complications in automatically removing it. It is common practice for daemons to clean up their own pids and to save it with their running user privileges.

If you would like to improve this then the init script themselves can either check the PID user or simply not relying on the PID file itself (which is a fairly old fashioned practice I would say).

Pull requests for init scripts are welcome, but I would avoid changing the tenshi executable logic itself to handle this as you suggest.

Contributor

abarisani commented Jul 29, 2017

I would hardly consider this a vulnerability as the tenshi user is not a generic user controlled process, but a system one.

I also think that saving the PID before dropping privileges would add complications in automatically removing it. It is common practice for daemons to clean up their own pids and to save it with their running user privileges.

If you would like to improve this then the init script themselves can either check the PID user or simply not relying on the PID file itself (which is a fairly old fashioned practice I would say).

Pull requests for init scripts are welcome, but I would avoid changing the tenshi executable logic itself to handle this as you suggest.

@abarisani abarisani closed this Jul 29, 2017

@abarisani abarisani added the wontfix label Jul 29, 2017

@orlitzky

This comment has been minimized.

Show comment
Hide comment
@orlitzky

orlitzky Jul 29, 2017

Contributor

I would hardly consider this a vulnerability as the tenshi user is not a generic user controlled process, but a system one.

The whole point of running as a restricted user is to limit what an attacker can do if he gains control of the tenshi process. Letting him e.g. shutdown the machine is a vulnerability, because that's exactly the sort of thing you're trying to prevent.

If you would like to improve this then the init script themselves can either check the PID user or simply not relying on the PID file itself (which is a fairly old fashioned practice I would say).

Can you write a bash function that does either of those things? It's not possible, which is why daemons write their own PID files.

Contributor

orlitzky commented Jul 29, 2017

I would hardly consider this a vulnerability as the tenshi user is not a generic user controlled process, but a system one.

The whole point of running as a restricted user is to limit what an attacker can do if he gains control of the tenshi process. Letting him e.g. shutdown the machine is a vulnerability, because that's exactly the sort of thing you're trying to prevent.

If you would like to improve this then the init script themselves can either check the PID user or simply not relying on the PID file itself (which is a fairly old fashioned practice I would say).

Can you write a bash function that does either of those things? It's not possible, which is why daemons write their own PID files.

@abarisani

This comment has been minimized.

Show comment
Hide comment
@abarisani

abarisani Jul 31, 2017

Contributor

The tenshi user would anyway have many ways to perform a trivial DoS of the system, I don't consider a shutdown as a critical vulnerability in this context. A compromise of the tenshi user would be much more valuable in terms of accessed logs for instance. I think it's a very marginal scenario which doesn't worth the potential hassle of fixing this within the tenshi process.

As I said I am not opposed in having more sanity checks, just that they probably better belong to the init scripts.

It is certainly possible to infer the PID of a spawned file or at least to verify its user, I can think of countless ways of doing so (pidof, fuser). Even start-stop-daemon itself doesn't strictly require --pid or --pidfile to work. Also cat /proc//status |grep -E '^Uid:' can be used to verify the uid of a process. As you can see it's far from being "not possible".

But anyway this remains a "wontfix" for me as I consider this a non issue, having said that I always welcome pull requests if you have any that can cleanly deal with this issue and without causing side effects.

Contributor

abarisani commented Jul 31, 2017

The tenshi user would anyway have many ways to perform a trivial DoS of the system, I don't consider a shutdown as a critical vulnerability in this context. A compromise of the tenshi user would be much more valuable in terms of accessed logs for instance. I think it's a very marginal scenario which doesn't worth the potential hassle of fixing this within the tenshi process.

As I said I am not opposed in having more sanity checks, just that they probably better belong to the init scripts.

It is certainly possible to infer the PID of a spawned file or at least to verify its user, I can think of countless ways of doing so (pidof, fuser). Even start-stop-daemon itself doesn't strictly require --pid or --pidfile to work. Also cat /proc//status |grep -E '^Uid:' can be used to verify the uid of a process. As you can see it's far from being "not possible".

But anyway this remains a "wontfix" for me as I consider this a non issue, having said that I always welcome pull requests if you have any that can cleanly deal with this issue and without causing side effects.

@orlitzky orlitzky changed the title from Tenshi should create its PID file before dropping privileges to Tenshi should create its PID file before dropping privileges (CVE-2017-11746) Aug 1, 2017

@orlitzky

This comment has been minimized.

Show comment
Hide comment
@orlitzky

orlitzky Aug 1, 2017

Contributor

Both pidof and process table grepping have the same problem, that they don't necessarily find the right PID. An init script should stop the daemon that it started, not some other process with the same executable name.

$ pidof emacs
21650 21427

Which one of those should the init script stop?

Anyway, this has been assigned CVE-2017-11746.

Contributor

orlitzky commented Aug 1, 2017

Both pidof and process table grepping have the same problem, that they don't necessarily find the right PID. An init script should stop the daemon that it started, not some other process with the same executable name.

$ pidof emacs
21650 21427

Which one of those should the init script stop?

Anyway, this has been assigned CVE-2017-11746.

@abarisani

This comment has been minimized.

Show comment
Hide comment
@abarisani

abarisani Aug 1, 2017

Contributor

The /proc/status proposal should work well to this end.

Again I am happy to apply pull requests or shall I take that you won't contribute any fix to this?

Contributor

abarisani commented Aug 1, 2017

The /proc/status proposal should work well to this end.

Again I am happy to apply pull requests or shall I take that you won't contribute any fix to this?

@abarisani

This comment has been minimized.

Show comment
Hide comment
@abarisani

abarisani Aug 1, 2017

Contributor

Just to be absolutely clear, I don't object the relevance of the issue. I object to the fact that this is considered a vulnerability within tenshi itself, as I think it's not (I also think that this is an enhancement if anything and not a vulnerability, otherwise shall we add a CVE to every single process that runs as root, even if not vulnerable by themselves?).

We are not "letting" the tenshi user "to shutdown the machine", the tenshi user is merely publishing information that the init scripts are blindly trusting.

For this reason I think that init scripts should be more careful in getting PID information, and correlating it with the supposed user of execution, rather than modifying tenshi logic.

If we save the pid within tenshi before dropping privileges then we have the issue of leaving the pid file as left over as we can't unlink it, this would case programs like start-stop-daemon to kill the wrong process at the next iteration.

If you think this can be worked around cleanly then I am open to suggestions.

Contributor

abarisani commented Aug 1, 2017

Just to be absolutely clear, I don't object the relevance of the issue. I object to the fact that this is considered a vulnerability within tenshi itself, as I think it's not (I also think that this is an enhancement if anything and not a vulnerability, otherwise shall we add a CVE to every single process that runs as root, even if not vulnerable by themselves?).

We are not "letting" the tenshi user "to shutdown the machine", the tenshi user is merely publishing information that the init scripts are blindly trusting.

For this reason I think that init scripts should be more careful in getting PID information, and correlating it with the supposed user of execution, rather than modifying tenshi logic.

If we save the pid within tenshi before dropping privileges then we have the issue of leaving the pid file as left over as we can't unlink it, this would case programs like start-stop-daemon to kill the wrong process at the next iteration.

If you think this can be worked around cleanly then I am open to suggestions.

@abarisani abarisani reopened this Aug 1, 2017

@abarisani abarisani removed the wontfix label Aug 1, 2017

@abarisani

This comment has been minimized.

Show comment
Hide comment
@abarisani

abarisani Aug 2, 2017

Contributor

I still think that this is not a tenshi vulnerability, doesn't warrant a CVE and that should be addressed elsewhere.

Nonetheless, rather than further disputing this, I made the following changes, please review 46b0148

tenshi now inconveniently creates the PID file before sanity checks are being performed (e..g checking that log files are readable to the tenshi user), but there was no other easy way to address the change.

Contributor

abarisani commented Aug 2, 2017

I still think that this is not a tenshi vulnerability, doesn't warrant a CVE and that should be addressed elsewhere.

Nonetheless, rather than further disputing this, I made the following changes, please review 46b0148

tenshi now inconveniently creates the PID file before sanity checks are being performed (e..g checking that log files are readable to the tenshi user), but there was no other easy way to address the change.

@orlitzky

This comment has been minimized.

Show comment
Hide comment
@orlitzky

orlitzky Aug 2, 2017

Contributor

Thanks for working on this. I know you didn't want to fix it in Tenshi, but I still think that reordering the setuid/savepid in the daemon is a safer approach than trying to fix all of the init scripts. Especially with sysvinit scripts, the best alternative that we found was to check the process uid against the pidfile, and that was going to require doing something OS-specific (meaning more init scripts would have to be modified).

I tried this version, and the pid file is created in the right place and with the right ownership, but it contains the wrong pid =)

I think it's writing the pre-fork PID rather than the one for the daemonized process.

Contributor

orlitzky commented Aug 2, 2017

Thanks for working on this. I know you didn't want to fix it in Tenshi, but I still think that reordering the setuid/savepid in the daemon is a safer approach than trying to fix all of the init scripts. Especially with sysvinit scripts, the best alternative that we found was to check the process uid against the pidfile, and that was going to require doing something OS-specific (meaning more init scripts would have to be modified).

I tried this version, and the pid file is created in the right place and with the right ownership, but it contains the wrong pid =)

I think it's writing the pre-fork PID rather than the one for the daemonized process.

@orlitzky

This comment has been minimized.

Show comment
Hide comment
@orlitzky

orlitzky Aug 2, 2017

Contributor

Moving

save_pid();
prepare_process();

to the end of daemonize() seems to work.

Contributor

orlitzky commented Aug 2, 2017

Moving

save_pid();
prepare_process();

to the end of daemonize() seems to work.

@abarisani

This comment has been minimized.

Show comment
Hide comment
@abarisani

abarisani Aug 2, 2017

Contributor

Fixed in d0e7f28

Please confirm and close. Thx!

Contributor

abarisani commented Aug 2, 2017

Fixed in d0e7f28

Please confirm and close. Thx!

@orlitzky

This comment has been minimized.

Show comment
Hide comment
@orlitzky

orlitzky Aug 2, 2017

Contributor

It's working great with OpenRC, thanks.

Contributor

orlitzky commented Aug 2, 2017

It's working great with OpenRC, thanks.

@orlitzky orlitzky closed this Aug 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment