[apparmor] dbus/pair address rule encoding

2013-05-08 Thread John Johansen
One of the decisions made last week while several us met at a sprint. Was
to change the dbus prototype syntax slightly to make it follow the same
general format of the proposed network/ipc rules.

Currently this is just a syntactic change, no new functionality is being
added at this time.

The change is to separate the local and remote addresses of a connection,
message.

  dbus name=foo.com acquire,

will acquire/bind a name for a local address. The only change here is the
use of the more appropriate word 'name' instead of 'dest'

For sending messages the syntax becomes
  dbus - name=foo.com send,

the change here is '-' to a remote/peer address is going to be specified
and again 'name' instead of dest.

This syntax allows for rules (though unsupported atm) that specify both,
  dbus name=foo.com - name=bar.com  send,

which allows tying a send to a specific source and to a specific destination.
This doesn't make a lot of sense when just addresses are used but it may
when labeling is supported in the future.

Now while this syntactic change can be encoded using the current rule
encoding, it is problematic for potential future expansion in that it overlays
both the address being acquired and the address being sent to.


So we need to change to a pair encoding which is similar to link, mount rules
and what is planned for the extended networking.

the general scheme is to encode the address that is to be looked up
first/most common. I would assume this would be the local address for the
acquire rule as this would mean that lookups for the acquire perm would
not require both addresses, and having both addresses does not make
sense.

the second address encoding follows the first as a pairing. This allows us
to restrict access to the second address based on the first if so desired.

Eg. for link rules we have
  /path/to/file\0/path/to/link
  ^  ^
perm1  perm2

the permissions are encoded out of band (that is each state could have its
own set of permissions. And we use one special perm to indicate there is
more match data/perms to follow.

When making a permission request you only need to proceed with matching until
your requested permissions are matched (knowing the encoding you can short
circuit).


In the case of dbus I see

  local address set encodingremote address set encoding
 ^^
   perm1perm2

the AA_ACQUIRE perm gets encoded into perm1. And if there is a remote
address for the rule, the AA_CONT perm which means there is more matching
that can happen. AA_CONT isn't actually required it just allows bailing
out early if no more matches are possible (it would also be filtered out
from the set of permissions returned).

technically we could even encode send/receive in perm1 if it is followed
by a match that allows all remote addresses.

With the way the backend of the compiler is setup currently this means we
need to generate 2 rules when a remote address needs to be specified
  local address perm1
  local address separator remote address perm2

perm1 - may just be the AA_CONT perm. While the AA_CONT perm isn't required
having it present makes sure short circuiting can be done in the future
either from user space using a none naive query (see below) or if dbus
moves into the kernel.

the backend will take care of collapsing/recombining states where possible.
I would like to get to a point where this isn't needed but, we aren't there
yet.

the separator is anything that separates the two addresses in the encoding.
In link pairs, its a null transition (\0 character). We use this already
in the address encoding to separate the various possible strings (path,
name, iface, member).

We can either put in a second \0, so at the separator point we have \0\0
where the first is the separator for the member encoding, or we can just
rely on the fact that we know we have a fixed number of parts to the
source address, and that no additional separator is needed.

For the permissions I think we should encode them on the \0 separator,
not the last character of the string. This will allow us to specify a
don't care/any source for the query string of
  \0\0\0\0
which will find us the source permission if rules are encoded such that
for the parts of the address that can match anything they are encoded using
the regex [^0]*\0


To query this encoding,

aquire: just needs to specify the local address and return the perms found
there.

send/receive: need to specify both the source and dest addresses in their
  query string. This allows us to allow these permissions
  conditionally based on what source they come from.

  I know that we aren't doing the currently but it leaves the
  possibility open in the future.

  I would also assume the source address would be the well
  known address as long as the 

Re: [apparmor] [patch] backport python3 compability patch to 2.8 branch

2013-05-08 Thread Steve Beattie
On Tue, May 07, 2013 at 10:51:01PM +0200, Christian Boltz wrote:
  You probably also want to pull in trunk commit 2108 as well.
 
 openSUSE seems to be better than Ubuntu - at least my test builds worked 
 without r2108 ;-)

Oddly enough, the 2.8 backported patch kicked off jenkins builds that
succeeded on 13.04 (as well as other releases). However, it makes
sense to me to use python-config if available.

 Anyway, r2108 is a small patch and looks good to me - I'd say 
 Skimmed-by: Christian Boltz appar...@cboltz.de   ;-)
 (credits for this new patch review level go to Seth ;-)

To be explicit, the patch looks like so:

=== modified file 'libraries/libapparmor/m4/ac_python_devel.m4'
--- libraries/libapparmor/m4/ac_python_devel.m4 2012-06-12 12:56:57 +
+++ libraries/libapparmor/m4/ac_python_devel.m4 2013-01-29 23:21:47 +
@@ -79,6 +79,9 @@
 # Check for Python include path
 #
 AC_MSG_CHECKING([for Python include path])
+if type $PYTHON-config; then
+PYTHON_CPPFLAGS=`$PYTHON-config --includes`
+fi
 if test -z $PYTHON_CPPFLAGS; then
 python_path=`$PYTHON -c import sys; import 
distutils.sysconfig;\
 sys.stdout.write('%s\n' % distutils.sysconfig.get_python_inc());`
@@ -94,6 +97,9 @@
 # Check for Python library path
 #
 AC_MSG_CHECKING([for Python library path])
+if type $PYTHON-config; then
+PYTHON_LDFLAGS=`$PYTHON-config --ldflags`
+fi
 if test -z $PYTHON_LDFLAGS; then
 # (makes two attempts to ensure we've got a version number
 # from the interpreter)

As nice as a skimmed-by review is, I admit to wanting a proper acked-by
statement, if possible. :)

-- 
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


Re: [apparmor] [PATCH 08/36] apparmor: provide the ability to boot with a default profile set on init

2013-05-08 Thread Seth Arnold
On Wed, May 01, 2013 at 02:30:53PM -0700, John Johansen wrote:
 --- a/security/apparmor/Kconfig
 +++ b/security/apparmor/Kconfig
 @@ -29,3 +29,14 @@ config SECURITY_APPARMOR_BOOTPARAM_VALUE
 boot.
  
 If you are unsure how to answer this question, answer 1.
 +
 +config SECURITY_APPARMOR_UNCONFINED_INIT
 + bool Set init to unconfined on boot
 + depends on SECURITY_APPARMOR
 + default y
 + help
 +   This option determines policy behavior during early boot by
 +   placing the init process in the unconfined state, or the
 +   'default' profile.
 +
 +   If you are unsure how to answer this question, answer Y.

I think this description needs some enhancement; I thought the boolean
was the other way around until I thought I spotted a bug with a ! in
the conditionals.

How about:

 +   This option determines policy behavior during early boot by
 +   placing the init process in the unconfined state, or the
 +   'default' profile.
 +
 +   'Y' means init and its children are not confined, and never
 +   can be confined; loaded policy will only apply to processes
 +   started afterwards.
 +
 +   'N' means init and its children are confined in a profile
 +   named 'default', which can be replaced later and thus
 +   provide for confining even processes started early at boot,
 +   though not confined during early boot. This can provide for
 +   complete system confinement.
 +
 +   If you are unsure how to answer this question, answer Y.

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 09/36] apparmor: fix fs extry display for default profile

2013-05-08 Thread Seth Arnold
On Wed, May 01, 2013 at 02:30:54PM -0700, John Johansen wrote:
 The default profile needs its replaced by information set as its on
 the profile list and will have an fs interface (and the fs interface
 files require a valid replacedby).
 
 Signed-off-by: John Johansen john.johan...@canonical.com
 ---
  security/apparmor/policy.c | 3 +++
  1 file changed, 3 insertions(+)
 
 diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c
 index 333cbb7..a7e6bd9 100644
 --- a/security/apparmor/policy.c
 +++ b/security/apparmor/policy.c
 @@ -731,6 +731,9 @@ struct aa_profile *aa_setup_default_profile(void)
  
   profile-ns = aa_get_namespace(root_ns);
  
 + /* replacedby being set needed by fs interface */
 + rcu_assign_pointer(profile-replacedby-profile,
 +aa_get_profile(profile));
   __list_add_profile(root_ns-base.profiles, profile);
  
   return profile;

Will aa_get_profile(profile) here cause an inability to ever free the
profile, say in case it is replaced?

I know this whole area is drastically changed in a later patch, but if
this is going to be part of a bisectable kernel tree, it'd be nice if
this wasn't terribly leaky. :)

This patch might profitably be merged with an earlier patch; it looks
like a necessary bugfix for something earlier.

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 10/36] apparmor: allow setting any profile into the unconfined state

2013-05-08 Thread Seth Arnold
On Wed, May 01, 2013 at 02:30:55PM -0700, John Johansen wrote:
 Allow emulating the default profile behavior from boot, by allowing
 loading of a profile in the unconfined state into a new NS.
 
 Signed-off-by: John Johansen john.johan...@canonical.com

Acked-by: Seth Arnold seth.arn...@canonical.com

... with the caveat / note that the following hunk _may_ require
userspace changes. (Those changes may already have been made.)

 index 69894ad..c69f7c4 100644
 --- a/security/apparmor/policy_unpack.c
 +++ b/security/apparmor/policy_unpack.c
 @@ -510,12 +510,16 @@ static struct aa_profile *unpack_profile(struct aa_ext 
 *e)
   goto fail;
   if (!unpack_u32(e, tmp, NULL))
   goto fail;
 - if (tmp)
 + if (tmp  PACKED_FLAG_HAT)
   profile-flags |= PFLAG_HAT;
   if (!unpack_u32(e, tmp, NULL))
   goto fail;
 - if (tmp)
 + if (tmp == PACKED_MODE_COMPLAIN)
   profile-mode = APPARMOR_COMPLAIN;
 + else if (tmp == PACKED_MODE_KILL)
 +  profile-mode = APPARMOR_KILL;
 + else if (tmp == PACKED_MODE_UNCONFINED)
 + profile-mode = APPARMOR_UNCONFINED;
   if (!unpack_u32(e, tmp, NULL))
   goto fail;
   if (tmp)

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 01/36] apparmor: provide base for multiple profiles to be replaced at once

2013-05-08 Thread John Johansen
On 05/01/2013 05:05 PM, Seth Arnold wrote:
 On Wed, May 01, 2013 at 02:30:46PM -0700, John Johansen wrote:
 previously profiles had to be loaded one at a time, which could result
 in cases where a replacement would partially succeed, and then fail
 resulting in inconsitent policy.

 Allow multiple profiles to replaced atomically so that the replacement
 either succeeeds or fails atomically for the set of profiles.

 Note: this does not provide multiple load of profiles when adding a parent
 and its children as an atomic profile set. Because of this limitation
 the atomic set load of profiles should not be exposed to userspace at this
 time.

 Signed-off-by: John Johansen john.johan...@canonical.com
 
 I'm having trouble reviewing this one, it all looks too familiar -- it all
 looks like 'normal', now. The only thing I spotted is that the description
 isn't true now, since you did get child profiles in the atomic load.
 
thats interesting as this patch saw the largest overhaul of all the
previous patches sent.

Anyways thanks for catching the description problem, its fixed now.



-- 
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/apparmor