[libvirt] [PATCHv1.5 5/8] security: DAC: Plumb usage of chown callback

[Peter Krempa]

On 07/22/14 17:59, John Ferlan wrote:
> 
> 
> On 07/22/2014 05:20 AM, Peter Krempa wrote:
>> Use the callback to set disk and storage image labels by modifying the
>> existing functions and adding wrappers to avoid refactoring a lot of the
>> code.
>> ---
>>  src/security/security_dac.c | 89 +++++++++++++++++++++++++++++++++++----------
>>  1 file changed, 69 insertions(+), 20 deletions(-)
>>
>> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
>> index 1fb0c86..989329f 100644
>> --- a/src/security/security_dac.c
>> +++ b/src/security/security_dac.c
>> @@ -230,23 +230,54 @@ virSecurityDACPreFork(virSecurityManagerPtr mgr)
>>  }
>>
>>  static int
>> -virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid)
>> +virSecurityDACSetOwnershipInternal(virSecurityDACDataPtr priv,
>> +                                   virStorageSourcePtr src,
>> +                                   const char *path,
>> +                                   uid_t uid,
>> +                                   gid_t gid)
>>  {
>> +    int rc;
>> +    int chown_errno;
>> +
>>      VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'",
>> -             path, (long) uid, (long) gid);
>> +             NULLSTR(src ? src->path : path), (long) uid, (long) gid);
>> +
>> +    if (priv && src && priv->chownCallback) {
> 
> This is the 'src' option and 'path' is NULL

Hmm, yes, if control reaches here with "path" equal to NULL, and ..


> 
>> +        rc = priv->chownCallback(src, uid, gid);

... the chown callback fails to chown the file, then ...

>> +
>> +        /* on -2 returned an error was already reported */
>> +        if (rc == -2)
>> +            return -1;
>>
>> -    if (chown(path, uid, gid) < 0) {
>> +        /* on -1 only errno was set */
>> +        chown_errno = errno;
> 
> Thus rc == -1, path == NULL
> 
>> +    } else {
>>          struct stat sb;
>> -        int chown_errno = errno;
>>
>> -        if (stat(path, &sb) >= 0) {
>> +        if (!path) {
>> +            if (!src || !src->path)
> 
> Maybe a "if !src && !path" return 0 should be before the VIR_INFO...
> Maybe with a VIR_DEBUG that'll help someone some day figure out why they
> aren't getting what they were expecting. Just a thought...
> 
>> +                return 0;
>> +
>> +            if (!virStorageSourceIsLocalStorage(src))
>> +                return 0;
>> +
>> +            path = src->path;
> 
> Reusing a passed parameter is where things get dicey for me.
> 
>> +        }
>> +
>> +        rc = chown(path, uid, gid);
>> +        chown_errno = errno;
>> +
>> +        if (rc < 0 &&
>> +            stat(path, &sb) >= 0) {
>>              if (sb.st_uid == uid &&
>>                  sb.st_gid == gid) {
>>                  /* It's alright, there's nothing to change anyway. */
>>                  return 0;
>>              }
>>          }
>> +    }
>>
>> +    if (rc < 0) {
> 
> I think we can get here with path == NULL and rc == -1, resulting in
> subsequent usage of '%s' for path to have 'nil', right?

... yes this will try to *printf a NULL string. So I'll add an
initialization of path to something sane in the branch using the
callback as at that point the "path" variable will be used only to
report errors.

Our reporting of "path" in case of remote storage sucks anyways so I'm
planning on adding a function that will format the path of the source
for error messages. At that point I'll revisit this (maybe even today as
the number of broken error messages I've seen in the last month is vast
and I'm not helping it currently)

> 
>>          if (chown_errno == EOPNOTSUPP || chown_errno == EINVAL) {
>>              VIR_INFO("Setting user and group to '%ld:%ld' on '%s' not "
>>                       "supported by filesystem",
>> @@ -270,13 +301,31 @@ virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid)
>>      return 0;
>>  }
>>
>> +
>>  static int
>> -virSecurityDACRestoreSecurityFileLabel(const char *path)
>> +virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid)
>> +{
>> +    return virSecurityDACSetOwnershipInternal(NULL, NULL, path, uid, gid);
>> +}
>> +
>> +
>> +static int
>> +virSecurityDACRestoreSecurityFileLabelInternal(virSecurityDACDataPtr priv,
>> +                                               virStorageSourcePtr src,
>> +                                               const char *path)
>>  {
>> -    VIR_INFO("Restoring DAC user and group on '%s'", path);
>> +    VIR_INFO("Restoring DAC user and group on '%s'",
>> +             NULLSTR(src ? src->path : path));
>>
>>      /* XXX record previous ownership */
>> -    return virSecurityDACSetOwnership(path, 0, 0);
>> +    return virSecurityDACSetOwnershipInternal(priv, src, path, 0, 0);
> 
> I know this just follows the previous code, but in the big picture - how
> does this "play" in with future patches where fs & gluster will
> seemingly go though this path?
> 
> 
> ACK - in general with the 'path' issue above understood.  For this code,
> I'm mostly curious.

I'll push this patch with "path" initialized to something sane and
revisit it when tweaking the error messages for functions using
virStorageSource struct.

> 
> John
>> +}

Peter


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 884 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140724/b9383783/attachment-0001.sig>