Re: [PATCH v3 00/11] sysctl: treewide: constify ctl_table argument of sysctl handlers
On Sun, May 12, 2024 at 09:32:40PM +0200, Joel Granados wrote: > On Sat, May 11, 2024 at 11:51:18AM +0200, Thomas Weißschuh wrote: > > Hi Kees, > > > > On 2024-05-08 10:11:35+, Kees Cook wrote: > > > On Wed, Apr 24, 2024 at 08:12:34PM -0700, Jakub Kicinski wrote: > > > > On Tue, 23 Apr 2024 09:54:35 +0200 Thomas Weißschuh wrote: > > > > > The series was split from my larger series sysctl-const series [0]. > > > > > It only focusses on the proc_handlers but is an important step to be > > > > > able to move all static definitions of ctl_table into .rodata. > > > > > > > > Split this per subsystem, please. > > > > > > I've done a few painful API transitions before, and I don't think the > > > complexity of these changes needs a per-subsystem constification pass. I > > > think this series is the right approach, but that patch 11 will need > > > coordination with Linus. We regularly do system-wide prototype changes > > > like this right at the end of the merge window before -rc1 comes out. > > > > That sounds good. > > > > > The requirements are pretty simple: it needs to be a obvious changes > > > (this certainly is) and as close to 100% mechanical as possible. I think > > > patch 11 easily qualifies. Linus should be able to run the same Coccinelle > > > script and get nearly the same results, etc. And all the other changes > > > need to have landed. This change also has no "silent failure" conditions: > > > anything mismatched will immediately stand out. > > > > Unfortunately coccinelle alone is not sufficient, as some helpers with > > different prototypes are called by handlers and themselves are calling > > handler and therefore need to change in the same commit. > > But if I add a diff for those on top of the coccinelle script to the > > changelog it should be obvious. > Judging by Kees' comment on "100% mechanical", it might be better just > having the diff and have Linus apply than rather than two step process? > Have not these types of PRs, so am interested in what folks think. I tried to soften it a little with my "*close* to 100%" modifier, and I think that patch basically matched that requirement, and where it had manual changes it was detailed in the commit log. I only split out the seccomp part because it could actually stand alone. So yeah, let's get the last of the subsystem specific stuff landed after -rc1, and it should be possible to finish it all up for 6.11. Yay! :) -Kees -- Kees Cook
Re: [PATCH v3 00/11] sysctl: treewide: constify ctl_table argument of sysctl handlers
On Wed, Apr 24, 2024 at 08:12:34PM -0700, Jakub Kicinski wrote: > On Tue, 23 Apr 2024 09:54:35 +0200 Thomas Weißschuh wrote: > > The series was split from my larger series sysctl-const series [0]. > > It only focusses on the proc_handlers but is an important step to be > > able to move all static definitions of ctl_table into .rodata. > > Split this per subsystem, please. I've done a few painful API transitions before, and I don't think the complexity of these changes needs a per-subsystem constification pass. I think this series is the right approach, but that patch 11 will need coordination with Linus. We regularly do system-wide prototype changes like this right at the end of the merge window before -rc1 comes out. The requirements are pretty simple: it needs to be a obvious changes (this certainly is) and as close to 100% mechanical as possible. I think patch 11 easily qualifies. Linus should be able to run the same Coccinelle script and get nearly the same results, etc. And all the other changes need to have landed. This change also has no "silent failure" conditions: anything mismatched will immediately stand out. So, have patches 1-10 go via their respective subsystems, and once all of those are in Linus's tree, send patch 11 as a stand-alone PR. (From patch 11, it looks like the seccomp read/write function changes could be split out? I'll do that now...) -Kees -- Kees Cook
Re: [apparmor] [PATCH 2/7] security: Remove the now superfluous sentinel element from ctl_table array
On Thu, Mar 28, 2024 at 04:57:49PM +0100, Joel Granados via B4 Relay wrote: > From: Joel Granados > > This commit comes at the tail end of a greater effort to remove the > empty elements at the end of the ctl_table arrays (sentinels) which will > reduce the overall build time size of the kernel and run time memory > bloat by ~64 bytes per sentinel (further information Link : > https://lore.kernel.org/all/zo5yx5jfoggi%2f...@bombadil.infradead.org/) > > Remove the sentinel from all files under security/ that register a > sysctl table. > > Signed-off-by: Joel Granados Acked-by: Kees Cook # loadpin & yama -- Kees Cook
Re: [apparmor] [PATCH 2/2] apparmor: fix typo in kernel doc
On Fri, Mar 15, 2024 at 01:54:09PM +0100, Christian Göttsche wrote: > Fix the typo in the function documentation to please kernel doc > warnings. > > Signed-off-by: Christian Göttsche Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH] exec: Check __FMODE_EXEC instead of in_execve for LSMs
On Wed, Jan 24, 2024 at 10:40:49PM +0100, Jann Horn wrote: > On Wed, Jan 24, 2024 at 10:32 PM Kees Cook wrote: > > > > On Wed, Jan 24, 2024 at 12:47:34PM -0800, Linus Torvalds wrote: > > > On Wed, 24 Jan 2024 at 12:15, Kees Cook wrote: > > > > > > > > Hmpf, and frustratingly Ubuntu (and Debian) still builds with > > > > CONFIG_USELIB, even though it was reported[2] to them almost 4 years > > > > ago. > > > > For completeness, Fedora hasn't had CONFIG_USELIB for a while now. > > > > > Well, we could just remove the __FMODE_EXEC from uselib. > > > > > > It's kind of wrong anyway. > > > > Yeah. > > > > > So I think just removing __FMODE_EXEC would just do the > > > RightThing(tm), and changes nothing for any sane situation. > > > > Agreed about these: > > > > - fs/fcntl.c is just doing a bitfield sanity check. > > > > - nfs_open_permission_mask(), as you say, is only checking for > > unreadable case. > > > > - fsnotify would also see uselib() as a read, but afaict, > > that's what it would see for an mmap(), so this should > > be functionally safe. > > > > This one, though, I need some more time to examine: > > > > - AppArmor, TOMOYO, and LandLock will see uselib() as an > > open-for-read, so that might still be a problem? As you > > say, it's more of a mmap() call, but that would mean > > adding something a call like security_mmap_file() into > > uselib()... > > > > The issue isn't an insane "support uselib() under AppArmor" case, but > > rather "Can uselib() be used to bypass exec/mmap checks?" > > > > This totally untested patch might give appropriate coverage: > > > > diff --git a/fs/exec.c b/fs/exec.c > > index d179abb78a1c..0c9265312c8d 100644 > > --- a/fs/exec.c > > +++ b/fs/exec.c > > @@ -143,6 +143,10 @@ SYSCALL_DEFINE1(uselib, const char __user *, library) > > if (IS_ERR(file)) > > goto out; > > > > + error = security_mmap_file(file, PROT_READ | PROT_EXEC, MAP_FIXED | > > MAP_SHARED); > > + if (error) > > + goto exit; > > Call path from here is: > > sys_uselib -> load_elf_library -> elf_load -> elf_map -> vm_mmap -> > vm_mmap_pgoff > > Call path for normal mmap is: > > sys_mmap_pgoff -> ksys_mmap_pgoff -> vm_mmap_pgoff > > So I think the call paths converge before any real security checks > happen, and the check you're suggesting should be superfluous. (There > is some weird audit call in ksys_mmap_pgoff() but that's just to > record the FD number, so I guess that doesn't matter.) Yeah, I was just noticing this. I was over thinking. :) It does look like all that is needed is to remove __FMODE_EXEC. -- Kees Cook
Re: [PATCH] exec: Check __FMODE_EXEC instead of in_execve for LSMs
On Wed, Jan 24, 2024 at 01:32:02PM -0800, Kees Cook wrote: > On Wed, Jan 24, 2024 at 12:47:34PM -0800, Linus Torvalds wrote: > > On Wed, 24 Jan 2024 at 12:15, Kees Cook wrote: > > > > > > Hmpf, and frustratingly Ubuntu (and Debian) still builds with > > > CONFIG_USELIB, even though it was reported[2] to them almost 4 years ago. > > For completeness, Fedora hasn't had CONFIG_USELIB for a while now. > > > Well, we could just remove the __FMODE_EXEC from uselib. > > > > It's kind of wrong anyway. > > Yeah. > > > So I think just removing __FMODE_EXEC would just do the > > RightThing(tm), and changes nothing for any sane situation. > > Agreed about these: > > - fs/fcntl.c is just doing a bitfield sanity check. > > - nfs_open_permission_mask(), as you say, is only checking for > unreadable case. > > - fsnotify would also see uselib() as a read, but afaict, > that's what it would see for an mmap(), so this should > be functionally safe. > > This one, though, I need some more time to examine: > > - AppArmor, TOMOYO, and LandLock will see uselib() as an > open-for-read, so that might still be a problem? As you > say, it's more of a mmap() call, but that would mean > adding something a call like security_mmap_file() into > uselib()... > > The issue isn't an insane "support uselib() under AppArmor" case, but > rather "Can uselib() be used to bypass exec/mmap checks?" > > This totally untested patch might give appropriate coverage: > > diff --git a/fs/exec.c b/fs/exec.c > index d179abb78a1c..0c9265312c8d 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -143,6 +143,10 @@ SYSCALL_DEFINE1(uselib, const char __user *, library) > if (IS_ERR(file)) > goto out; > > + error = security_mmap_file(file, PROT_READ | PROT_EXEC, MAP_FIXED | > MAP_SHARED); Actually, this should probably match was load_shlib() uses: PROT_READ | PROT_WRITE | PROT_EXEC, MAP_FIXED_NOREPLACE | MAP_PRIVATE, -- Kees Cook
Re: [PATCH] exec: Check __FMODE_EXEC instead of in_execve for LSMs
On Wed, Jan 24, 2024 at 12:47:34PM -0800, Linus Torvalds wrote: > On Wed, 24 Jan 2024 at 12:15, Kees Cook wrote: > > > > Hmpf, and frustratingly Ubuntu (and Debian) still builds with > > CONFIG_USELIB, even though it was reported[2] to them almost 4 years ago. For completeness, Fedora hasn't had CONFIG_USELIB for a while now. > Well, we could just remove the __FMODE_EXEC from uselib. > > It's kind of wrong anyway. Yeah. > So I think just removing __FMODE_EXEC would just do the > RightThing(tm), and changes nothing for any sane situation. Agreed about these: - fs/fcntl.c is just doing a bitfield sanity check. - nfs_open_permission_mask(), as you say, is only checking for unreadable case. - fsnotify would also see uselib() as a read, but afaict, that's what it would see for an mmap(), so this should be functionally safe. This one, though, I need some more time to examine: - AppArmor, TOMOYO, and LandLock will see uselib() as an open-for-read, so that might still be a problem? As you say, it's more of a mmap() call, but that would mean adding something a call like security_mmap_file() into uselib()... The issue isn't an insane "support uselib() under AppArmor" case, but rather "Can uselib() be used to bypass exec/mmap checks?" This totally untested patch might give appropriate coverage: diff --git a/fs/exec.c b/fs/exec.c index d179abb78a1c..0c9265312c8d 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -143,6 +143,10 @@ SYSCALL_DEFINE1(uselib, const char __user *, library) if (IS_ERR(file)) goto out; + error = security_mmap_file(file, PROT_READ | PROT_EXEC, MAP_FIXED | MAP_SHARED); + if (error) + goto exit; + /* * may_open() has already checked for this, so it should be * impossible to trip now. But we need to be extra cautious > Of course, as you say, not having CONFIG_USELIB enabled at all is the > _truly_ sane thing, but the only thing that used the FMODE_EXEC bit > were landlock and some special-case nfs stuff. Do we want to attempt deprecation again? This was suggested last time: https://lore.kernel.org/lkml/20200518130251.zih2s32q2rxhxg6f@wittgenstein/ -Kees -- Kees Cook
Re: [PATCH] exec: Check __FMODE_EXEC instead of in_execve for LSMs
On Wed, Jan 24, 2024 at 08:58:55PM +0100, Jann Horn wrote:
> On Wed, Jan 24, 2024 at 8:22 PM Kees Cook wrote:
> > After commit 978ffcbf00d8 ("execve: open the executable file before
> > doing anything else"), current->in_execve was no longer in sync with the
> > open(). This broke AppArmor and TOMOYO which depend on this flag to
> > distinguish "open" operations from being "exec" operations.
> >
> > Instead of moving around in_execve, switch to using __FMODE_EXEC, which
> > is where the "is this an exec?" intent is stored. Note that TOMOYO still
> > uses in_execve around cred handling.
>
> I think this is wrong. When CONFIG_USELIB is enabled, the uselib()
> syscall will open a file with __FMODE_EXEC but without going through
> execve(). From what I can tell, there are no bprm hooks on this path.
Hrm, that's true.
We've been trying to remove uselib for at least 10 years[1]. :(
> I don't know if it _matters_ much, given that it'll only let you
> read/execute stuff from files with valid ELF headers, but still.
Hmpf, and frustratingly Ubuntu (and Debian) still builds with
CONFIG_USELIB, even though it was reported[2] to them almost 4 years ago.
-Kees
[1] https://lore.kernel.org/lkml/20140221181103.GA5773@jtriplet-mobl1/
[2] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1879454
--
Kees Cook
Re: [PATCH] exec: Check __FMODE_EXEC instead of in_execve for LSMs
On Wed, Jan 24, 2024 at 12:39:38PM -0700, Kevin Locke wrote:
> On Wed, 2024-01-24 at 11:22 -0800, Kees Cook wrote:
> > After commit 978ffcbf00d8 ("execve: open the executable file before
> > doing anything else"), current->in_execve was no longer in sync with the
> > open(). This broke AppArmor and TOMOYO which depend on this flag to
> > distinguish "open" operations from being "exec" operations.
> >
> > Instead of moving around in_execve, switch to using __FMODE_EXEC, which
> > is where the "is this an exec?" intent is stored. Note that TOMOYO still
> > uses in_execve around cred handling.
>
> It solves the AppArmor issue I was experiencing and I don't notice any
> other issues.
>
> Tested-by: Kevin Locke
Thanks!
Sounds like Linus has taken the patch directly, and I'll send a follow-up
PR with other clean-ups.
-Kees
--
Kees Cook
[apparmor] [PATCH] exec: Check __FMODE_EXEC instead of in_execve for LSMs
After commit 978ffcbf00d8 ("execve: open the executable file before
doing anything else"), current->in_execve was no longer in sync with the
open(). This broke AppArmor and TOMOYO which depend on this flag to
distinguish "open" operations from being "exec" operations.
Instead of moving around in_execve, switch to using __FMODE_EXEC, which
is where the "is this an exec?" intent is stored. Note that TOMOYO still
uses in_execve around cred handling.
Reported-by: Kevin Locke
Closes: https://lore.kernel.org/all/zbe4qn9_h14oq...@kevinlocke.name
Suggested-by: Linus Torvalds
Fixes: 978ffcbf00d8 ("execve: open the executable file before doing anything
else")
Cc: Josh Triplett
Cc: John Johansen
Cc: Paul Moore
Cc: James Morris
Cc: "Serge E. Hallyn"
Cc: Kentaro Takeda
Cc: Tetsuo Handa
Cc: Alexander Viro
Cc: Christian Brauner
Cc: Jan Kara
Cc: Eric Biederman
Cc: Andrew Morton
Cc: Sebastian Andrzej Siewior
Cc: linux-fsde...@vger.kernel.org
Cc: linux...@kvack.org
Cc: apparmor@lists.ubuntu.com
Cc: linux-security-mod...@vger.kernel.org
Signed-off-by: Kees Cook
---
security/apparmor/lsm.c | 4 +++-
security/tomoyo/tomoyo.c | 3 ++-
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 7717354ce095..98e1150bee9d 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -469,8 +469,10 @@ static int apparmor_file_open(struct file *file)
* Cache permissions granted by the previous exec check, with
* implicit read and executable mmap which are required to
* actually execute the image.
+*
+* Illogically, FMODE_EXEC is in f_flags, not f_mode.
*/
- if (current->in_execve) {
+ if (file->f_flags & __FMODE_EXEC) {
fctx->allow = MAY_EXEC | MAY_READ | AA_EXEC_MMAP;
return 0;
}
diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index 3c3af149bf1c..04a92c3d65d4 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -328,7 +328,8 @@ static int tomoyo_file_fcntl(struct file *file, unsigned
int cmd,
static int tomoyo_file_open(struct file *f)
{
/* Don't check read permission here if called from execve(). */
- if (current->in_execve)
+ /* Illogically, FMODE_EXEC is in f_flags, not f_mode. */
+ if (f->f_flags & __FMODE_EXEC)
return 0;
return tomoyo_check_open_permission(tomoyo_domain(), >f_path,
f->f_flags);
--
2.34.1
Re: [apparmor] [PATCH] apparmor: aa_buffer: Convert 1-element array to flexible array
On Wed, May 31, 2023 at 05:21:40AM -0700, John Johansen wrote: > On 5/30/23 15:55, Kees Cook wrote: > > On Thu, May 11, 2023 at 02:48:29PM -0700, John Johansen wrote: > > > On 5/11/23 14:34, Kees Cook wrote: > > > > In the ongoing effort to convert all fake flexible arrays to proper > > > > flexible arrays, replace aa_buffer's 1-element "buffer" member with a > > > > flexible array. > > > > > > > > Cc: John Johansen > > > > Cc: Gustavo A. R. Silva > > > > Cc: Paul Moore > > > > Cc: James Morris > > > > Cc: "Serge E. Hallyn" > > > > Cc: apparmor@lists.ubuntu.com > > > > Cc: linux-security-mod...@vger.kernel.org > > > > Signed-off-by: Kees Cook > > > > > > Acked-by: John Johansen > > > > > > I have pulled this into my tree. > > > > Just a quick ping: I haven't seen this show up in -next yet... > > > > oop, sorry looks like I didn't push, it should be fixed now Awesome; thanks! -- Kees Cook
Re: [apparmor] [PATCH] apparmor: aa_buffer: Convert 1-element array to flexible array
On Thu, May 11, 2023 at 02:48:29PM -0700, John Johansen wrote: > On 5/11/23 14:34, Kees Cook wrote: > > In the ongoing effort to convert all fake flexible arrays to proper > > flexible arrays, replace aa_buffer's 1-element "buffer" member with a > > flexible array. > > > > Cc: John Johansen > > Cc: Gustavo A. R. Silva > > Cc: Paul Moore > > Cc: James Morris > > Cc: "Serge E. Hallyn" > > Cc: apparmor@lists.ubuntu.com > > Cc: linux-security-mod...@vger.kernel.org > > Signed-off-by: Kees Cook > > Acked-by: John Johansen > > I have pulled this into my tree. Just a quick ping: I haven't seen this show up in -next yet... -- Kees Cook
Re: [apparmor] [PATCH] apparmor: aa_buffer: Convert 1-element array to flexible array
On Thu, May 11, 2023 at 02:48:29PM -0700, John Johansen wrote: > On 5/11/23 14:34, Kees Cook wrote: > > In the ongoing effort to convert all fake flexible arrays to proper > > flexible arrays, replace aa_buffer's 1-element "buffer" member with a > > flexible array. > > > > Cc: John Johansen > > Cc: Gustavo A. R. Silva > > Cc: Paul Moore > > Cc: James Morris > > Cc: "Serge E. Hallyn" > > Cc: apparmor@lists.ubuntu.com > > Cc: linux-security-mod...@vger.kernel.org > > Signed-off-by: Kees Cook > > Acked-by: John Johansen > > I have pulled this into my tree. Thanks! > > > --- > > One thing I notice here is that it may be rare for "buffer" to ever change > > for a given kernel. Could this just be made PATH_MAX * 2 directly and > > remove the module parameter, etc, etc? > > possibly. Currently the only use case I know of is for some stress testing > where we drop the buffer size down really small to try and break things. > This isn't part of the regular regression runs and could be handle with a > config/compile time to a buffer size constant. Okay, cool. I figured the conversion to fixed-size is sort of nice, but it probably won't be of much use as-is since it's the buffer, not the aa_buffer, is passed around. The compiler would still not have any idea what the bounds are. :) -- Kees Cook
[apparmor] [PATCH] apparmor: aa_buffer: Convert 1-element array to flexible array
In the ongoing effort to convert all fake flexible arrays to proper
flexible arrays, replace aa_buffer's 1-element "buffer" member with a
flexible array.
Cc: John Johansen
Cc: Gustavo A. R. Silva
Cc: Paul Moore
Cc: James Morris
Cc: "Serge E. Hallyn"
Cc: apparmor@lists.ubuntu.com
Cc: linux-security-mod...@vger.kernel.org
Signed-off-by: Kees Cook
---
One thing I notice here is that it may be rare for "buffer" to ever change
for a given kernel. Could this just be made PATH_MAX * 2 directly and
remove the module parameter, etc, etc?
---
security/apparmor/lsm.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index d6cc4812ca53..35eb41bb9e3a 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -46,7 +46,7 @@ int apparmor_initialized;
union aa_buffer {
struct list_head list;
- char buffer[1];
+ DECLARE_FLEX_ARRAY(char, buffer);
};
#define RESERVE_COUNT 2
@@ -1647,7 +1647,7 @@ char *aa_get_buffer(bool in_atomic)
list_del(_buf->list);
buffer_count--;
spin_unlock(_buffers_lock);
- return _buf->buffer[0];
+ return aa_buf->buffer;
}
if (in_atomic) {
/*
@@ -1670,7 +1670,7 @@ char *aa_get_buffer(bool in_atomic)
pr_warn_once("AppArmor: Failed to allocate a memory buffer.\n");
return NULL;
}
- return _buf->buffer[0];
+ return aa_buf->buffer;
}
void aa_put_buffer(char *buf)
@@ -1747,7 +1747,7 @@ static int __init alloc_buffers(void)
destroy_buffers();
return -ENOMEM;
}
- aa_put_buffer(_buf->buffer[0]);
+ aa_put_buffer(aa_buf->buffer);
}
return 0;
}
--
2.34.1
Re: [apparmor] [PATCH 06/11] yama: simplfy sysctls with register_sysctl()
On Thu, Mar 02, 2023 at 12:28:21PM -0800, Luis Chamberlain wrote: > register_sysctl_paths() is only need if you have directories with > entries, simplify this by using register_sysctl(). > > Signed-off-by: Luis Chamberlain Acked-by: Kees Cook -- Kees Cook
Re: [apparmor] [PATCH 07/11] seccomp: simplify sysctls with register_sysctl_init()
On Thu, Mar 02, 2023 at 12:28:22PM -0800, Luis Chamberlain wrote: > register_sysctl_paths() is only needed if you have childs (directories) > with entries. Just use register_sysctl_init() as it also does the > kmemleak check for you. > > Signed-off-by: Luis Chamberlain Acked-by: Kees Cook -- Kees Cook
Re: [apparmor] [PATCH 05/11] loadpin: simplify sysctls use with register_sysctl()
On Thu, Mar 02, 2023 at 12:28:20PM -0800, Luis Chamberlain wrote: > register_sysctl_paths() is not required, we can just use > register_sysctl() with the required path specified. > > Signed-off-by: Luis Chamberlain Acked-by: Kees Cook -- Kees Cook
Re: [apparmor] [PATCH 1/2] fs/exec: Explicitly unshare fs_struct on exec
On Fri, Oct 14, 2022 at 05:35:26PM +0200, Jann Horn wrote:
> On Fri, Oct 14, 2022 at 5:18 AM Andy Lutomirski wrote:
> > On Thu, Oct 6, 2022, at 7:13 AM, Jann Horn wrote:
> > > On Thu, Oct 6, 2022 at 11:05 AM Christian Brauner
> > > wrote:
> > >> On Thu, Oct 06, 2022 at 01:27:34AM -0700, Kees Cook wrote:
> > >> > The check_unsafe_exec() counting of n_fs would not add up under a
> > >> > heavily
> > >> > threaded process trying to perform a suid exec, causing the suid
> > >> > portion
> > >> > to fail. This counting error appears to be unneeded, but to catch any
> > >> > possible conditions, explicitly unshare fs_struct on exec, if it ends
> > >> > up
> > >>
> > >> Isn't this a potential uapi break? Afaict, before this change a call to
> > >> clone{3}(CLONE_FS) followed by an exec in the child would have the
> > >> parent and child share fs information. So if the child e.g., changes the
> > >> working directory post exec it would also affect the parent. But after
> > >> this change here this would no longer be true. So a child changing a
> > >> workding directoro would not affect the parent anymore. IOW, an exec is
> > >> accompanied by an unshare(CLONE_FS). Might still be worth trying ofc but
> > >> it seems like a non-trivial uapi change but there might be few users
> > >> that do clone{3}(CLONE_FS) followed by an exec.
> > >
> > > I believe the following code in Chromium explicitly relies on this
> > > behavior, but I'm not sure whether this code is in active use anymore:
> > >
> > > https://source.chromium.org/chromium/chromium/src/+/main:sandbox/linux/suid/sandbox.c;l=101?q=CLONE_FS==chromium
> >
> > Wait, this is absolutely nucking futs. On a very quick inspection, the
> > sharable things like this are fs, files, sighand, and io.files and
> > sighand get unshared, which makes sense. fs supposedly checks for extra
> > refs and prevents gaining privilege. io is... ignored! At least it's not
> > immediately obvious that io is a problem.
> >
> > But seriously, this makes no sense at all. It should not be possible to
> > exec a program and then, without ptrace, change its cwd out from under it.
> > Do we really need to preserve this behavior?
>
> I agree that this is pretty wild.
>
> The single user I'm aware of is Chrome, and as far as I know, they use
> it for establishing their sandbox on systems where unprivileged user
> namespaces are disabled - see
> <https://chromium.googlesource.com/chromium/src/+/main/docs/linux/suid_sandbox.md>.
> They also have seccomp-based sandboxing, but IIRC there are some small
> holes that mean it's still useful for them to be able to set up
> namespaces, like how sendmsg() on a unix domain socket can specify a
> file path as the destination address.
>
> (By the way, I think maybe Chrome wouldn't need this wacky trick with
> the shared fs_struct if the "NO_NEW_PRIVS permits chroot()" thing had
> ever landed that you
> (https://lore.kernel.org/lkml/0e2f0f54e19bff53a3739ecfddb4ffa9a6dbde4d.1327858005.git.l...@amacapital.net/)
> and Mickaël Salaün proposed in the past... or alternatively, if there
> was a way to properly filter all the syscalls that Chrome has to
> permit for renderers.)
>
> (But also, to be clear, I don't speak for Chrome, this is just my
> understanding of how their stuff works.)
Chrome seems to just want a totally empty filesystem view, yes?
Let's land the nnp+chroot change. :P Only 10 years late! Then we can
have Chrome use this and we can unshare fs on exec...
--
Kees Cook
Re: [apparmor] [PATCH 1/2] fs/exec: Explicitly unshare fs_struct on exec
On Thu, Oct 13, 2022 at 08:18:04PM -0700, Andy Lutomirski wrote: > But seriously, this makes no sense at all. It should not be possible to exec > a program and then, without ptrace, change its cwd out from under it. Do we > really need to preserve this behavior? Yup, already abandoned: https://lore.kernel.org/lkml/202210061301.207A20C8E5@keescook/ -- Kees Cook
Re: [apparmor] [PATCH 1/2] fs/exec: Explicitly unshare fs_struct on exec
On October 6, 2022 7:13:37 AM PDT, Jann Horn wrote:
>On Thu, Oct 6, 2022 at 11:05 AM Christian Brauner wrote:
>> On Thu, Oct 06, 2022 at 01:27:34AM -0700, Kees Cook wrote:
>> > The check_unsafe_exec() counting of n_fs would not add up under a heavily
>> > threaded process trying to perform a suid exec, causing the suid portion
>> > to fail. This counting error appears to be unneeded, but to catch any
>> > possible conditions, explicitly unshare fs_struct on exec, if it ends up
>>
>> Isn't this a potential uapi break? Afaict, before this change a call to
>> clone{3}(CLONE_FS) followed by an exec in the child would have the
>> parent and child share fs information. So if the child e.g., changes the
>> working directory post exec it would also affect the parent. But after
>> this change here this would no longer be true. So a child changing a
>> workding directoro would not affect the parent anymore. IOW, an exec is
>> accompanied by an unshare(CLONE_FS). Might still be worth trying ofc but
>> it seems like a non-trivial uapi change but there might be few users
>> that do clone{3}(CLONE_FS) followed by an exec.
>
>I believe the following code in Chromium explicitly relies on this
>behavior, but I'm not sure whether this code is in active use anymore:
>
>https://source.chromium.org/chromium/chromium/src/+/main:sandbox/linux/suid/sandbox.c;l=101?q=CLONE_FS==chromium
Oh yes. I think I had tried to forget this existed. Ugh. Okay, so back to the
drawing board, I guess. The counting will need to be fixed...
It's possible we can move the counting after dethread -- it seems the early
count was just to avoid setting flags after the point of no return, but it's
not an error condition...
--
Kees Cook
Re: [apparmor] [PATCH] Fix race condition when exec'ing setuid files
On Thu, Sep 13, 2022 at 15:03:38 -0700, Kees Cook wrote:
> It seems quite unusual to have a high-load heavily threaded
> process decide to exec.
In looking at this a bunch more, I actually think everything is working
as intended. If a process is actively launching threads while also trying
to exec, they're going to create races for themselves.
So the question, then, is "why are they trying to exec while actively
spawning new threads?" That appears to be the core problem here, and as
far as I can tell, the kernel has behaved this way for a very long time.
I don't think the kernel should fix this, either, because it leads to a
very weird state for userspace, where the thread spawner may suddenly
die due to the exec happening in another thread. This really looks like
something userspace needs to handle correctly (i.e. don't try to exec
while actively spawning threads).
For example, here's a fix to the original PoC:
--- a.c.original2022-10-06 13:07:13.279845619 -0700
+++ a.c 2022-10-06 13:10:27.702941645 -0700
@@ -8,8 +8,10 @@
return NULL;
}
+int stop_spawning;
+
void *target(void *p) {
- for (;;) {
+ while (!stop_spawning) {
pthread_t t;
if (pthread_create(, NULL, nothing, NULL) == 0)
pthread_join(t, NULL);
@@ -17,18 +19,26 @@
return NULL;
}
+#define MAX_THREADS10
+
int main(void)
{
+ pthread_t threads[MAX_THREADS];
struct timespec tv;
int i;
- for (i = 0; i < 10; i++) {
- pthread_t t;
- pthread_create(, NULL, target, NULL);
+ for (i = 0; i < MAX_THREADS; i++) {
+ pthread_create([i], NULL, target, NULL);
}
tv.tv_sec = 0;
tv.tv_nsec = 10;
nanosleep(, NULL);
+
+ /* Signal shut down, and collect spawners. */
+ stop_spawning = 1;
+ for (i = 0; i < MAX_THREADS; i++)
+ pthread_join(threads[i], NULL);
+
if (execl("./b", "./b", NULL) < 0)
perror("execl");
return 0;
--
Kees Cook
[apparmor] [PATCH 2/2] exec: Remove LSM_UNSAFE_SHARE
With fs_struct explicitly unshared during exec, it is no longer possible
to have unexpected shared state, and LSM_UNSAFE_SHARE can be entirely
removed.
Cc: Alexander Viro
Cc: Eric Biederman
Cc: John Johansen
Cc: Paul Moore
Cc: James Morris
Cc: "Serge E. Hallyn"
Cc: Stephen Smalley
Cc: Eric Paris
Cc: Richard Haines
Cc: Casey Schaufler
Cc: Xin Long
Cc: "David S. Miller"
Cc: Todd Kjos
Cc: Ondrej Mosnacek
Cc: linux-fsde...@vger.kernel.org
Cc: linux...@kvack.org
Cc: apparmor@lists.ubuntu.com
Cc: linux-security-mod...@vger.kernel.org
Cc: seli...@vger.kernel.org
Signed-off-by: Kees Cook
---
fs/exec.c | 17 +
include/linux/security.h | 5 ++---
security/apparmor/domain.c | 5 -
security/selinux/hooks.c | 10 --
4 files changed, 3 insertions(+), 34 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index 7d5f63f03c58..3cd058711098 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1563,8 +1563,7 @@ EXPORT_SYMBOL(bprm_change_interp);
*/
static void check_unsafe_exec(struct linux_binprm *bprm)
{
- struct task_struct *p = current, *t;
- unsigned n_fs;
+ struct task_struct *p = current;
if (p->ptrace)
bprm->unsafe |= LSM_UNSAFE_PTRACE;
@@ -1575,20 +1574,6 @@ static void check_unsafe_exec(struct linux_binprm *bprm)
*/
if (task_no_new_privs(current))
bprm->unsafe |= LSM_UNSAFE_NO_NEW_PRIVS;
-
- t = p;
- n_fs = 1;
- spin_lock(>fs->lock);
- rcu_read_lock();
- while_each_thread(p, t) {
- if (t->fs == p->fs)
- n_fs++;
- }
- rcu_read_unlock();
-
- if (p->fs->users > n_fs)
- bprm->unsafe |= LSM_UNSAFE_SHARE;
- spin_unlock(>fs->lock);
}
static void bprm_fill_uid(struct linux_binprm *bprm, struct file *file)
diff --git a/include/linux/security.h b/include/linux/security.h
index 1bc362cb413f..db508a8c3cc7 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -215,9 +215,8 @@ struct sched_param;
struct request_sock;
/* bprm->unsafe reasons */
-#define LSM_UNSAFE_SHARE 1
-#define LSM_UNSAFE_PTRACE 2
-#define LSM_UNSAFE_NO_NEW_PRIVS4
+#define LSM_UNSAFE_PTRACE BIT(0)
+#define LSM_UNSAFE_NO_NEW_PRIVSBIT(1)
#ifdef CONFIG_MMU
extern int mmap_min_addr_handler(struct ctl_table *table, int write,
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 91689d34d281..1b2c0bb4d9ae 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -924,11 +924,6 @@ int apparmor_bprm_creds_for_exec(struct linux_binprm *bprm)
goto audit;
}
- if (bprm->unsafe & LSM_UNSAFE_SHARE) {
- /* FIXME: currently don't mediate shared state */
- ;
- }
-
if (bprm->unsafe & (LSM_UNSAFE_PTRACE)) {
/* TODO: test needs to be profile of label to new */
error = may_change_ptraced_domain(new, );
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 79573504783b..3ec80cc8ad1c 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2349,16 +2349,6 @@ static int selinux_bprm_creds_for_exec(struct
linux_binprm *bprm)
if (rc)
return rc;
- /* Check for shared state */
- if (bprm->unsafe & LSM_UNSAFE_SHARE) {
- rc = avc_has_perm(_state,
- old_tsec->sid, new_tsec->sid,
- SECCLASS_PROCESS, PROCESS__SHARE,
- NULL);
- if (rc)
- return -EPERM;
- }
-
/* Make sure that anyone attempting to ptrace over a task that
* changes its SID has the appropriate permit */
if (bprm->unsafe & LSM_UNSAFE_PTRACE) {
--
2.34.1
[apparmor] [PATCH 0/2] fs/exec: Explicitly unshare fs_struct on exec
Hi,
These changes seek to address an issue reported[1] by Jorge Merlino where
high-thread-count processes would sometimes fail to setuid during a
setuid execve().
It looks to me like the solution is to explicitly do an unshare_fs(),
which should almost always be a no-op. Current testing seems to indicate
that only the swapper->init exec triggers this condition (and I'm unclear
on whether that's expected or undesirable). This has only received very
light testing so far, but I wanted to share it so other folks could look
it over.
Jorge, can you test with these patches? Your PoC triggered immediately
for me on an unpatched kernel, and did not trigger on a patched one.
I added this patch on top of the series to see if the code ever fired:
diff --git a/kernel/fork.c b/kernel/fork.c
index 53b7248f7a4b..3c197d9d8daa 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -3113,6 +3113,7 @@ int unshare_fs(void)
if (error || !new_fs)
return error;
+ pr_notice("UNSHARE of \"%s\" [%d]\n", current->comm, current->pid);
unshare_fs_finalize(_fs);
if (new_fs)
Thanks!
-Kees
[1]
https://lore.kernel.org/lkml/20220910211215.140270-1-jorge.merl...@canonical.com/
Kees Cook (2):
fs/exec: Explicitly unshare fs_struct on exec
exec: Remove LSM_UNSAFE_SHARE
fs/exec.c | 26
fs/fs_struct.c | 1 -
include/linux/fdtable.h| 1 +
include/linux/fs_struct.h | 1 -
include/linux/security.h | 5 ++-
kernel/fork.c | 62 ++
security/apparmor/domain.c | 5 ---
security/selinux/hooks.c | 10 --
8 files changed, 51 insertions(+), 60 deletions(-)
--
2.34.1
[apparmor] [PATCH 1/2] fs/exec: Explicitly unshare fs_struct on exec
The check_unsafe_exec() counting of n_fs would not add up under a heavily
threaded process trying to perform a suid exec, causing the suid portion
to fail. This counting error appears to be unneeded, but to catch any
possible conditions, explicitly unshare fs_struct on exec, if it ends up
needing to happen. This means fs->in_exec must be removed (since it would
never get cleared in the old copy), and is specifically no longer needed.
See also commit 498052bba55e ("New locking/refcounting for fs_struct").
This additionally allows the "in_exec" member to be dropped, showing an
8 bytes savings per fs_struct on 64-bit. Before:
struct fs_struct {
intusers;/* 0 4 */
spinlock_t lock; /* 4 4 */
seqcount_spinlock_tseq; /* 8 4 */
intumask;/*12 4 */
intin_exec; /*16 4 */
/* XXX 4 bytes hole, try to pack */
struct pathroot; /*2416 */
struct pathpwd; /*4016 */
/* size: 56, cachelines: 1, members: 7 */
/* sum members: 52, holes: 1, sum holes: 4 */
/* last cacheline: 56 bytes */
};
After:
struct fs_struct {
intusers;/* 0 4 */
spinlock_t lock; /* 4 4 */
seqcount_spinlock_tseq; /* 8 4 */
intumask;/*12 4 */
struct pathroot; /*1616 */
struct pathpwd; /*3216 */
/* size: 48, cachelines: 1, members: 6 */
/* last cacheline: 48 bytes */
};
Reported-by: Jorge Merlino
Link:
https://lore.kernel.org/lkml/20220910211215.140270-1-jorge.merl...@canonical.com/
Cc: Eric Biederman
Cc: Alexander Viro
Cc: "Christian Brauner (Microsoft)"
Cc: Thomas Gleixner
Cc: Andy Lutomirski
Cc: Sebastian Andrzej Siewior
Cc: Andrew Morton
Cc: linux...@kvack.org
Cc: linux-fsde...@vger.kernel.org
Signed-off-by: Kees Cook
---
fs/exec.c | 9 +++---
fs/fs_struct.c| 1 -
include/linux/fdtable.h | 1 +
include/linux/fs_struct.h | 1 -
kernel/fork.c | 62 ++-
5 files changed, 48 insertions(+), 26 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index 902bce45b116..7d5f63f03c58 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1272,6 +1272,11 @@ int begin_new_exec(struct linux_binprm * bprm)
if (retval)
goto out;
+ /* Ensure the fs_struct is not shared. */
+ retval = unshare_fs();
+ if (retval)
+ goto out;
+
/*
* Must be called _before_ exec_mmap() as bprm->mm is
* not visible until then. This also enables the update
@@ -1583,8 +1588,6 @@ static void check_unsafe_exec(struct linux_binprm *bprm)
if (p->fs->users > n_fs)
bprm->unsafe |= LSM_UNSAFE_SHARE;
- else
- p->fs->in_exec = 1;
spin_unlock(>fs->lock);
}
@@ -1843,7 +1846,6 @@ static int bprm_execve(struct linux_binprm *bprm,
goto out;
/* execve succeeded */
- current->fs->in_exec = 0;
current->in_execve = 0;
rseq_execve(current);
acct_update_integrals(current);
@@ -1861,7 +1863,6 @@ static int bprm_execve(struct linux_binprm *bprm,
force_fatal_sig(SIGSEGV);
out_unmark:
- current->fs->in_exec = 0;
current->in_execve = 0;
return retval;
diff --git a/fs/fs_struct.c b/fs/fs_struct.c
index 04b3f5b9c629..c27c67023d01 100644
--- a/fs/fs_struct.c
+++ b/fs/fs_struct.c
@@ -115,7 +115,6 @@ struct fs_struct *copy_fs_struct(struct fs_struct *old)
/* We don't need to lock fs - think why ;-) */
if (fs) {
fs->users = 1;
- fs->in_exec = 0;
spin_lock_init(>lock);
seqcount_spinlock_init(>seq, >lock);
fs->umask = old->umask;
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index e066816f3519..fbfb89ee3bda 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -117,6 +117,7 @@ struct task_struct;
void put_files_struct(struct files_struct *fs);
int unshare_files(void);
+int unshare_fs(void);
struct files_struct *dup_fd(struct files_struct *, unsigned, int *)
__latent_entropy;
void do_close_on_exec(struct files_struct *);
int iterate_fd(struct files_struct *, unsigned,
diff --git a/include/linux/fs_struct.h b/include/linux/fs_struct.h
index 783b48dedb72..08b82a90ceae 100644
--- a/include/linux/
Re: [apparmor] abstractions/apache2-common - path for stapling-cache
Hi Christian, On Sat, Jun 09, 2018 at 12:35:23AM +0200, Christian Boltz wrote: > Hello, > > I just got a private bugreport (as part of a somewhat unrelated > discussion) that abstractions/apache2-common contains a strange path: > > # OCSP stapling > /var/log/apache2/stapling-cache rw, > >shouldn't that be /var/run/.. ? > > Kees, you added this line in 2e3a871b1 a year ago. Can you please check > if it's really /var/log/apache2/ in your setup or if the bugreport is > valid? The use of the log directory was suggested by this: https://raymii.org/s/tutorials/OCSP_Stapling_on_Apache2.html However, in checking my Apache install, it seems the default location is: /run/lock/apache2/ssl-stapling.$pid and /run/lock/apache2/ssl-stapling-refresh.$pid and in all cases, apache runs with it deleted, so /var/log is likely wrong. So I think we should use: /run/lock/apache2/stapling-cache* rw, -Kees -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] Fun with mod_apparmor + keepalive + iOS
On Sat, Jul 18, 2015 at 06:15:51PM +0200, Walter Hop wrote: > > Hi Walter, > > > > Anything new with this? I have a similar hat mismatch, but I've never been > > able to reproduce it. Did you manage to get strace output? > > Hi Kees, > > I did manage to minimize my case (e.g. use a simple ‘hello world' instead of > Wordpress) and still reproduce. I have strace output for a successful run > versus a failing run, with the same sequence and timing of of client requests. Thanks for doing this! I haven't been able to do any more testing on my end because I've hit a kernel bug with AppArmor under mod_apparmor. I hope to get back to looking at your straces once I have something to test with again. :) > The traces are huge and I didn’t find a good tool to present them (like a > sideways diff HTML generator), so I forgot about them. But they are here (I > replaced some variables like the pid to lower the number of uninteresting > diffs): > http://lf.ms/apparmor/strace-ok.txt <http://lf.ms/apparmor/strace-ok.txt> > http://lf.ms/apparmor/strace-fail.txt <http://lf.ms/apparmor/strace-fail.txt> On a quick look, it just seems like the failed strace simply doesn't do the changehat it needs to. :( > The Apache install is not completely minimal; there is still some unnecessary > ‘noise’ in the traces from ModSecurity. Its delays however make reproduction > much easier for me. When I disabled ModSec rules, I could reproduce much less > reliably, like 1 in 100 tries, so I never got a good trace in that state. Interesting! > PS: I also talked to a developer of an (unrelated) Apache module. He was > quite skeptical about using the log_transaction hook in the way that we rely > on for changing hats back. I didn’t find more appropriate hooks from a quick > look in Apache source, but if this hook turns out to be unreliable, maybe we > could try going on the Apache modules dev list and see if a more reliable > hook can be added which is guaranteed to fire at a useful time. Since the > request lifecycle is also undergoing architectural changes with mod_h2 > coming, maybe the module will require a bit of work anyway to be future > proof… But as I understood it, this shouldn’t be a whole lot. Yeah, I haven't looked at the source myself yet. It seems adding a hook would be a good way forward, though. > I’ll be happy to invest more time if I can be useful. Thanks and sorry for the giant delay in my reply! :) -Kees -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] Fun with mod_apparmor + keepalive + iOS
Hi Walter,
Anything new with this? I have a similar hat mismatch, but I've never been
able to reproduce it. Did you manage to get strace output?
Thanks!
-Kees
On Thu, Apr 23, 2015 at 09:25:04AM +0200, Walter Hop wrote:
On 23 Apr 2015, at 01:46, Steve Beattie st...@nxnw.org wrote:
I also am unable to see this script, as a mod_security firewall(?) seems
to block it.
Oops sorry. That ModSecurity rule against PHP source leakage… It’s nothing
special, just replays the GET requests to the server, without even reading
from the socket. I’ll paste it here.
I’ll try to get syscall traces this week. I’m pretty sure the problem will
appear when having only 1 Apache child, so it should be easy to do. Thanks to
both for listening! :)
?php
function replay(array $requests, $slowdownfactor, $host, $port = 80)
{
$fp = fsockopen($host, $port);
foreach ($requests as $request) {
list($sleep, $get) = $request;
$usleep = round($sleep * $slowdownfactor);
echo Sleeping $usleep usec... ;
usleep($usleep);
$uri = substr($get, 4, strpos($get, HTTP/1.1) - 4);
echo Getting $uri\n;
if (!fwrite($fp, $get)) {
exit(Yay! Connection was broken!\n);
}
}
fclose($fp);
}
$inputfile = 'requests.json';
$host = 'ubuntutest';
$slowdownfactor = 40; # must be between 15 - 80 for a 100%
successful reproduce
$requests = json_decode(file_get_contents($inputfile));
replay($requests, $slowdownfactor, $host);
--
Walter Hop | PGP key: https://lifeforms.nl/pgp
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
--
Kees Cook
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [patch 1/3] profiles: allow apache hats to receive signals from unconfined
On Wed, Jun 18, 2014 at 05:44:03PM -0700, Steve Beattie wrote: Allow apache hats to receive signals from unconfined. [I'm on the fence about this. On the one hand, unconfined should be able to kill thing in hats. On the other, using apache2ctl/apachectl is preferred to shutdown apache, and it uses the apache binary itself (and the profile it runs under) to kill its children.] Without this, a sysadmin or automated monitoring tools attempting to send signals to Apache will fail by default. For example, pkill -U www-data wouldn't work. This is, I think, extremely unexpected. Also, manipulating the system from unconfined has been a long-standing not protected state in AppArmor (e.g. setting up hardlinks that bypass path rules), so it seems strange to start trying to protect a profile from unconfined only for signals. -Kees --- profiles/apparmor.d/abstractions/apache2-common |2 ++ 1 file changed, 2 insertions(+) Index: b/profiles/apparmor.d/abstractions/apache2-common === --- a/profiles/apparmor.d/abstractions/apache2-common +++ b/profiles/apparmor.d/abstractions/apache2-common @@ -4,6 +4,8 @@ #include abstractions/nameservice + # Allow unconfined processes to send us signals by default + signal (receive) peer=unconfined, # Allow apache to send us signals by default signal (receive) peer=/usr/sbin/apache2, # Allow us to signal ourselves -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [patch 2/3] profiles: allow php5 abstraction access to Zend opcache files
On Wed, Jun 18, 2014 at 11:44:26PM -0700, Seth Arnold wrote: On Wed, Jun 18, 2014 at 05:44:04PM -0700, Steve Beattie wrote: Allow php5 abstraction to access Zend opcache files. [Personally, I don't really like things like this ending up in /tmp, as there's no need for it; but it's not obvious to me looking at http://www.php.net/manual/en/opcache.configuration.php if there's a way to configure things such that the opcache files end up in a php specific directory, that we could advocate packagers should make as the default.] Blech. Annoying php. Yes. This took a long time to find digging through PHP code to find the file pattern. :) Maybe add 'owner'? I'm not entirely sure how PHP expects these things to be used but it feels like a sane thing to require that the reader and writer be the same uid. Yeah, owner seems like a good idea. -Kees Acked-by: Seth Arnold seth.arn...@canonical.com Thanks --- profiles/apparmor.d/abstractions/php5 |3 +++ 1 file changed, 3 insertions(+) Index: b/profiles/apparmor.d/abstractions/php5 === --- a/profiles/apparmor.d/abstractions/php5 +++ b/profiles/apparmor.d/abstractions/php5 @@ -30,3 +30,6 @@ # MySQL extension /usr/share/mysql/** r, + + # Zend opcache + /tmp/.ZendSem.* rwlk, -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [patch 3/3] profiles: apache2 — allow HANDLING_UNTRUSTED_INPUT access to abstractions/base
On Wed, Jun 18, 2014 at 11:29:31PM -0700, Seth Arnold wrote:
On Wed, Jun 18, 2014 at 05:44:05PM -0700, Steve Beattie wrote:
This patch adds the abstractions/base abstraction to the
HANDLING_UNTRUSTED_INPUT apache2 hat.
[I dislike this because the idea for the HANDLING_UNTRUSTED_INPUT is
that it is to be as minimal as possible, as sort of a poor man's
privilege separation for when apache is parsing a request and
determining what to do with it. The abstractions/base abstraction allows
too much for such a hat IMO. (Honestly, I'd like cut down the existing
allowed accesses in it.)]
HANDLING_UNTRUSTED_INPUT has always had some unexpected consequences; I
love the idea but it just might not work with Apache's reality. Since we
don't have much chance of fixing reality and changing the module to do
something else is probably not going to happen soon, we might as well make
this as painless as possible.
Also, agreed on cutting down on abstractions/base, but I'm so reluctant to
tighten shipped profiles.
This part is a little weird, actually. So, my bug report didn't entirely
match my patch for HANDLING_UNTRUSTED_INPUT. (In the report I say to add
the signal handling only... but in the patch I end up adding base and
apache2-common.) However, this probably doesn't matter much because the
current default for HANDLING_UNTRUSTED_INPUT is:
/ rw,
/** mrwlkix,
Which is totally crazy if the goal is tight isolation (which I totally
agree with and mentioned in the bug).
I ran out of time trying to further analyze the needs of
HANDLING_UNTRUSTED_INPUT, but it seems that any access needed during
location resolution is needed (e.g. authentication modules), so it may make
sense to extend this with a local include. In my case I had to explicitly
allow /run/saslauthd/mux rw, in apache2-common.
Anyway, I guess my point is, at a minimum, the signal handlers need to be
added, but given the existing totally open permissions, it doesn't
currently hurt to add base.
-Kees
Acked-by: Seth Arnold seth.arn...@canonical.com
Thanks
---
profiles/apparmor.d/usr.sbin.apache2 |1 +
1 file changed, 1 insertion(+)
Index: b/profiles/apparmor.d/usr.sbin.apache2
===
--- a/profiles/apparmor.d/usr.sbin.apache2
+++ b/profiles/apparmor.d/usr.sbin.apache2
@@ -88,6 +88,7 @@
}
^HANDLING_UNTRUSTED_INPUT {
+#include abstractions/base
#include abstractions/apache2-common
/ rw,
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
--
Kees Cook
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [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
On Thu, Jan 16, 2014 at 11:11:22AM +0100, Didier 'OdyX' Raboud wrote: Le mercredi, 15 janvier 2014, 11.14:07 Seth Arnold a écrit : On Wed, Jan 15, 2014 at 07:30:52PM +0100, intrigeri wrote: From: Didier Raboud o...@debian.org apparmor could have an 'interest /etc/apparmor.d/' triggers file and its postinst would then do the machinery to create (or remove) the /etc/apparmor.d/local/* files accordingly. This does sound nice, but the next part worries me.. This could also have the side benefit of only running apparmor_parser once for all files installed at the same time. When would this single apparmor_parser run happen? It needs to happen before daemons are started or restarted in their postinst scripts, otherwise the AppArmor policy won't be enforced. 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. All-policy reloads should be avoided entirely, so they shouldn't be triggers either. :) -Kees -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [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
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. -Kees -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] switch away from REPO_URL shortcut
Since --per-file-timestamps is broken over the SSH transport, make the default the HTTPS URI instead. Signed-off-by: Kees Cook k...@ubuntu.com --- === modified file 'Makefile' --- Makefile2012-03-22 20:17:48 + +++ Makefile2013-12-02 21:47:38 + @@ -12,7 +12,9 @@ changehat/pam_apparmor \ tests -REPO_URL?=lp:apparmor +#REPO_URL?=lp:apparmor +# --per-file-timestamps is failing over SSH, https://bugs.launchpad.net/bzr/+bug/1257078 +REPO_URL?=https://code.launchpad.net/~apparmor-dev/apparmor/master # alternate possibilities to export from #REPO_URL=. #REPO_URL=bzr+ssh://bazaar.launchpad.net/~sbeattie/+junk/apparmor-dev/ -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] RFC: Patch [Bug 1207424] Re: mod_apparmor should let me use ServerName as default hat name
On Fri, Aug 02, 2013 at 01:41:37AM -0700, John Johansen wrote: This is a first pass at providing the feature requested in Bug 1207424 It leverages the appache config option AADefaultHatName and when its value is specified as hostname the hostname will be looked up and used. Obviously this patch isn't complete, but its a first pass and I wanted feedback before I put any more work into it. Hm, I don't think this is what the intention of the bug was describing. This doesn't want to fall back to the actual host name, it wants AADefaultHatName to contain the virtualhost ServerName. I assume this can really only be implemented at check-time, with a similar empty value? I.e. AADefaultHatName should contain the string vhost or something, and at check time, vhost will be expanded to the servername of the currently active vhost for the request. I haven't read the code at all though, so I'm kind of guessing blindly. :) -Kees -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH] fix missing long opt arg value
Using --subdomainfs without an argument triggers a segfault. This was due
to the long option missing the has_arg flag.
Signed-off-by: Kees Cook k...@ubuntu.com
=== modified file 'parser/parser_main.c'
--- parser/parser_main.c2013-05-02 18:32:56 +
+++ parser/parser_main.c2013-06-26 18:04:16 +
@@ -91,7 +91,7 @@
{add, 0, 0, 'a'},
{binary, 0, 0, 'B'},
{base,1, 0, 'b'},
- {subdomainfs, 0, 0, 'f'},
+ {subdomainfs, 1, 0, 'f'},
{help,2, 0, 'h'},
{replace, 0, 0, 'r'},
{reload, 0, 0, 'r'}, /* undocumented reload
option == replace */
--
Kees Cook
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH v2] apparmor: implement profile-based query interface in apparmorfs
Hi,
On Tue, Mar 05, 2013 at 06:38:35PM -0800, Tyler Hicks wrote:
Allow userspace applications to query for allowed, denied, audit, and
quiet permissions using a profile name and a DFA match string. Userspace
applications that wish to enforce access controls defined in the
system's AppArmor policy can use this interface to perform access
control lookups.
This patch adds a new file, called .access, to the apparmorfs root
directory. The semantics of the .access file should be hidden behind a
libapparmor interface, but the process for doing a query looks like
this:
open(/sys/kernel/security/apparmor/.access, O_RDWR) = 3
write(3, profile\0/usr/bin/app\0 system\0org..., 98) = 98
read(3, allow 0x02\ndeny 0x00\naud..., 1024) = 59
close(3) = 0
The write() buffer contains the prefix specific to the current type of
current (profile\0 in this case), the profile name followed by a '\0',
and the binary DFA match string. The read() buffer contains the query
results. Here's an example of the query results:
allow 0x02
deny 0x00
audit 0x00
quiet 0x00
The returned masks can be compared to the permission mask of interest.
In the above example, the permission represented by 0x02 would be
allowed and the action would not be audited. The permission represented
by 0x01 would not be allowed and an AVC audit message would need to
be generated.
Signed-off-by: Tyler Hicks tyhi...@canonical.com
[...]
+ * Returns: number of bytes written or -errno on failure
+ */
+static ssize_t aa_write_access(struct file *file, const char __user *ubuf,
+size_t count, loff_t *ppos)
+{
+ char *buf;
+ ssize_t len;
+
+ if (*ppos)
+ return -ESPIPE;
[...]
@@ -787,6 +911,7 @@ static struct aa_fs_entry aa_fs_entry_apparmor[] = {
AA_FS_FILE_FOPS(.load, 0640, aa_fs_profile_load),
AA_FS_FILE_FOPS(.replace, 0640, aa_fs_profile_replace),
AA_FS_FILE_FOPS(.remove, 0640, aa_fs_profile_remove),
+ AA_FS_FILE_FOPS(.access, 0666, aa_fs_access),
#ifdef CONFIG_SECURITY_APPARMOR_COMPAT_24
AA_FS_FILE_FOPS(profiles, 0640, aa_fs_profiles_fops),
#endif
This is a mode-666 file with no check for CAP_MAC_ADMIN. Is there a reason
this is so open? Is there a chance path names could leak via this
interface? (i.e. query for a path only readable by root, etc?) On the other
hand, everything in /etc/apparmor.d/ is readable (not that it needs to be).
Could it leak dynamic profiles? The list of profiles is 0640...
-Kees
--
Kees Cook
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [patch] libapparmor - use python-config if it exists when configuring
On Mon, Jan 28, 2013 at 11:44:18PM -0800, Steve Beattie wrote: Author: Dmitrijs Ledkovs dmitrij.led...@ubuntu.com This patch modifies the libapparmor macro for python to use python-config if it exists to determine what CPPFLAGS and LDFLAGS to use when building the python swig libraries. Without this addition, python detection fails on ubuntu 13.04. I've confirmed that with this patch applied, the python libraries still build successfully on older releases as well (as far back as ubuntu 11.10). Acked-by: Kees Cook k...@ubuntu.com -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 10/27] apparmor: misc cleanup of match
On Tue, Nov 20, 2012 at 08:39:50PM -0800, John Johansen wrote: tidying up comments, includes and defines Signed-off-by: John Johansen john.johan...@canonical.com Acked-by: Kees Cook k...@ubuntu.com -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 11/27] apparmor: move perm defines into policy_unpack
On Tue, Nov 20, 2012 at 08:39:51PM -0800, John Johansen wrote: Signed-off-by: John Johansen john.johan...@canonical.com Acked-by: Kees Cook k...@ubuntu.com -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 12/27] apparmor: remove sid from profiles
On Tue, Nov 20, 2012 at 08:39:52PM -0800, John Johansen wrote:
The sid is not going to be a direct property of a profile anymore, instead
it will be directly related to the label, and the profile will pickup
a label back reference.
For null-profiles replace the use of sid with a per namespace unique
id.
Signed-off-by: John Johansen john.johan...@canonical.com
diff --git a/security/apparmor/include/policy.h
b/security/apparmor/include/policy.h
index 95979c4..aadcbf8 100644
--- a/security/apparmor/include/policy.h
+++ b/security/apparmor/include/policy.h
@@ -127,6 +127,8 @@ struct aa_namespace {
struct aa_ns_acct acct;
struct aa_profile *unconfined;
struct list_head sub_ns;
+
+ atomic_t uniq_null;
};
Drop empty line?
After that,
Acked-by: Kees Cook k...@ubuntu.com
(Yay, no sid!)
--
Kees Cook
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 13/27] apparmor: move the free_profile fn ahead of aa_alloc_profile
On Tue, Nov 20, 2012 at 08:39:53PM -0800, John Johansen wrote: From: John Johansen john.johan...@canonical.com Move the free_profile fn ahead of aa_alloc_profile so it can be used in aa_alloc_profile without a forward declaration. Signed-off-by: John Johansen john.johan...@canonical.com Acked-by: Kees Cook k...@ubuntu.com -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 14/27] apparmor: reserve and mask off the top 8 bits of the base field
On Tue, Nov 20, 2012 at 08:39:54PM -0800, John Johansen wrote: The top 8 bits of the base field have never been used, in fact can't be used, by the current 'dfa16' format. However they will be used in the future as flags, so mask them off when using base as an index value. Note: the use of the top 8 bits, without masking is trapped by the verify checks that base entries are within the size bounds. Signed-off-by: John Johansen john.johan...@canonical.com Acked-by: Kees Cook k...@ubuntu.com -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 16/27] apparmor: add a features/policy dir to interface
On Tue, Nov 20, 2012 at 08:39:56PM -0800, John Johansen wrote: Add a policy directory to features to contain features that can affect policy compilation but do not affect mediation. Eg of such features would be types of dfa compression supported, etc. Signed-off-by: John Johansen john.johan...@canonical.com Acked-by: Kees Cook k...@ubuntu.com -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [patch] add pkg-config support to libapparmor
On Sat, Nov 10, 2012 at 11:08:44AM -0800, Steve Beattie wrote: Given that we want to do more apparmor things in user space (dbus mediation, file picker, etc.), making it easier for other source bases to detect the presence of libapparmor would be beneficial. This patch adds pkg-config support to the build infrastructure for libapparmor. (This code is mostly monkey-patched mimicry; people who actually know what they're doing with pkg-config are encouraged to comment.) Better than not having it! :) Acked-by: Kees Cook k...@ubuntu.com -Kees -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] Allow defaults except for reading a directory
On Sun, Aug 26, 2012 at 12:01:41PM -0500, Ian Nicholson wrote: On 08/26/2012 11:58 AM, Ahmet Emre Aladağ wrote: Now I found the problem. When I use sh pycharm.sh, it doesn't work When I use ./pycharm.sh, it works! Access denied. Thank you very very much, it was very kind of you. I ran into this myself yesterday, except with a python file. I assume it's because running python filename.py causes apparmor to apply the profile for the python interpreter, whereas running ./filename.py will cause apparmor to use the profile that I've created for the actual script. Can anyone tell me if that's right? That's correct. It's a matter of what the kernel thinks is executing. The first is running the interpreter with an argument (the script) and the latter is running the script, which has a given interpreter. -Kees -- Kees Cook@outflux.net -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [Patch] fix parser to check for network flag when generating policy
On Sat, Jun 30, 2012 at 12:26:48AM -0700, John Johansen wrote: 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 Acked-by: Kees Cook k...@ubuntu.com -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [patch] techdoc.pdf improvements
Hi Christian, On Tue, May 08, 2012 at 09:59:11PM +0200, Christian Boltz wrote: - make table of contents, footnotes etc. clickable hyperlinks Nice! - use timestamp of techdoc.tex (instead of build time) as creationdate in the PDF metadata - don't include build date on first page of the PDF Oh good -- this had been bothering me. - make clean: - delete techdoc.out (created by pdftex) - fix deletion of techdoc.txt (was techdo_r_.txt) Hah, whoops. The initial target was to get reproduceable PDF builds (therefore the timestamp-related changes), the other things came up during discussing this patch with David Haller. The only remaining difference in the PDF from build to build is the /ID line. This line can't be controlled in pdflatex and is now filtered out by build-compare in the openSUSE build service (bnc#760867). Credits go to David Haller for writing large parts of this patch (but he didn't notice the techdo_r_.txt ;-) Looks like our pdflatex outputs differ. On Debian/Ubuntu, there's no techdoc.txt generated: -rw-rw-r-- 1 kees kees 3542 May 8 14:41 techdoc.aux -rw-rw-r-- 1 kees kees 2200 May 8 14:41 techdoc.toc -rw-rw-r-- 1 kees kees 246153 May 8 14:41 techdoc.pdf -rw-rw-r-- 1 kees kees 14265 May 8 14:41 techdoc.log Signed-Off-By: Christian Boltz appar...@cboltz.de Acked-by: Kees Cook k...@ubuntu.com -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH] towards a common build infrastructure
On Mon, May 07, 2012 at 12:09:08AM +, Seth Arnold wrote: Use $MAKE instead, that way the jobserver can keep informed about how many jobs sub-makes are using. I do like that ./configure --prefix makes selecting between /usr and /usr/local easy but I have trouble seeing what else it gives us. Sanity for shared library creation (libtool, pkg-config, etc), automatic handling of finding functions (e.g. I'm building on Debian kfreebsd...), and finding dep version (e.g. swig, perl, python, ruby, apache). Trying to do all this by hand is a waste of time -- all of the needed logic already exists in autoconf/automake. From: Christian Boltz appar...@cboltz.de [...] I'd guess even more people understand plain Makefiles ;-) and I can guarantee you that nearly nobody understands the automake-generated Makefiles (reading them might be needed to track down build issues) It's extremely rare to need to look at anything by the Makefile.am. Well, I remember (from another project) that plain Makefiles can work with subdirs without problems. Something like this will work as Makefile in the top-level directory (untested braindump) Right. When I redesigned the Inkscape build system, the final result was a single top-level Makefile (generated by autoconf and automake), that allowed for full parallelization of the build, etc. It was extremely nice. We can easily get there, but doing the migration is going to take a few steps. And now tell me how automake can make things simpler than this ;-) I know it'll be hard to convince you, but without being too flippant, the automake version of your example is a single line, just SUBDIRS = :) -Kees -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH] build on non-Linux systems again
This uses sys/capability.h instead of linux/capability.h, so that AppArmor can build on non-Linux systems again. Signed-off-by: Kees Cook k...@ubuntu.com Index: apparmor-debian/common/Make.rules === --- apparmor-debian.orig/common/Make.rules 2012-04-24 11:23:59.0 -0700 +++ apparmor-debian/common/Make.rules 2012-05-05 09:46:19.614215990 -0700 @@ -157,10 +157,10 @@ # = # emits defined capabilities in a simple list, e.g. CAP_NAME CAP_NAME2 -CAPABILITIES=$(shell echo \#include linux/capability.h | cpp -dM | LC_ALL=C sed -n -e '/CAP_EMPTY_SET/d' -e 's/^\#define[ \t]\+CAP_\([A-Z0-9_]\+\)[ \t]\+\([0-9xa-f]\+\)\(.*\)$$/CAP_\1/p' | sort) +CAPABILITIES=$(shell echo \#include sys/capability.h | cpp -dM | LC_ALL=C sed -n -e '/CAP_EMPTY_SET/d' -e 's/^\#define[ \t]\+CAP_\([A-Z0-9_]\+\)[ \t]\+\([0-9xa-f]\+\)\(.*\)$$/CAP_\1/p' | sort) .PHONY: list_capabilities -list_capabilities: /usr/include/linux/capability.h +list_capabilities: @echo $(CAPABILITIES) # = -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH] build on non-Linux systems again
On Sat, May 05, 2012 at 12:24:23PM -0700, John Johansen wrote: On 05/05/2012 12:12 PM, Kees Cook wrote: This uses sys/capability.h instead of linux/capability.h, so that AppArmor can build on non-Linux systems again. Signed-off-by: Kees Cook k...@ubuntu.com Hrmm, we need something different here r2008 was added specifically because using sys/capability.h was causing builds to fail. My memory is really fuzzy but I think there where two issues a libcap2 dependency, and some capabilities missing from the sys/ variant that where present in the linux variant. Hrm, my tests showed them producing the same output on Linux. Though, that said, now my builds are failing on Linux (and not on Debian kfreebsd). So, this still needs work. Where was sys/ missing stuff? -Kees -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH] build on non-Linux systems again
On Sat, May 05, 2012 at 12:29:56PM -0700, Kees Cook wrote: Hrm, my tests showed them producing the same output on Linux. Though, that said, now my builds are failing on Linux (and not on Debian kfreebsd). So, this still needs work. Ah! Okay, got it now. Yeah, libcap-dev is also only on Linux, the problem is actually just the second chunk of my patch. I'll resend. -Kees -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH] do not require /usr/include/linux/capability.h
For non-Linux builds, the unused list_capabilities target requirements blows up the build. It's valid for non-Linux systems to have an empty capability list, since they do not build the real AppArmor parser. Signed-off-by: Kees Cook k...@ubuntu.com Index: apparmor-debian/common/Make.rules === --- apparmor-debian.orig/common/Make.rules 2012-05-05 12:36:17.0 -0700 +++ apparmor-debian/common/Make.rules 2012-05-05 12:45:30.658963850 -0700 @@ -160,7 +160,7 @@ CAPABILITIES=$(shell echo \#include linux/capability.h | cpp -dM | LC_ALL=C sed -n -e '/CAP_EMPTY_SET/d' -e 's/^\#define[ \t]\+CAP_\([A-Z0-9_]\+\)[ \t]\+\([0-9xa-f]\+\)\(.*\)$$/CAP_\1/p' | sort) .PHONY: list_capabilities -list_capabilities: /usr/include/linux/capability.h +list_capabilities: @echo $(CAPABILITIES) # = -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH] Adjust notify group name
Hi Jamie, On Wed, Apr 25, 2012 at 07:13:46AM -0500, Jamie Strandboge wrote: On Tue, 2012-04-24 at 16:58 -0700, Kees Cook wrote: The group for reading /var/log/kern.log is adm, not admin. Bug-Debian: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=660078 Index: apparmor-debian/utils/notify.conf === --- apparmor-debian.orig/utils/notify.conf 2010-11-03 17:03:52.0 -0700 +++ apparmor-debian/utils/notify.conf 2012-04-24 11:54:27.997521983 -0700 @@ -12,4 +12,4 @@ show_notifications=yes # Only people in use_group can use aa-notify -use_group=admin +use_group=adm This group's intended use is not for DAC filesystem access but instead to limit who is allowed to run the utility. From the man page which describes /etc/apparmor/notify.conf: # only people in use_group can use aa-notify use_group=admin Also, this can be overridden via /etc/apparmor/notify.conf, so I'm not sure why it needs to be changed in the script itself. Was there a particular problem that this patch is trying to address? Right, I'm patching notify.conf here. The details are in the Debian bug, but basically, checking for admin isn't sane since it tries to read the log files that are only readable by adm. Therefore, switch the check to what is actually needed. -Kees -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH] include IceWeasel in FireFox abstraction
Hi Jamie,
On Wed, Apr 25, 2012 at 07:21:26AM -0500, Jamie Strandboge wrote:
On Tue, 2012-04-24 at 17:01 -0700, Kees Cook wrote:
Include IceWeasel in FireFox abstraction.
Author: Intrigeri intrig...@debian.org
Bug-Debian: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=661176
Signed-off-by: Kees Cook k...@ubuntu.com
Index: apparmor-debian/profiles/apparmor.d/abstractions/ubuntu-browsers
===
--- apparmor-debian.orig/profiles/apparmor.d/abstractions/ubuntu-browsers
2012-04-24 11:03:46.506994000 -0700
+++ apparmor-debian/profiles/apparmor.d/abstractions/ubuntu-browsers
2012-04-24 13:01:22.499517948 -0700
@@ -29,8 +29,8 @@
# this should cover all firefox browsers and versions (including
shiretoko
# and abrowser)
- /usr/bin/firefox Cxr - sanitized_helper,
- /usr/lib/firefox*/firefox*.sh Cx - sanitized_helper,
+ /usr/bin/{firefox,iceweasel} Cxr - sanitized_helper,
+ /usr/lib/{firefox*,iceweasel}/{firefox*.sh,iceweasel} Cx -
sanitized_helper,
# some unpackaged, but popular browsers
/usr/lib/icecat-*/icecat Cx - sanitized_helper,
Hmmm, there is a namespace issue here. We are fixing a Debian bug in an
Ubuntu abstraction for a package that is not available on Ubuntu. I
understand why this was done, and I see icecat is in the context. ISTR
there being some repo that Ubuntu users could get icecat So I guess
I just wanted to bring up the point that Debian may want to consider
their own namespace. I would prefer if this was not mixed in with the
official Ubuntu browser though. Can you separate it out like with
icecat? Once that is done:
Acked-By: Jamie Strandboge ja...@canonical.com
Do you mean putting this in the file:
+ /usr/bin/iceweasel Cxr - sanitized_helper,
+ /usr/lib/iceweasel/iceweasel Cx - sanitized_helper,
Instead of combining it with firefox, or do you mean an entirely separate
abstraction?
-Kees
--
Kees Cook
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH] enhance aa-status to deal with lack of interface patch
On Tue, Apr 24, 2012 at 11:33:09PM -0700, Steve Beattie wrote:
On Tue, Apr 24, 2012 at 04:59:08PM -0700, Kees Cook wrote:
Handle lacking the interface patch, and examine the enabled
parameter for determining the state of AppArmor on the system.
Bug-Debian: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=661153
Index: apparmor-debian/utils/aa-status
===
--- apparmor-debian.orig/utils/aa-status2011-05-27 12:08:50.0
-0700
+++ apparmor-debian/utils/aa-status 2012-04-24 12:42:39.540597212 -0700
@@ -14,8 +14,7 @@
def cmd_enabled():
'''Returns error code if AppArmor is not enabled'''
-if get_profiles() == {}:
-sys.exit(2)
+find_apparmorfs()
I think this changes the behavior when apparmor is loaded and there are
no profiles loaded:
# without patch
$ sudo cat /sys/kernel/security/apparmor/profiles | wc -l
0
$ sudo aa-status --enabled
$ echo $?
2
# with patch
$ sudo cat /sys/kernel/security/apparmor/profiles | wc -l
0
$ sudo ./aa-status --enabled
$ echo $?
0
Yeah. I've kind of always felt that this was a mis-feature. The things
the init system wants to know are:
- is AppArmor capable of functioning? (in kernel and enabled)
- is it time to start loading profiles? (... in Ubuntu we cheated by
looking at if profiles are already loaded -- since all systems with
AppArmor will have preloaded the network-early-start profiles. This
is, however, a catch-22 and takes advantage of a specific side-effect
of how Ubuntu loads profiles.)
So, if we can eliminate the need for the second thing above, then the
problem goes away, and --enabled can stop lying. :)
If I remember correctly, doing the profile count was to avoid loading
profiles in certain conditions. Unfortunately, I can't remember for sure
what those situations were. I think it was: LiveCD and release upgrades,
but I'm pretty sure both are covered now.
I'd like to have --enabled not lie, and work out the bugs this triggers
as separate issues. If this isn't cool for now, how about having it only
lie if the legacy profiles interface exists?
def cmd_profiled():
'''Prints the number of loaded profiles'''
@@ -72,19 +71,15 @@
'''Fetch loaded profiles'''
profiles = {}
-
-if os.path.exists(/sys/module/apparmor):
-stdmsg(apparmor module is loaded.)
-else:
-errormsg(apparmor module is not loaded.)
-sys.exit(1)
-
apparmorfs = find_apparmorfs()
-if not apparmorfs:
-errormsg(apparmor filesystem is not mounted.)
-sys.exit(3)
+# Kernel with the stock kernel cannot read profiles, but shouldn't
+# be considered a fatal failure mode.
apparmor_profiles = os.path.join(apparmorfs, profiles)
+if not os.path.exists(apparmor_profiles):
+errormsg(AppArmor running without interface patch -- cannot
determine loaded profiles.)
+return profiles
+
This should possibly be its own error code state, I think, to
distinguish from apparmor enabled, no policy loaded to apparmor
loaded, but the version doesn't let me determine how many profiles
are loaded. (It's up to the calling program to treat the return codes
as fatal events -- there's no real other way for aa-status to return
complex results.)
Well... as I mention in the comment, I don't think it's actually
an error condition. I think aa-status should attempt to handle it,
but emit a warning (which this does). It just means it can't tell the
user about processes that are running that are unconfined by a loaded
profile. It can still report all the other states sanely (since it reads
/proc/$pid/attr/current).
if not os.access(apparmor_profiles, os.R_OK):
errormsg(You do not have enough privilege to read the profile
set.)
sys.exit(4)
@@ -134,11 +129,29 @@
def find_apparmorfs():
'''Finds AppArmor mount point'''
+
+apparmor_module = /sys/module/apparmor
+if os.path.exists(apparmor_module):
+stdmsg(AppArmor available in kernel.)
+else:
+errormsg(AppArmor not available in kernel.)
+sys.exit(1)
+
+apparmor_enabled = os.path.join(apparmor_module, parameters,
enabled)
+if not os.access(apparmor_enabled, os.R_OK):
+errormsg(You do not have enough privilege to check AppArmor
parameters.)
+sys.exit(2)
This should be sys.exit(4), no? From aa-status(1):
Upon exiting, aa-status will set its return value to the following values:
[SNIP]
4 if the user running the script doesn't have enough privileges to
read the apparmor control files.
Ah, yes. I will fix that.
Also, it's not a huge thing, but I'd appreciate a blank line here, to make the
sys.exit(2) more visible. I'm sure this violates some PEP somewhere.
Sure.
+if open
Re: [apparmor] [PATCH] enhance aa-status to deal with lack of interface patch
On Wed, Apr 25, 2012 at 04:37:15PM -0700, Steve Beattie wrote: On Wed, Apr 25, 2012 at 12:32:48PM -0700, Kees Cook wrote: Yeah. I've kind of always felt that this was a mis-feature. The things the init system wants to know are: - is AppArmor capable of functioning? (in kernel and enabled) - is it time to start loading profiles? (... in Ubuntu we cheated by looking at if profiles are already loaded -- since all systems with AppArmor will have preloaded the network-early-start profiles. This is, however, a catch-22 and takes advantage of a specific side-effect of how Ubuntu loads profiles.) So, if we can eliminate the need for the second thing above, then the problem goes away, and --enabled can stop lying. :) If I remember correctly, doing the profile count was to avoid loading profiles in certain conditions. Unfortunately, I can't remember for sure what those situations were. I think it was: LiveCD and release upgrades, but I'm pretty sure both are covered now. Actually, there are more conditions than that, at least for operating within an LSB compliant init script. In particular, the 'try-restart' option needs to restart apparmor only if it's already running. Given that apparmor is not a daemon, but a kernel element that needs to be configured, we (upstream) made the decision to assume that even if apparmor is enabled in the kernel but has 0 profiles loaded, this was considered not running, under the assumption that the initscript had either been disabled or an admin had done a 'stop' action, which would unload all profiles (and yes, I'm aware that the ubuntu/debian initscript differs in behavior here). In other words, we map the LSB running state to apparmor is enabled *and* enforcing at least one policy. Identifying the situation where apparmor was enabled but not enforcing anything *in a way that can be detected programmatically without worrying about translations/locales/strings that change, etc.* was deemed important and why it was done as a return code. Now, that said, the initscripts we provide don't actually make use of aa-status to determine the state of apparmor, other than for the 'status' target (I think because we worked with the assumption that the apparmor utils weren't guaranteed to be installed; also that people wouldn't necessarily want to pay the costs of starting a perl/python interpreter during boot). However, the non-verbose options were added to aa-status to support being invoked by other programs that could potentially make decisions/take actions like restarting services based on its output; things like CIM providers and other monitoring/controlling services. Hrm, okay. Well, aa-status is used by dh_apparmor to decide if a profile should be loaded. I guess it should check for rc == 0 || rc == 2. E.g.: # Reload AppArmor profile APP_PROFILE=/etc/apparmor.d/usr.sbin.named if [ -f $APP_PROFILE ] aa-status --enabled 2/dev/null; then apparmor_parser -r $APP_PROFILE || true fi What do you think this should be doing here? I'd like to have --enabled not lie, and work out the bugs this triggers as separate issues. If this isn't cool for now, how about having it only lie if the legacy profiles interface exists? This should possibly be its own error code state, I think, to distinguish from apparmor enabled, no policy loaded to apparmor loaded, but the version doesn't let me determine how many profiles are loaded. (It's up to the calling program to treat the return codes as fatal events -- there's no real other way for aa-status to return complex results.) Well... as I mention in the comment, I don't think it's actually an error condition. I think aa-status should attempt to handle it, but emit a warning (which this does). It just means it can't tell the user about processes that are running that are unconfined by a loaded profile. It can still report all the other states sanely (since it reads /proc/$pid/attr/current). The thing about aa-status is that it serves two purposes: 1) as a user facing tool to get a snapshot of the state of apparmor policy and enforcement on the system and 2) a tool intended to be used by other programs to capture the state of apparmor. Emitting warning messages is fine in the former situation, but less so in the latter. Knowing *why* 'aa-status --profiled' returned 0 is useful (are there actually zero profiles loaded or is it that this is using an upstream kernel?). My goal is to have aa-status able to run, even in the face of a missing interface file. Since my other goal is to solving the missing interface file, I also don't want to over-engineer an aa-status solution. I am comfortable with this warning about not being able to read the list of in-kernel profiles. That said, I think we can construct things in such a way to satisfy people's needs. Arguably, in hindsight, I can see the utility
[apparmor] [PATCH] Adjust notify group name
The group for reading /var/log/kern.log is adm, not admin. Bug-Debian: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=660078 Index: apparmor-debian/utils/notify.conf === --- apparmor-debian.orig/utils/notify.conf 2010-11-03 17:03:52.0 -0700 +++ apparmor-debian/utils/notify.conf 2012-04-24 11:54:27.997521983 -0700 @@ -12,4 +12,4 @@ show_notifications=yes # Only people in use_group can use aa-notify -use_group=admin +use_group=adm -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH] enhance aa-status to deal with lack of interface patch
Handle lacking the interface patch, and examine the enabled
parameter for determining the state of AppArmor on the system.
Bug-Debian: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=661153
Index: apparmor-debian/utils/aa-status
===
--- apparmor-debian.orig/utils/aa-status2011-05-27 12:08:50.0
-0700
+++ apparmor-debian/utils/aa-status 2012-04-24 12:42:39.540597212 -0700
@@ -14,8 +14,7 @@
def cmd_enabled():
'''Returns error code if AppArmor is not enabled'''
-if get_profiles() == {}:
-sys.exit(2)
+find_apparmorfs()
def cmd_profiled():
'''Prints the number of loaded profiles'''
@@ -72,19 +71,15 @@
'''Fetch loaded profiles'''
profiles = {}
-
-if os.path.exists(/sys/module/apparmor):
-stdmsg(apparmor module is loaded.)
-else:
-errormsg(apparmor module is not loaded.)
-sys.exit(1)
-
apparmorfs = find_apparmorfs()
-if not apparmorfs:
-errormsg(apparmor filesystem is not mounted.)
-sys.exit(3)
+# Kernel with the stock kernel cannot read profiles, but shouldn't
+# be considered a fatal failure mode.
apparmor_profiles = os.path.join(apparmorfs, profiles)
+if not os.path.exists(apparmor_profiles):
+errormsg(AppArmor running without interface patch -- cannot determine
loaded profiles.)
+return profiles
+
if not os.access(apparmor_profiles, os.R_OK):
errormsg(You do not have enough privilege to read the profile set.)
sys.exit(4)
@@ -134,11 +129,29 @@
def find_apparmorfs():
'''Finds AppArmor mount point'''
+
+apparmor_module = /sys/module/apparmor
+if os.path.exists(apparmor_module):
+stdmsg(AppArmor available in kernel.)
+else:
+errormsg(AppArmor not available in kernel.)
+sys.exit(1)
+
+apparmor_enabled = os.path.join(apparmor_module, parameters, enabled)
+if not os.access(apparmor_enabled, os.R_OK):
+errormsg(You do not have enough privilege to check AppArmor
parameters.)
+sys.exit(2)
+if open(apparmor_enabled, r).readline().strip() != Y:
+errormsg(AppArmor not enabled. (Kernel not booted with
\security=apparmor\?))
+sys.exit(3)
+
for p in open(/proc/mounts).readlines():
if p.split()[2] == securityfs and \
os.path.exists(os.path.join(p.split()[1], apparmor)):
return os.path.join(p.split()[1], apparmor)
-return False
+
+errormsg(AppArmor securityfs not mounted.)
+sys.exit(3)
def errormsg(message):
'''Prints to stderr if verbose mode is on'''
--
Kees Cook
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH] add gdm3 path to X abstraction.
Updates the X abstraction to include gdm3 path.
Author: Intrigeri intrig...@debian.org
Bug-Debian: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=660079
Signed-off-by: Kees Cook k...@ubuntu.com
Index: apparmor-debian/profiles/apparmor.d/abstractions/X
===
--- apparmor-debian.orig/profiles/apparmor.d/abstractions/X 2012-04-24
11:03:46.506994000 -0700
+++ apparmor-debian/profiles/apparmor.d/abstractions/X 2012-04-24
12:56:42.255783258 -0700
@@ -17,7 +17,7 @@
# .Xauthority files required for X connections, per user
@{HOME}/.Xauthority r,
- owner /{,var/}run/gdm/*/database r,
+ owner /{,var/}run/gdm{,3}/*/database r,
owner /{,var/}run/lightdm/authority/[0-9]* r,
# the unix socket to use to connect to the display
--
Kees Cook
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [patch 3/4] fix autodep profile construction
On Tue, Mar 27, 2012 at 12:24:53PM -0700, Steve Beattie wrote: This patch fixes a couple of issue with autodep: 1) The initial profile construction had not been adjusted to include the 'allow' or 'deny' hash prefixing the path elements. This fixes it by eliminating the path portion entirely and pushing the path based accesses to the later analysis section of code. 2) the mode of the original binary was accidentally getting reset to 0, when it was intended to initialize the audit field to 0. Acked-by: Kees Cook k...@ubuntu.com -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 1/5] Modifify regression test infrastructure to stop on failure when retainingtmpdir
On Mon, Mar 26, 2012 at 06:03:53AM -0700, John Johansen wrote: The retaining of the tmpdir is used during debugging of test failures, but currently when a test fails, the next test is run overwritting the previous tmpdir value. This is a problem even when manually running individual test shell scripts if the failure is not the last test in the script. Instead cause testing to about when retaintmpdir is true, which will cover the debugging needs for the majority of failure cases. Signed-off-by: John Johansen john.johan...@canonical.com Acked-by: Kees Cook k...@ubuntu.com -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 2/5] Fix the changehat_wrapper regression test
On Mon, Mar 26, 2012 at 06:03:54AM -0700, John Johansen wrote: The capabilities tests where failing in the changehat_wrapper test. This was because they could not the changehat_wrapper sub executable, which trying to exec a binary in the tmpdir. Specifically if the test was for syscall_ptrace. It would generate a profile with a hat for ^syscall_ptrace and attempt to execute ./syscall_ptrace. However this was failing in some situations, including when trying to debug from the tmpdir, as the syscall_XXX binary is no longer local. Instead use the fully qualified path for the hat name, and the exec path. Signed-off-by: John Johansen john.johan...@canonical.com Acked-by: Kees Cook k...@ubuntu.com -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [patch 2/6] abstract out cap and net proto generation to common/Make.rules
On Thu, Mar 22, 2012 at 10:06:09AM -0700, Steve Beattie wrote: It also sorts the resulting lists, which causes it to output differently than the before case. I did confirm that the results for the generated files used in the parser build were the same after taking the sorting into account. Okay, good. I'm still nervous that this sorting will break something, but I suppose it would be better to be more robust in this regard anyway. -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 10/11] Fix caching when used with a newer kernel with the feature directory
On Thu, Mar 08, 2012 at 01:57:44PM -0800, John Johansen wrote:
On 03/08/2012 01:46 PM, Kees Cook wrote:
On Thu, Mar 08, 2012 at 01:40:54PM -0800, John Johansen wrote:
On 03/08/2012 11:17 AM, Steve Beattie wrote:
On Thu, Mar 08, 2012 at 11:15:12AM -0800, Steve Beattie wrote:
On Wed, Mar 07, 2012 at 06:17:29AM -0800, John Johansen wrote:
On newer kernels the features directory causes the creation of a
cache/.feature file that contains newline characters. This causes the
feature comparison to fail, because get_flags_string() uses fgets
which stop reading in the feature file after the first newline.
This caches the features comparision to compare a single line of the
file against the full kernel feature directory resulting in caching
failure.
Worse this also means the cache won't get updated as the parser doesn't
change what set gets caches after the .feature file gets created.
Signed-off-by: John Johansen john.johan...@canonical.com
Acked-By: Steve Beattie sbeat...@ubuntu.com
Actually, fread() doesn't null terminate what it reads like fgets()
does. I think you'll need to address that.
How about with this additional patch on top
diff --git a/parser/parser_main.c b/parser/parser_main.c
index 67ab231..3f4789b 100644
--- a/parser/parser_main.c
+++ b/parser/parser_main.c
@@ -844,6 +844,7 @@ out:
static void get_flags_string(char **flags, char *flags_file) {
char *pos;
FILE *f = NULL;
+ size_t size;
/* abort if missing or already set */
if (!flags || *flags)
@@ -857,8 +858,10 @@ static void get_flags_string(char **flags, char
*flags_file) {
if (!*flags)
goto fail;
- if (!fread(*flags, 1, FLAGS_STRING_SIZE, f))
+ size = fread(*flags, 1, FLAGS_STRING_SIZE - 1, f);
+ if (ferror(f))
How about:
if (size 1 || ferror(f))
...
goto fail;
+ *flags[size] = 0;
Then this can't freak out.
hrmmm well it shouldn't freak out if size == 0 and that is the only value
less than 1 it
could be, but from read fread again, yeah we should be checking for 0
Ah, yeah, good point. size 0 should be fine.
--
Kees Cook
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 10/11] Fix caching when used with a newer kernel with the feature directory
On Thu, Mar 08, 2012 at 02:21:19PM -0800, John Johansen wrote:
On 03/08/2012 02:11 PM, Kees Cook wrote:
On Thu, Mar 08, 2012 at 01:57:44PM -0800, John Johansen wrote:
On 03/08/2012 01:46 PM, Kees Cook wrote:
On Thu, Mar 08, 2012 at 01:40:54PM -0800, John Johansen wrote:
On 03/08/2012 11:17 AM, Steve Beattie wrote:
On Thu, Mar 08, 2012 at 11:15:12AM -0800, Steve Beattie wrote:
On Wed, Mar 07, 2012 at 06:17:29AM -0800, John Johansen wrote:
On newer kernels the features directory causes the creation of a
cache/.feature file that contains newline characters. This causes the
feature comparison to fail, because get_flags_string() uses fgets
which stop reading in the feature file after the first newline.
This caches the features comparision to compare a single line of the
file against the full kernel feature directory resulting in caching
failure.
Worse this also means the cache won't get updated as the parser
doesn't
change what set gets caches after the .feature file gets created.
Signed-off-by: John Johansen john.johan...@canonical.com
Acked-By: Steve Beattie sbeat...@ubuntu.com
Actually, fread() doesn't null terminate what it reads like fgets()
does. I think you'll need to address that.
How about with this additional patch on top
diff --git a/parser/parser_main.c b/parser/parser_main.c
index 67ab231..3f4789b 100644
--- a/parser/parser_main.c
+++ b/parser/parser_main.c
@@ -844,6 +844,7 @@ out:
static void get_flags_string(char **flags, char *flags_file) {
char *pos;
FILE *f = NULL;
+size_t size;
/* abort if missing or already set */
if (!flags || *flags)
@@ -857,8 +858,10 @@ static void get_flags_string(char **flags, char
*flags_file) {
if (!*flags)
goto fail;
-if (!fread(*flags, 1, FLAGS_STRING_SIZE, f))
+size = fread(*flags, 1, FLAGS_STRING_SIZE - 1, f);
+if (ferror(f))
How about:
if (size 1 || ferror(f))
...
goto fail;
+*flags[size] = 0;
Then this can't freak out.
hrmmm well it shouldn't freak out if size == 0 and that is the only value
less than 1 it
could be, but from read fread again, yeah we should be checking for 0
Ah, yeah, good point. size 0 should be fine.
err its an unsigned type how about size FLAGS_STRING_SIZE
Oh, right _f_read. Ignore me entirely. If fread returns greater than
nmemb arg, there are much more serious problems. Original patch is
fine. I need lunch. :P
-Kees
--
Kees Cook
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 04/16] AppArmor: fix mapping of META_READ to audit and quiet flags
On Wed, Feb 22, 2012 at 09:10:29AM -0800, John Johansen wrote: The mapping of AA_MAY_META_READ for the allow mask was also being mapped to the audit and quiet masks. This would result in some operations being audited when the should not. This flaw was hidden by the previous audit bug which would drop some messages that where supposed to be audited. Signed-off-by: John Johansen john.johan...@canonical.com Signed-off-by: Kees Cook k...@ubuntu.com -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 08/16] AppArmor: Update dfa matching routines.
On Wed, Feb 22, 2012 at 09:10:33AM -0800, John Johansen wrote: Update aa_dfa_match so that it doesn't result in an input string being walked twice (once to get its length and another time to match) Add a single step functions aa_dfa_next Signed-off-by: John Johansen john.johan...@canonical.com Signed-off-by: Kees Cook k...@ubuntu.com -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 10/16] AppArmor: Make chroot relative the default path lookup type
On Wed, Feb 22, 2012 at 09:10:35AM -0800, John Johansen wrote: Profiles that want name lookup past the chroot to the namespace root must be marked as such, all other profiles should be chroot relative. Currently the autogenerated null (learning), and unconfined profiles are not marked as such. Make sure they are properly flagged. This should not affect behavior except for auto-generated profiles when a chroot is entered. Profiles loaded from userspace will not be affected as they provide their own value for the flag. This change does not affect mediation as it only changes the path reported by the unconfined (none mediating), an null learning profiles. Also ensure that if a profile is ever loaded with out path flags set, that it defaults to being chroot relative. Signed-off-by: John Johansen john.johan...@canonical.com Signed-off-by: Kees Cook k...@ubuntu.com -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 11/16] AppArmor: Add ability to load extended policy
On Wed, Feb 22, 2012 at 09:10:36AM -0800, John Johansen wrote: Add the base support for the new policy extensions. This does not bring any additional functionality, or change current semantics. Signed-off-by: John Johansen john.johan...@canonical.com Signed-off-by: Kees Cook k...@ubuntu.com -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 13/16] AppArmor: Add profile introspection file to interface
On Wed, Feb 22, 2012 at 09:10:38AM -0800, John Johansen wrote: Add the dynamic profiles file to the interace, to allow load policy introspection. Signed-off-by: John Johansen john.johan...@canonical.com I thought we were going to hold off on this until we had the real profile introspection tree, but I'm not very opposed to it, since it would be extremely nice to have this in upstream. Signed-off-by: Kees Cook k...@ubuntu.com -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 14/16] AppArmor: Add the ability to mediate mount
On Wed, Feb 22, 2012 at 09:10:39AM -0800, John Johansen wrote: Add the ability for apparmor to do mediation of mount operations. Signed-off-by: John Johansen john.johan...@canonical.com Signed-off-by: Kees Cook k...@ubuntu.com -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 15/16] AppArmor: Add mount information to apparmorfs
On Wed, Feb 22, 2012 at 09:10:40AM -0800, John Johansen wrote: Update the apparmorfs introspection interface to reflect that mount rules are available. As part of this change the namespace entry from a binary file to a directory so it can store interface information for operations that affect the namespace like pivot_root. Signed-off-by: John Johansen john.johan...@canonical.com Signed-off-by: Kees Cook k...@ubuntu.com -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 16/16] AppArmor: Add mediation of chroot
On Wed, Feb 22, 2012 at 09:10:41AM -0800, John Johansen wrote: Add support for chroot rules, which allow a profile to specify where a chroot can be made. chroot /tmp/example, Signed-off-by: John Johansen john.johan...@canonical.com Signed-off-by: Kees Cook k...@ubuntu.com -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 03/16] AppArmor: Fix underflow in xindex calculation
On Wed, Feb 22, 2012 at 12:44:54PM -0800, John Johansen wrote:
On 02/22/2012 12:27 PM, Kees Cook wrote:
On Wed, Feb 22, 2012 at 09:10:28AM -0800, John Johansen wrote:
If the xindex value stored in the accept tables is 0, the extraction of
that value will result in an underflow (0 - 4).
In properly compiled policy this should not happen for file rules but
it may be possible for other rule types in the future.
To exploit this underflow a user would have to be able to load a corrupt
policy, which requires CAP_MAC_ADMIN, overwrite system policy in kernel
memory or know of a compiler error resulting in the flaw being present
for loaded policy (no such flaw is known at this time).
Signed-off-by: John Johansen john.johan...@canonical.com
---
security/apparmor/include/file.h |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/security/apparmor/include/file.h
b/security/apparmor/include/file.h
index ab8c6d8..f98fd47 100644
--- a/security/apparmor/include/file.h
+++ b/security/apparmor/include/file.h
@@ -117,7 +117,7 @@ static inline u16 dfa_map_xindex(u16 mask)
index |= AA_X_NAME;
} else if (old_index == 3) {
index |= AA_X_NAME | AA_X_CHILD;
- } else {
+ } else if (old_index) {
index |= AA_X_TABLE;
index |= old_index - 4;
}
What about the cases where old_index 4, but != 0?
look above cases 1, 2, and 3 are covered by the if blocks
eg.
} else if (old_index == 3) {
index |= AA_X_NAME | AA_X_CHILD;
Ah, right. Okay. Missed that bit. Thanks!
Signed-off-by: Kees Cook k...@ubuntu.com
--
Kees Cook
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 02/20] AppArmor: add initial features directory to securityfs
Hi John,
On Wed, Feb 22, 2012 at 09:22:45AM -0800, John Johansen wrote:
+static struct aa_fs_entry aa_fs_entry_features[] = {
+ AA_FS_DIR(domain, aa_fs_entry_domain),
+ AA_FS_FILE_BOOLEAN(namespaces,1),
If namespaces is going to change into a directory, perhaps just leave
it out for now?
-Kees
--
Kees Cook
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 1/2] Fix an error in tree normalization that can result in an infinite loop
On Thu, Feb 16, 2012 at 08:26:09AM -0800, John Johansen wrote:
Tree normailization tries to reshape expr tree into a normal from like
|1 |1
/ \ / \
|2 T - a |2
/ \ / \
|3 cb |3
/ \ / \
a bc T
which requires rotating the alt and cat nodes, at the same time it
also tries to rotate epsnods to the same side as alt and cat nodes.
However there is a bug that results in the epsnode flipping and
node rotation reverting each others work. If the tree is of the
follow form this will result in an infinite loop.
|1
/ \
e2 |3
/ \
e3 C
epsnode flipping is not supposed to take precedence over node rotation
and there is even a test to check for an alt or cat node, unfortunately
it is wrong resulting in unnecessary swapping and the possibility for
the above infinite loop
Signed-off-by: John Johansen john.johan...@canonical.com
---
parser/libapparmor_re/expr-tree.cc |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/parser/libapparmor_re/expr-tree.cc
b/parser/libapparmor_re/expr-tree.cc
index e9a1275..b502d54 100644
--- a/parser/libapparmor_re/expr-tree.cc
+++ b/parser/libapparmor_re/expr-tree.cc
@@ -189,7 +189,7 @@ void normalize_tree(Node *t, int dir)
for (;;) {
if ((epsnode == t-child[dir])
(epsnode != t-child[!dir])
- dynamic_castTwoChildNode *(t)) {
+ !dynamic_castTwoChildNode *(t-child[!dir])) {
// (E | a) - (a | E)
// Ea - aE
Node *c = t-child[dir];
I don't understand this area well enough to be comfortable to ACK it, but
if you say it's needed, that's good enough for me. ;) That said, is there
a simple test-case that can be used to show the before/after of this
change?
-Kees
--
Kees Cook
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 2/2] Default profiles to be chroot relative
On Thu, Feb 16, 2012 at 08:26:10AM -0800, John Johansen wrote:
Due to changes in path looks and the work going forward default profiles
to resolve relative to the chroot instead of the namespace.
This will only affect profiles that are used on tasks within a chroot.
For now it will be possible to get the old default namespace relative
behavior by passing the namespace_relative flag to the profile
eg.
profile /example (namespace_relative) { .. }
Signed-off-by: John Johansen john.johan...@canonical.com
Acked-by: Kees Cook k...@ubuntu.com
--
Kees Cook
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [Tails-dev] AppArmor profiles in Debian
Hi, On Thu, Feb 16, 2012 at 12:19:49AM +0100, intrigeri wrote: Kees Cook wrote (14 Feb 2012 19:59:45 GMT) : Ubuntu's evince and isc-dhcp-client profiles are very well tested at this point. I think it should be easy to move those into Debian if they're not there already. Right, I'm glad I've not encountered any single problem with these profiles. I'd like to thank everyone involved in their initial development, adaptation to Ubuntu, and testing. \o/ Glad it's been treating you well! :) 3. some low-hanging fruits from Ubuntu's Supported profiles in main list [1] that, I guess, you know very well: apache2, libvirt. [0] https://tails.boum.org/ [1] https://wiki.ubuntu.com/SecurityTeam/KnowledgeBase/AppArmorProfiles I think just encouraging all the maintainers to pull in the Ubuntu patches for AppArmor profiles is the best way to start. Several packages have already done this (e.g. bind9). I think this will be totally relevant at some point, but I beg to disagree this is a good way to start. Here's why. Very well tested in Ubuntu is great, but sometimes is not always enough to be usable at all in Debian. The gdm vs. gdm3 bug in the X abstraction I've just reported against the apparmor package (sorry, being offline, I've no bug number yet) indicates that even trivial Debian-specific problems, in very common apparmor abstractions, were not detected yet. Therefore, I think at least some basic manual testing is due before we ask a Debian maintainer to ship any profile with their packages. Right, absolutely. I didn't mean to imply that we should just throw everything possible at the maintainers. :) To get things started, I have started using some of the profiles shipped in the apparmor-profiles packages; but none of the There's documentation in the Ubuntu wiki on transitioning profiles from the apparmor-profiles package to the individual packages; https://help.ubuntu.com/community/AppArmor#Migrating_an_apparmor-profiles_profile_to_a_package Thanks. I don't understand exactly what specific profiles we would want to migrate from apparmor-profiles to individual packages. Can you please elaborate? I didn't have any in mind, but I just wanted to point out the procedure we've documented for doing it in the past. For example, if we wanted to move the ping, traceroute, etc stuff to their packages, we'd follow that process. aforementioned software is supported, so I've extracted the profiles from the following Ubuntu packages, and have been running them in enforcing mode on my main Debian (sid) system: * firefox 11.0~b2+build1-0ubuntu1 The firefox profile remains tricky as far as enabling by default. I agree it should probably not be enabled by default in Debian: the iceweasel beast is so huge, and extensions scope so large, that a working-for-everyone profile would probably be liberal to a point it would not be very different from unconfined. However, in the context of a Live system such as Tails, where we control quite well the deployed iceweasel setup, I believe an AppArmor profile makes sense. I guess many other kinds of managed deployements share this property, and in such deployements, supporting tons of random add-ons is probably not wished by the system administrators, if allowed at all by site policies. I would love to see such a minimal profile shipped, although disabled or in complain mode by default, in Debian. Do you think it makes sense / is doable? Yeah, absolutely. This is effectively what's done in Ubuntu. Local configuration or a derivative can enable it, etc. * isc-dhcp 4.1.1-P1-17ubuntu12 (client only) Yes, very handy. Order of operations is important here, though. The profile must load before any network interface. In Ubuntu, this is being done via upstart jobs -- I haven't tested it with sysvinit. Ah, so this was the meaning of /etc/apparmor/init/network-interface-security/sbin.dhclient being a symlink to /etc/apparmor.d/sbin.dhclient in the Ubuntu patch. Makes sense. Yeah, it's a bit weird, but that's the least of a few evils. With sysvinit, I think that Required-Start: $remote_fs in the apparmor initscript will prevent AppArmor profiles to be loaded before the networking initscript starts. At least on my system, insserv ordered the scripts this way. Is this dependency present only to support the /usr-on-NFS usecase? I suspect so, yes. It probably means that apparmor will either need to have 2 init files (early and late), or have its init modified not to require /usr. Both we done at various times before in Ubuntu, so it shouldn't be much work to make it happen. The desktop + NetworkManager usecase works fine, though, but this is mostly by chance, and the network-manager initscript should probably be added a dependency on AppArmor. One of the major stumbling blocks right now is that the legacy interface patch is not carried by the Debian kernel
Re: [apparmor] [PATCH 07/13] Make expressing all capabilities easier
On Wed, Feb 15, 2012 at 12:01:42PM +0100, Christian Boltz wrote: Am Dienstag, 14. Februar 2012 schrieb John Johansen: Allow the capability rule to be bare to represent all capabilities similar to how network, and other rule types work. capability, I hope not too many people use this ;-) but nevertheless here's the patch to update apparmor.vim to support it. Using just capability will be marked in the dangerous capability color. Additionally, the patch removes the (already commented out) code for set capability. Acked-by: Kees Cook k...@ubuntu.com -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH] aa_find_mountpoint man page format fix
This is a trivial manpage fix that makes pod2man stop yelling at me. Index: apparmor-debian/libraries/libapparmor/doc/aa_find_mountpoint.pod === --- apparmor-debian.orig/libraries/libapparmor/doc/aa_find_mountpoint.pod 2012-02-09 15:00:40.299385520 -0800 +++ apparmor-debian/libraries/libapparmor/doc/aa_find_mountpoint.pod 2012-02-09 15:01:39.984184575 -0800 @@ -57,10 +57,10 @@ =head1 ERRORS -=over 4 - Baa_is_enabled +=over 4 + =item BENOSYS AppArmor extensions to the system are not available. @@ -86,9 +86,12 @@ +Did not have sufficient permissions to determine if AppArmor is enabled. +=back Baa_find_mountpoint +=over 4 + =item BENOMEM Insufficient memory was available. -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH] aa_find_mountpoint man page format fix
On Wed, Feb 15, 2012 at 04:17:56PM -0800, Steve Beattie wrote: On Wed, Feb 15, 2012 at 02:17:16PM -0800, Kees Cook wrote: This is a trivial manpage fix that makes pod2man stop yelling at me. Acked-By: Steve Beattie sbeat...@ubuntu.com for both trunk and 2.7. I was wondering what yelling you were referring to, as a local build without your patch didn't yell at me. It turns out we don't pass --stderr to pod2man; once that's added, the yelling starts. :-) Ah! Yes, this was from the Debian builds I was doing. Attached is a patch (against trunk with your patch applied) that adds it, as well as fixes a few other minor text issues I noticed while peeking at your patch (in one case I re-flowed a paragraph, forgetting that it would make review difficult, the change in question is just s/errno/errno(3)/). Nice! Please feel free to push both together... Acked-by: Kees Cook k...@ubuntu.com -Kees -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 01/13] Rework the definition of ID and POST_VAR_ID to use a define for the charset
On Tue, Feb 14, 2012 at 09:32:23AM -0800, John Johansen wrote: ID and POST_VAR_ID define a set of characters that is reused, pull this out to avoid making mistakes when updating the character set. Signed-off-by: John Johansen john.johan...@canonical.com Acked-by: Kees Cook k...@ubuntu.com -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 07/13] Make expressing all capabilities easier
On Tue, Feb 14, 2012 at 09:32:29AM -0800, John Johansen wrote:
Allow the capability rule to be bare to represent all capabilities similar
to how network, and other rule types work.
capability,
Signed-off-by: John Johansen john.johan...@canonical.com
---
parser/parser_yacc.y| 18 +++---
parser/tst/simple_tests/capability/bad_3.sd |9 +
parser/tst/simple_tests/capability/bad_4.sd |9 +
parser/tst/simple_tests/capability/ok3.sd |9 +
4 files changed, 34 insertions(+), 11 deletions(-)
create mode 100644 parser/tst/simple_tests/capability/bad_3.sd
create mode 100644 parser/tst/simple_tests/capability/bad_4.sd
create mode 100644 parser/tst/simple_tests/capability/ok3.sd
diff --git a/parser/parser_yacc.y b/parser/parser_yacc.y
index 2a4fa5d..fff7e23 100644
--- a/parser/parser_yacc.y
+++ b/parser/parser_yacc.y
@@ -1057,10 +1057,15 @@ set_caps: TOK_SET TOK_CAPABILITY caps
TOK_END_OF_RULE
capability: TOK_CAPABILITY caps TOK_END_OF_RULE
{
- $$ = $2;
+ if ($2 == 0) {
+ /* bare capability keyword - set all caps */
+ $$ = 0x;
Should this be something more dynamic, using _LINUX_CAPABILITY_U32S_3 or
something similar to detect size, or is it sufficient to assume unsigned
long now?
If it's safe, then:
Acked-by: Kees Cook k...@ubuntu.com
:)
+ } else
+ $$ = $2;
};
-caps: caps TOK_ID
+caps: { /* nothing */ $$ = 0; }
+ | caps TOK_ID
{
int cap = name_to_capability($2);
if (cap == -1)
@@ -1069,15 +1074,6 @@ caps: caps TOK_ID
$$ = $1 | CAP_TO_MASK(cap);
}
-caps: TOK_ID
- {
- int cap = name_to_capability($1);
- if (cap == -1)
- yyerror(_(Invalid capability %s.), $1);
- free($1);
- $$ = CAP_TO_MASK(cap);
- };
-
%%
#define MAXBUFSIZE 4096
diff --git a/parser/tst/simple_tests/capability/bad_3.sd
b/parser/tst/simple_tests/capability/bad_3.sd
new file mode 100644
index 000..00e4f4b
--- /dev/null
+++ b/parser/tst/simple_tests/capability/bad_3.sd
@@ -0,0 +1,9 @@
+#
+#=DESCRIPTION fail CAP_XXX syntax.
+#=EXRESULT FAIL
+# vim:syntax=subdomain
+# Last Modified: Sun Apr 17 19:44:44 2005
+#
+/does/not/exist {
+ capability chown CAP_CHOWN,
+}
diff --git a/parser/tst/simple_tests/capability/bad_4.sd
b/parser/tst/simple_tests/capability/bad_4.sd
new file mode 100644
index 000..502c74a
--- /dev/null
+++ b/parser/tst/simple_tests/capability/bad_4.sd
@@ -0,0 +1,9 @@
+#
+#=DESCRIPTION fail unknown keyword
+#=EXRESULT FAIL
+# vim:syntax=subdomain
+# Last Modified: Sun Apr 17 19:44:44 2005
+#
+/does/not/exist {
+ capability chown foobar,
+}
diff --git a/parser/tst/simple_tests/capability/ok3.sd
b/parser/tst/simple_tests/capability/ok3.sd
new file mode 100644
index 000..454b96c
--- /dev/null
+++ b/parser/tst/simple_tests/capability/ok3.sd
@@ -0,0 +1,9 @@
+#
+#=DESCRIPTION validate some uses of capabilties.
+#=EXRESULT PASS
+# vim:syntax=subdomain
+# Last Modified: Sun Apr 17 19:44:44 2005
+#
+/does/not/exist {
+ capability,
+}
--
1.7.9
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
--
Kees Cook
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 08/13] Remove setting of capabilities from the syntax
On Tue, Feb 14, 2012 at 09:32:30AM -0800, John Johansen wrote: The ability to set capabilities from a profile has been removed from the kernel for several releases. Remove it from the parser as well. Signed-off-by: John Johansen john.johan...@canonical.com Acked-by: Kees Cook k...@ubuntu.com -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 09/13] Allow the 'file' keyword to be optionally used on file rules.
On Tue, Feb 14, 2012 at 09:32:31AM -0800, John Johansen wrote: Add the optional 'file' keyword to the language/grammer. The main reason for doing this is to support false token injection. Which is needed to move towards the parser being broken out into an api that can be used to parse individual rule types, separate from parsing the whole file. Since we are adding the token to the grammar expose it to userspace with the 'file' keyword. While not needed it helps bring consistency, as all the other rule types start with a keyword (capability, network, rlimit, ...). Also allow the bare keyword to be used to represent allowing all file operations, just as with network and capability. Domain transitions are defaulted to ix. Thus file, is equivalent to /** rwlkmix, Signed-off-by: John Johansen john.johan...@canonical.com Oh, very cool. I like this. :) Acked-by: Kees Cook k...@ubuntu.com -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 10/13] Make value_list generic so it can be reused.
On Tue, Feb 14, 2012 at 09:32:32AM -0800, John Johansen wrote: value_list can be reused by conditionals and list values, so pull it out and abstract it some more. Signed-off-by: John Johansen john.johan...@canonical.com Acked-by: Kees Cook k...@ubuntu.com -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 2/3] Track full permission set through all stages of DFA construction.
On Tue, Feb 14, 2012 at 09:57:27AM -0800, John Johansen wrote: Previously permission information was thrown away early and permissions where packed to their CHFA form at the start of DFA construction. Because of this permissions hashing to setup the initial DFA partitions was required as x transition conflicts, etc. could not be resolved. Move the mapping of permissions to CHFA construction, and track the full permission set through DFA construction. This allows removal of the perm_hashing hack, which prevented a full minimization from happening in some DFAs. It also could result in x conflicts not being correctly detected, and deny rules not being fully applied in some situations. Does this mean the big x collision test is useless now? Signed-off-by: John Johansen john.johan...@canonical.com Acked-by: Kees Cook k...@ubuntu.com @@ -462,6 +465,7 @@ void DFA::minimize(dfaflags_t flags) partitions.size() \tinit partitions.size() (accept accept_count )\r; } + /* perm_map is no longer needed so free the memory it is using. * Don't remove - doing it manually here helps reduce peak memory usage. */ The prior patch drops this whitespace. Probably best to decide one way or the other. :) -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 04/13] Instead of using a special flags= token and keyword use TOK_CONDID
On Tue, Feb 14, 2012 at 12:51:55PM -0800, John Johansen wrote:
On 02/14/2012 11:10 AM, Kees Cook wrote:
On Tue, Feb 14, 2012 at 09:32:26AM -0800, John Johansen wrote:
Signed-off-by: John Johansen john.johan...@canonical.com
---
parser/parser_lex.l | 31 +--
parser/parser_yacc.y |8 +++-
2 files changed, 20 insertions(+), 19 deletions(-)
diff --git a/parser/parser_lex.l b/parser/parser_lex.l
index bfcbd57..8f549c8 100644
--- a/parser/parser_lex.l
+++ b/parser/parser_lex.l
@@ -192,7 +192,6 @@ QUOTED_ID \{ALLOWED_QUOTED_ID}*\
IP{NUMBER}\.{NUMBER}\.{NUMBER}\.{NUMBER}
-FLAGS flags{WS}*=?{WS}*
HAT hat{WS}*
PROFILE profile{WS}*
KEYWORD [[:alpha:]_]+
@@ -254,6 +253,19 @@ LT_EQUAL =
if ( !YY_CURRENT_BUFFER ) yyterminate();
}
+{VARIABLE_NAME}/{WS}*={WS}* {
+ /* we match to the = in the lexer so that
+ * can switch scanner state. By the time
+ * the parser see the = it may be to late
+ * as bison may have requested the next
+ * token from the scanner
+ */
+ PDEBUG(conditional %s=\n, yytext);
+ yylval.id = processid(yytext, yyleng);
+ yy_push_state(EXTCOND_MODE);
+ return TOK_CONDID;
+ }
+
Why the relocation of this chunk? Seems better to keep it near the other
VARIABLE_NAME use...
Originally I did, but it got lifted and state shared as part of the mount
patch, and actually had other modifications done as well. I pulled most
of those changes back into the original.
We could always rework it into a couple of patches but I guess I am just
getting lazy
Hehe, no worries. Just curious, it's not really a blocker from my
perspective.
Acked-by: Kees Cook k...@ubuntu.com
--
Kees Cook
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH] private-files should disallow writing to .pki so files
On Wed, Jan 04, 2012 at 10:43:31AM -0600, Jamie Strandboge wrote:
The private-files abstraction should explicitly deny writes to this
directory. Since nss also stores certificates, etc in this directory,
should use something like:
audit deny @{HOME}/.pki/nssdb/*.so{,.[0-9]*} wl,
Attached is a patch to achieve this (and fixes 2 spelling errors).
Acked-by: Kees Cook k...@ubuntu.com
--
Kees Cook
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 1/9] Move rlimit start condition and rules up to be with other start conditions.
On Tue, Dec 27, 2011 at 07:01:44PM -0800, John Johansen wrote: The rlimit start condition was separating different rules of the base set making the lexer grammer harder to read than necessary. Signed-off-by: John Johansen john.johan...@canonical.com Acked-by: Kees Cook k...@ubuntu.com -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 3/9] Update the flex scanner to use a stack for its start conditions
On Tue, Dec 27, 2011 at 07:01:46PM -0800, John Johansen wrote: This is the first step in reducing the number of shared rules between the different start conditions. Signed-off-by: John Johansen john.johan...@canonical.com Acked-by: Kees Cook k...@ubuntu.com -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 6/9] Update parsing of the 'hat' and 'profile' keyword to use SUB_NAME
On Tue, Dec 27, 2011 at 07:01:49PM -0800, John Johansen wrote: Change how we handle the parsing of the hat and profile keywords this allows us to get rid of the SUB_NAME2 start condition because the the whitespace that is allowed by these rules are now consumed by matching the keyword Signed-off-by: John Johansen john.johan...@canonical.com Acked-by: Kees Cook k...@ubuntu.com -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [RFC] features directory for version/capability information
Based on some of the initial design[1] discussions, here is a stab at the
simple static portion of the new apparmorfs interface.
[1] https://lists.ubuntu.com/archives/apparmor/2010-November/000491.html
Signed-off-by: Kees Cook k...@ubuntu.com
---
security/apparmor/apparmorfs.c | 114 +
1 file changed, 104 insertions(+), 10 deletions(-)
---
diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index 0848292..8990316 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -146,11 +146,67 @@ static const struct file_operations aa_fs_profile_remove
= {
static struct dentry *aa_fs_dentry __initdata;
-static void __init aafs_remove(const char *name)
+struct aa_fs_file {
+ char *name;
+ char *value;
+ const struct file_operations *file_ops;
+};
+
+struct aa_fs_dir {
+ const char *name;
+ struct dentry *dentry;
+ struct aa_fs_file *files;
+};
+
+static int aa_fs_seq_show(struct seq_file *seq, void *v)
+{
+ struct aa_fs_file *fs_file = seq-private;
+
+ if (fs_file)
+ seq_printf(seq, %s\n, fs_file-value);
+
+ return 0;
+}
+
+static int aa_fs_seq_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, aa_fs_seq_show, inode-i_private);
+}
+
+static const struct file_operations aa_fs_seq_file_ops = {
+ .owner = THIS_MODULE,
+ .open = aa_fs_seq_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release= single_release,
+};
+
+static struct aa_fs_file aa_fs_features_files[] = {
+ { capability, 2.0, aa_fs_seq_file_ops },
+ { change_hat, 1.5, aa_fs_seq_file_ops },
+ { change_hatv,1.0, aa_fs_seq_file_ops },
+ { change_onexec, 1.0, aa_fs_seq_file_ops },
+ { change_profile, 1.1, aa_fs_seq_file_ops },
+ { file, 3.1, aa_fs_seq_file_ops },
+ { namespaces, 1.1, aa_fs_seq_file_ops },
+ { network,1.0, aa_fs_seq_file_ops },
+ { rlimit, 1.1, aa_fs_seq_file_ops },
+ { NULL, NULL, NULL },
+};
+
+static struct aa_fs_dir aa_fs_dirs[] = {
+ { features, NULL, aa_fs_features_files },
+ { NULL, NULL, NULL }
+};
+
+static void __init aafs_remove(const char *name, struct dentry *dir_dentry)
{
struct dentry *dentry;
- dentry = lookup_one_len(name, aa_fs_dentry, strlen(name));
+ if (!dir_dentry)
+ return;
+
+ dentry = lookup_one_len(name, dir_dentry, strlen(name));
if (!IS_ERR(dentry)) {
securityfs_remove(dentry);
dput(dentry);
@@ -166,12 +222,14 @@ static void __init aafs_remove(const char *name)
* Used aafs_remove to remove entries created with this fn.
*/
static int __init aafs_create(const char *name, int mask,
+ struct dentry *dir_dentry,
+ void *data,
const struct file_operations *fops)
{
struct dentry *dentry;
- dentry = securityfs_create_file(name, S_IFREG | mask, aa_fs_dentry,
- NULL, fops);
+ dentry = securityfs_create_file(name, S_IFREG | mask, dir_dentry,
+ data, fops);
return IS_ERR(dentry) ? PTR_ERR(dentry) : 0;
}
@@ -184,9 +242,21 @@ static int __init aafs_create(const char *name, int mask,
void __init aa_destroy_aafs(void)
{
if (aa_fs_dentry) {
- aafs_remove(.remove);
- aafs_remove(.replace);
- aafs_remove(.load);
+ struct aa_fs_dir *fs_dir;
+
+ aafs_remove(.remove, aa_fs_dentry);
+ aafs_remove(.replace, aa_fs_dentry);
+ aafs_remove(.load, aa_fs_dentry);
+
+ for (fs_dir = aa_fs_dirs; fs_dir-name; ++fs_dir) {
+ struct aa_fs_file *fs_file;
+
+ for (fs_file = fs_dir-files; fs_file-name; ++fs_file)
+ aafs_remove(fs_file-name, fs_dir-dentry);
+
+ aafs_remove(fs_dir-name, aa_fs_dentry);
+ fs_dir-dentry = NULL;
+ }
securityfs_remove(aa_fs_dentry);
aa_fs_dentry = NULL;
@@ -203,6 +273,7 @@ void __init aa_destroy_aafs(void)
int __init aa_create_aafs(void)
{
int error;
+ struct aa_fs_dir *fs_dir;
if (!apparmor_initialized)
return 0;
@@ -219,16 +290,39 @@ int __init aa_create_aafs(void)
goto error;
}
- error = aafs_create(.load, 0640, aa_fs_profile_load);
+ error = aafs_create(.load, 0640, aa_fs_dentry, NULL,
+ aa_fs_profile_load);
if (error)
goto error;
- error = aafs_create(.replace, 0640
Re: [apparmor] Minimal apparmor profile
Hi Alex,
On Fri, Dec 09, 2011 at 01:11:41PM -0500, Alex Coventry wrote:
Hi, does anyone have the minimal profile necessary to allow a
gcc-compiled hello-world program to run on ubuntu?
It seems you've already found this, but I'd start with:
/path/to/hello {
#include abstractions/base
}
All that is really needed for hello-world is the loader and libc, though.
Alternatively, is there a quick way to reload a single profile, without
restarting apparmor? It would be pretty easy to figure the minimal
ruleset out by sucessively trimming entries from abstractions/base,
given that.
sudo apparmor_parser -r /etc/apparmor.d/name.of.profile.file
Also, is there an apparmor rule allowing the prctl syscall?
prctl() is not mediated by apparmor.
-Kees
--
Kees Cook
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [patch] libapparmor: add log-parsing support for encoded comm strings
On Wed, Nov 30, 2011 at 10:21:50AM -0800, Steve Beattie wrote: While trying to track down the source of the problem for https://bugs.launchpad.net/apparmor/+bug/897957/ I discovered that the libapparmor log parsing library doesn't take into account comm entries that have been hex-encoded; these occur when the binary path name includes a space or other character that needs encoding. The attached patch fixes the issue as well as adding a testcase that demonstrates the issue. Acked-by: Kees Cook k...@ubuntu.com -- Kees Cook -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
