[apparmor] [PATCH] aa-easyprof updates, take 2

Wed Jul 10 03:09:00 UTC 2013

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. We have the hex-encoding mechanism to allow "difficult"
> characters in attachment paths but those are somewhat specific to the
> parser and potentially not needed in the json -- but the json
> representations might need different constraints.
> 
> Forcing a-bA-Z0-9_- just feels a bit blunt. (How well will our Chinese,
> Korean, and Japanese friends like this?)
> 
> Anyway, if any of this makes it easier to handle the data, awesome. If
> it's just noise, feel free to ignore me. :)
>

FYI, I don't really care about the json input-- I figured that is what
json.loads() is for. If it croaks, we raise an exception. If the structure isn't
what we expect, we raise an exception. Of course, we do other input validation
to make sure things are sane for us.

I was quite strict on profile names because I know that we are quite limited on
the profile name in terms of what the kernel can use (ie, 'profile fθθ {}' is
not allowed (along with all the other UTF-8 characters, so our Asian friends are
already out of luck)). I also then went quite strict based on click package
requirements-- the profile name will be a union of allowable Debian/Ubuntu
source package and version characters and '_'. This could be viewed as quite
distribution specific, but this regex is surely quite usable for other distros
and I would argue the current regex is quite within the norm of how people
normally profile, so I thought it ok. Furthermore, path-based attachment is not
limited by profile name regexes, only by valid_path() (both '"/path/to/fθθ" {}'
and 'profile "hi_there" "/path/to/fθθ" {}' work fine). I suppose I could add a
--strict option that enforces the current behavior and allows a more
full-featured set by default (or make that configurable via a configuration
file), but again, I'm not convinced there is a practical benefit. If someone
complains, we can review this in the future (or the submitted patches to add it).

Template variables could arguably be expanded-- the regex is quite strict on the
LHS, but the RHS is not at all. I wanted to keep template variables quite simple
so that we have something usable now. We can expand on this in the future if
people desire/complain (or again, review patches).

valid_path() itself is considerably less strict and is expected to handle the
full gamut of allowed characters in a filename with one exception: '"'. You
could say I punted on this, but I don't mind admitting that-- to support this
correctly would take considerable work and there are bigger fish to fry atm.
Perhaps I should add it to KNOWN BUGS that '"' is not allowed in the path at
this time.

-- 
Jamie Strandboge                 http://www.ubuntu.com/

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 899 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20130709/fd98643d/attachment-0001.pgp>