Add apparmor change hat functionality to fpm #343

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Contributor

notti commented May 14, 2013

With this patch the pool config gets the additional parameter apparmor_hat.
Workers of this pool change their app armor hat to the configured one during spawn.

tlbdk commented Jun 7, 2013

This is very useful for shared hosting, my current workaround is using:

owner /home/ rw,
owner /home/** mrwlkix,

But this would be much nicer. Anything I can do to help get this patch merged?

Contributor

notti commented Jun 7, 2013

Well I also wrote a proposal to the mailing list, but no one answered. So as far as I read the next step would be to create an RFC, let people ask questions/suggest stuff and then put it up for a vote; so I'll try to get RFC karma next;
I'll let you know as soon as I got it up

Contributor

lstrojny commented Jun 8, 2013

@notti the no reaction part could be simply due to the fact, that nobody is using it. And RFC is the way to go.

tlbdk commented Jun 11, 2013

@lstrojny what happens next, will it help if I try it out?

Contributor

notti commented Jun 23, 2013

voting just started and ends on 1.7.2013

So having change_hat functionality has been really useful for apache, and I think it would be great to have it in fpm. I know we have received requests for this functionality in the past.

Hrmm looking at the source, it looks like the change is one direction. That is a return call is never used. In this case it might be better to use aa_change_profile. The change_hat call has the deficiency that if an attacker can obtain the magic_token and do a the return call, then it can escape the confinement. This was necessary as it was designed for task that get reused like in apache_prefork. The magic token keeps the attacker from just being able to directly call the return, because if a wrong token is supplied the task is killed. Of course this assumes that an attacker can get/know the token, and also make the return call, if this is not the case the change_hat is as secure as change_profile.

The aa_change_profile call is designed for a one way transition, where the task does not need to return out of the child confinement. However it does have a deficiency in its current implementation that to transition to a child profile the parent needs to construct a fully qualified path. This basically means the parent needs to know its own confinement (have permission to query its confinement) and the child profile name. So to transition to a child profile like change_hat it needs to do a query like (in C sorry)
if (aa_getcon(&parent_name, NULL) == -1)
goto error;
if (sprintf(&buffer, "%s//%s", parent_name, hat_name) == -1)
goto error;
if (aa_change_profile(buffer) == -1)
goto error;

This of course isn't as convenient as the current change_hat api and will be fixed in the next release of apparmor, but for current versions if you are using hats/child profiles it is less than ideal and change_hat might be the better choice.

Contributor

notti commented Jun 26, 2013

I passed 0 as magic token, which according to the manpage for change_hat should make it impossible to ever change back. So if the implementation of change_hat hasn't changed it should work as expected.
Reasoning behind all this is in the proposal at https://wiki.php.net/rfc/fpm_change_hat. Sorry if i haven't written that part clear enough.

Sadly the man page is wrong. Long ago this was indeed the case but it was changed years ago, I am not sure of exactly when. The man page was not correctly updated and the bug was not caught for years (finally fixed in feb 2011). I have searched back in the kernel code as far as I have direct history and the behavior is that 0 is not a permanent change.

I am sorry for the confusion and the man page being incorrect.

Contributor

notti commented Jun 26, 2013

Woa that is bad news. I'll have a look at it at the weekend.

On Wed, Jun 26, 2013 at 12:11 PM, jjohansen notifications@github.comwrote:

Sadly the man page is wrong. Long ago this was indeed the case but it was
changed years ago, I am not sure of exactly when. The man page was not
correctly updated and the bug was not caught for years (finally fixed in
feb 2011). I have searched back in the kernel code as far as I have direct
history and the behavior is that 0 is not a permanent change.

I am sorry for the confusion and the man page being incorrect.


Reply to this email directly or view it on GitHubhttps://github.com/php/php-src/pull/343#issuecomment-20037622
.

Contributor

notti commented Jun 30, 2013

@jjohansen: Yeah I had a look at it and I think your proposal for a different way looks best - I'll be implementing and testing it tomorrow. As soon as I have it working, I'll create a new pull request, add a reference to the new one and close this one.
Thanks for having a look at the code and the tip with the hats/profiles.

Contributor

notti commented Jul 2, 2013

Fixed in #373

@notti notti closed this Jul 2, 2013

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