From aa96c6cb44a6eeccc506ae055aae2519a7f914e1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 29 Apr 2013 18:39:12 -0300 Subject: [PATCH] id128: when taking user input for a 128bit ID, validate syntax Also, always accept both our simple hexdump syntax and UUID syntax. --- TODO | 8 +---- man/sd_id128_to_string.xml | 17 +++++---- src/core/load-dropin.c | 40 +++++++++++++-------- src/core/machine-id-setup.c | 30 +++++++++------- src/libsystemd-id128/sd-id128.c | 61 +++++++++++++++++++++------------ src/nspawn/nspawn.c | 5 +++ src/shared/util.c | 41 ++++++++++++++++++++++ src/shared/util.h | 2 ++ src/systemd/sd-id128.h | 2 +- src/test/test-id128.c | 27 +++++++++++++-- 10 files changed, 168 insertions(+), 65 deletions(-) diff --git a/TODO b/TODO index 80a591f01..bb1802813 100644 --- a/TODO +++ b/TODO @@ -26,9 +26,7 @@ Fedora 19: Features: -* nss-myhostname: investigate whether there's any point in also - resolving localhost6, localhost.localdomain, ip6-localhost or any of - the other names often seen in /etc/hosts +* investigate endianess issues of UUID vs. GUID * see if we can fix https://bugs.freedesktop.org/show_bug.cgi?id=63672 without dropping the location cache entirely. @@ -36,8 +34,6 @@ Features: * dbus: when a unit failed to load (i.e. is in UNIT_ERROR state), we should be able to safely try another attempt when the bus call LoadUnit() is invoked. -* for instanced unit drop-ins we should look in foo@bar.service.d/ as well as foo@.service.d/ - * if pam_systemd is invoked by su from a process that is outside of a any session we should probably just become a NOP, since that's usually not a real user session but just some system code that just @@ -61,8 +57,6 @@ Features: * make sure cg_pid_get_path() works properly for co-mounted controllers -* nspawn: ensure syntax of --uuid= argument is correct - * explicitly disallow changing the cgroup path of units in the name=systemd hierarchy, unless it is outside of /system diff --git a/man/sd_id128_to_string.xml b/man/sd_id128_to_string.xml index ec8b263e0..593d0752d 100644 --- a/man/sd_id128_to_string.xml +++ b/man/sd_id128_to_string.xml @@ -59,7 +59,7 @@ int sd_id128_from_string - const char s[33], sd_id128_t* ret + const char* s, sd_id128_t* ret @@ -77,14 +77,19 @@ sd_id128_from_string() implements the reverse operation: it takes a 33 - character array with 32 hexadecimal digits - (terminated by NUL) and parses them back into an - 128 bit ID returned in - ret. + character string with 32 hexadecimal digits + (either lowercase or uppercase, terminated by NUL) and parses them back into an 128 + bit ID returned in + ret. Alternatively, this call + can also parse a 37 character string with a 128bit ID + formatted as RFC UUID. For more information about the sd_id128_t type see - sd-id1283. + sd-id1283. Note + that these calls operate the same way on all + architectures, i.e. the results do not depend on + endianess. When formatting a 128 bit ID into a string it is often easier to use a format string for diff --git a/src/core/load-dropin.c b/src/core/load-dropin.c index 0318296f1..a877e6609 100644 --- a/src/core/load-dropin.c +++ b/src/core/load-dropin.c @@ -31,7 +31,12 @@ #include "load-fragment.h" #include "conf-files.h" -static int iterate_dir(Unit *u, const char *path, UnitDependency dependency, char ***strv) { +static int iterate_dir( + Unit *u, + const char *path, + UnitDependency dependency, + char ***strv) { + _cleanup_closedir_ DIR *d = NULL; int r; @@ -86,7 +91,14 @@ static int iterate_dir(Unit *u, const char *path, UnitDependency dependency, cha return 0; } -static int process_dir(Unit *u, const char *unit_path, const char *name, const char *suffix, UnitDependency dependency, char ***strv) { +static int process_dir( + Unit *u, + const char *unit_path, + const char *name, + const char *suffix, + UnitDependency dependency, + char ***strv) { + int r; char *path; @@ -97,7 +109,7 @@ static int process_dir(Unit *u, const char *unit_path, const char *name, const c path = strjoin(unit_path, "/", name, suffix, NULL); if (!path) - return -ENOMEM; + return log_oom(); if (u->manager->unit_path_cache && !set_get(u->manager->unit_path_cache, path)) @@ -115,13 +127,13 @@ static int process_dir(Unit *u, const char *unit_path, const char *name, const c template = unit_name_template(name); if (!template) - return -ENOMEM; + return log_oom(); path = strjoin(unit_path, "/", template, suffix, NULL); free(template); if (!path) - return -ENOMEM; + return log_oom(); if (u->manager->unit_path_cache && !set_get(u->manager->unit_path_cache, path)) @@ -138,10 +150,10 @@ static int process_dir(Unit *u, const char *unit_path, const char *name, const c } char **unit_find_dropin_paths(Unit *u) { - Iterator i; - char *t; _cleanup_strv_free_ char **strv = NULL; char **configs = NULL; + Iterator i; + char *t; int r; assert(u); @@ -157,14 +169,14 @@ char **unit_find_dropin_paths(Unit *u) { } } - if (!strv_isempty(strv)) { - r = conf_files_list_strv(&configs, ".conf", NULL, (const char**) strv); - if (r < 0) { - log_error("Failed to get list of configuration files: %s", strerror(-r)); - strv_free(configs); - return NULL; - } + if (strv_isempty(strv)) + return NULL; + r = conf_files_list_strv(&configs, ".conf", NULL, (const char**) strv); + if (r < 0) { + log_error("Failed to get list of configuration files: %s", strerror(-r)); + strv_free(configs); + return NULL; } return configs; diff --git a/src/core/machine-id-setup.c b/src/core/machine-id-setup.c index 608b0a5e7..18e015fe7 100644 --- a/src/core/machine-id-setup.c +++ b/src/core/machine-id-setup.c @@ -72,16 +72,19 @@ static int generate(char id[34]) { /* First, try reading the D-Bus machine id, unless it is a symlink */ fd = open("/var/lib/dbus/machine-id", O_RDONLY|O_CLOEXEC|O_NOCTTY|O_NOFOLLOW); if (fd >= 0) { - - k = loop_read(fd, id, 32, false); + k = loop_read(fd, id, 33, false); close_nointr_nofail(fd); - if (k >= 32) { - id[32] = '\n'; - id[33] = 0; + if (k == 33 && id[32] == '\n') { - log_info("Initializing machine ID from D-Bus machine ID."); - return 0; + id[32] = 0; + if (id128_is_valid(id)) { + id[32] = '\n'; + id[33] = 0; + + log_info("Initializing machine ID from D-Bus machine ID."); + return 0; + } } } @@ -113,7 +116,7 @@ static int generate(char id[34]) { * $container_uuid the way libvirt/LXC does it */ r = detect_container(NULL); if (r > 0) { - char *e; + _cleanup_free_ char *e = NULL; r = getenv_for_pid(1, "container_uuid", &e); if (r > 0) { @@ -121,12 +124,9 @@ static int generate(char id[34]) { r = shorten_uuid(id, e); if (r >= 0) { log_info("Initializing machine ID from container UUID."); - free(e); return 0; } } - - free(e); } } @@ -183,8 +183,12 @@ int machine_id_setup(void) { } if (S_ISREG(st.st_mode)) - if (loop_read(fd, id, 32, false) >= 32) - return 0; + if (loop_read(fd, id, 33, false) == 33 && id[32] == '\n') { + id[32] = 0; + + if (id128_is_valid(id)) + return 0; + } /* Hmm, so, the id currently stored is not useful, then let's * generate one */ diff --git a/src/libsystemd-id128/sd-id128.c b/src/libsystemd-id128/sd-id128.c index 68c498714..64ddd0923 100644 --- a/src/libsystemd-id128/sd-id128.c +++ b/src/libsystemd-id128/sd-id128.c @@ -44,30 +44,50 @@ _public_ char *sd_id128_to_string(sd_id128_t id, char s[33]) { return s; } -_public_ int sd_id128_from_string(const char s[33], sd_id128_t *ret) { - unsigned n; +_public_ int sd_id128_from_string(const char s[], sd_id128_t *ret) { + unsigned n, i; sd_id128_t t; + bool is_guid = false; if (!s) return -EINVAL; if (!ret) return -EINVAL; - for (n = 0; n < 16; n++) { + for (n = 0, i = 0; n < 16;) { int a, b; - a = unhexchar(s[n*2]); + if (s[i] == '-') { + /* Is this a GUID? Then be nice, and skip over + * the dashes */ + + if (i == 8) + is_guid = true; + else if (i == 13 || i == 18 || i == 23) { + if (!is_guid) + return -EINVAL; + } else + return -EINVAL; + + i++; + continue; + } + + a = unhexchar(s[i++]); if (a < 0) return -EINVAL; - b = unhexchar(s[n*2+1]); + b = unhexchar(s[i++]); if (b < 0) return -EINVAL; - t.bytes[n] = (a << 4) | b; + t.bytes[n++] = (a << 4) | b; } - if (s[32] != 0) + if (i != (is_guid ? 36 : 32)) + return -EINVAL; + + if (s[i] != 0) return -EINVAL; *ret = t; @@ -90,8 +110,8 @@ static sd_id128_t make_v4_uuid(sd_id128_t id) { _public_ int sd_id128_get_machine(sd_id128_t *ret) { static __thread sd_id128_t saved_machine_id; static __thread bool saved_machine_id_valid = false; - int fd; - char buf[32]; + _cleanup_close_ int fd = -1; + char buf[33]; ssize_t k; unsigned j; sd_id128_t t; @@ -108,13 +128,14 @@ _public_ int sd_id128_get_machine(sd_id128_t *ret) { if (fd < 0) return -errno; - k = loop_read(fd, buf, 32, false); - close_nointr_nofail(fd); - + k = loop_read(fd, buf, 33, false); if (k < 0) return (int) k; - if (k < 32) + if (k != 33) + return -EIO; + + if (buf[32] !='\n') return -EIO; for (j = 0; j < 16; j++) { @@ -139,7 +160,7 @@ _public_ int sd_id128_get_machine(sd_id128_t *ret) { _public_ int sd_id128_get_boot(sd_id128_t *ret) { static __thread sd_id128_t saved_boot_id; static __thread bool saved_boot_id_valid = false; - int fd; + _cleanup_close_ int fd = -1; char buf[36]; ssize_t k; unsigned j; @@ -159,12 +180,10 @@ _public_ int sd_id128_get_boot(sd_id128_t *ret) { return -errno; k = loop_read(fd, buf, 36, false); - close_nointr_nofail(fd); - if (k < 0) return (int) k; - if (k < 36) + if (k != 36) return -EIO; for (j = 0, p = buf; j < 16; j++) { @@ -195,9 +214,9 @@ _public_ int sd_id128_get_boot(sd_id128_t *ret) { } _public_ int sd_id128_randomize(sd_id128_t *ret) { - int fd; - ssize_t k; + _cleanup_close_ int fd = -1; sd_id128_t t; + ssize_t k; if (!ret) return -EINVAL; @@ -207,12 +226,10 @@ _public_ int sd_id128_randomize(sd_id128_t *ret) { return -errno; k = loop_read(fd, &t, 16, false); - close_nointr_nofail(fd); - if (k < 0) return (int) k; - if (k < 16) + if (k != 16) return -EIO; /* Turn this into a valid v4 UUID, to be nice. Note that we diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c index a49cbc223..0a4631363 100644 --- a/src/nspawn/nspawn.c +++ b/src/nspawn/nspawn.c @@ -222,6 +222,11 @@ static int parse_argv(int argc, char *argv[]) { break; case ARG_UUID: + if (!id128_is_valid(optarg)) { + log_error("Invalid UUID: %s", optarg); + return -EINVAL; + } + arg_uuid = optarg; break; diff --git a/src/shared/util.c b/src/shared/util.c index 38ee493a8..f9ec14aff 100644 --- a/src/shared/util.c +++ b/src/shared/util.c @@ -5854,3 +5854,44 @@ void* greedy_realloc(void **p, size_t *allocated, size_t need) { *allocated = a; return q; } + +bool id128_is_valid(const char *s) { + size_t i, l; + + l = strlen(s); + if (l == 32) { + + /* Simple formatted 128bit hex string */ + + for (i = 0; i < l; i++) { + char c = s[i]; + + if (!(c >= '0' && c <= '9') && + !(c >= 'a' && c <= 'z') && + !(c >= 'A' && c <= 'Z')) + return false; + } + + } else if (l == 36) { + + /* Formatted UUID */ + + for (i = 0; i < l; i++) { + char c = s[i]; + + if ((i == 8 || i == 13 || i == 18 || i == 23)) { + if (c != '-') + return false; + } else { + if (!(c >= '0' && c <= '9') && + !(c >= 'a' && c <= 'z') && + !(c >= 'A' && c <= 'Z')) + return false; + } + } + + } else + return false; + + return true; +} diff --git a/src/shared/util.h b/src/shared/util.h index e3fc87685..5d1b0b1c3 100644 --- a/src/shared/util.h +++ b/src/shared/util.h @@ -733,3 +733,5 @@ static inline void _reset_locale_(struct _locale_struct_ *s) { } \ !_saved_locale_.quit; }) ; \ _saved_locale_.quit = true) + +bool id128_is_valid(const char *s); diff --git a/src/systemd/sd-id128.h b/src/systemd/sd-id128.h index 79bb8b3c9..3a8393209 100644 --- a/src/systemd/sd-id128.h +++ b/src/systemd/sd-id128.h @@ -40,7 +40,7 @@ union sd_id128 { char *sd_id128_to_string(sd_id128_t id, char s[33]); -int sd_id128_from_string(const char s[33], sd_id128_t *ret); +int sd_id128_from_string(const char *s, sd_id128_t *ret); int sd_id128_randomize(sd_id128_t *ret); diff --git a/src/test/test-id128.c b/src/test/test-id128.c index bfd743eca..2ed8e292e 100644 --- a/src/test/test-id128.c +++ b/src/test/test-id128.c @@ -27,10 +27,13 @@ #include "macro.h" #define ID128_WALDI SD_ID128_MAKE(01, 02, 03, 04, 05, 06, 07, 08, 09, 0a, 0b, 0c, 0d, 0e, 0f, 10) +#define STR_WALDI "0102030405060708090a0b0c0d0e0f10" +#define UUID_WALDI "01020304-0506-0708-090a-0b0c0d0e0f10" int main(int argc, char *argv[]) { sd_id128_t id, id2; char t[33]; + _cleanup_free_ char *b = NULL; assert_se(sd_id128_randomize(&id) == 0); printf("random: %s\n", sd_id128_to_string(id, t)); @@ -45,8 +48,28 @@ int main(int argc, char *argv[]) { printf("boot: %s\n", sd_id128_to_string(id, t)); printf("waldi: %s\n", sd_id128_to_string(ID128_WALDI, t)); - - printf("waldi2: " SD_ID128_FORMAT_STR "\n", SD_ID128_FORMAT_VAL(ID128_WALDI)); + assert_se(streq(t, STR_WALDI)); + + assert_se(asprintf(&b, SD_ID128_FORMAT_STR, SD_ID128_FORMAT_VAL(ID128_WALDI)) == 32); + printf("waldi2: %s\n", b); + assert_se(streq(t, b)); + + assert_se(sd_id128_from_string(UUID_WALDI, &id) >= 0); + assert_se(sd_id128_equal(id, ID128_WALDI)); + + assert_se(sd_id128_from_string("", &id) < 0); + assert_se(sd_id128_from_string("01020304-0506-0708-090a-0b0c0d0e0f101", &id) < 0); + assert_se(sd_id128_from_string("01020304-0506-0708-090a-0b0c0d0e0f10-", &id) < 0); + assert_se(sd_id128_from_string("01020304-0506-0708-090a0b0c0d0e0f10", &id) < 0); + assert_se(sd_id128_from_string("010203040506-0708-090a-0b0c0d0e0f10", &id) < 0); + + assert_se(id128_is_valid(STR_WALDI)); + assert_se(id128_is_valid(UUID_WALDI)); + assert_se(!id128_is_valid("")); + assert_se(!id128_is_valid("01020304-0506-0708-090a-0b0c0d0e0f101")); + assert_se(!id128_is_valid("01020304-0506-0708-090a-0b0c0d0e0f10-")); + assert_se(!id128_is_valid("01020304-0506-0708-090a0b0c0d0e0f10")); + assert_se(!id128_is_valid("010203040506-0708-090a-0b0c0d0e0f10")); return 0; } -- 2.30.2