All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Daniel P. Berrangé" <berrange@redhat.com>
To: qemu-devel@nongnu.org
Cc: "Wainer dos Santos Moschetta" <wainersm@redhat.com>,
	"Daniel P. Berrangé" <berrange@redhat.com>,
	"Marc-André Lureau" <marcandre.lureau@redhat.com>,
	"Philippe Mathieu-Daudé" <philmd@linaro.org>,
	"Thomas Huth" <thuth@redhat.com>,
	"Alex Bennée" <alex.bennee@linaro.org>
Subject: [PULL 13/14] ui: validate NUL byte padding in SASL client data more strictly
Date: Tue, 22 Oct 2024 16:24:14 +0100	[thread overview]
Message-ID: <20241022152415.1632556-14-berrange@redhat.com> (raw)
In-Reply-To: <20241022152415.1632556-1-berrange@redhat.com>

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.

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
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..3f4cfc471d 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 NUL padding byte */
     }
 
     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 NUL padding byte */
     }
 
     err = sasl_server_start(vs->sasl.conn,
-- 
2.46.0



  parent reply	other threads:[~2024-10-22 15:25 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-10-22 15:24 [PULL 00/14] Misc fixes patches Daniel P. Berrangé
2024-10-22 15:24 ` [PULL 01/14] crypto: Remove unused DER string functions Daniel P. Berrangé
2024-10-22 15:24 ` [PULL 02/14] sockets: Remove deadcode Daniel P. Berrangé
2024-10-22 15:24 ` [PULL 03/14] util: don't set SO_REUSEADDR on client sockets Daniel P. Berrangé
2024-10-22 15:24 ` [PULL 04/14] crypto/hash: avoid overwriting user supplied result pointer Daniel P. Berrangé
2024-10-22 15:24 ` [PULL 05/14] tests: correctly validate result buffer in hash/hmac tests Daniel P. Berrangé
2024-10-22 15:24 ` [PULL 06/14] include/crypto: clarify @result/@result_len for hash/hmac APIs Daniel P. Berrangé
2024-10-22 15:24 ` [PULL 07/14] crypto/hash-afalg: Fix broken build Daniel P. Berrangé
2024-10-22 15:24 ` [PULL 08/14] ui/vnc: don't return an empty SASL mechlist to the client Daniel P. Berrangé
2024-10-22 15:24 ` [PULL 09/14] ui/vnc: don't raise error formatting socket address for non-inet Daniel P. Berrangé
2024-10-22 15:24 ` [PULL 10/14] ui/vnc: fix skipping SASL SSF on UNIX sockets Daniel P. Berrangé
2024-10-22 15:24 ` [PULL 11/14] ui/vnc: don't check for SSF after SASL authentication " Daniel P. Berrangé
2024-10-22 15:24 ` [PULL 12/14] ui: fix handling of NULL SASL server data Daniel P. Berrangé
2024-10-22 15:24 ` Daniel P. Berrangé [this message]
2024-10-22 15:24 ` [PULL 14/14] gitlab: enable afalg tests in fedora system test Daniel P. Berrangé
2024-10-24 14:20 ` [PULL 00/14] Misc fixes patches Peter Maydell

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20241022152415.1632556-14-berrange@redhat.com \
    --to=berrange@redhat.com \
    --cc=alex.bennee@linaro.org \
    --cc=marcandre.lureau@redhat.com \
    --cc=philmd@linaro.org \
    --cc=qemu-devel@nongnu.org \
    --cc=thuth@redhat.com \
    --cc=wainersm@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.