Re: [libvirt] Resubmission #2: [PATCH 2/3] sVirt AppArmor security driver

Attached is an updated patch_2_apparmor_driver_updated.patch and patch_3_docs_updated.patch. Thanks for the review! These two patches along with the previous patch_1_reenable-nonfile-labels.patch pass syntax-check, make check (for the tests I added) and introduce no regressions over the previous patch. See below for inline comments. In addition to implenting your suggestions, the patch also now supports <readonly/> for disks and I defensively quote the pid, monitor and logfile. I also added ReserveSecurityLabel (a noop) along with stubs for SetSecurityHostdevLabel() and RestoreSecurityHostdevLabel(). Jamie On Wed, 30 Sep 2009, Daniel P. Berrange wrote: > > +static int > > +profile_status(const char *str, const int check_enforcing) > > +{ > > Since you've already got yourself a 'rc' variable declared & initialized, > it'd benice to replace the duplicated error paths: > Done (along with others you mentioned). > > +static int > > +profile_status_file(const char *str) > > This one should just be virReportOOMError(NULL); > Done (along with others you mentioned). > > +static int > > +load_profile(virConnectPtr conn, const char *profile, virDomainObjPtr vm, > > No need to report an error when the virDomainDef* functions > fail, since they'll have already done that for you. > Done > Casting away const multiple times here. THis can be avoided by > changing > This is a favorite thing for me to do. :P Done (along with others you mentioned). > I find the out-of-order initialization of argv indicies here > a little confusing. Even though it'd be duplicating a little, > I think it'd be clearer to have > Done > I think I'd prefer to see this returning a string allocated on the heap, > rather than the caller's stack. The whole method could thus be simplified > down to just > Done > > +/* returns -1 on error or profile for libvirtd is unconfined, 0 if complain > > + * mode and 1 if enforcing > > + */ > > +static int > > +use_apparmor(void) > > Is this the best way to check if app armour is enabled on a host ? > It ought to be possible to use the app armour security driver for > protecting guests regardless of whether the libvirtd daemon itself > has a profile surely. > Actually, this is required. Currently, an unconfined process may not aa_change_profile(), which is why I am very careful here. > > +if WITH_SECDRIVER_APPARMOR > > +bin_PROGRAMS += virt-aa-helper > > Since this program is only really intended for libvirt to call > rather than general admin usage I think it'd be beter to list > it as libexec_PROGRAMS, so it ends up in /usr/libexec > instead of /usr/bin > > I think I'd actually have it in the src/security/ directory, > since we usually keep libexec programs alongside the driver > code that's using them, just have tools/ for end user programs > Done and done. > > + if (STRNEQLEN(AA_PREFIX, uuid, strlen(AA_PREFIX))) > > + return -1; > > This one can just be > > if (!STRPREFIX(uuid, AA_PREFIX)) > return -1; > Done > How about just using virUUIDParse to check UUID validity, such as > > char rawuuid[VIR_UUID_BUFLEN]; > if (virUUIDParse(uuid + strlen(AA_PREFIX), rawuuid) < 0) > return -1; > return 0; > Done > FYI, there'a convenient libc function for checking a string for bad > characters - strspn, you can use it in this way: > > if (strspn(name, bad) != strlen(name)) > return -1; > Ah, missed that one. I actually used strcspn() instead. Done > > +static int > > +valid_path(const char *path, const bool readonly) > > More const issues. Rather than manually declaring the array > size, and indexing manually its safer to initialize the > array at time of declaration with all the strings. I'd > probably go for separate arrays for the read-write vs > read-only difference > Done > > +static int > > +vah_add_file(virBufferPtr buf, const char *path, const char *perms) > > + > > + if (virBufferError(buf)) { > > + vah_error(NULL, 0, "failed to allocate file buffer"); > > + rc = -1; > > + } > > It is not really neeccesssar to check virBufferError here. The > idea is that you can call virBufferVSprintf() as many times > as you like in a row without any checking, and then only check > virBufferError at the end when you've finishing writing to the > buffer. > Done > > +static int > > +get_files(vahControl * ctl) > > +{ > > + virBuffer uuid_buf = VIR_BUFFER_INITIALIZER; > > Using virBuffer for a single printf here is a little overkill - the > virAsprintf() function would be more than sufficient > Yeah, at one point I was thinking of doing more here and forgot to go back to virAsprintf(). Done. > > + /* needs patch to hostusb.h to work > > + for (i = 0; i < ctl->def->nhostdevs; i++) > > + if (ctl->def->hostdevs[i]) { > > + virDomainHostdevDefPtr hostdev = ctl->def->hostdevs[i]; > > + if (hostdev->source.subsys.type == > > + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) { > > + usbDevice *usb = usbGetDevice(NULL, > > + hostdev->source.subsys.u.usb.bus, > > + hostdev->source.subsys.u.usb.device); > > + if (usb == NULL) > > + continue; > > + rc = vah_add_file(&buf, usb->path, "rw"); > > + usbFreeDevice(NULL, usb); > > This is the bit of code that should be changed to use the > usbDeviceFileIterate() method, since you can't assume there > is only 1 path which is why we didn't make 'path' public. > Done. virt-aa-helper now does this right, but overall attach-device with a USB or PCI host device still does not work (need to figure out the best way to get the hostdev XML to virt-aa-helper, as previously discussed). Users can adjust the profile/abstraction in the meantime as desired. > If you remove the virBufferError check from the vah_add_file > calls, it could just go in here > Done