No Passenger Issue (was: Invoke setgroup() with empty groups as fallback in switchGroup() in ExecHelperMain.cpp)

[kbabioch]

In the switchGroup() function there should be a fallback for the setgroup() invocation in case of failures, so that if anything goes wrong the user is not a member of any supplementary group(s) rather than being a member of some group(s).

These checks, for instance, could return an unexpected value:

passenger/src/agent/ExecHelper/ExecHelperMain.cpp

Line 192 in 8e733e8

int ret = getgrouplist(userInfo->pw_name, gid,

passenger/src/agent/ExecHelper/ExecHelperMain.cpp

Line 201 in 8e733e8

if (ngroups <= NGROUPS_MAX) {

In these cases setgroups() is not invoked at all. The default, however, should be that it is invoked with an empty list of groups. Since you already keep track of whether setgroups has been invoked with the setgroupsCalled flag, it should be easy to check for this in the end of the function.

[FooBarWidget]

Thanks for reporting this issue, but I don't really get it. You're saying that if ngroups > NGROUPS_MAX, then setgroups() is not called at all. That's true. However, if setgroups() is not called then the if (!setgroupsCalled && initgroups(userInfo->pw_name, gid) == -1) ensures that in any case initgroups() is called. Doesn't that already ensure that the right supplementary groups are set?

Actually, now I look at the code, the getgrouplist()/setgroups() parts seem to be relics from an earlier version of the codebase, before a large refactoring happened. I think just calling initgroups() here is already sufficient.

[FooBarWidget]

Closing this for now until the case is confirmed.

[kbabioch]

@FooBarWidget You are right with your analysis. I've misjudged the situation and overlooked the setgroupsCalled logic. This issue can therefore be closed as invalid. Sorry for the noise.