From 15dca3711da63ac5fcfa01187a3b964d7042b99c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 30 Nov 2018 11:54:42 +0100 Subject: [PATCH] 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. --- src/basic/socket-label.c | 2 +- src/basic/socket-util.c | 76 ++++++++++++++++++++++--------------- src/basic/socket-util.h | 2 +- src/test/test-socket-util.c | 52 +++++++++++++++++++++++++ 4 files changed, 100 insertions(+), 32 deletions(-) diff --git a/src/basic/socket-label.c b/src/basic/socket-label.c index 470bde5145..4ed19cd937 100644 --- 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 d94bac4287..9bae1552fc 100644 --- a/src/basic/socket-util.c +++ b/src/basic/socket-util.c @@ -15,6 +15,7 @@ #include #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(""); if (!p) return -ENOMEM; - } else if (sa->un.sun_path[0] == 0) { - /* abstract */ - - /* 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 */ - - p = new(char, sizeof(sa->un.sun_path)+1); - if (!p) - return -ENOMEM; - - p[0] = '@'; - memcpy(p+1, sa->un.sun_path+1, sizeof(sa->un.sun_path)-1); - p[sizeof(sa->un.sun_path)] = 0; - } else { - p = strndup(sa->un.sun_path, sizeof(sa->un.sun_path)); + size_t path_len = salen - offsetof(struct sockaddr_un, sun_path); + + 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. */ + + _cleanup_free_ char *e = NULL; + + e = cescape_length(sa->un.sun_path + 1, path_len - 1); + if (!e) + return -ENOMEM; + + 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 8090e21657..37b1bca81a 100644 --- 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 687caf83be..349d866442 100644 --- 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, ""); + test_socket_print_unix_one("\0", 1, ""); + 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();