[systemd-devel] [PATCH] bus-proxy: --clone-smack-label option

2014-11-05 Thread Przemyslaw Kedzierski
This patch adds a '--clone-smack-label' option to systemd-bus-proxyd.
When dbus client connects to systemd-bus-proxyd through Unix domain socket
and this option is enabled
proxy takes client's smack label and sets for itself.

It is done before and independent of dropping privileges.

The reason of such soluton is fact that tests of access rights
performed by lsm may take place inside kernel, not only
in userspace of recipient of message.

The bus-proxyd needs CAP_MAC_ADMIN to manipulate its label.

In case of systemd running in system mode, CAP_MAC_ADMIN
should be added to CapabilityBoundingSet in service file of bus-proxyd.

In case of systemd running in user mode ('systemd --user')
it can be achieved by addition
Capabilities=cap_mac_admin=i and SecureBits=keep-caps
to user@.service file
and setting cap_mac_admin+ei on bus-proxyd binary.

Change-Id: I5a2c77348d4d293dd3707e82349cf624ddaf744a
Signed-off-by: Przemyslaw Kedzierski p.kedzier...@samsung.com
---
 man/systemd-bus-proxyd.xml  |  9 +
 src/bus-proxyd/bus-proxyd.c | 37 +
 src/shared/capability.c | 18 ++
 src/shared/capability.h |  2 ++
 src/shared/smack-util.c | 18 ++
 src/shared/smack-util.h |  1 +
 6 files changed, 85 insertions(+)

diff --git a/man/systemd-bus-proxyd.xml b/man/systemd-bus-proxyd.xml
index f9400f0..0aa24cf 100644
--- a/man/systemd-bus-proxyd.xml
+++ b/man/systemd-bus-proxyd.xml
@@ -87,6 +87,15 @@ along with systemd; If not, see 
http://www.gnu.org/licenses/.
 /listitem
   /varlistentry
 
+  varlistentry
+termoption--clone-smack-label/option/term
+
+listitem
+  paraTake client's smack label and set for itself.
+  The commandsystemd-bus-proxyd/command needs CAP_MAC_ADMIN to 
manipulate it./para
+/listitem
+  /varlistentry
+
   xi:include href=standard-options.xml xpointer=help /
   xi:include href=standard-options.xml xpointer=version /
 /variablelist
diff --git a/src/bus-proxyd/bus-proxyd.c b/src/bus-proxyd/bus-proxyd.c
index d10de2f..ae8cd02 100644
--- a/src/bus-proxyd/bus-proxyd.c
+++ b/src/bus-proxyd/bus-proxyd.c
@@ -45,11 +45,13 @@
 #include def.h
 #include capability.h
 #include bus-policy.h
+#include smack-util.h
 
 static char *arg_address = NULL;
 static char *arg_command_line_buffer = NULL;
 static bool arg_drop_privileges = false;
 static char **arg_configuration = NULL;
+static bool arg_clone_smack_label = false;
 
 static int help(void) {
 
@@ -58,6 +60,7 @@ static int help(void) {
  -h --help   Show this help\n
 --versionShow package version\n
 --drop-privilegesDrop privileges\n
+--clone-smack-label  Clone smack label\n
 --configuration=PATH Configuration file or directory\n
 --machine=MACHINEConnect to specified machine\n
 --address=ADDRESSConnect to the bus specified by 
ADDRESS\n
@@ -75,6 +78,7 @@ static int parse_argv(int argc, char *argv[]) {
 ARG_DROP_PRIVILEGES,
 ARG_CONFIGURATION,
 ARG_MACHINE,
+ARG_CLONE_SMACK_LABEL,
 };
 
 static const struct option options[] = {
@@ -84,6 +88,7 @@ static int parse_argv(int argc, char *argv[]) {
 { drop-privileges, no_argument,   NULL, 
ARG_DROP_PRIVILEGES },
 { configuration,   required_argument, NULL, 
ARG_CONFIGURATION   },
 { machine, required_argument, NULL, ARG_MACHINE  
   },
+{ clone-smack-label, no_argument, NULL, 
ARG_CLONE_SMACK_LABEL },
 {},
 };
 
@@ -149,6 +154,9 @@ static int parse_argv(int argc, char *argv[]) {
 break;
 }
 
+case ARG_CLONE_SMACK_LABEL:
+arg_clone_smack_label = true;
+break;
 case '?':
 return -EINVAL;
 
@@ -1168,6 +1176,35 @@ int main(int argc, char *argv[]) {
 if (is_unix) {
 (void) getpeercred(in_fd, ucred);
 (void) getpeersec(in_fd, peersec);
+
+if (arg_clone_smack_label) {
+
+if (!mac_smack_use()) {
+log_warning(No SMACK found);
+goto exit_clone_smack_label;
+}
+
+if (!peersec) {
+log_warning(Invalid SMACK label);
+goto exit_clone_smack_label;
+}
+
+r = have_effective_cap(CAP_MAC_ADMIN);
+if (r = 0) {
+log_warning(No CAP_MAC_ADMIN capability);
+goto exit_clone_smack_label;
+}
+

Re: [systemd-devel] [PATCH] bus-proxy: --clone-smack-label option

2014-11-05 Thread Lennart Poettering
On Wed, 05.11.14 16:08, Przemyslaw Kedzierski (p.kedzier...@samsung.com) wrote:

 This patch adds a '--clone-smack-label' option to systemd-bus-proxyd.
 When dbus client connects to systemd-bus-proxyd through Unix domain socket
 and this option is enabled
 proxy takes client's smack label and sets for itself.

Why is this an option? Shouldn't this be the default behaviour?
 +if (arg_clone_smack_label) {
 +
 +if (!mac_smack_use()) {
 +log_warning(No SMACK found);
 +goto exit_clone_smack_label;
 +}
 +
 +if (!peersec) {
 +log_warning(Invalid SMACK label);
 +goto exit_clone_smack_label;
 +}
 +
 +r = have_effective_cap(CAP_MAC_ADMIN);
 +if (r = 0) {
 +log_warning(No CAP_MAC_ADMIN capability);
 +goto exit_clone_smack_label;
 +}

Instead of checking the cap in userspace we should just try to update
the label right away and let the kernel decide whether it wants to
allow this or not...

 +
 @@ -33,3 +33,4 @@ int mac_smack_apply(const char *path, const char *label);
  int mac_smack_apply_fd(int fd, const char *label);
  int mac_smack_apply_ip_in_fd(int fd, const char *label);
  int mac_smack_apply_ip_out_fd(int fd, const char *label);
 +int mac_smack_set_current_label(char *label);

The selinux code has a code mac_selinux_get_our_label(). I figure we
should unify the naming scheme here... 

I don't really like the word current in this context, since, well,
of course it's current, and we don't use the word current for any
other calls...

As a matter of fact I actually don't like the our bit in the
existing selinux call, since it's not clear why that's supposed to be
plural.

Maybe name the SMACK call:

int mac_smack_set_process_label(const char *label);

And then rename the the selinux call:

int mac_selinux_get_process_label(char **label);

(I'll make the selinux change, please just rename the SMACK call as
pointed out.)

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel