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

[Luyao Huang]

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 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.
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 at redhat.com
>>>> https://www.redhat.com/mailman/listinfo/libvir-list
>>