Misc sockets, crypto and VNC fixes

* Fix rare EADDRINUSE failures on OpenBSD platforms seen
   with migration
 * Fix & test overwriting of hash output buffer
 * Close connection instead of returning empty SASL mechlist to
   VNC clients
 * Fix handling of SASL SSF on VNC server UNIX sockets
 * Fix handling of NULL SASL server data in VNC server
 * Validate trailing NUL padding byte from SASL client
 * Fix & test AF_ALG crypto backend build
 * Remove unused code in sockets and crypto subsystems
 -----BEGIN PGP SIGNATURE-----
 
 iQIzBAABCAAdFiEE2vOm/bJrYpEtDo4/vobrtBUQT98FAmcXscUACgkQvobrtBUQ
 T9+S+Q//W9fywFY42VnsPqIAi7Q+QPDvXrPVVQ1z817hcyxdMVWC+eAg97i3QsE8
 f/+nwrigV9CIv9jqdBdMUIRLm4XhyuDspksgBAQUJ1XYmmVSmFwh2ej31m/qI8fK
 fu0v6N6udkcg+5eoWEOL873hKAA+vjq30tM5Zp74fMHZahnvgjThgaJY6Z6OsCyX
 6Pgxl3Z1gym1IqQFz0nOdTMnzsQrAJbV8z2FWMKgHayg01nVoXlo5FMnNgIdItJC
 v+4qX5sfRJIENJcRKMNY4dQUqbO1004+HXECLbge8pR7vsUli06xjLBkSbt/9M6r
 x3lfDGKavPrKfsPk1H+eTlge/43IjJk+mXMgZxmyvrvgnyVulxRvz7ABKJ+VBUeq
 CDrAuAK4tm5BIxKu6cg4CcmlqsDXwq6Sb+NdsbxTv0Deop73WZR3HCamRNU1JXkA
 eXBY4QSuVA96s5TnlfZWZytIY9NmyjN48ov+ly2fOkbv/xxoUNFBY8TApSJZ/Veo
 4EvGlIfgxjv668n/2eyt67E00dGC3idTbaWYeGjgUKVyNPpxicDOnM3NTwMP3/0k
 DZbvfoJcwfhPVoFMdV7ZvJKA3i8v11HdaEI0urfjm5nJWbyik6+++skan9F/femL
 eRTnH2hr5sUV+eQAl2YhGuBElLmKf/HqTCeNs3lwrUQsnb9bPNc=
 =fK8K
 -----END PGP SIGNATURE-----

Merge tag 'misc-fixes-pull-request' of https://gitlab.com/berrange/qemu into staging

Misc sockets, crypto and VNC fixes

* Fix rare EADDRINUSE failures on OpenBSD platforms seen
  with migration
* Fix & test overwriting of hash output buffer
* Close connection instead of returning empty SASL mechlist to
  VNC clients
* Fix handling of SASL SSF on VNC server UNIX sockets
* Fix handling of NULL SASL server data in VNC server
* Validate trailing NUL padding byte from SASL client
* Fix & test AF_ALG crypto backend build
* Remove unused code in sockets and crypto subsystems

