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

[Martin Kletzander]

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 at redhat.com>
>>> ---
>>> 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.

>>> +    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).

>>>         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 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/20141201/5c024dc3/attachment-0001.sig>