basic/socket-util: use c-escaping to print unprintable socket paths

We are pretty careful to reject abstract sockets that are too long to fit in
the address structure as a NUL-terminated string. And since we parse sockets as
strings, it is not possible to embed a NUL in the the address either.  But we
might receive an external socket (abstract or not), and we want to be able to
print its address in all cases. We would call socket_address_verify() and
refuse to print various sockets that the kernel considers legit.

Let's do the strict verification only in case of socket addresses we parse and
open ourselves, and do less strict verification when printing addresses of
existing sockets, and use c-escaping to print embedded NULs and such.

More tests are added.

This should make LGTM happier because on FIXME comment is removed.

diff --git a/src/basic/socket-label.c b/src/basic/socket-label.c
index 470bde514528b14693b11ec19cd189b91cd81946..4ed19cd937573168550a4bdb2f22b0372b99495f 100644 (file)
--- a/src/basic/socket-label.c
+++ b/src/basic/socket-label.c
@@ -39,7 +39,7 @@ int socket_address_listen(
 
         assert(a);
 
-        r = socket_address_verify(a);
+        r = socket_address_verify(a, true);
         if (r < 0)
                 return r;
 

diff --git a/src/basic/socket-util.c b/src/basic/socket-util.c
index d94bac42878caf910ea9a4742b402fd53eeac74a..9bae1552fc4486be5b28418d1cc7bad13f9716f7 100644 (file)
--- a/src/basic/socket-util.c
+++ b/src/basic/socket-util.c
@@ -15,6 +15,7 @@
 #include <unistd.h>
 
 #include "alloc-util.h"
+#include "escape.h"
 #include "fd-util.h"
 #include "fileio.h"
 #include "format-util.h"
@@ -111,7 +112,7 @@ int socket_address_parse(SocketAddress *a, const char *s) {
                 size_t l;
 
                 l = strlen(s+1);
-                if (l >= sizeof(a->sockaddr.un.sun_path) - 1) /* Note that we refuse non-NUL-terminate sockets here
+                if (l >= sizeof(a->sockaddr.un.sun_path) - 1) /* Note that we refuse non-NUL-terminated sockets here
                                                                * when parsing, even though abstract namespace sockets
                                                                * explicitly allow embedded NUL bytes and don't consider
                                                                * them special. But it's simply annoying to debug such
@@ -261,9 +262,12 @@ int socket_address_parse_netlink(SocketAddress *a, const char *s) {
         return 0;
 }
 
-int socket_address_verify(const SocketAddress *a) {
+int socket_address_verify(const SocketAddress *a, bool strict) {
         assert(a);
 
+        /* With 'strict' we enforce additional sanity constraints which are not set by the standard,
+         * but should only apply to sockets we create ourselves. */
+
         switch (socket_address_family(a)) {
 
         case AF_INET:
@@ -293,19 +297,20 @@ int socket_address_verify(const SocketAddress *a) {
         case AF_UNIX:
                 if (a->size < offsetof(struct sockaddr_un, sun_path))
                         return -EINVAL;
-                if (a->size > sizeof(struct sockaddr_un)+1) /* Allow one extra byte, since getsockname() on Linux will
-                                                             * append a NUL byte if we have path sockets that are above
-                                                             * sun_path' full size */
+                if (a->size > sizeof(struct sockaddr_un) + !strict)
+                        /* If !strict, allow one extra byte, since getsockname() on Linux will append
+                         * a NUL byte if we have path sockets that are above sun_path's full size. */
                         return -EINVAL;
 
                 if (a->size > offsetof(struct sockaddr_un, sun_path) &&
-                    a->sockaddr.un.sun_path[0] != 0) { /* Only validate file system sockets here */
-
+                    a->sockaddr.un.sun_path[0] != 0 &&
+                    strict) {
+                        /* Only validate file system sockets here, and only in strict mode */
                         const char *e;
 
                         e = memchr(a->sockaddr.un.sun_path, 0, sizeof(a->sockaddr.un.sun_path));
                         if (e) {
-                                /* If there's an embedded NUL byte, make sure the size of the socket addresses matches it */
+                                /* If there's an embedded NUL byte, make sure the size of the socket address matches it */
                                 if (a->size != offsetof(struct sockaddr_un, sun_path) + (e - a->sockaddr.un.sun_path) + 1)
                                         return -EINVAL;
                         } else {
@@ -353,7 +358,10 @@ int socket_address_print(const SocketAddress *a, char **ret) {
         assert(a);
         assert(ret);
 
-        r = socket_address_verify(a);
+        r = socket_address_verify(a, false); /* We do non-strict validation, because we want to be
+                                              * able to pretty-print any socket the kernel considers
+                                              * valid. We still need to do validation to know if we
+                                              * can meaningfully print the address. */
         if (r < 0)
                 return r;
 
@@ -386,8 +394,8 @@ bool socket_address_equal(const SocketAddress *a, const SocketAddress *b) {
         assert(b);
 
         /* Invalid addresses are unequal to all */
-        if (socket_address_verify(a) < 0 ||
-            socket_address_verify(b) < 0)
+        if (socket_address_verify(a, false) < 0 ||
+            socket_address_verify(b, false) < 0)
                 return false;
 
         if (a->type != b->type)
@@ -642,31 +650,39 @@ int sockaddr_pretty(const struct sockaddr *_sa, socklen_t salen, bool translate_
         }
 
         case AF_UNIX:
-                if (salen <= offsetof(struct sockaddr_un, sun_path)) {
+                if (salen <= offsetof(struct sockaddr_un, sun_path) ||
+                    (sa->un.sun_path[0] == 0 && salen == offsetof(struct sockaddr_un, sun_path) + 1)) {
+                        /* The name must have at least one character (and the leading NUL does not count) */
+
                         p = strdup("<unnamed>");
                         if (!p)
                                 return -ENOMEM;
 
-                } else if (sa->un.sun_path[0] == 0) {
-                        /* abstract */
+                } else {
+                        size_t path_len = salen - offsetof(struct sockaddr_un, sun_path);
 
-                        /* FIXME: We assume we can print the
-                         * socket path here and that it hasn't
-                         * more than one NUL byte. That is
-                         * actually an invalid assumption */
+                        if (sa->un.sun_path[0] == 0) {
+                                /* Abstract socket. When parsing address information from, we
+                                 * explicitly reject overly long paths and paths with embedded NULs.
+                                 * But we might get such a socket from the outside. Let's return
+                                 * something meaningful and printable in this case. */
 
-                        p = new(char, sizeof(sa->un.sun_path)+1);
-                        if (!p)
-                                return -ENOMEM;
+                                _cleanup_free_ char *e = NULL;
 
-                        p[0] = '@';
-                        memcpy(p+1, sa->un.sun_path+1, sizeof(sa->un.sun_path)-1);
-                        p[sizeof(sa->un.sun_path)] = 0;
+                                e = cescape_length(sa->un.sun_path + 1, path_len - 1);
+                                if (!e)
+                                        return -ENOMEM;
 
-                } else {
-                        p = strndup(sa->un.sun_path, sizeof(sa->un.sun_path));
+                                p = strjoin("@", e);
+                        } else {
+                                if (sa->un.sun_path[path_len - 1] == '\0')
+                                        /* We expect a terminating NUL and don't print it */
+                                        path_len --;
+
+                                p = cescape_length(sa->un.sun_path, path_len);
+                        }
                         if (!p)
-                                return -ENOMEM;
+                                        return -ENOMEM;
                 }
 
                 break;

diff --git a/src/basic/socket-util.h b/src/basic/socket-util.h
index 8090e21657e5c4d056f1550a5abb501a846ff78a..37b1bca81a8e104ffea667f1265867e06a5e61ec 100644 (file)
--- a/src/basic/socket-util.h
+++ b/src/basic/socket-util.h
@@ -70,7 +70,7 @@ int socket_address_parse(SocketAddress *a, const char *s);
 int socket_address_parse_and_warn(SocketAddress *a, const char *s);
 int socket_address_parse_netlink(SocketAddress *a, const char *s);
 int socket_address_print(const SocketAddress *a, char **p);
-int socket_address_verify(const SocketAddress *a) _pure_;
+int socket_address_verify(const SocketAddress *a, bool strict) _pure_;
 
 int sockaddr_un_unlink(const struct sockaddr_un *sa);
 

diff --git a/src/test/test-socket-util.c b/src/test/test-socket-util.c
index 687caf83bedd8d704c4218cf057de4feed09cedd..349d866442adf7db8057972261b501fa36d3e719 100644 (file)
--- a/src/test/test-socket-util.c
+++ b/src/test/test-socket-util.c
@@ -6,6 +6,7 @@
 
 #include "alloc-util.h"
 #include "async.h"
+#include "escape.h"
 #include "exit-status.h"
 #include "fd-util.h"
 #include "fileio.h"
@@ -62,6 +63,9 @@ static void test_socket_address_parse_one(const char *in, int ret, int family, c
         }
 }
 
+#define SUN_PATH_LEN (sizeof(((struct sockaddr_un){}).sun_path))
+assert_cc(sizeof(((struct sockaddr_un){}).sun_path) == 108);
+
 static void test_socket_address_parse(void) {
         log_info("/* %s */", __func__);
 
@@ -95,8 +99,21 @@ static void test_socket_address_parse(void) {
         test_socket_address_parse_one("[::1]:8888", 0, AF_INET6, NULL);
         test_socket_address_parse_one("192.168.1.254:8888", 0, AF_INET, NULL);
         test_socket_address_parse_one("/foo/bar", 0, AF_UNIX, NULL);
+        test_socket_address_parse_one("/", 0, AF_UNIX, NULL);
         test_socket_address_parse_one("@abstract", 0, AF_UNIX, NULL);
 
+        {
+                char aaa[SUN_PATH_LEN + 1] = "@";
+
+                memset(aaa + 1, 'a', SUN_PATH_LEN - 1);
+                char_array_0(aaa);
+
+                test_socket_address_parse_one(aaa, -EINVAL, 0, NULL);
+
+                aaa[SUN_PATH_LEN - 1] = '\0';
+                test_socket_address_parse_one(aaa, 0, AF_UNIX, NULL);
+        }
+
         test_socket_address_parse_one("vsock:2:1234", 0, AF_VSOCK, NULL);
         test_socket_address_parse_one("vsock::1234", 0, AF_VSOCK, NULL);
         test_socket_address_parse_one("vsock:2:1234x", -EINVAL, 0, NULL);
@@ -104,6 +121,40 @@ static void test_socket_address_parse(void) {
         test_socket_address_parse_one("vsock:2", -EINVAL, 0, NULL);
 }
 
+static void test_socket_print_unix_one(const char *in, size_t len_in, const char *expected) {
+        _cleanup_free_ char *out = NULL, *c = NULL;
+
+        SocketAddress a = { .sockaddr = { .un = { .sun_family = AF_UNIX } },
+                            .size = offsetof(struct sockaddr_un, sun_path) + len_in,
+                            .type = SOCK_STREAM,
+        };
+        memcpy(a.sockaddr.un.sun_path, in, len_in);
+
+        assert_se(socket_address_print(&a, &out) >= 0);
+        assert_se(c = cescape(in));
+        log_info("\"%s\" → \"%s\" (expect \"%s\")", in, out, expected);
+        assert_se(streq(out, expected));
+}
+
+static void test_socket_print_unix(void) {
+        log_info("/* %s */", __func__);
+
+        /* Some additional tests for abstract addresses which we don't parse */
+
+        test_socket_print_unix_one("\0\0\0\0", 4, "@\\000\\000\\000");
+        test_socket_print_unix_one("@abs", 5, "@abs");
+        test_socket_print_unix_one("\n", 2, "\\n");
+        test_socket_print_unix_one("", 1, "<unnamed>");
+        test_socket_print_unix_one("\0", 1, "<unnamed>");
+        test_socket_print_unix_one("\0_________________________there's 108 characters in this string_____________________________________________", 108,
+                                   "@_________________________there\\'s 108 characters in this string_____________________________________________");
+        test_socket_print_unix_one("////////////////////////////////////////////////////////////////////////////////////////////////////////////", 108,
+                                   "////////////////////////////////////////////////////////////////////////////////////////////////////////////");
+        test_socket_print_unix_one("////////////////////////////////////////////////////////////////////////////////////////////////////////////", 109,
+                                   "////////////////////////////////////////////////////////////////////////////////////////////////////////////");
+        test_socket_print_unix_one("\0\a\b\n\255", 6, "@\\a\\b\\n\\255\\000");
+}
+
 static void test_socket_address_parse_netlink(void) {
         SocketAddress a;
 
@@ -748,6 +799,7 @@ int main(int argc, char *argv[]) {
         test_ifname_valid();
 
         test_socket_address_parse();
+        test_socket_print_unix();
         test_socket_address_parse_netlink();
         test_socket_address_equal();
         test_socket_address_get_path();