[PATCH 0/6] ui: various improvements to VNC SASL code

[PATCH 0/6] ui: various improvements to VNC SASL code

[Daniel P. Berrangé]

This fixes a few minor edge cases noticed when writing the formal
specification for the SASL VNC extension.

Daniel P. Berrangé (6):
  ui/vnc: don't return an empty SASL mechlist to the client
  ui/vnc: don't raise error formatting socket address for non-inet
  ui/vnc: fix skipping SASL SSF on UNIX sockets
  ui/vnc: don't check for SSF after SASL authentication on UNIX sockets
  ui: fix handling of NULL SASL server data
  ui: validate NUL byte padding in SASL client data more strictly

 ui/vnc-auth-sasl.c | 75 ++++++++++++++++++++++++++++++++--------------
 ui/vnc.c           |  3 --
 ui/vnc.h           |  1 -
 3 files changed, 52 insertions(+), 27 deletions(-)

-- 
2.46.0

[PATCH 1/6] ui/vnc: don't return an empty SASL mechlist to the client

[Daniel P. Berrangé]

The SASL initialization phase may determine that there are no valid
mechanisms available to use. This may be because the host OS admin
forgot to install some packages, or it might be because the requested
SSF level is incompatible with available mechanisms, or other unknown
reasons.

If we return an empty mechlist to the client, they're going to get a
failure from the SASL library on their end and drop the connection.
Thus there is no point even sending this back to the client, we can
just drop the connection immediately.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 ui/vnc-auth-sasl.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
index 47fdae5b21..7d9ca9e8ac 100644
--- a/ui/vnc-auth-sasl.c
+++ b/ui/vnc-auth-sasl.c
@@ -674,6 +674,13 @@ void start_auth_sasl(VncState *vs)
     }
     trace_vnc_auth_sasl_mech_list(vs, mechlist);
 
+    if (g_str_equal(mechlist, "")) {
+        trace_vnc_auth_fail(vs, vs->auth, "no available SASL mechanisms", "");
+        sasl_dispose(&vs->sasl.conn);
+        vs->sasl.conn = NULL;
+        goto authabort;
+    }
+
     vs->sasl.mechlist = g_strdup(mechlist);
     mechlistlen = strlen(mechlist);
     vnc_write_u32(vs, mechlistlen);
-- 
2.46.0

[PATCH 2/6] ui/vnc: don't raise error formatting socket address for non-inet

[Daniel P. Berrangé]

The SASL library requires the connection's local & remote IP address to
be passed in, since some mechanism may use this information. Currently
QEMU raises an error for non-inet sockets, but it is valid to pass NULL
to the SASL library. Doing so makes SASL work on UNIX sockets.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 ui/vnc-auth-sasl.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
index 7d9ca9e8ac..edf19deb3b 100644
--- a/ui/vnc-auth-sasl.c
+++ b/ui/vnc-auth-sasl.c
@@ -524,13 +524,13 @@ static int protocol_client_auth_sasl_mechname_len(VncState *vs, uint8_t *data, s
     return 0;
 }
 
