[Libvir] PATCH: BZ 427107: fix crash wrt auth callback
If the application does not supply an authentication callback, and tries to connect to a server with PolicyKit auth turned on it will try to deference a NULL pointer with predictably crashtastic results: https://bugzilla.redhat.com/show_bug.cgi?id=427107 This patch has been tested by bug reporter to fix the issue Index: src/remote_internal.c =================================================================== RCS file: /data/cvs/libvirt/src/remote_internal.c,v retrieving revision 1.49 diff -u -p -r1.49 remote_internal.c --- src/remote_internal.c 17 Dec 2007 17:41:02 -0000 1.49 +++ src/remote_internal.c 31 Dec 2007 20:30:19 -0000 @@ -3347,24 +3347,26 @@ remoteAuthPolkit (virConnectPtr conn, st }; remoteDebug(priv, "Client initialize PolicyKit authentication"); - for (i = 0 ; i < auth->ncredtype ; i++) { - if (auth->credtype[i] == VIR_CRED_EXTERNAL) - allowcb = 1; - } + if (auth && auth->cb) { + /* Check if the neccessary credential type for PolicyKit is supported */ + for (i = 0 ; i < auth->ncredtype ; i++) { + if (auth->credtype[i] == VIR_CRED_EXTERNAL) + allowcb = 1; + } - /* Run the authentication callback */ - if (allowcb) { - if (auth && auth->cb && - (*(auth->cb))(&cred, 1, auth->cbdata) < 0) { - __virRaiseError (in_open ? NULL : conn, NULL, NULL, VIR_FROM_REMOTE, - VIR_ERR_AUTH_FAILED, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, - "Failed to collect auth credentials"); - return -1; + if (allowcb) { + /* Run the authentication callback */ + if ((*(auth->cb))(&cred, 1, auth->cbdata) < 0) { + __virRaiseError (in_open ? NULL : conn, NULL, NULL, VIR_FROM_REMOTE, + VIR_ERR_AUTH_FAILED, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, + "Failed to collect auth credentials"); + return -1; + } } else { - remoteDebug(priv, "No auth callback provided for PolicyKit"); + remoteDebug(priv, "Client auth callback does not support PolicyKit"); } } else { - remoteDebug(priv, "Client auth callback does not support PolicyKit"); + remoteDebug(priv, "No auth callback provided"); } memset (&ret, 0, sizeof ret); Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
Daniel P. Berrange wrote:
If the application does not supply an authentication callback, and tries to connect to a server with PolicyKit auth turned on it will try to deference a NULL pointer with predictably crashtastic results:
+1 looks sane to me. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
On Wed, Jan 02, 2008 at 03:14:32PM +0000, Richard W.M. Jones wrote:
Daniel P. Berrange wrote:
If the application does not supply an authentication callback, and tries to connect to a server with PolicyKit auth turned on it will try to deference a NULL pointer with predictably crashtastic results:
+1 looks sane to me.
Thanks, have committed it now Regards, Dan -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
On Wed, Jan 02, 2008 at 12:31:56PM +0000, Daniel P. Berrange wrote:
If the application does not supply an authentication callback, and tries to connect to a server with PolicyKit auth turned on it will try to deference a NULL pointer with predictably crashtastic results:
https://bugzilla.redhat.com/show_bug.cgi?id=427107
This patch has been tested by bug reporter to fix the issue
Here is a second patch which fixes the same issue in the SASL client code too Dan. diff -rupN libvirt-0.4.0.orig/src/remote_internal.c libvirt-0.4.0.new/src/remote_internal.c --- libvirt-0.4.0.orig/src/remote_internal.c 2008-01-11 10:39:34.000000000 -0500 +++ libvirt-0.4.0.new/src/remote_internal.c 2008-01-11 10:43:12.000000000 -0500 @@ -3054,8 +3054,12 @@ remoteAuthSASL (virConnectPtr conn, stru if ((remoteAddr = addrToString(&sa, salen)) == NULL) goto cleanup; - if ((saslcb = remoteAuthMakeCallbacks(auth->credtype, auth->ncredtype)) == NULL) - goto cleanup; + if (auth) { + if ((saslcb = remoteAuthMakeCallbacks(auth->credtype, auth->ncredtype)) == NULL) + goto cleanup; + } else { + saslcb = NULL; + } /* Setup a handle for being a client */ err = sasl_client_new("libvirt", @@ -3168,15 +3172,21 @@ remoteAuthSASL (virConnectPtr conn, stru goto cleanup; } /* Run the authentication callback */ - if ((*(auth->cb))(cred, ncred, auth->cbdata) < 0) { + if (auth && auth->cb) { + if ((*(auth->cb))(cred, ncred, auth->cbdata) < 0) { + __virRaiseError (in_open ? NULL : conn, NULL, NULL, VIR_FROM_REMOTE, + VIR_ERR_AUTH_FAILED, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, + "Failed to collect auth credentials"); + goto cleanup; + } + remoteAuthFillInteract(cred, interact); + goto restart; + } else { __virRaiseError (in_open ? NULL : conn, NULL, NULL, VIR_FROM_REMOTE, VIR_ERR_AUTH_FAILED, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, - "Failed to collect auth credentials"); + "No authentication callback available"); goto cleanup; - return -1; } - remoteAuthFillInteract(cred, interact); - goto restart; } free(iret.mechlist); @@ -3240,15 +3250,22 @@ remoteAuthSASL (virConnectPtr conn, stru return -1; } /* Run the authentication callback */ - if ((*(auth->cb))(cred, ncred, auth->cbdata) < 0) { + if (auth && auth->cb) { + if ((*(auth->cb))(cred, ncred, auth->cbdata) < 0) { + __virRaiseError (in_open ? NULL : conn, NULL, NULL, VIR_FROM_REMOTE, + VIR_ERR_AUTH_FAILED, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, + "Failed to collect auth credentials"); + goto cleanup; + return -1; + } + remoteAuthFillInteract(cred, interact); + goto restep; + } else { __virRaiseError (in_open ? NULL : conn, NULL, NULL, VIR_FROM_REMOTE, VIR_ERR_AUTH_FAILED, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, - "Failed to collect auth credentials"); + "No authentication callback available"); goto cleanup; - return -1; } - remoteAuthFillInteract(cred, interact); - goto restep; } if (serverin) { @@ -3319,7 +3336,8 @@ remoteAuthSASL (virConnectPtr conn, stru if (remoteAddr) free(remoteAddr); if (serverin) free(serverin); - free(saslcb); + if (saslcb) + free(saslcb); remoteAuthFreeCredentials(cred, ncred); if (ret != 0 && saslconn) sasl_dispose(&saslconn); -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
Yup, looks also sensible. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Wed, Jan 02, 2008 at 12:31:56PM +0000, Daniel P. Berrange wrote:
If the application does not supply an authentication callback, and tries to connect to a server with PolicyKit auth turned on it will try to deference a NULL pointer with predictably crashtastic results:
https://bugzilla.redhat.com/show_bug.cgi?id=427107
This patch has been tested by bug reporter to fix the issue
Here is a second patch which fixes the same issue in the SASL client code too
Hi Dan, Looks correct. You can apply these to create an equivalent patch with the following changes: I saw that you removed one dead-code "return -1". This removes another: [PATCH] Remove dead-code "return -1" after goto. I found those two if/else blocks with all the gotos hard to read. This reorganizes and also avoids some duplication and shortens some longer-than-80-byte lines. [PATCH] Factor out two __virRaiseError,goto pairs. I know there are lots of other "if (var) free(var);" sequences in libvirt, but that "if (var)" part has been unnecessary for ages: [PATCH] Don't bother to check for non-NULL before calling free. ===================================================================== Date: Fri, 11 Jan 2008 18:53:57 +0100 Subject: [PATCH] Remove dead-code "return -1" after goto. diff --git a/src/remote_internal.c b/src/remote_internal.c index 88e9a72..88960ec 100644 --- a/src/remote_internal.c +++ b/src/remote_internal.c @@ -3249,7 +3249,6 @@ remoteAuthSASL (virConnectPtr conn, struct private_data *priv, int in_open, VIR_ERR_AUTH_FAILED, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, "Failed to collect auth credentials"); goto cleanup; - return -1; } remoteAuthFillInteract(cred, interact); goto restep; ===================================================================== Date: Fri, 11 Jan 2008 19:03:31 +0100 Subject: [PATCH] Factor out two __virRaiseError,goto pairs. diff --git a/src/remote_internal.c b/src/remote_internal.c index 88960ec..bca3ad3 100644 --- a/src/remote_internal.c +++ b/src/remote_internal.c @@ -3152,6 +3152,7 @@ remoteAuthSASL (virConnectPtr conn, struct private_data *priv, int in_open, /* Need to gather some credentials from the client */ if (err == SASL_INTERACT) { + const char *msg; if (cred) { remoteAuthFreeCredentials(cred, ncred); cred = NULL; @@ -3166,20 +3167,18 @@ remoteAuthSASL (virConnectPtr conn, struct private_data *priv, int in_open, } /* Run the authentication callback */ if (auth && auth->cb) { - if ((*(auth->cb))(cred, ncred, auth->cbdata) < 0) { - __virRaiseError (in_open ? NULL : conn, NULL, NULL, VIR_FROM_REMOTE, - VIR_ERR_AUTH_FAILED, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, - "Failed to collect auth credentials"); - goto cleanup; + if ((*(auth->cb))(cred, ncred, auth->cbdata) >= 0) { + remoteAuthFillInteract(cred, interact); + goto restart; } - remoteAuthFillInteract(cred, interact); - goto restart; + msg = "Failed to collect auth credentials"; } else { - __virRaiseError (in_open ? NULL : conn, NULL, NULL, VIR_FROM_REMOTE, - VIR_ERR_AUTH_FAILED, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, - "No authentication callback available"); - goto cleanup; + msg = "No authentication callback available"; } + __virRaiseError (in_open ? NULL : conn, NULL, NULL, VIR_FROM_REMOTE, + VIR_ERR_AUTH_FAILED, VIR_ERR_ERROR, NULL, NULL, NULL, + 0, 0, msg); + goto cleanup; } free(iret.mechlist); @@ -3231,6 +3230,7 @@ remoteAuthSASL (virConnectPtr conn, struct private_data *priv, int in_open, } /* Need to gather some credentials from the client */ if (err == SASL_INTERACT) { + const char *msg; if (cred) { remoteAuthFreeCredentials(cred, ncred); cred = NULL; @@ -3244,20 +3244,18 @@ remoteAuthSASL (virConnectPtr conn, struct private_data *priv, int in_open, } /* Run the authentication callback */ if (auth && auth->cb) { - if ((*(auth->cb))(cred, ncred, auth->cbdata) < 0) { - __virRaiseError (in_open ? NULL : conn, NULL, NULL, VIR_FROM_REMOTE, - VIR_ERR_AUTH_FAILED, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, - "Failed to collect auth credentials"); - goto cleanup; + if ((*(auth->cb))(cred, ncred, auth->cbdata) >= 0) { + remoteAuthFillInteract(cred, interact); + goto restep; } - remoteAuthFillInteract(cred, interact); - goto restep; + msg = "Failed to collect auth credentials"; } else { - __virRaiseError (in_open ? NULL : conn, NULL, NULL, VIR_FROM_REMOTE, - VIR_ERR_AUTH_FAILED, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, - "No authentication callback available"); - goto cleanup; + msg = "No authentication callback available"; } + __virRaiseError (in_open ? NULL : conn, NULL, NULL, VIR_FROM_REMOTE, + VIR_ERR_AUTH_FAILED, VIR_ERR_ERROR, NULL, NULL, NULL, + 0, 0, msg); + goto cleanup; } if (serverin) { ===================================================================== Date: Fri, 11 Jan 2008 19:05:45 +0100 Subject: [PATCH] Don't bother to check for non-NULL before calling free. diff --git a/src/remote_internal.c b/src/remote_internal.c index bca3ad3..88978d3 100644 --- a/src/remote_internal.c +++ b/src/remote_internal.c @@ -3326,8 +3326,7 @@ remoteAuthSASL (virConnectPtr conn, struct private_data *priv, int in_open, if (remoteAddr) free(remoteAddr); if (serverin) free(serverin); - if (saslcb) - free(saslcb); + free(saslcb); remoteAuthFreeCredentials(cred, ncred); if (ret != 0 && saslconn) sasl_dispose(&saslconn); --
On Fri, Jan 11, 2008 at 04:03:22PM +0000, Daniel P. Berrange wrote:
On Wed, Jan 02, 2008 at 12:31:56PM +0000, Daniel P. Berrange wrote:
If the application does not supply an authentication callback, and tries to connect to a server with PolicyKit auth turned on it will try to deference a NULL pointer with predictably crashtastic results:
https://bugzilla.redhat.com/show_bug.cgi?id=427107
This patch has been tested by bug reporter to fix the issue
Here is a second patch which fixes the same issue in the SASL client code too
I applied this & the code cleanup Jim suggested. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
participants (3)
-
Daniel P. Berrange -
Jim Meyering -
Richard W.M. Jones