[PULL 00/14] Misc fixes patches

[PULL 00/14] Misc fixes patches

[Daniel P. Berrangé]

The following changes since commit cc5adbbd50d81555b8eb73602ec16fde40b55be4:

  Merge tag 'pull-tpm-2024-10-18-1' of https://github.com/stefanberger/qemu-tpm into staging (2024-10-18 15:45:02 +0100)

are available in the Git repository at:

  https://gitlab.com/berrange/qemu tags/misc-fixes-pull-request

for you to fetch changes up to c64df333f92798823c4897ae6d4bd7f49d060225:

  gitlab: enable afalg tests in fedora system test (2024-10-22 13:02:33 +0100)

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

----------------------------------------------------------------

Daniel P. Berrangé (11):
  util: don't set SO_REUSEADDR on client sockets
  crypto/hash: avoid overwriting user supplied result pointer
  tests: correctly validate result buffer in hash/hmac tests
  include/crypto: clarify @result/@result_len for hash/hmac APIs
  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
  gitlab: enable afalg tests in fedora system test

Dr. David Alan Gilbert (2):
  crypto: Remove unused DER string functions
  sockets: Remove deadcode

Markus Armbruster (1):
  crypto/hash-afalg: Fix broken build

 .gitlab-ci.d/buildtest.yml    |  2 +-
 crypto/der.c                  | 13 ------
 crypto/der.h                  | 22 ----------
 crypto/hash-afalg.c           | 10 ++---
 crypto/hash-gcrypt.c          | 15 +++++--
 crypto/hash-glib.c            | 11 ++++-
 crypto/hash-gnutls.c          | 16 ++++++--
 crypto/hash-nettle.c          | 14 +++++--
 include/crypto/hash.h         | 47 ++++++++++++++++------
 include/crypto/hmac.h         | 34 +++++++++++-----
 include/qemu/sockets.h        | 16 --------
 tests/unit/test-crypto-hash.c |  7 ++--
 tests/unit/test-crypto-hmac.c |  6 ++-
 ui/vnc-auth-sasl.c            | 75 ++++++++++++++++++++++++-----------
 ui/vnc.c                      |  3 --
 ui/vnc.h                      |  1 -
 util/qemu-sockets.c           | 36 -----------------
 17 files changed, 170 insertions(+), 158 deletions(-)

-- 
2.46.0

[PULL 01/14] crypto: Remove unused DER string functions

[Daniel P. Berrangé]

From: "Dr. David Alan Gilbert" <dave@treblig.org>

qcrypto_der_encode_octet_str_begin and _end have been unused
since they were added in
  3b34ccad66 ("crypto: Support DER encodings")

Remove them.

Signed-off-by: Dr. David Alan Gilbert <dave@treblig.org>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/der.c | 13 -------------
 crypto/der.h | 22 ----------------------
 2 files changed, 35 deletions(-)

diff --git a/crypto/der.c b/crypto/der.c
index ebbecfc3fe..81367524c3 100644
--- a/crypto/der.c
+++ b/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;
diff --git a/crypto/der.h b/crypto/der.h
index f4ba6da28a..bcfa4a2495 100644
--- a/crypto/der.h
+++ b/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.
-- 
2.46.0

[PULL 02/14] sockets: Remove deadcode

[Daniel P. Berrangé]

From: "Dr. David Alan Gilbert" <dave@treblig.org>

socket_remote_address hasn't been used since it was added in
  17c55decec ("sockets: add helpers for creating SocketAddress from a socket")

inet_connect hasn't been used since 2017's
  8ecc2f9eab ("sheepdog: Use SocketAddress and socket_connect()")

Remove them.

Signed-off-by: Dr. David Alan Gilbert <dave@treblig.org>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/qemu/sockets.h | 16 ----------------
 util/qemu-sockets.c    | 35 -----------------------------------
 2 files changed, 51 deletions(-)

diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index d935fd80da..c562690d89 100644
--- a/include/qemu/sockets.h
+++ b/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
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 60c44b2b56..c1b162b056 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -707,26 +707,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 +1401,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;
-- 
2.46.0

[PULL 03/14] util: don't set SO_REUSEADDR on client sockets

[Daniel P. Berrangé]

Setting the SO_REUSEADDR property on a socket allows binding to a port
number that is in the TIMED_WAIT state. This is usually done on listener
sockets, to enable a server to restart itself without having to wait for
the completion of TIMED_WAIT on the port.

It is also possible, but highly unusual, to set it on client sockets. It
is rare to explicitly bind() a client socket, since it is almost always
fine to allow the kernel to auto-bind a client socket to a random free
port. Most systems will have many 10's of 1000's of free ports that
client sockets will be bound to.

eg on Linux

  $ sysctl -a | grep local_port
  net.ipv4.ip_local_port_range = 32768	60999

eg on OpenBSD

  $ sysctl -a | grep net.inet.ip.port
  net.inet.ip.portfirst=1024
  net.inet.ip.portlast=49151
  net.inet.ip.porthifirst=49152
  net.inet.ip.porthilast=65535

A connected socket must have a unique set of value for

 (protocol, localip, localport, remoteip, remoteport)

otherwise it is liable to get EADDRINUSE.

A client connection should trivially avoid EADDRINUSE if letting the
kernel auto-assign the 'localport' value, which QEMU always does.

When QEMU sets SO_REUSEADDR on a client socket on OpenBSD, however, it
upsets this situation.

The OpenBSD kernel appears to happily pick a 'localport' that is in the
TIMED_WAIT state, even if there are many other available local ports
available for use that are not in the TIMED_WAIT state.

A test program that just loops opening client sockets will start seeing
EADDRINUSE on OpenBSD when as few as 2000 ports are in TIMED_WAIT,
despite 10's of 1000's ports still being unused. This contrasts with
Linux which appears to avoid picking local ports in TIMED_WAIT state.

This problem on OpenBSD exhibits itself periodically with the migration
test failing with a message like[1]:

  qemu-system-ppc64: Failed to connect to '127.0.0.1:24109': Address already in use

While I have not been able to reproduce the OpenBSD failure in my own
testing, given the scope of what QEMU tests do, it is entirely possible
that there could be a lot of ports in TIMED_WAIT state when the
migration test runs.

Removing SO_REUSEADDR from the client sockets should not affect normal
QEMU usage, and should improve reliability on OpenBSD.

This use of SO_REUSEADDR on client sockets is highly unusual, and
appears to have been present since the very start of the QEMU socket
helpers in 2008. The orignal commit has no comment about the use of
SO_REUSEADDR on the client, so is most likely just an 16 year old
copy+paste bug.

[1] https://lists.nongnu.org/archive/html/qemu-devel/2024-10/msg03427.html
    https://lists.nongnu.org/archive/html/qemu-devel/2024-02/msg01572.html

Fixes: d247d25f18764402899b37c381bb696a79000b4e
Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 util/qemu-sockets.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index c1b162b056..77477c1cd5 100644
--- a/util/qemu-sockets.c
+++ b/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 {
-- 
2.46.0

[PULL 04/14] crypto/hash: avoid overwriting user supplied result pointer

[Daniel P. Berrangé]

If the user provides a pre-allocated buffer for the hash result,
we must use that rather than re-allocating a new buffer.

Reported-by: Dorjoy Chowdhury <dorjoychy111@gmail.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/hash-gcrypt.c | 15 ++++++++++++---
 crypto/hash-glib.c   | 11 +++++++++--
 crypto/hash-gnutls.c | 16 +++++++++++++---
 crypto/hash-nettle.c | 14 +++++++++++---
 4 files changed, 45 insertions(+), 11 deletions(-)

diff --git a/crypto/hash-gcrypt.c b/crypto/hash-gcrypt.c
index ccc3cce3f8..73533a4949 100644
--- a/crypto/hash-gcrypt.c
+++ b/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]);
-    if (*result_len == 0) {
+    ret = gcry_md_get_algo_dlen(qcrypto_hash_alg_map[hash->alg]);
+    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);
diff --git a/crypto/hash-glib.c b/crypto/hash-glib.c
index 02a6ec1edf..809cef98ae 100644
--- a/crypto/hash-glib.c
+++ b/crypto/hash-glib.c
@@ -99,8 +99,15 @@ int qcrypto_glib_hash_finalize(QCryptoHash *hash,
         return -1;
     }
 
-    *result_len = ret;
-    *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;
+    }
 
     g_checksum_get_digest(ctx, *result, result_len);
     return 0;
diff --git a/crypto/hash-gnutls.c b/crypto/hash-gnutls.c
index 34a63994c9..99fbe824ea 100644
--- a/crypto/hash-gnutls.c
+++ b/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]);
-    if (*result_len == 0) {
+    ret = gnutls_hash_get_len(qcrypto_hash_alg_map[hash->alg]);
+    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;
 }
diff --git a/crypto/hash-nettle.c b/crypto/hash-nettle.c
index 3b847aa60e..c78624b347 100644
--- a/crypto/hash-nettle.c
+++ b/crypto/hash-nettle.c
@@ -150,9 +150,17 @@ int qcrypto_nettle_hash_finalize(QCryptoHash *hash,
                                  Error **errp)
 {
     union qcrypto_hash_ctx *ctx = hash->opaque;
-
-    *result_len = qcrypto_hash_alg_map[hash->alg].len;
-    *result = g_new(uint8_t, *result_len);
+    int ret = qcrypto_hash_alg_map[hash->alg].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;
+    }
 
     qcrypto_hash_alg_map[hash->alg].result(ctx, *result_len, *result);
 
-- 
2.46.0

[PULL 05/14] tests: correctly validate result buffer in hash/hmac tests

[Daniel P. Berrangé]

Validate that the pre-allocated buffer pointer was not overwritten
by the hash/hmac APIs.

