[libvirt] Resubmission: [PATCH 3/6] sVirt AppArmor security driver
Daniel P. Berrange
berrange at redhat.com
Thu Sep 10 12:07:19 UTC 2009
On Tue, Sep 08, 2009 at 04:21:23PM -0500, Jamie Strandboge wrote:
> diff -Nurp ./libvirt.orig/src/security_apparmor.c ./libvirt/src/security_apparmor.c
> --- ./libvirt.orig/src/security_apparmor.c 1969-12-31 18:00:00.000000000 -0600
> +++ ./libvirt/src/security_apparmor.c 2009-09-08 15:32:42.000000000 -0500
> +/*
> + * profile_status returns '-1' on error, '0' if loaded
> + *
> + * If check_enforcing is set to '1', then returns '-1' on error, '0' if
> + * loaded in complain mode, and '1' if loaded in enforcing mode.
> + */
> +static int
> +profile_status(const char *str, const int check_enforcing)
> +{
> + char ebuf[1024];
> + char *content = NULL;
> + char *tmp = NULL;
> + char *etmp = NULL;
> + int rc = -1;
> +
> + /* create string that is '<str> \0' for accurate matching */
> + if (VIR_ALLOC_N(tmp, strlen(str) + 2) < 0) {
> + virSecurityReportError(NULL, VIR_ERR_ERROR,
> + _
> + ("%s: could not allocate memory for string"),
> + __func__);
> + return rc;
> + }
> + sprintf(tmp, "%s ", str);
> +
> + if (check_enforcing != 0) {
> + /* create string that is '<str> (enforce)\0' for accurate matching */
> + if (VIR_ALLOC_N(etmp, strlen(str) + 11) < 0) {
> + virSecurityReportError(NULL, VIR_ERR_ERROR,
> + _("%s: could not allocate memory for "
> + "string"), __func__);
> + VIR_FREE(tmp);
> + return rc;
> + }
> + sprintf(etmp, "%s (enforce)", str);
> + }
Can you replace the VIR_ALLOC_N + sprintf calls with just virAsprintf()
which is safer wrt future changes because it is guarenteed to automatically
allocate the correct amount of memory without us needing to hardcode it.
Likewise for future instances of ALLOC+printf in this patch.
> +
> + if (virFileReadAll(APPARMOR_PROFILES_PATH, MAX_FILE_LEN, &content) < 0) {
> + virSecurityReportError(NULL, VIR_ERR_ERROR,
> + _
> + ("%s: Failed to read AppArmor profiles list "
> + "%s: %s"), __func__,
> + APPARMOR_PROFILES_PATH, virStrerror(errno,
> + ebuf,
> + sizeof
> + ebuf));
I realize you're just copying the existing SELinux code style here, but
unfortunately the SELinux driver code was not following our best practices
for error reporting. Any place where you have an 'errno' value should call
the generic virReportSystemError() function, and not use virStrerror().
Also, there's no need to ever pass __func__ to any error reporting APIs
since we capture function, filename & lineno automatically in all cases.
> +static int
> +load_profile(const char *profile, virDomainObjPtr vm,
> + const char *skip_disk)
> +{
> + const char **argv;
> + int rc = -1;
> + int offset;
> + size_t i;
> + size_t len;
> + int create;
> + int j;
> +
> + create = 1;
> + offset = 6;
> + if (profile_status_file(profile) >= 0)
> + create = 0;
> + len = offset + vm->def->ndisks + 1;
> +
> + if (VIR_ALLOC_N(argv, len) < 0) {
> + virSecurityReportError(NULL, VIR_ERR_ERROR,
> + _("%s: could not allocate memory"),
> + __func__);
> + return rc;
> + }
> +
> + argv[0] = VIRT_AA_HELPER_PATH;
> + argv[2] = (char *) "-u";
> + argv[3] = profile;
> + argv[4] = (char *) "-n";
> + argv[5] = vm->def->name;
> +
> + if (create == 0)
> + argv[1] = (char *) "-r";
> + else
> + argv[1] = (char *) "-c";
> +
> + /* add disks to argv, skipping skip_disk if it is defined. */
> + j = 0;
> + for (i = 0; i < vm->def->ndisks; i++) {
> + if (vm->def->disks[i]->src == NULL || (skip_disk != NULL &&
> + STREQ(skip_disk,
> + vm->def->
> + disks[i]->src))) {
> + continue;
> + }
> + if (offset + j >= len - 1)
> + break;
> + argv[offset + j] = vm->def->disks[i]->src;
> + j++;
> + }
> + argv[offset + j] = NULL;
> +
> + if (virRun(NULL, argv, NULL) == 0)
> + rc = 0;
> +
> + VIR_FREE(argv);
> + return rc;
> +}
IMHO the way data is passed from the security driver to the aa-helper program
is sub-optimal & requires too much unneccessary code. As you add in support
for USB & PCI device labelling you'll have a large number of additional args
to pass to the helper. We'll likely add even more later for serial/parallel
device assignment & other config options. In effect we'll end up inventing a
entirely new custom data interchange format for this helper, when we already
have a perfect one for the job.... the domain XML.
Thus I'd prefer to see this code simply take the virDomainDefPtr object,
call virDomainDefFormat(), and feed the resulting XML string to the
helper program on its STDIN file descriptor (or any other FD if desired).
The helper program can then simply read the XML off the FD, and call
virDomainDefParseString() to get back a virDomainDefPtr object. That
way guarentees the aa-helper always has all the data it will ever need
about the geust config without us needing to add more & more command
line options to aa-helper to deal with new config params.
> +
> +/*
> + * profile_name is buffer to hold name and len is how many bytes in the
> + * buffer
> + */
> +static int
> +get_profile_name(virConnectPtr conn,
> + virDomainObjPtr vm, char *profile_name, const size_t len)
> +{
> + virDomainPtr dom = NULL;
> + int rc = -1;
> + char *prefix = (char *) "libvirt-";
Don't cast away const-ness, declare the variable with the neccessary
const-ness eg
const char *prefix = "libvirt-";
> +
> + if (len < PROFILE_NAME_SIZE) {
> + virSecurityReportError(conn, VIR_ERR_ERROR,
> + _("%s: profile_name has wrong size"),
> + __func__);
> + return rc;
> + }
> + profile_name[0] = '\0';
> + strcat(profile_name, prefix);
> +
> + /* generate the profile name */
> + dom = virGetDomain(conn, vm->def->name, vm->def->uuid);
> + if (!dom) {
> + virSecurityReportError(conn, VIR_ERR_ERROR,
> + _("%s: could not get domain for VM"),
> + __func__);
There's no need to raise an error when virGetDomain calls fail - it
will already have done that for you.
> +typedef struct {
> + char uuid[PROFILE_NAME_SIZE]; /* UUID of vm */
> + char name[PATH_MAX]; /* name of vm */
> + bool dryrun; /* dry run */
> + char cmd; /* 'c' create
> + * 'a' add (load)
> + * 'r' replace
> + * 'R' remove */
> + int ndisks; /* number of disks */
> + char **disks; /* list of disks */
> +} vahControl;
> +
> +static int
> +vahDeinit(vahControl * ctl)
> +{
> + int i;
> +
> + if (ctl->disks != NULL) {
> + for (i = 0; i < ctl->ndisks; i++) {
> + if (ctl->disks[i] == NULL)
> + continue;
> + free(ctl->disks[i]);
> + }
> + VIR_FREE(ctl->disks);
> + }
> + return 0;
> +}
> +
> +static void
> +vah_error(vahControl * ctl, int doexit, const char *str)
> +{
> + fprintf(stderr, _("%s: error: %s\n"), progname, str);
> +
> + if (doexit) {
> + if (ctl != NULL)
> + vahDeinit(ctl);
> + exit(EXIT_FAILURE);
> + }
> +}
> +
> +static void
> +vah_warning(const char *str)
> +{
> + fprintf(stderr, _("%s: warning: %s\n"), progname, str);
> +}
> +
> +static void
> +vah_info(const char *str)
> +{
> + fprintf(stderr, _("%s:\n%s\n"), progname, str);
> +}
> +
> +/*
> + * Replace @oldstr in @orig with @repstr
> + * @len is number of bytes allocated for @orig. Assumes @orig, @oldstr and
> + * @repstr are null terminated
> + */
> +static int
> +replace_string(char *orig, const size_t len, const char *oldstr,
> + const char *repstr)
> +{
> + int idx;
> + char *pos = NULL;
> + char *tmp = NULL;
> +
> + if ((pos = strstr(orig, oldstr)) == NULL) {
> + vah_error(NULL, 0, "could not find replacement string");
> + return -1;
> + }
> +
> + if (VIR_ALLOC_N(tmp, len) < 0) {
> + vah_error(NULL, 0, "could not allocate memory for string");
> + return -1;
> + }
> + tmp[0] = '\0';
> +
> + idx = abs(pos - orig);
> +
> + /* copy everything up to oldstr */
> + strncat(tmp, orig, idx);
> +
> + /* add the replacement string */
> + if (strlen(tmp) + strlen(repstr) > len - 1) {
> + vah_error(NULL, 0, "not enough space in target buffer");
> + VIR_FREE(tmp);
> + return -1;
> + }
> + strcat(tmp, repstr);
> +
> + /* add everything after oldstr */
> + if (strlen(tmp) + strlen(orig) - (idx + strlen(oldstr)) > len - 1) {
> + vah_error(NULL, 0, "not enough space in target buffer");
> + VIR_FREE(tmp);
> + return -1;
> + }
> + strncat(tmp, orig + idx + strlen(oldstr),
> + strlen(orig) - (idx + strlen(oldstr)));
> +
> + strncpy(orig, tmp, len);
> + orig[len - 1] = '\0';
> + VIR_FREE(tmp);
> +
> + return 0;
> +}
> +
> +/*
> + * run an apparmor_parser command
> + */
> +static int
> +parserCommand(const char *profile_name, const char cmd)
> +{
> + const char *argv[4];
> + char flag[3];
> + char profile[PATH_MAX];
> +
> + if (strchr("arR", cmd) == NULL) {
> + vah_error(NULL, 0, "invalid flag");
> + return -1;
> + }
> +
> + snprintf(flag, 3, "-%c", cmd);
> +
> + if (snprintf(profile, PATH_MAX, "%s/%s",
> + APPARMOR_DIR "/libvirt", profile_name) > PATH_MAX - 1) {
> + vah_error(NULL, 0, "profile name exceeds maximum length");
> + return -1;
> + }
> +
> + if (!virFileExists(profile)) {
> + vah_error(NULL, 0, "profile does not exist");
> + return -1;
> + }
> +
> + argv[0] = (char *) "/sbin/apparmor_parser";
> + argv[1] = flag;
> + argv[2] = profile;
> + argv[3] = NULL;
> +
> + if (virRun(NULL, argv, NULL) != 0) {
> + vah_error(NULL, 0, "failed to run apparmor_parser");
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * Update the disks file
> + */
> +static int
> +update_include_file(const char *include_file, const char *included_files)
> +{
> + int plen;
> + int fd;
> + char *pcontent = NULL;
> + char *existing = NULL;
> + char *warning = (char *)
> + "# DO NOT EDIT THIS FILE DIRECTLY. IT IS MANAGED BY LIBVIRT.\n";
More const-ness oissues here.
> + /* While 3*PATH_MAX is not accurate when thinking about PATH_MAX,
> + * it is good enough since the #include line is a relative path
> + * and we are just trying to keep tmp from overflowing here anyway
> + */
> + if (snprintf(tmp, 3 * PATH_MAX,
> + " %s/log/libvirt/**/%s.log w,\n"
> + " %s/lib/libvirt/**/%s.monitor rw,\n"
> + " %s/run/libvirt/**/%s.pid rwk,\n",
> + LOCAL_STATE_DIR, ctl->name, LOCAL_STATE_DIR, ctl->name,
> + LOCAL_STATE_DIR, ctl->name) > 3 * PATH_MAX - 1) {
> + vah_error(ctl, 0, "added rules exceed maximum length");
> + goto clean;
> + }
It is much safer to just use virAsprintf() here instead of a huge static
buffer
Regards,
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
More information about the libvir-list
mailing list