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

Set all user groups of user in setuidgid #788

Closed
wants to merge 3 commits into from

Conversation

Grinnz
Copy link
Contributor

@Grinnz Grinnz commented Apr 26, 2015

I have inspected Apache 2.4.10 and nginx 1.6.3 and determined they both use the following behavior to set user and group of their worker processes.

Set user to specified user, and set groups to all groups of specified user, replacing the primary group with the specified group (default same name as user in nginx). Also add specified group as secondary group if not already listed.

This patch removes the ability to set the group without setting the user, or to set a user without a group change (defaulting to the user if unspecified), also matching the behavior of Apache and nginx.

The reason this is necessary is to allow the application to gain the secondary group permissions of the specified user, while still starting as root to bind to privileged ports like 80.

There are no tests or documentation included, but this feature currently has neither, and it would be impossible to test in an automated fashion. However, I have verified that this sets the groups correctly on Centos 6, in perl 5.10.1 and 5.20.2, using the following script (changing user/group/listen as needed).

use Mojolicious::Lite;

app->config(hypnotoad => {
    user => 'grinnz',
    group => 'nginx',
    listen => ['http://*:8081'],
});

get '/' => sub {
    my $c = shift;
    $c->render(json => {
            real_uid => $<,
            eff_uid => $>,
            real_gids => $(,
            eff_gids => $),
    });
};

app->start;

@Grinnz
Copy link
Contributor Author

Grinnz commented Apr 26, 2015

Security implications: Applications that set only the user configuration will be more secure with this change, as they will be required to change from the root groups. Security of setting both user and group will be as expected when changing user: gaining the privileges of the specified user and group only (and none of root's privileges), but including the user's secondary group permissions.

@Grinnz
Copy link
Contributor Author

Grinnz commented Apr 26, 2015

nginx implements this by calling initgroups(3), as seen here: https://github.com/nginx/nginx/blob/ae88e2338f6e27459ace8a23754afc5892c2c5be/src/os/unix/ngx_process_cycle.c#L864

Perl does not have the initgroups syscall, so the closest implementation is to find the groups of which the user is a member, and set those plus the specified group as the secondary groups.

@kraih
Copy link
Member

kraih commented Apr 26, 2015

This is basically an untested security feature. So we need very clear code that has been reviewed very carefully by multiple parties. If it cannot be determined that the current solution is correct, i don't think it should stay, no matter the outcome of the vote. Personally, i'm still in favor of removing the feature completely and letting plugins handle it.

@kraih
Copy link
Member

kraih commented Apr 26, 2015

@jberger @marcusramberg @tempire @jhthorsen @amenonsen What do you think?

@Grinnz
Copy link
Contributor Author

Grinnz commented Apr 26, 2015

Apache also implements this functionality with initgroups, as on this page line 126:

http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/arch/unix/mod_unixd.c?view=markup

On platforms that don't have initgroups, it reimplements them with the function on line 279 of this file:

http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm_common.c?view=markup

It iterates through getgrent(), retrieves the groups which contain the user as a member (skipping the specified group), and sets those groups plus the specified group, which matches with the functionality in the patch.

@kraih
Copy link
Member

kraih commented Apr 26, 2015

@Grinnz So, evidence points towards our current solution being incorrect.

@jhthorsen
Copy link
Member

👊 Let's remove the whole thing. I've never used this myself. I rather use iptables to forward 80/443 to the server https://metacpan.org/pod/Toadfarm::Manual::RunningToadfarm#Listen-to-standard-HTTP-ports

@Grinnz
Copy link
Contributor Author

Grinnz commented Apr 27, 2015

Closing with the release of https://metacpan.org/pod/Mojolicious::Plugin::SetUserGroup

@Grinnz Grinnz closed this Apr 27, 2015
@Grinnz Grinnz deleted the set_user_group branch April 27, 2015 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants