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

[Seth Arnold]

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?

>  - 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. :)

Thanks
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20130709/d7c1e7f6/attachment.pgp>