[apparmor] [Patch 0/2] A couple more unrelated patches

2012-02-16 Thread John Johansen 
A couple more related patches in preparation for mount rules


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

[apparmor] [PATCH 1/2] Fix an error in tree normalization that can result in an infinite loop

2012-02-16 Thread John Johansen 
Tree normailization tries to reshape expr tree into a normal from like

   |1   |1
  / \  / \
 |2  T - a   |2
/ \  / \
   |3  cb   |3
  / \  / \
 a   bc   T

which requires rotating the alt and cat nodes, at the same time it
also tries to rotate epsnods to the same side as alt and cat nodes.

However there is a bug that results in the epsnode flipping and
node rotation reverting each others work.  If the tree is of the
follow form this will result in an infinite loop.

   |1
  / \
 e2  |3
/ \
   e3  C

epsnode flipping is not supposed to take precedence over node rotation
and there is even a test to check for an alt or cat node, unfortunately
it is wrong resulting in unnecessary swapping and the possibility for
the above infinite loop

Signed-off-by: John Johansen john.johan...@canonical.com
---
 parser/libapparmor_re/expr-tree.cc |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/parser/libapparmor_re/expr-tree.cc 
b/parser/libapparmor_re/expr-tree.cc
index e9a1275..b502d54 100644
--- a/parser/libapparmor_re/expr-tree.cc
+++ b/parser/libapparmor_re/expr-tree.cc
@@ -189,7 +189,7 @@ void normalize_tree(Node *t, int dir)
for (;;) {
if ((epsnode == t-child[dir]) 
(epsnode != t-child[!dir]) 
-   dynamic_castTwoChildNode *(t)) {
+   !dynamic_castTwoChildNode *(t-child[!dir])) {
// (E | a) - (a | E)
// Ea - aE
Node *c = t-child[dir];
-- 
1.7.9


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

Re: [apparmor] [PATCH 1/2] Fix an error in tree normalization that can result in an infinite loop

2012-02-16 Thread Kees Cook 
On Thu, Feb 16, 2012 at 08:26:09AM -0800, John Johansen wrote:
 Tree normailization tries to reshape expr tree into a normal from like
 
|1   |1
   / \  / \
  |2  T - a   |2
 / \  / \
|3  cb   |3
   / \  / \
  a   bc   T
 
 which requires rotating the alt and cat nodes, at the same time it
 also tries to rotate epsnods to the same side as alt and cat nodes.
 
 However there is a bug that results in the epsnode flipping and
 node rotation reverting each others work.  If the tree is of the
 follow form this will result in an infinite loop.
 
|1
   / \
  e2  |3
 / \
e3  C
 
 epsnode flipping is not supposed to take precedence over node rotation
 and there is even a test to check for an alt or cat node, unfortunately
 it is wrong resulting in unnecessary swapping and the possibility for
 the above infinite loop
 
 Signed-off-by: John Johansen john.johan...@canonical.com
 ---
  parser/libapparmor_re/expr-tree.cc |2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)
 
 diff --git a/parser/libapparmor_re/expr-tree.cc 
 b/parser/libapparmor_re/expr-tree.cc
 index e9a1275..b502d54 100644
 --- a/parser/libapparmor_re/expr-tree.cc
 +++ b/parser/libapparmor_re/expr-tree.cc
 @@ -189,7 +189,7 @@ void normalize_tree(Node *t, int dir)
   for (;;) {
   if ((epsnode == t-child[dir]) 
   (epsnode != t-child[!dir]) 
 - dynamic_castTwoChildNode *(t)) {
 + !dynamic_castTwoChildNode *(t-child[!dir])) {
   // (E | a) - (a | E)
   // Ea - aE
   Node *c = t-child[dir];

I don't understand this area well enough to be comfortable to ACK it, but
if you say it's needed, that's good enough for me. ;) That said, is there
a simple test-case that can be used to show the before/after of this
change?

-Kees

-- 
Kees Cook

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

Re: [apparmor] [PATCH 2/2] Default profiles to be chroot relative

2012-02-16 Thread Kees Cook 
On Thu, Feb 16, 2012 at 08:26:10AM -0800, John Johansen wrote:
 Due to changes in path looks and the work going forward default profiles
 to resolve relative to the chroot instead of the namespace.
 
 This will only affect profiles that are used on tasks within a chroot.
 For now it will be possible to get the old default namespace relative
 behavior by passing the namespace_relative flag to the profile
 
 eg.
   profile /example (namespace_relative) { .. }
 
 Signed-off-by: John Johansen john.johan...@canonical.com

Acked-by: Kees Cook k...@ubuntu.com

-- 
Kees Cook

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

Re: [apparmor] [Tails-dev] AppArmor profiles in Debian

2012-02-16 Thread Kees Cook 
Hi,

On Thu, Feb 16, 2012 at 12:19:49AM +0100, intrigeri wrote:
 Kees Cook wrote (14 Feb 2012 19:59:45 GMT) :
  Ubuntu's evince and isc-dhcp-client profiles are very well tested at
  this point. I think it should be easy to move those into Debian if
  they're not there already.
 
 Right, I'm glad I've not encountered any single problem with these
 profiles. I'd like to thank everyone involved in their initial
 development, adaptation to Ubuntu, and testing.

\o/ Glad it's been treating you well! :)

   3. some low-hanging fruits from Ubuntu's Supported profiles in main
  list [1] that, I guess, you know very well: apache2, libvirt.
  
   [0] https://tails.boum.org/
   [1] https://wiki.ubuntu.com/SecurityTeam/KnowledgeBase/AppArmorProfiles
 
  I think just encouraging all the maintainers to pull in the Ubuntu
  patches for AppArmor profiles is the best way to start.
  Several packages have already done this (e.g. bind9).
 
 I think this will be totally relevant at some point,
 but I beg to disagree this is a good way to start.
 Here's why.
 
 Very well tested in Ubuntu is great, but sometimes is not always
 enough to be usable at all in Debian. The gdm vs. gdm3 bug in the
 X abstraction I've just reported against the apparmor package (sorry,
 being offline, I've no bug number yet) indicates that even trivial
 Debian-specific problems, in very common apparmor abstractions, were
 not detected yet.
 
 Therefore, I think at least some basic manual testing is due before we
 ask a Debian maintainer to ship any profile with their packages.

Right, absolutely. I didn't mean to imply that we should just throw
everything possible at the maintainers. :)

  To get things started, I have started using some of the profiles
  shipped in the apparmor-profiles packages; but none of the
 
  There's documentation in the Ubuntu wiki on transitioning profiles from the
  apparmor-profiles package to the individual packages;
  https://help.ubuntu.com/community/AppArmor#Migrating_an_apparmor-profiles_profile_to_a_package
 
 Thanks. I don't understand exactly what specific profiles we would
 want to migrate from apparmor-profiles to individual packages. Can you
 please elaborate?

I didn't have any in mind, but I just wanted to point out the procedure
we've documented for doing it in the past. For example, if we wanted to
move the ping, traceroute, etc stuff to their packages, we'd follow that
process.

  aforementioned software is supported, so I've extracted the profiles
  from the following Ubuntu packages, and have been running them in
  enforcing mode on my main Debian (sid) system:
  
* firefox 11.0~b2+build1-0ubuntu1
 
  The firefox profile remains tricky as far as enabling by default.
 
 I agree it should probably not be enabled by default in Debian:
 the iceweasel beast is so huge, and extensions scope so large, that
 a working-for-everyone profile would probably be liberal to a point
 it would not be very different from unconfined.
 
 However, in the context of a Live system such as Tails, where we
 control quite well the deployed iceweasel setup, I believe an AppArmor
 profile makes sense. I guess many other kinds of managed deployements
 share this property, and in such deployements, supporting tons of
 random add-ons is probably not wished by the system administrators, if
 allowed at all by site policies. I would love to see such a minimal
 profile shipped, although disabled or in complain mode by default,
 in Debian.
 
 Do you think it makes sense / is doable?

Yeah, absolutely. This is effectively what's done in Ubuntu. Local
configuration or a derivative can enable it, etc.

* isc-dhcp 4.1.1-P1-17ubuntu12 (client only)
 
  Yes, very handy. Order of operations is important here, though. The profile
  must load before any network interface. In Ubuntu, this is being done via
  upstart jobs -- I haven't tested it with sysvinit.
 
 Ah, so this was the meaning of
 /etc/apparmor/init/network-interface-security/sbin.dhclient being
 a symlink to /etc/apparmor.d/sbin.dhclient in the Ubuntu patch.
 Makes sense.

Yeah, it's a bit weird, but that's the least of a few evils.

 With sysvinit, I think that Required-Start: $remote_fs in the
 apparmor initscript will prevent AppArmor profiles to be loaded before
 the networking initscript starts. At least on my system, insserv
 ordered the scripts this way. Is this dependency present only to
 support the /usr-on-NFS usecase?

I suspect so, yes. It probably means that apparmor will either need to have
2 init files (early and late), or have its init modified not to require
/usr. Both we done at various times before in Ubuntu, so it shouldn't be
much work to make it happen.

 The desktop + NetworkManager usecase works fine, though, but this is
 mostly by chance, and the network-manager initscript should probably
 be added a dependency on AppArmor.
 
  One of the major stumbling blocks right now is that the legacy
  interface patch is not carried by the Debian kernel. This

Re: [apparmor] [PATCH 2/2] Default profiles to be chroot relative

2012-02-16 Thread John Johansen 

On 02/16/2012 03:50 PM, Steve Beattie wrote:

On Thu, Feb 16, 2012 at 08:26:10AM -0800, John Johansen wrote:

Due to changes in path looks and the work going forward default profiles
to resolve relative to the chroot instead of the namespace.


Hrm, can you expand on why it's needed? The patch itself looks fine,
just unclear on the motivation. Is this driven by the upcoming mount
work?


No not the mount work.  It was triggered by the lkml discussion around
the rework of __d_path and disconnected paths etc.  Currently we can
maintain the behavior of chroot paths walking back to the namespace
root, however there are those who would like to see this go away.

And I can see their point, from the pov of the tasks such files are out
of its view, so why are we treating them different than disconnected
paths.

For disconnected paths its not safe to reattach them so we have been
relying on the small bit of label caching we do to avoid having to
re-lookup the path (which will result in access being denied).

Long term the goal has been to fix this fully with implicit labeling
and delegation.

So the goal is to move to treating chroot applications the same.  It
provides consistency whether pivot root, bind mounts, or chroots are
used.

It also makes dealing with the policy inside and outside of a chroot
easier.  Think stacked profiles, a system one and the one for the
OS is the chroot/container.  In those cases the ones in the container
need to be chroot relative while system one of the host OS can be
either, but if they are both chroot relative, there is consistency
and things are easier to understand.

We don't have a lot of users of apparmor + chroots at the moment so
making the switch now makes a lot of sense.  It won't break any thing
that isn't using chroot, and with the containers work, and stacking
that is coming, not making the switch now could lead to problems


(Cue my usual note about noticing that we don't have tests around the
behaviors here.)


hrmmm, /me wonders what happened to them, we did have somethings at
one point.

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

Re: [apparmor] [PATCH 1/2] Fix an error in tree normalization that can result in an infinite loop

2012-02-16 Thread John Johansen 

On 02/16/2012 03:34 PM, Steve Beattie wrote:

On Thu, Feb 16, 2012 at 12:25:05PM -0800, Kees Cook wrote:

On Thu, Feb 16, 2012 at 08:26:09AM -0800, John Johansen wrote:

Tree normailization tries to reshape expr tree into a normal from like

|1   |1
   / \  / \
  |2  T -  a   |2
 / \  / \
|3  cb   |3
   / \  / \
  a   bc   T

which requires rotating the alt and cat nodes, at the same time it
also tries to rotate epsnods to the same side as alt and cat nodes.

However there is a bug that results in the epsnode flipping and
node rotation reverting each others work.  If the tree is of the
follow form this will result in an infinite loop.

|1
   / \
  e2  |3
 / \
e3  C

epsnode flipping is not supposed to take precedence over node rotation
and there is even a test to check for an alt or cat node, unfortunately
it is wrong resulting in unnecessary swapping and the possibility for
the above infinite loop

Signed-off-by: John Johansenjohn.johan...@canonical.com
---
  parser/libapparmor_re/expr-tree.cc |2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/parser/libapparmor_re/expr-tree.cc 
b/parser/libapparmor_re/expr-tree.cc
index e9a1275..b502d54 100644
--- a/parser/libapparmor_re/expr-tree.cc
+++ b/parser/libapparmor_re/expr-tree.cc
@@ -189,7 +189,7 @@ void normalize_tree(Node *t, int dir)
for (;;) {
if ((epsnode == t-child[dir])
(epsnode != t-child[!dir])
-   dynamic_castTwoChildNode *(t)) {
+   !dynamic_castTwoChildNode *(t-child[!dir])) {
// (E | a) -  (a | E)
// Ea -  aE
Node *c = t-child[dir];


I don't understand this area well enough to be comfortable to ACK it, but
if you say it's needed, that's good enough for me. ;) That said, is there
a simple test-case that can be used to show the before/after of this
change?

yes and no.

I can trivially trigger it, patching the parser but you can't easily trigger
it yet, with the way rules are currently exposed.  Mount rules on the other
hand where trivially triggering this.

This was the fast fix that got the problem out of the way, but well it has
its own issues.

however the improvement is dramatic with out it infinite loop, with it
things complete :)



I feel similarly, though I think I see the logic. Does the fixed
check invalidate the comment that follows the child swap:

// Don't break here as 'a' may be a tree that
// can be pulled up.

or can 'a' still be a tree even if it's not a TwoChildNode?


sigh yes this does break that

well it can be the root of a * or + expression but they can't be pulled
up.

basically this was the quick fix, and while it doesn't break things, it
does make the simplification less effective and I think we can do better
I will take another pass at it.


(Also, the swap code could be simplified or at least made to be
consistent with the comment; since t-child[dir] is known to point
epsnode, I think you could do:

t-child[dir] = t-child[!dir];
t-child[!dir] =epsnode;


yeah, we used to not use a single shared epsnode, and the code hasn't
been cleaned up for it, mostly because I have been completely rewriting
the tree simplification.


or if you wanted to indicate that you're swapping positions, this may be
more consistent with the comments referring to a

Node *a = t-child[!dir];
t-child[!dir] = t-child[dir];
t-child[dir] = a;

because the temporary behavior would match up with a.)


hehe yeah


It *really* would be nice if we had some unit tests for this code.


yep that is badly needed

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