External/libvirt
1
0
Fork 0
mirror of https://gitlab.com/libvirt/libvirt.git synced 2025-01-21 20:15:17 +00:00
Code Issues Packages Projects Releases Wiki Activity

rpc: virnetserver: Fix race on srv->nclients_unauth

Browse Source 
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>
This commit is contained in:
Marc Hartmayer 2017-12-21 15:29:05 +01:00 committed by John Ferlan
parent f1d8251972
commit 94bbbcee1f
12 changed files with 95 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
src/libvirt_remote.syms
View File

@ -138,6 +138,7 @@ virNetServerClientGetUNIXIdentity;
virNetServerClientImmediateClose;
virNetServerClientInit;
virNetServerClientInitKeepAlive;
virNetServerClientIsAuthPendingLocked;
virNetServerClientIsClosedLocked;
virNetServerClientIsLocal;
virNetServerClientIsSecure;
@ -152,6 +153,7 @@ virNetServerClientRemoteAddrStringURI;
virNetServerClientRemoveFilter;
virNetServerClientSendMessage;
virNetServerClientSetAuthLocked;
virNetServerClientSetAuthPendingLocked;
virNetServerClientSetCloseHook;
virNetServerClientSetDispatcher;
virNetServerClientSetReadonly;

27
src/rpc/virnetserver.c
View File

@ -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);

48
src/rpc/virnetserverclient.c
View File

@ -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)
{

2
src/rpc/virnetserverclient.h
View File

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

4
tests/virnetdaemondata/output-data-admin-nomdns.json
View File

@ -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": {

4
tests/virnetdaemondata/output-data-admin-server-names.json
View File

@ -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": {

2
tests/virnetdaemondata/output-data-anon-clients.json
View File

@ -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": {

2
tests/virnetdaemondata/output-data-client-ids.json
View File

@ -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": {

2
tests/virnetdaemondata/output-data-client-timestamp.json
View File

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

2
tests/virnetdaemondata/output-data-initial-nomdns.json
View File

@ -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": {

2
tests/virnetdaemondata/output-data-initial.json
View File

@ -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": {

4
tests/virnetdaemondata/output-data-no-keepalive-required.json
View File

@ -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": {