sockets: improve error reporting if UNIX socket path is too long
PATCH v3 1/1 sockets: improve error reporting if UNIX socket path is too long
The 'struct sockaddr_un' only allows 108 bytes for the socket path.
If the user supplies a path, QEMU uses snprintf() to silently truncate it when too long. This is undesirable because the user will then be unable to connect to the path they asked for.
If the user doesn't supply a path, QEMU builds one based on TMPDIR, but if that leads to an overlong path, it mistakenly uses error_setg_errno() with a stale errno value, because snprintf() does not set errno on truncation.
In solving this the code needed some refactoring to ensure we don't pass 'un.sun_path' directly to any APIs which expect NUL-terminated strings, because the path is not required to be terminated.
---
Changed in v3:
- Also fix unix_connect_saddr error reporting
- Avoid calling unlink with path that might not be nul terminated
- Ensure the TMPDIR derived path can fill up sun_path space.
- Don't update saddr->path until we have succesfully listened
- Unify error reporting across both explicit path & TMPDIR path branches
util/qemu-sockets.c | 64 +++++++++++++++++++++++++++++++++++------------------ 1 file changed, 43 insertions(+), 21 deletions(-)
util/qemu-sockets.c +43 -21
@@ -845,6 +845,8 @@ static int unix_listen_saddr(UnixSocketAddress *saddr, {struct sockaddr_un un;int sock, fd; +char *pathbuf =NULL ; +const char *path; sock =qemu_socket (PF_UNIX ,SOCK_STREAM ,0 );if (sock <0 ) {
@@ -852,19 +854,12 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,return -1 ; } -memset (&un,0 ,sizeof (un)); - un.sun_family =AF_UNIX ; -if (saddr->path &&strlen (saddr->path)) { -snprintf (un.sun_path,sizeof (un.sun_path),"%s" , saddr->path); +if (saddr->path && saddr->path[0 ]) { + path = saddr->path; }else {const char *tmpdir =getenv ("TMPDIR" ); tmpdir = tmpdir ? tmpdir :"/tmp" ; -if (snprintf (un.sun_path,sizeof (un.sun_path),"%s/qemu-socket-XXXXXX" , - tmpdir) >=sizeof (un.sun_path)) { -error_setg_errno (errp, errno, -"TMPDIR environment variable (%s) too large" , tmpdir); - goto err; - } + path = pathbuf =g_strdup_printf ("%s/qemu-socket-XXXXXX" , tmpdir);/* * This dummy fd usage silences the mktemp() unsecure warning.
@@ -873,24 +868,32 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,* to unlink first and thus re-open the race window. The * worst case possible is bind() failing, i.e. a DoS attack. */ - fd =mkstemp (un.sun_path); + fd =mkstemp (pathbuf);if (fd <0 ) {error_setg_errno (errp, errno,"Failed to make a temporary socket name in %s" , tmpdir); goto err; }close (fd); -if (update_addr) { -g_free (saddr->path); - saddr->path =g_strdup (un.sun_path); - } } -if (unlink (un.sun_path) <0 && errno !=ENOENT ) { +if (strlen (path) >sizeof (un.sun_path)) { +error_setg (errp,"UNIX socket path '%s' is too long" , path); +error_append_hint (errp,"Path must be less than %zu bytes\n" , +sizeof (un.sun_path)); + goto err; + } + +if (unlink (path) <0 && errno !=ENOENT ) {error_setg_errno (errp, errno, -"Failed to unlink socket %s" , un.sun_path); +"Failed to unlink socket %s" , path); goto err; } + +memset (&un,0 ,sizeof (un)); + un.sun_family =AF_UNIX ; +strncpy (un.sun_path, path,sizeof (un.sun_path)); +if (bind (sock, (struct sockaddr *) &un,sizeof (un)) <0 ) {error_setg_errno (errp, errno,"Failed to bind socket to %s" , un.sun_path); goto err;
@@ -900,9 +903,16 @@ static int unix_listen_saddr(UnixSocketAddress *saddr, goto err; } +if (update_addr && pathbuf) { +g_free (saddr->path); + saddr->path = pathbuf; + }else { +g_free (pathbuf); + }return sock; err: +g_free (pathbuf);closesocket (sock);return -1 ; }
@@ -932,9 +942,16 @@ static int unix_connect_saddr(UnixSocketAddress *saddr,qemu_set_nonblock (sock); } +if (strlen (saddr->path) >sizeof (un.sun_path)) { +error_setg (errp,"UNIX socket path '%s' is too long" , saddr->path); +error_append_hint (errp,"Path must be less than %zu bytes\n" , +sizeof (un.sun_path)); + goto err; + } +memset (&un,0 ,sizeof (un)); un.sun_family =AF_UNIX ; -snprintf (un.sun_path,sizeof (un.sun_path),"%s" , saddr->path); +strncpy (un.sun_path, saddr->path,sizeof (un.sun_path));/* connect to peer */ do {
@@ -956,13 +973,18 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, }if (rc <0 ) { -error_setg_errno (errp, -rc,"Failed to connect socket" ); -close (sock); - sock =-1 ; +error_setg_errno (errp, -rc,"Failed to connect socket %s" , + saddr->path); + goto err; }g_free (connect_state);return sock; + + err: +close (sock); +g_free (connect_state); +return -1 ; }#else
Signature · 1 line
2.9.3
Quoted 24 lines · Daniel P. Berrange
The 'struct
sockaddr_un' only allows 108 bytes for the socket path.If the user supplies a path, QEMU uses
snprintf()to silently truncate it when too long. This is undesirable because the user will then be unable to connect to the path they asked for.If the user doesn't supply a path, QEMU builds one based on TMPDIR, but if that leads to an overlong path, it mistakenly uses
error_setg_errno()with a stale errno value, becausesnprintf()does not set errno on truncation.In solving this the code needed some refactoring to ensure we don't pass 'un.sun_path' directly to any APIs which expect NUL-terminated strings, because the path is not required to be terminated.
Signed-off-by Daniel P. Berrange <[email protected]>---
} else { const char *tmpdir = getenv("TMPDIR"); tmpdir = tmpdir ? tmpdir : "/tmp";
util/qemu-sockets.c static int unix_listen_saddr(UnixSocketAddress *saddr, · line 866
Quoted 5 lines
util/qemu-sockets.c +0
-5-if (snprintf (un.sun_path,sizeof (un.sun_path),"%s/qemu-socket-XXXXXX" , - tmpdir) >=sizeof (un.sun_path)) { -error_setg_errno (errp, errno, -"TMPDIR environment variable (%s) too large" , tmpdir); - goto err;
The old code fails early if the generated name is too long,...
util/qemu-sockets.c static int unix_listen_saddr(UnixSocketAddress *saddr, · line 871
Quoted 11 lines
util/qemu-sockets.c +2
-2- } + path = pathbuf =g_strdup_printf ("%s/qemu-socket-XXXXXX" , tmpdir);/* * This dummy fd usage silences the mktemp() unsecure warning. @@ -873,24 +868,32 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,* to unlink first and thus re-open the race window. The * worst case possible is bind() failing, i.e. a DoS attack. */ - fd =mkstemp (un.sun_path); + fd =mkstemp (pathbuf);
...while the new code ends up creating a too-long name in the file-system anyways,...
Quoted 7 lines
util/qemu-sockets.c +0
-1if (fd <0 ) {error_setg_errno (errp, errno,"Failed to make a temporary socket name in %s" , tmpdir); goto err; }close (fd); -if (update_addr) {
util/qemu-sockets.c static int unix_listen_saddr(UnixSocketAddress *saddr, · line 883
Quoted 10 lines
util/qemu-sockets.c +4
-4-g_free (saddr->path); - saddr->path =g_strdup (un.sun_path); - } } -if (unlink (un.sun_path) <0 && errno !=ENOENT ) { +if (strlen (path) >sizeof (un.sun_path)) { +error_setg (errp,"UNIX socket path '%s' is too long" , path); +error_append_hint (errp,"Path must be less than %zu bytes\n" , +sizeof (un.sun_path));
The old message mentioned the name that was too long, the new does not. That's a potential slight loss in error message quality, particularly since you are no longer mentioning a UNIX socket (which may give the user a clue why 108 is special), but I can probably live with it (it's still pretty obvious from the message that you should investigate what is too long).
Review static int unix_listen_saddr(UnixSocketAddress *saddr, · line 884
Quoted 1 line
(quoted diff) +1
-0+ goto err;
...here, you finally diagnose that the name is too long,...
util/qemu-sockets.c static int unix_listen_saddr(UnixSocketAddress *saddr, · line 887
Quoted 3 lines
util/qemu-sockets.c +3
-0+ } + +if (unlink (path) <0 && errno !=ENOENT ) {
...but skip the unlink of the just-created too-long name. Oops.
util/qemu-sockets.c static int unix_listen_saddr(UnixSocketAddress *saddr, · line 891
Quoted 5 lines
util/qemu-sockets.c +1
-1error_setg_errno (errp, errno, -"Failed to unlink socket %s" , un.sun_path); +"Failed to unlink socket %s" , path); goto err; }
I think you're okay if you just swap the unlink() with the length check.
Although it seems rather sys-call heavy to mkstemp/unlink compared to
just doing a length check first, I do like how your refactoring ended up with fewer conditionals rather than splitting the generated name code across multiple conditionals.
util/qemu-sockets.c static int unix_connect_saddr(UnixSocketAddress *saddr, · line 948
Quoted 8 lines
util/qemu-sockets.c +4
-0@@ -932,9 +942,16 @@ static int unix_connect_saddr(UnixSocketAddress *saddr,qemu_set_nonblock (sock); } +if (strlen (saddr->path) >sizeof (un.sun_path)) { +error_setg (errp,"UNIX socket path '%s' is too long" , saddr->path); +error_append_hint (errp,"Path must be less than %zu bytes\n" , +sizeof (un.sun_path));
Again, potential loss in error message quality.
Hopefully fourth time is the charm!
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Quoted 36 lines · Eric Blake
Quoted 24 lines · Daniel P. Berrange
The 'struct
sockaddr_un' only allows 108 bytes for the socket path.If the user supplies a path, QEMU uses
snprintf()to silently truncate it when too long. This is undesirable because the user will then be unable to connect to the path they asked for.If the user doesn't supply a path, QEMU builds one based on TMPDIR, but if that leads to an overlong path, it mistakenly uses
error_setg_errno()with a stale errno value, becausesnprintf()does not set errno on truncation.In solving this the code needed some refactoring to ensure we don't pass 'un.sun_path' directly to any APIs which expect NUL-terminated strings, because the path is not required to be terminated.
Signed-off-by Daniel P. Berrange <[email protected]>---
} else { const char *tmpdir = getenv("TMPDIR"); tmpdir = tmpdir ? tmpdir : "/tmp";util/qemu-sockets.c static int unix_listen_saddr(UnixSocketAddress *saddr, · line 866
Quoted 5 lines
util/qemu-sockets.c +0
-5-if (snprintf (un.sun_path,sizeof (un.sun_path),"%s/qemu-socket-XXXXXX" , - tmpdir) >=sizeof (un.sun_path)) { -error_setg_errno (errp, errno, -"TMPDIR environment variable (%s) too large" , tmpdir); - goto err;The old code fails early if the generated name is too long,...
Quoted 3 lines
util/qemu-sockets.c +1
-1- } + path = pathbuf =g_strdup_printf ("%s/qemu-socket-XXXXXX" , tmpdir);
Quoted 11 lines
util/qemu-sockets.c static int unix_listen_saddr(UnixSocketAddress *saddr, · line 871
Quoted 8 lines
/*
* This dummy fd usage silences themktemp()unsecure warning.util/qemu-sockets.c +1
-1@@ -873,24 +868,32 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,* to unlink first and thus re-open the race window. The * worst case possible is bind() failing, i.e. a DoS attack. */ - fd =mkstemp (un.sun_path); + fd =mkstemp (pathbuf);...while the new code ends up creating a too-long name in the file-system anyways,...
Opps, yes, that's not good.
Quoted 19 lines
Quoted 8 lines
util/qemu-sockets.c +0
-1if (fd <0 ) {error_setg_errno (errp, errno,"Failed to make a temporary socket name in %s" , tmpdir); goto err; }close (fd); -if (update_addr) {util/qemu-sockets.c static int unix_listen_saddr(UnixSocketAddress *saddr, · line 883
Quoted 10 lines
util/qemu-sockets.c +4
-4-g_free (saddr->path); - saddr->path =g_strdup (un.sun_path); - } } -if (unlink (un.sun_path) <0 && errno !=ENOENT ) { +if (strlen (path) >sizeof (un.sun_path)) { +error_setg (errp,"UNIX socket path '%s' is too long" , path); +error_append_hint (errp,"Path must be less than %zu bytes\n" , +sizeof (un.sun_path));
Quoted 6 lines
The old message mentioned the name that was too long, the new does not. That's a potential slight loss in error message quality, particularly since you are no longer mentioning a UNIX socket (which may give the user a clue why 108 is special), but I can probably live with it (it's still pretty obvious from the message that you should investigate what is too long).
I'm not sure why you are saying I don't mention the path or that it is a UNIX socket - I do exactly that right there.
$ ./x86_64-softmmu/qemu-system-x86_64 -monitor unix:/home/berrange/src/virt/qemu/01234567890123456789012345678901234567890123456789012345678901234567890123456789,server qemu-system-x86_64: -monitor unix:/home/berrange/src/virt/qemu/01234567890123456789012345678901234567890123456789012345678901234567890123456789,server: UNIX socket path '/home/berrange/src/virt/qemu/01234567890123456789012345678901234567890123456789012345678901234567890123456789' is too long Path must be less than 108 bytes
Quoted 3 lines
util/qemu-sockets.c static int unix_listen_saddr(UnixSocketAddress *saddr, · line 887
Quoted 1 line
util/qemu-sockets.c +1
-0+if (unlink (path) <0 && errno !=ENOENT ) {...but skip the unlink of the just-created too-long name. Oops.
yeah will fix that too
Quoted 17 lines
util/qemu-sockets.c static int unix_listen_saddr(UnixSocketAddress *saddr, · line 891
Quoted 6 lines
util/qemu-sockets.c +1
-1error_setg_errno (errp, errno, -"Failed to unlink socket %s" , un.sun_path); +"Failed to unlink socket %s" , path); goto err; }I think you're okay if you just swap the
unlink()with the length check.
Although it seems rather sys-call heavy to mkstemp/unlink compared to
just doing a length check first, I do like how your refactoring ended up with fewer conditionals rather than splitting the generated name code across multiple conditionals.Quoted 4 lines
util/qemu-sockets.c +0
-0@@ -932,9 +942,16 @@ static int unix_connect_saddr(UnixSocketAddress *saddr,qemu_set_nonblock (sock); }
Quoted 6 lines
util/qemu-sockets.c static int unix_connect_saddr(UnixSocketAddress *saddr, · line 948
Quoted 4 lines
util/qemu-sockets.c +4
-0+if (strlen (saddr->path) >sizeof (un.sun_path)) { +error_setg (errp,"UNIX socket path '%s' is too long" , saddr->path); +error_append_hint (errp,"Path must be less than %zu bytes\n" , +sizeof (un.sun_path));Again, potential loss in error message quality.
I don't think so.
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Quoted 35 lines · Daniel P. Berrange
Quoted 27 lines · Eric Blake
Quoted 20 lines · Daniel P. Berrange
The 'struct
sockaddr_un' only allows 108 bytes for the socket path.If the user supplies a path, QEMU uses
snprintf()to silently truncate it when too long. This is undesirable because the user will then be unable to connect to the path they asked for.If the user doesn't supply a path, QEMU builds one based on TMPDIR, but if that leads to an overlong path, it mistakenly uses
error_setg_errno()with a stale errno value, becausesnprintf()does not set errno on truncation.In solving this the code needed some refactoring to ensure we don't pass 'un.sun_path' directly to any APIs which expect NUL-terminated strings, because the path is not required to be terminated.
Signed-off-by Daniel P. Berrange <[email protected]>util/qemu-sockets.c static int unix_listen_saddr(UnixSocketAddress *saddr, · line 883
Quoted 5 lines
util/qemu-sockets.c +4
-1-if (unlink (un.sun_path) <0 && errno !=ENOENT ) { +if (strlen (path) >sizeof (un.sun_path)) { +error_setg (errp,"UNIX socket path '%s' is too long" , path); +error_append_hint (errp,"Path must be less than %zu bytes\n" , +sizeof (un.sun_path));Quoted 6 lines
The old message mentioned the name that was too long, the new does not. That's a potential slight loss in error message quality, particularly since you are no longer mentioning a UNIX socket (which may give the user a clue why 108 is special), but I can probably live with it (it's still pretty obvious from the message that you should investigate what is too long).
Quoted 2 lines
I'm not sure why you are saying I don't mention the path or that it is a UNIX socket - I do exactly that right there.
Oh shoot, I was misreading things. I saw two lines of error_*, and must have thought that the first was the deletion and the second an insertion. But you're right that they are both insertions, that the first is an accurate message, and the second is a hint.
Never mind, I'm obviously not fully awake yet today ;)
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
util/qemu-sockets.c +0
-0@@ -932,9 +942,16 @@ static int unix_connect_saddr(UnixSocketAddress *saddr,qemu_set_nonblock (sock); }
Quoted 8 lines
Quoted 6 lines
util/qemu-sockets.c static int unix_connect_saddr(UnixSocketAddress *saddr, · line 948
Quoted 4 lines
util/qemu-sockets.c +4
-0+if (strlen (saddr->path) >sizeof (un.sun_path)) { +error_setg (errp,"UNIX socket path '%s' is too long" , saddr->path); +error_append_hint (errp,"Path must be less than %zu bytes\n" , +sizeof (un.sun_path));Again, potential loss in error message quality.
I don't think so.
Yep, another instance of me reading the patch wrong. This part's fine, after all.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org