Re: [apparmor] [PATCH] aa-easyprof updates
On Sat, Jul 06, 2013 at 12:37:20PM -0500, Jamie Strandboge wrote: There's a race condition here; well, maybe not -race-, but self.template is updated before the sanity checks are performed. If either of those exceptions gets ignored in callers, the template is set to unsafe values. Hmmm, I can fix it, but if a caller chooses to use a try clause and then ignore it, I'm kinda thinking that is the caller's problem, no? I mean, that is why I'm raising the exception here. Yes, it would be the caller's problem, but I like to think that in the event of an exeption or error there should be as few side-effects as possible. Unexpected side-effects are a common source of problems, which is why I mentioned this. It was likely fine, as it was, but who knows where it'll be used in the future... I don't like how I am mixing and matching checking template and self.template though, so I'll change that right away. Woo :) thanks. signature.asc Description: Digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH] aa-easyprof updates, take 2
On 07/09/2013 07:56 PM, Seth Arnold wrote: On Sun, Jul 07, 2013 at 08:46:31PM -0500, Jamie Strandboge wrote: Attached is a patch to address Seth's comments, and a few more fixes. After submitting the last patch, we discussed the JSON structure[1] a bit more, and realized that it needed a refinement. In particular, rather than having manifest['security'] contain the profile objects (keyed by 'profile_name'), we should move the profile objects in to their own profiles dictionary, manifest['security']['profiles'], such that this dictionary contains all the profile objects. It doesn't do anything for us now, but is a better format in case we want to add new toplevel keys in the future. The full changes are as follows: Heh, this change likely fixed a potential security issue of someone naming their profiles with the same names as sections in the manifest.. - don't add vendor directory to self.templates and self.policy_groups - utils/aa-easyprof: adjust error message for manifest read failure - utils/aa-easyprof: adjust to use EnvironmentError on failed read of the manifest - utils/apparmor/easyprof.py: clean up set_template() - utils/apparmor/easyprof.py: read_paths should use 'rk' I'm not so sure about this 'rk' for read_paths approach. I had thought of a lock_paths approach myself, but then someone could just as easily include a /** k rule in their profile via that mechanism. What are the potential downsides of letting a process lock essentially arbitrary files? Can they prevent updates by locking /usr/lib/**? Prevent adding new users by locking /etc/passwd? In my mind, easyprof is primarily about using templates, abstractions and policy groups to handle files on the system. --read-path and --write-path are there primary to deal with corner cases with your own files. easyprof also doesn't guarantee sane policy-- it is just a tool and as such it will do what you tell it-- its output needs to be reviewed. --verify-manifest is present to help alert people to certain unsafe options such as using --read-path and --write-path. Ubuntu is planning on using easyprof for its application confinement work, but we will enforce the use of templates and policy groups and --read-path and --write-path won't be allowed (but that is a policy decision we are taking, not something easyprof should do). As such, easyprof is not really a fine-grained tool. In fact, its man page specifically says: Also, this tool may create policy which is less restricted than creating policy by hand or with aa-genprof and aa-logprof. I used 'rk' with --read-path because I thought that was what you guys wanted and I thought it was reasonable considering the above and since --write-path already had 'k'. We could add --lock-path, but that is adding more complexity, and to what end? Yes, it is more fine-grained, but if people are going that fine-grained, why not just ignore easyprof and do your own thing with the other tools that are designed for being that fine-grained. In retrospect, maybe I should just revert it and say if you want locking, just use --write-path and document that in the man page. I really don't want to get into a situation where we are doing intersections of --lock-path and --read-path/--write-path or adding --read-lock-path and --write-lock-path. I just don't see a lot of benefit. If others do, I'm happy to revert the patch and maybe someone who feels very strongly about lock path can submit a patch (or convince me how lock path is actually the cat's pajamas ;). - utils/test/test-aa-easyprof.py: adjust tests for above - utils/apparmor/easyprof.py + valid_path should verify os.path.normpath(path) == (path) + adjust valid_profile_name() to start with alpha-numeric and allow Debian source package names and version, plus '_' + adjust tests for above - update valid_variable() to check for valid_path if '/' is in the value - adjust valid_path() to have a relative_ok flag (default to False) - adjust valid_path() to verify path is same as normalized path - add some valid_path() test cases - adjust to always quote template vars in policy output - add a couple tests that have spaces in the binary and template var - update manifest JSON structure to use m['security']['profiles']['profile_name'] instead of m['security']['profile_name'] When reading the valid_path() and similar functions, I had a thought that our problem is similar to web-apps: we need to perform one kind of sanitizing on input and another kind of sanitizing on output. A great many applications make the mistake of enforcing SQL-and-HTML-safe variable values on input rather than just HTML-escaping on output and SQL-escaping when running queries. Our problem is different of course, but it did strike me that the parser and kernel interface have one set of constraints on legal profile names, attachment paths, etc., but the json output has different sets of name constraints.
Re: [apparmor] [PATCH] aa-easyprof updates
On 07/05/2013 07:17 PM, Seth Arnold wrote: On Mon, Jul 01, 2013 at 05:15:07PM -0500, Jamie Strandboge wrote: @@ -428,6 +546,7 @@ s = %s# No read paths specified % prefix if len(read_path) 0: s = %s# Specified read permissions % (prefix) +read_path.sort() for i in read_path: for r in self.gen_path_rule(i, 'r'): s += \n%s%s % (prefix, r) @@ -438,17 +557,109 @@ s = %s# No write paths specified % prefix if len(write_path) 0: s = %s# Specified write permissions % (prefix) +write_path.sort() for i in write_path: for r in self.gen_path_rule(i, 'rwk'): s += \n%s%s % (prefix, r) policy = re.sub(r' *%s' % search, s, policy) We may also need a way to allow profile authors to push 'k' through on files that they'll only read. (Maybe all these cases will already be handled via abstractions.) Agreed. Fixed. -- Jamie Strandboge http://www.ubuntu.com/ signature.asc Description: OpenPGP digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH] aa-easyprof updates
On Mon, Jul 01, 2013 at 05:15:07PM -0500, Jamie Strandboge wrote:
+def set_template(self, template, allow_abs_path=True):
'''Set current template'''
self.template = template
+if ../ in template:
+raise AppArmorException('template %s contains ../ escape
path' % (template))
+if template.startswith('/'):
+if not allow_abs_path:
+raise AppArmorException(Cannot use an absolute path
template '%s' % template)
+else:
self.template = os.path.join(self.dirs['templates'], template)
if not os.path.exists(self.template):
raise AppArmorException('%s does not exist' % (self.template))
There's a race condition here; well, maybe not -race-, but self.template
is updated before the sanity checks are performed. If either of those
exceptions gets ignored in callers, the template is set to unsafe values.
Thanks
signature.asc
Description: Digital signature
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH] aa-easyprof updates
On Mon, Jul 01, 2013 at 05:15:07PM -0500, Jamie Strandboge wrote: @@ -428,6 +546,7 @@ s = %s# No read paths specified % prefix if len(read_path) 0: s = %s# Specified read permissions % (prefix) +read_path.sort() for i in read_path: for r in self.gen_path_rule(i, 'r'): s += \n%s%s % (prefix, r) @@ -438,17 +557,109 @@ s = %s# No write paths specified % prefix if len(write_path) 0: s = %s# Specified write permissions % (prefix) +write_path.sort() for i in write_path: for r in self.gen_path_rule(i, 'rwk'): s += \n%s%s % (prefix, r) policy = re.sub(r' *%s' % search, s, policy) We may also need a way to allow profile authors to push 'k' through on files that they'll only read. (Maybe all these cases will already be handled via abstractions.) signature.asc Description: Digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH] aa-easyprof updates
On Fri, Jul 05, 2013 at 05:17:48PM -0700, Seth Arnold wrote: On Mon, Jul 01, 2013 at 05:15:07PM -0500, Jamie Strandboge wrote: @@ -428,6 +546,7 @@ s = %s# No read paths specified % prefix if len(read_path) 0: s = %s# Specified read permissions % (prefix) +read_path.sort() for i in read_path: for r in self.gen_path_rule(i, 'r'): s += \n%s%s % (prefix, r) @@ -438,17 +557,109 @@ s = %s# No write paths specified % prefix if len(write_path) 0: s = %s# Specified write permissions % (prefix) +write_path.sort() for i in write_path: for r in self.gen_path_rule(i, 'rwk'): s += \n%s%s % (prefix, r) policy = re.sub(r' *%s' % search, s, policy) We may also need a way to allow profile authors to push 'k' through on files that they'll only read. (Maybe all these cases will already be handled via abstractions.) Abstractions will help in some cases, but I think you're correct; consider stuff like application specific sqlite databases. -- Steve Beattie sbeat...@ubuntu.com http://NxNW.org/~steve/ signature.asc Description: Digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
