Hello,
(with >34 °C outside, I decided to spend some hours in the office ;-)
Am Montag, 17. Juni 2013 schrieb nore...@launchpad.net:
> ------------------------------------------------------------
> revno: 7
> committer: Kshitij Gupta <kgupta8...@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]
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