-static char *
+static int
 vnc_socket_ip_addr_string(QIOChannelSocket *ioc,
                           bool local,
+                          char **addrstr,
                           Error **errp)
 {
     SocketAddress *addr;
-    char *ret;
 
     if (local) {
         addr = qio_channel_socket_get_local_address(ioc, errp);
@@ -538,17 +538,17 @@ vnc_socket_ip_addr_string(QIOChannelSocket *ioc,
         addr = qio_channel_socket_get_remote_address(ioc, errp);
     }
     if (!addr) {
-        return NULL;
+        return -1;
     }
 
     if (addr->type != SOCKET_ADDRESS_TYPE_INET) {
-        error_setg(errp, "Not an inet socket type");
+        *addrstr = NULL;
         qapi_free_SocketAddress(addr);
-        return NULL;
+        return 0;
     }
-    ret = g_strdup_printf("%s;%s", addr->u.inet.host, addr->u.inet.port);
+    *addrstr = g_strdup_printf("%s;%s", addr->u.inet.host, addr->u.inet.port);
     qapi_free_SocketAddress(addr);
-    return ret;
+    return 0;
 }
 
 void start_auth_sasl(VncState *vs)
@@ -561,15 +561,15 @@ void start_auth_sasl(VncState *vs)
     int mechlistlen;
 
     /* Get local & remote client addresses in form  IPADDR;PORT */
-    localAddr = vnc_socket_ip_addr_string(vs->sioc, true, &local_err);
-    if (!localAddr) {
+    if (vnc_socket_ip_addr_string(vs->sioc, true,
+                                  &localAddr, &local_err) < 0) {
         trace_vnc_auth_fail(vs, vs->auth, "Cannot format local IP",
                             error_get_pretty(local_err));
         goto authabort;
     }
 
-    remoteAddr = vnc_socket_ip_addr_string(vs->sioc, false, &local_err);
-    if (!remoteAddr) {
+    if (vnc_socket_ip_addr_string(vs->sioc, false,
+                                  &remoteAddr, &local_err) < 0) {
         trace_vnc_auth_fail(vs, vs->auth, "Cannot format remote IP",
                             error_get_pretty(local_err));
         g_free(localAddr);
-- 
2.46.0

[PATCH 3/6] ui/vnc: fix skipping SASL SSF on UNIX sockets

[Daniel P. Berrangé]

The 'is_unix' flag is set on the VNC server during startup, however,
a regression in:

  commit 8bd22f477f68bbd7a9c88e926e7a58bf65605e39
  Author: Daniel P. Berrangé <berrange@redhat.com>
  Date:   Fri Feb 3 12:06:46 2017 +0000

    ui: extract code to connect/listen from vnc_display_open

meant we stopped setting the 'is_unix' flag when QEMU listens for
VNC sockets, only setting when QEMU does a reverse VNC connection.

Rather than fixing setting of the 'is_unix' flag, remove it, and
directly check the live client socket address. This is more robust
to a possible situation where the VNC server was listening on a
mixture of INET and UNIX sockets.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 ui/vnc-auth-sasl.c | 14 +++++++++++---
 ui/vnc.c           |  3 ---
 ui/vnc.h           |  1 -
 3 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
index edf19deb3b..43515447fb 100644
--- a/ui/vnc-auth-sasl.c
+++ b/ui/vnc-auth-sasl.c
@@ -551,6 +551,13 @@ vnc_socket_ip_addr_string(QIOChannelSocket *ioc,
     return 0;
 }
 
+static bool
+vnc_socket_is_unix(QIOChannelSocket *ioc)
+{
+    SocketAddress *addr = qio_channel_socket_get_local_address(ioc, NULL);
+    return addr && addr->type == SOCKET_ADDRESS_TYPE_UNIX;
+}
+
 void start_auth_sasl(VncState *vs)
 {
     const char *mechlist = NULL;
@@ -627,10 +634,11 @@ void start_auth_sasl(VncState *vs)
     memset (&secprops, 0, sizeof secprops);
     /* Inform SASL that we've got an external SSF layer from TLS.
      *
-     * Disable SSF, if using TLS+x509+SASL only. TLS without x509
-     * is not sufficiently strong
+     * Disable SSF, if using TLS+x509+SASL only, or UNIX sockets.
+     * TLS without x509 is not sufficiently strong, nor is plain
+     * TCP
      */
-    if (vs->vd->is_unix ||
+    if (vnc_socket_is_unix(vs->sioc) ||
         (vs->auth == VNC_AUTH_VENCRYPT &&
          vs->subauth == VNC_AUTH_VENCRYPT_X509SASL)) {
         /* If we've got TLS or UNIX domain sock, we don't care about SSF */
diff --git a/ui/vnc.c b/ui/vnc.c
index 93a8dbd253..5fcb35bf25 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3430,7 +3430,6 @@ static void vnc_display_close(VncDisplay *vd)
     if (!vd) {
         return;
     }
-    vd->is_unix = false;
 
     if (vd->listener) {
         qio_net_listener_disconnect(vd->listener);
@@ -3932,8 +3931,6 @@ static int vnc_display_connect(VncDisplay *vd,
         error_setg(errp, "Expected a single address in reverse mode");
         return -1;
     }
-    /* TODO SOCKET_ADDRESS_TYPE_FD when fd has AF_UNIX */
-    vd->is_unix = saddr_list->value->type == SOCKET_ADDRESS_TYPE_UNIX;
     sioc = qio_channel_socket_new();
     qio_channel_set_name(QIO_CHANNEL(sioc), "vnc-reverse");
     if (qio_channel_socket_connect_sync(sioc, saddr_list->value, errp) < 0) {
diff --git a/ui/vnc.h b/ui/vnc.h
index e5fa2efa3e..acc53a2cc1 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -168,7 +168,6 @@ struct VncDisplay
 
     const char *id;
     QTAILQ_ENTRY(VncDisplay) next;
-    bool is_unix;
     char *password;
     time_t expires;
     int auth;
-- 
2.46.0

[PATCH 4/6] ui/vnc: don't check for SSF after SASL authentication on UNIX sockets

[Daniel P. Berrangé]

Although we avoid requesting an SSF when querying SASL mechanisms for a
UNIX socket client, we still mistakenly checked for availability of an
SSF once the SASL auth process is complete.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 ui/vnc-auth-sasl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
index 43515447fb..25f6b4b776 100644
--- a/ui/vnc-auth-sasl.c
+++ b/ui/vnc-auth-sasl.c
@@ -628,7 +628,7 @@ void start_auth_sasl(VncState *vs)
             goto authabort;
         }
     } else {
-        vs->sasl.wantSSF = 1;
+        vs->sasl.wantSSF = !vnc_socket_is_unix(vs->sioc);
     }
 
     memset (&secprops, 0, sizeof secprops);
-- 
2.46.0

[PATCH 5/6] ui: fix handling of NULL SASL server data

[Daniel P. Berrangé]

The code is supposed to distinguish between SASL server data that
is NULL, vs non-NULL but zero-length. It was incorrectly checking
the 'serveroutlen' variable, rather than 'serverout' though, so
failing to distinguish the cases.

Fortunately we can fix this without breaking compatibility with
clients, as clients already know how to decode the input data
correctly.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 ui/vnc-auth-sasl.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
index 25f6b4b776..a04feeb429 100644
--- a/ui/vnc-auth-sasl.c
+++ b/ui/vnc-auth-sasl.c
@@ -289,9 +289,10 @@ static int protocol_client_auth_sasl_step(VncState *vs, uint8_t *data, size_t le
         goto authabort;
     }
 
-    if (serveroutlen) {
+    if (serverout) {
         vnc_write_u32(vs, serveroutlen + 1);
-        vnc_write(vs, serverout, serveroutlen + 1);
+        vnc_write(vs, serverout, serveroutlen);
+        vnc_write_u8(vs, '\0');
     } else {
         vnc_write_u32(vs, 0);
     }
@@ -410,9 +411,10 @@ static int protocol_client_auth_sasl_start(VncState *vs, uint8_t *data, size_t l
         goto authabort;
     }
 
-    if (serveroutlen) {
+    if (serverout) {
         vnc_write_u32(vs, serveroutlen + 1);
-        vnc_write(vs, serverout, serveroutlen + 1);
+        vnc_write(vs, serverout, serveroutlen);
+        vnc_write_u8(vs, '\0');
     } else {
         vnc_write_u32(vs, 0);
     }
-- 
2.46.0

[PATCH 6/6] ui: validate NUL byte padding in SASL client data more strictly

[Daniel P. Berrangé]

When the SASL data is non-NULL, the SASL protocol spec requires that
it is padded with a trailing NUL byte. QEMU discards the trailing
byte, but does not currently validate that it was in fact a NUL.
Apply strict validation to better detect any broken clients.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 ui/vnc-auth-sasl.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
index a04feeb429..df2825797d 100644
--- a/ui/vnc-auth-sasl.c
+++ b/ui/vnc-auth-sasl.c
@@ -263,8 +263,14 @@ static int protocol_client_auth_sasl_step(VncState *vs, uint8_t *data, size_t le
     /* NB, distinction of NULL vs "" is *critical* in SASL */
     if (datalen) {
         clientdata = (char*)data;
-        clientdata[datalen-1] = '\0'; /* Wire includes '\0', but make sure */
-        datalen--; /* Don't count NULL byte when passing to _start() */
+        if (clientdata[datalen-1] != '\0') {
+            trace_vnc_auth_fail(vs, vs->auth, "Malformed SASL client data",
+                                "Missing SASL NUL padding byte");
+            sasl_dispose(&vs->sasl.conn);
+            vs->sasl.conn = NULL;
+            goto authabort;
+        }
+        datalen--; /* Discard the extra NULL byte padding when calling _start() */
     }
 
     err = sasl_server_step(vs->sasl.conn,
@@ -385,8 +391,14 @@ static int protocol_client_auth_sasl_start(VncState *vs, uint8_t *data, size_t l
     /* NB, distinction of NULL vs "" is *critical* in SASL */
     if (datalen) {
         clientdata = (char*)data;
-        clientdata[datalen-1] = '\0'; /* Should be on wire, but make sure */
-        datalen--; /* Don't count NULL byte when passing to _start() */
+        if (clientdata[datalen-1] != '\0') {
+            trace_vnc_auth_fail(vs, vs->auth,  "Malformed SASL client data",
+                                "Missing SASL NUL padding byte");
+            sasl_dispose(&vs->sasl.conn);
+            vs->sasl.conn = NULL;
+            goto authabort;
+        }
+        datalen--; /* Discard the extra NULL byte padding when calling _start() */
     }
 
     err = sasl_server_start(vs->sasl.conn,
-- 
2.46.0

Re: [PATCH 0/6] ui: various improvements to VNC SASL code

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 955 bytes --]

Hi Daniel

On Mon, Oct 21, 2024 at 8:21 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> This fixes a few minor edge cases noticed when writing the formal
> specification for the SASL VNC extension.
>
> Daniel P. Berrangé (6):
>   ui/vnc: don't return an empty SASL mechlist to the client
>   ui/vnc: don't raise error formatting socket address for non-inet
>   ui/vnc: fix skipping SASL SSF on UNIX sockets
>   ui/vnc: don't check for SSF after SASL authentication on UNIX sockets
>   ui: fix handling of NULL SASL server data
>   ui: validate NUL byte padding in SASL client data more strictly
>

For the series:
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


>  ui/vnc-auth-sasl.c | 75 ++++++++++++++++++++++++++++++++--------------
>  ui/vnc.c           |  3 --
>  ui/vnc.h           |  1 -
>  3 files changed, 52 insertions(+), 27 deletions(-)
>
> --
> 2.46.0
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 1665 bytes --]