# -----BEGIN PGP SIGNATURE-----
#
# iQIzBAABCAAdFiEE2vOm/bJrYpEtDo4/vobrtBUQT98FAmcXscUACgkQvobrtBUQ
# T9+S+Q//W9fywFY42VnsPqIAi7Q+QPDvXrPVVQ1z817hcyxdMVWC+eAg97i3QsE8
# f/+nwrigV9CIv9jqdBdMUIRLm4XhyuDspksgBAQUJ1XYmmVSmFwh2ej31m/qI8fK
# fu0v6N6udkcg+5eoWEOL873hKAA+vjq30tM5Zp74fMHZahnvgjThgaJY6Z6OsCyX
# 6Pgxl3Z1gym1IqQFz0nOdTMnzsQrAJbV8z2FWMKgHayg01nVoXlo5FMnNgIdItJC
# v+4qX5sfRJIENJcRKMNY4dQUqbO1004+HXECLbge8pR7vsUli06xjLBkSbt/9M6r
# x3lfDGKavPrKfsPk1H+eTlge/43IjJk+mXMgZxmyvrvgnyVulxRvz7ABKJ+VBUeq
# CDrAuAK4tm5BIxKu6cg4CcmlqsDXwq6Sb+NdsbxTv0Deop73WZR3HCamRNU1JXkA
# eXBY4QSuVA96s5TnlfZWZytIY9NmyjN48ov+ly2fOkbv/xxoUNFBY8TApSJZ/Veo
# 4EvGlIfgxjv668n/2eyt67E00dGC3idTbaWYeGjgUKVyNPpxicDOnM3NTwMP3/0k
# DZbvfoJcwfhPVoFMdV7ZvJKA3i8v11HdaEI0urfjm5nJWbyik6+++skan9F/femL
# eRTnH2hr5sUV+eQAl2YhGuBElLmKf/HqTCeNs3lwrUQsnb9bPNc=
# =fK8K
# -----END PGP SIGNATURE-----
# gpg: Signature made Tue 22 Oct 2024 15:08:05 BST
# gpg:                using RSA key DAF3A6FDB26B62912D0E8E3FBE86EBB415104FDF
# gpg: Good signature from "Daniel P. Berrange <dan@berrange.com>" [full]
# gpg:                 aka "Daniel P. Berrange <berrange@redhat.com>" [full]
# Primary key fingerprint: DAF3 A6FD B26B 6291 2D0E  8E3F BE86 EBB4 1510 4FDF

* tag 'misc-fixes-pull-request' of https://gitlab.com/berrange/qemu:
  gitlab: enable afalg tests in fedora system test
  ui: validate NUL byte padding in SASL client data more strictly
  ui: fix handling of NULL SASL server data
  ui/vnc: don't check for SSF after SASL authentication on UNIX sockets
  ui/vnc: fix skipping SASL SSF on UNIX sockets
  ui/vnc: don't raise error formatting socket address for non-inet
  ui/vnc: don't return an empty SASL mechlist to the client
  crypto/hash-afalg: Fix broken build
  include/crypto: clarify @result/@result_len for hash/hmac APIs
  tests: correctly validate result buffer in hash/hmac tests
  crypto/hash: avoid overwriting user supplied result pointer
  util: don't set SO_REUSEADDR on client sockets
  sockets: Remove deadcode
  crypto: Remove unused DER string functions

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

.gitlab-ci.d/buildtest.yml

@@ -115,7 +115,7 @@ build-system-fedora:
     job: amd64-fedora-container
   variables:
     IMAGE: fedora
-    CONFIGURE_ARGS: --disable-gcrypt --enable-nettle --enable-docs
+    CONFIGURE_ARGS: --disable-gcrypt --enable-nettle --enable-docs --enable-crypto-afalg
     TARGETS: microblaze-softmmu mips-softmmu
       xtensa-softmmu m68k-softmmu riscv32-softmmu ppc-softmmu sparc64-softmmu
     MAKE_CHECK_ARGS: check-build

crypto/der.c

@@ -408,19 +408,6 @@ void qcrypto_der_encode_octet_str(QCryptoEncodeContext *ctx,
     qcrypto_der_encode_prim(ctx, tag, src, src_len);
 }
 
