[libvirt] [PATCH v3 1/2] security: also parse user/group names instead of just IDs for DAC labels

[Marcelo Cerri]

Just one question: do you think that makes sense for virGetUserID and
virGetGroupID to check if a id exists when a numeric value is given?

Regards,
Marcelo

On Fri, Oct 05, 2012 at 09:44:46AM -0300, Marcelo Cerri wrote:
> On Thu, Oct 04, 2012 at 05:46:31PM -0600, Eric Blake wrote:
> > On 10/02/2012 11:57 AM, Marcelo Cerri wrote:
> > > The DAC driver is missing parsing of group and user names for DAC labels
> > > and currently just parses uid and gid. This patch extends it to support
> > > names, so the following security label definition is now valid:
> > > 
> > >   <seclabel type='static' model='dac' relabel='yes'>
> > >       <label>qemu:qemu</label>
> > >       <imagelabel>qemu:qemu</imagelabel>
> > >   </seclabel>
> > > 
> > > When it tries to parse an owner or a group, it first tries to resolve it as
> > > a name, if it fails or it's an invalid user/group name then it tries to
> > > parse it as an UID or GID. A leading '+' can also be used for both owner and
> > > group to force it to be parsed as IDs, so the following example is also
> > > valid:
> > > 
> > >   <seclabel type='static' model='dac' relabel='yes'>
> > >       <label>+101:+101</label>
> > >       <imagelabel>+101:+101</imagelabel>
> > >   </seclabel>
> > > 
> > 
> > Yuck.  With this patch, I'm seeing lots of ugly error messages in the log:
> > 
> > 2012-10-04 22:59:52.584+0000: 9225: error : virGetUserID:2535 : Failed
> > to find user record for name '0': Success
> > 
> > I think the correct fix is to move this logic...
> > 
> > > +    /* Parse owner */
> > > +    if (*owner == '+') {
> > > +        if (virStrToLong_ui(++owner, NULL, 10, &theuid) < 0) {
> > > +            virReportError(VIR_ERR_INVALID_ARG,
> > > +                           _("Invalid uid \"%s\" in DAC label \"%s\""),
> > > +                           owner, label);
> > > +            goto cleanup;
> > > +        }
> > > +    } else {
> > > +        if (virGetUserID(owner, &theuid) < 0 &&
> > > +            virStrToLong_ui(owner, NULL, 10, &theuid) < 0) {
> > > +            virReportError(VIR_ERR_INVALID_ARG,
> > > +                           _("Invalid owner \"%s\" in DAC label \"%s\""),
> > > +                           owner, label);
> > > +            goto cleanup;
> > > +        }
> > >      }
> > 
> > ...out of security_dac.c and into src/util/util.c:virGetUserID(), so
> > that we are consistently parsing in this manner for ALL places where we
> > convert a string into a user id, and also so that virGetUserID will quit
> > logging such a bogus error message when it fails to find a given id
> > string that happens to be a valid number.
> Ok. I'll provide a patch for this.
> 
> I made a search in the code for virGetUserID and, other than in
> security_dac.c, it seems to be used just for parsing user name in
> qemu.conf.
> 
> >
> > Likewise for virGetGroupID.
> 
> Same for virGetGroupID.
> 
> > 
> > -- 
> > Eric Blake   eblake at redhat.com    +1-919-301-3266
> > Libvirt virtualization library http://libvirt.org
> > 
> 
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>