[PATCH] rpc: Deny REMOTE_PROC_AUTH_SASL_STEP without REMOTE_PROC_AUTH_SASL_START
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
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 <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 :|
Hi Michal, On Fri, Oct 30, 2026 at 11:43 AM Michal Prívozník <mprivozn@redhat.com> 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 ...]].
Well, this line: https://gitlab.com/libvirt/libvirt/-/blob/master/src/remote/remote_driver.c?... in combination with this "goto": https://gitlab.com/libvirt/libvirt/-/blob/master/src/remote/remote_driver.c?... led me to believe that it's possible to see two START commands on the same connection, which, I must say, confused me a lot at the time. I don't think my client supports the pattern, but then again, it uses a Perl implementation of the SASL protocol, not libsasl2. Regards, -- Bye, Erik. http://efficito.com -- Hosted accounting and ERP. Robust and Flexible. No vendor lock-in.
On Fri, Oct 30, 2026 at 05:48:56PM +0100, Erik Huelsmann wrote:
Hi Michal,
On Fri, Oct 30, 2026 at 11:43 AM Michal Prívozník <mprivozn@redhat.com> 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 ...]].
Well, this line:
https://gitlab.com/libvirt/libvirt/-/blob/master/src/remote/remote_driver.c?...
in combination with this "goto":
https://gitlab.com/libvirt/libvirt/-/blob/master/src/remote/remote_driver.c?...
led me to believe that it's possible to see two START commands on the same connection, which, I must say, confused me a lot at the time. I don't think my client supports the pattern, but then again, it uses a Perl implementation of the SASL protocol, not libsasl2.
Ah, no, this is client side logic. We're calling the SASL client library to start the auth start, and it indicates that we need to prompt for credentials. We do that prompt and then initiate the auth start again in the client library. Eventually it will have enough credentials such that auth start succeeds, and we can then send that auth start data over to the server in the START message. IOW, we call start on the client library many times, but the server only ever sees a single START rpc message. 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 :|
Hi Daniel,
At the time, I thought I saw patters where the following sequence might be going on: INIT -> START -> STEP -> START 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 ...]]. Well, this line: https://gitlab.com/libvirt/libvirt/-/blob/master/src/remote/remote_driver.c?... in combination with this "goto": https://gitlab.com/libvirt/libvirt/-/blob/master/src/remote/remote_driver.c?... [...] Ah, no, this is client side logic. [..] IOW, we call start on the client library many times, but the server only ever sees a single START rpc message.
Thanks! Good to have that cleared up! Then I guess that validates Michal's patch too. -- Bye, Erik. http://efficito.com -- Hosted accounting and ERP. Robust and Flexible. No vendor lock-in.
participants (4)
-
Daniel P. Berrangé -
Erik Huelsmann -
Michal Privoznik -
Michal Prívozník