From 2d17d699bb10803a46624569322900e9f7086406 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 9 Feb 2022 09:45:12 +0100 Subject: [PATCH 1/7] conf-parse: add generic config_parse_safe_string() helper This helper is just like config_parse_string() but does some superficial checks for control characters and quotes. In most cases we currently use config_parse_string() we probably want to use config_parse_safe_string() for safety reasons. --- src/shared/conf-parser.c | 27 +++++++++++++++++++++++++++ src/shared/conf-parser.h | 1 + 2 files changed, 28 insertions(+) diff --git a/src/shared/conf-parser.c b/src/shared/conf-parser.c index 64e67fb6f5..bc80eaffeb 100644 --- a/src/shared/conf-parser.c +++ b/src/shared/conf-parser.c @@ -873,6 +873,33 @@ int config_parse_string( return free_and_strdup_warn(s, empty_to_null(rvalue)); } +int config_parse_safe_string( + const char *unit, + const char *filename, + unsigned line, + const char *section, + unsigned section_line, + const char *lvalue, + int ltype, + const char *rvalue, + void *data, + void *userdata) { + + char **s = data; + + assert(filename); + assert(lvalue); + assert(rvalue); + assert(data); + + if (!string_is_safe(rvalue)) { + log_syntax(unit, LOG_WARNING, filename, line, 0, "Specified string contains unsafe characters, ignoring: %s", rvalue); + return 0; + } + + return free_and_strdup_warn(s, empty_to_null(rvalue)); +} + int config_parse_path( const char *unit, const char *filename, diff --git a/src/shared/conf-parser.h b/src/shared/conf-parser.h index 1e03f93bce..e93ea89b29 100644 --- a/src/shared/conf-parser.h +++ b/src/shared/conf-parser.h @@ -168,6 +168,7 @@ CONFIG_PARSER_PROTOTYPE(config_parse_bool); CONFIG_PARSER_PROTOTYPE(config_parse_id128); CONFIG_PARSER_PROTOTYPE(config_parse_tristate); CONFIG_PARSER_PROTOTYPE(config_parse_string); +CONFIG_PARSER_PROTOTYPE(config_parse_safe_string); CONFIG_PARSER_PROTOTYPE(config_parse_path); CONFIG_PARSER_PROTOTYPE(config_parse_strv); CONFIG_PARSER_PROTOTYPE(config_parse_sec); From deec0b6dd53486809e25c70716dc8e5e5d168915 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 9 Feb 2022 09:47:10 +0100 Subject: [PATCH 2/7] conf-parser: use _cleanup_free_ where appropriate --- src/shared/conf-parser.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/shared/conf-parser.c b/src/shared/conf-parser.c index bc80eaffeb..1de8eb9474 100644 --- a/src/shared/conf-parser.c +++ b/src/shared/conf-parser.c @@ -180,9 +180,9 @@ static int parse_line( if (!utf8_is_valid(l)) return log_syntax_invalid_utf8(unit, LOG_WARNING, filename, line, l); - if (*l == '[') { + if (l[0] == '[') { + _cleanup_free_ char *n = NULL; size_t k; - char *n; k = strlen(l); assert(k > 0); @@ -210,7 +210,6 @@ static int parse_line( if (!ignore) log_syntax(unit, LOG_WARNING, filename, line, 0, "Unknown section '%s'. Ignoring.", n); - free(n); *section = mfree(*section); *section_line = 0; *section_ignored = true; From 73a4ac8a75a3d0806dde14c66e5156a246648ecd Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 9 Feb 2022 09:47:50 +0100 Subject: [PATCH 3/7] conf-parser: minor refactorings/modernizations --- src/shared/conf-parser.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/shared/conf-parser.c b/src/shared/conf-parser.c index 1de8eb9474..ebbfa0d6c2 100644 --- a/src/shared/conf-parser.c +++ b/src/shared/conf-parser.c @@ -160,7 +160,7 @@ static int parse_line( char **section, unsigned *section_line, bool *section_ignored, - char *l, + char *l, /* is modified */ void *userdata) { char *e; @@ -171,10 +171,10 @@ static int parse_line( assert(l); l = strstrip(l); - if (!*l) + if (isempty(l)) return 0; - if (*l == '\n') + if (l[0] == '\n') return 0; if (!utf8_is_valid(l)) @@ -195,14 +195,14 @@ static int parse_line( return log_oom(); if (sections && !nulstr_contains(sections, n)) { - bool ignore = flags & CONFIG_PARSE_RELAXED; + bool ignore; const char *t; - ignore = ignore || startswith(n, "X-"); + ignore = (flags & CONFIG_PARSE_RELAXED) || startswith(n, "X-"); if (!ignore) NULSTR_FOREACH(t, sections) - if (streq_ptr(n, startswith(t, "-"))) { + if (streq_ptr(n, startswith(t, "-"))) { /* Ignore sections prefixed with "-" in valid section list */ ignore = true; break; } From cec7f09dc8beb21afcae981dbe3543686f793810 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 9 Feb 2022 09:47:53 +0100 Subject: [PATCH 4/7] conf-parser: refuse section headers with control characters early, just in case --- src/shared/conf-parser.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/shared/conf-parser.c b/src/shared/conf-parser.c index ebbfa0d6c2..dbf5cc2fcd 100644 --- a/src/shared/conf-parser.c +++ b/src/shared/conf-parser.c @@ -194,6 +194,9 @@ static int parse_line( if (!n) return log_oom(); + if (!string_is_safe(n)) + return log_syntax(unit, LOG_ERR, filename, line, SYNTHETIC_ERRNO(EBADMSG), "Bad characters in section header '%s'", l); + if (sections && !nulstr_contains(sections, n)) { bool ignore; const char *t; From 0b95409928c520e59b345eea14a571820eb8f7b3 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 9 Feb 2022 09:55:21 +0100 Subject: [PATCH 5/7] conf-parser: update config_item_*_lookup() to follow modern coding style Let's rename the return parameters ret_xyz, and always initialize them if we return >= 0, as per our current coding style. --- src/shared/conf-parser.c | 53 +++++++++++-------- src/shared/conf-parser.h | 10 ++-- .../xdg-autostart-service.c | 14 ++--- 3 files changed, 42 insertions(+), 35 deletions(-) diff --git a/src/shared/conf-parser.c b/src/shared/conf-parser.c index dbf5cc2fcd..ef2aeaf79d 100644 --- a/src/shared/conf-parser.c +++ b/src/shared/conf-parser.c @@ -40,18 +40,18 @@ int config_item_table_lookup( const void *table, const char *section, const char *lvalue, - ConfigParserCallback *func, - int *ltype, - void **data, + ConfigParserCallback *ret_func, + int *ret_ltype, + void **ret_data, void *userdata) { const ConfigTableItem *t; assert(table); assert(lvalue); - assert(func); - assert(ltype); - assert(data); + assert(ret_func); + assert(ret_ltype); + assert(ret_data); for (t = table; t->lvalue; t++) { @@ -61,12 +61,15 @@ int config_item_table_lookup( if (!streq_ptr(section, t->section)) continue; - *func = t->parse; - *ltype = t->ltype; - *data = t->data; + *ret_func = t->parse; + *ret_ltype = t->ltype; + *ret_data = t->data; return 1; } + *ret_func = NULL; + *ret_ltype = 0; + *ret_data = NULL; return 0; } @@ -74,9 +77,9 @@ int config_item_perf_lookup( const void *table, const char *section, const char *lvalue, - ConfigParserCallback *func, - int *ltype, - void **data, + ConfigParserCallback *ret_func, + int *ret_ltype, + void **ret_data, void *userdata) { ConfigPerfItemLookup lookup = (ConfigPerfItemLookup) table; @@ -84,9 +87,9 @@ int config_item_perf_lookup( assert(table); assert(lvalue); - assert(func); - assert(ltype); - assert(data); + assert(ret_func); + assert(ret_ltype); + assert(ret_data); if (section) { const char *key; @@ -95,12 +98,16 @@ int config_item_perf_lookup( p = lookup(key, strlen(key)); } else p = lookup(lvalue, strlen(lvalue)); - if (!p) + if (!p) { + *ret_func = NULL; + *ret_ltype = 0; + *ret_data = NULL; return 0; + } - *func = p->parse; - *ltype = p->ltype; - *data = (uint8_t*) userdata + p->offset; + *ret_func = p->parse; + *ret_ltype = p->ltype; + *ret_data = (uint8_t*) userdata + p->offset; return 1; } @@ -133,11 +140,11 @@ static int next_assignment( if (r < 0) return r; if (r > 0) { - if (func) - return func(unit, filename, line, section, section_line, - lvalue, ltype, rvalue, data, userdata); + if (!func) + return 0; - return 0; + return func(unit, filename, line, section, section_line, + lvalue, ltype, rvalue, data, userdata); } /* Warn about unknown non-extension fields. */ diff --git a/src/shared/conf-parser.h b/src/shared/conf-parser.h index e93ea89b29..fd1781774f 100644 --- a/src/shared/conf-parser.h +++ b/src/shared/conf-parser.h @@ -69,18 +69,18 @@ typedef int (*ConfigItemLookup)( const void *table, const char *section, const char *lvalue, - ConfigParserCallback *func, - int *ltype, - void **data, + ConfigParserCallback *ret_func, + int *ret_ltype, + void **ret_data, void *userdata); /* Linear table search implementation of ConfigItemLookup, based on * ConfigTableItem arrays */ -int config_item_table_lookup(const void *table, const char *section, const char *lvalue, ConfigParserCallback *func, int *ltype, void **data, void *userdata); +int config_item_table_lookup(const void *table, const char *section, const char *lvalue, ConfigParserCallback *ret_func, int *ret_ltype, void **ret_data, void *userdata); /* gperf implementation of ConfigItemLookup, based on gperf * ConfigPerfItem tables */ -int config_item_perf_lookup(const void *table, const char *section, const char *lvalue, ConfigParserCallback *func, int *ltype, void **data, void *userdata); +int config_item_perf_lookup(const void *table, const char *section, const char *lvalue, ConfigParserCallback *ret_func, int *ret_ltype, void **ret_data, void *userdata); int config_parse( const char *unit, diff --git a/src/xdg-autostart-generator/xdg-autostart-service.c b/src/xdg-autostart-generator/xdg-autostart-service.c index d450341b9f..0e1e84eda8 100644 --- a/src/xdg-autostart-generator/xdg-autostart-service.c +++ b/src/xdg-autostart-generator/xdg-autostart-service.c @@ -288,22 +288,22 @@ static int xdg_config_item_table_lookup( const void *table, const char *section, const char *lvalue, - ConfigParserCallback *func, - int *ltype, - void **data, + ConfigParserCallback *ret_func, + int *ret_ltype, + void **ret_data, void *userdata) { assert(lvalue); /* Ignore any keys with [] as those are translations. */ if (strchr(lvalue, '[')) { - *func = NULL; - *ltype = 0; - *data = NULL; + *ret_func = NULL; + *ret_ltype = 0; + *ret_data = NULL; return 1; } - return config_item_table_lookup(table, section, lvalue, func, ltype, data, userdata); + return config_item_table_lookup(table, section, lvalue, ret_func, ret_ltype, ret_data, userdata); } XdgAutostartService *xdg_autostart_service_parse_desktop(const char *path) { From a99a85242c66c3ff419bb8f1b8a4bfbbfd9e18e3 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 9 Feb 2022 10:07:17 +0100 Subject: [PATCH 6/7] tree-wide: use config_parse_safe_string() at various places --- src/core/load-fragment.c | 1 + src/journal-remote/journal-upload.c | 2 +- src/network/netdev/netdev-gperf.gperf | 8 ++++---- src/network/networkd-network-gperf.gperf | 4 ++-- src/nspawn/nspawn-gperf.gperf | 2 +- .../tty-ask-password-agent.c | 14 +++++++------- 6 files changed, 16 insertions(+), 15 deletions(-) diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 92a52819e2..50d1c86bb2 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -6173,6 +6173,7 @@ void unit_dump_config_items(FILE *f) { { config_parse_si_uint64, "SIZE" }, { config_parse_bool, "BOOLEAN" }, { config_parse_string, "STRING" }, + { config_parse_safe_string, "STRING" }, { config_parse_path, "PATH" }, { config_parse_unit_path_printf, "PATH" }, { config_parse_colon_separated_paths, "PATH" }, diff --git a/src/journal-remote/journal-upload.c b/src/journal-remote/journal-upload.c index fc52c546ed..9ddca7ac50 100644 --- a/src/journal-remote/journal-upload.c +++ b/src/journal-remote/journal-upload.c @@ -569,7 +569,7 @@ finalize: static int parse_config(void) { const ConfigTableItem items[] = { - { "Upload", "URL", config_parse_string, 0, &arg_url }, + { "Upload", "URL", config_parse_safe_string, 0, &arg_url }, { "Upload", "ServerKeyFile", config_parse_path_or_ignore, 0, &arg_key }, { "Upload", "ServerCertificateFile", config_parse_path_or_ignore, 0, &arg_cert }, { "Upload", "TrustedCertificateFile", config_parse_path_or_ignore, 0, &arg_trust }, diff --git a/src/network/netdev/netdev-gperf.gperf b/src/network/netdev/netdev-gperf.gperf index eadafbddb8..0b87e35087 100644 --- a/src/network/netdev/netdev-gperf.gperf +++ b/src/network/netdev/netdev-gperf.gperf @@ -178,14 +178,14 @@ Tun.OneQueue, config_parse_warn_compat, Tun.MultiQueue, config_parse_bool, 0, offsetof(TunTap, multi_queue) Tun.PacketInfo, config_parse_bool, 0, offsetof(TunTap, packet_info) Tun.VNetHeader, config_parse_bool, 0, offsetof(TunTap, vnet_hdr) -Tun.User, config_parse_string, 0, offsetof(TunTap, user_name) -Tun.Group, config_parse_string, 0, offsetof(TunTap, group_name) +Tun.User, config_parse_safe_string, 0, offsetof(TunTap, user_name) +Tun.Group, config_parse_safe_string, 0, offsetof(TunTap, group_name) Tap.OneQueue, config_parse_warn_compat, DISABLED_LEGACY, 0 Tap.MultiQueue, config_parse_bool, 0, offsetof(TunTap, multi_queue) Tap.PacketInfo, config_parse_bool, 0, offsetof(TunTap, packet_info) Tap.VNetHeader, config_parse_bool, 0, offsetof(TunTap, vnet_hdr) -Tap.User, config_parse_string, 0, offsetof(TunTap, user_name) -Tap.Group, config_parse_string, 0, offsetof(TunTap, group_name) +Tap.User, config_parse_safe_string, 0, offsetof(TunTap, user_name) +Tap.Group, config_parse_safe_string, 0, offsetof(TunTap, group_name) Bond.Mode, config_parse_bond_mode, 0, offsetof(Bond, mode) Bond.TransmitHashPolicy, config_parse_bond_xmit_hash_policy, 0, offsetof(Bond, xmit_hash_policy) Bond.LACPTransmitRate, config_parse_bond_lacp_rate, 0, offsetof(Bond, lacp_rate) diff --git a/src/network/networkd-network-gperf.gperf b/src/network/networkd-network-gperf.gperf index 8b19ce006b..c2ea2d681f 100644 --- a/src/network/networkd-network-gperf.gperf +++ b/src/network/networkd-network-gperf.gperf @@ -220,7 +220,7 @@ DHCPv4.SendHostname, config_parse_bool, DHCPv4.Hostname, config_parse_hostname, 0, offsetof(Network, dhcp_hostname) DHCPv4.Label, config_parse_dhcp_label, 0, offsetof(Network, dhcp_label) DHCPv4.RequestBroadcast, config_parse_tristate, 0, offsetof(Network, dhcp_broadcast) -DHCPv4.VendorClassIdentifier, config_parse_string, 0, offsetof(Network, dhcp_vendor_class_identifier) +DHCPv4.VendorClassIdentifier, config_parse_safe_string, 0, offsetof(Network, dhcp_vendor_class_identifier) DHCPv4.MUDURL, config_parse_mud_url, 0, offsetof(Network, dhcp_mudurl) DHCPv4.MaxAttempts, config_parse_dhcp_max_attempts, 0, 0 DHCPv4.UserClass, config_parse_dhcp_user_or_vendor_class, AF_INET, offsetof(Network, dhcp_user_class) @@ -539,7 +539,7 @@ DHCP.SendHostname, config_parse_bool, DHCP.Hostname, config_parse_hostname, 0, offsetof(Network, dhcp_hostname) DHCP.RequestBroadcast, config_parse_tristate, 0, offsetof(Network, dhcp_broadcast) DHCP.CriticalConnection, config_parse_tristate, 0, offsetof(Network, dhcp_critical) -DHCP.VendorClassIdentifier, config_parse_string, 0, offsetof(Network, dhcp_vendor_class_identifier) +DHCP.VendorClassIdentifier, config_parse_safe_string, 0, offsetof(Network, dhcp_vendor_class_identifier) DHCP.UserClass, config_parse_dhcp_user_or_vendor_class, AF_INET, offsetof(Network, dhcp_user_class) DHCP.IAID, config_parse_iaid, AF_INET, 0 DHCP.DUIDType, config_parse_network_duid_type, 0, 0 diff --git a/src/nspawn/nspawn-gperf.gperf b/src/nspawn/nspawn-gperf.gperf index d25bef7468..13f51569e8 100644 --- a/src/nspawn/nspawn-gperf.gperf +++ b/src/nspawn/nspawn-gperf.gperf @@ -24,7 +24,7 @@ Exec.Ephemeral, config_parse_tristate, 0, of Exec.ProcessTwo, config_parse_pid2, 0, 0 Exec.Parameters, config_parse_strv, 0, offsetof(Settings, parameters) Exec.Environment, config_parse_strv, 0, offsetof(Settings, environment) -Exec.User, config_parse_string, 0, offsetof(Settings, user) +Exec.User, config_parse_safe_string, 0, offsetof(Settings, user) Exec.Capability, config_parse_capability, 0, offsetof(Settings, capability) Exec.AmbientCapability, config_parse_capability, 0, offsetof(Settings, ambient_capability) Exec.DropCapability, config_parse_capability, 0, offsetof(Settings, drop_capability) diff --git a/src/tty-ask-password-agent/tty-ask-password-agent.c b/src/tty-ask-password-agent/tty-ask-password-agent.c index 54a03af082..c7105cde57 100644 --- a/src/tty-ask-password-agent/tty-ask-password-agent.c +++ b/src/tty-ask-password-agent/tty-ask-password-agent.c @@ -177,13 +177,13 @@ static int process_one_password_file(const char *filename) { unsigned pid = 0; const ConfigTableItem items[] = { - { "Ask", "Socket", config_parse_string, 0, &socket_name }, - { "Ask", "NotAfter", config_parse_uint64, 0, ¬_after }, - { "Ask", "Message", config_parse_string, 0, &message }, - { "Ask", "PID", config_parse_unsigned, 0, &pid }, - { "Ask", "AcceptCached", config_parse_bool, 0, &accept_cached }, - { "Ask", "Echo", config_parse_bool, 0, &echo }, - { "Ask", "Silent", config_parse_bool, 0, &silent }, + { "Ask", "Socket", config_parse_safe_string, 0, &socket_name }, + { "Ask", "NotAfter", config_parse_uint64, 0, ¬_after }, + { "Ask", "Message", config_parse_string, 0, &message }, + { "Ask", "PID", config_parse_unsigned, 0, &pid }, + { "Ask", "AcceptCached", config_parse_bool, 0, &accept_cached }, + { "Ask", "Echo", config_parse_bool, 0, &echo }, + { "Ask", "Silent", config_parse_bool, 0, &silent }, {} }; From 65a0ede2ed931a42f0906bb241ffe5634f510e91 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 9 Feb 2022 10:13:59 +0100 Subject: [PATCH 7/7] conf-parser: add specific parser for PID values --- src/shared/conf-parser.c | 1 + src/shared/conf-parser.h | 1 + src/tty-ask-password-agent/tty-ask-password-agent.c | 10 +++++----- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/shared/conf-parser.c b/src/shared/conf-parser.c index ef2aeaf79d..853f60f517 100644 --- a/src/shared/conf-parser.c +++ b/src/shared/conf-parser.c @@ -642,6 +642,7 @@ DEFINE_PARSER(nsec, nsec_t, parse_nsec); DEFINE_PARSER(sec, usec_t, parse_sec); DEFINE_PARSER(sec_def_infinity, usec_t, parse_sec_def_infinity); DEFINE_PARSER(mode, mode_t, parse_mode); +DEFINE_PARSER(pid, pid_t, parse_pid); int config_parse_iec_size( const char* unit, diff --git a/src/shared/conf-parser.h b/src/shared/conf-parser.h index fd1781774f..7badb4b12b 100644 --- a/src/shared/conf-parser.h +++ b/src/shared/conf-parser.h @@ -195,6 +195,7 @@ CONFIG_PARSER_PROTOTYPE(config_parse_ether_addrs); CONFIG_PARSER_PROTOTYPE(config_parse_in_addr_non_null); CONFIG_PARSER_PROTOTYPE(config_parse_percent); CONFIG_PARSER_PROTOTYPE(config_parse_permyriad); +CONFIG_PARSER_PROTOTYPE(config_parse_pid); typedef enum Disabled { DISABLED_CONFIGURATION, diff --git a/src/tty-ask-password-agent/tty-ask-password-agent.c b/src/tty-ask-password-agent/tty-ask-password-agent.c index c7105cde57..72559e11a9 100644 --- a/src/tty-ask-password-agent/tty-ask-password-agent.c +++ b/src/tty-ask-password-agent/tty-ask-password-agent.c @@ -174,13 +174,13 @@ static int process_one_password_file(const char *filename) { _cleanup_free_ char *socket_name = NULL, *message = NULL; bool accept_cached = false, echo = false, silent = false; uint64_t not_after = 0; - unsigned pid = 0; + pid_t pid = 0; const ConfigTableItem items[] = { { "Ask", "Socket", config_parse_safe_string, 0, &socket_name }, { "Ask", "NotAfter", config_parse_uint64, 0, ¬_after }, { "Ask", "Message", config_parse_string, 0, &message }, - { "Ask", "PID", config_parse_unsigned, 0, &pid }, + { "Ask", "PID", config_parse_pid, 0, &pid }, { "Ask", "AcceptCached", config_parse_bool, 0, &accept_cached }, { "Ask", "Echo", config_parse_bool, 0, &echo }, { "Ask", "Silent", config_parse_bool, 0, &silent }, @@ -212,14 +212,14 @@ static int process_one_password_file(const char *filename) { switch (arg_action) { case ACTION_LIST: - printf("'%s' (PID %u)\n", strna(message), pid); + printf("'%s' (PID " PID_FMT ")\n", strna(message), pid); return 0; case ACTION_WALL: { _cleanup_free_ char *wall = NULL; if (asprintf(&wall, - "Password entry required for \'%s\' (PID %u).\r\n" + "Password entry required for \'%s\' (PID " PID_FMT ").\r\n" "Please enter password with the systemd-tty-ask-password-agent tool.", strna(message), pid) < 0) @@ -235,7 +235,7 @@ static int process_one_password_file(const char *filename) { if (access(socket_name, W_OK) < 0) { if (arg_action == ACTION_QUERY) - log_info("Not querying '%s' (PID %u), lacking privileges.", strna(message), pid); + log_info("Not querying '%s' (PID " PID_FMT "), lacking privileges.", strna(message), pid); return 0; }