Bug#1050256: AppArmor breaks locking non-fs Unix sockets

[John Johansen]

On 12/30/23 20:24, Mathias Gibbens wrote:

On Sat, 2023-12-30 at 16:44 +0100, Salvatore Bonaccorso wrote:

John, did you had a chance to work on this backport for 6.1.y stable
upstream so we could pick it downstream in Debian in one of the next
stable imports? Cherry-picking 1cf26c3d2c4c ("apparmor: fix apparmor
mediating locking non-fs unix sockets") does not work, if not
havinging the work around e2967ede2297 ("apparmor: compute policydb
permission on profile load") AFAICS, so that needs a 6.1.y specific
backport submitted to sta...@vger.kernel.org ?

I think we could have people from this bug as well providing a
Tested-by when necessary. I'm not feeling confident enough to be able
to provide myself such a patch to sent to stable (and you only giving
an Acked-by/Reviewed-by), so if you can help out here with your
upstream hat on that would be more than appreciated and welcome :)

Thanks a lot for your work!


   I played around with this a bit the past week as well, and came to
the same conclusion as Salvatore did that commits e2967ede2297 and
1cf26c3d2c4c need to be cherry-picked back to the 6.1 stable tree.

   I've attached the two commits rebased onto 6.1.y as patches to this
message. Commit e2967ede2297 needed a little bit of touchup to apply
cleanly, and 1cf26c3d2c4c just needed adjustments for line number
changes. I included some comments at the top of each patch.

   With these two commits cherry-picked on top of the 6.1.69 kernel, I
can boot a bookworm system and successfully start a service within a
container that utilizes `PrivateNetwork=yes`. Rebooting back into an
unpatched vanilla 6.1.69 kernel continues to show the problem.

   While I didn't see any immediate issues (ie, `aa-status` and log
files looked OK), I don't understand the changes in the first commit
well enough to be confident in sending these patches for inclusion in
the upstream stable tree on my own.

Mathias


Your backports look good to me, and you can stick my acked-by on them.
The changes are strictly more than necessary for the fix. They are
part of a larger change set that is trying to cleanup the runtime
code by changing the permission mapping from a runtime operation
to something that is done only at policy load/unpack time.

The advantage of this approach is that while it is a larger change
than strictly necessary. It is backporting patches that are already
upstream, keep the code closer and making backports easier.

Georgia did a minimal backport fix by keeping the version as part
of policy and doing the permission mapping at runtime. I have
included that patch below. Its advantage is it is a minimal
change to fix the issue.

I am happy with either version going into stable. Do you want to
send them or do you want me to do it?

Acked-by: John Johansen 
From d716d8e6e8b6dc2fa1db2e72ee95c721aef12064 Mon Sep 17 00:00:00 2001
From: Georgia Garcia 
Date: Tue, 9 Jan 2024 17:54:52 -0300
Subject: [PATCH] apparmor: backport fix apparmor mediating locking non-fs,
 unix sockets

This is a minimal backport of
1cf26c3d2c4c apparmor: fix apparmor mediating locking non-fs unix sockets

instead of pulling in the dependency patch
e2967ede2297 apparmor: compute policydb permission on profile load

which moves the permission mapping to unpack time. We push the version
information into the profile so the permission mapping fix can be
done in the run time aa_compute_perms() instead of the load time
equivalent in compute_perms_entry() introduced by e2967ede2297.

Signed-off-by: Georgia Garcia 
Acked-by: John Johansen 
---
 security/apparmor/apparmorfs.c |  3 ++-
 security/apparmor/include/perms.h  |  2 +-
 security/apparmor/include/policy.h | 12 
 security/apparmor/label.c  |  9 ++---
 security/apparmor/lib.c|  4 +++-
 security/apparmor/net.c|  3 ++-
 security/apparmor/policy_unpack.c  | 11 +--
 7 files changed, 27 insertions(+), 17 deletions(-)

diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index 7160e7aa58b9..8131c9345900 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -633,7 +633,8 @@ static void profile_query_cb(struct aa_profile *profile, struct aa_perms *perms,
 		state = aa_dfa_match_len(dfa, profile->policy.start[0],
 	 match_str, match_len);
 		if (state)
-			aa_compute_perms(dfa, state, );
+			aa_compute_perms(dfa, state, ,
+	 profile->policy.version);
 	}
 	aa_apply_modes_to_perms(profile, );
 	aa_perms_accum_raw(perms, );