Reviewed-by: Dorjoy Chowdhury <dorjoychy111@gmail.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/unit/test-crypto-hash.c | 7 ++++---
 tests/unit/test-crypto-hmac.c | 6 ++++--
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/tests/unit/test-crypto-hash.c b/tests/unit/test-crypto-hash.c
index e5829ca766..76c4699c15 100644
--- a/tests/unit/test-crypto-hash.c
+++ b/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]);
diff --git a/tests/unit/test-crypto-hmac.c b/tests/unit/test-crypto-hmac.c
index 3fa50f24bb..cdb8774443 100644
--- a/tests/unit/test-crypto-hmac.c
+++ b/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++) {
-- 
2.46.0

[PULL 06/14] include/crypto: clarify @result/@result_len for hash/hmac APIs

[Daniel P. Berrangé]

The @result parameter passed to hash/hmac APIs may either contain
a pre-allocated buffer, or a buffer can be allocated on the fly.
Clarify these two different usage models in the API docs.

Reviewed-by: Dorjoy Chowdhury <dorjoychy111@gmail.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/crypto/hash.h | 47 ++++++++++++++++++++++++++++++++-----------
 include/crypto/hmac.h | 34 ++++++++++++++++++++++---------
 2 files changed, 59 insertions(+), 22 deletions(-)

diff --git a/include/crypto/hash.h b/include/crypto/hash.h
index b791ca92a4..712cac79ee 100644
--- a/include/crypto/hash.h
+++ b/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
- * filled with raw bytes representing the computed
- * hash, which will have length @resultlen. The
- * memory pointer in @result must be released
- * with a call to g_free() when no longer required.
+ * present in @iov.
+ *
+ * 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
  */
@@ -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
- * filled with raw bytes representing the computed
- * hash, which will have length @resultlen. The
- * memory pointer in @result must be released
- * with a call to g_free() when no longer required.
+ * @buf of length @len.
+ *
+ * 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
  */
@@ -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
  */
diff --git a/include/crypto/hmac.h b/include/crypto/hmac.h
index c69a0dfab3..da8a1e3ceb 100644
--- a/include/crypto/hmac.h
+++ b/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
- * filled with raw bytes representing the computed
- * hmac, which will have length @resultlen. The
- * memory pointer in @result must be released
- * with a call to g_free() when no longer required.
+ * present in @iov.
+ *
+ * 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
@@ -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
- * filled with raw bytes representing the computed
- * hmac, which will have length @resultlen. The
- * memory pointer in @result must be released
- * with a call to g_free() when no longer required.
+ * @buf of length @len.
+ *
+ * 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
-- 
2.46.0

[PULL 07/14] crypto/hash-afalg: Fix broken build

[Daniel P. Berrangé]

From: Markus Armbruster <armbru@redhat.com>

Fux build broken by semantic conflict with commit
8f525028bc6 (qapi/crypto: Rename QCryptoAFAlg to QCryptoAFAlgo).

Fixes: 90c3dc60735a (crypto/hash-afalg: Implement new hash API)
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/hash-afalg.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/crypto/hash-afalg.c b/crypto/hash-afalg.c
index 06e1e4699c..8c0ce5b520 100644
--- a/crypto/hash-afalg.c
+++ b/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);
 }
 
-- 
2.46.0

[PULL 08/14] 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.

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
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

[PULL 09/14] 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.

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
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

[PULL 10/14] 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.

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
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

[PULL 11/14] 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.

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
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

[PULL 12/14] 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.

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
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

[PULL 13/14] 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.

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

[PULL 14/14] gitlab: enable afalg tests in fedora system test

[Daniel P. Berrangé]

The AF_ALG crypto integration for Linux is not being tested in
any CI scenario. It always requires an explicit configure time
flag to be passed to turn it on. The Fedora system test is
arbitrarily picked as the place to test it.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 .gitlab-ci.d/buildtest.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
index 01e8470a69..f0cbdf1992 100644
--- a/.gitlab-ci.d/buildtest.yml
+++ b/.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
-- 
2.46.0

Re: [PULL 00/14] Misc fixes patches

[Peter Maydell]

On Tue, 22 Oct 2024 at 16:24, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> The following changes since commit cc5adbbd50d81555b8eb73602ec16fde40b55be4:
>
>   Merge tag 'pull-tpm-2024-10-18-1' of https://github.com/stefanberger/qemu-tpm into staging (2024-10-18 15:45:02 +0100)
>
> are available in the Git repository at:
>
>   https://gitlab.com/berrange/qemu tags/misc-fixes-pull-request
>
> for you to fetch changes up to c64df333f92798823c4897ae6d4bd7f49d060225:
>
>   gitlab: enable afalg tests in fedora system test (2024-10-22 13:02:33 +0100)
>
> ----------------------------------------------------------------
> 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
>
> ----------------------------------------------------------------


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/9.2
for any user-visible changes.

-- PMM