On 03/21/2014 07:58 AM, Tyler Hicks wrote:
> 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 <[email protected]>
>> ---
>> 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.
>
yep
>> + } 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.
>
yep
--
AppArmor mailing list
[email protected]
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