diff --git a/security/apparmor/include/perms.h b/security/apparmor/include/perms.h
index 13f20c598448..7d5c0c004978 100644
--- a/security/apparmor/include/perms.h
+++ b/security/apparmor/include/perms.h
@@ -142,7 +142,7 @@ void aa_audit_perm_mask(struct audit_buffer *ab, u32 mask, const char *chrs,
 void aa_apply_modes_to_perms(struct aa_profile *profile,
 			 struct aa_perms *perms);
 void aa_compute_perms(struct aa_dfa *dfa, unsigned int state

Bug#1050256: AppArmor breaks locking non-fs Unix sockets

[John Johansen]

On 1/27/24 01:21, Salvatore Bonaccorso wrote:

Hi John,

On Sun, Dec 31, 2023 at 04:24:47AM +, Mathias Gibbens wrote:

On Sat, 2023-12-30 at 16:44 +0100, Salvatore Bonaccorso wrote:

John, did you had a chance to work on this backport for 6.1.y stable
upstream so we could pick it downstream in Debian in one of the next
stable imports? Cherry-picking 1cf26c3d2c4c ("apparmor: fix apparmor
mediating locking non-fs unix sockets") does not work, if not
havinging the work around e2967ede2297 ("apparmor: compute policydb
permission on profile load") AFAICS, so that needs a 6.1.y specific
backport submitted to sta...@vger.kernel.org ?

I think we could have people from this bug as well providing a
Tested-by when necessary. I'm not feeling confident enough to be able
to provide myself such a patch to sent to stable (and you only giving
an Acked-by/Reviewed-by), so if you can help out here with your
upstream hat on that would be more than appreciated and welcome :)

Thanks a lot for your work!


   I played around with this a bit the past week as well, and came to
the same conclusion as Salvatore did that commits e2967ede2297 and
1cf26c3d2c4c need to be cherry-picked back to the 6.1 stable tree.

   I've attached the two commits rebased onto 6.1.y as patches to this
message. Commit e2967ede2297 needed a little bit of touchup to apply
cleanly, and 1cf26c3d2c4c just needed adjustments for line number
changes. I included some comments at the top of each patch.

   With these two commits cherry-picked on top of the 6.1.69 kernel, I
can boot a bookworm system and successfully start a service within a
container that utilizes `PrivateNetwork=yes`. Rebooting back into an
unpatched vanilla 6.1.69 kernel continues to show the problem.

   While I didn't see any immediate issues (ie, `aa-status` and log
files looked OK), I don't understand the changes in the first commit
well enough to be confident in sending these patches for inclusion in
the upstream stable tree on my own.


Do you had a chance to look at this for 6.1.y upstream?

Asking/Poking since the point release dates are now clear:

https://lists.debian.org/debian-security/2024/01/msg5.html

if possible I would like to include those fixes, but only if they are
at least queued fror 6.1.y itself to not diverge from upstream.

Otherwise we will wait another round, but which means usually 2 months
for the point release cadence.


I am looking at it right now, I should be done with it today

Bug#1050256: autopkgtest fails on debci

[John Johansen]

On 9/4/23 12:32, Michael Biebl wrote:

Am 04.09.23 um 20:23 schrieb Mathias Gibbens:

On Mon, 2023-09-04 at 01:00 -0700, John Johansen wrote:

I took a quick look through v6.1..v6.3.1

there is a patch that I think is the likely fix, it first landed in v6.2

1cf26c3d2c4c apparmor: fix apparmor mediating locking non-fs unix sockets


   Thanks for the pointer John -- I think that is the fix we've been
looking for!

   Commit 1cf26c3d2c4c doesn't apply cleanly to the v6.1 tree due to the
other commits from the patchset of Oct 3, 2022 that modified a bunch of
the apparmor code. Because I couldn't quickly cherry-pick all the
changes without amassing a large diff, I made the small proof-of-
concept patch at the end of this message and applied it to the  6.1.38-
4 kernel from bookworm. Booting with the patched kernel allows services
to start up in containers without any issues. :)

   So, I think the next step should be to get that commit properly
backported to the v6.1 longterm tree and included in an upstream
release. Hopefully that would be able to happen in enough time so that
it is bundled with the kernel updates for bookworm's point release next
month. If not, we should be sure to get it into Debian's packaging so
at least there's a proper fix available.



Thanks for the update Mathias, this looks very promising.
A stable update of the Linux 6.1.x kernel would obviously be the ideal solution.

John, could you help with getting this fix into 6.1.x?



yes, I am working on a patch.

Bug#1050256: autopkgtest fails on debci

[John Johansen]

I took a quick look through v6.1..v6.3.1

there is a patch that I think is the likely fix, it first landed in v6.2

1cf26c3d2c4c apparmor: fix apparmor mediating locking non-fs unix sockets

it matches up the reported audit logs. Unfortunately it does not have a Fixes
tag but as best I can figure it should be applied all the way back to.

56974a6fcfef apparmor: add base infastructure for socket mediation

how/where this bug surfaces partly depends on the userspace policy and
compiler which combines the features set supported by the kernel with what
policy claims to support. So it is possible to have an affected kernel
but not trigger the bug.

Bug#963493: Repeatable hard lockup running strace testsuite on 4.19.98+ onwards

[John Johansen]

On 6/26/20 9:50 AM, Steve McIntyre wrote:
> Hi Jann,
> 
> On Fri, Jun 26, 2020 at 04:25:59PM +0200, Jann Horn wrote:
>> On Fri, Jun 26, 2020 at 3:41 PM Greg KH  wrote:
>>> On Fri, Jun 26, 2020 at 12:35:58PM +0100, Steve McIntyre wrote:
> 
> ...
> 
 Considering I'm running strace build tests to provoke this bug,
 finding the failure in a commit talking about ptrace changes does look
 very suspicious...!

 Annoyingly, I can't reproduce this on my disparate other machines
 here, suggesting it's maybe(?) timing related.
>>
>> Does "hard lockup" mean that the HARDLOCKUP_DETECTOR infrastructure
>> prints a warning to dmesg? If so, can you share that warning?
> 
> I mean the machine locks hard - X stops updating, the mouse/keyboard
> stop responding. No pings, etc. When I reboot, there's nothing in the
> logs.
> 
>> If you don't have any way to see console output, and you don't have a
>> working serial console setup or such, you may want to try re-running
>> those tests while the kernel is booted with netconsole enabled to log
>> to a different machine over UDP (see
>> https://www.kernel.org/doc/Documentation/networking/netconsole.txt).
> 
> ACK, will try that now for you.
> 
>> You may want to try setting the sysctl kernel.sysrq=1 , then when the
>> system has locked up, press ALT+PRINT+L (to generate stack traces for
>> all active CPUs from NMI context), and maybe also ALT+PRINT+T and
>> ALT+PRINT+W (to collect more information about active tasks).
> 
> Nod.
> 
>> (If you share stack traces from these things with us, it would be
>> helpful if you could run them through scripts/decode_stacktrace.pl
>>from the kernel tree first, to add line number information.)
> 
> ACK.
> 
>> Trying to isolate the problem:
>>
>> __end_current_label_crit_section and end_current_label_crit_section
>> are aliases of each other (via #define), so that line change can't
>> have done anything.
>>
>> That leaves two possibilities AFAICS:
>> - the might_sleep() call by itself is causing issues for one of the
>> remaining users of begin_current_label_crit_section() (because it
>> causes preemption to happen more often where it didn't happen on
>> PREEMPT_VOLUNTARY before, or because it's trying to print a warning
>> message with the runqueue lock held, or something like that)
>> - the lack of "if (aa_replace_current_label(label) == 0)
>> aa_put_label(label);" in __begin_current_label_crit_section() is
>> somehow causing issues
>>
>> You could try to see whether just adding the might_sleep(), or just
>> replacing the begin_current_label_crit_section() call with
>> __begin_current_label_crit_section(), triggers the bug.
>>
>>
>> If you could recompile the kernel with CONFIG_DEBUG_ATOMIC_SLEEP - if
>> that isn't already set in your kernel config -, that might help track
>> down the problem, unless it magically makes the problem stop
>> triggering (which I guess would be conceivable if this indeed is a
>> race).
> 
> OK, will try that second...
> 

I have not been able to reproduce but

So looking at linux-4.19.y it looks like
1f8266ff5884 apparmor: don't try to replace stale label in ptrace access check

was picked without
ca3fde5214e1 apparmor: don't try to replace stale label in ptraceme check

Both of them are marked as
Fixes: b2d09ae449ced ("apparmor: move ptrace checks to using labels")

so I would expect them to be picked together.


ptraceme is potentially updating the task's cred while the access check is
running.

Try building after picking
ca3fde5214e1 apparmor: don't try to replace stale label in ptraceme check

Bug#914370: [apparmor] Bug#914370: cups-daemon: AppArmor profile allows cupsd to create setuid binaries under /etc

[John Johansen]

On 12/16/18 6:05 AM, intrigeri wrote:
> Hi,
> 
> (+ AppArmor upstream mailing list as I don't feel sufficiently
> knowledgeable to provide authoritative answers or guidance)
> 
> Didier 'OdyX' Raboud:
>> Le jeudi, 22 novembre 2018, 19.05:19 h CET deb...@dbwats.plus.com a écrit :
>>> The AppArmor profile supplied with cupsd isn't much use against local
>>> attackers, as it allows cupsd to create setuid binaries at paths it
>>> can write to (e.g. under /etc/cups).  Since cupsd is run as root by
>>> default, these binaries can be setuid root.
>>>
>>> (…)
>>>
>>> In default installations /etc is not on a nosuid mount, so provided
>>> that they have a suitable exploit, local attackers who are unconfined
>>> but non-root can use cupsd to create a setuid binary, then run the
>>> binary themselves to gain unconfined root privileges.

This is a known issue with unconfined. I can say there are some changes
coming that will help. With the setuid issue, and also with creating
the setuid binaries.

1. profile attachments are going to gain additional ability around
setuid. So it will be possible to create policy that can trap these
type of execs.

2. policy will be picking up extended permission allowing it to control
the creation of setuid/setguid files etc, so the cups profile will be
able to block the creation of these files.

I can't give a time line for when these will land but I can say they
won't make 4.21

> 
> Right. AppArmor does a decent job at sandboxing individual processes
> but it offers little protection against a group of processes, running
> under different policies, that cooperate to escape their sandbox or
> otherwise escalate their privileges. The shared bits of a Linux system
> that various processes can access are simply too many to write
> AppArmor policy that successfully protects against such attacks
> without using other isolation techniques. This bug report exemplifies
> this, one of these cooperating processes here being mostly unconfined
> (local user with arbitrary code execution as non-root) while the other
> runs as root under a rather loose AppArmor policy.
> 

Indeed. AppArmor in its current form is not well suited to total system
confinement. It can be done but there are limitations. Instead policy
has focused on application confinement/sandboxing.

If a local user is not trusted they should not be unconfined,
unconfined was designed specifically so that apparmor would not alter
standard system behavior for the unconfined application. It is
possible to confine users (though harder than it should be atm) and
still have application confinement

However as intrigeri points out sharing at wrietable locations is
currently problematic and one of the missing pieces that we need to
land before apparmor can be effectively used

>> @Intri: any insight in how to address this?
> 
> First, sorry for the delay, I've had less time than usual for Debian
> recently. Thankfully I'm now back! :)
> 
> tl;dr: AFAICT this is a known AppArmor limitation and there's nothing
> we can do about it as long as cupsd runs as root (I assume we would
> already be running it under a non-privileged user if that was easily
> doable).
> 

Policy can be adjusted to include trap profiles that will attach
to binaries executed out of these directories. The trap profile
can grant limited to no permissions.

> There's no fine-grained enough Linux capability to fix this and
> there's nothing in the AppArmor policy language to express "not
> allowed to give files the setuid/setgid bits". I don't know if the
> existing LSM hooks are sufficient for AppArmor to add such support
> both in the kernel and in the policy language. But even if that
> specific instance of the "groups of processes can cooperate to
> escalate privileges" was fixed, the broader problem would remain.
> AppArmor folks, can you confirm and maybe shed some light upon what
> options we have here, both short and long term?
> 

short term: confine users & a trap profile(s) on the /etc/cups dir
long term: apparmor will pickup permissions to control setuid/setguid
   both at the file creation level and at the profile
   attachment level.

Bug#883584: [apparmor] Bug#883584: A reload deletes /etc/apparmor.d/cache/CACHEDIR.TAG

[John Johansen]

On 01/07/2018 07:26 AM, intrigeri wrote:
> intrigeri:
>> intrigeri:
>>> Dear upstream/parser developers, would it feel crazy to modify
>>> clear_cache_cb to ignore the passed file if its basename is
>>> CACHEDIR.TAG? Or should _aa_dirat_for_each get a list of excluded file
>>> names as a new argument, or something similar?
> 
>>> If any of these approaches seems acceptable, is anyone around willing
>>> to write this patch, or should I try to find a C person elsewhere?
> 
>> Ping?
> 
> After thinking about it, as discussed on the upstream AppArmor mailing
> list I'd rather move the binary cache to /var/cache which should make
> this suggested change unnecessary.
> 
> Sorry for the too hasty ping!
> 

Well that isn't reason to add another file type that will be skipped. It
should be a fairly easy modification. In fact the cache isn't necessarily
the only dir we might want to skip so

  SKIPDIR.TAG
or
  IGNOREDIR.TAG

might even be better to have

Bug#883703: apparmor: Feature pinning breaks mount

[John Johansen]

On 01/06/2018 07:50 AM, intrigeri wrote:
> Hi John,
> 
> John Johansen:
>> Attached is the patch for the kernel that is currently in testing
> 
>> From 1aa96ec6d0fce613e06fa4d073c8cf3e183989da Mon Sep 17 00:00:00 2001
>> From: John Johansen <john.johan...@canonical.com>
>> Date: Thu, 7 Dec 2017 00:28:27 -0800
>> Subject: [PATCH] apparmor: fix regression in mount mediation when feature set
>>  is pinned
>> MIME-Version: 1.0
>> Content-Type: text/plain; charset=UTF-8
>> Content-Transfer-Encoding: 8bit
> 
>> When the mount code was refactored for Labels it was not correctly
>> updated to check whether policy supported mediation of the mount
>> class.  This causes a regression when the kernel feature set is
>> reported as supporting mount and policy is pinned to a feature set
>> that does not support mount mediation.
> 
> What's the status of this patch?
> 
it is in 4.15-rc7, and has started working its way into the 4.14 stable
tree, I expect it will be in the 4.14.13 stable release.

> Context & meta: I'd like to pin the feature set to 4.9's in Debian
> Stretch (and Tails) ASAP but if I do this now, I'll break "mount"
> operations for all confined software. I appreciate the work you're
> putting into the longer term, nicer solution (policy versioning); I'm
> confident it will make things better for future stable releases of our
> distros; but sadly it won't fix the problems we currently have in the
> already released LTS distros that won't backport big kernel patch sets
> to their stable kernel, so on the short term what we need, at least in
> Debian and Tails, is bugfixes in the feature set pinning facility.
> 

Bug#885775: [apparmor] Bug#885775: apparmor: Apparmor triggers NULL pointer dereference in kernel 4.14.7-1 when updating with aptitude

[John Johansen]

On 12/29/2017 11:29 PM, intrigeri wrote:
> Control: forwarded -1 appar...@lists.ubuntu.com
> 
> Hi AppArmor kernel developers,
> 
> we got this bug reported in Debian:
> 
> Kertesz Laszlo:
>>* What led up to the situation?
>>  Installed kernel 4.14 in Debian Testing and ever since at every upgrade 
>> where systemd or other important 
>>  packages were upgraded the system froze with the last 2 lines in the 
>> system log:
> 
>>  Dec 29 21:25:26 laca-desktop kernel: BUG: unable to handle kernel NULL 
>> pointer dereference at 0005
>>  Dec 29 21:25:26 laca-desktop kernel: IP: __task_pid_nr_ns+0xc7/0xf0
> 
>>  The system became unresponsive, not even the sysrq combinations were 
>> working
>>* What exactly did you do (or not do) that was effective (or
>>  ineffective)?
>>  Restarted ran dpkg --configure -a and every time the system froze
>>  Booting with the 4.13 kernel was fine, dpkg finished its run
> 
>>  At the next upgrade i still got the freeze, after reboot i disabled the 
>> kernel security features with the kernel command line security=false
> 
>>* What was the outcome of this action?
>>  dpkg was working fine with the kernel 4.14 too
> 
> Do you need more info from me or from the bug reporter (Kertesz
> Laszlo, Cc'ed)?
> 

yes please. This really isn't enough info to debug off of. AppArmor
does not call __task_pid_nr_ns() nor even any of the fns() that call
it directly.

We need more of a call trace to even have a chance of chasing this
down.

Bug#883703: apparmor: Feature pinning breaks mount

[John Johansen]

Attached is the patch for the kernel that is currently in testing

>From 1aa96ec6d0fce613e06fa4d073c8cf3e183989da Mon Sep 17 00:00:00 2001
From: John Johansen <john.johan...@canonical.com>
Date: Thu, 7 Dec 2017 00:28:27 -0800
Subject: [PATCH] apparmor: fix regression in mount mediation when feature set
 is pinned
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When the mount code was refactored for Labels it was not correctly
updated to check whether policy supported mediation of the mount
class.  This causes a regression when the kernel feature set is
reported as supporting mount and policy is pinned to a feature set
that does not support mount mediation.

BugLink: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=882697#41
Fixes: 2ea3ffb7782a ("apparmor: add mount mediation")
Reported-by: Fabian Grünbichler <f.gruenbich...@proxmox.com>
Cc: Stable <sta...@vger.kernel.org>
Signed-off-by: John Johansen <john.johan...@canonical.com>
---
 security/apparmor/mount.c | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/security/apparmor/mount.c b/security/apparmor/mount.c
index 82a64b58041d..e395137ecff1 100644
--- a/security/apparmor/mount.c
+++ b/security/apparmor/mount.c
@@ -330,6 +330,9 @@ static int match_mnt_path_str(struct aa_profile *profile,
 	AA_BUG(!mntpath);
 	AA_BUG(!buffer);
 
+	if (!PROFILE_MEDIATES(profile, AA_CLASS_MOUNT))
+		return 0;
+
 	error = aa_path_name(mntpath, path_flags(profile, mntpath), buffer,
 			 , , profile->disconnected);
 	if (error)
@@ -381,6 +384,9 @@ static int match_mnt(struct aa_profile *profile, const struct path *path,
 	AA_BUG(!profile);
 	AA_BUG(devpath && !devbuffer);
 
+	if (!PROFILE_MEDIATES(profile, AA_CLASS_MOUNT))
+		return 0;
+
 	if (devpath) {
 		error = aa_path_name(devpath, path_flags(profile, devpath),
  devbuffer, , ,
@@ -559,6 +565,9 @@ static int profile_umount(struct aa_profile *profile, struct path *path,
 	AA_BUG(!profile);
 	AA_BUG(!path);
 
+	if (!PROFILE_MEDIATES(profile, AA_CLASS_MOUNT))
+		return 0;
+
 	error = aa_path_name(path, path_flags(profile, path), buffer, ,
 			 , profile->disconnected);
 	if (error)
@@ -614,7 +623,8 @@ static struct aa_label *build_pivotroot(struct aa_profile *profile,
 	AA_BUG(!new_path);
 	AA_BUG(!old_path);
 
-	if (profile_unconfined(profile))
+	if (profile_unconfined(profile) ||
+	!PROFILE_MEDIATES(profile, AA_CLASS_MOUNT))
 		return aa_get_newest_label(>label);
 
 	error = aa_path_name(old_path, path_flags(profile, old_path),
-- 
2.11.0

Bug#883703: apparmor: Feature pinning breaks mount

[John Johansen]

On 12/06/2017 10:09 AM, intrigeri wrote:
> Hi,
> 
> Felix Geyer:
>> With feature pinning enabled the parser seem to not load the mount rules but 
>> the
>> kernel still somewhat enforces mount mediation.
> 
>> For example starting a libvirt qemu VM fails with:
>> AVC apparmor="DENIED" operation="mount" info="failed mntpnt match" error=-13 
>> profile="/usr/sbin/libvirtd" name="/" pid=8043 comm="libvirtd" flags="rw, 
>> rslave"
> 
>> The libvirtd profile simply has a "mount," rule.
> 
>> See also https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=882697#41
>> (same problem with stretch-pu)
> 
> Ouch. We had a similar problem for network rules but I had no idea we
> have one for mount rules as well (I'm running without the pinning
> myself, in order to identify issues early so we can update the policy
> before we bump the pinned feature set).
> 
> For sid, I think we should simply bump the pinned feature set to
> 4.14's: it's easier to fix policy than to deal with kernel bugs.
> Cc'ing John so he's aware of this kernel bug.
> 
> For Stretch, my proposed update shall be reverted. I'll follow up on
> the corresponding release.d.o bug.
> 

Ouch sorry, I'll get a patch together for the kernel. With that said
it is possible to have a work around in the compiler so userspace
patching is possible, if we hit a need to do so to support an
existing release.

Bug#879584: apparmor: Pin the AppArmor feature set to Linux 4.12's or 4.13's until our policy has been updated for Linux 4.14

[John Johansen]

On 10/23/2017 10:30 AM, intrigeri wrote:
> Hi,
> 
> (John, there's a question for you below and I'll like you to
> double-check my testing procedure :)
> 
> intrigeri:
>>> 1. in testing/sid, ship a conffile (in a package built from
>>>src:apparmor) that pins the most recent feature set fully supported
>>>by our policy, i.e. Linux 4.12's or 4.13's (depending on whether
>>>we've fixed all the regressions brought by 4.13 yet); this should
>>>make the transition to Linux 4.14 smooth;
> 
> Implemented locally.
> 
> All test results below look good to me so I'll wait for John's answer
> and will then upload to id.
> 
> Sadly, that this is not fully effective in the specific case of
> 4.14-rcN up to 4.14-rc6 due to a kernel bug with pinned older feature
> sets, that will likely be fixed in Linux 4.14-rc7. For example, with
> Linux 4.14-rc5 some network (e.g. unix, inet, inet6) operations are
> denied despite the fact this pinned feature does not enable network
> mediation support. For details, see:
> https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/1721278
> A fix is being tested and will hopefully be sent to Linus today.
> 
Testing so far has been good, I have a volunteer who has been encountering
this issue testing as well.

>> I'll need to test what happens when upgrading the feature set file +
>> Linux in lockstep, when the new feature set includes features brought
>> by the Linux upgrade and not supported by the running kernel.
>> This will happens when upgrading to the next Debian stable, or on
>> a rarely upgraded testing/sid system. It'll be solved on reboot but
>> I want to ensure the package upgrade succeeds.
> 
> It seems to work just fine. Here's how I tested it:
> 
> 1. boot sid with Linux 4.14-rc5 from experimental
> 2. save the 4.14-rc5 feature set:
>cp /etc/apparmor.d/cache/.features /etc/apparmor/features-4.14
this step is likely not needed as apparmor will detect the feature
set doesn't match and update the features as it recompiles to the
new feature set. That is unless you have not enabled writing cache
files during init, in which case apparmor will just not update the
cache file and basically you won't get cache hits except for on
kernels that match those of the feature file.

> 3. boot sid with Linux 4.13 from sid
might want to test that the features file was updated, a timestamp
since boot should be all you need to check

> 4. save the 4.13 feature set to /etc/apparmor/features:
>cp /etc/apparmor.d/cache/.features /etc/apparmor/features
> 5. add features-file=/etc/apparmor/features to /etc/apparmor/parser.conf
> 6. reboot on Linux 4.13 again
> 7. cp /etc/apparmor/features{-4.14,}
this step is not needed

> 8. systemctl restart apparmor
this step is not needed either, but if you want to get the service
message without digging it shouldn't hurt either.

> 9. look at what happened:
>- apparmor.service was successfully restarted
>- the restart logs say 'apparmor="STATUS"
>  operation="profile_replace" info="same as current profile,
>  skipping"'
>- the policy has been successfully reloaded (actually not as said
>  above, but that's fine)
>- the output of aa-status is what I would expect
> 
looks good

> I'm kinda surprised that apparmor_parser does not complain about being
> explicitly told to enable features that the kernel does not support,

Well it can, you can use
--warn=rule-downgraded

which means some enforcement of the rule but at a coarser level, eg.
a unix rule to network unix,

and
--warn=rule-not-enforced

which means no enforcement

you can enable them by default by setting them in /etc/apparmor/parser.conf

# enable rule downgrade warnings by default
warn=rule-not-enforced
warn=rule-downgraded

see
apparmor_parser --help=warn for all supported options in a given parser

what currently isn't supported is failing the compile based on enforcement
being downgraded or not enforced.

btw that trick works for Optimize and Dump
apparmor_parser --help=O
apparmor_parser --help=D



> but OTOH its --help says "--features-file n Use only features in file
> n" that I understand as the features file is only used to exclude
> features not listed in there, and the parser actually uses the
> intersection of the features specified in that file and the features
> actually supported by the kernel. John, did I guess right?
> 
nope, but the result is the same.

It will compile it to the given feature set. If the kernel supports
that given abi it will then do the other half providing the intersection.

This is done to allow policies from different versions to be loaded and
used simultaneously. Think of a buster host running an lxd stretch
guest, sharing the same kernel but enforcing different version of
policy.

>> A special case of this general problem is when one intentionally runs
>> an older kernel than the one whose feature set we've pinned. I did not
>> test this yet. My understanding is that the worst that can happens is
>> that one gets 

Bug#877255: Pux is a deprecated syntax

[John Johansen]

The Pux syntax is deprecated because it contains mixed case qualifiers
on an x rule.

Capital case qualifiers indicate that scrubbing of certain dangerous
environment variables should be done. While lower case qualifiers
indicate that the environment should be passed unchanged. In the case
of Pux you have both which is a conflict and confusing.

Please use
  PUx - for cases where you want the environment to be scrubbed and
  pux - for cases where you do not want the environment to be scrubbed.

While the parser currently supports the old style syntax it should be
outputting a warning about deprecated syntax. The wiki obviously needs
to be updated to document this change but the man page has been
updated, it would be nice if there was a note about Pux/pUx being
deprecated. The profile development tools have been patched and no
longer support the old format, though it would be nice if the could
output a warning and perform the conversion.

Bug#871441: apparmor: Including tunables/sys to tunables/global?

[John Johansen]

On 09/20/2017 07:32 AM, intrigeri wrote:
> Control: tag -1 + upstream
> 
> Hi,
> 
> Vincent Blut:
>> /etc/apparmor.d/tunables/proc being part of
>> /etc/apparmor.d/tunables/global, I’m wondering if there are any reasons
>> preventing the sysfs pseudo file system location variable (defined in
>> /etc/apparmor.d/tunables/sys) from being included as well?
> 
> Good question! I have no idea.
> 
> I see that tunables/sys was introduced in 2012 by John (Cc'ed) as part
> of a commit that adds "abstractions to support the apparmor api".
> On my system, nothing uses these abstractions nor the @{sys} tunable.
> So I admit I have no idea what problem @{sys} is meant to solve.
> If it _is_ useful then it should be used everywhere instead of /sys/,
> which requires quite some work for no obvious (to me) benefit.
> 
> John, what do you think?
> 
yeah, I think it would be worth starting to do the conversion of
/sys/ to @{sys} as has been done with /proc/ to @{proc}

with that said I haven't ever seen sys mounted somewhere different
than /sys/ where I have seen that for proc.

The big win of course is when fstype conditionals land at which
point @{sys} could be further restricted to be /sys/ with and
fs type of sysfs or even allowing disconnected access to sysfs.

As for why this was introduced as part of the api abstraction
profile management is done through sys and you probably haven't
seen it because its not currently common to confine services
doing profile management.

I expect that will change more in the future as we open up policy
namespaces more, which will safely allow users and applications
to load their own policy.

Bug#872726: linux: apparmor doesn't use proper audit event ids

[John Johansen]

On 09/09/2017 10:25 AM, intrigeri wrote:
> Hi Laurent & everyone else who's listening!
> 
> Laurent Bigonville:
>> Le 03/09/17 à 13:01, intrigeri a écrit :
>>> Laurent Bigonville:
 IMVHO, in regard to the recent proposal of enabling apparmor in debian
 by default, this needs to be addressed first.
>>> I'm genuinely curious why this should be a blocker for Debian: this is
>>> not obvious to me as a number of distros could enable AppArmor by
>>> default and can apparently live with this bug.
>>>
>>> Can you please make it explicit, e.g. describing what exact use cases
>>> would be harmed by enabling AppArmor by default without fixing this
>>> bug first?
> 
>> I think that having the denials of a MAC properly logged is important for 
>> both people
>> developing their policy and also for intrusion/non conformity detection.
> 
define properly logged, apparmor uses the common LSM audit subsystem
the same as smack, and selinux.

>> If someone wants to send their logs to some logging services 
>> (ELK/splunk/...) having
>> the messages properly logged/categorized seems to be the start here.
> 
> I see, thanks for the explanation! I agree it's important and I'm sad
> this bug makes AppArmor look a bit rough around the edges (because
> that doesn't correctly reflect my experience as a user, sysadmin and
> distro maintainer).
> 
> I'll look closer at each of these use cases from the "what would we
> *break* if we enabled AppArmor by default" perspective:
> 
>  * people developing their own AppArmor policy: 100% of the existing
>AppArmor policy was developed without proper audit event IDs;
>I'll be happy to give ausearch(8) a try once AppArmor does the
>right thing, but so far I can very well live without it.
> 
>⇒ from a user-centric perspective, I don't see why this bug would
>be a blocker as far as this use case is concerned.
> 
It complicates things because while all LSMs using the common LSM audit
frame work share 1400, the userspace end of audit only has fields
for selinux defined for 1400.

That is there is a disconnect between userspace and kernel. Userspace
has argued the kernel should change.

>  * intrusion detection: the current state of things in Debian (no MAC
>framework enabled by default) does not give any such capability to
>system administrators; the proposal of enabling AppArmor by default
>does not pretend it'll magically give this bonus feature.
> 
>⇒ AppArmor improves other things, just not this one, or at least
>not in an ideal way; too bad, but I don't see why this limitation
>would be a blocker.
> 
>  * centralized logging: enabling AppArmor by default will generate
>audit events; if nothing improves on this front, they'll be
>erroneously tagged type=1400 and thus might land into a SELinux
>bucket or similar by mistake, which can be confusing for sysadmins
>who also run SELinux-enabled systems, and even more so once one can
>stack SELinux and AppArmor.
> 
It depends on what you mean by erroneously tagged. AppArmor used to have
the audit range 1500-1599. When the LSM audit infrastructure was created
all LSMs using itwere moved to have id 1400. This was a deliberate choice
made by the kernel community, alls LSMs that use this are affected and
apparmor has had to live with it.

>From an audit tagging perspective 1500 was much better, I would like
to see apparmor move back to 1500 but apparmor will continue to
use the common LSM audit infrastructure, so the change needs to
be agreed to by more than just the apparmor devs.

>⇒ I understand there *is* definitely potential for painful impact
>here. We'll need to keep an eye on this when evaluating "AppArmor
>by default in Buster?" 1 year after it's been enabled by default
>(as per my proposal on -devel@). But I bet this bug would have been
>fixed a while ago if it was a serious problem in practice:
>apparently, tons of Ubuntu and SUSE sysadmins have apparently
>managed to cope with this bug just fine for many years.
> 
> So I see very little ground for arguing this bug is a blocker for
> enabling AppArmor by default in testing/sid, and reconsidering a year
> later — after all, it's one of the purposes of the evaluation
> exercises: nobody claims AppArmor is perfect, and what's at stake is
> rather whether it brings more value than costs for Debian and its
> users. The best, the good, enemies, etc. :)
> 
> If I misunderstood something, sorry! I'm all ears.
> 
> Cheers,
> 

Bug#865206: [apparmor] Bug#865206: apparmor: Should apparmor abstractions allow flatpak directories?

[John Johansen]

On 06/30/2017 11:20 AM, intrigeri wrote:
> Control: tag -1 + upstream
> 
> Hi Diane,
> 
> Diane Trout:
>> I was updating my browser profiles and saw firefox was trying to load some
>> flatpak mime exports.
> 
>> Should the apparmor profiles allow those?
> 
> Good question, thanks for raising this topic. I'm redirecting this
> discussion to the upstream AppArmor mailing list, as I think it is not
> Debian-specific.
> 
> Logs are at https://bugs.debian.org/865206.
> 

So this very much depends on the policy style you want. The firefox
profile in its current form is very permissive. And I don't see a
problem adding them to it and an abstraction does seem the right place
to do it so 

For a tighter policy where enumerating other application etc is not
allowed then we would want to block access. I don't think we can do
that well with applications like firefox until support for delegation
lands. At which point we are going to have to either reworking the
reference policy or splitting it into different types dependent on
your wants/needs.

Bug#805002: [pkg-apparmor] Bug#805002: libvirt-client: "virsh attach-disk" fails with AppArmor enabled

[John Johansen]

On 07/30/2016 07:54 AM, intrigeri wrote:
> Hi,
> 
> Christian Boltz:
>> I think you are misreading the documentation here ;-)
> 
> I suspect it might be easier to improve the documentation,
> than to fix all people who would "misread" it.
> 
> (Sorry I did not find this funny.)
> 
>> OTOH, if you already have a profile loaded, start a process and then 
>> reload the modified profile, it will be applied instantly.
> 
> Thanks!
> 
>> Note that there were bugs both in apparmor_parser and the kernel that 
>> broke reload and could cause the problem you described. So please check 
>> if Debian has the fixes in apparmor_parser (likely, because this was fixed 
>> a while ago) and the kernel (less likely because that patch is quite 
>> new). If in doubt, John should be able to point you to the relevant 
>> patches.
> 
> Good to know! Indeed, I have no clue what kernel patch you're
> referring to ⇒ John, can you please point me to it? Is it part of the
> pull request for 4.8? Thanks in advance!
> 
Yes, and also available in the 4.8 fixes backports I did for 4.4 - 4.7 (I
haven't had time to backport further yet).

git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor
v4.4-aa2.8-out-of-tree
v4.5-aa2.8-out-of-tree
v4.6-aa2.8-out-of-tree
v4.7-aa2.8-out-of-tree

once the 4.8 request gets merged I can look at submitting to stable.

the specific patch for this issue is
In linux security/next
  ec34fa2 apparmor: fix replacement bug that adds new child to old parent

v4.4-aa2.8-out-of-tree
  b02fdc2 apparmor: fix replacement bug that adds new child to old parent


The kernel side messes up in the specific case of a profile already existing
and the replacement adds new hats.

The userspace fix is rev 3440 in the userspace main branch (lp:apparmor)

Bug#826218: [pkg-apparmor] Bug#826218: Bug#826218: Complain still interferes

[John Johansen]

On 06/05/2016 11:22 PM, Guido Günther wrote:
> Hi Christian,
> 
> Thanks a lot for your comments!
> 
> On Mon, Jun 06, 2016 at 01:14:08AM +0200, Christian Boltz wrote:
> [..snip..]
>> You can enable the logging by adding the audit keyword, but the general 
>> rule is not to log anything that is already handled (allowed or denied) 
>> in the profile.
>>
>>> * a way to audit calls to subprocesses indicating whether the
>>>   environment was scrubbed or not
>>
>> You'll get this information by reading the profile ;-)   It already had 
>> "/usr/sbin/* PUx" [1] which also allowed /usr/sbin/virtlogd - but with 
>> environment scrubbing.
> 
> The rule tells me what the profile is supposed to do, not what it is
> doing. Ideally I'd like to debug this without  modifying any rules.
> 
> It's like the difference between reading code and debugging code. With a
> debugger I can see what the program is doing while the code tells me
> what it is supposed to do.
> 

Another mode that might be helpful is forcing audit mode. This can be done
in two ways, to apply it globally

  echo -n all > /sys/module/apparmor/parameters/audit

or by setting the mode on the profile

  profile foo flags=(audit) { ... }


this will audit a message for every rule that apparmor allows. This can
be extremely noisy even just doing it for a single profile, so I would not
use the global option.

You will likely also have to turn off rate limiting by doing

  echo 0 > /proc/sys/kernel/printk_ratelimit

even then the kernel ring buffer may overflow and you might lose messages.


Ideally I would like to add a full debug mode back in to apparmor, but
it was so noisy and lossy in the past that it was almost useless. It
got ripped out in the cleanup/rewrite and will resurface when we can
better control it. We do have some ideas, like being able to specify
a set of rules to trigger events (audit, or signal on). A signal to stop
tasks so a debugger can have a chance to introspect and catch up on
the debug stream etc. Not that that helps you as it is not available
yet.

Bug#826218: [pkg-apparmor] Bug#826218: Bug#826218: Complain still interferes

[John Johansen]

On 06/05/2016 04:14 PM, Christian Boltz wrote:
> Hello,
> 
> Am Sonntag, 5. Juni 2016, 13:34:19 CEST schrieb Guido Günther:
>> On Sat, Jun 04, 2016 at 06:38:46PM +0200, Christian Boltz wrote:
>>> deny rules are enforced even if you switch the profile to complain
>>> mode, and don't leave any log events behind. You might want to
>>> change them to"audit deny" temporarily to get log events (with
>>> AUDIT).
>>
>> I did not know. Thanks! IMHO this needs to be mentioned in the
>> aa-complain manpage to fulfill the "no PhD in computer science needed
>> for" promise.
> 
> Good point. I just commited an updated manpage upstream (will be in 
> 2.11, 2.10.2 and 2.9.4 whenever they get released).
> 
>> The issue turned out to be environment scrubbing:
>>
>> https://www.redhat.com/archives/libvir-list/2016-June/msg00117.html
>>
>> but I think the issue is still valid: getting an idea what gets
>> dropped to the floor is too hard atm. With complain mode I'd exepct:
>>
>> * denials logged by default
> 
> The whole point of deny rules is to silence the logging  (except if they 
> also have the audit keyword).
> 
> You can enable the logging by adding the audit keyword, but the general 
> rule is not to log anything that is already handled (allowed or denied) 
> in the profile.

I will add that new versions of apparmor will be picking up a new mode
that will not apply quieting for known denials.

> 
>> * a way to audit calls to subprocesses indicating whether the
>>   environment was scrubbed or not
> 
> You'll get this information by reading the profile ;-)   It already had 
> "/usr/sbin/* PUx" [1] which also allowed /usr/sbin/virtlogd - but with 
> environment scrubbing.
> 
> I'm CC'ing another upstream developer, but I wouldn't be surprised if he 
> tells you the same ;-)
> 
So the logging of scrubbing is not where I would like at the moment. We do
have work planned around improving environment variable handling so this
hasn't been touched yet.

With that said if you turn of debug mode apparmor will log a few extra
messages to dmesg (not via the audit subsystem). This will let you see
when environment scrubbing has been applied.

  echo 1 > /sys/module/apparmor/parameters/debug

Also not this isn't going to give you a flood of extra messages its just
for a few things like, env scrubbing, clearing unsafe personality bits,
no new privs etc.

> @John: Do you have a different opinion on Guido's points?
> 

yeah we should be logging extra info. As for complain mode we aren't
changing its behavior but their will be a new mode that is closer to
what I think he wants.

Also it is possible to turn off deny audit quieting by doing

echo -n noquiet >/sys/module/apparmor/parameters/audit

sadly this is global, not per profile

>> * other stuff I might not even know about yet like DBus denials …
> 
DBus denials are weird in that they are handled by an apparmor extension
inside of the user space daemon which will use the audit subsystem if it
can (system bus + audit subsystem is set up) but will fallback to regular
syslog if it can't.

So session bus and system bus can end up in different logs, and there
are a few other oddities. There is some work that could be done to improve
it but right now priorities are else where, and there is a hesitancy
to put a lot more effort into it until the whole kdbus thing is more
clear.

> Actually I can't tell you too much about DBus because only the Ubuntu 
> kernel has DBus support for AppArmor (it's not upstreamed yet), and I'm 
> using openSUSE ;-)
> 
right, sorry I still mean to get you experimental kernels in the build
service. I have just been side tracked.

Bug#735470: [apparmor] Bug#735470: Fwd: Bug#735470: Could be implemented centrally with a dpkg trigger instead of requiring every package shipping an apparmor file to use dh_apparmor

[John Johansen]

On 01/17/2014 02:26 AM, Didier 'OdyX' Raboud wrote:
 Le jeudi, 16 janvier 2014, 14.49:06 Kees Cook a écrit :
 On Thu, Jan 16, 2014 at 07:37:04PM +0100, Didier 'OdyX' Raboud wrote:
 man deb-trigggers contradicts you, in my reading; an 'activate
 /etc/apparmor.d' triggers' file in apparmor would make its action
 run _before_ cups (which would have shipped
 /etc/apparmor.d/usr.sbin.cupsd) would be 'configured' (hence its
 postinst run).

 Right, sorry, you are right, but my original observation stands: we
 should never reload all apparmor profiles when installing a single
 profile. Just the single profile should be reloaded. Otherwise we end
 up doing very CPU expensive work for no reason. The point of
 dh-apparmor is to reload a single profile, not all of them.
 
 That's quite easily circumvented in the trigger code by maintaining a 
 list of timestamps for the various apparmor.d/* files, as is done for 
 cups:
 
 http://sources.debian.net/src/cups/1.7.1-2/debian/cups.postinst#L181
 
 Then the trigger can reload only the concerned profiles, and never do it 
 for all of them. (Using the dpkg hashsums instead of timestamps would 
 allow doing it only for _changed_ profiles too.)
 
 I'll try implementing something along those lines this week-end.
 
Its not just profile time stamps, any update to any of the include files
the profile depends on must also be checked.

The parser does check those dependent time stamps at the moment. What it
doesn't do is abort loading the profile if the cached version is up to
date.

The parser could and should pickup the ability to introspect the kernel
and abort the load if the profile is there. Which would fix the problem
for kernels that have the introspection patch, or 3.12+ kernels. For
other kernels we can just always load or fallback to the -u flag style
patch.

The -u flag patch I posted is a work around that could work from triggers.
Basically it tells the parser to make the assumption that if there is
a cached profile and it is good to assume you are doing an update and
to skip loading it to the kernel.

This should not be used in initial load from the init script because
you can't assume a profile is loaded. But it works for packaging hooks
because if the package adds a profile or updates a profile or dependencies
it will get properly loaded


-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#735470: [apparmor] Bug#735470: Fwd: Bug#735470: Could be implemented centrally with a dpkg trigger instead of requiring every package shipping an apparmor file to use dh_apparmor

[John Johansen]

On 01/16/2014 02:49 PM, Kees Cook wrote:
 On Thu, Jan 16, 2014 at 07:37:04PM +0100, Didier 'OdyX' Raboud wrote:
 Le jeudi, 16 janvier 2014 10.14:14, vous avez écrit :
 On Thu, Jan 16, 2014 at 11:11:22AM +0100, Didier 'OdyX' Raboud wrote:
 As far as I understand deb-triggers' manpage, this can be enforced
 using 'activate /etc/apparmor.d/', which will then make the trigger
 run at the start of the configure operation, which ensures
 exactly what you want.

 Per-policy reloads must happen before a daemon restarts, so they
 cannot be triggers.

 Err…

 man deb-trigggers contradicts you, in my reading; an 'activate 
 /etc/apparmor.d' triggers' file in apparmor would make its action run 
 _before_ cups (which would have shipped /etc/apparmor.d/usr.sbin.cupsd) 
 would be 'configured' (hence its postinst run).

 Isn't it?
 
 Right, sorry, you are right, but my original observation stands: we should
 never reload all apparmor profiles when installing a single profile. Just
 the single profile should be reloaded. Otherwise we end up doing very
 CPU expensive work for no reason. The point of dh-apparmor is to reload a
 single profile, not all of them. Doing a trigger for all-profile reload
 isn't something we want. Think of the situation where someone has 5000
 apache virtual host profiles and they update cups. We never want to wait
 for those 5000 to be reloaded when cups's profile is installed. Hence,
 dh_apparmor.
 
Is there a way for a trigger to notice which file was updated?
That way we could use a trigger.

If not another option that comes to mind is we could add a new flag to the
parser that would say reload only if the cache is out of date.

The trigger would have to still do work figuring out which cache files
are out of date but thats still better than reloading everything to the
kernel.


-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#735470: [apparmor] Bug#735470: Fwd: Bug#735470: Could be implemented centrally with a dpkg trigger instead of requiring every package shipping an apparmor file to use dh_apparmor

[John Johansen]

On 01/16/2014 02:57 PM, John Johansen wrote:
 On 01/16/2014 02:49 PM, Kees Cook wrote:
 On Thu, Jan 16, 2014 at 07:37:04PM +0100, Didier 'OdyX' Raboud wrote:
 Le jeudi, 16 janvier 2014 10.14:14, vous avez écrit :
 On Thu, Jan 16, 2014 at 11:11:22AM +0100, Didier 'OdyX' Raboud wrote:
 As far as I understand deb-triggers' manpage, this can be enforced
 using 'activate /etc/apparmor.d/', which will then make the trigger
 run at the start of the configure operation, which ensures
 exactly what you want.

 Per-policy reloads must happen before a daemon restarts, so they
 cannot be triggers.

 Err…

 man deb-trigggers contradicts you, in my reading; an 'activate 
 /etc/apparmor.d' triggers' file in apparmor would make its action run 
 _before_ cups (which would have shipped /etc/apparmor.d/usr.sbin.cupsd) 
 would be 'configured' (hence its postinst run).

 Isn't it?

 Right, sorry, you are right, but my original observation stands: we should
 never reload all apparmor profiles when installing a single profile. Just
 the single profile should be reloaded. Otherwise we end up doing very
 CPU expensive work for no reason. The point of dh-apparmor is to reload a
 single profile, not all of them. Doing a trigger for all-profile reload
 isn't something we want. Think of the situation where someone has 5000
 apache virtual host profiles and they update cups. We never want to wait
 for those 5000 to be reloaded when cups's profile is installed. Hence,
 dh_apparmor.

 Is there a way for a trigger to notice which file was updated?
 That way we could use a trigger.
 
 If not another option that comes to mind is we could add a new flag to the
 parser that would say reload only if the cache is out of date.
 
 The trigger would have to still do work figuring out which cache files
 are out of date but thats still better than reloading everything to the
 kernel.
 
by trigger, I mean the parser/script called by the trigger.


-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#735470: [apparmor] Bug#735470: Fwd: Bug#735470: Could be implemented centrally with a dpkg trigger instead of requiring every package shipping an apparmor file to use dh_apparmor

[John Johansen]

On 01/16/2014 03:03 PM, Kees Cook wrote:
 On Thu, Jan 16, 2014 at 02:59:54PM -0800, John Johansen wrote:
 On 01/16/2014 02:57 PM, John Johansen wrote:
 On 01/16/2014 02:49 PM, Kees Cook wrote:
 On Thu, Jan 16, 2014 at 07:37:04PM +0100, Didier 'OdyX' Raboud wrote:
 Le jeudi, 16 janvier 2014 10.14:14, vous avez écrit :
 On Thu, Jan 16, 2014 at 11:11:22AM +0100, Didier 'OdyX' Raboud wrote:
 As far as I understand deb-triggers' manpage, this can be enforced
 using 'activate /etc/apparmor.d/', which will then make the trigger
 run at the start of the configure operation, which ensures
 exactly what you want.

 Per-policy reloads must happen before a daemon restarts, so they
 cannot be triggers.

 Err…

 man deb-trigggers contradicts you, in my reading; an 'activate 
 /etc/apparmor.d' triggers' file in apparmor would make its action run 
 _before_ cups (which would have shipped /etc/apparmor.d/usr.sbin.cupsd) 
 would be 'configured' (hence its postinst run).

 Isn't it?

 Right, sorry, you are right, but my original observation stands: we should
 never reload all apparmor profiles when installing a single profile. Just
 the single profile should be reloaded. Otherwise we end up doing very
 CPU expensive work for no reason. The point of dh-apparmor is to reload a
 single profile, not all of them. Doing a trigger for all-profile reload
 isn't something we want. Think of the situation where someone has 5000
 apache virtual host profiles and they update cups. We never want to wait
 for those 5000 to be reloaded when cups's profile is installed. Hence,
 dh_apparmor.

 Is there a way for a trigger to notice which file was updated?
 That way we could use a trigger.

 If not another option that comes to mind is we could add a new flag to the
 parser that would say reload only if the cache is out of date.

 The trigger would have to still do work figuring out which cache files
 are out of date but thats still better than reloading everything to the
 kernel.

 by trigger, I mean the parser/script called by the trigger.
 
 I can't remember -- does the parser do the right thing in the face of
 included files timestamps? If so, yeah, this could work.
 
it has for the last couple years now.


-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#735470: [apparmor] Bug#735470: Fwd: Bug#735470: Could be implemented centrally with a dpkg trigger instead of requiring every package shipping an apparmor file to use dh_apparmor

[John Johansen]

On 01/16/2014 03:03 PM, Kees Cook wrote:
 On Thu, Jan 16, 2014 at 02:59:54PM -0800, John Johansen wrote:
 On 01/16/2014 02:57 PM, John Johansen wrote:
 On 01/16/2014 02:49 PM, Kees Cook wrote:
 On Thu, Jan 16, 2014 at 07:37:04PM +0100, Didier 'OdyX' Raboud wrote:
 Le jeudi, 16 janvier 2014 10.14:14, vous avez écrit :
 On Thu, Jan 16, 2014 at 11:11:22AM +0100, Didier 'OdyX' Raboud wrote:
 As far as I understand deb-triggers' manpage, this can be enforced
 using 'activate /etc/apparmor.d/', which will then make the trigger
 run at the start of the configure operation, which ensures
 exactly what you want.

 Per-policy reloads must happen before a daemon restarts, so they
 cannot be triggers.

 Err…

 man deb-trigggers contradicts you, in my reading; an 'activate 
 /etc/apparmor.d' triggers' file in apparmor would make its action run 
 _before_ cups (which would have shipped /etc/apparmor.d/usr.sbin.cupsd) 
 would be 'configured' (hence its postinst run).

 Isn't it?

 Right, sorry, you are right, but my original observation stands: we should
 never reload all apparmor profiles when installing a single profile. Just
 the single profile should be reloaded. Otherwise we end up doing very
 CPU expensive work for no reason. The point of dh-apparmor is to reload a
 single profile, not all of them. Doing a trigger for all-profile reload
 isn't something we want. Think of the situation where someone has 5000
 apache virtual host profiles and they update cups. We never want to wait
 for those 5000 to be reloaded when cups's profile is installed. Hence,
 dh_apparmor.

 Is there a way for a trigger to notice which file was updated?
 That way we could use a trigger.

 If not another option that comes to mind is we could add a new flag to the
 parser that would say reload only if the cache is out of date.

 The trigger would have to still do work figuring out which cache files
 are out of date but thats still better than reloading everything to the
 kernel.

 by trigger, I mean the parser/script called by the trigger.
 
 I can't remember -- does the parser do the right thing in the face of
 included files timestamps? If so, yeah, this could work.
 
Here is a start on the patch,

It does not at the moment consider what is loaded into the kernel, but only
works off of the cache time stamps. For older kernels there really isn't
a way to check if what is loaded in the kernel is up to date. However
for 3.12+ kernels we could leverage the policy dir to compare timestamps
or if policy hashing is enabled we could compare to the sha1 stored for
each profile that is loaded.

we can of course also change the letter chosen

---

---
 parser/parser.h|1 +
 parser/parser_common.c |1 +
 parser/parser_main.c   |   15 ++-
 3 files changed, 16 insertions(+), 1 deletion(-)

--- 2.9-test.orig/parser/parser.h
+++ 2.9-test/parser/parser.h
@@ -229,6 +229,7 @@
 extern int conf_quiet;
 extern int names_only;
 extern int option;
+extern int update_only;
 extern int current_lineno;
 extern dfaflags_t dfaflags;
 extern const char *progname;
--- 2.9-test.orig/parser/parser_common.c
+++ 2.9-test/parser/parser_common.c
@@ -33,6 +33,7 @@
 int names_only = 0;
 int current_lineno = 1;
 int option = OPTION_ADD;
+int update_only = 0;
 
 dfaflags_t dfaflags = (dfaflags_t)(DFA_CONTROL_TREE_NORMAL | 
DFA_CONTROL_TREE_SIMPLE | DFA_CONTROL_MINIMIZE );
 
--- 2.9-test.orig/parser/parser_main.c
+++ 2.9-test/parser/parser_main.c
@@ -96,6 +96,7 @@
{help,2, 0, 'h'},
{replace, 0, 0, 'r'},
{reload,  0, 0, 'r'}, /* undocumented reload option 
== replace */
+   {update,  0, 0, 'u'},
{version, 0, 0, 'V'},
{complain,0, 0, 'C'},
{Complain,0, 0, 'C'}, /* Erk, apparently documented 
as --Complain */
@@ -144,6 +145,7 @@
   -a, --add   Add apparmor definitions [default]\n
   -r, --replace   Replace apparmor definitions\n
   -R, --removeRemove apparmor definitions\n
+  -u, --updateOnly load/replace if updated, ie. cache 
is out of date\n
   -C, --Complain  Force the profile into complain mode\n
   -B, --binaryInput is precompiled profile\n
   -N, --names Dump names of profiles in input.\n
@@ -294,6 +296,9 @@
option = OPTION_REMOVE;
skip_cache = 1;
break;
+   case 'u':
+   update_only++;
+   break;
case 'V':
display_version();
exit(0);
@@ -430,7 +435,7 @@
int count = 0;
option = OPTION_ADD;
 
-   while ((c = getopt_long(argc, argv, 
adf:h::rRVvI:b:BCD:NSm:qQn:XKTWkO:po:, long_options, o)) != -1)
+   while ((c = getopt_long(argc, argv, 
adf:h::rRuVvI:b:BCD:NSm:qQn:XKTWkO:po:, long_options, o)) != -1

Bug#735470: [apparmor] Bug#735470: Fwd: Bug#735470: Could be implemented centrally with a dpkg trigger instead of requiring every package shipping an apparmor file to use dh_apparmor

[John Johansen]

On 01/16/2014 04:23 PM, Seth Arnold wrote:
 On Thu, Jan 16, 2014 at 02:57:52PM -0800, John Johansen wrote:
 Is there a way for a trigger to notice which file was updated?
 That way we could use a trigger.

 If not another option that comes to mind is we could add a new flag to the
 parser that would say reload only if the cache is out of date.

 The trigger would have to still do work figuring out which cache files
 are out of date but thats still better than reloading everything to the
 kernel.
 
 One of my work-items for 14.04 LTS is to rework the AppArmor policy
 loading.
 
yes something that desperately needs to be done

 Currently, the initscript does some work to figure out which profile files
 to load, which ones to ignore, and calls the parser individually for each
 file. There are (in Ubuntu) profiles in both /etc/apparmor.d/ and in
 /var/lib/apparmor/profiles/.
 
yep, its a pita

 The initscript also attempts to manage the caches; 'service apparmor
 restart' or 'service apparmor stop' actually deletes the entire cache
 regardless if it makes sense to do so.
 
yep

 I'd like the parser to take on as many of the tasks from the initscripts
 as possible. Clearing the cache is a bad idea when stopping or restarting
 the AppArmor service, and in the usual case of service apparmor start,
 I'd like the parser to be in charge of determining which files to load
 and if the cached profiles can be read. (This could remove some 30-50
 or more fork+exec()s of the parser.)
 
yep

 Perhaps the parser should also be extended to determine if a loaded
 profile is older than the cached profile or the plaintext profile; I
this will be possible but requires a newer kernel. In older kernels you
either have no introspection (upstream), or limited introspection (patched).

With the 3.12 we finally have the policy directory which we can leverage.
We could use the timestamps (though there is a bug where the modified
time stamp isn't being updated on replace) or the sha1 profile hash.

 believe the parser currently loads the profiles it was asked to load
 regardless if the kernel already has the identical profile already loaded.
 
yes it does. Again determining whether the loaded profile was the same
was just not possible in the past

 If dh_apparmor doesn't currently use --write-cache we should make it do
 so, to allow the compilation to be saved for later. Same with the click
 packaging hooks.
 
well it has to use write-cache to update the cache. The issue isn't so
much the current dh_apparmor hook. But that you have to use the dh_apparmor
hook instead of a trigger to reload policy.

the -u patch I just posted is a 95% solution for that. It is missing the
ability to know what has been loaded to the kernel. It can't detect if
the profile has been loaded so shouldn't be used on in an init script
but it will work from a trigger. If the profile is new (cache file is
missing) or updated, it will reload the profile.

 Upstart currently has some AppArmor policy knowledge built-in. We should
 also make sure it Does The Right Thing, ideally that'd be mostly up to
 the parser to get correct.
 
Well some of this will depend on which parser version you want to support.
Eg. the dev branch supports having the parser manage the whole directory
   apparmor_parser -r -W /etc/apparmor.d/

where in the past you had to give it a list of profiles

 I'm sure there's more I've over-looked, I've not looked at this for a
 while, so please feel free to speak up if I've overlooked important
 considerations.
 
 Thanks
 


-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#735470: [apparmor] Bug#735470: Fwd: Bug#735470: Could be implemented centrally with a dpkg trigger instead of requiring every package shipping an apparmor file to use dh_apparmor

[John Johansen]

On 01/16/2014 08:02 PM, Seth Arnold wrote:
 On Thu, Jan 16, 2014 at 05:03:43PM -0800, John Johansen wrote:
 Well some of this will depend on which parser version you want to support.
 
 Argh. Leave it to me to forget that kernel, userspace, and surrounding
 frameworks do not update in lockstep. Just how many dimensions does this
 matrix have, anyway?
 
 - Kernel
   - No introspection
   - Poor introspection
   - Good introspection
   - Features file
   - Features directory
profile sha1 that can be enabled based on whether you want to linkin/compiler 
crypto

 
 - Parser
   - No caching
- No cache invalidation. The first generation of caching was done entirely in 
the
initscripts and was based on the parser using -S to dump the profile.

   - Cache invalidation based on profile timestamp?
   - Cache invalidation based on profile timestamp and features?
   - Cache invalidation based on profile and include timestamps and
 features?
   - Explicitly named profiles
you can delete this. Explicitly named profiles existed before caching. They 
where
added in 2.3, -S was added in 2.1 and the init script handling of caching later.

   - Directory-at-a-time
 
 - init
   - No AppArmor knowledge
how about initscripts with all too much apparmor knowledge? having to manage the
cache etc.

   - Upstart with AppArmor knowledge with --write-cache (I hope this is the
 only version..)
Hrmmm I am not sure what upstart is doing wrt the --write-cache flag

 
 - Packages
   - dh_apparmor with --write-cache (I hope this is the only version..)
 
as far as I know

 - Click packages
   - click.py with --write-cache (I hope this is the only version..)
 
hrmm just a couple things.

Realistically we don't need to worry about history too much. We modify init 
scripts
going forward. And we can selectively backport if needed.

The case we really have to watch is people compiling their own kernels, so
we would like to work at least some in those cases. And we need to support 
kernels
that are a couple years old as best we can.

I think -u flag achieves most of this, and it can be made better by extending 
the
parser to introspect the kernel before policy loads on kernels that support it.


-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#682431: [apparmor] Factorize Qt and GTK themes abstractions?

[John Johansen]

On 08/22/2012 12:48 PM, intrigeri wrote:
 Hi,
 
 it was reported as Debian bug #682431 [0] that the AppArmor profile
 we've been shipping for Vidalia is incomplete: given a system with Qt
 configured to use the GTK theme, which is the Debian Wheezy default,
 Vidalia (a Qt application) needs to read files such as
 /usr/share/themes/*/gtk-2.0/gtkrc and various ~/.gtk* and friends that
 are listed in the gnome abstraction.
 
 On the short term, I could copy these lines from the gnome abstraction
 into the Vidalia profile, but for obvious reasons I'd hate doing this
 without having better mid-term plans.
 
 So, I think the lines about themes and gtkrc should be extracted from
 the gnome abstraction into another abstraction (gtk?), that can be
 sourced by:
 
   * the existing gnome abstraction
   * a new qt abstraction (to be used by the kde abstraction, as well
 as by non-KDE Qt applications)
 
 Thoughts?
 
While I haven't looked at it, in any great detail generally I agree
that refactorying of the abstractions is needed and this seems like a
reasonable change.
 
 If the above makes sense, and given there are tools to do the contrary
 (GTK applications using the configured Qt theme), I guess Qt theme
 configuration -related lines shall be somehow extracted from the kde
 abstraction as well.
 
that would make sense to me as well

  [0] http://bugs.debian.org/682431
 
thanks


-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#679597: apparmor: AppArmor totally broken

[John Johansen]

On 07/01/2012 03:02 PM, intrigeri wrote:
 tags 679597 + patch
 thanks
 
 Hi,
 
 John Johansen wrote (30 Jun 2012 07:30:20 GMT) :
 Fix the parser so it checks for the presence of the network feature in the
 compatibility interface. Previously it was assuming that if the compatibility
 interface was present that network rules where also present, this is not
 necessarily true and causes apparmor to break when only the compatibility
 patch is applied.
 
 Thanks for this patch.
 
 It works fine for me with the current sid kernel
 (linux-image-3.2.0-3-amd64 3.2.21-3).
 
 However, on a kernel that both the compat + network patches applied
 (that is, not the current sid kernel), installing the apparmor
 userspace tools with this patch applied results in reloading all
 profiles (I guess this is normal postinst operation), which triggers
 tons of such error messages:
 
   Warning from /etc/apparmor.d/usr.bin.evince
   (/etc/apparmor.d/usr.bin.evince line 148): profile sanitized_helper
   network rules not enforced
 
 And then, it seems like the applications covered by these profile are
 denied access to the network entirely:
 
   type=1400 audit(1341176452.889:291): apparmor=DENIED
   operation=create parent=1 profile=/usr/sbin/ntpd pid=6748
   comm=ntpd family=inet sock_type=dgram protocol=0
 
 (I've not tried rebooting and see what happens, though.)
 
 So I'm not too sure the network feature detection was fixed entirely.
 
 But well, in any case, the patch fixes the actual, current bug,
 which is great!
 

Gah, yes I didn't test this patch in the case of a kernel without the
networking patch followed by a kernel with it.

What is happening is it is applying the check against both the kernel
and cached policy feature set, and turning off networking based on
what is stored in the cached policy. Which in turn causes it to generate
the new cache without networking support. The only way to fix this with
the original patch is to remove the cache and then regenerate it.
Sorry about that

The check just needs to be moved a little. The initial patch should be
reversed and the following patch should be applied. With the caveat that
I haven't had a chance to finish testing it yet.  Though I should have
that done in a few hours.


=== modified file 'parser/parser_main.c'
--- parser/parser_main.c2012-07-01 08:35:05 +
+++ parser/parser_main.c2012-07-02 07:49:14 +
@@ -1187,7 +1182,12 @@
write_cache = 0;
skip_read_cache = 1;
return;
-   }
+   } else if (strstr(flags_string, network))
+   kernel_supports_network = 1;
+   else
+   kernel_supports_network = 0;
+
+
 
/*
  * Deal with cache directory versioning:






-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#679597: apparmor: AppArmor totally broken

[John Johansen]

On 06/29/2012 07:54 PM, intrig...@debian.org wrote:
 Package: apparmor
 Version: 2.7.103-3
 Severity: grave
 X-Debbugs-CC: john.johan...@canonical.com, k...@debian.org, mi...@riseup.net
 
 Hi,
 
 (following-up on #676515)
 
 John Johansen wrote (26 Jun 2012 17:48:38 GMT) :
 Okay, there are 4 kernel patches, not all of them are needed depending on 
 whether
 the network patch is applied or not.
 
 If you don't want to apply the networking patch
   0001-apparmor-remove-advertising-the-support-of-network-r.patch
 
   Stops the kernel interface from incorrectly advertising that it
   supports network rules. A further patch (not attached) to
   userspace will also have to be applied
 
 Thanks, John, for your work on this.
 
 For those who did not follow the entire saga, this patch was applied
 in the linux 3.2.21-3 source package, to complement the incomplete
 AppArmor compatibility patch, so Debian bug #676515 was closed,
 as the kernel side is now sorted out. So far, so good.
 
 However, as expected, this is not enough to make AppArmor usable, so
 the current state in current sid is still a regression compared to
 when the compatibility patch was not applied to the kernel: it used to
 be bad, but relatively usable, and it's now totally unusable.
 
 This bug is here to track the additional patch against userspace,
 that John mentioned was needed, which is confirmed by my experience.
 
 

Sorry I meant to have attached this patch already as a separate comment
when I posted the kernel patches.

---

Fix the parser so it checks for the presence of the network feature in the
compatibility interface. Previously it was assuming that if the compatibility
interface was present that network rules where also present, this is not
necessarily true and causes apparmor to break when only the compatibility
patch is applied.

Signed-off-by: John Johansen john.johan...@canonical.com

=== modified file 'parser/parser_main.c'
--- parser/parser_main.c2012-04-11 23:03:21 +
+++ parser/parser_main.c2012-06-30 06:31:05 +
@@ -873,6 +873,11 @@
 //fprintf(stderr, flags string: %s\n, flags_string);
 //fprintf(stderr, changehat %d\n, flag_changehat_version);
}
+   if (strstr(flags_string, network))
+   kernel_supports_network = 1;
+   else
+   kernel_supports_network = 0;
+
return;
 
 fail:



-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#676515: linux-2.6: AppArmor totally broken

[John Johansen]

On 06/23/2012 11:53 AM, intrigeri wrote:
 Hi John,
 
 John Johansen wrote (17 Jun 2012 19:08:20 GMT) :
 On 06/15/2012 05:08 PM, Ben Hutchings wrote:

 If we don't want to restrict sockets used by the kernel, don't we need
 to store the kern flag for later use by aa_revalidate_sk()?

 For how apparmor is generally deployed it can get away with this, the
 kernel bits generally bail out earlier on the check for unconfined.

 That is not to say it isn't a good idea, or that it shouldn't be done.
 The fact is this patch is going to be replaced with completely rewritten
 controls, that do store info on the socket, it just hasn't happened yet
 due to resources and priorities (not my priorities).

 Ben, is this a blocker?

 I want to be convinced that this is not a bug, or else get a fix for it.

 I am looking at the kernel bits here, but I don't have a patch yet
 
 Do you think you'll manage to do it in time for the Wheezy freeze
 (June 30th)?
 
 Since denied has already been masked with ~quiet_mask, this condition
 can never be true.

 indeed

 Ben, is this a blocker?
 [...]

 This clearly is a bug and I want to be convinced that it is harmless or
 else get a fix for it.

 Right this breaks the controls over quieting of denial messages. Basically
 if policy specifies a reject should not be logged then the global controls
 that turn quieting off so that all rejects get logged aren't working for
 networking.
 
 This is an easy patch that I can provide separately or with the
 patch I am working on for the larger issue.
 
 Do you think you'll manage to prepare at least the easy fix it in time
 for the Wheezy freeze?
 

Okay, there are 4 kernel patches, not all of them are needed depending on 
whether
the network patch is applied or not.

If you don't want to apply the networking patch
  0001-apparmor-remove-advertising-the-support-of-network-r.patch

  Stops the kernel interface from incorrectly advertising that it supports 
network
  rules. A further patch (not attached) to userspace will also have to be 
applied

If the networking patch is applied
  these two patches can be applied or ignored, 0001 will be folded into the 
compat
  interface patch upstream, and then 0002 will be folded into the networking 
patch
  0001-apparmor-remove-advertising-the-support-of-network-r.patch
  0002-apparmor-Advertise-network-mediation-from-the-compat.patch

  these two patches address the two bugs pointed out in the networking patch
  0003-apparmor-Fix-quieting-of-audit-messages-for-network-.patch
  0004-apparmor-Ensure-apparmor-does-not-mediate-kernel-bas.patch
From 873143ceca69a2e54e7face1be49ad6b5514525d Mon Sep 17 00:00:00 2001
From: John Johansen john.johan...@canonical.com
Date: Tue, 26 Jun 2012 02:12:10 -0700
Subject: [PATCH 1/4] apparmor: remove advertising the support of network
 rules from compat iface

The interface compatibility patch was advertising support of network rules,
however this is not true if the networking patch is not applied. Move
advertising of network rules into a third patch that can be applied if
both the compatibility and network patches are applied.

Signed-off-by: John Johansen john.johan...@canonical.com
---
 security/apparmor/apparmorfs-24.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/apparmor/apparmorfs-24.c b/security/apparmor/apparmorfs-24.c
index dc8c744..367c7ea 100644
--- a/security/apparmor/apparmorfs-24.c
+++ b/security/apparmor/apparmorfs-24.c
@@ -49,7 +49,7 @@ const struct file_operations aa_fs_matching_fops = {
 static ssize_t aa_features_read(struct file *file, char __user *buf,
 size_t size, loff_t *ppos)
 {
-	const char features[] = file=3.1 capability=2.0 network=1.0 
+	const char features[] = file=3.1 capability=2.0 
 	change_hat=1.5 change_profile=1.1  aanamespaces=1.1 rlimit=1.1;
 
 	return simple_read_from_buffer(buf, size, ppos, features,
-- 
1.7.9.5

From 89a577e1bd55ee589c66bb82babb1dbbb8d73dad Mon Sep 17 00:00:00 2001
From: John Johansen john.johan...@canonical.com
Date: Tue, 26 Jun 2012 02:15:36 -0700
Subject: [PATCH 2/4] apparmor: Advertise network mediation from the
 compatibility interface

The userspace needs to know if the apparmor kernel module supports network
mediation. Apply this patch if both the v2.3 compatibility patch and
network mediation patches are applied.

Signed-off-by: John Johansen john.johan...@canonical.com
---
 security/apparmor/apparmorfs-24.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/apparmor/apparmorfs-24.c b/security/apparmor/apparmorfs-24.c
index 367c7ea..dc8c744 100644
--- a/security/apparmor/apparmorfs-24.c
+++ b/security/apparmor/apparmorfs-24.c
@@ -49,7 +49,7 @@ const struct file_operations aa_fs_matching_fops = {
 static ssize_t aa_features_read(struct file *file, char __user *buf,
 size_t size, loff_t *ppos)
 {
-	const char features[] = file=3.1 capability=2.0 
+	const char features[] = file=3.1 capability=2.0 network=1.0 
 	change_hat=1.5

Bug#676515: linux-2.6: AppArmor totally broken

[John Johansen]

On 06/23/2012 11:53 AM, intrigeri wrote:
 Hi John,
 
 John Johansen wrote (17 Jun 2012 19:08:20 GMT) :
 On 06/15/2012 05:08 PM, Ben Hutchings wrote:

 If we don't want to restrict sockets used by the kernel, don't we need
 to store the kern flag for later use by aa_revalidate_sk()?

 For how apparmor is generally deployed it can get away with this, the
 kernel bits generally bail out earlier on the check for unconfined.

 That is not to say it isn't a good idea, or that it shouldn't be done.
 The fact is this patch is going to be replaced with completely rewritten
 controls, that do store info on the socket, it just hasn't happened yet
 due to resources and priorities (not my priorities).

 Ben, is this a blocker?

 I want to be convinced that this is not a bug, or else get a fix for it.

 I am looking at the kernel bits here, but I don't have a patch yet
 
 Do you think you'll manage to do it in time for the Wheezy freeze
 (June 30th)?
 
Yes for the don't mediate kernel sockets tagging, and the quiet masking.

The check if in interrupt context I am not comfortable removing yet and it
will have to wait for the full new networking patches.

 Since denied has already been masked with ~quiet_mask, this condition
 can never be true.

 indeed

 Ben, is this a blocker?
 [...]

 This clearly is a bug and I want to be convinced that it is harmless or
 else get a fix for it.

 Right this breaks the controls over quieting of denial messages. Basically
 if policy specifies a reject should not be logged then the global controls
 that turn quieting off so that all rejects get logged aren't working for
 networking.
 
 This is an easy patch that I can provide separately or with the
 patch I am working on for the larger issue.
 
 Do you think you'll manage to prepare at least the easy fix it in time
 for the Wheezy freeze?
 
Yes I should have something for you by the end of the weekend if not sooner



-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#676515: linux-2.6: AppArmor totally broken

[John Johansen]

On 06/23/2012 12:30 PM, intrigeri wrote:
 Hi,
 
 Ben Hutchings wrote (23 Jun 2012 19:02:06 GMT) :
 What is it that you think will happen at the freeze? We stop fixing
 all bugs and do nothing for the next few months?
 
 Of course, and we'll lazily eat lots of icecream while you work hard
 to release many shiny new Linux 3.2.x :)
 
 Irony set aside, I believe there are two different ways of fixing this
 bug:
 
   - Either by fixing the actual regression directly, without applying
 the network control patch: it seems clear to me that this can
 happen after the freeze. So far, so good.
 
   - Or by applying the AppArmor network control patch, in an improved
 version that John is apparently working on; this would be my
 preferred solution. But, given it would be a feature addition,
 rather than being merely a minimal bugfix, I'm not too sure it
 would be fit for a freeze exception. Hence my gentle pressuring on
 John to get this done before the freeze.
 
 Feel free to correct me if my understanding of the situation is wrong.
 
To be clear the network stuff I am working on for wheezy is 2 patches to
the current networking patch. The larger networking rewrite is not ready
yet and even if I could get it done in time I would not feel comfortable
pushing that in to wheezy as it would be at best early beta quality





-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#676515: linux-2.6: AppArmor totally broken

[John Johansen]

On 06/15/2012 05:08 PM, Ben Hutchings wrote:
 On Fri, 2012-06-15 at 22:38 +0200, intrigeri wrote:
 Hi John, Ben and all other involved ones,

 I'd like to see this moving forward, since the Wheezy freeze is coming
 soon. See bellow explicit questions.
 
 Me too; thanks for the mail.
 
 John Johansen wrote (07 Jun 2012 16:45:36 GMT) :
 On 06/07/2012 07:34 AM, Ben Hutchings wrote:

 If we don't want to restrict sockets used by the kernel, don't we need
 to store the kern flag for later use by aa_revalidate_sk()?

 For how apparmor is generally deployed it can get away with this, the
 kernel bits generally bail out earlier on the check for unconfined.

 That is not to say it isn't a good idea, or that it shouldn't be done.
 The fact is this patch is going to be replaced with completely rewritten
 controls, that do store info on the socket, it just hasn't happened yet
 due to resources and priorities (not my priorities).

 Ben, is this a blocker?
 
 I want to be convinced that this is not a bug, or else get a fix for it.
 
I am looking at the kernel bits here, but I don't have a patch yet

 Since denied has already been masked with ~quiet_mask, this condition
 can never be true.

 indeed

 Ben, is this a blocker?
 [...]
 
 This clearly is a bug and I want to be convinced that it is harmless or
 else get a fix for it.
 
Right this breaks the controls over quieting of denial messages. Basically
if policy specifies a reject should not be logged then the global controls
that turn quieting off so that all rejects get logged aren't working for
networking.

This is an easy patch that I can provide separately or with the patch I
am working on for the larger issue.

I have also been looking into why the regression is happening, it actually
looks to be in the userspace caching of compiled policy. I can run the
same basic profile loads on ubuntu with a kernel that only has the single
interface patch applied and it works. So its just a matter of tracking
down which patches are needed now.




-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#676515: linux-2.6: AppArmor totally broken

[John Johansen]

On 06/07/2012 07:34 AM, Ben Hutchings wrote:
 On Thu, 2012-06-07 at 15:35 +0200, intrig...@debian.org wrote:
 Package: linux-2.6
 Severity: normal
 Version: 3.2.19-1
 Tags: patch
 X-Debbugs-CC: john.johan...@canonical.com, k...@debian.org, mi...@riseup.net

 Hi,

 the AppArmor compatibility patch applied to fix #661151
 totally breaks AppArmor support; this is a regression.
 Details: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=661151#83

 Applying the AppArmor: compatibility patch for v5 network controll
 on top of the already applied one fixes the problem for me.
 I did test with the patch that can be found there:
 http://kernel.ubuntu.com/git?p=ubuntu/ubuntu-precise.git;a=commit;h=d253e5fb4a6b552e7cd2a3c80934ab4f92faec97

 Please consider applying this network compatibility patch,
 not only to fix this regression,
 but also to get us working network mediation.
 
 So these independent patches really aren't... or else userland gets
 confused if we apply just the one.
 
Hrmmm, no they should be. That is a bug in userland that needs to be
fixed. I do know that they used to work when applied separately. It
certainly not a configuration that gets a lot of testing, but it
should have worked. I will look into it and figure out what is causing
it to break.

 Looking at the network controller patch:
 
 --- a/security/apparmor/lsm.c
 +++ b/security/apparmor/lsm.c
 [...]
 @@ -621,6 +622,104 @@ static int apparmor_task_setrlimit(struct task_struct 
 *task,
  return error;
  }
  
 +static int apparmor_socket_create(int family, int type, int protocol, int 
 kern)
 +{
 +struct aa_profile *profile;
 +int error = 0;
 +
 +if (kern)
 +return 0;
 
 If we don't want to restrict sockets used by the kernel, don't we need
 to store the kern flag for later use by aa_revalidate_sk()?
 
For how apparmor is generally deployed it can get away with this, the
kernel bits generally bail out earlier on the check for unconfined.

That is not to say it isn't a good idea, or that it shouldn't be done.
The fact is this patch is going to be replaced with completely rewritten
controls, that do store info on the socket, it just hasn't happened yet
due to resources and priorities (not my priorities).

 +profile = __aa_current_profile();
 +if (!unconfined(profile))
 +error = aa_net_perm(OP_CREATE, profile, family, type, protocol,
 +NULL);
 +return error;
 +}
 [...]
 --- /dev/null
 +++ b/security/apparmor/net.c
 [...]
 +static int audit_net(struct aa_profile *profile, int op, u16 family, int 
 type,
 + int protocol, struct sock *sk, int error)
 +{
 [...]
 +} else {
 +u16 quiet_mask = profile-net.quiet[sa.u.net.family];
 +u16 kill_mask = 0;
 +u16 denied = (1  sa.aad.net.type)  ~quiet_mask;
 +
 +if (denied  kill_mask)
 +audit_type = AUDIT_APPARMOR_KILL;
 +
 +if ((denied  quiet_mask) 
 
 Since denied has already been masked with ~quiet_mask, this condition
 can never be true.
 
indeed

 +AUDIT_MODE(profile) != AUDIT_NOQUIET 
 +AUDIT_MODE(profile) != AUDIT_ALL)
 +return COMPLAIN_MODE(profile) ? 0 : sa.aad.error;
 +}
 +
 +return aa_audit(audit_type, profile, GFP_KERNEL, sa, audit_cb);
 +}
 [...]
 +/**
 + * aa_revalidate_sk - Revalidate access to a sock
 + * @op: operation being checked
 + * @sk: sock being revalidated  (NOT NULL)
 + *
 + * Returns: %0 else error if permission denied
 + */
 +int aa_revalidate_sk(int op, struct sock *sk)
 +{
 +struct aa_profile *profile;
 +int error = 0;
 +
 +/* aa_revalidate_sk should not be called from interrupt context
 + * don't mediate these calls as they are not task related
 + */
 +if (in_interrupt())
 +return 0;
 
 I wonder why this is being checked at all.
 
Good question, I will have to dig into it.

 +profile = __aa_current_profile();
 +if (!unconfined(profile))
 +error = aa_net_perm(op, profile, sk-sk_family, sk-sk_type,
 +sk-sk_protocol, sk);
 +
 +return error;
 +}
 [...]
 
 Ben.
 




-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#661151: [apparmor] Bug#661151: linux-2.6: lacks AppArmor kernel/userspace interface

[John Johansen]

On 05/30/2012 08:08 AM, micah anderson wrote:
 
 Hi all,
 
 Its been 2 months without a reply on this issue, and we are getting
 close to a freeze. Kees and John it looks like there are some pending
 questions for you below, it would be great if you could chime in with
 your opinons:
 
 If the Debian kernel team was willing to carry some kind of AppArmor
 kernel/userspace interface patch, I'm now unsure if the old or new
 ones would be better suited. (I assume AppArmor 2.8 is released long
 enough before the Wheezy freeze, so that we can ship it in there, and
 are given this choice.)

 On the one hand, the old compat' patches are confidence inspiring, as
 they are small and have been shipped by Ubuntu for a while.
 
 My opinon: the 2.4 compat patch is tiny, and it works well, and has been
 tested for some time, I think it makes the most sense to include this
 one.
 
probably, especially if you are looking to keep the patch as small as
possible

 On the other hand, it seems the new patches are being upstreamed,
 which makes them more appealing somehow than the older ones.
 
 The newer patch is bigger, some of it must be backported from Linux 3.4,
 some from Ubuntu, it is much less tested and I suspect because of that
 will encounter much more resistance from Debian's kernel team to include
 it. Presumably this will eventually be the one that will be upstreamed,
 but it isn't there yet. This is why I think the 2.4 compat patch is the
 way to go with Wheezy, when the newer patch is upstreamed that can be
 swapped out then.
 
yeah to clarify, half of the new interface went upstream in 3.4 and I can
provide a version of that that is backported but its a few patches and
not as small as the compat patch.  In addition to that you would need
a compatibility patch on top of that, that provides the features the
current upstream interface doesn't

 John, I think it would help if you could please point us more
 precisely to the commits of the new interface that have been
 upstreamed already, and to the ones that have not been, so that we can
 get a rough idea of where things are at.

hrmmm, I think I missed answering this before

the upstream patches
9acd494be9387b0608612cd139967201dd7a4e12
e74abcf3359d0130e99a6511ac484a3ea9e6e988
a9bf8e9fd561ba9ff1f0f2a1d96e439fced4
d384b0a1a35f87f0ad70c29518f98f922b1c15cb

the additional patch to complete the interface
git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor v3.4-aa2.8
8de755e4dfdbc40bfcaca848ae6b5aeaf0ede0e8

vs. the old compat patch
git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor
da1ce2265ebb70860b9c137a542e48b170e4606b

 Kees, others, what do you think?
 

While I like to see the latest stuff, I think the old patch is a smaller
delta, well tested and going to be less to maintain so it really seems
the way to go.




-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#661151: [apparmor] Bug#661151: linux-2.6: lacks AppArmor kernel/userspace interface

[John Johansen]

On 05/30/2012 06:10 PM, Ben Hutchings wrote:
 On Wed, 2012-05-30 at 16:46 -0700, John Johansen wrote:
 On 05/30/2012 08:08 AM, micah anderson wrote:

 Hi all,

 Its been 2 months without a reply on this issue, and we are getting
 close to a freeze. Kees and John it looks like there are some pending
 questions for you below, it would be great if you could chime in with
 your opinons:

 If the Debian kernel team was willing to carry some kind of AppArmor
 kernel/userspace interface patch, I'm now unsure if the old or new
 ones would be better suited. (I assume AppArmor 2.8 is released long
 enough before the Wheezy freeze, so that we can ship it in there, and
 are given this choice.)

 On the one hand, the old compat' patches are confidence inspiring, as
 they are small and have been shipped by Ubuntu for a while.

 My opinon: the 2.4 compat patch is tiny, and it works well, and has been
 tested for some time, I think it makes the most sense to include this
 one.

 probably, especially if you are looking to keep the patch as small as
 possible
 
 Should I take it that '2.4 compat' actually means '2.4-2.7 compat'?

yeah it turned out to be that. The rewrite to use the newer LSM path hooks
began after 2.4, and the patch provided the 2.4 interface until a newer
interface that was more sysfs in style could be created.
 
 [...]
 vs. the old compat patch
 git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor
 da1ce2265ebb70860b9c137a542e48b170e4606b

 Kees, others, what do you think?


 While I like to see the latest stuff, I think the old patch is a smaller
 delta, well tested and going to be less to maintain so it really seems
 the way to go.
 
 So you're saying we should take just the one quoted above for wheezy?
 
 The aafs_create() and aafs_remove() calls are mismatched when
 CONFIG_SECURITY_APPARMOR_COMPAT_24 is not set, but aside from that it
 doesn't look too horrible.

oops I guess we never built it that way, I can fix that for you

 What about this one:
 
 commit 1023c7c2f9d9c5707147479104312c4c3d1a2c2b
 Author: John Johansen john.johan...@canonical.com
 Date:   Wed Aug 10 22:02:39 2011 -0700
 
 AppArmor: compatibility patch for v5 network controll
 
 Add compatibility for v5 network rules.
 

That will provide support for the network rules and if you are willing
to carry it that would be greate but is not strictly necessary. Policy can
still be loaded and introspected. If that patch is missing and if profile
contains network rules the parser will complain about them not being
enforced, but it will still load and enforce the rest of the policy




-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#661151: [apparmor] Bug#661151: linux-2.6: lacks AppArmor kernel/userspace interface

[John Johansen]

On 03/14/2012 03:24 AM, intrigeri wrote:
 Hi,
 
 John Johansen wrote (13 Mar 2012 16:33:53 GMT) :
 sorry I missed this,
 
 Thank you, John, for your answers :)
 
 yes you can pull them out of the tarball,
 
 That would be 0002-AppArmor-compatibility-patch-for-v5-interface.patch
 that can be found in the kernel-patches/$LATEST/ directory of the
 apparmor Debian source package. Given $LATEST == 3.1 currently, see
 bellow for the Ubuntu patches that were maybe refreshed.
 
 John, do you confirm this patch does not depend on any of the
 two others?
 
It does not but there may be a small conflict or two to resolve if
0001-AppArmor-compatibility-patch-for-v5-network-controll.patch is not
applied first.

If it doesn't apply cleanly I will be happy to update it for you.

 (namely:
 0001-AppArmor-compatibility-patch-for-v5-network-controll.patch and
 0003-AppArmor-Allow-dfa-backward-compatibility-with-broke.patch)
 
 or from the ubuntu kernel tree.
 
 I guess that would be
 http://kernel.ubuntu.com/git?p=ubuntu/ubuntu-precise.git;a=commit;h=56f928f0cbf810c047a9a72e4e5c4840800437ec
 
 John, please correct me if I did not guess right.
 
You are right

 There are also a new set of patches available against the 3.3
 kernel. The static parts of the interface have been updated and
 pushed into the 3.4 kernel. And the goal is to get the other part
 into the 3.5 kernel (still a wip).
 
 John: I guess the Linux 3.2 kernel shipped in Precise will carry those
 patches, and this is why the v5 compat' patches got recently reverted
 in Precise's kernel tree, right?
 
correct

 Though those will require a more recent userspace.
 
 John: that will be called 2.8, right?
 
correct. The 2.8 userspace release will ship with precise and will be
compatible with both the older and newer kernel interfaces.



-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#661151: [apparmor] Bug#661151: linux-2.6: lacks AppArmor kernel/userspace interface

[John Johansen]

On 02/24/2012 08:40 AM, intrigeri wrote:
 Ben Hutchings wrote (24 Feb 2012 16:06:16 GMT) :
 Where can I find the patch?
 
 Kees and Ubuntu AppArmor developers: can you please confirm the
 patches that the Debian kernel team should consider for supporting the
 AppArmor legacy interface would be the ones found in the
 kernel-patches/$LATEST/ directory of the apparmor 2.7.x tarball?
 
 Or have you got updated patches, e.g. for Linux 3.2.x, published
 somewhere to be found?
 
sorry I missed this,

yes you can pull them out of the tarball, or from the ubuntu kernel
tree.

You will also be able to pull them from the git tree at
git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor.git

though I still need to restore them as the tree got wiped after the
kernel.org breakin.

There are also a new set of patches available against the 3.3 kernel.
The static parts of the interface have been updated and pushed into
the 3.4 kernel.  And the goal is to get the other part into the 3.5
kernel (still a wip).  Though those will require a more recent userspace.

If someone will let me know what is desired I will set up a tree with
patches pre-applied.



-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org