Re: [apparmor] [PATCH 1/3] apparmor: Add support for attaching profiles via xattr, presence and value

[Seth Arnold]

On Thu, Feb 08, 2018 at 04:04:37PM -0800, John Johansen wrote:
> > If this step fails before completion, the xattrs array may have some
> > entries that weren't properly initialized; I suspect the free operation
> > will cause serious trouble in this case.
> > 
> yep we can switch the kmalloc_array for a kcalloc, and that will fix it
> 

Ah I like the sound of that better than zeroing the arrays manually.

With this change,

Acked-by: Seth Arnold 

Thanks


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

Re: [apparmor] [PATCH 1/3] apparmor: Add support for attaching profiles via xattr, presence and value

[John Johansen]

On 02/08/2018 02:07 PM, Seth Arnold wrote:
> Hello,
> 
> On Thu, Feb 08, 2018 at 12:37:19PM -0800, John Johansen wrote:
>> +static bool unpack_xattrs(struct aa_ext *e, struct aa_profile *profile)
>> +{
>> +void *pos = e->pos;
>> +
>> +if (unpack_nameX(e, AA_STRUCT, "xattrs")) {
>> +int i, size;
>> +
>> +size = unpack_array(e, NULL);
>> +profile->xattr_count = size;
>> +profile->xattrs = kmalloc_array(size, sizeof(char *),
>> +GFP_KERNEL);
>> +if (!profile->xattrs)
>> +goto fail;
>> +for (i = 0; i < size; i++) {
>> +if (!unpack_strdup(e, >xattrs[i], NULL))
>> +goto fail;
> 
> If this step fails before completion, the xattrs array may have some
> entries that weren't properly initialized; I suspect the free operation
> will cause serious trouble in this case.
> 
yep we can switch the kmalloc_array for a kcalloc, and that will fix it

thanks

>> +}
>> +if (!unpack_nameX(e, AA_ARRAYEND, NULL))
>> +goto fail;
>> +if (!unpack_nameX(e, AA_STRUCTEND, NULL))
>> +goto fail;
>> +}
>> +
>> +if (unpack_nameX(e, AA_STRUCT, "xattr_values")) {
>> +int i, size;
>> +
>> +size = unpack_array(e, NULL);
>> +
>> +/* Must be the same number of xattr values as xattrs */
>> +if (size != profile->xattr_count)
>> +goto fail;
>> +
>> +profile->xattr_lens = kmalloc_array(size, sizeof(size_t),
>> +GFP_KERNEL);
>> +if (!profile->xattr_lens)
>> +goto fail;
>> +
>> +profile->xattr_values = kmalloc_array(size, sizeof(char *),
>> +  GFP_KERNEL);
> 
> Same thing here with the xattr_lens and xattr_values arrays.
> 
>> +if (!profile->xattr_values)
>> +goto fail;
>> +
>> +for (i = 0; i < size; i++) {
>> +profile->xattr_lens[i] = unpack_blob(e,
>> +  >xattr_values[i], NULL);
>> +profile->xattr_values[i] =
>> +kvmemdup(profile->xattr_values[i],
>> + profile->xattr_lens[i]);
>> +}
>> +
>> +if (!unpack_nameX(e, AA_ARRAYEND, NULL))
>> +goto fail;
>> +if (!unpack_nameX(e, AA_STRUCTEND, NULL))
>> +goto fail;
>> +}
>> +return 1;
>> +
>> +fail:
>> +e->pos = pos;
>> +return 0;
>> +}
> 
> Thanks
> 
> 
> 


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

Re: [apparmor] [PATCH 2/3] apparmor: convert attaching profiles via xattrs to use, dfa matching

[Seth Arnold]

On Thu, Feb 08, 2018 at 12:38:57PM -0800, John Johansen wrote:
> This converts profile attachment based on xattrs to a fixed extended
> conditional using dfa matching.
> 
> This has a couple of advantages
> - pattern matching can be used for the xattr match
> 
> - xattrs can be optional for an attachment or marked as required
> 
> - the xattr attachment conditional will be able to be combined with
>   other extended conditionals when the flexible extended conditional
>   work lands.
> 
> The xattr fixed extended conditional is appended to the xmatch
> conditional. If an xattr attachment is specified the profile xmatch
> will be generated regardless of whether there is a pattern match on
> the executable name.
> 
> Signed-off-by: John Johansen 

Acked-by: Seth Arnold 

Thanks

