From: Zbigniew Jędrzejewski-Szmek Date: Fri, 30 Nov 2018 10:54:42 +0000 (+0100) Subject: basic/socket-util: use c-escaping to print unprintable socket paths X-Git-Tag: v240~165^2 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=15dca3711da63ac5fcfa01187a3b964d7042b99c;p=thirdparty%2Fsystemd.git 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 470bde51452..4ed19cd9375 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 d94bac42878..9bae1552fc4 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 */ + } 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 8090e21657e..37b1bca81a8 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 687caf83bed..349d866442a 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();