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.
signature.asc
Description: Digital signature
-- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor