[apparmor] [Branch ~kgupta8592/apparmor-profile-tools/trunk] Rev 7: added severity.py with tested convert_regex and the old and new config

[Christian Boltz]

Hello,

(with >34 °C outside, I decided to spend some hours in the office ;-)

Am Montag, 17. Juni 2013 schrieb noreply at launchpad.net:
> ------------------------------------------------------------
> revno: 7
> committer: Kshitij Gupta <kgupta8592 at gmail.com
> branch nick: apparmor-profile-tools
> timestamp: Tue 2013-06-18 03:49:05 +0530
> message:
>   added severity.py with tested convert_regex and the old and new
> config added:
>   lib/configbkp.py
>   lib/severity.py
> modified:
>   lib/config.py

Your code looks quite good, but let me add some random thoughts about 
it nevertheless ;-)

=== lib/config.py ===

> def write_config(filename, config):
>     """Writes the given configparser to the specified file"""
>     filepath = confdir + '/' + filename
>     try:
>         with open(filepath, 'w') as config_file:
>             config.write(config_file)
>     except IOError:
>         raise IOError("Unable to write to %s"%filename)
>     else:
>         permission_600 = stat.S_IRUSR | stat.S_IWUSR    # Owner read and write
>         # Set file permissions as 0600
>         os.chmod(filepath, permission_600)

This code has two small bugs, at least for nitpickers ;-)
- it should write to a tempfile, check for errors and then replace the 
  config_file atomically (to avoid breaking the configfile when your 
  disk is full ;-)
- the correct order is: create file, set permissions, write the content
  (your code leaves the file readable for others for a short moment - 
  except if the python default permissions are 600 of course ;-)

>From users' POV, it might be better to keep the previous permissions
instead of using hardcoded 600 - but 600 is more secure.
@John: what is your opinion about this?


=== severity.py ===

> class Severity:
>     def __init__(self, dbname=None, default_rank=10):
> [...]
>             if line.startswith('/'):
>                 try:
>                     path, read, write, execute = line.split()                 
>                 except ValueError:
>                     raise("Insufficient values for permissions")

Very helpful error message ;-)  Please include the filename and the 
line (line number or content). A good error message would be:

    Error in /etc/apparmor/severity.db:
    Insufficient values for permissions in line
        /foo/bar 2

You should also check if read, write, execute contain valid severity
numbers (0-10).

>                                ptr[regexp] = {'SD_RANK': {'r': read, 'w': write, 'x': execute}}

The "SD_" prefix probably dates back to the times when AppArmor was 
named "SubDomain". Feel free to use another prefix, maybe "AA_" ;-)

>             elif line.startswith('CAP'):
>                 resource, severity = line.split()
>                 self.severity['CAPABILITIES'][resource] = severity

Nitpicking again: I'd check for "CAP_" ;-)

Besides that, the error check is missing - I'm quite sure a line
    CAP_FOOBAR
(without a number) will break something ;-)

You should check that
a) the line consists of two parts (+ optional comment?)
b) the second part (the severity) is a valid number (0-10)

>             else:
>                 print("unexpected database line: %s"%line)   

This error message is better than the one I quoted above, but adding
the filename won't hurt ;-)

>     def convert_regexp(self, path):
> [...]
>         regex = regex.replace('**', '.SDPROF_INTERNAL_GLOB')
>         regex = regex.replace('*', '[^/]SDPROF_INTERNAL_GLOB')
> [...]
>         # Restore the * in the final regex
>         regex = regex.replace('SDPROF_INTERNAL_GLOB', '*')

I might be paranoid, but - what happens if access to a file called 
/foo/barSDPROF_INTERNAL_GLOB is requested? ;-)
This is highly theoretical for severity.db, but please keep it in mind 
if you use similar code for logprof/genprof. (You probably won't be 
able to avoid such conflicts, but you can reduce their chance by 
choosing a longer/more "cryptical" string.)

=== configbkp.py ===

Looks like a copy of the old code in config.py. That's why bzr has a
version history - no need to checkin backups of old code ;-)


Regards,

Christian Boltz
-- 
Sach ma, siggst du alles von mir? ;)
[David Haller in fontlinge-devel]