[apparmor] [Patch] Bug 888077 - aliases being partially applied
Seth Arnold
seth.arnold at canonical.com
Tue Jul 9 03:03:34 UTC 2013
On Mon, Jul 08, 2013 at 02:06:42AM -0700, John Johansen wrote:
> Below is a mostly complete patch for https://bugs.launchpad.net/apparmor/+bug/888077
>
> It is currently missing support for link and mount rules. This patch is
> done against the 2.8 branch, and the question is, is this something we
> want in 2.8.2/3. It is rather large, and I want to rework it some before
> it goes to the 2.9/3.0 branch. So the two branch will differ on this
> point if we pull it into 2.8
I'm thinking this is better 2.8.3 material than 2.8.2. I've given it a
read and while I didn't spot any problems, I don't understand most of
what it does.
I'd feel better if someone who needs this is in a position to test it
out and report back. (Even without link and mount I think it'd be
useful.)
A few comments are inline..
>
> diff --git a/parser/libapparmor_re/aare_rules.cc b/parser/libapparmor_re/aare_rules.cc
> index 45664c0..5e6d652 100644
> --- a/parser/libapparmor_re/aare_rules.cc
> +++ b/parser/libapparmor_re/aare_rules.cc
> @@ -76,6 +76,9 @@ DenyMatchFlag *deny_flags[FLAGS_WIDTH][MATCH_FLAGS_SIZE];
> MatchFlag *exec_match_flags[FLAGS_WIDTH][EXEC_MATCH_FLAGS_SIZE]; /* mods + unsafe + ix + pux * u::o */
> ExactMatchFlag *exact_match_flags[FLAGS_WIDTH][EXEC_MATCH_FLAGS_SIZE]; /* mods + unsafe + ix + pux *u::o */
>
> +typedef map<pair<const char *, const char *>, AliasToNode *> AliasMap;
> +AliasMap alias_map;
> +
> extern "C" void aare_reset_matchflags(void)
> {
> uint32_t i, j;
> @@ -92,6 +95,42 @@ extern "C" void aare_reset_matchflags(void)
> RESET_FLAGS(exec_match_flags, EXEC_MATCH_FLAGS_SIZE);
> RESET_FLAGS(exact_match_flags, EXEC_MATCH_FLAGS_SIZE);
> #undef RESET_FLAGS
> + alias_map.clear();
> +}
> +
> +extern "C" int aare_add_alias(aare_ruleset_t *rules, const char *from,
> + const char *to)
> +{
> + Node *tree_from = NULL, *tree_to = NULL;
> + AliasToNode *node = NULL;
> +
> + AliasMap::iterator i = alias_map.find(make_pair(from, to));
> + if (i == alias_map.end()) {
> + node = new AliasToNode();
> + alias_map.insert(make_pair(make_pair(from, to), node));
> + } else {
> + node = i->second;
> + }
> +
> + if (regex_parse(&tree_from, from))
> + return 0;
> + if (rules->reverse)
> + flip_tree(tree_from);
> + if (rules->root)
> + rules->root = new AltNode(rules->root, new CatNode(tree_from, &node->from));
> + else
> + rules->root = new CatNode(tree_from, &node->from);
If flip_tree() isn't used in the current code, I'd vote for removing it
from this function and the other location that has it -- as well as the
aare_ruleset_t reverse handling. (Consider: why were two calls added in
their current locations, rather than one call just before the "return 1;"
statement? I just don't know how to tell if it is correct or not.)
> +
> + if (regex_parse(&tree_to, to))
> + return 0;
> + if (rules->reverse)
> + flip_tree(tree_to);
> + if (rules->root)
> + rules->root = new AltNode(rules->root, new CatNode(tree_to, node));
> + else
> + rules->root = new CatNode(tree_to, node);
> +
> + return 1;
> }
>
> extern "C" int aare_add_rule_vec(aare_ruleset_t *rules, int deny,
> diff --git a/parser/libapparmor_re/aare_rules.h b/parser/libapparmor_re/aare_rules.h
> index 33e554c..c053530 100644
> --- a/parser/libapparmor_re/aare_rules.h
> +++ b/parser/libapparmor_re/aare_rules.h
> @@ -40,6 +40,7 @@ int aare_add_rule(aare_ruleset_t *rules, char *rule, int deny, uint32_t perms,
> int aare_add_rule_vec(aare_ruleset_t *rules, int deny, uint32_t perms,
> uint32_t audit, int count, char **rulev,
> dfaflags_t flags);
> +int aare_add_alias(aare_ruleset_t *rules, const char *from, const char *to);
> void *aare_create_dfa(aare_ruleset_t *rules, size_t *size, dfaflags_t flags);
> void aare_reset_matchflags(void);
>
> diff --git a/parser/libapparmor_re/apparmor_re.h b/parser/libapparmor_re/apparmor_re.h
> index 186899c..1fdb915 100644
> --- a/parser/libapparmor_re/apparmor_re.h
> +++ b/parser/libapparmor_re/apparmor_re.h
> @@ -30,6 +30,10 @@ typedef enum dfaflags {
> DFA_CONTROL_REMOVE_UNREACHABLE = 1 << 7,
> DFA_CONTROL_TRANS_HIGH = 1 << 8,
>
> + DFA_COMPAT_OLD_ALIAS = 1 << 10,
> +
> + DFA_DUMP_ALIAS_INFO = 1 << 11,
> + DFA_DUMP_PRE_ALIAS = 1 << 12,
> DFA_DUMP_MIN_PARTS = 1 << 13,
> DFA_DUMP_UNIQ_PERMS = 1 << 14,
> DFA_DUMP_MIN_UNIQ_PERMS = 1 << 15,
> diff --git a/parser/libapparmor_re/expr-tree.h b/parser/libapparmor_re/expr-tree.h
> index 29c2ded..651a02d 100644
> --- a/parser/libapparmor_re/expr-tree.h
> +++ b/parser/libapparmor_re/expr-tree.h
> @@ -205,6 +205,7 @@ public:
> void compute_lastpos() { lastpos.insert(this); }
> virtual void follow(Cases &cases) = 0;
> virtual int is_accept(void) = 0;
> + virtual int is_postprocess(void) = 0;
> };
>
> /* common base class for all the different classes that contain
> @@ -214,6 +215,7 @@ class CNode: public ImportantNode {
> public:
> CNode(): ImportantNode() { }
> int is_accept(void) { return false; }
> + int is_postprocess(void) { return false; }
> };
>
> /* Match one specific character (/c/). */
> @@ -358,35 +360,6 @@ public:
> ostream &dump(ostream &os) { return os << "."; }
> };
>
> -/**
> - * Indicate that a regular expression matches. An AcceptNode itself
> - * doesn't match anything, so it will never generate any transitions.
> - */
> -class AcceptNode: public ImportantNode {
> -public:
> - AcceptNode() { }
> - int is_accept(void) { return true; }
> - void release(void)
> - {
> - /* don't delete AcceptNode via release as they are shared, and
> - * will be deleted when the table the are stored in is deleted
> - */
> - }
> -
> - void follow(Cases &cases __attribute__ ((unused)))
> - {
> - /* Nothing to follow. */
> - }
> -
> - /* requires accept nodes to be common by pointer */
> - int eq(Node *other)
> - {
> - if (dynamic_cast<AcceptNode *>(other))
> - return (this == other);
> - return 0;
> - }
> -};
> -
> /* Match a node zero or more times. (This is a unary operator.) */
> class StarNode: public OneChildNode {
> public:
> @@ -523,6 +496,55 @@ public:
> }
> };
>
> +class SharedNode: public ImportantNode {
> +public:
> + SharedNode() { }
> + void release(void)
> + {
> + /* don't delete SharedNodes via release as they are shared, and
> + * will be deleted when the table they are stored in is deleted
> + */
> + }
> +
> + void follow(Cases &cases __attribute__ ((unused)))
> + {
> + /* Nothing to follow. */
> + }
> +
> + /* requires shared nodes to be common by pointer */
> + int eq(Node *other) { return (this == other); }
In the old AcceptNode class, there was a dynamic_cast<> around other --
is it intentional that the dynamic_cast<> has been removed in this
slightly modified class?
> +};
> +
> +/**
> + * Indicate that a regular expression matches. An AcceptNode itself
> + * doesn't match anything, so it will never generate any transitions.
> + */
> +class AcceptNode: public SharedNode {
> +public:
> + AcceptNode() { }
> + int is_accept(void) { return true; }
> + int is_postprocess(void) { return false; }
> +};
> +
> +class MatchFlag: public AcceptNode {
> +public:
> + MatchFlag(uint32_t flag, uint32_t audit): flag(flag), audit(audit) { }
> + ostream &dump(ostream &os) { return os << '<' << flag << '>'; }
> +
> + uint32_t flag;
> + uint32_t audit;
> +};
> +
> +class ExactMatchFlag: public MatchFlag {
> +public:
> + ExactMatchFlag(uint32_t flag, uint32_t audit): MatchFlag(flag, audit) {}
> +};
> +
> +class DenyMatchFlag: public MatchFlag {
> +public:
> + DenyMatchFlag(uint32_t flag, uint32_t quiet): MatchFlag(flag, quiet) {}
> +};
> +
> /* Traverse the syntax tree depth-first in an iterator-like manner. */
> class depth_first_traversal {
> stack<Node *>pos;
> @@ -574,24 +596,4 @@ void label_nodes(Node *root);
> unsigned long hash_NodeSet(NodeSet *ns);
> void flip_tree(Node *node);
>
> -
> -class MatchFlag: public AcceptNode {
> -public:
> - MatchFlag(uint32_t flag, uint32_t audit): flag(flag), audit(audit) { }
> - ostream &dump(ostream &os) { return os << '<' << flag << '>'; }
> -
> - uint32_t flag;
> - uint32_t audit;
> -};
> -
> -class ExactMatchFlag: public MatchFlag {
> -public:
> - ExactMatchFlag(uint32_t flag, uint32_t audit): MatchFlag(flag, audit) {}
> -};
> -
> -class DenyMatchFlag: public MatchFlag {
> -public:
> - DenyMatchFlag(uint32_t flag, uint32_t quiet): MatchFlag(flag, quiet) {}
> -};
> -
> #endif /* __LIBAA_RE_EXPR */
> diff --git a/parser/libapparmor_re/hfa.cc b/parser/libapparmor_re/hfa.cc
> index e779dd3..34a18d9 100644
> --- a/parser/libapparmor_re/hfa.cc
> +++ b/parser/libapparmor_re/hfa.cc
> @@ -73,10 +73,13 @@ ostream &operator<<(ostream &os, const State &state)
> return os;
> }
>
> -static void split_node_types(NodeSet *nodes, NodeSet **anodes, NodeSet **nnodes
> -)
> +typedef set<PostProcessNode *> PostNodeSet;
> +
> +static void split_node_types(NodeSet *nodes, NodeSet **anodes, NodeSet **nnodes,
> + PostNodeSet **postnodes)
> {
> *anodes = *nnodes = NULL;
> + *postnodes = NULL;
> for (NodeSet::iterator i = nodes->begin(); i != nodes->end(); ) {
> if ((*i)->is_accept()) {
> if (!*anodes)
> @@ -84,20 +87,21 @@ static void split_node_types(NodeSet *nodes, NodeSet **anodes, NodeSet **nnodes
> (*anodes)->insert(*i);
> NodeSet::iterator k = i++;
> nodes->erase(k);
> + } else if ((*i)->is_postprocess()) {
> + if (!*postnodes)
> + *postnodes = new PostNodeSet;
> + (*postnodes)->insert((PostProcessNode *) *i);
> + NodeSet::iterator k = i++;
> + nodes->erase(k);
This erases the next node. But I don't know why. :)
.. From here on out, most of it goes right over my head.
Again, it all -looks- fine, but I don't understand what I'm looking at.
Sorry.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20130708/607944d7/attachment.pgp>
More information about the AppArmor
mailing list