[apparmor] [patch] fix load of binary cache files
Tyler Hicks
tyhicks at canonical.com
Fri Mar 21 14:58:01 UTC 2014
On 2014-03-21 04:25:02, John Johansen wrote:
> Fix profile loads from cache files that contain multiple profiles
>
> 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"
> <sth0R> [82932.058388] type=1400 audit(1395415826.937:616):
> apparmor="STATUS" operation="profile_load" name="DEFAULT_URI" pid=8631
> comm="apparmor_parser"
> <sth0R> [82932.058391] type=1400 audit(1395415826.937:617):
> apparmor="STATUS" operation="profile_load"
> name="HANDLING_UNTRUSTED_INPUT" pid=8631 comm="apparmor_parser"
> <sth0R> [82932.058394] type=1400 audit(1395415826.937:618):
> apparmor="STATUS" operation="profile_load" name="phpsysinfo" pid=8631
> comm="apparmor_parser"
> <sth0R> [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"
> <sth0R> [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 <john.johansen at canonical.com>
> ---
> parser/parser.h | 1 +
> parser/parser_common.c | 1 +
> parser/parser_interface.c | 43 ++++++++++++++++++++++++++++++++-----------
> parser/parser_main.c | 2 ++
> 4 files changed, 36 insertions(+), 11 deletions(-)
>
> --- 2.9-test.orig/parser/parser.h
> +++ 2.9-test/parser/parser.h
> @@ -222,6 +222,7 @@
> 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 kernel_supports_dbus;
> --- 2.9-test.orig/parser/parser_common.c
> +++ 2.9-test/parser/parser_common.c
> @@ -25,6 +25,7 @@
> 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 kernel_supports_dbus = 0; /* kernel supports dbus rules */
> --- 2.9-test.orig/parser/parser_interface.c
> +++ 2.9-test/parser/parser_interface.c
> @@ -871,7 +871,6 @@
> int fd = -1;
> int error = -ENOMEM, wsize, bsize;
> char *filename = NULL;
> - char *b;
>
> switch (option) {
> case OPTION_ADD:
> @@ -898,19 +897,41 @@
> }
>
> 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) {
> + wsize = write(fd, buffer, size);
> + if (wsize == 0)
> + error = -errno;
> + else if (wsize < size)
> + PERROR(_("%s: Unable to write entire profile set\n"),
> + progname);
error should be set to something non-zero in this situation, shouldn't
it? If we just failed to load the entire profile, exiting with 0 is
papering over a potentially large system security issue.
> + } else {
> + char *b, *next;
> +
> + 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;
> + if (kernel_load) {
> + wsize = write(fd, b, bsize);
> + if (wsize < 0) {
> + error = -errno;
> + PERROR(_("%s: Unable to write profile\n"),
> + progname);
> + break;
> + } else if (wsize < bsize) {
> + PERROR(_("%s: Unable to write entire profile entry\n"),
> + progname);
Same here.
This one isn't quite as cut and dry as the one above, but I don't see
the point in continuing on and trying to load the next buffer. There's a
decent chance that the profile in the next buffer might depend on the
profile in the buffer that failed to load.
I think we should set error and break here.
Tyler
> + }
> }
> }
> }
> - if (kernel_load) close(fd);
> + if (kernel_load)
> + close(fd);
> +
> exit:
> free(filename);
> return error;
> --- 2.9-test.orig/parser/parser_main.c
> +++ 2.9-test/parser/parser_main.c
> @@ -761,6 +761,8 @@
>
> flags_string = (char *) malloc(FLAGS_STRING_SIZE);
> handle_features_dir(FLAGS_FILE, &flags_string, FLAGS_STRING_SIZE, flags_string);
> + if (strstr(flags_string, "set_load"))
> + kernel_supports_setload = 1;
> if (strstr(flags_string, "network"))
> kernel_supports_network = 1;
> else
>
> --
> AppArmor mailing list
> AppArmor at lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20140321/47b948cf/attachment.pgp>
More information about the AppArmor
mailing list