Hello,
On Tue, Jun 18, 2013 at 6:34 PM, Christian Boltz <appar...@cboltz.de> wrote:
> Hello,
>
> (with >34 °C outside, I decided to spend some hours in the office ;-)
>
You seem to be having a pretty hot summer, apparently that works in my
favour today. :-)
> 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 ;-)
My bad, John had mentioned it previously and I did make of a note of
making a temp file. Forgot to implement it.
> - 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 ;-)
>
Thanks, Will be sure to make a better note of the order than I did of
the temp file thing. :-)
> 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?
>
For old files, we can skip setting the permissions, while owner
read/write seems fairly okay to me for new files, but then I might be
completely missing out on some scenario. ;-)
>
> === 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_" ;-)
>
I was actually beginning to wonder where did all SD's come from,
thanks for clearing that. That's why its necessary to know about
history ;-)
>> 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)
>
Should've had that in a try/except block. On it.
>> 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.)
I had a good laugh at that. ;-)
I tested the code against severity.db for any anomalies and felt that
was sufficient.
If ever such a bug did arise, I believe it would be very hard to
trace. Now I see how you got all those bug-reports. :-)
"Cryptic"? Any weird things you'd want inserted? ;-)
>
> === 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 ;-)
>
>
Actually I have the backup module to be able to compare the results
with the new module. Assuming, the previous one worked.
Regards,
Kshitij
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
