switch_user_group(): rpmlint reports call to setgroups before setuid

[jfkw]

switch_user_group(): rpmlint reports call to setgroups before setuid:

Building coreos-metadata-3.0.1, Crate users is pulled in via dependency crate update-ssh-keys-0.3.0. rpmlint reports the warning:

RPMLINT report:
===============
coreos-metadata.x86_64:
W: missing-call-to-setgroups-before-setuid /usr/bin/coreos-metadata
This executable is calling setuid and setgid without setgroups or initgroups.
There is a high probability this means it didn't relinquish all groups, and
this would be a potential security issue to be fixed. Seek POS36-C on the web
for details about the problem.

The warning may not indicate an actual problem, but it would be helpful to eliminate the warning. Upstream https://github.com/coreos/update-ssh-keys and https://github.com/coreos/coreos-metadata have been notified via coreos/afterburn#118.

Per @lucab discussion in that issue:

The warning is related to switch_user_group in the users crate (all versions):

rust-users/src/switch.rs

Lines 134 to 143 in 15af157

	pub fn switch_user_group(uid: uid_t, gid: gid_t) -> IOResult<SwitchUserGuard> {
	let current_state = SwitchUserGuard {
	uid: get_effective_uid(),
	gid: get_effective_gid(),
	};
	try!(set_effective_gid(gid));
	try!(set_effective_uid(uid));
	Ok(current_state)
	}

We are calling that in update-ssh-keys: https://github.com/coreos/update-ssh-keys/blob/v0.3.0/src/lib.rs#L108

In our specific case I think this is not a security bug. We are calling that method in order to align user/group on file creation only, not to drop privileges for the process (those are reset when the guard value is dropped at the end of the function).

[ogham]

Interesting! I think you’re right in saying this doesn’t affect your case, but rust-users is a general library and someone else is bound to run into the same problem eventually. Also it looks like doing the Rust equivalent of setgroups(0, NULL); is enough to make the warning go away.

[lucab]

@ogham if you are going that route, it looks like the guard should record getgroups state before resetting groups and recover them back on drop.

[ogham]

This doesn’t go all the way to solving the problem, but I’ve changed the code to setgid before setuid in f1b9902.