rpc: virnetserver: Fix race on srv->nclients_unauth

There is a race between virNetServerProcessClients (main thread) and
remoteDispatchAuthList/remoteDispatchAuthPolkit/remoteSASLFinish (worker
thread) that can lead to decrementing srv->nclients_unauth when it's
zero. Since virNetServerCheckLimits relies on the value
srv->nclients_unauth the underrun causes libvirtd to stop accepting
new connections forever.

Example race scenario (assuming libvirtd is using policykit and the
client is privileged):
  1. The client calls the RPC remoteDispatchAuthList =>
     remoteDispatchAuthList is executed on a worker thread (Thread
     T1). We're assuming now the execution stops for some time before
     the line 'virNetServerClientSetAuth(client, 0)'
  2. The client closes the connection irregularly. This causes the
     event loop to wake up and virNetServerProcessClient to be
     called (on the main thread T0). During the
     virNetServerProcessClients the srv lock is hold. The condition
     virNetServerClientNeedAuth(client) will be checked and as the
     authentication is not finished right now
     virNetServerTrackCompletedAuthLocked(srv) will be called =>
     --srv->nclients_unauth => 0
  3. The Thread T1 continues, marks the client as authenticated, and
     calls virNetServerTrackCompletedAuthLocked(srv) =>
     --srv->nclients_unauth => --0 => wrap around as nclient_unauth is
     unsigned
  4. virNetServerCheckLimits(srv) will disable the services forever

To fix it, add an auth_pending field to the client struct so that it
is now possible to determine if the authentication process has already
been handled for this client.

Setting the authentication method to none for the client in
virNetServerProcessClients is not a proper way to indicate that the
counter has been decremented, as this would imply that the client is
authenticated.

Additionally, adjust the existing test cases for this new field.

Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>

diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index 62eac5ae9fddc9964110369fc7d806962df0edd9..5436e3ec18a477f2641af8a859d5826c3a7e3841 100644 (file)
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -138,6 +138,7 @@ virNetServerClientGetUNIXIdentity;
 virNetServerClientImmediateClose;
 virNetServerClientInit;
 virNetServerClientInitKeepAlive;
+virNetServerClientIsAuthPendingLocked;
 virNetServerClientIsClosedLocked;
 virNetServerClientIsLocal;
 virNetServerClientIsSecure;
@@ -152,6 +153,7 @@ virNetServerClientRemoteAddrStringURI;
 virNetServerClientRemoveFilter;
 virNetServerClientSendMessage;
 virNetServerClientSetAuthLocked;
+virNetServerClientSetAuthPendingLocked;
 virNetServerClientSetCloseHook;
 virNetServerClientSetDispatcher;
 virNetServerClientSetReadonly;

diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 946fc29283d83881d5f364ebca17712c9b3f6d27..77a4c0b8dce37658dbae9b77a1bbe9a818667d0a 100644 (file)
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -286,7 +286,7 @@ int virNetServerAddClient(virNetServerPtr srv,
     srv->clients[srv->nclients-1] = virObjectRef(client);
 
     virObjectLock(client);
-    if (virNetServerClientNeedAuthLocked(client))
+    if (virNetServerClientIsAuthPendingLocked(client))
         virNetServerTrackPendingAuthLocked(srv);
     virObjectUnlock(client);
 
@@ -737,6 +737,25 @@ int virNetServerSetTLSContext(virNetServerPtr srv,
 #endif
 
 
+/**
+ * virNetServerSetClientAuthCompletedLocked:
+ * @srv: server must be locked by the caller
+ * @client: client must be locked by the caller
+ *
+ * If the client authentication was pending, clear that pending and
+ * update the server tracking.
+ */
+static void
+virNetServerSetClientAuthCompletedLocked(virNetServerPtr srv,
+                                         virNetServerClientPtr client)
+{
+    if (virNetServerClientIsAuthPendingLocked(client)) {
+        virNetServerClientSetAuthPendingLocked(client, false);
+        virNetServerTrackCompletedAuthLocked(srv);
+    }
+}
+
+
 /**
  * virNetServerSetClientAuthenticated:
  * @srv: server must be unlocked
@@ -753,7 +772,7 @@ virNetServerSetClientAuthenticated(virNetServerPtr srv,
     virObjectLock(srv);
     virObjectLock(client);
     virNetServerClientSetAuthLocked(client, VIR_NET_SERVER_SERVICE_AUTH_NONE);
-    virNetServerTrackCompletedAuthLocked(srv);
+    virNetServerSetClientAuthCompletedLocked(srv, client);
     virNetServerCheckLimits(srv);
     virObjectUnlock(client);
     virObjectUnlock(srv);
@@ -872,8 +891,8 @@ virNetServerProcessClients(virNetServerPtr srv)
         if (virNetServerClientIsClosedLocked(client)) {
             VIR_DELETE_ELEMENT(srv->clients, i, srv->nclients);
 
-            if (virNetServerClientNeedAuthLocked(client))
-                virNetServerTrackCompletedAuthLocked(srv);
+            /* Update server authentication tracking */
+            virNetServerSetClientAuthCompletedLocked(srv, client);
             virObjectUnlock(client);
 
             virNetServerCheckLimits(srv);

diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index 5ebc970e340d141232f533a38afdb8a4f7783f6b..7786e3e2df8ef1297ca71fb39c35413dd1bbdafe 100644 (file)
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -71,6 +71,7 @@ struct _virNetServerClient
     bool delayedClose;
     virNetSocketPtr sock;
     int auth;
+    bool auth_pending;
     bool readonly;
 #if WITH_GNUTLS
     virNetTLSContextPtr tlsCtxt;
@@ -375,6 +376,7 @@ static virNetServerClientPtr
 virNetServerClientNewInternal(unsigned long long id,
                               virNetSocketPtr sock,
                               int auth,
+                              bool auth_pending,
 #ifdef WITH_GNUTLS
                               virNetTLSContextPtr tls,
 #endif
@@ -393,6 +395,7 @@ virNetServerClientNewInternal(unsigned long long id,
     client->id = id;
     client->sock = virObjectRef(sock);
     client->auth = auth;
+    client->auth_pending = auth_pending;
     client->readonly = readonly;
 #ifdef WITH_GNUTLS
     client->tlsCtxt = virObjectRef(tls);
@@ -440,6 +443,7 @@ virNetServerClientPtr virNetServerClientNew(unsigned long long id,
 {
     virNetServerClientPtr client;
     time_t now;
+    bool auth_pending = !virNetServerClientAuthMethodImpliesAuthenticated(auth);
 
     VIR_DEBUG("sock=%p auth=%d tls=%p", sock, auth,
 #ifdef WITH_GNUTLS
@@ -454,7 +458,7 @@ virNetServerClientPtr virNetServerClientNew(unsigned long long id,
         return NULL;
     }
 
-    if (!(client = virNetServerClientNewInternal(id, sock, auth,
+    if (!(client = virNetServerClientNewInternal(id, sock, auth, auth_pending,
 #ifdef WITH_GNUTLS
                                                  tls,
 #endif
@@ -486,7 +490,7 @@ virNetServerClientPtr virNetServerClientNewPostExecRestart(virJSONValuePtr objec
     virNetServerClientPtr client = NULL;
     virNetSocketPtr sock;
     int auth;
-    bool readonly;
+    bool readonly, auth_pending;
     unsigned int nrequests_max;
     unsigned long long id;
     long long timestamp;
@@ -496,6 +500,26 @@ virNetServerClientPtr virNetServerClientNewPostExecRestart(virJSONValuePtr objec
                        _("Missing auth field in JSON state document"));
         return NULL;
     }
+
+    if (!virJSONValueObjectHasKey(object, "auth_pending")) {
+        auth_pending = !virNetServerClientAuthMethodImpliesAuthenticated(auth);
+    } else {
+        if (virJSONValueObjectGetBoolean(object, "auth_pending", &auth_pending) < 0) {
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("Malformed auth_pending field in JSON state document"));
+            return NULL;
+        }
+
+        /* If the used authentication method implies that the new
+         * client is automatically authenticated, the authentication
+         * cannot be pending */
+        if (auth_pending && virNetServerClientAuthMethodImpliesAuthenticated(auth)) {
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("Invalid auth_pending and auth combination in JSON state document"));
+            return NULL;
+        }
+    }
+
     if (virJSONValueObjectGetBoolean(object, "readonly", &readonly) < 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("Missing readonly field in JSON state document"));
@@ -544,6 +568,7 @@ virNetServerClientPtr virNetServerClientNewPostExecRestart(virJSONValuePtr objec
     if (!(client = virNetServerClientNewInternal(id,
                                                  sock,
                                                  auth,
+                                                 auth_pending,
 #ifdef WITH_GNUTLS
                                                  NULL,
 #endif
@@ -592,6 +617,8 @@ virJSONValuePtr virNetServerClientPreExecRestart(virNetServerClientPtr client)
 
     if (virJSONValueObjectAppendNumberInt(object, "auth", client->auth) < 0)
         goto error;
+    if (virJSONValueObjectAppendBoolean(object, "auth_pending", client->auth_pending) < 0)
+        goto error;
     if (virJSONValueObjectAppendBoolean(object, "readonly", client->readonly) < 0)
         goto error;
     if (virJSONValueObjectAppendNumberUint(object, "nrequests_max", client->nrequests_max) < 0)
@@ -1556,6 +1583,23 @@ virNetServerClientNeedAuth(virNetServerClientPtr client)
 }
 
 
+/* The caller must hold the lock for @client */
+void
+virNetServerClientSetAuthPendingLocked(virNetServerClientPtr client,
+                                       bool auth_pending)
+{
+    client->auth_pending = auth_pending;
+}
+
+
+/* The caller must hold the lock for @client */
+bool
+virNetServerClientIsAuthPendingLocked(virNetServerClientPtr client)
+{
+    return client->auth_pending;
+}
+
+
 static void
 virNetServerClientKeepAliveDeadCB(void *opaque)
 {

diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h
index 054bea4f2f101dc8026bc544e531539b842694af..c174e8285f0cf5aa6728dea03e8d3b6acde9068c 100644 (file)
--- a/src/rpc/virnetserverclient.h
+++ b/src/rpc/virnetserverclient.h
@@ -149,6 +149,8 @@ int virNetServerClientSendMessage(virNetServerClientPtr client,
 
 bool virNetServerClientNeedAuth(virNetServerClientPtr client);
 bool virNetServerClientNeedAuthLocked(virNetServerClientPtr client);
+bool virNetServerClientIsAuthPendingLocked(virNetServerClientPtr client);
+void virNetServerClientSetAuthPendingLocked(virNetServerClientPtr client, bool auth_pending);
 
 int virNetServerClientGetTransport(virNetServerClientPtr client);
 int virNetServerClientGetInfo(virNetServerClientPtr client,

diff --git a/tests/virnetdaemondata/output-data-admin-nomdns.json b/tests/virnetdaemondata/output-data-admin-nomdns.json
index fc960f02cee7b75ad310385a846a394f7954e700..d6d02163e282471b442f1db2878240d4682da4cf 100644 (file)
--- a/tests/virnetdaemondata/output-data-admin-nomdns.json
+++ b/tests/virnetdaemondata/output-data-admin-nomdns.json
@@ -41,6 +41,7 @@
         {
           "id": 1,
           "auth": 1,
+          "auth_pending": true,
           "readonly": true,
           "nrequests_max": 15,
           "sock": {
@@ -53,6 +54,7 @@
         {
           "id": 2,
           "auth": 2,
+          "auth_pending": true,
           "readonly": true,
           "nrequests_max": 66,
           "sock": {
@@ -105,6 +107,7 @@
         {
           "id": 1,
           "auth": 1,
+          "auth_pending": true,
           "readonly": true,
           "nrequests_max": 15,
           "sock": {
@@ -117,6 +120,7 @@
         {
           "id": 2,
           "auth": 2,
+          "auth_pending": true,
           "readonly": true,
           "nrequests_max": 66,
           "sock": {

diff --git a/tests/virnetdaemondata/output-data-admin-server-names.json b/tests/virnetdaemondata/output-data-admin-server-names.json
index fc960f02cee7b75ad310385a846a394f7954e700..d6d02163e282471b442f1db2878240d4682da4cf 100644 (file)
--- a/tests/virnetdaemondata/output-data-admin-server-names.json
+++ b/tests/virnetdaemondata/output-data-admin-server-names.json
@@ -41,6 +41,7 @@
         {
           "id": 1,
           "auth": 1,
+          "auth_pending": true,
           "readonly": true,
           "nrequests_max": 15,
           "sock": {
@@ -53,6 +54,7 @@
         {
           "id": 2,
           "auth": 2,
+          "auth_pending": true,
           "readonly": true,
           "nrequests_max": 66,
           "sock": {
@@ -105,6 +107,7 @@
         {
           "id": 1,
           "auth": 1,
+          "auth_pending": true,
           "readonly": true,
           "nrequests_max": 15,
           "sock": {
@@ -117,6 +120,7 @@
         {
           "id": 2,
           "auth": 2,
+          "auth_pending": true,
           "readonly": true,
           "nrequests_max": 66,
           "sock": {

diff --git a/tests/virnetdaemondata/output-data-anon-clients.json b/tests/virnetdaemondata/output-data-anon-clients.json
index 9f1c63504a1b570e94fd78a3605d054cd000df21..cb6005bfe68c895469b61670800df9c569a75f7d 100644 (file)
--- a/tests/virnetdaemondata/output-data-anon-clients.json
+++ b/tests/virnetdaemondata/output-data-anon-clients.json
@@ -41,6 +41,7 @@
         {
           "id": 1,
           "auth": 1,
+          "auth_pending": true,
           "readonly": true,
           "nrequests_max": 15,
           "sock": {
@@ -53,6 +54,7 @@
         {
           "id": 2,
           "auth": 2,
+          "auth_pending": true,
           "readonly": true,
           "nrequests_max": 66,
           "sock": {

diff --git a/tests/virnetdaemondata/output-data-client-ids.json b/tests/virnetdaemondata/output-data-client-ids.json
index 43c61cc46ec789c835f06dbbca72d0b9c0261fc3..2b1663d4f8c652da42afdc9149724e7b5fb21a87 100644 (file)
--- a/tests/virnetdaemondata/output-data-client-ids.json
+++ b/tests/virnetdaemondata/output-data-client-ids.json
@@ -41,6 +41,7 @@
         {
           "id": 2,
           "auth": 1,
+          "auth_pending": true,
           "readonly": true,
           "nrequests_max": 15,
           "sock": {
@@ -53,6 +54,7 @@
         {
           "id": 3,
           "auth": 2,
+          "auth_pending": true,
           "readonly": true,
           "nrequests_max": 66,
           "sock": {

diff --git a/tests/virnetdaemondata/output-data-client-timestamp.json b/tests/virnetdaemondata/output-data-client-timestamp.json
index b706c147ed0ea1a33437382d3f1ed0d5b292ed5e..e8c8e9a991d910924f25897a6ca53e615b3b7047 100644 (file)
--- a/tests/virnetdaemondata/output-data-client-timestamp.json
+++ b/tests/virnetdaemondata/output-data-client-timestamp.json
@@ -41,6 +41,7 @@
         {
           "id": 1,
           "auth": 1,
+          "auth_pending": true,
           "readonly": true,
           "nrequests_max": 15,
           "conn_time": 1234567890,
@@ -54,6 +55,7 @@
         {
           "id": 2,
           "auth": 2,
+          "auth_pending": true,
           "readonly": true,
           "nrequests_max": 66,
           "conn_time": 1234567890,

diff --git a/tests/virnetdaemondata/output-data-initial-nomdns.json b/tests/virnetdaemondata/output-data-initial-nomdns.json
index ca6b9955375430ee7845590fb2a1a22dad8c8212..167aef8d295eaf8c40215dbd8d661bf68d99776a 100644 (file)
--- a/tests/virnetdaemondata/output-data-initial-nomdns.json
+++ b/tests/virnetdaemondata/output-data-initial-nomdns.json
@@ -41,6 +41,7 @@
         {
           "id": 1,
           "auth": 1,
+          "auth_pending": true,
           "readonly": true,
           "nrequests_max": 15,
           "sock": {
@@ -53,6 +54,7 @@
         {
           "id": 2,
           "auth": 2,
+          "auth_pending": true,
           "readonly": true,
           "nrequests_max": 66,
           "sock": {

diff --git a/tests/virnetdaemondata/output-data-initial.json b/tests/virnetdaemondata/output-data-initial.json
index a8df6336c316b6fcbfc2f6bc1ade306f68494d1a..3281e868d7aa25ad2b5893d859998f5ec10346da 100644 (file)
--- a/tests/virnetdaemondata/output-data-initial.json
+++ b/tests/virnetdaemondata/output-data-initial.json
@@ -42,6 +42,7 @@
         {
           "id": 1,
           "auth": 1,
+          "auth_pending": true,
           "readonly": true,
           "nrequests_max": 15,
           "sock": {
@@ -54,6 +55,7 @@
         {
           "id": 2,
           "auth": 2,
+          "auth_pending": true,
           "readonly": true,
           "nrequests_max": 66,
           "sock": {

diff --git a/tests/virnetdaemondata/output-data-no-keepalive-required.json b/tests/virnetdaemondata/output-data-no-keepalive-required.json
index fc960f02cee7b75ad310385a846a394f7954e700..d6d02163e282471b442f1db2878240d4682da4cf 100644 (file)
--- a/tests/virnetdaemondata/output-data-no-keepalive-required.json
+++ b/tests/virnetdaemondata/output-data-no-keepalive-required.json
@@ -41,6 +41,7 @@
         {
           "id": 1,
           "auth": 1,
+          "auth_pending": true,
           "readonly": true,
           "nrequests_max": 15,
           "sock": {
@@ -53,6 +54,7 @@
         {
           "id": 2,
           "auth": 2,
+          "auth_pending": true,
           "readonly": true,
           "nrequests_max": 66,
           "sock": {
@@ -105,6 +107,7 @@
         {
           "id": 1,
           "auth": 1,
+          "auth_pending": true,
           "readonly": true,
           "nrequests_max": 15,
           "sock": {
@@ -117,6 +120,7 @@
         {
           "id": 2,
           "auth": 2,
+          "auth_pending": true,
           "readonly": true,
           "nrequests_max": 66,
           "sock": {