[apparmor] [PATCH 1/3] Generate the features list from the features directory

[John Johansen]

On 02/22/2012 05:00 PM, Steve Beattie wrote:
> On Wed, Feb 22, 2012 at 03:04:56PM -0800, John Johansen wrote:
>> Newer versions of AppArmor use a features directory instead of a file
>> update the parser to use this to determine features and match string
>>
>> This is just a first pass at this to get things up quickly.  A much
>> more comprehensive rework that can parse and use the full information
>> set is needed.
>>
>> Signed-off-by: John Johansen <john.johansen at canonical.com>
>> ---
>>  parser/parser.h        |    1 +
>>  parser/parser_common.c |    1 +
>>  parser/parser_main.c   |  121 +++++++++++++++++++++++++++++++++++++++++++++++-
>>  3 files changed, 122 insertions(+), 1 deletions(-)
>>
>> diff --git a/parser/parser.h b/parser/parser.h
>> index 8d3ef6c..7be0f83 100644
>> --- a/parser/parser.h
>> +++ b/parser/parser.h
>> @@ -247,6 +247,7 @@ extern int perms_create;
>>  extern int net_af_max_override;
>>  extern int kernel_load;
>>  extern int kernel_supports_network;
>> +extern int kernel_supports_mount;
>>  extern int flag_changehat_version;
>>  extern int conf_verbose;
>>  extern int conf_quiet;
>> diff --git a/parser/parser_common.c b/parser/parser_common.c
>> index df78a10..572eaad 100644
>> --- a/parser/parser_common.c
>> +++ b/parser/parser_common.c
>> @@ -27,6 +27,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_network = 1;        /* kernel supports network rules */
>> +int kernel_supports_mount = 0;		/* kernel supports mount rules */
> 
> (minor nit) tabs vs whitespace?
> 
yep, I used tab and it lined up with what was there, which is using whitespace :/

>>  int flag_changehat_version = FLAG_CHANGEHAT_1_5;
>>  int conf_verbose = 0;
>>  int conf_quiet = 0;
>> diff --git a/parser/parser_main.c b/parser/parser_main.c
>> index 721582d..94ad2b9 100644
>> --- a/parser/parser_main.c
>> +++ b/parser/parser_main.c
>> @@ -30,6 +30,7 @@
>>  #include <mntent.h>
>>  #include <libintl.h>
>>  #include <locale.h>
>> +#include <dirent.h>
>>  #define _(s) gettext(s)
>>  
>>  /* enable the following line to get voluminous debug info */
>> @@ -667,17 +668,134 @@ int have_enough_privilege(void)
>>  	return 0;
>>  }
>>  
>> +static char *handle_features_dir(const char *filename, char **buffer, int size,
>> +				 char *pos)
>> +{
>> +	DIR *dir = NULL;
>> +	char *dirent_path = NULL;
>> +	struct dirent *dirent;
>> +	struct stat my_stat;
>> +	int len;
>> +
>> +	PDEBUG("Opened features directory \"%s\"\n", filename);
>> +	if (!(dir = opendir(filename))) {
>> +		PDEBUG("opendir failed '%s'", filename);
>> +		exit(1);
>> +	}
>> +
>> +	while ((dirent = readdir(dir)) != NULL) {
>> +		int name_len, remaining;
>> +		/* skip dotfiles silently. */
>> +		if (dirent->d_name[0] == '.')
>> +			continue;
>> +
>> +		if (dirent_path)
>> +			free(dirent_path);
>> +		if (asprintf(&dirent_path, "%s/%s", filename, dirent->d_name) < 0)
>> +		{
>> +			PERROR(_("Memory allocation error."));
>> +			exit(1);
>> +		}
>> +
>> +		name_len = strlen(dirent->d_name);
>> +		if (!name_len)
>> +			continue;
>> +
>> +		if (stat(dirent_path, &my_stat)) {
>> +			PERROR(_("stat failed for '%s'"), dirent_path);
>> +			exit(1);
>> +		}
>> +
>> +		remaining = size - (pos - *buffer);
>> +		len = snprintf(pos, remaining, "%s { ", dirent->d_name);
>> +		if (len > remaining) {
> 
> I think there's an off by one error here, in that if len == remaining,
> then the output has been truncated, as the value returned by snprintf()
> is the number of characters that would have been written not including
> the trailing \0 character. When len == remaining, then it has been
> truncated by 1 character.
> 
yep

>> +			PERROR(_("Feature buffer full."));
>> +			exit(1);
>> +		}
>> +		pos += len;
>> +
>> +		if (S_ISREG(my_stat.st_mode)) {
>> +			int file;
>> +			int remaining = size - (pos - *buffer);
>> +			if (!(file = open(dirent_path, O_RDONLY))) {
>> +				PDEBUG("Could not open '%s' in '%s'", dirent_path, filename);
>> +				exit(1);
>> +				break;
>> +			}
>> +			PDEBUG("Opened features \"%s\" in \"%s\"\n", dirent_path, filename);
>> +			if (my_stat.st_size > remaining) {
>> +				PERROR(_("Feature buffer full."));
>> +				exit(1);
>> +			}
>> +
>> +			do {
>> +				len = read(file, pos, remaining);
>> +				if (pos > 0) {
> 
> Is this test correct? Given that pos is a pointer to a character
> string, it's, uh, likely to not be 0. Did you mean (len > 0)?
> 
indeed

>> +					remaining -= len;
>> +					pos += len;
>> +					*pos = 0;
>> +				}
>> +			} while (len > 0);
>> +			if (len < 0) {
>> +				PDEBUG("Error reading feature file '%s'\n",
>> +				       dirent_path);
>> +				exit(1);
>> +			}
>> +			close(file);
>> +
>> +		} else if (S_ISDIR(my_stat.st_mode)) {
>> +			pos = handle_features_dir(dirent_path, buffer, size,
>> +						  pos);
>> +			if (!pos)
>> +				break;
>> +
>> +		}
>> +
>> +		remaining = size - (pos - *buffer);
>> +		len = snprintf(pos, remaining, " }\n");
> 
> With the newline, I think your formatting might end up a little goofy
> if you have multiply nested directories.
> 
oh it is, however with out its goofy too as the file reads have a new line at their
end as well.  I thought about stripping it but first a first pass I decided I didn't
care.

As I said what we really need to do is move to a more complete parse of the file

>> +		if (len > remaining) {
> 
> Same problem here as the earlier snprintf case. It might be useful to
> abstract appending to the feature string to a helper function, one that
> you can write unit tests for. (Hooray for handling strings in C. Sigh.)
> 
yeah, truth be told I actually considered that on the first pass and skipped
it because I did feel like looking up varags syntax :/

>> +			PERROR(_("Feature buffer full."));
>> +			exit(1);
>> +		}
>> +		pos += len;
>> +	}
>> +	if (dirent_path)
>> +		free(dirent_path);
>> +	closedir(dir);
>> +
>> +	return pos;
>> +}
>> +
>>  /* match_string == NULL --> no match_string available
>>     match_string != NULL --> either a matching string specified on the
>>     command line, or the kernel supplied a match string */
>>  static void get_match_string(void) {
>>  
>>  	FILE *ms = NULL;
>> +	struct stat stat_file;
>>  
>>  	/* has process_args() already assigned a match string? */
>>  	if (match_string)
>>  		goto out;
>>  
>> +	if (stat(FLAGS_FILE, &stat_file) == -1)
>> +		goto out;
>> +
>> +	if (S_ISDIR(stat_file.st_mode)) {
>> +		/* if we have a features directory default to */
>> +		regex_type = AARE_DFA;
>> +		perms_create = 1;
>> +		flag_changehat_version = FLAG_CHANGEHAT_1_4;
> 
> Should this be 1.5 or 1.4? The default setting is 1.5 and the comments
> around the checks for FLAG_CHANGEHAT_1_4 indicate it to be an apparmor
> 2.3 specific thing (though I know better than to trust comments :-) ).
> 
heh it is 1.5, which means we can drop it as that is the default.

>> +
>> +		flags_string = malloc(1024);
>> +		handle_features_dir(FLAGS_FILE, &flags_string, 2048, flags_string);
> 
> Kees already highlighted the disparity, but that's a reason to use
> a #defined value or statically allocated memory + sizeof().
> 
yep

>> +		if (strstr(flags_string, "network"))
>> +			kernel_supports_network = 1;
>> +		if (strstr(flags_string, "mount"))
>> +			kernel_supports_mount = 1;
>> +		return;
>> +	}
>> +
>>  	ms = fopen(MATCH_STRING, "r");
>>  	if (!ms)
>>  		goto out;
>> @@ -718,7 +836,8 @@ static void get_flags_string(char **flags, char *flags_file) {
>>  	FILE *f = NULL;
>>  
>>  	/* abort if missing or already set */
>> -	if (!flags || *flags) return;
>> +	if (!flags || *flags)
>> +		return;
> 
> *cough* Thank you. :-)
> 
>>  
>>  	f = fopen(flags_file, "r");
>>  	if (!f)
> 
> 
>