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

RFC: Capabilities #185

Open
wants to merge 49 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@mhaas
Contributor

mhaas commented Apr 3, 2015

I'm submitting a PR as a request for comments/review for capabilities (7) support.

The code drops all privileges except CAP_NET_ADMIN and CAP_NET_RAW and switches the effective user ID to a non-privileged user and group. specified in the config file.

On execvp or popen (e.g. calls to iptables), the code switches the effective UID back to 0. This is needed because there is no way for the child process to inherit these capabilites (1, 2).

This solution is not without problems, as (obviously) an attacker could go back to UID0 and place shell scripts in /etc/cron.d/ or use similar attack vectors. To avoid this problem, a chroot environment would be required. CAP_CHROOT is not permitted anymore, so breaking out would not be a problem.

@acv

This comment has been minimized.

acv commented on configure.in in db3d867 Mar 15, 2015

I see this is indeed a quick hack ;-)

@acv

This comment has been minimized.

acv commented on src/gateway.c in db3d867 Mar 15, 2015

Reload forks and then transfers the running state over the wdctl socket. If reload doesn't have the privileges to call LIBCAP, it will will need to be able to detect this and do nothing.

@acv

This comment has been minimized.

acv commented on src/gateway.c in db3d867 Mar 15, 2015

Maybe wrap with a check if we're already nobody?

User and group should come from config file or command-line.

mhaas added some commits Mar 16, 2015

Add debug output
Also protect capabilities with ifdefs
Capabilities: Fix unnecessary calls
The new strategy is to use seteuid(), in which case we don't need to
care about the capabilities of child processes.
Fix: CAP_NET_ADMIN is actually required, add back
I made some wrong assumptions about execve() in a real-uid 0 context.
Only the file capabilities are all enabled, which still requires the
caller to have the appropriate capabilities. I will add a comment
clarifying the behaviour here.
Refactor capabilities.c and hook up fw_iptables.c
Turns out I was mistaken about some things - I had accidentally
still ran iptables as root.

The current code now drops from UID0 to a non-privileged user
at startup. There are two occasions where the effective
user ID is set back to 0:

* in execute() in util.c
* in fw_iptables.c with a wrapper around popen
@acv

This comment has been minimized.

Contributor

acv commented Apr 3, 2015

Somehow, it's always the non-cyassl build that fails. Looks to be a missing header maybe that gets brought in with cyassl. Try building it without cyassl.

@mhaas

This comment has been minimized.

Contributor

mhaas commented Apr 3, 2015

No, the normal build does not respect CFLAGS=-Werror. For some reason.

@mhaas

This comment has been minimized.

Contributor

mhaas commented Apr 3, 2015

Re your earlier comment:

Reload forks and then transfers the running state over the wdctl socket. If reload doesn't have the >privileges to call LIBCAP, it will will need to be able to detect this and do nothing.

This will also need a switch_to_root() call then. INHERITABLE is probably also not needed in the patch above.

@acv

This comment has been minimized.

Contributor

acv commented Apr 3, 2015

Ahhhh, interesting about -Werror...

caps = cap_get_proc();
debug(LOG_DEBUG, "Dropped caps, now: %s", cap_to_text(caps, NULL));
cap_free(caps);
caps = cap_get_proc();

This comment has been minimized.

@acv

acv Apr 3, 2015

Contributor

Missing a cap_free() after this cap_get_proc()

/* Clear all caps and then set the caps we desire */
cap_clear(caps);
cap_set_flag(caps, CAP_PERMITTED, num_caps, cap_values, CAP_SET);
ret = cap_set_proc(caps);

This comment has been minimized.

@acv

acv Apr 3, 2015

Contributor

Also not cap_free() for this cap_t, my reading of the man page is that cap_set_proc() does not release the memory used by the cap_t.

This comment has been minimized.

@mhaas

mhaas Apr 8, 2015

Contributor

Fixed all occurences.

cap_value_t cap_values[] = { CAP_NET_RAW, CAP_NET_ADMIN };
cap_t caps;
caps = cap_get_proc();

This comment has been minimized.

@acv

acv Apr 3, 2015

Contributor

This allocates ram therefore could return a NULL. Same comment apply to all subsequent cap_get_proc() calls.

This comment has been minimized.

@mhaas

mhaas Apr 8, 2015

Contributor

Fixes.

@acv

This comment has been minimized.

Contributor

acv commented Apr 3, 2015

I'm done adding line comments throughout your patch ;-)

mhaas added some commits Apr 8, 2015

Rename build type cyassl -> full
"full" build type also builds with --enable-libcap
Refactor travis_configure_wrapper
Also fix build and install of libattr
Merge branch 'master' of https://github.com/wifidog/wifidog-gateway i…
…nto feature-capabilities-3

Conflicts:
	src/Makefile.am
	src/util.c
Fix libcap install location
Do not install into lib64/, prefer lib/
Merge branch 'master' of https://github.com/wifidog/wifidog-gateway i…
…nto feature-capabilities-3

Conflicts:
	src/conf.c
@mhaas

This comment has been minimized.

Contributor

mhaas commented Apr 8, 2015

Finally got travis to play nice.

mhaas added some commits Apr 8, 2015

Remove erroneous comment
The comment explained a behaviour which no longer reflected
the code
@mhaas

This comment has been minimized.

Contributor

mhaas commented Jul 26, 2015

Updated so it can be merged.

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