1:31 p.m.
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 -=|
4:14 p.m.
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
11:48 p.m.
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 -=|
5:03 p.m.
New subject: [Libvir] Re: PATCH: BZ 427107: fix crash wrt auth callback
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 -=|
6:37 p.m.
New subject: [Libvir] Re: PATCH: BZ 427107: fix crash wrt auth callback
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
7:29 p.m.
New subject: [Libvir] Re: PATCH: BZ 427107: fix crash wrt auth callback
"Daniel P. Berrange" <berrange(a)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);
--
5:05 a.m.
New subject: [Libvir] Re: PATCH: BZ 427107: fix crash wrt auth callback
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 -=|
6094
days inactive
6106
days old
6 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Jim Meyering -
Richard W.M. Jones