-void qcrypto_der_encode_octet_str_begin(QCryptoEncodeContext *ctx)
-{
-    uint8_t tag = QCRYPTO_DER_TAG(QCRYPTO_DER_TAG_CLASS_UNIV,
-                                  QCRYPTO_DER_TAG_ENC_PRIM,
-                                  QCRYPTO_DER_TYPE_TAG_OCT_STR);
-    qcrypto_der_encode_cons_begin(ctx, tag);
-}
-
-void qcrypto_der_encode_octet_str_end(QCryptoEncodeContext *ctx)
-{
-    qcrypto_der_encode_cons_end(ctx);
-}
-
 size_t qcrypto_der_encode_ctx_buffer_len(QCryptoEncodeContext *ctx)
 {
     return ctx->root.dlen;

crypto/der.h

@@ -242,28 +242,6 @@ void qcrypto_der_encode_null(QCryptoEncodeContext *ctx);
 void qcrypto_der_encode_octet_str(QCryptoEncodeContext *ctx,
                                   const uint8_t *src, size_t src_len);
 
-/**
- * qcrypto_der_encode_octet_str_begin:
- * @ctx: the encode context.
- *
- * Start encoding a octet string, All fields between
- * qcrypto_der_encode_octet_str_begin and qcrypto_der_encode_octet_str_end
- * are encoded as an octet string. This is useful when we need to encode a
- * encoded SEQUENCE as OCTET STRING.
- */
-void qcrypto_der_encode_octet_str_begin(QCryptoEncodeContext *ctx);
-
-/**
- * qcrypto_der_encode_octet_str_end:
- * @ctx: the encode context.
- *
- * Finish encoding a octet string, All fields between
- * qcrypto_der_encode_octet_str_begin and qcrypto_der_encode_octet_str_end
- * are encoded as an octet string. This is useful when we need to encode a
- * encoded SEQUENCE as OCTET STRING.
- */
-void qcrypto_der_encode_octet_str_end(QCryptoEncodeContext *ctx);
-
 /**
  * qcrypto_der_encode_ctx_buffer_len:
  * @ctx: the encode context.

crypto/hash-afalg.c

@@ -142,7 +142,7 @@ QCryptoHash *qcrypto_afalg_hash_new(QCryptoHashAlgo alg, Error **errp)
 static
 void qcrypto_afalg_hash_free(QCryptoHash *hash)
 {
-    QCryptoAFAlg *ctx = hash->opaque;
+    QCryptoAFAlgo *ctx = hash->opaque;
 
     if (ctx) {
         qcrypto_afalg_comm_free(ctx);
@@ -159,7 +159,7 @@ void qcrypto_afalg_hash_free(QCryptoHash *hash)
  * be provided to calculate the final hash.
  */
 static
-int qcrypto_afalg_send_to_kernel(QCryptoAFAlg *afalg,
+int qcrypto_afalg_send_to_kernel(QCryptoAFAlgo *afalg,
                                  const struct iovec *iov,
                                  size_t niov,
                                  bool more_data,
@@ -183,7 +183,7 @@ int qcrypto_afalg_send_to_kernel(QCryptoAFAlg *afalg,
 }
 
 static
-int qcrypto_afalg_recv_from_kernel(QCryptoAFAlg *afalg,
+int qcrypto_afalg_recv_from_kernel(QCryptoAFAlgo *afalg,
                                    QCryptoHashAlgo alg,
                                    uint8_t **result,
                                    size_t *result_len,
@@ -222,7 +222,7 @@ int qcrypto_afalg_hash_update(QCryptoHash *hash,
                               size_t niov,
                               Error **errp)
 {
-    return qcrypto_afalg_send_to_kernel((QCryptoAFAlg *) hash->opaque,
+    return qcrypto_afalg_send_to_kernel((QCryptoAFAlgo *) hash->opaque,
                                         iov, niov, true, errp);
 }
 
@@ -232,7 +232,7 @@ int qcrypto_afalg_hash_finalize(QCryptoHash *hash,
                                  size_t *result_len,
                                  Error **errp)
 {
-    return qcrypto_afalg_recv_from_kernel((QCryptoAFAlg *) hash->opaque,
+    return qcrypto_afalg_recv_from_kernel((QCryptoAFAlgo *) hash->opaque,
                                           hash->alg, result, result_len, errp);
 }
 

crypto/hash-gcrypt.c

@@ -103,16 +103,25 @@ int qcrypto_gcrypt_hash_finalize(QCryptoHash *hash,
                                  size_t *result_len,
                                  Error **errp)
 {
+    int ret;
     unsigned char *digest;
     gcry_md_hd_t *ctx = hash->opaque;
 
-    *result_len = gcry_md_get_algo_dlen(qcrypto_hash_alg_map[hash->alg]);
+    ret = gcry_md_get_algo_dlen(qcrypto_hash_alg_map[hash->alg]);
-    if (*result_len == 0) {
+    if (ret == 0) {
         error_setg(errp, "Unable to get hash length");
         return -1;
     }
 
-    *result = g_new(uint8_t, *result_len);
+    if (*result_len == 0) {
+        *result_len = ret;
+        *result = g_new(uint8_t, *result_len);
+    } else if (*result_len != ret) {
+        error_setg(errp,
+                   "Result buffer size %zu is smaller than hash %d",
+                   *result_len, ret);
+        return -1;
+    }
 
     /* Digest is freed by gcry_md_close(), copy it */
     digest = gcry_md_read(*ctx, 0);

crypto/hash-glib.c

@@ -99,8 +99,15 @@ int qcrypto_glib_hash_finalize(QCryptoHash *hash,
         return -1;
     }
 
-    *result_len = ret;
+    if (*result_len == 0) {
-    *result = g_new(uint8_t, *result_len);
+        *result_len = ret;
+        *result = g_new(uint8_t, *result_len);
+    } else if (*result_len != ret) {
+        error_setg(errp,
+                   "Result buffer size %zu is smaller than hash %d",
+                   *result_len, ret);
+        return -1;
+    }
 
     g_checksum_get_digest(ctx, *result, result_len);
     return 0;

crypto/hash-gnutls.c

@@ -115,14 +115,24 @@ int qcrypto_gnutls_hash_finalize(QCryptoHash *hash,
                                  Error **errp)
 {
     gnutls_hash_hd_t *ctx = hash->opaque;
+    int ret;
 
-    *result_len = gnutls_hash_get_len(qcrypto_hash_alg_map[hash->alg]);
+    ret = gnutls_hash_get_len(qcrypto_hash_alg_map[hash->alg]);
-    if (*result_len == 0) {
+    if (ret == 0) {
         error_setg(errp, "Unable to get hash length");
         return -1;
     }
 
-    *result = g_new(uint8_t, *result_len);
+    if (*result_len == 0) {
+        *result_len = ret;
+        *result = g_new(uint8_t, *result_len);
+    } else if (*result_len != ret) {
+        error_setg(errp,
+                   "Result buffer size %zu is smaller than hash %d",
+                   *result_len, ret);
+        return -1;
+    }
+
     gnutls_hash_output(*ctx, *result);
     return 0;
 }

crypto/hash-nettle.c

@@ -150,9 +150,17 @@ int qcrypto_nettle_hash_finalize(QCryptoHash *hash,
                                  Error **errp)
 {
     union qcrypto_hash_ctx *ctx = hash->opaque;
+    int ret = qcrypto_hash_alg_map[hash->alg].len;
 
-    *result_len = qcrypto_hash_alg_map[hash->alg].len;
+    if (*result_len == 0) {
-    *result = g_new(uint8_t, *result_len);
+        *result_len = ret;
+        *result = g_new(uint8_t, *result_len);
+    } else if (*result_len != ret) {
+        error_setg(errp,
+                   "Result buffer size %zu is smaller than hash %d",
+                   *result_len, ret);
+        return -1;
+    }
 
     qcrypto_hash_alg_map[hash->alg].result(ctx, *result_len, *result);
 

include/crypto/hash.h

@@ -73,11 +73,18 @@ size_t qcrypto_hash_digest_len(QCryptoHashAlgo alg);
  * @errp: pointer to a NULL-initialized error object
  *
  * Computes the hash across all the memory regions
- * present in @iov. The @result pointer will be
+ * present in @iov.
- * filled with raw bytes representing the computed
+ *
- * hash, which will have length @resultlen. The
+ * If @result_len is set to a non-zero value by the caller, then
- * memory pointer in @result must be released
+ * @result must hold a pointer that is @result_len in size, and
- * with a call to g_free() when no longer required.
+ * @result_len match the size of the hash output. The digest will
+ * be written into @result.
+ *
+ * If @result_len is set to zero, then this function will allocate
+ * a buffer to hold the hash output digest, storing a pointer to
+ * the buffer in @result, and setting @result_len to its size.
+ * The memory referenced in @result must be released with a call
+ * to g_free() when no longer required by the caller.
  *
  * Returns: 0 on success, -1 on error
  */
@@ -98,11 +105,18 @@ int qcrypto_hash_bytesv(QCryptoHashAlgo alg,
  * @errp: pointer to a NULL-initialized error object
  *
  * Computes the hash across all the memory region
- * @buf of length @len. The @result pointer will be
+ * @buf of length @len.
- * filled with raw bytes representing the computed
+ *
- * hash, which will have length @resultlen. The
+ * If @result_len is set to a non-zero value by the caller, then
- * memory pointer in @result must be released
+ * @result must hold a pointer that is @result_len in size, and
- * with a call to g_free() when no longer required.
+ * @result_len match the size of the hash output. The digest will
+ * be written into @result.
+ *
+ * If @result_len is set to zero, then this function will allocate
+ * a buffer to hold the hash output digest, storing a pointer to
+ * the buffer in @result, and setting @result_len to its size.
+ * The memory referenced in @result must be released with a call
+ * to g_free() when no longer required by the caller.
  *
  * Returns: 0 on success, -1 on error
  */
@@ -215,8 +229,17 @@ int qcrypto_hash_finalize_base64(QCryptoHash *hash,
  *
  * Computes the hash from the given hash object. Hash object
  * is expected to have it's data updated from the qcrypto_hash_update function.
- * The memory pointer in @result must be released with a call to g_free()
+ *
- * when no longer required.
+ * If @result_len is set to a non-zero value by the caller, then
+ * @result must hold a pointer that is @result_len in size, and
+ * @result_len match the size of the hash output. The digest will
+ * be written into @result.
+ *
+ * If @result_len is set to zero, then this function will allocate
+ * a buffer to hold the hash output digest, storing a pointer to
+ * the buffer in @result, and setting @result_len to its size.
+ * The memory referenced in @result must be released with a call
+ * to g_free() when no longer required by the caller.
  *
  * Returns: 0 on success, -1 on error
  */

include/crypto/hmac.h

@@ -77,11 +77,18 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(QCryptoHmac, qcrypto_hmac_free)
  * @errp: pointer to a NULL-initialized error object
  *
  * Computes the hmac across all the memory regions
- * present in @iov. The @result pointer will be
+ * present in @iov.
- * filled with raw bytes representing the computed
+ *
- * hmac, which will have length @resultlen. The
+ * If @result_len is set to a non-zero value by the caller, then
- * memory pointer in @result must be released
+ * @result must hold a pointer that is @result_len in size, and
- * with a call to g_free() when no longer required.
+ * @result_len match the size of the hash output. The digest will
+ * be written into @result.
+ *
+ * If @result_len is set to zero, then this function will allocate
+ * a buffer to hold the hash output digest, storing a pointer to
+ * the buffer in @result, and setting @result_len to its size.
+ * The memory referenced in @result must be released with a call
+ * to g_free() when no longer required by the caller.
  *
  * Returns:
  *  0 on success, -1 on error
@@ -103,11 +110,18 @@ int qcrypto_hmac_bytesv(QCryptoHmac *hmac,
  * @errp: pointer to a NULL-initialized error object
  *
  * Computes the hmac across all the memory region
- * @buf of length @len. The @result pointer will be
+ * @buf of length @len.
- * filled with raw bytes representing the computed
+ *
- * hmac, which will have length @resultlen. The
+ * If @result_len is set to a non-zero value by the caller, then
- * memory pointer in @result must be released
+ * @result must hold a pointer that is @result_len in size, and
- * with a call to g_free() when no longer required.
+ * @result_len match the size of the hash output. The digest will
+ * be written into @result.
+ *
+ * If @result_len is set to zero, then this function will allocate
+ * a buffer to hold the hash output digest, storing a pointer to
+ * the buffer in @result, and setting @result_len to its size.
+ * The memory referenced in @result must be released with a call
+ * to g_free() when no longer required by the caller.
  *
  * Returns:
  *  0 on success, -1 on error

include/qemu/sockets.h

@@ -61,7 +61,6 @@ int socket_set_fast_reuse(int fd);
 int inet_ai_family_from_address(InetSocketAddress *addr,
                                 Error **errp);
 int inet_parse(InetSocketAddress *addr, const char *str, Error **errp);
-int inet_connect(const char *str, Error **errp);
 int inet_connect_saddr(InetSocketAddress *saddr, Error **errp);
 
 NetworkAddressFamily inet_netfamily(int family);
@@ -117,21 +116,6 @@ socket_sockaddr_to_address(struct sockaddr_storage *sa,
  */
 SocketAddress *socket_local_address(int fd, Error **errp);
 
-/**
- * socket_remote_address:
- * @fd: the socket file handle
- * @errp: pointer to uninitialized error object
- *
- * Get the string representation of the remote socket
- * address. A pointer to the allocated address information
- * struct will be returned, which the caller is required to
- * release with a call qapi_free_SocketAddress() when no
- * longer required.
- *
- * Returns: the socket address struct, or NULL on error
- */
-SocketAddress *socket_remote_address(int fd, Error **errp);
-
 /**
  * socket_address_flatten:
  * @addr: the socket address to flatten

tests/unit/test-crypto-hash.c

@@ -123,7 +123,7 @@ static void test_hash_prealloc(void)
     size_t i;
 
     for (i = 0; i < G_N_ELEMENTS(expected_outputs) ; i++) {
-        uint8_t *result;
+        uint8_t *result, *origresult;
         size_t resultlen;
         int ret;
         size_t j;
@@ -133,7 +133,7 @@ static void test_hash_prealloc(void)
         }
 
         resultlen = expected_lens[i];
-        result = g_new0(uint8_t, resultlen);
+        origresult = result = g_new0(uint8_t, resultlen);
 
         ret = qcrypto_hash_bytes(i,
                                  INPUT_TEXT,
@@ -142,7 +142,8 @@ static void test_hash_prealloc(void)
                                  &resultlen,
                                  &error_fatal);
         g_assert(ret == 0);
-
+        /* Validate that our pre-allocated pointer was not replaced */
+        g_assert(result == origresult);
         g_assert(resultlen == expected_lens[i]);
         for (j = 0; j < resultlen; j++) {
             g_assert(expected_outputs[i][j * 2] == hex[(result[j] >> 4) & 0xf]);

tests/unit/test-crypto-hmac.c

@@ -126,7 +126,7 @@ static void test_hmac_prealloc(void)
     for (i = 0; i < G_N_ELEMENTS(test_data); i++) {
         QCryptoHmacTestData *data = &test_data[i];
         QCryptoHmac *hmac = NULL;
-        uint8_t *result = NULL;
+        uint8_t *result = NULL, *origresult = NULL;
         size_t resultlen = 0;
         const char *exp_output = NULL;
         int ret;
@@ -139,7 +139,7 @@ static void test_hmac_prealloc(void)
         exp_output = data->hex_digest;
 
         resultlen = strlen(exp_output) / 2;
-        result = g_new0(uint8_t, resultlen);
+        origresult = result = g_new0(uint8_t, resultlen);
 
         hmac = qcrypto_hmac_new(data->alg, (const uint8_t *)KEY,
                                 strlen(KEY), &error_fatal);
@@ -149,6 +149,8 @@ static void test_hmac_prealloc(void)
                                  strlen(INPUT_TEXT), &result,
                                  &resultlen, &error_fatal);
         g_assert(ret == 0);
+        /* Validate that our pre-allocated pointer was not replaced */
+        g_assert(result == origresult);
 
         exp_output = data->hex_digest;
         for (j = 0; j < resultlen; j++) {

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 */
+        if (clientdata[datalen - 1] != '\0') {
-        datalen--; /* Don't count NULL byte when passing to _start() */
+            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,
@@ -289,9 +295,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);
     }
@@ -384,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 */
+        if (clientdata[datalen - 1] != '\0') {
-        datalen--; /* Don't count NULL byte when passing to _start() */
+            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,
@@ -410,9 +423,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);
     }
@@ -524,13 +538,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 +552,24 @@ 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;
+}
+
+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)
@@ -561,15 +582,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 (vnc_socket_ip_addr_string(vs->sioc, true,
-    if (!localAddr) {
+                                  &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 (vnc_socket_ip_addr_string(vs->sioc, false,
-    if (!remoteAddr) {
+                                  &remoteAddr, &local_err) < 0) {
         trace_vnc_auth_fail(vs, vs->auth, "Cannot format remote IP",
                             error_get_pretty(local_err));
         g_free(localAddr);
@@ -621,16 +642,17 @@ 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);
     /* Inform SASL that we've got an external SSF layer from TLS.
      *
-     * Disable SSF, if using TLS+x509+SASL only. TLS without x509
+     * Disable SSF, if using TLS+x509+SASL only, or UNIX sockets.
-     * is not sufficiently strong
+     * 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 */
@@ -674,6 +696,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);

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) {

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;

util/qemu-sockets.c

@@ -367,7 +367,6 @@ static int inet_connect_addr(const InetSocketAddress *saddr,
                          addr->ai_family);
         return -1;
     }
-    socket_set_fast_reuse(sock);
 
     /* connect to peer */
     do {
@@ -707,26 +706,6 @@ int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
 }
 
 
-/**
- * Create a blocking socket and connect it to an address.
- *
- * @str: address string
- * @errp: set in case of an error
- *
- * Returns -1 in case of error, file descriptor on success
- **/
-int inet_connect(const char *str, Error **errp)
-{
-    int sock = -1;
-    InetSocketAddress *addr = g_new(InetSocketAddress, 1);
-
-    if (!inet_parse(addr, str, errp)) {
-        sock = inet_connect_saddr(addr, errp);
-    }
-    qapi_free_InetSocketAddress(addr);
-    return sock;
-}
-
 #ifdef CONFIG_AF_VSOCK
 static bool vsock_parse_vaddr_to_sockaddr(const VsockSocketAddress *vaddr,
                                           struct sockaddr_vm *svm,
@@ -1421,21 +1400,6 @@ SocketAddress *socket_local_address(int fd, Error **errp)
 }
 
 
-SocketAddress *socket_remote_address(int fd, Error **errp)
-{
-    struct sockaddr_storage ss;
-    socklen_t sslen = sizeof(ss);
-
-    if (getpeername(fd, (struct sockaddr *)&ss, &sslen) < 0) {
-        error_setg_errno(errp, errno, "%s",
-                         "Unable to query remote socket address");
-        return NULL;
-    }
-
-    return socket_sockaddr_to_address(&ss, sslen, errp);
-}
-
-
 SocketAddress *socket_address_flatten(SocketAddressLegacy *addr_legacy)
 {
     SocketAddress *addr;