[apparmor] [patch] fix load of binary cache files

John Johansen john.johansen at canonical.com
Fri Mar 21 18:03:13 UTC 2014


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 <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.
> 
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




More information about the AppArmor mailing list