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. Regards, Erik. On Thu, Oct 29, 2026 at 4:29 PM Michal Privoznik via Devel <[email protected]> wrote: > > From: Michal Privoznik <[email protected]> > > 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 <[email protected]> > --- > 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.
