From d7aea443d9bc0b1f37a2c31d0d476d61ff41fb66 Mon Sep 17 00:00:00 2001 From: William Roberts Date: Thu, 1 Oct 2015 16:03:47 -0700 Subject: [PATCH 1/2] property_service: log pid,uid and gid of setprop client When auditing setprop denials, it is often unclear of who the process is in a multi-process domain. To help identify the invoker, log the pid, uid, and gid of the caller. Before: avc: denied { set } for property=wifi.xxx ... After: avc: denied { set } for property=wifi.xxx pid=30691 uid=123 gid=345 ... Change-Id: I5cdcb3d18fbd52e0987b5e1497b9f6620c6c742a Signed-off-by: William Roberts --- init/init.cpp | 11 ++++++++++- init/property_service.cpp | 20 ++++++++++++-------- init/property_service.h | 6 ++++++ 3 files changed, 28 insertions(+), 9 deletions(-) diff --git a/init/init.cpp b/init/init.cpp index ee1351d95..a898b03e3 100644 --- a/init/init.cpp +++ b/init/init.cpp @@ -467,7 +467,16 @@ int selinux_reload_policy(void) } static int audit_callback(void *data, security_class_t /*cls*/, char *buf, size_t len) { - snprintf(buf, len, "property=%s", !data ? "NULL" : (char *)data); + + property_audit_data *d = reinterpret_cast(data); + + if (!d || !d->name || !d->cr) { + ERROR("audit_callback invoked with null data arguments!"); + return 0; + } + + snprintf(buf, len, "property=%s pid=%d uid=%d gid=%d", d->name, + d->cr->pid, d->cr->uid, d->cr->gid); return 0; } diff --git a/init/property_service.cpp b/init/property_service.cpp index aa939a58d..579b92d5f 100644 --- a/init/property_service.cpp +++ b/init/property_service.cpp @@ -90,10 +90,11 @@ void property_init() { } } -static int check_mac_perms(const char *name, char *sctx) +static int check_mac_perms(const char *name, char *sctx, struct ucred *cr) { char *tctx = NULL; int result = 0; + property_audit_data audit_data; if (!sctx) goto err; @@ -104,7 +105,10 @@ static int check_mac_perms(const char *name, char *sctx) if (selabel_lookup(sehandle_prop, &tctx, name, 1) != 0) goto err; - if (selinux_check_access(sctx, tctx, "property_service", "set", (void*) name) == 0) + audit_data.name = name; + audit_data.cr = cr; + + if (selinux_check_access(sctx, tctx, "property_service", "set", reinterpret_cast(&audit_data)) == 0) result = 1; freecon(tctx); @@ -112,7 +116,7 @@ static int check_mac_perms(const char *name, char *sctx) return result; } -static int check_control_mac_perms(const char *name, char *sctx) +static int check_control_mac_perms(const char *name, char *sctx, struct ucred *cr) { /* * Create a name prefix out of ctl. @@ -126,19 +130,19 @@ static int check_control_mac_perms(const char *name, char *sctx) if (ret < 0 || (size_t) ret >= sizeof(ctl_name)) return 0; - return check_mac_perms(ctl_name, sctx); + return check_mac_perms(ctl_name, sctx, cr); } /* * Checks permissions for setting system properties. * Returns 1 if uid allowed, 0 otherwise. */ -static int check_perms(const char *name, char *sctx) +static int check_perms(const char *name, char *sctx, struct ucred *cr) { if(!strncmp(name, "ro.", 3)) name +=3; - return check_mac_perms(name, sctx); + return check_mac_perms(name, sctx, cr); } std::string property_get(const char* name) { @@ -314,14 +318,14 @@ static void handle_property_set_fd() // Keep the old close-socket-early behavior when handling // ctl.* properties. close(s); - if (check_control_mac_perms(msg.value, source_ctx)) { + if (check_control_mac_perms(msg.value, source_ctx, &cr)) { handle_control_message((char*) msg.name + 4, (char*) msg.value); } else { ERROR("sys_prop: Unable to %s service ctl [%s] uid:%d gid:%d pid:%d\n", msg.name + 4, msg.value, cr.uid, cr.gid, cr.pid); } } else { - if (check_perms(msg.name, source_ctx)) { + if (check_perms(msg.name, source_ctx, &cr)) { property_set((char*) msg.name, (char*) msg.value); } else { ERROR("sys_prop: permission denied uid:%d name:%s\n", diff --git a/init/property_service.h b/init/property_service.h index 51d740450..1a48fb188 100644 --- a/init/property_service.h +++ b/init/property_service.h @@ -18,9 +18,15 @@ #define _INIT_PROPERTY_H #include +#include #include #include +struct property_audit_data { + ucred *cr; + const char* name; +}; + extern void property_init(void); extern void property_load_boot_defaults(void); extern void load_persist_props(void); From 468573930df71230ab43a356d2b7c2e960a2f1ea Mon Sep 17 00:00:00 2001 From: William Roberts Date: Tue, 6 Oct 2015 12:03:01 -0700 Subject: [PATCH 2/2] debuggerd: audit pid, uid and gid on SE Linux denial When debugging SE Linux audit messages from debuggerd, its unclear what process is triggering the access violation. To assist in debugging, we also log pid, uid and gid. Before: avc: denied { dump_backtrace } for scontext=u:r:dumpstate:s0 ... After: avc: denied { dump_backtrace } for pid=198 uid=1019 gid=1019 .. Change-Id: I8263e6f5e77917139b73c3e84b76f7f97fd98003 Signed-off-by: William Roberts --- debuggerd/debuggerd.cpp | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/debuggerd/debuggerd.cpp b/debuggerd/debuggerd.cpp index 599995cfd..713638d11 100644 --- a/debuggerd/debuggerd.cpp +++ b/debuggerd/debuggerd.cpp @@ -130,31 +130,44 @@ static const char *debuggerd_perms[] = { "dump_backtrace" }; -static bool selinux_action_allowed(int s, pid_t tid, debugger_action_t action) +static int audit_callback(void* data, security_class_t /* cls */, char* buf, size_t len) +{ + struct debugger_request_t* req = reinterpret_cast(data); + + if (!req) { + ALOGE("No debuggerd request audit data"); + return 0; + } + + snprintf(buf, len, "pid=%d uid=%d gid=%d", req->pid, req->uid, req->gid); + return 0; +} + +static bool selinux_action_allowed(int s, debugger_request_t* request) { char *scon = NULL, *tcon = NULL; const char *tclass = "debuggerd"; const char *perm; bool allowed = false; - if (action <= 0 || action >= (sizeof(debuggerd_perms)/sizeof(debuggerd_perms[0]))) { - ALOGE("SELinux: No permission defined for debugger action %d", action); + if (request->action <= 0 || request->action >= (sizeof(debuggerd_perms)/sizeof(debuggerd_perms[0]))) { + ALOGE("SELinux: No permission defined for debugger action %d", request->action); return false; } - perm = debuggerd_perms[action]; + perm = debuggerd_perms[request->action]; if (getpeercon(s, &scon) < 0) { ALOGE("Cannot get peer context from socket\n"); goto out; } - if (getpidcon(tid, &tcon) < 0) { - ALOGE("Cannot get context for tid %d\n", tid); + if (getpidcon(request->tid, &tcon) < 0) { + ALOGE("Cannot get context for tid %d\n", request->tid); goto out; } - allowed = (selinux_check_access(scon, tcon, tclass, perm, NULL) == 0); + allowed = (selinux_check_access(scon, tcon, tclass, perm, reinterpret_cast(request)) == 0); out: freecon(scon); @@ -225,7 +238,7 @@ static int read_request(int fd, debugger_request_t* out_request) { return -1; } - if (!selinux_action_allowed(fd, out_request->tid, out_request->action)) + if (!selinux_action_allowed(fd, out_request)) return -1; } else { // No one else is allowed to dump arbitrary processes. @@ -566,6 +579,8 @@ static void usage() { int main(int argc, char** argv) { union selinux_callback cb; if (argc == 1) { + cb.func_audit = audit_callback; + selinux_set_callback(SELINUX_CB_AUDIT, cb); cb.func_log = selinux_log_callback; selinux_set_callback(SELINUX_CB_LOG, cb); return do_server();