[libvirt] [PATCH] security:selinux: Fix crash when tcon is NULL

[Martin Kletzander]

On Sat, Nov 08, 2014 at 06:17:26PM +0800, Luyao Huang wrote:
>https://bugzilla.redhat.com/show_bug.cgi?id=1161831
>
>Libvirtd will crash when parameter tcon = NULL in virSecuritySELinuxSetFileconHelper
>function, because libvirt do not check the first parameter when use strcmp().
>Add a check for tcon before use strcmp() and output a error in log when tcon is NULL.
>
>Signed-off-by: Luyao Huang <lhuang at redhat.com>
>---
> src/security/security_selinux.c | 5 +++++
> 1 file changed, 5 insertions(+)
>

NACK, calling virSecuritySELinuxSetFileconHelper() with NULL doesn't
make sense at all.  If you are trying to just fix a possible future
errot, then the check for non-NULL parameters needs to be done when
the function is starting.  But I doubt it's useful.  It's another
story if you managed to cause such call to this function.

>diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
>index f96be50..4fd09b8 100644
>--- a/src/security/security_selinux.c
>+++ b/src/security/security_selinux.c
>@@ -887,6 +887,11 @@ virSecuritySELinuxSetFileconHelper(const char *path, char *tcon, bool optional)
>         int setfilecon_errno = errno;
>
>         if (getfilecon_raw(path, &econ) >= 0) {
>+            if (tcon == NULL) {
>+                virReportSystemError(errno,"%s",
>+                                 _("Invalid security context : NULL"));
>+                return -1;
>+            }

Explaining how did you reach this point with tcon == NULL, or if you
even managed to do that, would help others understanding the issue
from higher level.  Anyway, the problem here is that this function is
called with tcon == NULL and not that it doesn't check for that (if
you managed to see the segfault).  To explain what I mean; if you
managed to see the segfault when you performed some operation, that's
bad.  But when this patch is applied, you still won't be able to
perform that operation successfully and we need to find out the root
cause of that.

The backtrace would fit nicely in the commit message as well.

Martin

>             if (STREQ(tcon, econ)) {
>                 freecon(econ);
>                 /* It's alright, there's nothing to change anyway. */
>--
>1.8.3.1
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20141110/2fd043e4/attachment-0001.sig>