From 417770f3033c426ca848b158d0bf057cd8ad1329 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 9 Jul 2020 23:15:47 +0200 Subject: [PATCH 1/3] basic/cap-list: parse/print numerical capabilities We would refuse to print capabilities which were didn't have a name for. The kernel adds new capabilities from time to time, most recently cap_bpf. 'systmectl show -p CapabilityBoundingSet ...' would fail with "Failed to parse bus message: Invalid argument" because capability_set_to_string_alloc() would fail with -EINVAL. So let's print such capabilities in hexadecimal: CapabilityBoundingSet=cap_chown cap_dac_override cap_dac_read_search cap_fowner cap_fsetid cap_kill cap_setgid cap_setuid cap_setpcap cap_linux_immutable cap_net_bind_service cap_net_broadcast cap_net_admin cap_net_raw cap_ipc_lock cap_ipc_owner 0x10 0x11 0x12 0x13 0x14 0x15 0x16 0x17 0x18 0x19 0x1a ... For symmetry, also allow capabilities that we don't know to be specified. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1853736. --- src/basic/cap-list.c | 10 +++++++--- src/test/test-cap-list.c | 4 +++- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/basic/cap-list.c b/src/basic/cap-list.c index 84083b4544..af27d84ba4 100644 --- a/src/basic/cap-list.c +++ b/src/basic/cap-list.c @@ -9,6 +9,7 @@ #include "extract-word.h" #include "macro.h" #include "parse-util.h" +#include "stdio-util.h" #include "util.h" static const struct capability_name* lookup_capability(register const char *str, register GPERF_LEN_TYPE len); @@ -36,7 +37,7 @@ int capability_from_name(const char *name) { /* Try to parse numeric capability */ r = safe_atoi(name, &i); if (r >= 0) { - if (i >= 0 && (size_t) i < ELEMENTSOF(capability_names)) + if (i >= 0 && i < 64) return i; else return -EINVAL; @@ -64,11 +65,14 @@ int capability_set_to_string_alloc(uint64_t set, char **s) { for (i = 0; i <= cap_last_cap(); i++) if (set & (UINT64_C(1) << i)) { const char *p; + char buf[2 + 16 + 1]; size_t add; p = capability_to_name(i); - if (!p) - return -EINVAL; + if (!p) { + xsprintf(buf, "0x%lx", i); + p = buf; + } add = strlen(p); diff --git a/src/test/test-cap-list.c b/src/test/test-cap-list.c index 563b996941..33dd2461c3 100644 --- a/src/test/test-cap-list.c +++ b/src/test/test-cap-list.c @@ -31,6 +31,8 @@ static void test_cap_list(void) { assert_se(capability_from_name("cAp_aUdIt_rEAd") == CAP_AUDIT_READ); assert_se(capability_from_name("0") == 0); assert_se(capability_from_name("15") == 15); + assert_se(capability_from_name("63") == 63); + assert_se(capability_from_name("64") == -EINVAL); assert_se(capability_from_name("-1") == -EINVAL); for (i = 0; i < capability_list_length(); i++) { @@ -65,7 +67,7 @@ static void test_capability_set_one(uint64_t c, const char *t) { free(t1); assert_se(t1 = strjoin("'cap_chown cap_dac_override' \"cap_setgid cap_setuid\"", t, - " hogehoge foobar 12345 3.14 -3 ", t)); + " hogehoge foobar 18446744073709551616 3.14 -3 ", t)); assert_se(capability_set_from_string(t1, &c1) == 0); assert_se(c1 == c_masked); } From 864a25d99bb523e7a5c166771e3ddbf39baffd33 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 10 Jul 2020 16:53:51 +0200 Subject: [PATCH 2/3] basic/capability-util: let cap_last_cap() return unsigned integer We never return anything higher than 63, so using "long unsigned" as the type only confused the reader. (We can still use "long unsigned" and safe_atolu() to parse the kernel file.) --- src/basic/cap-list.c | 5 ++--- src/basic/capability-util.c | 31 ++++++++++++------------------- src/basic/capability-util.h | 2 +- src/libsystemd/sd-bus/bus-creds.c | 5 ++--- src/test/test-capability.c | 2 +- 5 files changed, 18 insertions(+), 27 deletions(-) diff --git a/src/basic/cap-list.c b/src/basic/cap-list.c index af27d84ba4..2b7834ad98 100644 --- a/src/basic/cap-list.c +++ b/src/basic/cap-list.c @@ -57,12 +57,11 @@ int capability_list_length(void) { int capability_set_to_string_alloc(uint64_t set, char **s) { _cleanup_free_ char *str = NULL; - unsigned long i; size_t allocated = 0, n = 0; assert(s); - for (i = 0; i <= cap_last_cap(); i++) + for (unsigned i = 0; i <= cap_last_cap(); i++) if (set & (UINT64_C(1) << i)) { const char *p; char buf[2 + 16 + 1]; @@ -70,7 +69,7 @@ int capability_set_to_string_alloc(uint64_t set, char **s) { p = capability_to_name(i); if (!p) { - xsprintf(buf, "0x%lx", i); + xsprintf(buf, "0x%x", i); p = buf; } diff --git a/src/basic/capability-util.c b/src/basic/capability-util.c index ac96eabc03..5a4d020f52 100644 --- a/src/basic/capability-util.c +++ b/src/basic/capability-util.c @@ -31,8 +31,8 @@ int have_effective_cap(int value) { return fv == CAP_SET; } -unsigned long cap_last_cap(void) { - static thread_local unsigned long saved; +unsigned cap_last_cap(void) { + static thread_local unsigned saved; static thread_local bool valid = false; _cleanup_free_ char *content = NULL; unsigned long p = 0; @@ -65,7 +65,7 @@ unsigned long cap_last_cap(void) { if (prctl(PR_CAPBSET_READ, p) < 0) { /* Hmm, look downwards, until we find one that works */ - for (p--; p > 0; p --) + for (p--; p > 0; p--) if (prctl(PR_CAPBSET_READ, p) >= 0) break; @@ -84,12 +84,10 @@ unsigned long cap_last_cap(void) { } int capability_update_inherited_set(cap_t caps, uint64_t set) { - unsigned long i; - /* Add capabilities in the set to the inherited caps, drops capabilities not in the set. * Do not apply them yet. */ - for (i = 0; i <= cap_last_cap(); i++) { + for (unsigned i = 0; i <= cap_last_cap(); i++) { cap_flag_value_t flag = set & (UINT64_C(1) << i) ? CAP_SET : CAP_CLEAR; cap_value_t v; @@ -104,11 +102,10 @@ int capability_update_inherited_set(cap_t caps, uint64_t set) { int capability_ambient_set_apply(uint64_t set, bool also_inherit) { _cleanup_cap_free_ cap_t caps = NULL; - unsigned long i; int r; /* Remove capabilities requested in ambient set, but not in the bounding set */ - for (i = 0; i <= cap_last_cap(); i++) { + for (unsigned i = 0; i <= cap_last_cap(); i++) { if (set == 0) break; @@ -140,7 +137,7 @@ int capability_ambient_set_apply(uint64_t set, bool also_inherit) { return -errno; } - for (i = 0; i <= cap_last_cap(); i++) { + for (unsigned i = 0; i <= cap_last_cap(); i++) { if (set & (UINT64_C(1) << i)) { @@ -167,7 +164,6 @@ int capability_ambient_set_apply(uint64_t set, bool also_inherit) { int capability_bounding_set_drop(uint64_t keep, bool right_now) { _cleanup_cap_free_ cap_t before_cap = NULL, after_cap = NULL; cap_flag_value_t fv; - unsigned long i; int r; /* If we are run as PID 1 we will lack CAP_SETPCAP by default @@ -204,7 +200,7 @@ int capability_bounding_set_drop(uint64_t keep, bool right_now) { if (!after_cap) return -errno; - for (i = 0; i <= cap_last_cap(); i++) { + for (unsigned i = 0; i <= cap_last_cap(); i++) { cap_value_t v; if ((keep & (UINT64_C(1) << i))) @@ -390,7 +386,6 @@ bool ambient_capabilities_supported(void) { } bool capability_quintet_mangle(CapabilityQuintet *q) { - unsigned long i; uint64_t combined, drop = 0; bool ambient_supported; @@ -402,7 +397,7 @@ bool capability_quintet_mangle(CapabilityQuintet *q) { if (ambient_supported) combined |= q->ambient; - for (i = 0; i <= cap_last_cap(); i++) { + for (unsigned i = 0; i <= cap_last_cap(); i++) { unsigned long bit = UINT64_C(1) << i; if (!FLAGS_SET(combined, bit)) continue; @@ -431,16 +426,15 @@ int capability_quintet_enforce(const CapabilityQuintet *q) { int r; if (q->ambient != (uint64_t) -1) { - unsigned long i; bool changed = false; c = cap_get_proc(); if (!c) return -errno; - /* In order to raise the ambient caps set we first need to raise the matching inheritable + permitted - * cap */ - for (i = 0; i <= cap_last_cap(); i++) { + /* In order to raise the ambient caps set we first need to raise the matching + * inheritable + permitted cap */ + for (unsigned i = 0; i <= cap_last_cap(); i++) { uint64_t m = UINT64_C(1) << i; cap_value_t cv = (cap_value_t) i; cap_flag_value_t old_value_inheritable, old_value_permitted; @@ -475,7 +469,6 @@ int capability_quintet_enforce(const CapabilityQuintet *q) { if (q->inheritable != (uint64_t) -1 || q->permitted != (uint64_t) -1 || q->effective != (uint64_t) -1) { bool changed = false; - unsigned long i; if (!c) { c = cap_get_proc(); @@ -483,7 +476,7 @@ int capability_quintet_enforce(const CapabilityQuintet *q) { return -errno; } - for (i = 0; i <= cap_last_cap(); i++) { + for (unsigned i = 0; i <= cap_last_cap(); i++) { uint64_t m = UINT64_C(1) << i; cap_value_t cv = (cap_value_t) i; diff --git a/src/basic/capability-util.h b/src/basic/capability-util.h index b5bce29ab5..fcc59daedc 100644 --- a/src/basic/capability-util.h +++ b/src/basic/capability-util.h @@ -12,7 +12,7 @@ #define CAP_ALL (uint64_t) -1 -unsigned long cap_last_cap(void); +unsigned cap_last_cap(void); int have_effective_cap(int value); int capability_bounding_set_drop(uint64_t keep, bool right_now); int capability_bounding_set_drop_usermode(uint64_t keep); diff --git a/src/libsystemd/sd-bus/bus-creds.c b/src/libsystemd/sd-bus/bus-creds.c index 908b9e75b2..2740be9226 100644 --- a/src/libsystemd/sd-bus/bus-creds.c +++ b/src/libsystemd/sd-bus/bus-creds.c @@ -650,16 +650,15 @@ _public_ int sd_bus_creds_get_description(sd_bus_creds *c, const char **ret) { } static int has_cap(sd_bus_creds *c, size_t offset, int capability) { - unsigned long lc; size_t sz; assert(c); assert(capability >= 0); assert(c->capability); - lc = cap_last_cap(); + unsigned lc = cap_last_cap(); - if ((unsigned long) capability > lc) + if ((unsigned) capability > lc) return 0; /* If the last cap is 63, then there are 64 caps defined, and we need 2 entries รก 32bit hence. * diff --git a/src/test/test-capability.c b/src/test/test-capability.c index f8766256e2..249323f8cf 100644 --- a/src/test/test-capability.c +++ b/src/test/test-capability.c @@ -243,7 +243,7 @@ static void test_ensure_cap_64bit(void) { assert_se(p <= 63); /* Also check for the header definition */ - assert_se(CAP_LAST_CAP <= 63); + assert_cc(CAP_LAST_CAP <= 63); } int main(int argc, char *argv[]) { From 5700780389ddea40df6abd4f54d32214aaf4a640 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 9 Jul 2020 23:23:07 +0200 Subject: [PATCH 3/3] basic/cap-list: reduce scope of variables --- src/basic/cap-list.c | 4 +--- src/test/test-cap-list.c | 6 ++---- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/src/basic/cap-list.c b/src/basic/cap-list.c index 2b7834ad98..b483833947 100644 --- a/src/basic/cap-list.c +++ b/src/basic/cap-list.c @@ -18,7 +18,6 @@ static const struct capability_name* lookup_capability(register const char *str, #include "cap-to-name.h" const char *capability_to_name(int id) { - if (id < 0) return NULL; @@ -94,11 +93,10 @@ int capability_set_to_string_alloc(uint64_t set, char **s) { int capability_set_from_string(const char *s, uint64_t *set) { uint64_t val = 0; - const char *p; assert(set); - for (p = s;;) { + for (const char *p = s;;) { _cleanup_free_ char *word = NULL; int r; diff --git a/src/test/test-cap-list.c b/src/test/test-cap-list.c index 33dd2461c3..81d5c456d7 100644 --- a/src/test/test-cap-list.c +++ b/src/test/test-cap-list.c @@ -12,12 +12,10 @@ /* verify the capability parser */ static void test_cap_list(void) { - int i; - assert_se(!capability_to_name(-1)); assert_se(!capability_to_name(capability_list_length())); - for (i = 0; i < capability_list_length(); i++) { + for (int i = 0; i < capability_list_length(); i++) { const char *n; assert_se(n = capability_to_name(i)); @@ -35,7 +33,7 @@ static void test_cap_list(void) { assert_se(capability_from_name("64") == -EINVAL); assert_se(capability_from_name("-1") == -EINVAL); - for (i = 0; i < capability_list_length(); i++) { + for (int i = 0; i < capability_list_length(); i++) { _cleanup_cap_free_charp_ char *a = NULL; const char *b; unsigned u;