[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