From 409e8703cf7b78db960b65a1dea49607b557be95 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Thu, 8 May 2014 09:37:01 -0700 Subject: [PATCH] Fix profile loads from cache files that contain multiple profiles backport of dev commit 2510 v3: fix freeing of filename when undefined v2: address tyhicks feedback refactor to have a common write routine fix issue with set profile load being done even if !kernel_load Profile loads from cache files that contain multiple profiles can result in multiple reloads of the same profile or error messages about failure to load profiles if the --add option is used. eg. apparmor="STATUS" operation="profile_load" name="/usr/lib/apache2/mpm-prefork/apache2" pid=8631 comm="apparmor_parser" [82932.058388] type=1400 audit(1395415826.937:616): apparmor="STATUS" operation="profile_load" name="DEFAULT_URI" pid=8631 comm="apparmor_parser" [82932.058391] type=1400 audit(1395415826.937:617): apparmor="STATUS" operation="profile_load" name="HANDLING_UNTRUSTED_INPUT" pid=8631 comm="apparmor_parser" [82932.058394] type=1400 audit(1395415826.937:618): apparmor="STATUS" operation="profile_load" name="phpsysinfo" pid=8631 comm="apparmor_parser" [82932.059058] type=1400 audit(1395415826.937:619): apparmor="STATUS" operation="profile_replace" info="profile can not be replaced" error=-17 name="/usr/lib/apache2/mpm-prefork/apache2//DEFAULT_URI" pid=8631 comm="apparmor_parser" [82932.059574] type=1400 audit(1395415826.937:620): apparmor="STATUS" operation="profile_replace" info="profile can not be replaced" error=-17 name="/usr/lib/apache2/mpm-prefork/apache2//HANDLING_UNTRUSTED_INPUT" pid=8631 comm="apparmor_parser" The reason this happens is that the cache file is a container that can contain multiple profiles in sequential order profile1 profile2 profile3 The parser loads the entire cache file to memory and the writes the whole file to the kernel interface. It then skips foward in the file to the next profile and reloads the file from that profile into the kernel. eg. First load profile1 profile2 profile3 advance to profile2, do second load profile2 profile3 advance to profile3, do third load profile3 With older kernels the interface would stop after the first profile and return that it had processed the whole file, thus while wasting compute resources copying extra data no errors occurred. However newer kernels now support atomic loading of multipe profiles, so that all the profiles passed in to the interface get processed. This means on newer kernels the current parser load behavior results in multiple loads/replacements when a cache file contains more than one profile (note: loads from a compile do not have this problem). To fix this, detect if the kernel supports atomic set loads, and load the cache file once. If it doesn't only load one profile section from a cache file at a time. Signed-off-by: John Johansen Acked-by: Seth Arnold --- parser/parser.h | 1 + parser/parser_common.c | 1 + parser/parser_interface.c | 69 +++++++++++++++++++++++++-------------- parser/parser_main.c | 2 ++ 4 files changed, 49 insertions(+), 24 deletions(-) diff --git a/parser/parser.h b/parser/parser.h index 8199f4357..68aae7982 100644 --- a/parser/parser.h +++ b/parser/parser.h @@ -265,6 +265,7 @@ extern int regex_type; extern int perms_create; extern int net_af_max_override; extern int kernel_load; +extern int kernel_supports_setload; extern int kernel_supports_network; extern int kernel_supports_mount; extern int flag_changehat_version; diff --git a/parser/parser_common.c b/parser/parser_common.c index 15f0978a2..775e42954 100644 --- a/parser/parser_common.c +++ b/parser/parser_common.c @@ -26,6 +26,7 @@ int regex_type = AARE_DFA; int perms_create = 0; /* perms contain create flag */ int net_af_max_override = -1; /* use kernel to determine af_max */ int kernel_load = 1; +int kernel_supports_setload = 0; /* kernel supports atomic set loads */ int kernel_supports_network = 1; /* kernel supports network rules */ int kernel_supports_mount = 0; /* kernel supports mount rules */ int flag_changehat_version = FLAG_CHANGEHAT_1_5; diff --git a/parser/parser_interface.c b/parser/parser_interface.c index a8be7aa6b..70a7638a6 100644 --- a/parser/parser_interface.c +++ b/parser/parser_interface.c @@ -888,52 +888,73 @@ static char *next_profile_buffer(char *buffer, int size) return NULL; } +static int write_buffer(int fd, char *buffer, int size, bool set) +{ + const char *err_str = set ? "profile set" : "profile"; + int wsize = write(fd, buffer, size); + if (wsize < 0) { + PERROR(_("%s: Unable to write %s\n"), progname, err_str); + return -errno; + } else if (wsize < size) { + PERROR(_("%s: Unable to write %s\n"), progname, err_str); + return -EPROTO; + } + return 0; +} + int sd_load_buffer(int option, char *buffer, int size) { int fd = -1; - int error = -ENOMEM, wsize, bsize; + int error, bsize; char *filename = NULL; - char *b; + + /* TODO: push backup into caller */ + if (!kernel_load) + return 0; switch (option) { case OPTION_ADD: if (asprintf(&filename, "%s/.load", subdomainbase) == -1) - goto exit; - if (kernel_load) fd = open(filename, O_WRONLY); + return -ENOMEM; break; case OPTION_REPLACE: if (asprintf(&filename, "%s/.replace", subdomainbase) == -1) - goto exit; - if (kernel_load) fd = open(filename, O_WRONLY); + return -ENOMEM; break; default: - error = -EINVAL; - goto exit; - break; + return -EINVAL; } - if (kernel_load && fd < 0) { + fd = open(filename, O_WRONLY); + if (fd < 0) { PERROR(_("Unable to open %s - %s\n"), filename, strerror(errno)); error = -errno; - goto exit; + goto out; } - error = 0; - for (b = buffer; b ; b = next_profile_buffer(b + sizeof(header_version), bsize)) { - bsize = size - (b - buffer); - if (kernel_load) { - wsize = write(fd, b, bsize); - if (wsize < 0) { - error = -errno; - } else if (wsize < bsize) { - PERROR(_("%s: Unable to write entire profile entry\n"), - progname); - } + if (kernel_supports_setload) { + error = write_buffer(fd, buffer, size, true); + } else { + char *b, *next; + + error = 0; /* in case there are no profiles */ + for (b = buffer; b; b = next, size -= bsize) { + next = next_profile_buffer(b + sizeof(header_version), + size); + if (next) + bsize = next - b; + else + bsize = size; + error = write_buffer(fd, b, bsize, false); + if (error) + break; } } - if (kernel_load) close(fd); -exit: + close(fd); + +out: free(filename); + return error; } diff --git a/parser/parser_main.c b/parser/parser_main.c index eff551e0e..9a69ab8a3 100644 --- a/parser/parser_main.c +++ b/parser/parser_main.c @@ -826,6 +826,8 @@ static void get_match_string(void) { kernel_supports_network = 0; if (strstr(flags_string, "mount")) kernel_supports_mount = 1; + if (strstr(flags_string, "set_load")) + kernel_supports_setload = 1; return; }