[apparmor] [Patch] Bug 888077 - aliases being partially applied
John Johansen
john.johansen at canonical.com
Tue Jul 9 06:55:24 UTC 2013
On 07/08/2013 08:03 PM, Seth Arnold wrote:
> 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
So 2.8.3 is fine, I don't want to hold up 2.8.2 on this
> read and while I didn't spot any problems, I don't understand most of
> what it does.
>
Hehe I'll try to get some documentation together on this but the
basis is this.
Alias remap a subsection of a dfa from one to another
That is say we do an alias from /home/ to /usr/
everything that is matched by /home/** can now be matched by using /usr/
as the prefix. So the patch creates a new type of accept node that is
a pair (alias from, and alias to)
to create an alias, for each unique alias from and to pair we add two
rules to the expression tree
so for
alias /from/ -> /to/,
we add
/from/ (alias_from_1),
/to/ (alias_from_2),
we then create the dfa, where each state in the dfa is represented by
a set of nodes.
So the set of states that have the alias_from_1 permission on them
are the set of states that match to /from/
Similarly for /to/.
During the construction we split out nodes into none accepting, accepting
and our new post accepting nodes. We still only generate states based
off of the none accepting and accepting nodes. This means that post
nodes don't change the dfa that is created, but we detect them and
add the create state to a list in the node to track which states the
node appears on.
This means we don't have to do something special to store these on the
state (so they are easier to throw away), and we don't have to walk
all states to discover the pairings. We have a list of states to pairings
out of the construction.
So after the first do_work_queue pass we have a valid dfa, and our postnode
pairings. We now begin to rewrite the dfa.
We need to add all the transitions from the /from/ states to the /to/
states.
At this point we have state and still have expr node information that was
used to create a state. So say we have a transition on 'j' to state {10}
which is composed of a set of nodes.
For a transition (say 'j') from the alias_from state we look up the state
'j' transitions to. That state still has its proto state (which is the
set of anodes and nnodes that resulted in its creation), we compute the
set that is the union of the 'j' transition for the from_alias state and
the to_alias state.
We insert this resultant "proto state (again pair of anodes, and nnodes)
into the protostate table. And this will either give us an existing state
which we use or a new state which we use but also add to the work_queue
as it may create its own set of new states to satisfy its transitions.
We don't remove the transitions from the alias_from state as our alias
rule still allows those paths (our rewrite rule would remove those
transitions). And any states that are now unreachable (do to rewrite
rule or alias_to no longer pointing to them) are cleaned up in
unreachable state removal and minimization.
We have to rerun the do_work_queue routine again to generate any subsequent
new states caused by the alias/rewrite. And now we have a dfa that we can
throw away the node information on.
> 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.)
>
yes we need to add some regression tests. For 2.8.x we can only add them
to the kernel regression tests unless we want to do text match of dumped
dfas.
> 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.)
>
No, at least not yet. This is a feature I'd like to revisit and experiment
with. It could save us the run time costs of buffer allocation in some cases.
In the end it may not be worth it, but I am not ready to get rid of it just
yet.
>
>> +
>> + 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?
>
yep its not needed we are doing a straight pointer comparison. Having the
dynamic_cast just is adding a lot of work that just gets thrown away
whether the cast succeeds or not.
>> +};
>> +
>> +/**
>> + * 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. :)
>
nope, the current node. k is assigned the value of i, and then i is incremented
> .. From here on out, most of it goes right over my head.
>
we erase the current *node from the set because its a pointer to the actual node
and we have just moved it from the nodes set to the postnodes set. So think
just moving a pointer between two sets.
> Again, it all -looks- fine, but I don't understand what I'm looking at.
>
heh, no worries hopefully my explanation helps a bit, and thanks for the review
More information about the AppArmor
mailing list