> ---
>  security/apparmor/apparmorfs.c |  5 
>  security/apparmor/domain.c | 52 
> ++
>  security/apparmor/include/policy.h |  2 --
>  security/apparmor/policy.c |  6 +
>  security/apparmor/policy_unpack.c  | 32 ---
>  5 files changed, 42 insertions(+), 55 deletions(-)
> 
> diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
> index 1e63ff2e5b85..35e6b240fb14 100644
> --- a/security/apparmor/apparmorfs.c
> +++ b/security/apparmor/apparmorfs.c
> @@ -2147,6 +2147,10 @@ static struct aa_sfs_entry aa_sfs_entry_signal[] = {
>   { }
>  };
>  
> +static struct aa_sfs_entry aa_sfs_entry_attach[] = {
> + AA_SFS_FILE_BOOLEAN("xattr", 1),
> + { }
> +};
>  static struct aa_sfs_entry aa_sfs_entry_domain[] = {
>   AA_SFS_FILE_BOOLEAN("change_hat",   1),
>   AA_SFS_FILE_BOOLEAN("change_hatv",  1),
> @@ -2155,6 +2159,7 @@ static struct aa_sfs_entry aa_sfs_entry_domain[] = {
>   AA_SFS_FILE_BOOLEAN("stack",1),
>   AA_SFS_FILE_BOOLEAN("fix_binfmt_elf_mmap",  1),
>   AA_SFS_FILE_BOOLEAN("post_nnp_subset",  1),
> + AA_SFS_DIR("attach_conditions", aa_sfs_entry_attach),
>   AA_SFS_FILE_STRING("version", "1.2"),
>   { }
>  };
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index ca8353f3e7a0..9651e18cbae5 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -306,11 +306,12 @@ static int change_profile_perms(struct aa_profile 
> *profile,
>   * aa_xattrs_match - check whether a file matches the xattrs defined in 
> profile
>   * @bprm: binprm struct for the process to validate
>   * @profile: profile to match against (NOT NULL)
> + * @state: state to start match in
>   *
>   * Returns: number of extended attributes that matched, or < 0 on error
>   */
>  static int aa_xattrs_match(const struct linux_binprm *bprm,
> -struct aa_profile *profile)
> +struct aa_profile *profile, unsigned int state)
>  {
>   int i;
>   size_t size;
> @@ -321,27 +322,40 @@ static int aa_xattrs_match(const struct linux_binprm 
> *bprm,
>   if (!bprm || !profile->xattr_count)
>   return 0;
>  
> + /* transition from exec match to xattr set */
> + state = aa_dfa_null_transition(profile->xmatch, state);
> +
>   d = bprm->file->f_path.dentry;
>  
>   for (i = 0; i < profile->xattr_count; i++) {
>   size = vfs_getxattr_alloc(d, profile->xattrs[i], ,
> value_size, GFP_KERNEL);
> - if (size < 0) {
> - ret = -EINVAL;
> - goto out;
> - }
> + if (size >= 0) {
> + u32 perm;
>  
> - /* Check the xattr value, not just presence */
> - if (profile->xattr_lens[i]) {
> - if (profile->xattr_lens[i] != size) {
> + /* Check the xattr value, not just presence */
> + state = aa_dfa_match_len(profile->xmatch, state, value,
> +  size);
> + perm = dfa_user_allow(profile->xmatch, state);
> + if (!(perm & MAY_EXEC)) {
>   ret = -EINVAL;
>   goto out;
>   }
> -
> - if (memcmp(value, profile->xattr_values[i], size)) {
> + }
> + /* transition to next element */
> + state = aa_dfa_null_transition(profile->xmatch, state);
> + if (size < 0) {
> + /*
> +  * No xattr match, so verify if transition to
> +  * next element was valid. IFF so the xattr
> +  * was optional.
> +  */
> + if (!state) {
>   ret = -EINVAL;
>   goto out;
>   }
> + /* don't count missing optional xattr as 

Re: [apparmor] [PATCH 1/3] apparmor: Add support for attaching profiles via xattr, presence and value

[Seth Arnold]

Hello,

On Thu, Feb 08, 2018 at 12:37:19PM -0800, John Johansen wrote:
> +static bool unpack_xattrs(struct aa_ext *e, struct aa_profile *profile)
> +{
> + void *pos = e->pos;
> +
> + if (unpack_nameX(e, AA_STRUCT, "xattrs")) {
> + int i, size;
> +
> + size = unpack_array(e, NULL);
> + profile->xattr_count = size;
> + profile->xattrs = kmalloc_array(size, sizeof(char *),
> + GFP_KERNEL);
> + if (!profile->xattrs)
> + goto fail;
> + for (i = 0; i < size; i++) {
> + if (!unpack_strdup(e, >xattrs[i], NULL))
> + goto fail;

If this step fails before completion, the xattrs array may have some
entries that weren't properly initialized; I suspect the free operation
will cause serious trouble in this case.

> + }
> + if (!unpack_nameX(e, AA_ARRAYEND, NULL))
> + goto fail;
> + if (!unpack_nameX(e, AA_STRUCTEND, NULL))
> + goto fail;
> + }
> +
> + if (unpack_nameX(e, AA_STRUCT, "xattr_values")) {
> + int i, size;
> +
> + size = unpack_array(e, NULL);
> +
> + /* Must be the same number of xattr values as xattrs */
> + if (size != profile->xattr_count)
> + goto fail;
> +
> + profile->xattr_lens = kmalloc_array(size, sizeof(size_t),
> + GFP_KERNEL);
> + if (!profile->xattr_lens)
> + goto fail;
> +
> + profile->xattr_values = kmalloc_array(size, sizeof(char *),
> +   GFP_KERNEL);

Same thing here with the xattr_lens and xattr_values arrays.

> + if (!profile->xattr_values)
> + goto fail;
> +
> + for (i = 0; i < size; i++) {
> + profile->xattr_lens[i] = unpack_blob(e,
> +   >xattr_values[i], NULL);
> + profile->xattr_values[i] =
> + kvmemdup(profile->xattr_values[i],
> +  profile->xattr_lens[i]);
> + }
> +
> + if (!unpack_nameX(e, AA_ARRAYEND, NULL))
> + goto fail;
> + if (!unpack_nameX(e, AA_STRUCTEND, NULL))
> + goto fail;
> + }
> + return 1;
> +
> +fail:
> + e->pos = pos;
> + return 0;
> +}

Thanks


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

Re: [apparmor] Note: NVIDIA drivers are mapping user-writable files by default

[Jamie Strandboge]

On Thu, 2018-02-08 at 19:46 +0200, Vincas Dargis wrote:
> On 2/6/18 9:25 PM, Jamie Strandboge wrote:
> > > Anyway, do we _really_ want to allow mmap on writable files..?
> > 
> > Not usually, but in the case of actual shared memory files, there
> > isn't
> > another choice atm. Some day we'll mediate shared memory with non-
> > file
> > rules[1].
> 
> There is a choice to deny it.

Of course. My point was that an nvidia user of the profiled application
is going to expect 3d acceleration from the drivers so a profile that
is meant to work with nvidia should do that (but see below where I
respond to your upstream docs). An admin or profile author can choose
to use nvidia-strict, nvidia, neither, etc.

>  Since these... paths (not originally 
> intended to be a "physical file", except to these .gl*) are requested
> to 
> be mmap'ed for execution, some _real_ file named @{HOME}/#3141528 or 
> /tmp/.glMy1337Exploit could be loaded to enhance attack further. Or
> mine 
> Monero or smth. Or scan Thunderbird's memory for passwords.

Obviously it weakens the profile.

Importantly, if these are actually anonymous shared memory files (the
ones starting with '#'), the kernel shouldn't be creating a name for
the anonymous shared memory that already exists as an actual file.
Furthermore, there needs to be coordination between the processes (ie,
parent/child) to access the kernel file-- an unrelated outside process
can't just start reading from a random anonymous shared memory file.

For the named shared memory files, yes this is potentially an issue,
but an exploitable issue would be a bug in the attacked application.
Applications using shared memory will use an unpredictable path to open
with O_CREAT|O_EXCL, then unlink the file, then pass the fd to the
processes it cares about coordinating with (look in /dev/shm now-- you
won't see anything, as opposed to `sudo lsof|grep '/shm/'` which should
show a lot if you have a browser open).

The paths chosen by the nvidia driver are not ideal, but in practice,
barring bugs, it is mainly that there isn't perfect isolation between
applications (ie, app A and app B can coordinate to share with the
globbed file rules). The other thing it allows is that if an attacker
took control of the profiled application, it could then write out files
that could be mmap'd PROT_EXEC by the application. This is a valid
point, but if you have that much control of the application you don't
need to write out those files.

My point is that the abstractions are meant to be generally usable and
erring on the side of security in an abstraction that is clearly meant
to work with something such that it breaks that something, isn't useful
to anyone. I also mentioned that these would need to be investigated,
which you've done below, which I responded to.


> I'm betting on this because it doesn't seem we have a lot of 
> graphic-intensive applications with AppArmor profile that might feel
> a 
> hit due to disabled optional NVIDIA optimizations.

The AppArmor project doesn't nor does Ubuntu, Debian, OpenSUSE, Tails,
etc, but there are consumers of apparmor that absolutely do (ie, there
are a lot of games packaged as snaps, and snappy uses AppArmor).

> > Furthermore, as a user, if I had an nvidia card I would expect that
> > the
> > nvidia abstraction would allow 3D acceleration to work and it
> > sounds
> > like nvidia changed the way their shared memory works (again, I
> > don't
> > have an nvidia system and basing this on the fact that these
> > denials
> > are only showing up now). 
> 
> 3D acceleration works fine with denies. It is only a additional 
> optimization NVIDIA have implemented. It is even documented that it
> does 
> not work on systems like SELinux and GRSecurity.
> 
>  From "Disabling executable memory optimizations" [0]:
> 
> quote
> 
> By default, the NVIDIA driver will attempt to use optimizations
> which 
> rely on being able to write to executable memory. This may cause 
> problems in certain system configurations (e.g., on SELinux when the 
> "allow_execmem" boolean is disabled or "deny_execmem" boolean is 
> enabled, and on grsecurity kernels configured with
> CONFIG_PAX_MPROTECT). 
> When possible, the driver will attempt to detect when it is running
> on 
> an unsupported configuration and disable these optimizations 
> automatically. If the __GL_WRITE_TEXT_SECTION environment variable
> is 
> set to 0, the driver will unconditionally disable these
> optimizations.
> 
> end quote.
> 
> So, if they disable them on SELinux and/or grsecurity kernels, we
> could 
> disable it by ourselves for AppArmor cases. Again, 3D acceleration
> works 
> fine with these denies (I use home-grew Wine and Steam profiles
> without 
> allowing these).

With this information, I would then suggest adding comments to the
nvidia abstraction on the above. Then someone profiling blender could
see that and add the necessary rules to the blender profile for the
optimizations.

So yes, whenever we get deny 

Re: [apparmor] IPC and sockets

[Viacheslav Salnikov]

Hi guys,

I checked out Ubuntu 16.04 and got this output:
$ cat /sys/kernel/security/apparmor/features/network/af_unix
yes

But Ubuntu 16.04 based on 4.4 kernel
$ uname -a
Linux 4.4.0-112-generic #135-Ubuntu SMP Fri Jan 19 11:48:36 UTC 2018 x86_64
x86_64 x86_64 GNU/Linux


I cloned xenial kernel for investigation and af_unit is in the kernel.
Does it mean that somebody did the backport or what? Maybe you know about
that.

Best regards, Slava.


2017-12-14 11:55 GMT+02:00 Viacheslav Salnikov :

> Hello Seth and John,
>
> Thanks for your answers.
> 
> -
> It seems that used version of apparmor parser has support for unix sockets
> (I use 2.11):
>
> on this
> *$ echo "profile p { unix, }" | apparmor_parser -Qd*
>
> I got the following output
>
>
>
>
>
> * Warning from stdin (line 1): apparmor_parser: cannot use or update
> cache, disable, or force-complain via stdin - Debugging built
> structures - Name: p Profile Mode: Enforce unix (),*
>
> 
> -
> Is it possible to back-port from v4.13 to the v4.4? There are a lot of
> changes.
> Well, it's not like I want you to do all the work for me, alright? Is it
> possible to cooperate on this one?
>
> I think that the main unix socket functionality was brought by this patch:
> https://gitlab.com/apparmor/apparmor/blob/master/kernel-
> patches/v4.13/0017-UBUNTU-SAUCE-apparmor-af_unix-mediation.patch
>
> What else should be added to the kernel?
>
>
> 2017-12-08 22:37 GMT+01:00 John Johansen :
>
>> On 12/08/2017 08:20 AM, Viacheslav Salnikov wrote:
>> > Hello,
>> >
>> > First of all, I googled and experimented. Didn't work out so well.
>> >
>> > I want to ensure that communication through unix socket is monitored by
>> apparmor.
>> > What should I do to make this happen?
>> >
>>
>> As Seth mentioned you will need a kernel, and userspace that supports
>> unix socket
>> mediation.
>>
>> AppArmor 2.11 (latest release) supports unix socket rules.
>>
>> The Ubuntu kernels have supported unix socket mediation in some form
>> since 14.10
>>
>> The patch does not currently exist in the upstream kernel but there is an
>> out of tree patchset available, in the kernel-patches/ directory of the
>> userspace project.
>>
>> You can find it in the release tarball, or gitlab.com/apparmor/apparmor
>>
>> you will want the v4.13 or v4.14 dir
>>
>>
>
-- 
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/apparmor

[apparmor] [PATCH 3/3] apparmor: improve overlapping domain attachment, resolution

[John Johansen]

Overlapping domain attachments using the current longest left exact
match fail in some simple cases, and with the fix to ensure consistant
behavior by failing unresolvable attachments it becomes important to
do a better job.

eg. under the current match the following are unresolvable where
the alternation is clearly a better match under the most specific
left match rule.
  /**
  /{bin/,}usr/

Use a counting match that detects when a loop in the state machine is
enter, and return the match count to provide a better specific left
match resolution.

Signed-off-by: John Johansen 
---
 security/apparmor/domain.c|  29 +
 security/apparmor/include/match.h |  19 ++
 security/apparmor/match.c | 122 +-
 3 files changed, 157 insertions(+), 13 deletions(-)

diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 9651e18cbae5..57cc892e05a2 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -385,10 +385,13 @@ static struct aa_profile *__attach_match(const struct 
linux_binprm *bprm,
 struct list_head *head,
 const char **info)
 {
-   int len = 0, xattrs = 0;
+   int candidate_len = 0, candidate_xattrs = 0;
bool conflict = false;
struct aa_profile *profile, *candidate = NULL;
 
+   AA_BUG(!name);
+   AA_BUG(!head);
+
list_for_each_entry_rcu(profile, head, base.list) {
if (profile->label.flags & FLAG_NULL &&
>label == ns_unconfined(profile->ns))
@@ -406,18 +409,20 @@ static struct aa_profile *__attach_match(const struct 
linux_binprm *bprm,
 * match.
 */
if (profile->xmatch) {
-   unsigned int state;
+   unsigned int state, count;
u32 perm;
-   if (profile->xmatch_len < len)
-   continue;
 
-   state = aa_dfa_match(profile->xmatch,
-DFA_START, name);
+   state = aa_dfa_leftmatch(profile->xmatch, DFA_START,
+name, );
perm = dfa_user_allow(profile->xmatch, state);
/* any accepting state means a valid match. */
if (perm & MAY_EXEC) {
-   int ret = aa_xattrs_match(bprm, profile, state);
+   int ret;
+
+   if (count < candidate_len)
+   continue;
 
+   ret = aa_xattrs_match(bprm, profile, state);
/* Fail matching if the xattrs don't match */
if (ret < 0)
continue;
@@ -428,10 +433,10 @@ static struct aa_profile *__attach_match(const struct 
linux_binprm *bprm,
 * The new match isn't more specific
 * than the current best match
 */
-   if (profile->xmatch_len == len &&
-   ret <= xattrs) {
+   if (count == candidate_len &&
+   ret <= candidate_xattrs) {
/* Match is equivalent, so conflict */
-   if (ret == xattrs)
+   if (ret == candidate_xattrs)
conflict = true;
continue;
}
@@ -440,8 +445,8 @@ static struct aa_profile *__attach_match(const struct 
linux_binprm *bprm,
 * xattrs, or a longer match
 */
candidate = profile;
-   len = profile->xmatch_len;
-   xattrs = ret;
+   candidate_len = profile->xmatch_len;
+   candidate_xattrs = ret;
conflict = false;
}
} else if (!strcmp(profile->base.name, name))
diff --git a/security/apparmor/include/match.h 
b/security/apparmor/include/match.h
index 96f757600860..aa2591c96828 100644
--- a/security/apparmor/include/match.h
+++ b/security/apparmor/include/match.h
@@ -141,6 +141,25 @@ unsigned int aa_dfa_matchn_until(struct aa_dfa *dfa, 
unsigned int start,
 
 void aa_dfa_free_kref(struct kref *kref);
 
+#define WB_HISTORY_SIZE 8
+struct match_workbuf {
+   unsigned int count;
+   unsigned int pos;
+   unsigned int len;
+   unsigned int size;  /* power of 2, same as 

[apparmor] [PATCH 2/3] apparmor: convert attaching profiles via xattrs to use, dfa matching

[John Johansen]

This converts profile attachment based on xattrs to a fixed extended
conditional using dfa matching.

This has a couple of advantages
- pattern matching can be used for the xattr match

- xattrs can be optional for an attachment or marked as required

- the xattr attachment conditional will be able to be combined with
  other extended conditionals when the flexible extended conditional
  work lands.

The xattr fixed extended conditional is appended to the xmatch
conditional. If an xattr attachment is specified the profile xmatch
will be generated regardless of whether there is a pattern match on
the executable name.

Signed-off-by: John Johansen 
---
 security/apparmor/apparmorfs.c |  5 
 security/apparmor/domain.c | 52 ++
 security/apparmor/include/policy.h |  2 --
 security/apparmor/policy.c |  6 +
 security/apparmor/policy_unpack.c  | 32 ---
 5 files changed, 42 insertions(+), 55 deletions(-)

diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index 1e63ff2e5b85..35e6b240fb14 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -2147,6 +2147,10 @@ static struct aa_sfs_entry aa_sfs_entry_signal[] = {
{ }
 };
 
+static struct aa_sfs_entry aa_sfs_entry_attach[] = {
+   AA_SFS_FILE_BOOLEAN("xattr", 1),
+   { }
+};
 static struct aa_sfs_entry aa_sfs_entry_domain[] = {
AA_SFS_FILE_BOOLEAN("change_hat",   1),
AA_SFS_FILE_BOOLEAN("change_hatv",  1),
@@ -2155,6 +2159,7 @@ static struct aa_sfs_entry aa_sfs_entry_domain[] = {
AA_SFS_FILE_BOOLEAN("stack",1),
AA_SFS_FILE_BOOLEAN("fix_binfmt_elf_mmap",  1),
AA_SFS_FILE_BOOLEAN("post_nnp_subset",  1),
+   AA_SFS_DIR("attach_conditions", aa_sfs_entry_attach),
AA_SFS_FILE_STRING("version", "1.2"),
{ }
 };
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index ca8353f3e7a0..9651e18cbae5 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -306,11 +306,12 @@ static int change_profile_perms(struct aa_profile 
*profile,
  * aa_xattrs_match - check whether a file matches the xattrs defined in profile
  * @bprm: binprm struct for the process to validate
  * @profile: profile to match against (NOT NULL)
+ * @state: state to start match in
  *
  * Returns: number of extended attributes that matched, or < 0 on error
  */
 static int aa_xattrs_match(const struct linux_binprm *bprm,
-  struct aa_profile *profile)
+  struct aa_profile *profile, unsigned int state)
 {
int i;
size_t size;
@@ -321,27 +322,40 @@ static int aa_xattrs_match(const struct linux_binprm 
*bprm,
if (!bprm || !profile->xattr_count)
return 0;
 
+   /* transition from exec match to xattr set */
+   state = aa_dfa_null_transition(profile->xmatch, state);
+
d = bprm->file->f_path.dentry;
 
for (i = 0; i < profile->xattr_count; i++) {
size = vfs_getxattr_alloc(d, profile->xattrs[i], ,
  value_size, GFP_KERNEL);
-   if (size < 0) {
-   ret = -EINVAL;
-   goto out;
-   }
+   if (size >= 0) {
+   u32 perm;
 
-   /* Check the xattr value, not just presence */
-   if (profile->xattr_lens[i]) {
-   if (profile->xattr_lens[i] != size) {
+   /* Check the xattr value, not just presence */
+   state = aa_dfa_match_len(profile->xmatch, state, value,
+size);
+   perm = dfa_user_allow(profile->xmatch, state);
+   if (!(perm & MAY_EXEC)) {
ret = -EINVAL;
goto out;
}
-
-   if (memcmp(value, profile->xattr_values[i], size)) {
+   }
+   /* transition to next element */
+   state = aa_dfa_null_transition(profile->xmatch, state);
+   if (size < 0) {
+   /*
+* No xattr match, so verify if transition to
+* next element was valid. IFF so the xattr
+* was optional.
+*/
+   if (!state) {
ret = -EINVAL;
goto out;
}
+   /* don't count missing optional xattr as matched */
+   ret--;
}
}
 
@@ -402,13 +416,16 @@ static struct aa_profile *__attach_match(const struct 
linux_binprm *bprm,
perm = dfa_user_allow(profile->xmatch, state);
  

[apparmor] [PATCH 1/3] apparmor: Add support for attaching profiles via xattr, presence and value

[John Johansen]

Make it possible to tie Apparmor profiles to the presence of one or more
extended attributes, and optionally their values. An example usecase for
this is to automatically transition to a more privileged Apparmor profile
if an executable has a valid IMA signature, which can then be appraised
by the IMA subsystem.

Signed-off-by: Matthew Garrett 
Signed-off-by: John Johansen 
---
 security/apparmor/domain.c | 151 +++--
 security/apparmor/include/policy.h |   6 ++
 security/apparmor/policy.c |   8 ++
 security/apparmor/policy_unpack.c  |  85 ++---
 4 files changed, 216 insertions(+), 34 deletions(-)

diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 9d1936519cfd..ca8353f3e7a0 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -19,6 +19,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "include/audit.h"
 #include "include/apparmorfs.h"
@@ -301,8 +302,57 @@ static int change_profile_perms(struct aa_profile *profile,
return label_match(profile, target, stack, start, true, request, perms);
 }
 
+/**
+ * aa_xattrs_match - check whether a file matches the xattrs defined in profile
+ * @bprm: binprm struct for the process to validate
+ * @profile: profile to match against (NOT NULL)
+ *
+ * Returns: number of extended attributes that matched, or < 0 on error
+ */
+static int aa_xattrs_match(const struct linux_binprm *bprm,
+  struct aa_profile *profile)
+{
+   int i;
+   size_t size;
+   struct dentry *d;
+   char *value = NULL;
+   int value_size = 0, ret = profile->xattr_count;
+
+   if (!bprm || !profile->xattr_count)
+   return 0;
+
+   d = bprm->file->f_path.dentry;
+
+   for (i = 0; i < profile->xattr_count; i++) {
+   size = vfs_getxattr_alloc(d, profile->xattrs[i], ,
+ value_size, GFP_KERNEL);
+   if (size < 0) {
+   ret = -EINVAL;
+   goto out;
+   }
+
+   /* Check the xattr value, not just presence */
+   if (profile->xattr_lens[i]) {
+   if (profile->xattr_lens[i] != size) {
+   ret = -EINVAL;
+   goto out;
+   }
+
+   if (memcmp(value, profile->xattr_values[i], size)) {
+   ret = -EINVAL;
+   goto out;
+   }
+   }
+   }
+
+out:
+   kfree(value);
+   return ret;
+}
+
 /**
  * __attach_match_ - find an attachment match
+ * @bprm - binprm structure of transitioning task
  * @name - to match against  (NOT NULL)
  * @head - profile list to walk  (NOT NULL)
  * @info - info message if there was an error (NOT NULL)
@@ -316,11 +366,12 @@ static int change_profile_perms(struct aa_profile 
*profile,
  *
  * Returns: profile or NULL if no match found
  */
-static struct aa_profile *__attach_match(const char *name,
+static struct aa_profile *__attach_match(const struct linux_binprm *bprm,
+const char *name,
 struct list_head *head,
 const char **info)
 {
-   int len = 0;
+   int len = 0, xattrs = 0;
bool conflict = false;
struct aa_profile *profile, *candidate = NULL;
 
@@ -329,26 +380,55 @@ static struct aa_profile *__attach_match(const char *name,
>label == ns_unconfined(profile->ns))
continue;
 
+   /* Find the "best" matching profile. Profiles must
+* match the path and extended attributes (if any)
+* associated with the file. A more specific path
+* match will be preferred over a less specific one,
+* and a match with more matching extended attributes
+* will be preferred over one with fewer. If the best
+* match has both the same level of path specificity
+* and the same number of matching extended attributes
+* as another profile, signal a conflict and refuse to
+* match.
+*/
if (profile->xmatch) {
-   if (profile->xmatch_len >= len) {
-   unsigned int state;
-   u32 perm;
-
-   state = aa_dfa_match(profile->xmatch,
-DFA_START, name);
-   perm = dfa_user_allow(profile->xmatch, state);
-   /* any accepting state means a valid match. */
-   if (perm & MAY_EXEC) {
-   if 

[apparmor] [Patch 0/3 Apparmor: Add support for attaching profiles via xattr presence and value

[John Johansen]

So this is my counter proposal

Patch 1/3 your V2 patch rebased

Patch 2/3 modifies the V2 patch so that the xattrs are matched using
the dfa. This provides more flexibility in what can be done with xattr
matching, and also makes it so the xattr match can be better
integrated with other match conditions when they land.

One thing to note is that it doesn't deal with the security.apparmor
xattr which will get special treatment later on. Conditions certainly
can use security.apparmor but it might be best to avoid it until the
patch dealing with it lands. The userspace patch dealing with this is
coming but I need a little more time with it. Basically I borrowed the
syntax for mount rule conditions, its not my favorite but it already
exists and is in use. The syntax can be revised if needed separate of
the kernel patch.  Basically it allows you to specify sets of xattr
conditions.

  profile foo xattrs=(security.apparmor=foo, security.ima=bar) {
..
  }

pattern matching can be used in the xattr values but not for the xattr
name. Multiple pattern sets can be defined, and xattrs can be set as
being required to be present or optional. I'll provide the full syntax
with the userspace patch so you can play with it

Patch 3/3 tries to address the overlapping expr bug, its not directly
related but I cherry-picked it on to the set as it might be worth
discussing if we want to partial overlap resolution of xattrs as we
are doing with the executable name or whether we want to at least for
now stick with just counting up the number of xattr matches.


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

Re: [apparmor] Note: NVIDIA drivers are mapping user-writable files by default

[Vincas Dargis]

On 2/6/18 9:25 PM, Jamie Strandboge wrote:

Anyway, do we _really_ want to allow mmap on writable files..?


Not usually, but in the case of actual shared memory files, there isn't
another choice atm. Some day we'll mediate shared memory with non-file
rules[1].


There is a choice to deny it. Since these... paths (not originally 
intended to be a "physical file", except to these .gl*) are requested to 
be mmap'ed for execution, some _real_ file named @{HOME}/#3141528 or 
/tmp/.glMy1337Exploit could be loaded to enhance attack further. Or mine 
Monero or smth. Or scan Thunderbird's memory for passwords.





I would, again, suggest to deny these with hope we would have a way
to
override denies in the future (John mentioned something in that
regard)
for some high performance 3D application really needs these NVIDIA
optimizations to be available, maybe in
`abstractions/nvidia-unsafe-optimizations`
abstraction.


I'm not sure this is the best way to handle this. Remember, explicit
deny rules are evaluated after deny rules so adding explicit deny rules
for these files means that an abstraction like nvidia-unsafe-
optimizations wouldn't be able to undo the deny (I guess this is what
you meant by overriding denies).


I've meant a new feature (we don't currently have) to override a `deny`, 
like a higher priority level. JJ was talking about that possibility in 
the future, in IRC I believe. If that get's designed I guess.


Maybe having something like `override allow @{HOME}/#[0-8]* rwm` would 
override that default `deny @{HOME}/#[0-8]* rwm` in 
`abstractions/nvidia`, for some exceptional cases, like in Blender 
application where real time 3D graphics performance is _very_ important 
(if it would ever have AppArmor profile.. but that's just an example).


I'm betting on this because it doesn't seem we have a lot of 
graphic-intensive applications with AppArmor profile that might feel a 
hit due to disabled optional NVIDIA optimizations.



Furthermore, as a user, if I had an nvidia card I would expect that the
nvidia abstraction would allow 3D acceleration to work and it sounds
like nvidia changed the way their shared memory works (again, I don't
have an nvidia system and basing this on the fact that these denials
are only showing up now). 


3D acceleration works fine with denies. It is only a additional 
optimization NVIDIA have implemented. It is even documented that it does 
not work on systems like SELinux and GRSecurity.


From "Disabling executable memory optimizations" [0]:

quote

By default, the NVIDIA driver will attempt to use optimizations which 
rely on being able to write to executable memory. This may cause 
problems in certain system configurations (e.g., on SELinux when the 
"allow_execmem" boolean is disabled or "deny_execmem" boolean is 
enabled, and on grsecurity kernels configured with CONFIG_PAX_MPROTECT). 
When possible, the driver will attempt to detect when it is running on 
an unsupported configuration and disable these optimizations 
automatically. If the __GL_WRITE_TEXT_SECTION environment variable is 
set to 0, the driver will unconditionally disable these optimizations.


end quote.

So, if they disable them on SELinux and/or grsecurity kernels, we could 
disable it by ourselves for AppArmor cases. Again, 3D acceleration works 
fine with these denies (I use home-grew Wine and Steam profiles without 
allowing these).




I would instead suggest that *if* we wanted
to split these accesses, we do something more like:

  * abstractions/nvidia-strict: all the safe accesses
  * abstractions/nvidia: #include nvidia-strict plus less desirable
accesses

In this manner, profiles continue to work with new nvidia accesses, but
  profilers can choose to do something more strict if they desire.
So if profile includes ``, they would have 
to add these

`deny @{HOME}/#[0-8]* rwm`,
`deny /tmp/.gl* rwm`
manually, in addition to the include?

If non-strict `abstractions/nvidia` include is left where it is now, 
that means in current state, Thunderbird, Firefox, Totem, and now I see 
Libreofice (running with `optirun`, to enabled discrete NVIDIA card, I 
see deny for nvidia-specific /usr/bin/nvidia-modrpobe, so include is 
needed there too) will have their security downgraded by default, and 
these does not look like graphic intensive apps. Meanwhile, updating 
these profiles would be too much a fuzz for a _optional_ NVIDIA 
optimizations, IMHO. While for security, I'm fuzzing here to defend :) .




[1]https://bugs.launchpad.net/apparmor/+bug/1370218



[0] 
https://download.nvidia.com/XFree86/Linux-x86_64/390.25/README/openglenvvariables.html#disableexecmem


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