Re: [PATCH 2/2] security: Add a new func use stat to get process DAC label

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 12/01/2014 11:20 PM, Martin Kletzander wrote:
On Mon, Dec 01, 2014 at 11:05:30PM +0800, Luyao Huang wrote:

On 12/01/2014 06:24 PM, Martin Kletzander wrote:
On Mon, Dec 01, 2014 at 05:54:36PM +0800, Luyao Huang wrote:
When use qemuProcessAttach to attach a qemu process, cannot
get a right DAC label. Add a new func to get process label
via stat func. Do not remove virDomainDefGetSecurityLabelDef
before try to use stat to get process DAC label, because
There are some other func call virSecurityDACGetProcessLabel.

Signed-off-by: Luyao Huang <lhuang@xxxxxxxxxx>
---
src/security/security_dac.c | 50
+++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 48 insertions(+), 2 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 85253af..2977f71 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1237,17 +1237,63 @@
virSecurityDACReserveLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
}

static int
+virSecurityDACGetProcessLabelInternal(pid_t pid,
+                                      virSecurityLabelPtr seclabel)
+{
+    struct stat sb;
+    char *path = NULL;
+    char *label = NULL;
+    int ret = -1;
+
+    VIR_INFO("Getting DAC user and group on process '%d'", pid);
+
+    if (virAsprintf(&path, "/proc/%d", (int) pid) < 0)
+        goto cleanup;
+

This won't work on systems without /proc.
Okay, i need find a way to check this or find a way to get other system
process uid and gid.


Have a look at virProcessGetStartTime(), its implementation for linux
and freebsd should be enough for where
virSecurityDACGetProcessLabelInternal is going to be called.
Thanks a lot! I am worrying about that ; )

+    if (stat(path, &sb) < 0)
+        goto cleanup;
+

Better use lstat.
Thanks your advice.


+    if (virAsprintf(&label, "+%u:+%u",
+                    (unsigned int) sb.st_uid,
+                    (unsigned int) sb.st_gid) < 0)
+        goto cleanup;
+
+    if (virStrcpy(seclabel->label, label,VIR_SECURITY_LABEL_BUFLEN)
== NULL)
+        goto cleanup;
+    ret = 0;
+
+cleanup:
+    VIR_FREE(path);
+    VIR_FREE(label);
+    return ret;
+}
+
+static int
virSecurityDACGetProcessLabel(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
                              virDomainDefPtr def,
-                              pid_t pid ATTRIBUTE_UNUSED,
+                              pid_t pid,
                              virSecurityLabelPtr seclabel)
{
    virSecurityLabelDefPtr secdef =
        virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);

-    if (!secdef || !seclabel)
+    if (!seclabel)

I wonder whether this won't screw up domain definitions that don't
want to have any seclabel set (those defined with XML), I need to
figure that out.
After check the func wihch call virSecurityManagerGetProcessLabel, both
of them
will report error and go to error when failed get the DAC label (also
for selinux).

The most important reason is the virSecuritySELinuxGetSecurityProcessLabel,
After i see this func, i find it don't use def in this func ( don't use
virSecuritySELinuxGetSecurityProcessLabel to get the labels from def)
And i think this is the right way, because virSecurityManagerGetProcessLabel
should be a func which get the label from the process.

So in my opinion, if we can give a right and good way to get process DAC
label
for the system we support, we can remove virDomainDefGetSecurityLabelDef
and do not use def in this func.


Well, if there is some, we can output that.  And if there is not,
we'll get the current one.  I think this function can stay as is
(apart from the problem pointed out below, of course).

I am agree with you :)
        return -1;

+    if (secdef == NULL) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("missing label for DAC security "
+                         "driver in domain %s"), def->name);
+

This should probably be VIR_DEBUG or VIR_INFO, otherwise you report
error without erroring out (returning -1) and it gets saved for the
connection.

Yes, i used wrong func in this place, it should be  VIR_DEBUG.
+ if (virSecurityDACGetProcessLabelInternal(pid, seclabel) < 0) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("Cannot get process %d DAC label"),pid);
+            return -1;

Also two errors will be reported if this fails.

Oh, i didn't notice that, thanks for pointing out.
Martin

+        }
+
+        return 0;
+    }
+
    if (secdef->label)
        ignore_value(virStrcpy(seclabel->label, secdef->label,
                               VIR_SECURITY_LABEL_BUFLEN));
--
1.8.3.1

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list


--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]