[apparmor] GSoC review r30

[John Johansen]

On 08/01/2013 02:59 PM, Christian Boltz wrote:
> Hello,
> 
> the review for r30 is attached - it had lots of new code (and
> interesting[tm] regexes) - therefore I have several notes about it ;-)
> 
> @John: The review contains some questions for you - can you please answer
> them?
> 
> 


=== modified file 'apparmor/aa.py'
--- apparmor/aa.py	2013-07-28 02:59:59 +0000
+++ apparmor/aa.py	2013-07-30 14:43:08 +0000
@@ -2977,3 +2976,533 @@
+def parse_profile_data(data, file, do_include):
...


+    RE_PROFILE_LINK = re.compile('^\s*(audit\s+)?(deny\s+)?link\s+(((subset)|(<=))\s+)?([\"\@\/].*?"??)\s+->\s*([\"\@\/].*?"??)\s*,\s*(#.*)?$')

### "owner" is missing in the regex
### I've never seen the  <=  part in link rules - John, is this valid syntax?

valid? That is an interesting question. The parser and old genprof suppor
it as an alternate form of the subset keyword.

  link subset /foo -> /**,
  link <= /foo -> /**,

  l subset /foo -> /**,
  l <= /foo -> /*,

  l /foo,
  /foo l,

are all equivalent, and the parser also supports the keyword 'to' in place
of ->

  link subsert /foo to /**,

the variation comes down to a decision never being made about what the
syntax should be exactly, and delaying until it just shipped with both.

I am not too fond of <= and so I haven't used it or advertised it, and
I think we could call it long deprecated.
  -> vs to

is a little tougher but I don't think to is in use in the wild (though it
did make it into the core policy reference manual in its current very
incomplete form) and I tend to prefer a symbol to separate two "words"

I think unless others would like to see it stay, that 'to' could be dropped
as well.


As to what subset does, it forces a subset test on the link target, such
that the destination has a subset of the permissions of granted to the
source.  This is important when you are using link rules of the form

  /foo l,

which is the same as
  link subset /foo -> /**,

but in the cases where you have an exact link pairing in mind you may not
want the runtime subset restriction.


+    RE_PROFILE_CHANGE_PROFILE = re.compile('^\s*change_profile\s+->\s*("??.+?"??),(#.*)?$')
+    RE_PROFILE_ALIAS = re.compile('^\s*alias\s+("??.+?"??)\s+->\s*("??.+?"??)\s*,(#.*)?$')
+    RE_PROFILE_RLIMIT = re.compile('^\s*set\s+rlimit\s+(.+)\s+<=\s*(.+)\s*,(#.*)?$')

### some rlimit rules do not contain   <=   - from apparmor.vim.in:
### /\v^\s*set\s+rlimit\s+(nofile|nproc|rtprio)\s+[0-9]+@@EOL@@/

Right the intention was to make <= optional in 2.7, the form is

  'set' 'rlimit' [ <limit> ['<=' <value> ]] ','

however these patches never went in and the parser currently only
supports
  'set' 'rlimit' <limit> '<=' <value> ','

notice the set keyword must be used currently with rlimit


+    RE_PROFILE_BOOLEAN = re.compile('^\s*(\$\{?[[:alpha:]][[:alnum:]_]*\}?)\s*=\s*(true|false)\s*,?\s*(#.*)?$')
+    RE_PROFILE_VARIABLE = re.compile('^\s*(@\{?[[:alpha:]][[:alnum:]_]+\}?)\s*\+?=\s*(.+?)\s*,?\s*(#.*)?$')
+    RE_PROFILE_CONDITIONAL = re.compile('^\s*if\s+(not\s+)?(\$\{?[[:alpha:]][[:alnum:]_]*\}?)\s*\{\s*(#.*)?$')
+    RE_PROFILE_CONDITIONAL_VARIABLE = re.compile('^\s*if\s+(not\s+)?defined\s+(@\{?[[:alpha:]][[:alnum:]_]+\}?)\s*\{\s*(#.*)?$')
+    RE_PROFILE_CONDITIONAL_BOOLEAN = re.compile('^\s*if\s+(not\s+)?defined\s+(\$\{?[[:alpha:]][[:alnum:]_]+\}?)\s*\{\s*(#.*)?$')

### I've never seen/used conditional syntax, but according to the tests in
### parser/tst/simple_tests/conditional/* it's supported ;-)

it is

### this matches only "if", but "else" and "else if" are also supported
### (this is not the most urgent thing to add/fix ;-)

agreed, I think conditional blocks will become more important but they
are largely unused atm


+    RE_PROFILE_PATH_ENTRY = re.compile('^\s*(audit\s+)?(deny\s+)?(owner\s+)?([\"\@\/].*?)\s+(\S+)(\s+->\s*(.*?))?\s*,\s*(#.*)?$')

### [\"\@\/] looks wrong - inside [...] escaping with \ shouldn't be needed (it makes \ an allowed char, which would be wrong)

+    RE_PROFILE_NETWORK = re.compile('^\s*(audit\s+)?(deny\s+)?network(.*)\s*(#.*)?$')
+    RE_PROFILE_CHANGE_HAT = re.compile('^\s*\^(\"??.+?\"??)\s*,\s*(#.*)?$')
+    RE_PROFILE_HAT_DEF = re.compile('^\s*\^(\"??.+?\"??)\s+    ((flags=)?\((.+)\)\s+)*     \{\s*(#.*)?$')

### another wrong flags= part, see above for details
### (again, spaces around flags= added for readability)

...

+        elif RE_PROFILE_END.search(line):
+            # If profile ends and we're not in one                                                                                                                               
+            if not profile:
+                 raise AppArmorException('Syntax Error: Unexpected End of Profile reached in file: %s line: %s' % (file, lineno+1))

### note that there are other blocks that also end with  }  - for example "if" conditionals

and uh audit (well prefix) blocks

audit {
  /foo r,
}

the parser currently parses them but it doesn't hook their
rules back into the profile correctly, so they haven't been
advertised as being there.


### needs an update for the "allow" keyword (and a way to keep the keyword when writing the profile)

for a first pass I wouldn't care if the allow keyword got dropped from
profiles as it is the default behavior.  Of course people do get cranky
when tools change things that are for asthetic value


...            
+            if audit:
+                profile_data[profile][hat][allow]['link'][link]['audit'] = profile_data[profile][hat][allow]['link'][link].get('audit', 0) | AA_LINK_SUBSET

### just wondering - what does AA_LINK_SUBSET have to do with the audit flag?

At the language level it doesn't. I think this is just an artifact of how
the permissions where being stored as bitsets and audit bits where being
set off of the set of perms in the rule



### a check if the hat already exists might be useful to avoid duplicate hat names (which might get merged on write, but I doubt that's intended behaviour)

### interestingly, the parser does _not_ complain about duplicate hats.
### John, is this a bug or intentional?

That should fail, there is an explicit test for this in the parser. And in
my quick testing I get
  
  Multiple definitions for hat foo in profile (null) exist,bailing out.

so a bug in the output but the check worked, can you forward an example
where it is not working correctly

+    # Below is not required I'd say

### hmm, not sure - John?

+    if not do_include:
+        for hatglob in cfg['required_hats'].keys():
+            for parsed_prof in sorted(parsed_profiles):
+                if re.search(hatglob, parsed_prof):
+                    for hat in cfg['required_hats'][hatglob].split():
+                        if not profile_data[parsed_prof].get(hat, False):
+                            profile_data[parsed_prof][hat] = hasher()

err, I am going to have to get back to you on this one. I need to dive
in and get more context first



+def write_single(profile_data, depth, allow, name, prefix, tail):
+    pre = '  ' * depth
+    data = []
+    ref, allow = set_ref_allow(profile_data, allow)
+    
+    if ref.get(name, False):
+        for key in sorted(re[name].keys()):

### we should discuss if we want to keep writing in sorted() order (which can be helpful, but also annoying)
### or if we want to keep the original order of a profile whenever possible
### (see discussion about writing config files)
### -> topic for the next meeting?

I prefer original order when possible, possibly with an option to tell it
to order and clean up the profile.  Basically it comes down to ordering
destroys semantic/logical groupings and commenting.