From a570877c12760def86fcf20b151685b6a736cdda Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 16 Jan 2024 11:27:40 +0100 Subject: [PATCH 1/4] varlink: optionally, mark all incoming message's "parameters" field as sensitive So far the varlink logic honoured the "sensitive" flag of output messages. Let's add something similar for input messages. Since we don't really know incoming messages, the flag simply controls whether the "parmaeters" field of all incoming messages should be marked as sensitive. Then, turn this on in the credentials logic and in homed, since both deal with credentials. --- src/creds/creds.c | 7 +------ src/home/homed-manager.c | 2 +- src/shared/varlink.c | 25 +++++++++++++++++++++++-- src/shared/varlink.h | 6 +++++- 4 files changed, 30 insertions(+), 10 deletions(-) diff --git a/src/creds/creds.c b/src/creds/creds.c index c9d1a6e8d94..bbc705c0069 100644 --- a/src/creds/creds.c +++ b/src/creds/creds.c @@ -996,8 +996,6 @@ static int vl_method_encrypt(Varlink *link, JsonVariant *parameters, VarlinkMeth assert(link); - json_variant_sensitive(parameters); - r = varlink_dispatch(link, parameters, dispatch_table, &p); if (r != 0) return r; @@ -1079,9 +1077,6 @@ static int vl_method_decrypt(Varlink *link, JsonVariant *parameters, VarlinkMeth assert(link); - /* Let's also mark the (theoretically encrypted) input as sensitive, in case the NULL encryption scheme was used. */ - json_variant_sensitive(parameters); - r = varlink_dispatch(link, parameters, dispatch_table, &p); if (r != 0) return r; @@ -1144,7 +1139,7 @@ static int run(int argc, char *argv[]) { /* Invocation as Varlink service */ - r = varlink_server_new(&varlink_server, VARLINK_SERVER_ACCOUNT_UID|VARLINK_SERVER_INHERIT_USERDATA); + r = varlink_server_new(&varlink_server, VARLINK_SERVER_ACCOUNT_UID|VARLINK_SERVER_INHERIT_USERDATA|VARLINK_SERVER_INPUT_SENSITIVE); if (r < 0) return log_error_errno(r, "Failed to allocate Varlink server: %m"); diff --git a/src/home/homed-manager.c b/src/home/homed-manager.c index 94b2ea5181a..b1d0c511203 100644 --- a/src/home/homed-manager.c +++ b/src/home/homed-manager.c @@ -998,7 +998,7 @@ static int manager_bind_varlink(Manager *m) { assert(m); assert(!m->varlink_server); - r = varlink_server_new(&m->varlink_server, VARLINK_SERVER_ACCOUNT_UID|VARLINK_SERVER_INHERIT_USERDATA); + r = varlink_server_new(&m->varlink_server, VARLINK_SERVER_ACCOUNT_UID|VARLINK_SERVER_INHERIT_USERDATA|VARLINK_SERVER_INPUT_SENSITIVE); if (r < 0) return log_error_errno(r, "Failed to allocate varlink server object: %m"); diff --git a/src/shared/varlink.c b/src/shared/varlink.c index 699d21745f1..96dfd8de92a 100644 --- a/src/shared/varlink.c +++ b/src/shared/varlink.c @@ -184,6 +184,7 @@ struct Varlink { bool allow_fd_passing_output:1; bool output_buffer_sensitive:1; /* whether to erase the output buffer after writing it to the socket */ + bool input_sensitive:1; /* Whether incoming messages might be sensitive */ int af; /* address family if socket; AF_UNSPEC if not socket; negative if not known */ @@ -703,7 +704,7 @@ static void varlink_clear(Varlink *v) { varlink_clear_current(v); - v->input_buffer = mfree(v->input_buffer); + v->input_buffer = v->input_sensitive ? erase_and_free(v->input_buffer) : mfree(v->input_buffer); v->output_buffer = v->output_buffer_sensitive ? erase_and_free(v->output_buffer) : mfree(v->output_buffer); v->input_control_buffer = mfree(v->input_control_buffer); @@ -1022,7 +1023,8 @@ static int varlink_read(Varlink *v) { } static int varlink_parse_message(Varlink *v) { - const char *e, *begin; + const char *e; + char *begin; size_t sz; int r; @@ -1047,6 +1049,8 @@ static int varlink_parse_message(Varlink *v) { sz = e - begin + 1; r = json_parse(begin, 0, &v->current, NULL, NULL); + if (v->input_sensitive) + explicit_bzero_safe(begin, sz); if (r < 0) { /* If we encounter a parse failure flush all data. We cannot possibly recover from this, * hence drop all buffered data now. */ @@ -1054,6 +1058,13 @@ static int varlink_parse_message(Varlink *v) { return varlink_log_errno(v, r, "Failed to parse JSON: %m"); } + if (v->input_sensitive) { + /* Mark the parameters subfield as sensitive right-away, if that's requested */ + JsonVariant *parameters = json_variant_by_key(v->current, "parameters"); + if (parameters) + json_variant_sensitive(parameters); + } + v->input_buffer_size -= sz; if (v->input_buffer_size == 0) @@ -3097,6 +3108,13 @@ int varlink_set_allow_fd_passing_output(Varlink *v, bool b) { return 0; } +int varlink_set_input_sensitive(Varlink *v) { + assert_return(v, -EINVAL); + + v->input_sensitive = true; + return 0; +} + int varlink_server_new(VarlinkServer **ret, VarlinkServerFlags flags) { _cleanup_(varlink_server_unrefp) VarlinkServer *s = NULL; int r; @@ -3325,6 +3343,9 @@ static int connect_callback(sd_event_source *source, int fd, uint32_t revents, v TAKE_FD(cfd); + if (FLAGS_SET(ss->server->flags, VARLINK_SERVER_INPUT_SENSITIVE)) + varlink_set_input_sensitive(v); + if (ss->server->connect_callback) { r = ss->server->connect_callback(ss->server, v, ss->server->userdata); if (r < 0) { diff --git a/src/shared/varlink.h b/src/shared/varlink.h index a971762a511..418ba49d1a0 100644 --- a/src/shared/varlink.h +++ b/src/shared/varlink.h @@ -47,7 +47,8 @@ typedef enum VarlinkServerFlags { VARLINK_SERVER_MYSELF_ONLY = 1 << 1, /* Only accessible by our own UID */ VARLINK_SERVER_ACCOUNT_UID = 1 << 2, /* Do per user accounting */ VARLINK_SERVER_INHERIT_USERDATA = 1 << 3, /* Initialize Varlink connection userdata from VarlinkServer userdata */ - _VARLINK_SERVER_FLAGS_ALL = (1 << 4) - 1, + VARLINK_SERVER_INPUT_SENSITIVE = 1 << 4, /* Automatically mark al connection input as sensitive */ + _VARLINK_SERVER_FLAGS_ALL = (1 << 5) - 1, } VarlinkServerFlags; typedef int (*VarlinkMethod)(Varlink *link, JsonVariant *parameters, VarlinkMethodFlags flags, void *userdata); @@ -154,6 +155,9 @@ VarlinkServer* varlink_get_server(Varlink *v); int varlink_set_description(Varlink *v, const char *d); +/* Automatically mark the parameters part of incoming messages as security sensitive */ +int varlink_set_input_sensitive(Varlink *v); + /* Create a varlink server */ int varlink_server_new(VarlinkServer **ret, VarlinkServerFlags flags); VarlinkServer *varlink_server_ref(VarlinkServer *s); From c609338b1d7f5c627f93cc57ae2967cfb675c53c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 16 Jan 2024 11:54:20 +0100 Subject: [PATCH 2/4] json: export json_variant_is_sensitive_recursive() Let's export this function, so that we can use it elsewhere. Also, while at it, let's cache the result in a flag. This is only safe if the result is positive, since we allow the flag to be enabled at any time down thre tree somewhere, which we need to look at. (We never allow it to be turned off however) --- src/shared/json.c | 49 +++++++++++++++++++++++++++++------------------ src/shared/json.h | 1 + 2 files changed, 31 insertions(+), 19 deletions(-) diff --git a/src/shared/json.c b/src/shared/json.c index 0259196e4e2..270aef41051 100644 --- a/src/shared/json.c +++ b/src/shared/json.c @@ -89,6 +89,9 @@ struct JsonVariant { /* Erase from memory when freeing */ bool sensitive:1; + /* True if we know that any referenced json object is marked sensitive */ + bool recursive_sensitive:1; + /* If this is an object the fields are strictly ordered by name */ bool sorted:1; @@ -1453,6 +1456,33 @@ bool json_variant_is_sensitive(JsonVariant *v) { return v->sensitive; } +bool json_variant_is_sensitive_recursive(JsonVariant *v) { + if (!v) + return false; + if (json_variant_is_sensitive(v)) + return true; + if (!json_variant_is_regular(v)) + return false; + if (v->recursive_sensitive) /* Already checked this before */ + return true; + if (!IN_SET(v->type, JSON_VARIANT_ARRAY, JSON_VARIANT_OBJECT)) + return false; + if (v->is_reference) { + if (!json_variant_is_sensitive_recursive(v->reference)) + return false; + + return (v->recursive_sensitive = true); + } + + for (size_t i = 0; i < json_variant_elements(v); i++) + if (json_variant_is_sensitive_recursive(json_variant_by_index(v, i))) + return (v->recursive_sensitive = true); + + /* Note: we only cache the result here in case true, since we allow all elements down the tree to + * have their sensitive flag toggled later on (but never off) */ + return false; +} + static void json_variant_propagate_sensitive(JsonVariant *from, JsonVariant *to) { if (json_variant_is_sensitive(from)) json_variant_sensitive(to); @@ -1773,25 +1803,6 @@ static int json_format(FILE *f, JsonVariant *v, JsonFormatFlags flags, const cha return 0; } -static bool json_variant_is_sensitive_recursive(JsonVariant *v) { - if (!v) - return false; - if (json_variant_is_sensitive(v)) - return true; - if (!json_variant_is_regular(v)) - return false; - if (!IN_SET(v->type, JSON_VARIANT_ARRAY, JSON_VARIANT_OBJECT)) - return false; - if (v->is_reference) - return json_variant_is_sensitive_recursive(v->reference); - - for (size_t i = 0; i < json_variant_elements(v); i++) - if (json_variant_is_sensitive_recursive(json_variant_by_index(v, i))) - return true; - - return false; -} - int json_variant_format(JsonVariant *v, JsonFormatFlags flags, char **ret) { _cleanup_(memstream_done) MemStream m = {}; size_t sz; diff --git a/src/shared/json.h b/src/shared/json.h index 3c20f94b587..156f8e731e4 100644 --- a/src/shared/json.h +++ b/src/shared/json.h @@ -155,6 +155,7 @@ bool json_variant_equal(JsonVariant *a, JsonVariant *b); void json_variant_sensitive(JsonVariant *v); bool json_variant_is_sensitive(JsonVariant *v); +bool json_variant_is_sensitive_recursive(JsonVariant *v); struct json_variant_foreach_state { JsonVariant *variant; From 9912897170fb52c25a13b1dd5524f505e3d36cc6 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 16 Jan 2024 11:55:54 +0100 Subject: [PATCH 3/4] json: replace JSON_FORMAT_REFUSE_SENSITIVE with JSON_FORMAT_CENSOR_SENSITIVE Previously, the flag would completely refuse formatting a JSON object if any field of it was marked sensitive. With this change we'll simply replace the subobject with the string "", and show everything else. This is tremendously useful when debugging, since it means that we can again trace varlink calls through the stack: we can show all the message metadata and just suppress the actually sensitive parameters. The ability to debug this matters, and we should not hide more information that we can get away with, to keep things debuggable and maintainable. --- src/shared/json.c | 13 +++++++++---- src/shared/json.h | 2 +- src/shared/varlink.c | 42 +++++++++++++++++++++++------------------- src/test/test-json.c | 36 ++++++++++++++++++++---------------- 4 files changed, 53 insertions(+), 40 deletions(-) diff --git a/src/shared/json.c b/src/shared/json.c index 270aef41051..7ec6368e1ec 100644 --- a/src/shared/json.c +++ b/src/shared/json.c @@ -1612,6 +1612,15 @@ static int json_format(FILE *f, JsonVariant *v, JsonFormatFlags flags, const cha assert(f); assert(v); + if (FLAGS_SET(flags, JSON_FORMAT_CENSOR_SENSITIVE) && json_variant_is_sensitive(v)) { + if (flags & JSON_FORMAT_COLOR) + fputs(ansi_red(), f); + fputs("\"\"", f); + if (flags & JSON_FORMAT_COLOR) + fputs(ANSI_NORMAL, f); + return 0; + } + switch (json_variant_type(v)) { case JSON_VARIANT_REAL: { @@ -1818,10 +1827,6 @@ int json_variant_format(JsonVariant *v, JsonFormatFlags flags, char **ret) { if (flags & JSON_FORMAT_OFF) return -ENOEXEC; - if ((flags & JSON_FORMAT_REFUSE_SENSITIVE)) - if (json_variant_is_sensitive_recursive(v)) - return -EPERM; - f = memstream_init(&m); if (!f) return -ENOMEM; diff --git a/src/shared/json.h b/src/shared/json.h index 156f8e731e4..9c8448f728b 100644 --- a/src/shared/json.h +++ b/src/shared/json.h @@ -197,7 +197,7 @@ typedef enum JsonFormatFlags { JSON_FORMAT_FLUSH = 1 << 8, /* call fflush() after dumping JSON */ JSON_FORMAT_EMPTY_ARRAY = 1 << 9, /* output "[]" for empty input */ JSON_FORMAT_OFF = 1 << 10, /* make json_variant_format() fail with -ENOEXEC */ - JSON_FORMAT_REFUSE_SENSITIVE = 1 << 11, /* return EPERM if any node in the tree is marked as senstitive */ + JSON_FORMAT_CENSOR_SENSITIVE = 1 << 11, /* Replace all sensitive elements with the string "" */ } JsonFormatFlags; int json_variant_format(JsonVariant *v, JsonFormatFlags flags, char **ret); diff --git a/src/shared/varlink.c b/src/shared/varlink.c index 96dfd8de92a..7c37211cdda 100644 --- a/src/shared/varlink.c +++ b/src/shared/varlink.c @@ -1860,56 +1860,60 @@ Varlink* varlink_flush_close_unref(Varlink *v) { static int varlink_format_json(Varlink *v, JsonVariant *m) { _cleanup_(erase_and_freep) char *text = NULL; - bool sensitive = false; - int r; + int sz, r; assert(v); assert(m); - r = json_variant_format(m, JSON_FORMAT_REFUSE_SENSITIVE, &text); - if (r == -EPERM) { - sensitive = true; - r = json_variant_format(m, /* flags= */ 0, &text); - } - if (r < 0) - return r; - assert(text[r] == '\0'); + sz = json_variant_format(m, /* flags= */ 0, &text); + if (sz < 0) + return sz; + assert(text[sz] == '\0'); - if (v->output_buffer_size + r + 1 > VARLINK_BUFFER_MAX) + if (v->output_buffer_size + sz + 1 > VARLINK_BUFFER_MAX) return -ENOBUFS; - varlink_log(v, "Sending message: %s", sensitive ? "" : text); + if (DEBUG_LOGGING) { + _cleanup_(erase_and_freep) char *censored_text = NULL; + + /* Suppress sensitive fields in the debug output */ + r = json_variant_format(m, /* flags= */ JSON_FORMAT_CENSOR_SENSITIVE, &censored_text); + if (r < 0) + return r; + + varlink_log(v, "Sending message: %s", censored_text); + } if (v->output_buffer_size == 0) { free_and_replace(v->output_buffer, text); - v->output_buffer_size = r + 1; + v->output_buffer_size = sz + 1; v->output_buffer_index = 0; } else if (v->output_buffer_index == 0) { - if (!GREEDY_REALLOC(v->output_buffer, v->output_buffer_size + r + 1)) + if (!GREEDY_REALLOC(v->output_buffer, v->output_buffer_size + sz + 1)) return -ENOMEM; - memcpy(v->output_buffer + v->output_buffer_size, text, r + 1); - v->output_buffer_size += r + 1; + memcpy(v->output_buffer + v->output_buffer_size, text, sz + 1); + v->output_buffer_size += sz + 1; } else { char *n; - const size_t new_size = v->output_buffer_size + r + 1; + const size_t new_size = v->output_buffer_size + sz + 1; n = new(char, new_size); if (!n) return -ENOMEM; - memcpy(mempcpy(n, v->output_buffer + v->output_buffer_index, v->output_buffer_size), text, r + 1); + memcpy(mempcpy(n, v->output_buffer + v->output_buffer_index, v->output_buffer_size), text, sz + 1); free_and_replace(v->output_buffer, n); v->output_buffer_size = new_size; v->output_buffer_index = 0; } - if (sensitive) + if (json_variant_is_sensitive_recursive(m)) v->output_buffer_sensitive = true; /* Propagate sensitive flag */ else text = mfree(text); /* No point in the erase_and_free() destructor declared above */ diff --git a/src/test/test-json.c b/src/test/test-json.c index 333fbe6cf2a..cb0adc26437 100644 --- a/src/test/test-json.c +++ b/src/test/test-json.c @@ -107,9 +107,9 @@ static void test_variant_one(const char *data, Test test) { assert_se(json_variant_equal(v, w)); s = mfree(s); - r = json_variant_format(w, JSON_FORMAT_REFUSE_SENSITIVE, &s); - assert_se(r == -EPERM); - assert_se(!s); + r = json_variant_format(w, JSON_FORMAT_CENSOR_SENSITIVE, &s); + assert_se(s); + assert_se(streq_ptr(s, "\"\"")); s = mfree(s); r = json_variant_format(w, JSON_FORMAT_PRETTY, &s); @@ -886,10 +886,11 @@ TEST(json_sensitive) { json_variant_sensitive(a); - assert_se(json_variant_format(a, JSON_FORMAT_REFUSE_SENSITIVE, &s) == -EPERM); - assert_se(!s); + assert_se(json_variant_format(a, JSON_FORMAT_CENSOR_SENSITIVE, &s) >= 0); + assert_se(streq_ptr(s, "\"\"")); + s = mfree(s); - r = json_variant_format(b, JSON_FORMAT_REFUSE_SENSITIVE, &s); + r = json_variant_format(b, JSON_FORMAT_CENSOR_SENSITIVE, &s); assert_se(r >= 0); assert_se(s); assert_se((size_t) r == strlen(s)); @@ -901,7 +902,7 @@ TEST(json_sensitive) { JSON_BUILD_PAIR("e", JSON_BUILD_EMPTY_OBJECT))) >= 0); json_variant_dump(v, JSON_FORMAT_COLOR|JSON_FORMAT_PRETTY, NULL, NULL); - r = json_variant_format(v, JSON_FORMAT_REFUSE_SENSITIVE, &s); + r = json_variant_format(v, JSON_FORMAT_CENSOR_SENSITIVE, &s); assert_se(r >= 0); assert_se(s); assert_se((size_t) r == strlen(s)); @@ -915,7 +916,7 @@ TEST(json_sensitive) { JSON_BUILD_PAIR("e", JSON_BUILD_EMPTY_OBJECT))) >= 0); json_variant_dump(v, JSON_FORMAT_COLOR|JSON_FORMAT_PRETTY, NULL, NULL); - r = json_variant_format(v, JSON_FORMAT_REFUSE_SENSITIVE, &s); + r = json_variant_format(v, JSON_FORMAT_CENSOR_SENSITIVE, &s); assert_se(r >= 0); assert_se(s); assert_se((size_t) r == strlen(s)); @@ -930,8 +931,9 @@ TEST(json_sensitive) { JSON_BUILD_PAIR("e", JSON_BUILD_EMPTY_OBJECT))) >= 0); json_variant_dump(v, JSON_FORMAT_COLOR|JSON_FORMAT_PRETTY, NULL, NULL); - assert_se(json_variant_format(v, JSON_FORMAT_REFUSE_SENSITIVE, &s) == -EPERM); - assert_se(!s); + assert_se(json_variant_format(v, JSON_FORMAT_CENSOR_SENSITIVE, &s) >= 0); + assert_se(streq_ptr(s, "{\"b\":[\"foo\",\"bar\",\"baz\",\"qux\"],\"a\":\"\",\"c\":-9223372036854775808,\"d\":\"-9223372036854775808\",\"e\":{}}")); + s = mfree(s); v = json_variant_unref(v); assert_se(json_build(&v, JSON_BUILD_OBJECT( @@ -942,8 +944,9 @@ TEST(json_sensitive) { JSON_BUILD_PAIR("e", JSON_BUILD_EMPTY_OBJECT))) >= 0); json_variant_dump(v, JSON_FORMAT_COLOR|JSON_FORMAT_PRETTY, NULL, NULL); - assert_se(json_variant_format(v, JSON_FORMAT_REFUSE_SENSITIVE, &s) == -EPERM); - assert_se(!s); + assert_se(json_variant_format(v, JSON_FORMAT_CENSOR_SENSITIVE, &s) >= 0); + assert_se(streq_ptr(s, "{\"b\":[\"foo\",\"bar\",\"baz\",\"qux\"],\"c\":-9223372036854775808,\"a\":\"\",\"d\":\"-9223372036854775808\",\"e\":{}}")); + s = mfree(s); v = json_variant_unref(v); assert_se(json_build(&v, JSON_BUILD_OBJECT( @@ -954,8 +957,9 @@ TEST(json_sensitive) { JSON_BUILD_PAIR("e", JSON_BUILD_EMPTY_OBJECT))) >= 0); json_variant_dump(v, JSON_FORMAT_COLOR|JSON_FORMAT_PRETTY, NULL, NULL); - assert_se(json_variant_format(v, JSON_FORMAT_REFUSE_SENSITIVE, &s) == -EPERM); - assert_se(!s); + assert_se(json_variant_format(v, JSON_FORMAT_CENSOR_SENSITIVE, &s) >= 0); + assert_se(streq_ptr(s, "{\"b\":[\"foo\",\"bar\",\"baz\",\"qux\"],\"c\":-9223372036854775808,\"d\":\"-9223372036854775808\",\"a\":\"\",\"e\":{}}")); + s = mfree(s); v = json_variant_unref(v); assert_se(json_build(&v, JSON_BUILD_OBJECT( @@ -966,8 +970,8 @@ TEST(json_sensitive) { JSON_BUILD_PAIR_VARIANT("a", a))) >= 0); json_variant_dump(v, JSON_FORMAT_COLOR|JSON_FORMAT_PRETTY, NULL, NULL); - assert_se(json_variant_format(v, JSON_FORMAT_REFUSE_SENSITIVE, &s) == -EPERM); - assert_se(!s); + assert_se(json_variant_format(v, JSON_FORMAT_CENSOR_SENSITIVE, &s) >= 0); + assert_se(streq_ptr(s, "{\"b\":[\"foo\",\"bar\",\"baz\",\"qux\"],\"c\":-9223372036854775808,\"d\":\"-9223372036854775808\",\"e\":{},\"a\":\"\"}")); } TEST(json_iovec) { From 85978d296cea62d42bd00fc7cb109bdfd3d616ab Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 16 Jan 2024 12:06:17 +0100 Subject: [PATCH 4/4] varlink: restore debug output on incoming messages Now that we can selectively suppress only sensitive fields in JSON objects we can reenable debug logging for incoming messages, which was removed in 2e3414660cb0c6a024661638d0b237d88b5a7cbc. This makes Varlink fully debuggable again: we'll see both incoming and outgoing messages, only the sensitive fields are suppressed. See: #30578 --- src/shared/varlink.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/shared/varlink.c b/src/shared/varlink.c index 7c37211cdda..84497c1a06e 100644 --- a/src/shared/varlink.c +++ b/src/shared/varlink.c @@ -1065,6 +1065,17 @@ static int varlink_parse_message(Varlink *v) { json_variant_sensitive(parameters); } + if (DEBUG_LOGGING) { + _cleanup_(erase_and_freep) char *censored_text = NULL; + + /* Suppress sensitive fields in the debug output */ + r = json_variant_format(v->current, /* flags= */ JSON_FORMAT_CENSOR_SENSITIVE, &censored_text); + if (r < 0) + return r; + + varlink_log(v, "Received message: %s", censored_text); + } + v->input_buffer_size -= sz; if (v->input_buffer_size == 0)