src/libvirt_sasl.syms | 1 + src/remote/remote_daemon_dispatch.c | 8 +++++--- src/rpc/virnetsaslcontext.c | 25 +++++++++++++++++++++++++ src/rpc/virnetsaslcontext.h | 2 ++ 4 files changed, 33 insertions(+), 3 deletions(-)
From: Michal Privoznik <mprivozn@redhat.com>
The usual flow of SASL is REMOTE_PROC_AUTH_SASL_INIT followed by
REMOTE_PROC_AUTH_SASL_START which can either complete
authentication or require further steps which are then done by
calling REMOTE_PROC_AUTH_SASL_STEP (multiple times even).
But if a malicious client skips the SASL_START step and calls
SASL_INIT followed by SASL_STEP then the 'sasl->conn' structure
inside of virNetSASLSessionServerStep() is not fully allocated
(most notably the list of auth mechanisms 'mech' is NULL). But
then sasl_server_step() dereferences that pointer leading to a
crash.
Fix this by disallowing SASL_STEP until SASL_START completed
successfully.
Fixes: CVE-2026-119151
Resolves: https://gitlab.com/libvirt/libvirt/-/work_items/940
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
src/libvirt_sasl.syms | 1 +
src/remote/remote_daemon_dispatch.c | 8 +++++---
src/rpc/virnetsaslcontext.c | 25 +++++++++++++++++++++++++
src/rpc/virnetsaslcontext.h | 2 ++
4 files changed, 33 insertions(+), 3 deletions(-)
diff --git a/src/libvirt_sasl.syms b/src/libvirt_sasl.syms
index 405ba1813e..c2a9144f0e 100644
--- a/src/libvirt_sasl.syms
+++ b/src/libvirt_sasl.syms
@@ -18,6 +18,7 @@ virNetSASLSessionExtKeySize;
virNetSASLSessionGetIdentity;
virNetSASLSessionGetKeySize;
virNetSASLSessionGetMaxBufSize;
+virNetSASLSessionIsStarted;
virNetSASLSessionListMechanisms;
virNetSASLSessionNewClient;
virNetSASLSessionNewServer;
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index e548438250..a813111c8d 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -3942,7 +3942,8 @@ remoteDispatchAuthSaslStart(virNetServer *server,
VIR_DEBUG("Start SASL auth %d", virNetServerClientGetFD(client));
if (virNetServerClientGetAuth(client) != VIR_NET_SERVER_SERVICE_AUTH_SASL ||
- priv->sasl == NULL) {
+ priv->sasl == NULL ||
+ virNetSASLSessionIsStarted(priv->sasl)) {
VIR_ERROR(_("client tried invalid SASL start request"));
goto authfail;
}
@@ -4033,8 +4034,9 @@ remoteDispatchAuthSaslStep(virNetServer *server,
VIR_DEBUG("Step SASL auth %d", virNetServerClientGetFD(client));
if (virNetServerClientGetAuth(client) != VIR_NET_SERVER_SERVICE_AUTH_SASL ||
- priv->sasl == NULL) {
- VIR_ERROR(_("client tried invalid SASL start request"));
+ priv->sasl == NULL ||
+ !virNetSASLSessionIsStarted(priv->sasl)) {
+ VIR_ERROR(_("client tried invalid SASL step request"));
goto authfail;
}
diff --git a/src/rpc/virnetsaslcontext.c b/src/rpc/virnetsaslcontext.c
index 8692703117..e6b57b7b95 100644
--- a/src/rpc/virnetsaslcontext.c
+++ b/src/rpc/virnetsaslcontext.c
@@ -43,6 +43,7 @@ struct _virNetSASLSession {
sasl_conn_t *conn;
size_t maxbufsize;
sasl_callback_t *callbacks;
+ bool started;
};
@@ -529,6 +530,13 @@ int virNetSASLSessionServerStart(virNetSASLSession *sasl,
int ret = -1;
virObjectLock(sasl);
+
+ if (sasl->started) {
+ virReportError(VIR_ERR_AUTH_FAILED, "%s",
+ _("unexpected state of SASL session"));
+ goto cleanup;
+ }
+
err = sasl_server_start(sasl->conn,
mechname,
clientin,
@@ -542,13 +550,16 @@ int virNetSASLSessionServerStart(virNetSASLSession *sasl,
case SASL_OK:
if (virNetSASLSessionUpdateBufSize(sasl) < 0)
goto cleanup;
+ sasl->started = true;
ret = VIR_NET_SASL_COMPLETE;
break;
case SASL_CONTINUE:
ret = VIR_NET_SASL_CONTINUE;
+ sasl->started = true;
break;
case SASL_INTERACT:
ret = VIR_NET_SASL_INTERACT;
+ sasl->started = true;
break;
default:
virReportError(VIR_ERR_AUTH_FAILED,
@@ -575,6 +586,13 @@ int virNetSASLSessionServerStep(virNetSASLSession *sasl,
int ret = -1;
virObjectLock(sasl);
+
+ if (sasl->started == false) {
+ virReportError(VIR_ERR_AUTH_FAILED, "%s",
+ _("unexpected state of SASL session"));
+ goto cleanup;
+ }
+
err = sasl_server_step(sasl->conn,
clientin,
inlen,
@@ -707,6 +725,13 @@ void virNetSASLSessionDispose(void *obj)
g_free(sasl->callbacks);
}
+bool virNetSASLSessionIsStarted(virNetSASLSession *sasl)
+{
+ VIR_LOCK_GUARD lock = virObjectLockGuard(sasl);
+
+ return sasl->started;
+}
+
#ifdef __APPLE__
VIR_WARNINGS_RESET
#endif
diff --git a/src/rpc/virnetsaslcontext.h b/src/rpc/virnetsaslcontext.h
index 7202822e5b..5bfb273e67 100644
--- a/src/rpc/virnetsaslcontext.h
+++ b/src/rpc/virnetsaslcontext.h
@@ -109,3 +109,5 @@ ssize_t virNetSASLSessionDecode(virNetSASLSession *sasl,
size_t inputLen,
const char **output,
size_t *outputlen);
+
+bool virNetSASLSessionIsStarted(virNetSASLSession *sasl);
--
2.56.0Hi Michal,
When I wrote Sys::Async::Virt 2 years ago, I extensively studied the
SASL code in libvirt.
At the time, I thought I saw patters where the following sequence
might be going on: INIT -> START -> STEP -> START
The second START would be issued after setting up the correct
authentication data after an authentication had been challenged. Do
you see the same pattern? If so, then the second START will already
have the "started" boolean set to true, even when it hasn't. Using a
state machine with a single "state" variable would prevent that.
Regards,
Erik.
On Thu, Oct 29, 2026 at 4:29 PM Michal Privoznik via Devel
<devel@lists.libvirt.org> wrote:
>
> From: Michal Privoznik <mprivozn@redhat.com>
>
> The usual flow of SASL is REMOTE_PROC_AUTH_SASL_INIT followed by
> REMOTE_PROC_AUTH_SASL_START which can either complete
> authentication or require further steps which are then done by
> calling REMOTE_PROC_AUTH_SASL_STEP (multiple times even).
>
> But if a malicious client skips the SASL_START step and calls
> SASL_INIT followed by SASL_STEP then the 'sasl->conn' structure
> inside of virNetSASLSessionServerStep() is not fully allocated
> (most notably the list of auth mechanisms 'mech' is NULL). But
> then sasl_server_step() dereferences that pointer leading to a
> crash.
>
> Fix this by disallowing SASL_STEP until SASL_START completed
> successfully.
>
> Fixes: CVE-2026-119151
> Resolves: https://gitlab.com/libvirt/libvirt/-/work_items/940
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> src/libvirt_sasl.syms | 1 +
> src/remote/remote_daemon_dispatch.c | 8 +++++---
> src/rpc/virnetsaslcontext.c | 25 +++++++++++++++++++++++++
> src/rpc/virnetsaslcontext.h | 2 ++
> 4 files changed, 33 insertions(+), 3 deletions(-)
>
> diff --git a/src/libvirt_sasl.syms b/src/libvirt_sasl.syms
> index 405ba1813e..c2a9144f0e 100644
> --- a/src/libvirt_sasl.syms
> +++ b/src/libvirt_sasl.syms
> @@ -18,6 +18,7 @@ virNetSASLSessionExtKeySize;
> virNetSASLSessionGetIdentity;
> virNetSASLSessionGetKeySize;
> virNetSASLSessionGetMaxBufSize;
> +virNetSASLSessionIsStarted;
> virNetSASLSessionListMechanisms;
> virNetSASLSessionNewClient;
> virNetSASLSessionNewServer;
> diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
> index e548438250..a813111c8d 100644
> --- a/src/remote/remote_daemon_dispatch.c
> +++ b/src/remote/remote_daemon_dispatch.c
> @@ -3942,7 +3942,8 @@ remoteDispatchAuthSaslStart(virNetServer *server,
>
> VIR_DEBUG("Start SASL auth %d", virNetServerClientGetFD(client));
> if (virNetServerClientGetAuth(client) != VIR_NET_SERVER_SERVICE_AUTH_SASL ||
> - priv->sasl == NULL) {
> + priv->sasl == NULL ||
> + virNetSASLSessionIsStarted(priv->sasl)) {
> VIR_ERROR(_("client tried invalid SASL start request"));
> goto authfail;
> }
> @@ -4033,8 +4034,9 @@ remoteDispatchAuthSaslStep(virNetServer *server,
>
> VIR_DEBUG("Step SASL auth %d", virNetServerClientGetFD(client));
> if (virNetServerClientGetAuth(client) != VIR_NET_SERVER_SERVICE_AUTH_SASL ||
> - priv->sasl == NULL) {
> - VIR_ERROR(_("client tried invalid SASL start request"));
> + priv->sasl == NULL ||
> + !virNetSASLSessionIsStarted(priv->sasl)) {
> + VIR_ERROR(_("client tried invalid SASL step request"));
> goto authfail;
> }
>
> diff --git a/src/rpc/virnetsaslcontext.c b/src/rpc/virnetsaslcontext.c
> index 8692703117..e6b57b7b95 100644
> --- a/src/rpc/virnetsaslcontext.c
> +++ b/src/rpc/virnetsaslcontext.c
> @@ -43,6 +43,7 @@ struct _virNetSASLSession {
> sasl_conn_t *conn;
> size_t maxbufsize;
> sasl_callback_t *callbacks;
> + bool started;
> };
>
>
> @@ -529,6 +530,13 @@ int virNetSASLSessionServerStart(virNetSASLSession *sasl,
> int ret = -1;
>
> virObjectLock(sasl);
> +
> + if (sasl->started) {
> + virReportError(VIR_ERR_AUTH_FAILED, "%s",
> + _("unexpected state of SASL session"));
> + goto cleanup;
> + }
> +
> err = sasl_server_start(sasl->conn,
> mechname,
> clientin,
> @@ -542,13 +550,16 @@ int virNetSASLSessionServerStart(virNetSASLSession *sasl,
> case SASL_OK:
> if (virNetSASLSessionUpdateBufSize(sasl) < 0)
> goto cleanup;
> + sasl->started = true;
> ret = VIR_NET_SASL_COMPLETE;
> break;
> case SASL_CONTINUE:
> ret = VIR_NET_SASL_CONTINUE;
> + sasl->started = true;
> break;
> case SASL_INTERACT:
> ret = VIR_NET_SASL_INTERACT;
> + sasl->started = true;
> break;
> default:
> virReportError(VIR_ERR_AUTH_FAILED,
> @@ -575,6 +586,13 @@ int virNetSASLSessionServerStep(virNetSASLSession *sasl,
> int ret = -1;
>
> virObjectLock(sasl);
> +
> + if (sasl->started == false) {
> + virReportError(VIR_ERR_AUTH_FAILED, "%s",
> + _("unexpected state of SASL session"));
> + goto cleanup;
> + }
> +
> err = sasl_server_step(sasl->conn,
> clientin,
> inlen,
> @@ -707,6 +725,13 @@ void virNetSASLSessionDispose(void *obj)
> g_free(sasl->callbacks);
> }
>
> +bool virNetSASLSessionIsStarted(virNetSASLSession *sasl)
> +{
> + VIR_LOCK_GUARD lock = virObjectLockGuard(sasl);
> +
> + return sasl->started;
> +}
> +
> #ifdef __APPLE__
> VIR_WARNINGS_RESET
> #endif
> diff --git a/src/rpc/virnetsaslcontext.h b/src/rpc/virnetsaslcontext.h
> index 7202822e5b..5bfb273e67 100644
> --- a/src/rpc/virnetsaslcontext.h
> +++ b/src/rpc/virnetsaslcontext.h
> @@ -109,3 +109,5 @@ ssize_t virNetSASLSessionDecode(virNetSASLSession *sasl,
> size_t inputLen,
> const char **output,
> size_t *outputlen);
> +
> +bool virNetSASLSessionIsStarted(virNetSASLSession *sasl);
> --
> 2.56.0
>
--
Bye,
Erik.
http://efficito.com -- Hosted accounting and ERP.
Robust and Flexible. No vendor lock-in.
On 10/30/26 09:42, Erik Huelsmann wrote: > Hi Michal, > > > When I wrote Sys::Async::Virt 2 years ago, I extensively studied the > SASL code in libvirt. > > At the time, I thought I saw patters where the following sequence > might be going on: INIT -> START -> STEP -> START > > The second START would be issued after setting up the correct > authentication data after an authentication had been challenged. Do > you see the same pattern? If so, then the second START will already > have the "started" boolean set to true, even when it hasn't. Using a > state machine with a single "state" variable would prevent that. > Ah, if that's the case then my patch is incorrect, though the only thing needed is to drop some checks to allow START after STEP again. I have not seen that in my testing, though. And looking into remoteAuthSASL() I don't see a way that a client could call the sequence from your example. The only sequence possible is INIT -> START [-> STEP [-> STEP ...]]. Michal
On Fri, Oct 30, 2026 at 11:43:05AM +0100, Michal Prívozník via Devel wrote: > On 10/30/26 09:42, Erik Huelsmann wrote: > > Hi Michal, > > > > > > When I wrote Sys::Async::Virt 2 years ago, I extensively studied the > > SASL code in libvirt. > > > > At the time, I thought I saw patters where the following sequence > > might be going on: INIT -> START -> STEP -> START > > > > The second START would be issued after setting up the correct > > authentication data after an authentication had been challenged. Do > > you see the same pattern? If so, then the second START will already > > have the "started" boolean set to true, even when it hasn't. Using a > > state machine with a single "state" variable would prevent that. > > > > Ah, if that's the case then my patch is incorrect, though the only thing > needed is to drop some checks to allow START after STEP again. I have > not seen that in my testing, though. And looking into remoteAuthSASL() I > don't see a way that a client could call the sequence from your example. > The only sequence possible is INIT -> START [-> STEP [-> STEP ...]]. That is certainly the only /intended/ sequence. If a "START"/"STEP" fails, then server should be dropping the client connection. There is no intended provision for retrying authentication a second time on the same connection. The client would be expected to open a new connection to retry auth. If we're not already, we should also be careful not to allow "INIT" again, if the client is in the middle of START/STEP and has not yet got to completion from the server's POV. A state machine like Erik suggests does sound somewhat appealing from this POV. With regards, Daniel -- |: https://berrange.com ~~ https://hachyderm.io/@berrange :| |: https://libvirt.org ~~ https://entangle-photo.org :| |: https://pixelfed.art/berrange ~~ https://fstop138.berrange.com :|
© 2016 - 2026 Red Hat, Inc.