From: Lennart Poettering Date: Thu, 16 Apr 2026 07:03:24 +0000 (+0200) Subject: string-util: beef up string_is_safe() X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=812aa57d2cbcb037219552c48d4ec1a6754892aa;p=thirdparty%2Fsystemd.git string-util: beef up string_is_safe() This tightens the checks of string_is_safe() and then adds flags to relax certain aspects of it. This does alter the rules on certain strings we pass a bit. We mostly tighten the rules (but I think it's find and good) but we relax them on others. I let claude review the changes in behaviour for the various call sites that I made. It summarized things in this table: ╭───────────────────────────────────────────────────┬──────────────────────────────────────────────╮ │ CALL SITE │ EFFECTIVE DELTA │ ├───────────────────────────────────────────────────┼──────────────────────────────────────────────┤ │ src/basic/syslog-util log_namespace_name_valid │ +UTF-8 required (globs already blocked) │ │ src/bootctl --efi-boot-option-description │ RELAXED: '\' and quotes now permitted │ │ src/core/dbus-manager pretimeout governor │ +UTF-8, +no-globs │ │ src/core/load-fragment ExecStart= path │ +UTF-8, +no-globs │ │ src/core/main pretimeout governor (kcmdline) │ +UTF-8, +no-globs │ │ src/core/service sd_notify STATUS= │ +no-globs (ASCII-only preserved) │ │ src/home/homectl --= │ empty now REJECTED; +UTF-8 │ │ src/libsystemd-network dhcp_option_parse_string │ (equivalent, just explicit) │ │ src/libsystemd-network sd_dhcp_server boot_fname │ ""→NULL coerced; else equivalent │ │ src/libsystemd/journal SYSLOG_IDENTIFIER fb │ +UTF-8, +no-globs │ │ src/libsystemd/sd-json SD_JSON_STRICT strings │ +UTF-8 required │ │ src/login/logind session desktop= │ +UTF-8 required │ │ src/pcrlock EFI variable string │ +UTF-8 │ │ src/pcrlock EFI action string │ RELAXED: empty + '\' now ok; +UTF-8 │ │ src/resolve dns-delegate id (from filename) │ +UTF-8, +no-globs │ │ src/shared/boot-entry boot_entry_token_valid │ (equivalent) │ │ src/shared/conf-parser section header │ +UTF-8, +no-globs │ │ src/shared/conf-parser CONFIG_PARSE_STRING_SAFE │ +UTF-8 required │ │ src/shared/kbd-util keymap_is_valid │ (equivalent; folded into STRING_FILENAME) │ │ src/shared/tpm2 nvpcr name │ +UTF-8 required │ │ src/shared/vconsole x11 layout/model/variant/opt │ +UTF-8, +no-globs │ │ src/systemctl --kernel-cmdline= │ +0x7f DEL rejected; empty path split out │ │ src/veritysetup salt= │ RELAXED: safety check removed entirely │ │ src/vmspawn --ssh-key-type= │ +UTF-8 required │ ╰───────────────────────────────────────────────────┴──────────────────────────────────────────────╯ --- diff --git a/src/basic/string-util.c b/src/basic/string-util.c index a2bbaf1ea8f..4d930839232 100644 --- a/src/basic/string-util.c +++ b/src/basic/string-util.c @@ -1099,25 +1099,40 @@ int strdup_to_full(char **ret, const char *src) { } }; -bool string_is_safe(const char *p) { - if (!p) +bool string_is_safe(const char *p, StringSafeFlags flags) { + + /* Baseline checks are: + * â¢ No control characters (i.e. 0â¦31 + 127) + * â¢ UTF-8 valid (well, technically we skip this test if STRING_ASCII is set, since that is a tighter test) + */ + + if (FLAGS_SET(flags, STRING_ALLOW_EMPTY) ? !p : isempty(p)) return false; - /* Checks if the specified string contains no quotes or control characters */ + if (!FLAGS_SET(flags, STRING_ASCII) && !utf8_is_valid(p)) + return false; for (const char *t = p; *t; t++) { - if (*t > 0 && *t < ' ') /* no control characters */ + if ((*t > 0 && *t < ' ') || *t == 0x7f) /* never allow control characters */ + return false; + + if (!FLAGS_SET(flags, STRING_ALLOW_BACKSLASHES) && *t == '\\') return false; - if (strchr(QUOTES "\\\x7f", *t)) + if (!FLAGS_SET(flags, STRING_ALLOW_QUOTES) && strchr(QUOTES, *t)) + return false; + + if (!FLAGS_SET(flags, STRING_ALLOW_GLOBS) && strchr(GLOB_CHARS, *t)) + return false; + + if (FLAGS_SET(flags, STRING_ASCII) && (uint8_t) *t >= 0x80) return false; } - return true; -} + if (FLAGS_SET(flags, STRING_FILENAME) && !filename_is_valid(p)) + return false; -bool string_is_safe_ascii(const char *p) { - return ascii_is_valid(p) && string_is_safe(p); + return true; } char* str_realloc(char *p) { diff --git a/src/basic/string-util.h b/src/basic/string-util.h index 820b8688c22..17eaf5d9a6a 100644 --- a/src/basic/string-util.h +++ b/src/basic/string-util.h @@ -220,8 +220,16 @@ static inline int strdup_to(char **ret, const char *src) { return r < 0 ? r : 0; /* Suppress return value of 1. */ } -bool string_is_safe(const char *p) _pure_; -bool string_is_safe_ascii(const char *p) _pure_; +typedef enum StringSafeFlags { + STRING_ASCII = 1 << 0, /* Verify string is 7-Bit ASCII (rather than just UTF-8) */ + STRING_ALLOW_EMPTY = 1 << 1, /* Allow empty strings */ + STRING_ALLOW_BACKSLASHES = 1 << 2, /* Allow backslashes (\) */ + STRING_ALLOW_QUOTES = 1 << 3, /* Allow quotes (" or ') */ + STRING_ALLOW_GLOBS = 1 << 4, /* Allow globs (?, * or [) */ + STRING_FILENAME = 1 << 5, /* Verify the string is valid as regular filename */ +} StringSafeFlags; + +bool string_is_safe(const char *p, StringSafeFlags flags) _pure_; DISABLE_WARNING_STRINGOP_TRUNCATION; static inline void strncpy_exact(char *buf, const char *src, size_t buf_len) { diff --git a/src/basic/syslog-util.c b/src/basic/syslog-util.c index fd910ca76d4..bd1bb652823 100644 --- a/src/basic/syslog-util.c +++ b/src/basic/syslog-util.c @@ -4,9 +4,7 @@ #include "sd-id128.h" -#include "glob-util.h" #include "hexdecoct.h" -#include "path-util.h" #include "string-table.h" #include "string-util.h" #include "syslog-util.h" @@ -111,7 +109,8 @@ bool log_namespace_name_valid(const char *s) { * (so that /var/log/journal/. can be created based on it). Also make sure it * is suitable as unit instance name, and does not contain fishy characters. */ - if (!filename_is_valid(s)) + /* Let's avoid globbing for now */ + if (!string_is_safe(s, STRING_FILENAME)) return false; if (strlen(s) > LOG_NAMESPACE_MAX) @@ -120,12 +119,5 @@ bool log_namespace_name_valid(const char *s) { if (!unit_instance_is_valid(s)) return false; - if (!string_is_safe(s)) - return false; - - /* Let's avoid globbing for now */ - if (string_is_glob(s)) - return false; - return true; } diff --git a/src/bootctl/bootctl.c b/src/bootctl/bootctl.c index 308ff6a106d..00ae512668e 100644 --- a/src/bootctl/bootctl.c +++ b/src/bootctl/bootctl.c @@ -39,7 +39,6 @@ #include "string-table.h" #include "string-util.h" #include "strv.h" -#include "utf8.h" #include "varlink-io.systemd.BootControl.h" #include "varlink-util.h" #include "verbs.h" @@ -573,7 +572,7 @@ static int parse_argv(int argc, char *argv[], char ***ret_args) { OPTION_LONG("efi-boot-option-description", "DESCRIPTION", "Description of the entry in the boot option list"): - if (isempty(arg) || !(string_is_safe(arg) && utf8_is_valid(arg))) { + if (!string_is_safe(arg, STRING_ALLOW_BACKSLASHES|STRING_ALLOW_QUOTES|STRING_ALLOW_GLOBS)) { _cleanup_free_ char *escaped = cescape(arg); return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Invalid --efi-boot-option-description=: %s", strna(escaped)); diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c index 5a7f70d78bf..1bc73e7b434 100644 --- a/src/core/dbus-manager.c +++ b/src/core/dbus-manager.c @@ -387,13 +387,16 @@ static int property_set_pretimeout_watchdog_governor( sd_bus_error *reterr_error) { Manager *m = ASSERT_PTR(userdata); - char *governor; + const char *governor; int r; r = sd_bus_message_read(value, "s", &governor); if (r < 0) return r; - if (!string_is_safe(governor)) + + if (isempty(governor)) + governor = NULL; + else if (!string_is_safe(governor, /* flags= */ 0)) return -EINVAL; return manager_override_watchdog_pretimeout_governor(m, governor); diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 52005c8c436..1fae521333f 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -990,7 +990,7 @@ int config_parse_exec( ignore ? ", ignoring" : "", rvalue); return ignore ? 0 : -ENOEXEC; } - if (!string_is_safe(path)) { + if (!string_is_safe(path, /* flags= */ 0)) { log_syntax(unit, ignore ? LOG_WARNING : LOG_ERR, filename, line, 0, "Executable path contains special characters%s: %s", ignore ? ", ignoring" : "", path); diff --git a/src/core/main.c b/src/core/main.c index b4022105e88..e89cb9da3d1 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -502,7 +502,7 @@ static int parse_proc_cmdline_item(const char *key, const char *value, void *dat return 0; } - if (!string_is_safe(value)) { + if (!string_is_safe(value, /* flags= */ 0)) { log_warning("Watchdog pretimeout governor '%s' is not valid, ignoring.", value); return 0; } diff --git a/src/core/manager.c b/src/core/manager.c index 01841a97d6d..56ee66e5bd2 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -3576,12 +3576,14 @@ int manager_set_watchdog_pretimeout_governor(Manager *m, const char *governor) { if (MANAGER_IS_USER(m)) return 0; + governor = empty_to_null(governor); + if (streq_ptr(m->watchdog_pretimeout_governor, governor)) return 0; - p = strdup(governor); - if (!p) - return -ENOMEM; + r = strdup_to(&p, governor); + if (r < 0) + return r; r = watchdog_setup_pretimeout_governor(governor); if (r < 0) @@ -3599,12 +3601,14 @@ int manager_override_watchdog_pretimeout_governor(Manager *m, const char *govern if (MANAGER_IS_USER(m)) return 0; + governor = empty_to_null(governor); + if (streq_ptr(m->watchdog_pretimeout_governor_overridden, governor)) return 0; - p = strdup(governor); - if (!p) - return -ENOMEM; + r = strdup_to(&p, governor); + if (r < 0) + return r; r = watchdog_setup_pretimeout_governor(governor); if (r < 0) diff --git a/src/core/service.c b/src/core/service.c index 63e65994218..b2a656a6fe8 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -5210,7 +5210,7 @@ static void service_notify_message( e = empty_to_null(e); - if (e && !string_is_safe_ascii(e)) { + if (e && !string_is_safe(e, STRING_ASCII)) { _cleanup_free_ char *escaped = cescape(e); log_unit_warning(u, "Got invalid %s string, ignoring: %s", i->tag, strna(escaped)); } else if (free_and_strdup_warn(status_error, e) > 0) diff --git a/src/home/homectl.c b/src/home/homectl.c index 194e73b4917..4ebf47ca9e7 100644 --- a/src/home/homectl.c +++ b/src/home/homectl.c @@ -4666,7 +4666,7 @@ static int parse_argv(int argc, char *argv[]) { IN_SET(c, ARG_STORAGE, ARG_FS_TYPE) ? &arg_identity_extra_this_machine : &arg_identity_extra; - if (!isempty(optarg) && !string_is_safe(optarg)) + if (!string_is_safe(optarg, STRING_ALLOW_GLOBS)) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Parameter for field %s not valid: %s", field, optarg); diff --git a/src/libsystemd-network/dhcp-option.c b/src/libsystemd-network/dhcp-option.c index c78e3cdad9d..a195a4b0983 100644 --- a/src/libsystemd-network/dhcp-option.c +++ b/src/libsystemd-network/dhcp-option.c @@ -427,7 +427,7 @@ int dhcp_option_parse_string(const uint8_t *option, size_t len, char **ret) { if (r < 0) return r; - if (!string_is_safe(string) || !utf8_is_valid(string)) + if (!string_is_safe(string, STRING_ALLOW_EMPTY|STRING_ALLOW_GLOBS)) return -EINVAL; *ret = TAKE_PTR(string); diff --git a/src/libsystemd-network/sd-dhcp-server.c b/src/libsystemd-network/sd-dhcp-server.c index 34ed3e10d33..fa0b8301969 100644 --- a/src/libsystemd-network/sd-dhcp-server.c +++ b/src/libsystemd-network/sd-dhcp-server.c @@ -271,7 +271,9 @@ int sd_dhcp_server_set_boot_server_name(sd_dhcp_server *server, const char *name int sd_dhcp_server_set_boot_filename(sd_dhcp_server *server, const char *filename) { assert_return(server, -EINVAL); - if (filename && !string_is_safe_ascii(filename)) + if (isempty(filename)) + filename = NULL; + else if (!string_is_safe(filename, STRING_ASCII|STRING_ALLOW_GLOBS)) return -EINVAL; return free_and_strdup(&server->boot_filename, filename); diff --git a/src/libsystemd/sd-journal/journal-send.c b/src/libsystemd/sd-journal/journal-send.c index 5c7b007b131..931e669a089 100644 --- a/src/libsystemd/sd-journal/journal-send.c +++ b/src/libsystemd/sd-journal/journal-send.c @@ -273,13 +273,11 @@ _public_ int sd_journal_sendv(const struct iovec *iov, int n) { } if (!have_syslog_identifier && - string_is_safe(program_invocation_short_name)) { + string_is_safe(program_invocation_short_name, /* flags= */ 0)) { - /* Implicitly add program_invocation_short_name, if it - * is not set explicitly. We only do this for - * program_invocation_short_name, and nothing else - * since everything else is much nicer to retrieve - * from the outside. */ + /* Implicitly add program_invocation_short_name, if it is not set explicitly. We only do this + * for program_invocation_short_name, and nothing else since everything else is much nicer to + * retrieve from the outside. */ w[j++] = IOVEC_MAKE_STRING("SYSLOG_IDENTIFIER="); w[j++] = IOVEC_MAKE_STRING(program_invocation_short_name); diff --git a/src/libsystemd/sd-json/sd-json.c b/src/libsystemd/sd-json/sd-json.c index 4c39b5660a6..4c541275c42 100644 --- a/src/libsystemd/sd-json/sd-json.c +++ b/src/libsystemd/sd-json/sd-json.c @@ -5652,7 +5652,7 @@ _public_ int sd_json_dispatch_const_string(const char *name, sd_json_variant *va if (!sd_json_variant_is_string(variant)) return json_log(variant, flags, SYNTHETIC_ERRNO(EINVAL), "JSON field '%s' is not a string.", strna(name)); - if ((flags & SD_JSON_STRICT) && !string_is_safe(sd_json_variant_string(variant))) + if ((flags & SD_JSON_STRICT) && !string_is_safe(sd_json_variant_string(variant), STRING_ALLOW_EMPTY|STRING_ALLOW_GLOBS)) return json_log(variant, flags, SYNTHETIC_ERRNO(EINVAL), "JSON field '%s' contains unsafe characters, refusing.", strna(name)); *s = sd_json_variant_string(variant); @@ -5675,7 +5675,7 @@ _public_ int sd_json_dispatch_strv(const char *name, sd_json_variant *variant, s /* Let's be flexible here: accept a single string in place of a single-item array */ if (sd_json_variant_is_string(variant)) { - if ((flags & SD_JSON_STRICT) && !string_is_safe(sd_json_variant_string(variant))) + if ((flags & SD_JSON_STRICT) && !string_is_safe(sd_json_variant_string(variant), STRING_ALLOW_EMPTY|STRING_ALLOW_GLOBS)) return json_log(variant, flags, SYNTHETIC_ERRNO(EINVAL), "JSON field '%s' contains unsafe characters, refusing.", strna(name)); l = strv_new(sd_json_variant_string(variant)); @@ -5693,7 +5693,7 @@ _public_ int sd_json_dispatch_strv(const char *name, sd_json_variant *variant, s if (!sd_json_variant_is_string(e)) return json_log(e, flags, SYNTHETIC_ERRNO(EINVAL), "JSON array element is not a string."); - if ((flags & SD_JSON_STRICT) && !string_is_safe(sd_json_variant_string(e))) + if ((flags & SD_JSON_STRICT) && !string_is_safe(sd_json_variant_string(e), STRING_ALLOW_EMPTY|STRING_ALLOW_GLOBS)) return json_log(e, flags, SYNTHETIC_ERRNO(EINVAL), "JSON field '%s' contains unsafe characters, refusing.", strna(name)); r = strv_extend(&l, sd_json_variant_string(e)); diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index 9668bf06098..14208bc1496 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -1155,7 +1155,7 @@ static int manager_create_session_by_bus( if (isempty(desktop)) desktop = NULL; else { - if (!string_is_safe(desktop)) + if (!string_is_safe(desktop, STRING_ALLOW_GLOBS)) return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Invalid desktop string %s", desktop); } diff --git a/src/pcrlock/pcrlock.c b/src/pcrlock/pcrlock.c index a6ec5c23d03..fd8b9e95a4f 100644 --- a/src/pcrlock/pcrlock.c +++ b/src/pcrlock/pcrlock.c @@ -543,7 +543,7 @@ static int event_log_record_parse_variable_data( if (!p) return log_oom_debug(); - if (!string_is_safe(p)) + if (!string_is_safe(p, STRING_ALLOW_GLOBS)) return log_debug_errno(SYNTHETIC_ERRNO(EBADMSG), "Unsafe EFI variable string in record."); *ret_variable_uuid = efi_guid_to_id128(vdata->variableName); @@ -627,7 +627,7 @@ static int event_log_record_extract_firmware_description(EventLogRecord *rec) { if (r < 0) return log_error_errno(r, "Failed to make C string from EFI action string: %m"); - if (!string_is_safe(d)) { + if (!string_is_safe(d, STRING_ALLOW_GLOBS|STRING_ALLOW_EMPTY|STRING_ALLOW_BACKSLASHES)) { log_warning("Unsafe EFI action string in record, ignoring."); goto invalid; } diff --git a/src/resolve/resolved-dns-delegate.c b/src/resolve/resolved-dns-delegate.c index eee2daab94b..db34916a297 100644 --- a/src/resolve/resolved-dns-delegate.c +++ b/src/resolve/resolved-dns-delegate.c @@ -172,8 +172,8 @@ static int dns_delegate_load(Manager *m, const char *path) { return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "DNS delegate file name does not end in .dns-delegate, refusing: %s", fn); _cleanup_free_ char *id = strndup(fn, e - fn); - if (!string_is_safe(id)) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "DNS delegate file name contains weird characters, refusing: %s", fn); + if (!string_is_safe(id, /* flags= */ 0)) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "DNS delegate file name is invalid, refusing: %s", fn); _cleanup_free_ char *dropin_dirname = strjoin(id, ".dns-delegate.d"); if (!dropin_dirname) diff --git a/src/shared/boot-entry.c b/src/shared/boot-entry.c index c9e966ba04e..b0dac0d7825 100644 --- a/src/shared/boot-entry.c +++ b/src/shared/boot-entry.c @@ -11,10 +11,9 @@ #include "string-table.h" #include "string-util.h" #include "strv.h" -#include "utf8.h" bool boot_entry_token_valid(const char *p) { - return utf8_is_valid(p) && string_is_safe(p) && filename_is_valid(p); + return string_is_safe(p, STRING_FILENAME); } static int entry_token_load_one(int rfd, const char *dir, BootEntryTokenType *type, char **token) { diff --git a/src/shared/conf-parser.c b/src/shared/conf-parser.c index b448032939d..feaed66cbe2 100644 --- a/src/shared/conf-parser.c +++ b/src/shared/conf-parser.c @@ -217,8 +217,8 @@ 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 (!string_is_safe(n, /* flags= */ 0)) + return log_syntax(unit, LOG_ERR, filename, line, SYNTHETIC_ERRNO(EBADMSG), "Section header invalid '%s'", l); if (sections && !nulstr_contains(sections, n)) { bool ignore; @@ -1244,7 +1244,7 @@ int config_parse_string( return 1; } - if (FLAGS_SET(ltype, CONFIG_PARSE_STRING_SAFE) && !string_is_safe(rvalue)) { + if (FLAGS_SET(ltype, CONFIG_PARSE_STRING_SAFE) && !string_is_safe(rvalue, STRING_ALLOW_GLOBS)) { _cleanup_free_ char *escaped = NULL; escaped = cescape(rvalue); diff --git a/src/shared/kbd-util.c b/src/shared/kbd-util.c index 84031df7843..28ea2e8612e 100644 --- a/src/shared/kbd-util.c +++ b/src/shared/kbd-util.c @@ -5,12 +5,10 @@ #include "errno-util.h" #include "kbd-util.h" #include "log.h" -#include "path-util.h" #include "recurse-dir.h" #include "set.h" #include "string-util.h" #include "strv.h" -#include "utf8.h" #define KBD_KEYMAP_DIRS \ "/usr/share/keymaps/", \ @@ -129,21 +127,12 @@ int get_keymaps(char ***ret) { } bool keymap_is_valid(const char *name) { - if (isempty(name)) + if (!string_is_safe(name, STRING_FILENAME)) return false; if (strlen(name) >= 128) return false; - if (!utf8_is_valid(name)) - return false; - - if (!filename_is_valid(name)) - return false; - - if (!string_is_safe(name)) - return false; - return true; } diff --git a/src/shared/tpm2-util.c b/src/shared/tpm2-util.c index 02f89c02ba7..fa0fa71130b 100644 --- a/src/shared/tpm2-util.c +++ b/src/shared/tpm2-util.c @@ -9489,7 +9489,6 @@ DEFINE_STRING_TABLE_LOOKUP_FROM_STRING_WITH_FALLBACK(tpm2_pcr_index, int, TPM2_P DEFINE_STRING_TABLE_LOOKUP_TO_STRING(tpm2_pcr_index, int); bool tpm2_nvpcr_name_is_valid(const char *name) { - return filename_is_valid(name) && - string_is_safe(name) && + return string_is_safe(name, STRING_FILENAME) && tpm2_pcr_index_from_string(name) < 0; /* don't allow nvpcrs to be name like pcrs */ } diff --git a/src/shared/vconsole-util.c b/src/shared/vconsole-util.c index cd623bebdbb..6e8c17561e8 100644 --- a/src/shared/vconsole-util.c +++ b/src/shared/vconsole-util.c @@ -87,10 +87,10 @@ bool x11_context_is_safe(const X11Context *xc) { assert(xc); return - (!xc->layout || string_is_safe(xc->layout)) && - (!xc->model || string_is_safe(xc->model)) && - (!xc->variant || string_is_safe(xc->variant)) && - (!xc->options || string_is_safe(xc->options)); + (!xc->layout || string_is_safe(xc->layout, /* flags= */ 0)) && + (!xc->model || string_is_safe(xc->model, /* flags= */ 0)) && + (!xc->variant || string_is_safe(xc->variant, /* flags= */ 0)) && + (!xc->options || string_is_safe(xc->options, /* flags= */ 0)); } bool x11_context_equal(const X11Context *a, const X11Context *b) { diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index 5c9a24b119a..e5e7b412f56 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -27,7 +27,6 @@ #include "systemctl-compat-shutdown.h" #include "systemctl-logind.h" #include "time-util.h" -#include "utf8.h" char **arg_types = NULL; char **arg_states = NULL; @@ -969,16 +968,18 @@ static int systemctl_parse_argv(int argc, char *argv[]) { break; case ARG_KERNEL_CMDLINE: - if (!utf8_is_valid(optarg)) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "--kernel-cmdline= argument is not valid UTF-8: %s", optarg); - if (string_has_cc(optarg, NULL)) + if (isempty(optarg)) { + arg_kernel_cmdline = mfree(arg_kernel_cmdline); + break; + } + + if (!string_is_safe(optarg, STRING_ALLOW_GLOBS|STRING_ALLOW_BACKSLASHES|STRING_ALLOW_QUOTES)) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "--kernel-cmdline= argument contains control characters: %s", optarg); + "--kernel-cmdline= argument contains invalid characters: %s", optarg); r = free_and_strdup_warn(&arg_kernel_cmdline, optarg); if (r < 0) - return r; + return r; break; case ARG_TIMESTAMP_STYLE: diff --git a/src/test/test-string-util.c b/src/test/test-string-util.c index 5707569e7e1..6e07d727636 100644 --- a/src/test/test-string-util.c +++ b/src/test/test-string-util.c @@ -1495,4 +1495,146 @@ TEST(str_common_prefix) { ASSERT_EQ(str_common_prefix("systemd-networkd", ""), 0U); } +TEST(string_is_safe) { + /* NULL is always rejected, regardless of flags. */ + ASSERT_FALSE(string_is_safe(NULL, 0)); + ASSERT_FALSE(string_is_safe(NULL, STRING_ALLOW_EMPTY)); + ASSERT_FALSE(string_is_safe(NULL, STRING_ASCII)); + ASSERT_FALSE(string_is_safe(NULL, STRING_ALLOW_BACKSLASHES)); + ASSERT_FALSE(string_is_safe(NULL, STRING_ALLOW_QUOTES)); + ASSERT_FALSE(string_is_safe(NULL, STRING_ALLOW_GLOBS)); + ASSERT_FALSE(string_is_safe(NULL, STRING_FILENAME)); + + /* Baseline (flags=0): rejects empty, backslashes, quotes, globs, control chars and invalid UTF-8. + * Plain alphanumerics/whitespace and valid UTF-8 accepted. */ + ASSERT_TRUE(string_is_safe("hello", 0)); + ASSERT_TRUE(string_is_safe("hello world", 0)); + ASSERT_TRUE(string_is_safe("Ã¼ber", 0)); /* valid UTF-8 allowed */ + ASSERT_TRUE(string_is_safe("Ã¼nÃ¯cÃ¶dÃ©", 0)); + + ASSERT_FALSE(string_is_safe("", 0)); /* empty rejected by default */ + ASSERT_FALSE(string_is_safe("a\\b", 0)); /* backslash rejected by default */ + ASSERT_FALSE(string_is_safe("\"", 0)); /* double quote rejected by default */ + ASSERT_FALSE(string_is_safe("'", 0)); /* single quote rejected by default */ + ASSERT_FALSE(string_is_safe("*", 0)); /* glob rejected by default */ + ASSERT_FALSE(string_is_safe("?", 0)); /* glob rejected by default */ + ASSERT_FALSE(string_is_safe("[", 0)); /* glob rejected by default */ + ASSERT_FALSE(string_is_safe("abc\x01", 0)); /* control char */ + ASSERT_FALSE(string_is_safe("\t", 0)); + ASSERT_FALSE(string_is_safe("\n", 0)); + ASSERT_FALSE(string_is_safe("abc\x1f", 0)); + ASSERT_FALSE(string_is_safe("abc\x7f", 0)); /* DEL */ + ASSERT_FALSE(string_is_safe("ab\xc3\x28", 0)); /* invalid UTF-8 continuation */ + ASSERT_FALSE(string_is_safe("\xff", 0)); /* not valid UTF-8 */ + + /* STRING_ALLOW_EMPTY. */ + ASSERT_TRUE(string_is_safe("", STRING_ALLOW_EMPTY)); + ASSERT_TRUE(string_is_safe("x", STRING_ALLOW_EMPTY)); + ASSERT_TRUE(string_is_safe("hello", STRING_ALLOW_EMPTY)); + ASSERT_FALSE(string_is_safe(NULL, STRING_ALLOW_EMPTY)); + + /* STRING_ASCII: high bytes rejected, low ASCII accepted, control chars still rejected. + * Empty is still rejected by default; backslashes/quotes/globs still rejected by default. */ + ASSERT_TRUE(string_is_safe("hello", STRING_ASCII)); + ASSERT_TRUE(string_is_safe("hello world 123!@#$%^&()", STRING_ASCII)); + ASSERT_FALSE(string_is_safe("", STRING_ASCII)); + ASSERT_FALSE(string_is_safe("Ã¼ber", STRING_ASCII)); + ASSERT_FALSE(string_is_safe("\x80", STRING_ASCII)); + ASSERT_FALSE(string_is_safe("\xff", STRING_ASCII)); + ASSERT_FALSE(string_is_safe("abc\x01", STRING_ASCII)); + ASSERT_FALSE(string_is_safe("abc\x7f", STRING_ASCII)); + ASSERT_FALSE(string_is_safe("a\\b", STRING_ASCII)); + ASSERT_FALSE(string_is_safe("a\"b", STRING_ASCII)); + ASSERT_FALSE(string_is_safe("a*b", STRING_ASCII)); + + /* STRING_ALLOW_BACKSLASHES: backslashes allowed, quotes/globs still rejected. */ + ASSERT_TRUE(string_is_safe("hello", STRING_ALLOW_BACKSLASHES)); + ASSERT_TRUE(string_is_safe("hello world", STRING_ALLOW_BACKSLASHES)); + ASSERT_TRUE(string_is_safe("\\", STRING_ALLOW_BACKSLASHES)); + ASSERT_TRUE(string_is_safe("a\\b", STRING_ALLOW_BACKSLASHES)); + ASSERT_TRUE(string_is_safe("foo\\", STRING_ALLOW_BACKSLASHES)); + ASSERT_TRUE(string_is_safe("\\foo", STRING_ALLOW_BACKSLASHES)); + ASSERT_TRUE(string_is_safe("foo\\nbar", STRING_ALLOW_BACKSLASHES)); /* literal backslash, not newline */ + ASSERT_FALSE(string_is_safe("\"", STRING_ALLOW_BACKSLASHES)); /* quotes still rejected */ + ASSERT_FALSE(string_is_safe("*", STRING_ALLOW_BACKSLASHES)); /* globs still rejected */ + + /* STRING_ALLOW_QUOTES: quotes allowed, backslashes/globs still rejected. */ + ASSERT_TRUE(string_is_safe("hello", STRING_ALLOW_QUOTES)); + ASSERT_TRUE(string_is_safe("hello world", STRING_ALLOW_QUOTES)); + ASSERT_TRUE(string_is_safe("\"", STRING_ALLOW_QUOTES)); + ASSERT_TRUE(string_is_safe("'", STRING_ALLOW_QUOTES)); + ASSERT_TRUE(string_is_safe("hello\"world", STRING_ALLOW_QUOTES)); + ASSERT_TRUE(string_is_safe("it's", STRING_ALLOW_QUOTES)); + ASSERT_FALSE(string_is_safe("a\\b", STRING_ALLOW_QUOTES)); /* backslashes still rejected */ + ASSERT_FALSE(string_is_safe("*", STRING_ALLOW_QUOTES)); /* globs still rejected */ + + /* STRING_ALLOW_GLOBS: globs allowed, backslashes/quotes still rejected. */ + ASSERT_TRUE(string_is_safe("hello", STRING_ALLOW_GLOBS)); + ASSERT_TRUE(string_is_safe("ab]c", STRING_ALLOW_GLOBS)); /* ']' is not in GLOB_CHARS anyway */ + ASSERT_TRUE(string_is_safe("*", STRING_ALLOW_GLOBS)); + ASSERT_TRUE(string_is_safe("?", STRING_ALLOW_GLOBS)); + ASSERT_TRUE(string_is_safe("[", STRING_ALLOW_GLOBS)); + ASSERT_TRUE(string_is_safe("foo*bar", STRING_ALLOW_GLOBS)); + ASSERT_TRUE(string_is_safe("foo?bar", STRING_ALLOW_GLOBS)); + ASSERT_TRUE(string_is_safe("foo[bar", STRING_ALLOW_GLOBS)); + ASSERT_FALSE(string_is_safe("\"", STRING_ALLOW_GLOBS)); /* quotes still rejected */ + ASSERT_FALSE(string_is_safe("a\\b", STRING_ALLOW_GLOBS)); /* backslashes still rejected */ + + /* STRING_FILENAME: rejects empty, ".", "..", and strings with '/'. */ + ASSERT_TRUE(string_is_safe("hello", STRING_FILENAME)); + ASSERT_TRUE(string_is_safe("hello.txt", STRING_FILENAME)); + ASSERT_TRUE(string_is_safe("...", STRING_FILENAME)); + ASSERT_TRUE(string_is_safe(".hidden", STRING_FILENAME)); + ASSERT_FALSE(string_is_safe("", STRING_FILENAME)); + ASSERT_FALSE(string_is_safe(".", STRING_FILENAME)); + ASSERT_FALSE(string_is_safe("..", STRING_FILENAME)); + ASSERT_FALSE(string_is_safe("/", STRING_FILENAME)); + ASSERT_FALSE(string_is_safe("/foo", STRING_FILENAME)); + ASSERT_FALSE(string_is_safe("foo/bar", STRING_FILENAME)); + + /* Pairwise combinations. */ + ASSERT_TRUE(string_is_safe("", STRING_ALLOW_EMPTY | STRING_ASCII)); + ASSERT_FALSE(string_is_safe("Ã¼ber", STRING_ALLOW_EMPTY | STRING_ASCII)); + ASSERT_TRUE(string_is_safe("hello", STRING_ALLOW_EMPTY | STRING_ASCII)); + + ASSERT_TRUE(string_is_safe("ab\"cd", STRING_ALLOW_QUOTES | STRING_ALLOW_GLOBS)); + ASSERT_TRUE(string_is_safe("ab*cd", STRING_ALLOW_QUOTES | STRING_ALLOW_GLOBS)); + ASSERT_TRUE(string_is_safe("ab'*cd", STRING_ALLOW_QUOTES | STRING_ALLOW_GLOBS)); + ASSERT_FALSE(string_is_safe("ab\\cd", STRING_ALLOW_QUOTES | STRING_ALLOW_GLOBS)); /* backslash still rejected */ + + ASSERT_TRUE(string_is_safe("hello.txt", STRING_FILENAME)); + ASSERT_FALSE(string_is_safe("", STRING_FILENAME)); + ASSERT_FALSE(string_is_safe("foo/bar", STRING_FILENAME)); + + ASSERT_TRUE(string_is_safe("foo?bar", STRING_ASCII | STRING_ALLOW_GLOBS)); + ASSERT_FALSE(string_is_safe("foo\"bar", STRING_ASCII | STRING_ALLOW_GLOBS)); /* quotes still rejected */ + ASSERT_FALSE(string_is_safe("Ã¼ber", STRING_ASCII | STRING_ALLOW_GLOBS)); + + ASSERT_TRUE(string_is_safe("foo\\bar", STRING_ALLOW_BACKSLASHES)); + ASSERT_FALSE(string_is_safe("foo\"bar", STRING_ALLOW_BACKSLASHES)); /* quotes still rejected */ + ASSERT_FALSE(string_is_safe("foo*bar", STRING_ALLOW_BACKSLASHES)); /* globs still rejected */ + ASSERT_TRUE(string_is_safe("foo\\\"bar", STRING_ALLOW_BACKSLASHES | STRING_ALLOW_QUOTES)); + ASSERT_TRUE(string_is_safe("foo\\bar", STRING_ALLOW_BACKSLASHES | STRING_ALLOW_QUOTES)); + ASSERT_TRUE(string_is_safe("foo\"bar", STRING_ALLOW_BACKSLASHES | STRING_ALLOW_QUOTES)); + + /* All allow flags combined: only baseline (control chars, invalid UTF-8) and STRING_FILENAME apply. */ + StringSafeFlags all = STRING_ALLOW_EMPTY | STRING_ASCII | STRING_ALLOW_BACKSLASHES | STRING_ALLOW_QUOTES | STRING_ALLOW_GLOBS | STRING_FILENAME; + ASSERT_TRUE(string_is_safe("hello.txt", all)); + ASSERT_TRUE(string_is_safe("foo-bar_baz.conf", all)); + ASSERT_TRUE(string_is_safe("a", all)); + ASSERT_TRUE(string_is_safe("foo\\bar", all)); /* backslash allowed */ + ASSERT_TRUE(string_is_safe("foo\"bar", all)); /* quote allowed */ + ASSERT_TRUE(string_is_safe("foo'bar", all)); /* quote allowed */ + ASSERT_TRUE(string_is_safe("foo*bar", all)); /* glob allowed */ + ASSERT_TRUE(string_is_safe("foo?bar", all)); /* glob allowed */ + ASSERT_TRUE(string_is_safe("foo[bar", all)); /* glob allowed */ + ASSERT_FALSE(string_is_safe("", all)); /* fails STRING_FILENAME */ + ASSERT_FALSE(string_is_safe("Ã¼ber", all)); /* fails STRING_ASCII */ + ASSERT_FALSE(string_is_safe("foo/bar", all)); /* fails STRING_FILENAME */ + ASSERT_FALSE(string_is_safe(".", all)); /* fails STRING_FILENAME */ + ASSERT_FALSE(string_is_safe("..", all)); /* fails STRING_FILENAME */ + ASSERT_FALSE(string_is_safe("foo\x01""bar", all)); /* fails baseline control-char check */ + ASSERT_FALSE(string_is_safe(NULL, all)); +} + DEFINE_TEST_MAIN(LOG_DEBUG); diff --git a/src/veritysetup/veritysetup.c b/src/veritysetup/veritysetup.c index 4a244ae83dc..2e44aab9633 100644 --- a/src/veritysetup/veritysetup.c +++ b/src/veritysetup/veritysetup.c @@ -221,9 +221,6 @@ static int parse_options(const char *options) { arg_hash_offset = off; } else if ((val = startswith(word, "salt="))) { - if (!string_is_safe(val)) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "salt= is not valid."); - if (isempty(val)) { arg_salt = mfree(arg_salt); arg_salt_size = 32; diff --git a/src/vmspawn/vmspawn.c b/src/vmspawn/vmspawn.c index 5c70809e47c..5c0847b4814 100644 --- a/src/vmspawn/vmspawn.c +++ b/src/vmspawn/vmspawn.c @@ -851,7 +851,12 @@ static int parse_argv(int argc, char *argv[]) { break; OPTION_LONG("ssh-key-type", "TYPE", "Choose what type of SSH key to pass"): - if (!string_is_safe(arg)) + if (isempty(arg)) { + arg_ssh_key_type = mfree(arg_ssh_key_type); + break; + } + + if (!string_is_safe(arg, STRING_ALLOW_GLOBS)) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Invalid value for --ssh-key-type=: %s", arg); r = free_and_strdup_warn(&arg_ssh_key_type, arg);