id128: when taking user input for a 128bit ID, validate syntax

Also, always accept both our simple hexdump syntax and UUID syntax.

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
 

man/sd_id128_to_string.xml

@@ -59,7 +59,7 @@
 
                         <funcprototype>
                                 <funcdef>int <function>sd_id128_from_string</function></funcdef>
-                                <paramdef>const char <parameter>s</parameter>[33], sd_id128_t* <parameter>ret</parameter></paramdef>
+                                <paramdef>const char* <parameter>s</parameter>, sd_id128_t* <parameter>ret</parameter></paramdef>
                         </funcprototype>
 
                 </funcsynopsis>
@@ -77,14 +77,19 @@
 
                 <para><function>sd_id128_from_string()</function>
                 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
-                <parameter>ret</parameter>.</para>
+                character string with 32 hexadecimal digits
+                (either lowercase or uppercase, terminated by NUL) and parses them back into an 128
+                bit ID returned in
+                <parameter>ret</parameter>. Alternatively, this call
+                can also parse a 37 character string with a 128bit ID
+                formatted as RFC UUID.</para>
 
                 <para>For more information about the
                 <literal>sd_id128_t</literal> type see
-                <citerefentry><refentrytitle>sd-id128</refentrytitle><manvolnum>3</manvolnum></citerefentry>.</para>
+                <citerefentry><refentrytitle>sd-id128</refentrytitle><manvolnum>3</manvolnum></citerefentry>. Note
+                that these calls operate the same way on all
+                architectures, i.e. the results do not depend on
+                endianess.</para>
 
                 <para>When formatting a 128 bit ID into a string it is
                 often easier to use a format string for

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;

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 */

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

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;
 

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;
+}

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);

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);
 

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;
 }