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

setpriv: using "all" for capabilities has multiple issues #1179

Closed
ericonr opened this issue Nov 7, 2020 · 4 comments
Closed

setpriv: using "all" for capabilities has multiple issues #1179

ericonr opened this issue Nov 7, 2020 · 4 comments

Comments

@ericonr
Copy link
Contributor

ericonr commented Nov 7, 2020

If setpriv and its dependencies are built with kernel headers which don't list a certain capability, but are used on a system where that capability is available, any command line option related to capabilities using all gets the following error message:

~ ➜ doas setpriv --inh-caps -all ls
setpriv: libcap-ng is too old for "all" caps

The issues here, as I see them, are:

  • Dropping all capabilities should always be possible, so -all shouldn't generate errors
  • Gaininig +all capabilities isn't possible when not all capabilities are known, so the man page should discourage its usage
  • The man page should warn explicitly about the potential compatibilty issues that can be caused by using setpriv built using different kernel headers from the currently running one.
@ericonr
Copy link
Contributor Author

ericonr commented Nov 9, 2020

I can try and make PRs for these, if you agree with my ideas.

@karelzak
Copy link
Collaborator

Thanks, go ahead.

@ericonr
Copy link
Contributor Author

ericonr commented Nov 14, 2020

@karelzak given this block in lib/caputils.c:

	f = fopen(_PATH_PROC_CAPLASTCAP, "r");
	if (!f) {
		ret = CAP_LAST_CAP;	/* guess */
		return ret;
	}

couldn't a system where /proc isn't mounted (or somehow hidden) possibly return CAP_LAST_CAP erroneously, even though the kernel actually has more available capabilities? For that matter, any failure (including out of memory - possibly introduced by an attacker?) could trigger this code path.

Wouldn't it also be more correct to check that the path in /proc actually is a procfs (using something like statfs(2))?

That said, the "safest" way forward would be to loop from INT_MAX to 0 and dropping each one; at the same time, I don't think cap_last_cap() returning a value greater CAP_LAST_CAP should be considered an error, since, at least with the current cap_last_cap() implementation, it would show that at least there was a /proc/sys/kernel/cap_last_cap file to read from, which I'd actually consider a more trustworthy piece of information than CAP_LAST_CAP being returned as a guess.

@karelzak
Copy link
Collaborator

I guess we can close this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants