[apparmor] [patch 06/26] Convert aare_rules into a class
Seth Arnold
seth.arnold at canonical.com
Wed Apr 16 02:06:56 UTC 2014
On Tue, Apr 15, 2014 at 10:22:13AM -0700, john.johansen at canonical.com wrote:
> This cleans things up a bit and fixes a bug where not all rules are
> getting properly counted so that the addition of policy_mediation
> rules fails to generate the policy dfa in some cases.
>
> Because the policy dfa is being generated correctly now we need to
> fix some tests to use the new -M flag to specify the expected features
> set of the test.
>
> Signed-off-by: John Johansen <john.johansen at canonical.com>
> Acked-by: Steve Beattie <steve at nxnw.org>
Most of this looked very straightforward, but I lost track of the
rule_count++ lines; I sort of expected to find a second instance of
rule_count++ somewhere. Should I have expected this?
Acked-by: Seth Arnold <seth.arnold at canonical.com>
Thanks
>
> ---
> parser/dbus.c | 24 ++++------
> parser/libapparmor_re/aare_rules.cc | 80 +++++++++++-------------------------
> parser/libapparmor_re/aare_rules.h | 34 +++++++--------
> parser/mount.c | 47 +++++++++------------
> parser/parser_regex.c | 79 +++++++++++++++--------------------
> parser/profile.cc | 4 -
> parser/profile.h | 6 +-
> 7 files changed, 114 insertions(+), 160 deletions(-)
>
> --- 2.9-test.orig/parser/dbus.c
> +++ 2.9-test/parser/dbus.c
> @@ -301,28 +301,26 @@
> }
>
> if (mode & AA_DBUS_BIND) {
> - if (!aare_add_rule_vec(prof.policy.rules, deny,
> - mode & AA_DBUS_BIND,
> - audit & AA_DBUS_BIND,
> - 2, vec, dfaflags))
> + if (!prof.policy.rules->add_rule_vec(deny, mode & AA_DBUS_BIND,
> + audit & AA_DBUS_BIND,
> + 2, vec, dfaflags))
> goto fail;
> }
> if (mode & (AA_DBUS_SEND | AA_DBUS_RECEIVE)) {
> - if (!aare_add_rule_vec(prof.policy.rules, deny,
> - mode & (AA_DBUS_SEND | AA_DBUS_RECEIVE),
> - audit & (AA_DBUS_SEND | AA_DBUS_RECEIVE),
> - 6, vec, dfaflags))
> + if (!prof.policy.rules->add_rule_vec(deny,
> + mode & (AA_DBUS_SEND | AA_DBUS_RECEIVE),
> + audit & (AA_DBUS_SEND | AA_DBUS_RECEIVE),
> + 6, vec, dfaflags))
> goto fail;
> }
> if (mode & AA_DBUS_EAVESDROP) {
> - if (!aare_add_rule_vec(prof.policy.rules, deny,
> - mode & AA_DBUS_EAVESDROP,
> - audit & AA_DBUS_EAVESDROP,
> - 1, vec, dfaflags))
> + if (!prof.policy.rules->add_rule_vec(deny,
> + mode & AA_DBUS_EAVESDROP,
> + audit & AA_DBUS_EAVESDROP,
> + 1, vec, dfaflags))
> goto fail;
> }
>
> - prof.policy.count++;
> return RULE_OK;
>
> fail:
> --- 2.9-test.orig/parser/libapparmor_re/aare_rules.cc
> +++ 2.9-test/parser/libapparmor_re/aare_rules.cc
> @@ -34,38 +34,20 @@
> #include "chfa.h"
> #include "../immunix.h"
>
> -struct aare_ruleset {
> - int reverse;
> - Node *root;
> -};
>
> -aare_ruleset_t *aare_new_ruleset(int reverse)
> -{
> - aare_ruleset_t *container = (aare_ruleset_t *) malloc(sizeof(aare_ruleset_t));
> - if (!container)
> - return NULL;
> -
> - container->root = NULL;
> - container->reverse = reverse;
> -
> - return container;
> -}
>
> -void aare_delete_ruleset(aare_ruleset_t *rules)
> +aare_rules::~aare_rules(void)
> {
> - if (rules) {
> - if (rules->root)
> - rules->root->release();
> - free(rules);
> - }
> + if (root)
> + root->release();
>
> aare_reset_matchflags();
> }
>
> -int aare_add_rule(aare_ruleset_t *rules, const char *rule, int deny,
> - uint32_t perms, uint32_t audit, dfaflags_t flags)
> +bool aare_rules::add_rule(const char *rule, int deny, uint32_t perms,
> + uint32_t audit, dfaflags_t flags)
> {
> - return aare_add_rule_vec(rules, deny, perms, audit, 1, &rule, flags);
> + return add_rule_vec(deny, perms, audit, 1, &rule, flags);
> }
>
> #define FLAGS_WIDTH 2
> @@ -94,9 +76,8 @@
> #undef RESET_FLAGS
> }
>
> -int aare_add_rule_vec(aare_ruleset_t *rules, int deny,
> - uint32_t perms, uint32_t audit,
> - int count, const char **rulev, dfaflags_t flags)
> +bool aare_rules::add_rule_vec(int deny, uint32_t perms, uint32_t audit,
> + int count, const char **rulev, dfaflags_t flags)
> {
> Node *tree = NULL, *accept;
> int exact_match;
> @@ -105,15 +86,15 @@
> assert(perms != 0);
>
> if (regex_parse(&tree, rulev[0]))
> - return 0;
> + return false;
> for (int i = 1; i < count; i++) {
> Node *subtree = NULL;
> Node *node = new CharNode(0);
> if (!node)
> - return 0;
> + return false;
> tree = new CatNode(tree, node);
> if (regex_parse(&subtree, rulev[i]))
> - return 0;
> + return false;
> tree = new CatNode(tree, subtree);
> }
>
> @@ -132,26 +113,15 @@
> exact_match = 0;
> }
>
> - if (rules->reverse)
> + if (reverse)
> flip_tree(tree);
>
> /* 0x7f == 4 bits x mods + 1 bit unsafe mask + 1 bit ix, + 1 pux after shift */
> #define EXTRACT_X_INDEX(perm, shift) (((perm) >> (shift + 7)) & 0x7f)
>
> -//if (perms & ALL_AA_EXEC_TYPE && (!perms & AA_EXEC_BITS))
> -// fprintf(stderr, "adding X rule without MAY_EXEC: 0x%x %s\n", perms, rulev[0]);
> -
> -//if (perms & ALL_EXEC_TYPE)
> -// fprintf(stderr, "adding X rule %s 0x%x\n", rulev[0], perms);
> -
> -//if (audit)
> -//fprintf(stderr, "adding rule with audit bits set: 0x%x %s\n", audit, rulev[0]);
> -
> -//if (perms & AA_CHANGE_HAT)
> -// fprintf(stderr, "adding change_hat rule %s\n", rulev[0]);
>
> -/* the permissions set is assumed to be non-empty if any audit
> - * bits are specified */
> + /* the permissions set is assumed to be non-empty if any audit
> + * bits are specified */
> accept = NULL;
> for (unsigned int n = 0; perms && n < (sizeof(perms) * 8); n++) {
> uint32_t mask = 1 << n;
> @@ -230,44 +200,44 @@
> cerr << "\n\n";
> }
>
> - if (rules->root)
> - rules->root = new AltNode(rules->root, new CatNode(tree, accept));
> + if (root)
> + root = new AltNode(root, new CatNode(tree, accept));
> else
> - rules->root = new CatNode(tree, accept);
> + root = new CatNode(tree, accept);
>
> - return 1;
> + rule_count++;
>
> + return true;
> }
>
> /* create a dfa from the ruleset
> * returns: buffer contain dfa tables, @size set to the size of the tables
> * else NULL on failure
> */
> -void *aare_create_dfa(aare_ruleset_t *rules, size_t *size,
> - dfaflags_t flags)
> +void *aare_rules::create_dfa(size_t *size, dfaflags_t flags)
> {
> char *buffer = NULL;
>
> - label_nodes(rules->root);
> + label_nodes(root);
> if (flags & DFA_DUMP_TREE) {
> cerr << "\nDFA: Expression Tree\n";
> - rules->root->dump(cerr);
> + root->dump(cerr);
> cerr << "\n\n";
> }
>
> if (flags & DFA_CONTROL_TREE_SIMPLE) {
> - rules->root = simplify_tree(rules->root, flags);
> + root = simplify_tree(root, flags);
>
> if (flags & DFA_DUMP_SIMPLE_TREE) {
> cerr << "\nDFA: Simplified Expression Tree\n";
> - rules->root->dump(cerr);
> + root->dump(cerr);
> cerr << "\n\n";
> }
> }
>
> stringstream stream;
> try {
> - DFA dfa(rules->root, flags);
> + DFA dfa(root, flags);
> if (flags & DFA_DUMP_UNIQ_PERMS)
> dfa.dump_uniq_perms("dfa");
>
> --- 2.9-test.orig/parser/libapparmor_re/aare_rules.h
> +++ 2.9-test/parser/libapparmor_re/aare_rules.h
> @@ -24,26 +24,24 @@
> #include <stdint.h>
>
> #include "apparmor_re.h"
> +#include "expr-tree.h"
>
> -#ifdef __cplusplus
> -extern "C" {
> -#endif
> +class aare_rules {
> + Node *root;
> +public:
> + int reverse;
> + int rule_count;
> + aare_rules(): root(NULL), reverse(0), rule_count(0) { };
> + aare_rules(int reverse): root(NULL), reverse(reverse), rule_count(0) { };
> + ~aare_rules();
> +
> + bool add_rule(const char *rule, int deny, uint32_t perms,
> + uint32_t audit, dfaflags_t flags);
> + bool add_rule_vec(int deny, uint32_t perms, uint32_t audit, int count,
> + const char **rulev, dfaflags_t flags);
> + void *create_dfa(size_t *size, dfaflags_t flags);
> +};
>
> -struct aare_ruleset;
> -
> -typedef struct aare_ruleset aare_ruleset_t;
> -
> -aare_ruleset_t *aare_new_ruleset(int reverse);
> -void aare_delete_ruleset(aare_ruleset_t *rules);
> -int aare_add_rule(aare_ruleset_t *rules, const char *rule, int deny, uint32_t perms,
> - uint32_t audit, dfaflags_t flags);
> -int aare_add_rule_vec(aare_ruleset_t *rules, int deny, uint32_t perms,
> - uint32_t audit, int count, const char **rulev,
> - dfaflags_t flags);
> -void *aare_create_dfa(aare_ruleset_t *rules, size_t *size, dfaflags_t flags);
> void aare_reset_matchflags(void);
>
> -#ifdef __cplusplus
> -}
> -#endif
> #endif /* __LIBAA_RE_RULES_H */
> --- 2.9-test.orig/parser/mount.c
> +++ 2.9-test/parser/mount.c
> @@ -631,9 +631,9 @@
> tmpallow = allow;
>
> /* rule for match without required data || data MATCH_CONT */
> - if (!aare_add_rule_vec(prof.policy.rules, deny, tmpallow,
> - audit | AA_AUDIT_MNT_DATA, 4,
> - vec, dfaflags))
> + if (!prof.policy.rules->add_rule_vec(deny, tmpallow,
> + audit | AA_AUDIT_MNT_DATA, 4,
> + vec, dfaflags))
> goto fail;
> count++;
>
> @@ -643,10 +643,9 @@
> if (!build_mnt_opts(optsbuf, opts))
> goto fail;
> vec[4] = optsbuf.c_str();
> - if (!aare_add_rule_vec(prof.policy.rules, deny,
> - allow,
> - audit | AA_AUDIT_MNT_DATA,
> - 5, vec, dfaflags))
> + if (!prof.policy.rules->add_rule_vec(deny, allow,
> + audit | AA_AUDIT_MNT_DATA,
> + 5, vec, dfaflags))
> goto fail;
> count++;
> }
> @@ -674,8 +673,8 @@
> if (!build_mnt_flags(flagsbuf, PATH_MAX, tmpflags, tmpinv_flags))
> goto fail;
> vec[3] = flagsbuf;
> - if (!aare_add_rule_vec(prof.policy.rules, deny, allow,
> - audit, 4, vec, dfaflags))
> + if (!prof.policy.rules->add_rule_vec(deny, allow, audit, 4, vec,
> + dfaflags))
> goto fail;
> count++;
> }
> @@ -703,8 +702,8 @@
> if (!build_mnt_flags(flagsbuf, PATH_MAX, tmpflags, tmpinv_flags))
> goto fail;
> vec[3] = flagsbuf;
> - if (!aare_add_rule_vec(prof.policy.rules, deny, allow,
> - audit, 4, vec, dfaflags))
> + if (!prof.policy.rules->add_rule_vec(deny, allow, audit, 4, vec,
> + dfaflags))
> goto fail;
> count++;
> }
> @@ -733,8 +732,8 @@
> if (!build_mnt_flags(flagsbuf, PATH_MAX, tmpflags, tmpinv_flags))
> goto fail;
> vec[3] = flagsbuf;
> - if (!aare_add_rule_vec(prof.policy.rules, deny, allow,
> - audit, 4, vec, dfaflags))
> + if (!prof.policy.rules->add_rule_vec(deny, allow, audit, 4, vec,
> + dfaflags))
> goto fail;
> count++;
> }
> @@ -773,9 +772,9 @@
> tmpallow = allow;
>
> /* rule for match without required data || data MATCH_CONT */
> - if (!aare_add_rule_vec(prof.policy.rules, deny, tmpallow,
> - audit | AA_AUDIT_MNT_DATA, 4,
> - vec, dfaflags))
> + if (!prof.policy.rules->add_rule_vec(deny, tmpallow,
> + audit | AA_AUDIT_MNT_DATA, 4,
> + vec, dfaflags))
> goto fail;
> count++;
>
> @@ -785,10 +784,9 @@
> if (!build_mnt_opts(optsbuf, opts))
> goto fail;
> vec[4] = optsbuf.c_str();
> - if (!aare_add_rule_vec(prof.policy.rules, deny,
> - allow,
> - audit | AA_AUDIT_MNT_DATA,
> - 5, vec, dfaflags))
> + if (!prof.policy.rules->add_rule_vec(deny, allow,
> + audit | AA_AUDIT_MNT_DATA,
> + 5, vec, dfaflags))
> goto fail;
> count++;
> }
> @@ -799,8 +797,8 @@
> if (!convert_entry(mntbuf, mnt_point))
> goto fail;
> vec[0] = mntbuf.c_str();
> - if (!aare_add_rule_vec(prof.policy.rules, deny, allow,
> - audit, 1, vec, dfaflags))
> + if (!prof.policy.rules->add_rule_vec(deny, allow, audit, 1, vec,
> + dfaflags))
> goto fail;
> count++;
> }
> @@ -813,8 +811,8 @@
> if (!clear_and_convert_entry(devbuf, device))
> goto fail;
> vec[1] = devbuf.c_str();
> - if (!aare_add_rule_vec(prof.policy.rules, deny, allow,
> - audit, 2, vec, dfaflags))
> + if (!prof.policy.rules->add_rule_vec(deny, allow, audit, 2, vec,
> + dfaflags))
> goto fail;
> count++;
> }
> @@ -823,7 +821,6 @@
> /* didn't actually encode anything */
> goto fail;
>
> - prof.policy.count++;
> return RULE_OK;
>
> fail:
> --- 2.9-test.orig/parser/parser_regex.c
> +++ 2.9-test/parser/parser_regex.c
> @@ -431,11 +431,11 @@
> prof->xmatch_size = 0;
> } else {
> /* build a dfa */
> - aare_ruleset_t *rule = aare_new_ruleset(0);
> - if (!rule)
> + aare_rules *rules = new aare_rules();
> + if (!rules)
> return FALSE;
> - if (!aare_add_rule(rule, tbuf.c_str(), 0, AA_MAY_EXEC, 0, dfaflags)) {
> - aare_delete_ruleset(rule);
> + if (!rules->add_rule(tbuf.c_str(), 0, AA_MAY_EXEC, 0, dfaflags)) {
> + delete rules;
> return FALSE;
> }
> if (prof->altnames) {
> @@ -450,15 +450,14 @@
> len = strlen(alt->name);
> if (len < prof->xmatch_len)
> prof->xmatch_len = len;
> - if (!aare_add_rule(rule, tbuf.c_str(), 0, AA_MAY_EXEC, 0, dfaflags)) {
> - aare_delete_ruleset(rule);
> + if (!rules->add_rule(tbuf.c_str(), 0, AA_MAY_EXEC, 0, dfaflags)) {
> + delete rules;
> return FALSE;
> }
> }
> }
> - prof->xmatch = aare_create_dfa(rule, &prof->xmatch_size,
> - dfaflags);
> - aare_delete_ruleset(rule);
> + prof->xmatch = rules->create_dfa(&prof->xmatch_size, dfaflags);
> + delete rules;
> if (!prof->xmatch)
> return FALSE;
> }
> @@ -466,7 +465,7 @@
> return TRUE;
> }
>
> -static int process_dfa_entry(aare_ruleset_t *dfarules, struct cod_entry *entry)
> +static int process_dfa_entry(aare_rules *dfarules, struct cod_entry *entry)
> {
> std::string tbuf;
> pattern_t ptype;
> @@ -498,13 +497,13 @@
> * entry of the pair
> */
> if (entry->deny && (entry->mode & AA_LINK_BITS)) {
> - if (!aare_add_rule(dfarules, tbuf.c_str(), entry->deny,
> - entry->mode & ~AA_LINK_BITS,
> - entry->audit & ~AA_LINK_BITS, dfaflags))
> + if (!dfarules->add_rule(tbuf.c_str(), entry->deny,
> + entry->mode & ~AA_LINK_BITS,
> + entry->audit & ~AA_LINK_BITS, dfaflags))
> return FALSE;
> } else if (entry->mode & ~AA_CHANGE_PROFILE) {
> - if (!aare_add_rule(dfarules, tbuf.c_str(), entry->deny, entry->mode,
> - entry->audit, dfaflags))
> + if (!dfarules->add_rule(tbuf.c_str(), entry->deny, entry->mode,
> + entry->audit, dfaflags))
> return FALSE;
> }
>
> @@ -526,7 +525,7 @@
> perms |= LINK_TO_LINK_SUBSET(perms);
> vec[1] = "/[^/].*";
> }
> - if (!aare_add_rule_vec(dfarules, entry->deny, perms, entry->audit & AA_LINK_BITS, 2, vec, dfaflags))
> + if (!dfarules->add_rule_vec(entry->deny, perms, entry->audit & AA_LINK_BITS, 2, vec, dfaflags))
> return FALSE;
> }
> if (entry->mode & AA_CHANGE_PROFILE) {
> @@ -545,12 +544,12 @@
> vec[index++] = tbuf.c_str();
>
> /* regular change_profile rule */
> - if (!aare_add_rule_vec(dfarules, 0, AA_CHANGE_PROFILE | AA_ONEXEC, 0, index - 1, &vec[1], dfaflags))
> + if (!dfarules->add_rule_vec(0, AA_CHANGE_PROFILE | AA_ONEXEC, 0, index - 1, &vec[1], dfaflags))
> return FALSE;
> /* onexec rules - both rules are needed for onexec */
> - if (!aare_add_rule_vec(dfarules, 0, AA_ONEXEC, 0, 1, vec, dfaflags))
> + if (!dfarules->add_rule_vec(0, AA_ONEXEC, 0, 1, vec, dfaflags))
> return FALSE;
> - if (!aare_add_rule_vec(dfarules, 0, AA_ONEXEC, 0, index, vec, dfaflags))
> + if (!dfarules->add_rule_vec(0, AA_ONEXEC, 0, index, vec, dfaflags))
> return FALSE;
> }
> return TRUE;
> @@ -560,15 +559,12 @@
> {
> int ret = TRUE;
> struct cod_entry *entry;
> - int count = 0;
>
> list_for_each(prof->entries, entry) {
> if (!process_dfa_entry(prof->dfa.rules, entry))
> ret = FALSE;
> - count++;
> }
>
> - prof->dfa.count = count;
> return ret;
> }
>
> @@ -579,17 +575,17 @@
> if (!process_profile_name_xmatch(prof))
> goto out;
>
> - prof->dfa.rules = aare_new_ruleset(0);
> + prof->dfa.rules = new aare_rules();
> if (!prof->dfa.rules)
> goto out;
>
> if (!post_process_entries(prof))
> goto out;
>
> - if (prof->dfa.count > 0) {
> - prof->dfa.dfa = aare_create_dfa(prof->dfa.rules, &prof->dfa.size,
> - dfaflags);
> - aare_delete_ruleset(prof->dfa.rules);
> + if (prof->dfa.rules->rule_count > 0) {
> + prof->dfa.dfa = prof->dfa.rules->create_dfa(&prof->dfa.size,
> + dfaflags);
> + delete prof->dfa.rules;
> prof->dfa.rules = NULL;
> if (!prof->dfa.dfa)
> goto out;
> @@ -683,7 +679,7 @@
> {
> int error = -1;
>
> - prof->policy.rules = aare_new_ruleset(0);
> + prof->policy.rules = new aare_rules();
> if (!prof->policy.rules)
> goto out;
>
> @@ -694,26 +690,21 @@
> * to be supported
> */
>
> - if (kernel_supports_mount) {
> - if (!aare_add_rule(prof->policy.rules, mediates_mount, 0, AA_MAY_READ, 0, dfaflags))
> + if (kernel_supports_mount &&
> + !prof->policy.rules->add_rule(mediates_mount, 0, AA_MAY_READ, 0, dfaflags))
> goto out;
> - prof->policy.count++;
> - }
> - if (kernel_supports_dbus) {
> - if (!aare_add_rule(prof->policy.rules, mediates_dbus, 0, AA_MAY_READ, 0, dfaflags))
> - goto out;
> - prof->policy.count++;
> - }
> - if (prof->policy.count > 0) {
> - prof->policy.dfa = aare_create_dfa(prof->policy.rules,
> - &prof->policy.size,
> - dfaflags);
> - aare_delete_ruleset(prof->policy.rules);
> + if (kernel_supports_dbus &&
> + !prof->policy.rules->add_rule(mediates_dbus, 0, AA_MAY_READ, 0, dfaflags))
> + goto out;
> + if (prof->policy.rules->rule_count > 0) {
> + prof->policy.dfa = prof->policy.rules->create_dfa(&prof->policy.size, dfaflags);
> + delete prof->policy.rules;
> +
> prof->policy.rules = NULL;
> if (!prof->policy.dfa)
> goto out;
> } else {
> - aare_delete_ruleset(prof->policy.rules);
> + delete prof->policy.rules;
> prof->policy.rules = NULL;
> }
>
> @@ -722,7 +713,7 @@
> error = 0;
>
> out:
> - aare_delete_ruleset(prof->policy.rules);
> + delete prof->policy.rules;
> prof->policy.rules = NULL;
>
> return error;
> --- 2.9-test.orig/parser/profile.cc
> +++ 2.9-test/parser/profile.cc
> @@ -67,11 +67,11 @@
> for (RuleList::iterator i = rule_ents.begin(); i != rule_ents.end(); i++)
> delete *i;
> if (dfa.rules)
> - aare_delete_ruleset(dfa.rules);
> + delete dfa.rules;
> if (dfa.dfa)
> free(dfa.dfa);
> if (policy.rules)
> - aare_delete_ruleset(policy.rules);
> + delete policy.rules;
> if (policy.dfa)
> free(policy.dfa);
> if (xmatch)
> --- 2.9-test.orig/parser/profile.h
> +++ 2.9-test/parser/profile.h
> @@ -18,6 +18,7 @@
>
> #include "parser.h"
> #include "rule.h"
> +#include "libapparmor_re/aare_rules.h"
>
> class Profile;
>
> @@ -120,12 +121,11 @@
> };
>
> struct dfa_stuff {
> - aare_ruleset_t *rules;
> - int count;
> + aare_rules *rules;
> void *dfa;
> size_t size;
>
> - dfa_stuff(void): rules(NULL), count(0), dfa(NULL), size(0) { }
> + dfa_stuff(void): rules(NULL), dfa(NULL), size(0) { }
> };
>
> class Profile {
>
>
> --
> AppArmor mailing list
> AppArmor at lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20140415/0d29eb28/attachment.pgp>
More information about the AppArmor
mailing list