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

Steve Beattie steve at nxnw.org
Thu Feb 23 01:00:27 UTC 2012


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?

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

> +			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)?

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

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

> +			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 :-) ).

> +
> +		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().

> +		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)

-- 
Steve Beattie
<sbeattie at ubuntu.com>
http://NxNW.org/~steve/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20120222/b0e3986c/attachment.pgp>


More information about the AppArmor mailing list