Re: [PATCH] apparmor: avoid -Wempty-body warning
On 3/22/21 4:00 AM, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> Building with 'make W=1' shows a warning for an empty macro:
>
> security/apparmor/label.c: In function '__label_update':
> security/apparmor/label.c:2096:59: error: suggest braces around empty body in
> an 'else' statement [-Werror=empty-body]
> 2096 | AA_BUG(labels_ns(label) != labels_ns(new));
>
> Change the macro defintion to use no_printk(), which improves
> format string checking and avoids the warning.
>
> Signed-off-by: Arnd Bergmann
Aked-by: John Johansen
I have pulled it into the apparmor tree
> ---
> security/apparmor/include/lib.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/security/apparmor/include/lib.h b/security/apparmor/include/lib.h
> index 7d27db740bc2..67fbb81a11f3 100644
> --- a/security/apparmor/include/lib.h
> +++ b/security/apparmor/include/lib.h
> @@ -36,7 +36,7 @@
> #define AA_BUG_FMT(X, fmt, args...) \
> WARN((X), "AppArmor WARN %s: (" #X "): " fmt, __func__, ##args)
> #else
> -#define AA_BUG_FMT(X, fmt, args...)
> +#define AA_BUG_FMT(X, fmt, args...) no_printk(fmt, ##args)
> #endif
>
> #define AA_ERROR(fmt, args...)
> \
>
Re: [PATCH] apparmor: fix error check
On 10/4/20 7:24 AM, t...@redhat.com wrote:
> From: Tom Rix
>
> clang static analysis reports this representative problem:
>
> label.c:1463:16: warning: Assigned value is garbage or undefined
> label->hname = name;
> ^
>
> In aa_update_label_name(), this the problem block of code
>
> if (aa_label_acntsxprint(, ...) == -1)
> return res;
>
> On failure, aa_label_acntsxprint() has a more complicated return
> that just -1. So check for a negative return.
>
> It was also noted that the aa_label_acntsxprint() main comment refers
> to a nonexistent parameter, so clean up the comment.
>
> Fixes: f1bd904175e8 ("apparmor: add the base fns() for domain labels")
> Signed-off-by: Tom Rix
> ---
> security/apparmor/label.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/security/apparmor/label.c b/security/apparmor/label.c
> index e68bcedca976..6222fdfebe4e 100644
> --- a/security/apparmor/label.c
> +++ b/security/apparmor/label.c
> @@ -1454,7 +1454,7 @@ bool aa_update_label_name(struct aa_ns *ns, struct
> aa_label *label, gfp_t gfp)
> if (label->hname || labels_ns(label) != ns)
> return res;
>
> - if (aa_label_acntsxprint(, ns, label, FLAGS_NONE, gfp) == -1)
> + if (aa_label_acntsxprint(, ns, label, FLAGS_NONE, gfp) < 0)
> return res;
>
> ls = labels_set(label);
> @@ -1704,7 +1704,7 @@ int aa_label_asxprint(char **strp, struct aa_ns *ns,
> struct aa_label *label,
>
> /**
> * aa_label_acntsxprint - allocate a __counted string buffer and print label
> - * @strp: buffer to write to. (MAY BE NULL if @size == 0)
> + * @strp: buffer to write to.
> * @ns: namespace profile is being viewed from
> * @label: label to view (NOT NULL)
> * @flags: flags controlling what label info is printed
>
sorry it seems I missed replying to this. This patch has been pulled into
apparmor-next
Re: [RFC][PATCH] apparmor: Enforce progressively tighter permissions for no_new_privs
On 1/20/21 2:56 PM, Eric W. Biederman wrote:
>
> TL;DR selinux and apparmor ignore no_new_privs
>
> What?
>
AppArmor does not ignore no_new_privs. Its mediation is bounded
and it doesn't grant anything that wasn't allowed when NNP was
set.
>
> John Johansen writes:
>
>> On 1/20/21 1:26 PM, Eric W. Biederman wrote:
>>>
>>> The current understanding of apparmor with respect to no_new_privs is at
>>> odds with how no_new_privs is implemented and understood by the rest of
>>> the kernel.
>>>
>>> The documentation of no_new_privs states:
>>>> With ``no_new_privs`` set, ``execve()`` promises not to grant the
>>>> privilege to do anything that could not have been done without the
>>>> execve call.
>>>
>>> And reading through the kernel except for apparmor that description
>>> matches what is implemented.
>>>
>>
>> That is not correct.
>>
>> commit 7b0d0b40cd78 ("selinux: Permit bounded transitions under
>> NO_NEW_PRIVS or NOSUID.")
>>
>> Allows for bound transitions under selinux
>> and
>
> As I understand a bound transition it is a transition to a state with
> a set of permissions that are a subset of what was previously held.
> Which is consistent with the mandate of no_new_privs.
>
>> commit af63f4193f9f selinux: Generalize support for NNP/nosuid SELinux
>> domain transitions
>>
>> goes further and "Decouple NNP/nosuid from SELinux transitions".
>
> Yes. Looking at that commit I do see that selinux appears to be
> deliberately ignoring no_new_privs in specific cases.
>
> WTF.
>
>>> There are two major divergences of apparmor from this definition:
>>> - proc_setattr enforces limitations when no_new_privs are set.
>>> - the limitation is enforced from the apparent time when no_new_privs is
>>> set instead of guaranteeing that each execve has progressively more
>>> narrow permissions.
>>>
>>> The code in apparmor that attempts to discover the apparmor label at the
>>> point where no_new_privs is set is not robust. The capture happens a
>>> long time after no_new_privs is set.
>>>
>>
>> yes, but that shouldn't matter. As apparmor has not changed its label
>> at any point between when no_new_privs was set and when the check is
>> done. AppArmor is attempting to change it label, and if it finds NNP
>> has been set we capture what the confinement was.
>>
>>> Capturing the label at the point where no_new_privs is set is
>>> practically impossible to implement robustly. Today the rule is struct
>>> cred can only be changed by it's current task. Today
>>
>> right, and apparmor only ever has the task update its own label.
>>
>>> SECCOMP_FILTER_FLAG_TSYNC sets no_new_privs from another thread. A
>>> robust implementation would require changing something fundamental in
>>> how creds are managed for SECCOMP_FILTER_FLAG_TSYNC to be able to
>>> capture the cred at the point it is set.
>>>
>> I am open to supporting something like that.
>
> I can't see how it would be possible to be robust without completely
> changing the locking. Locking that right now in a simpler model we have
> not figured out how to make obviously correct.
>
>>> Futhermore given the consistent documentation and how everything else
>>> implements no_new_privs, not having the permissions get progressively
>>
>> Again see above
>
> Except where selinux deliberately ignores no_new_privs this is
> consitent.
>
>>> tighter is a footgun aimed at userspace. I fully expect it to break any
>>
>> tighter is somewhat relative, nor is it only progressively tighter it
>> is bounded against the snapshot of the label that was on the task.
>
> Which is the BUG I am reporting. It should be progressingly tighter.
>
>>> security sensitive software that uses no_new_privs and was not
>>> deliberately designed and tested against apparmor.
>>>
>>
>> Currently the situation has become either an either or choice between
>> the LSM and NNP. We are trying to walk a balance. Ideally apparmor
>> would like to do something similar to selinux and decouple the label
>> transition from NNP and nosuid via an internal capability, but we
>> have not gone there yet.
>
> Why do you need to escape no_new_privs. Why does anyone need to escape
> no_new_privs?
>
>>> Avoid the questionable and hard to fix implementation and the
>>> potential to confuse userspac
Re: [RFC][PATCH] apparmor: Enforce progressively tighter permissions for no_new_privs
On 1/20/21 1:26 PM, Eric W. Biederman wrote:
>
> The current understanding of apparmor with respect to no_new_privs is at
> odds with how no_new_privs is implemented and understood by the rest of
> the kernel.
>
> The documentation of no_new_privs states:
>> With ``no_new_privs`` set, ``execve()`` promises not to grant the
>> privilege to do anything that could not have been done without the
>> execve call.
>
> And reading through the kernel except for apparmor that description
> matches what is implemented.
>
That is not correct.
commit 7b0d0b40cd78 ("selinux: Permit bounded transitions under
NO_NEW_PRIVS or NOSUID.")
Allows for bound transitions under selinux
and
commit af63f4193f9f selinux: Generalize support for NNP/nosuid SELinux
domain transitions
goes further and "Decouple NNP/nosuid from SELinux transitions".
> There are two major divergences of apparmor from this definition:
> - proc_setattr enforces limitations when no_new_privs are set.
> - the limitation is enforced from the apparent time when no_new_privs is
> set instead of guaranteeing that each execve has progressively more
> narrow permissions.
>
> The code in apparmor that attempts to discover the apparmor label at the
> point where no_new_privs is set is not robust. The capture happens a
> long time after no_new_privs is set.
>
yes, but that shouldn't matter. As apparmor has not changed its label
at any point between when no_new_privs was set and when the check is
done. AppArmor is attempting to change it label, and if it finds NNP
has been set we capture what the confinement was.
> Capturing the label at the point where no_new_privs is set is
> practically impossible to implement robustly. Today the rule is struct
> cred can only be changed by it's current task. Today
right, and apparmor only ever has the task update its own label.
> SECCOMP_FILTER_FLAG_TSYNC sets no_new_privs from another thread. A
> robust implementation would require changing something fundamental in
> how creds are managed for SECCOMP_FILTER_FLAG_TSYNC to be able to
> capture the cred at the point it is set.
>
I am open to supporting something like that.
> Futhermore given the consistent documentation and how everything else
> implements no_new_privs, not having the permissions get progressively
Again see above
> tighter is a footgun aimed at userspace. I fully expect it to break any
tighter is somewhat relative, nor is it only progressively tighter it
is bounded against the snapshot of the label that was on the task.
> security sensitive software that uses no_new_privs and was not
> deliberately designed and tested against apparmor.
>
Currently the situation has become either an either or choice between
the LSM and NNP. We are trying to walk a balance. Ideally apparmor
would like to do something similar to selinux and decouple the label
transition from NNP and nosuid via an internal capability, but we
have not gone there yet.
> Avoid the questionable and hard to fix implementation and the
> potential to confuse userspace by having no_new_privs enforce
> progressinvely tighter permissions.
>
This would completely break several use cases.
> Fixes: 9fcf78cca198 ("apparmor: update domain transitions that are subsets of
> confinement at nnp")
> Signed-off-by: Eric W. Biederman
> ---
>
> I came accross this while examining the places cred_guard_mutex is
> used and trying to find a way to make those code paths less insane.
>
> If it would be more pallatable I would not mind removing the
> task_no_new_privs test entirely from aa_change_hat and aa_change_profile
> as those are not part of exec, so arguably no_new_privs should not care
> about them at all.
>
> Can we please get rid of the huge semantic wart and pain in the
> implementation?
>
> security/apparmor/domain.c | 39
> security/apparmor/include/task.h | 4
> security/apparmor/task.c | 7 --
> 3 files changed, 4 insertions(+), 46 deletions(-)
>
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index f919ebd042fd..8f77059bf890 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -869,17 +869,6 @@ int apparmor_bprm_creds_for_exec(struct linux_binprm
> *bprm)
>
> label = aa_get_newest_label(cred_label(bprm->cred));
>
> - /*
> - * Detect no new privs being set, and store the label it
> - * occurred under. Ideally this would happen when nnp
> - * is set but there isn't a good way to do that yet.
> - *
> - * Testing for unconfined must be done before the subset test
> - */
> - if ((bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS) && !unconfined(label) &&
> - !ctx->nnp)
> - ctx->nnp = aa_get_label(label);
> -
> /* buffer freed below, name is pointer into buffer */
> buffer = aa_get_buffer(false);
> if (!buffer) {
> @@ -915,7 +904,7 @@ int apparmor_bprm_creds_for_exec(struct
Re: [PATCH v2] security: apparmor: delete repeated words in comments
On 12/20/20 7:27 PM, Randy Dunlap wrote:
> Drop repeated words in comments.
> {a, then, to}
>
> Signed-off-by: Randy Dunlap
> Cc: John Johansen
> Cc: appar...@lists.ubuntu.com
> Cc: James Morris
> Cc: "Serge E. Hallyn"
> Cc: linux-security-mod...@vger.kernel.org
> Reviewed-By: Seth Arnold
Acked-by: John Johansen
I will pull this into apparmor-next
> ---
> v2: rebase
>
> security/apparmor/include/file.h |2 +-
> security/apparmor/path.c |2 +-
> security/apparmor/policy_unpack.c |2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> --- linux-next-20201218.orig/security/apparmor/include/file.h
> +++ linux-next-20201218/security/apparmor/include/file.h
> @@ -167,7 +167,7 @@ int aa_audit_file(struct aa_profile *pro
> * @perms: permission table indexed by the matched state accept entry of @dfa
> * @trans: transition table for indexed by named x transitions
> *
> - * File permission are determined by matching a path against @dfa and then
> + * File permission are determined by matching a path against @dfa and
> * then using the value of the accept entry for the matching state as
> * an index into @perms. If a named exec transition is required it is
> * looked up in the transition table.
> --- linux-next-20201218.orig/security/apparmor/path.c
> +++ linux-next-20201218/security/apparmor/path.c
> @@ -83,7 +83,7 @@ static int disconnect(const struct path
> *
> * Returns: %0 else error code if path lookup fails
> * When no error the path name is returned in @name which points to
> - * to a position in @buf
> + * a position in @buf
> */
> static int d_namespace_path(const struct path *path, char *buf, char **name,
> int flags, const char *disconnected)
> --- linux-next-20201218.orig/security/apparmor/policy_unpack.c
> +++ linux-next-20201218/security/apparmor/policy_unpack.c
> @@ -39,7 +39,7 @@
>
> /*
> * The AppArmor interface treats data as a type byte followed by the
> - * actual data. The interface has the notion of a a named entry
> + * actual data. The interface has the notion of a named entry
> * which has a name (AA_NAME typecode followed by name string) followed by
> * the entries typecode and data. Named types allow for optional
> * elements and extensions to be added and tested for without breaking
>
Re: [PATCH v2 00/10] allow unprivileged overlay mounts
On 12/10/20 1:39 AM, Miklos Szeredi wrote: > On Thu, Dec 10, 2020 at 10:00 AM John Johansen > wrote: >> >> On 12/8/20 2:27 AM, Tetsuo Handa wrote: >>> On 2020/12/08 1:32, Miklos Szeredi wrote: >>>> A general observation is that overlayfs does not call security_path_*() >>>> hooks on the underlying fs. I don't see this as a problem, because a >>>> simple bind mount done inside a private mount namespace also defeats the >>>> path based security checks. Maybe I'm missing something here, so I'm >>>> interested in comments from AppArmor and Tomoyo developers. >>> >>> Regarding TOMOYO, I don't want overlayfs to call security_path_*() hooks on >>> the >>> underlying fs, but the reason is different. It is not because a simple bind >>> mount >>> done inside a private mount namespace defeats the path based security >>> checks. >>> TOMOYO does want to check what device/filesystem is mounted on which >>> location. But >>> currently TOMOYO is failing to check it due to >>> fsopen()/fsmount()/move_mount() API. >>> >> >> Regardless of TOMOYO's approach I would say that overlays should call the >> security_path_*() hooks, making it possible for an LSM to do something based >> off of >> them when needed. >> >> The current state of private mounts with regard to path based mediation is >> broken. >> I just haven't had time to try and come up with an acceptable fix for it. >> overlayfs >> is actually broken under apparmor mediation, and accesses to the lower layer >> end up >> getting denied but there is no way to properly allow them. So policy that >> hits this >> needs a flag set that allows for it in a very hacky way (its on the list of >> things >> to fix). >> >> Path based mediation has to carefully control mounts otherwise policy can be >> circumvented as Miklos rightly points out. Ideally path based LSM wouldn't >> allow >> you to do the simple bind mount inside a private mount namespace (at least >> not >> unless policy allowed for it). AppArmor does mediate the mount hooks and bind >> mounts in a private mount namespace (if they go through the LSM mount hooks) >> will >> be denied. Again the problem is how to allow them, and this is broken. > > Okay, so what does that mean for overlayfs? > > AA can deny the overlay mount just as well as the bind mount, and it > can allow it just as well as the bind mount. Policy could be the > same. > not entirely the private mount is always detached from the namespace and we have no way to correlate the private mount to the overlay so the only safe thing is to deny operations on the private mount. Ideally we would have a way to provide some kind of correlation so we could make an informed decision. > Also all the security_path_ hooks will still get called for each > access on overlayfs itself. They won't be called for the accesses > which overlayfs does on underlying layers, but is that needed? > maybe but maybe not. The thing is apparmor doesn't just used security_path_ hooks which means we get some operation on leaking through that are using the private mount. Its this inconsistent partial view that is problematic. > Overlay could call those hooks itself (since the vfs_ helpers don't) > but the big question is whether that makes any sense. AFAICS it might > make sense, but only if AA would correctly handle bind mounts, and > especially detached bind mounts (which is what overlayfs technically > uses). > I haven't investigated enough to say for sure whether AA needs the path hooks called, but I think it probably doesn't. What AA does need is a way to determine what to do with private mounts when it encounters them in the none path hooks. That could be a simple as a hook when the private mount is setup so it can setup some state. > Thanks, > Miklos > > Tja >
Re: [PATCH v2 00/10] allow unprivileged overlay mounts
On 12/8/20 2:27 AM, Tetsuo Handa wrote: > On 2020/12/08 1:32, Miklos Szeredi wrote: >> A general observation is that overlayfs does not call security_path_*() >> hooks on the underlying fs. I don't see this as a problem, because a >> simple bind mount done inside a private mount namespace also defeats the >> path based security checks. Maybe I'm missing something here, so I'm >> interested in comments from AppArmor and Tomoyo developers. > > Regarding TOMOYO, I don't want overlayfs to call security_path_*() hooks on > the > underlying fs, but the reason is different. It is not because a simple bind > mount > done inside a private mount namespace defeats the path based security checks. > TOMOYO does want to check what device/filesystem is mounted on which > location. But > currently TOMOYO is failing to check it due to > fsopen()/fsmount()/move_mount() API. > Regardless of TOMOYO's approach I would say that overlays should call the security_path_*() hooks, making it possible for an LSM to do something based off of them when needed. The current state of private mounts with regard to path based mediation is broken. I just haven't had time to try and come up with an acceptable fix for it. overlayfs is actually broken under apparmor mediation, and accesses to the lower layer end up getting denied but there is no way to properly allow them. So policy that hits this needs a flag set that allows for it in a very hacky way (its on the list of things to fix). Path based mediation has to carefully control mounts otherwise policy can be circumvented as Miklos rightly points out. Ideally path based LSM wouldn't allow you to do the simple bind mount inside a private mount namespace (at least not unless policy allowed for it). AppArmor does mediate the mount hooks and bind mounts in a private mount namespace (if they go through the LSM mount hooks) will be denied. Again the problem is how to allow them, and this is broken.
Re: [PATCH v1] apparmor: Remove duplicate macro list_entry_is_head()
On 12/8/20 2:06 AM, Andy Shevchenko wrote: > Strangely I hadn't had noticed the existence of the list_entry_is_head() in > apparmor code when added the same one in the list.h. Luckily it's fully > identical and didn't break builds. In any case we don't need a duplicate > anymore, thus remove it from apparmor code. > > Signed-off-by: Andy Shevchenko oh nice, I will pull into the apparmor tree Acked-by: John Johansen > --- > security/apparmor/apparmorfs.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c > index 5fd4a64e431f..f95c6bfa8b8e 100644 > --- a/security/apparmor/apparmorfs.c > +++ b/security/apparmor/apparmorfs.c > @@ -2046,9 +2046,6 @@ int __aafs_ns_mkdir(struct aa_ns *ns, struct dentry > *parent, const char *name, > return error; > } > > - > -#define list_entry_is_head(pos, head, member) (>member == (head)) > - > /** > * __next_ns - find the next namespace to list > * @root: root namespace to stop search at (NOT NULL) >
Re: [PATCH v22 12/23] LSM: Specify which LSM to display
On 11/9/20 2:28 PM, Casey Schaufler wrote:
> On 11/7/2020 2:05 PM, John Johansen wrote:
>> On 11/7/20 1:15 AM, Greg KH wrote:
>>> On Fri, Nov 06, 2020 at 04:20:43PM -0800, Casey Schaufler wrote:
>>>> On 11/5/2020 1:22 AM, Greg KH wrote:
>>>>> On Wed, Nov 04, 2020 at 03:41:03PM -0800, Casey Schaufler wrote:
>>>>>> Create a new entry "display" in the procfs attr directory for
>>>>>> controlling which LSM security information is displayed for a
>>>>>> process. A process can only read or write its own display value.
>>>>>>
>>>>>> The name of an active LSM that supplies hooks for
>>>>>> human readable data may be written to "display" to set the
>>>>>> value. The name of the LSM currently in use can be read from
>>>>>> "display". At this point there can only be one LSM capable
>>>>>> of display active. A helper function lsm_task_display() is
>>>>>> provided to get the display slot for a task_struct.
>>>>>>
>>>>>> Setting the "display" requires that all security modules using
>>>>>> setprocattr hooks allow the action. Each security module is
>>>>>> responsible for defining its policy.
>>>>>>
>>>>>> AppArmor hook provided by John Johansen
>>>>>> SELinux hook provided by Stephen Smalley
>>>>>>
>>>>>> Reviewed-by: Kees Cook
>>>>>> Acked-by: Stephen Smalley
>>>>>> Acked-by: Paul Moore
>>>>>> Signed-off-by: Casey Schaufler
>>>>>> Cc: linux-...@vger.kernel.org
>>>>>> ---
>>>>>> fs/proc/base.c | 1 +
>>>>>> include/linux/lsm_hooks.h| 17 +++
>>>>>> security/apparmor/include/apparmor.h | 3 +-
>>>>>> security/apparmor/lsm.c | 32 +
>>>>>> security/security.c | 169 ---
>>>>>> security/selinux/hooks.c | 11 ++
>>>>>> security/selinux/include/classmap.h | 2 +-
>>>>>> security/smack/smack_lsm.c | 7 ++
>>>>>> 8 files changed, 223 insertions(+), 19 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>>>>>> index 0f707003dda5..7432f24f0132 100644
>>>>>> --- a/fs/proc/base.c
>>>>>> +++ b/fs/proc/base.c
>>>>>> @@ -2806,6 +2806,7 @@ static const struct pid_entry attr_dir_stuff[] = {
>>>>>> ATTR(NULL, "fscreate", 0666),
>>>>>> ATTR(NULL, "keycreate", 0666),
>>>>>> ATTR(NULL, "sockcreate",0666),
>>>>>> +ATTR(NULL, "display", 0666),
>>>>> That's a vague name, any chance it can be more descriptive?
>>>> Sure. How about lsm_display, or display_lsm? I wouldn't say that
>>>> any of the files in /proc/*/attr have especially descriptive names,
>>>> but that's hardly an excuse.
>>> I still don't understand what "display" means in this context. Perhaps
>> its the LSM thats context is being displayed on the shared interface,
>> ie. /proc/*/attr/*
>>
>> thinking about it more owner or even interface_owner might be a better
>> name
>
> I was hoping for a single word, but I see how something more descriptive
> might be in order. How about "lsm_of_current"? Or "lsm_of_dot_slash_current",
> if you want to be pedantic. "format_of_current" isn't quite accurate, but
> might be easier for some people to understand. Maybe "interface_owning_lsm".
>
> /proc/*/attr/display answers the question "Which LSM is providing the data
> I see if I look in /proc/*/attr/current, prev or exec or if that process uses
> SO_PEERSEC".
>
lsm_of_current or interface_lsm or interface_owning_lsm all wfm
>
>>> documentation will help clear it up?
>>>
>> yeah this needs documented.
>
> Agreed. I've noticed that nothing in /proc/*/attr seems documented
> in an orderly (documentation/ABI) fashion. I will have to fix some of
> that for a description of /proc/*/attr/whatever_it_ends_up_getting_called
> to make sense. Working on it.
>
>>> thanks,
>>>
>>> greg k-h
>>>
>
Re: [PATCH v22 12/23] LSM: Specify which LSM to display
On 11/7/20 1:15 AM, Greg KH wrote:
> On Fri, Nov 06, 2020 at 04:20:43PM -0800, Casey Schaufler wrote:
>> On 11/5/2020 1:22 AM, Greg KH wrote:
>>> On Wed, Nov 04, 2020 at 03:41:03PM -0800, Casey Schaufler wrote:
>>>> Create a new entry "display" in the procfs attr directory for
>>>> controlling which LSM security information is displayed for a
>>>> process. A process can only read or write its own display value.
>>>>
>>>> The name of an active LSM that supplies hooks for
>>>> human readable data may be written to "display" to set the
>>>> value. The name of the LSM currently in use can be read from
>>>> "display". At this point there can only be one LSM capable
>>>> of display active. A helper function lsm_task_display() is
>>>> provided to get the display slot for a task_struct.
>>>>
>>>> Setting the "display" requires that all security modules using
>>>> setprocattr hooks allow the action. Each security module is
>>>> responsible for defining its policy.
>>>>
>>>> AppArmor hook provided by John Johansen
>>>> SELinux hook provided by Stephen Smalley
>>>>
>>>> Reviewed-by: Kees Cook
>>>> Acked-by: Stephen Smalley
>>>> Acked-by: Paul Moore
>>>> Signed-off-by: Casey Schaufler
>>>> Cc: linux-...@vger.kernel.org
>>>> ---
>>>> fs/proc/base.c | 1 +
>>>> include/linux/lsm_hooks.h| 17 +++
>>>> security/apparmor/include/apparmor.h | 3 +-
>>>> security/apparmor/lsm.c | 32 +
>>>> security/security.c | 169 ---
>>>> security/selinux/hooks.c | 11 ++
>>>> security/selinux/include/classmap.h | 2 +-
>>>> security/smack/smack_lsm.c | 7 ++
>>>> 8 files changed, 223 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>>>> index 0f707003dda5..7432f24f0132 100644
>>>> --- a/fs/proc/base.c
>>>> +++ b/fs/proc/base.c
>>>> @@ -2806,6 +2806,7 @@ static const struct pid_entry attr_dir_stuff[] = {
>>>>ATTR(NULL, "fscreate", 0666),
>>>>ATTR(NULL, "keycreate", 0666),
>>>>ATTR(NULL, "sockcreate",0666),
>>>> + ATTR(NULL, "display", 0666),
>>> That's a vague name, any chance it can be more descriptive?
>>
>> Sure. How about lsm_display, or display_lsm? I wouldn't say that
>> any of the files in /proc/*/attr have especially descriptive names,
>> but that's hardly an excuse.
>
> I still don't understand what "display" means in this context. Perhaps
its the LSM thats context is being displayed on the shared interface,
ie. /proc/*/attr/*
thinking about it more owner or even interface_owner might be a better
name
> documentation will help clear it up?
>
yeah this needs documented.
> thanks,
>
> greg k-h
>
Re: [PATCH v6 1/4] IMA: Add func to measure LSM state and policy
On 8/5/20 8:43 AM, Stephen Smalley wrote:
> On 8/5/20 11:07 AM, Tyler Hicks wrote:
>
>> On 2020-08-05 10:27:43, Stephen Smalley wrote:
>>> On Wed, Aug 5, 2020 at 9:20 AM Mimi Zohar wrote:
On Wed, 2020-08-05 at 09:03 -0400, Stephen Smalley wrote:
> On Wed, Aug 5, 2020 at 8:57 AM Mimi Zohar wrote:
>> On Wed, 2020-08-05 at 08:46 -0400, Stephen Smalley wrote:
>>> On 8/4/20 11:25 PM, Mimi Zohar wrote:
>>>
Hi Lakshmi,
There's still a number of other patch sets needing to be reviewed
before my getting to this one. The comment below is from a high level.
On Tue, 2020-08-04 at 17:43 -0700, Lakshmi Ramasubramanian wrote:
> Critical data structures of security modules need to be measured to
> enable an attestation service to verify if the configuration and
> policies for the security modules have been setup correctly and
> that they haven't been tampered with at runtime. A new IMA policy is
> required for handling this measurement.
>
> Define two new IMA policy func namely LSM_STATE and LSM_POLICY to
> measure the state and the policy provided by the security modules.
> Update ima_match_rules() and ima_validate_rule() to check for
> the new func and ima_parse_rule() to handle the new func.
I can understand wanting to measure the in kernel LSM memory state to
make sure it hasn't changed, but policies are stored as files. Buffer
measurements should be limited to those things that are not files.
Changing how data is passed to the kernel has been happening for a
while. For example, instead of passing the kernel module or kernel
image in a buffer, the new syscalls - finit_module, kexec_file_load -
pass an open file descriptor. Similarly, instead of loading the IMA
policy data, a pathname may be provided.
Pre and post security hooks already exist for reading files. Instead
of adding IMA support for measuring the policy file data, update the
mechanism for loading the LSM policy. Then not only will you be able
to measure the policy, you'll also be able to require the policy be
signed.
>>> To clarify, the policy being measured by this patch series is a
>>> serialized representation of the in-memory policy data structures being
>>> enforced by SELinux. Not the file that was loaded. Hence, this
>>> measurement would detect tampering with the in-memory policy data
>>> structures after the policy has been loaded. In the case of SELinux,
>>> one can read this serialized representation via /sys/fs/selinux/policy.
>>> The result is not byte-for-byte identical to the policy file that was
>>> loaded but can be semantically compared via sediff and other tools to
>>> determine whether it is equivalent.
>> Thank you for the clarification. Could the policy hash be included
>> with the other critical data? Does it really need to be measured
>> independently?
> They were split into two separate functions because we wanted to be
> able to support using different templates for them (ima-buf for the
> state variables so that the measurement includes the original buffer,
> which is small and relatively fixed-size, and ima-ng for the policy
> because it is large and we just want to capture the hash for later
> comparison against known-good). Also, the state variables are
> available for measurement always from early initialization, whereas
> the policy is only available for measurement once we have loaded an
> initial policy.
Ok, measuring the policy separately from other critical data makes
sense. Instead of measuring the policy, which is large, measure the
policy hash.
>>> I think that was the original approach. However, I had concerns with
>>> adding code to SELinux to compute a hash over the policy versus
>>> leaving that to IMA's existing policy and mechanism. If that's
>>> preferred I guess we can do it that way but seems less flexible and
>>> duplicative.
>> In AppArmor, we store the sha1 of the raw policy as the policy is
>> loaded. The hash is exposed to userspace in apparmorfs. See commit
>> 5ac8c355ae00 ("apparmor: allow introspecting the loaded policy pre
>> internal transform").
>>
>> It has proved useful as a mechanism for debugging as sometimes the
>> on-disk policy doesn't match the loaded policy and this can be a good
>> way to check that while providing support to users. John also mentions
>> checkpoint/restore in the commit message and I could certainly see how
>> the policy hashes would be useful in that scenario.
>>
>> When thinking through how Lakshmi's series could be extended for
>> AppArmor support, I was thinking that the AppArmor policy measurement
>> would be a measurement of these hashes that we already have in
Re: [PATCH ghak84 v4] audit: purge audit_log_string from the intra-kernel audit API
On 7/21/20 8:19 AM, Paul Moore wrote:
> On Tue, Jul 14, 2020 at 5:00 PM Richard Guy Briggs wrote:
>> On 2020-07-14 16:29, Paul Moore wrote:
>>> On Tue, Jul 14, 2020 at 1:44 PM Richard Guy Briggs wrote:
On 2020-07-14 12:21, Paul Moore wrote:
> On Mon, Jul 13, 2020 at 3:52 PM Richard Guy Briggs
> wrote:
>>
>> audit_log_string() was inteded to be an internal audit function and
>> since there are only two internal uses, remove them. Purge all external
>> uses of it by restructuring code to use an existing audit_log_format()
>> or using audit_log_format().
>>
>> Please see the upstream issue
>> https://github.com/linux-audit/audit-kernel/issues/84
>>
>> Signed-off-by: Richard Guy Briggs
>> ---
>> Passes audit-testsuite.
>>
>> Changelog:
>> v4
>> - use double quotes in all replaced audit_log_string() calls
>>
>> v3
>> - fix two warning: non-void function does not return a value in all
>> control paths
>> Reported-by: kernel test robot
>>
>> v2
>> - restructure to piggyback on existing audit_log_format() calls,
>> checking quoting needs for each.
>>
>> v1 Vlad Dronov
>> -
>> https://github.com/nefigtut/audit-kernel/commit/dbbcba46335a002f44b05874153a85b9cc18aebf
>>
>> include/linux/audit.h | 5 -
>> kernel/audit.c| 4 ++--
>> security/apparmor/audit.c | 10 --
>> security/apparmor/file.c | 25 +++--
>> security/apparmor/ipc.c | 46
>> +++---
>> security/apparmor/net.c | 14 --
>> security/lsm_audit.c | 4 ++--
>> 7 files changed, 46 insertions(+), 62 deletions(-)
>
> Thanks for restoring the quotes, just one question below ...
>
>> diff --git a/security/apparmor/ipc.c b/security/apparmor/ipc.c
>> index 4ecedffbdd33..fe36d112aad9 100644
>> --- a/security/apparmor/ipc.c
>> +++ b/security/apparmor/ipc.c
>> @@ -20,25 +20,23 @@
>>
>> /**
>> * audit_ptrace_mask - convert mask to permission string
>> - * @buffer: buffer to write string to (NOT NULL)
>> * @mask: permission mask to convert
>> + *
>> + * Returns: pointer to static string
>> */
>> -static void audit_ptrace_mask(struct audit_buffer *ab, u32 mask)
>> +static const char *audit_ptrace_mask(u32 mask)
>> {
>> switch (mask) {
>> case MAY_READ:
>> - audit_log_string(ab, "read");
>> - break;
>> + return "read";
>> case MAY_WRITE:
>> - audit_log_string(ab, "trace");
>> - break;
>> + return "trace";
>> case AA_MAY_BE_READ:
>> - audit_log_string(ab, "readby");
>> - break;
>> + return "readby";
>> case AA_MAY_BE_TRACED:
>> - audit_log_string(ab, "tracedby");
>> - break;
>> + return "tracedby";
>> }
>> + return "";
>
> Are we okay with this returning an empty string ("") in this case?
> Should it be a question mark ("?")?
>
> My guess is that userspace parsing should be okay since it still has
> quotes, I'm just not sure if we wanted to use a question mark as we do
> in other cases where the field value is empty/unknown.
Previously, it would have been an empty value, not even double quotes.
"?" might be an improvement.
>>>
>>> Did you want to fix that now in this patch, or leave it to later? As
>>> I said above, I'm not too bothered by it with the quotes so either way
>>> is fine by me.
>>
>> I'd defer to Steve, otherwise I'd say leave it, since there wasn't
>> anything there before and this makes that more evident.
>>
>>> John, I'm assuming you are okay with this patch?
>
> With no comments from John or Steve in the past week, I've gone ahead
> and merged the patch into audit/next.
>
sorry, for some reason I thought a new iteration of this was coming.
the patch is fine, the empty unknown value should be possible here
so changing it to "?" won't affect anything.
Re: [PATCH] Replace HTTP links with HTTPS ones: security
On 7/5/20 2:45 PM, Alexander A. Klimov wrote: > Rationale: > Reduces attack surface on kernel devs opening the links for MITM > as HTTPS traffic is much harder to manipulate. > > Deterministic algorithm: > For each file: > If not .svg: > For each line: > If doesn't contain `\bxmlns\b`: > For each link, `\bhttp://[^# \t\r\n]*(?:\w|/)`: > If both the HTTP and HTTPS versions > return 200 OK and serve the same content: > Replace HTTP with HTTPS. > > Signed-off-by: Alexander A. Klimov I went through and double checked all the https urls are good Acked-by: John Johansen > --- > Continuing my work started at 93431e0607e5. > > If there are any URLs to be removed completely or at least not HTTPSified: > Just clearly say so and I'll *undo my change*. > See also https://lkml.org/lkml/2020/6/27/64 > > If there are any valid, but yet not changed URLs: > See https://lkml.org/lkml/2020/6/26/837 > > security/Kconfig | 2 +- > security/apparmor/Kconfig| 2 +- > security/integrity/ima/Kconfig | 2 +- > security/integrity/ima/ima_template.c| 2 +- > security/integrity/ima/ima_template_lib.c| 2 +- > security/integrity/ima/ima_template_lib.h| 2 +- > security/keys/encrypted-keys/ecryptfs_format.c | 2 +- > security/keys/encrypted-keys/ecryptfs_format.h | 2 +- > security/keys/encrypted-keys/encrypted.c | 2 +- > security/keys/encrypted-keys/masterkey_trusted.c | 2 +- > 10 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/security/Kconfig b/security/Kconfig > index cd3cc7da3a55..7561f6f99f1d 100644 > --- a/security/Kconfig > +++ b/security/Kconfig > @@ -118,7 +118,7 @@ config INTEL_TXT > it was configured with, especially since they may be responsible for > providing such assurances to VMs and services running on it. > > - See <http://www.intel.com/technology/security/> for more information > + See <https://www.intel.com/technology/security/> for more information > about Intel(R) TXT. > See <http://tboot.sourceforge.net> for more information about tboot. > See Documentation/x86/intel_txt.rst for a description of how to enable > diff --git a/security/apparmor/Kconfig b/security/apparmor/Kconfig > index 03fae1bd48a6..348ed6cfa08a 100644 > --- a/security/apparmor/Kconfig > +++ b/security/apparmor/Kconfig > @@ -77,7 +77,7 @@ config SECURITY_APPARMOR_KUNIT_TEST > This builds the AppArmor KUnit tests. > > KUnit tests run during boot and output the results to the debug log > - in TAP format (http://testanything.org/). Only useful for kernel devs > + in TAP format (https://testanything.org/). Only useful for kernel devs > running KUnit test harness and are not for inclusion into a > production build. > > diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig > index edde88dbe576..6a5e4a77601b 100644 > --- a/security/integrity/ima/Kconfig > +++ b/security/integrity/ima/Kconfig > @@ -26,7 +26,7 @@ config IMA > an aggregate integrity value over this list inside the > TPM hardware, so that the TPM can prove to a third party > whether or not critical system files have been modified. > - Read <http://www.usenix.org/events/sec04/tech/sailer.html> > + Read <https://www.usenix.org/events/sec04/tech/sailer.html> > to learn more about IMA. > If unsure, say N. > > diff --git a/security/integrity/ima/ima_template.c > b/security/integrity/ima/ima_template.c > index 5a2def40a733..1e89e2d3851f 100644 > --- a/security/integrity/ima/ima_template.c > +++ b/security/integrity/ima/ima_template.c > @@ -1,7 +1,7 @@ > // SPDX-License-Identifier: GPL-2.0-only > /* > * Copyright (C) 2013 Politecnico di Torino, Italy > - *TORSEC group -- http://security.polito.it > + *TORSEC group -- https://security.polito.it > * > * Author: Roberto Sassu > * > diff --git a/security/integrity/ima/ima_template_lib.c > b/security/integrity/ima/ima_template_lib.c > index 635c6ac05050..41a5f435b793 100644 > --- a/security/integrity/ima/ima_template_lib.c > +++ b/security/integrity/ima/ima_template_lib.c > @@ -1,7 +1,7 @@ > // SPDX-License-Identifier: GPL-2.0-only > /* > * Copyright (C) 2013 Politecnico di Torino, Italy > - *TORSEC group -- http://security.polito.it > + *TORSEC group -- https://security.polito.it > * > * Author: Roberto Sassu > * > diff --git a/sec
Re: [PATCH 4/5] LSM: Define SELinux function to measure security state
On 6/15/20 10:44 AM, Mimi Zohar wrote:
> (Cc'ing John)
>
> On Mon, 2020-06-15 at 10:33 -0700, Casey Schaufler wrote:
>> On 6/15/2020 9:45 AM, Lakshmi Ramasubramanian wrote:
>>> On 6/15/20 4:57 AM, Stephen Smalley wrote:
>>>
>>> Hi Stephen,
>>>
>>> Thanks for reviewing the patches.
>>>
> +void security_state_change(char *lsm_name, void *state, int state_len)
> +{
> + ima_lsm_state(lsm_name, state, state_len);
> +}
> +
What's the benefit of this trivial function instead of just calling
ima_lsm_state() directly?
>>>
>>> One of the feedback Casey Schaufler had given earlier was that calling an
>>> IMA function directly from SELinux (or, any of the Security Modules) would
>>> be a layering violation.
>>
>> Hiding the ima_lsm_state() call doesn't address the layering.
>> The point is that SELinux code being called from IMA (or the
>> other way around) breaks the subsystem isolation. Unfortunately,
>> it isn't obvious to me how you would go about what you're doing
>> without integrating the subsystems.
>
> Casey, I'm not sure why you think there is a layering issue here.
> There were multiple iterations of IMA before it was upstreamed. One
> iteration had separate integrity hooks(LIM). Only when the IMA calls
> and the security hooks are co-located, are they combined, as requested
> by Linus.
>
I don't see the layering violation here either, Casey has already
responded and I don't have anything to add
> There was some AppArmour discussion about calling IMA directly, but I
> haven't heard about it in a while or seen the patch.
>
its lower priority than other work. I think calling IMA directly has use
cases but must be done very carefully, and well reviewed. I have would
have more concerns with IMA calling directly into the various LSMs.
[GIT PULL] apparmor updates for 5.8
Hi Linus, Can you please pull the following changes for apparmor Thanks! - John The following changes since commit c79f46a282390e0f5b306007bf7b11a46d529538: Linux 5.5-rc5 (2020-01-05 14:23:27 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor tags/apparmor-pr-2020-06-07 for you to fetch changes up to 3622ad25d4d68fcbdef3bc084b5916873e785344: apparmor: Fix memory leak of profile proxy (2020-06-07 13:38:55 -0700) Tag summary + Features - Replace zero-length array with flexible-array - add a valid state flags check - add consistency check between state and dfa diff encode flags - add apparmor subdir to proc attr interface - fail unpack if profile mode is unknown - add outofband transition and use it in xattr match - ensure that dfa state tables have entries + Cleanups - Use true and false for bool variable - Remove semicolon - Clean code by removing redundant instructions - Replace two seq_printf() calls by seq_puts() in aa_label_seq_xprint() - remove duplicate check of xattrs on profile attachment - remove useless aafs_create_symlink + Bug fixes - Fix memory leak of profile proxy - fix introspection of of task mode for unconfined tasks - fix nnp subset test for unconfined - check/put label on apparmor_sk_clone_security() Gustavo A. R. Silva (1): apparmor: Replace zero-length array with flexible-array John Johansen (11): apparmor: add a valid state flags check apparmor: add consistency check between state and dfa diff encode flags apparmor: add proc subdir to attrs apparmor: remove useless aafs_create_symlink apparmor: fix nnp subset test for unconfined apparmor: fail unpack if profile mode is unknown apparmor: add outofband transition and use it in xattr match apparmor: remove duplicate check of xattrs on profile attachment. apparmor: ensure that dfa state tables have entries apparmor: fix introspection of of task mode for unconfined tasks apparmor: Fix memory leak of profile proxy Markus Elfring (1): apparmor: Replace two seq_printf() calls by seq_puts() in aa_label_seq_xprint() Mateusz Nosek (1): security/apparmor/label.c: Clean code by removing redundant instructions Mauricio Faria de Oliveira (1): apparmor: check/put label on apparmor_sk_clone_security() Vasyl Gomonovych (1): AppArmor: Remove semicolon Zou Wei (1): apparmor: Use true and false for bool variable fs/proc/base.c| 13 + security/apparmor/apparmorfs.c| 56 +--- security/apparmor/domain.c| 39 + security/apparmor/file.c | 12 security/apparmor/include/label.h | 2 ++ security/apparmor/include/match.h | 11 +++ security/apparmor/label.c | 60 --- security/apparmor/lsm.c | 5 security/apparmor/match.c | 58 - security/apparmor/path.c | 2 +- security/apparmor/policy.c| 1 + security/apparmor/policy_unpack.c | 58 +++-- 12 files changed, 198 insertions(+), 119 deletions(-)
[GIT PULL] apparmor bug fixes for v5.7-rc6
Hi Linus, Can you please pull the following bug fixes for apparmor Thanks! - John The following changes since commit b85051e755b0e9d6dd8f17ef1da083851b83287d: Merge tag 'fixes-for-5.7-rc6' of git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux (2020-05-20 13:23:55 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor tags/apparmor-pr-2020-05-21 for you to fetch changes up to c54d481d71c6849e044690d3960aaebc730224cc: apparmor: Fix use-after-free in aa_audit_rule_init (2020-05-21 15:25:51 -0700) + Bug Fixes - Fix use-after-free in aa_audit_rule_init - Fix refcnt leak in policy_update - Fix potential label refcnt leak in aa_change_profile Navid Emamdoost (1): apparmor: Fix use-after-free in aa_audit_rule_init Xiyu Yang (2): apparmor: fix potential label refcnt leak in aa_change_profile apparmor: Fix aa_label refcnt leak in policy_update security/apparmor/apparmorfs.c | 3 ++- security/apparmor/audit.c | 3 ++- security/apparmor/domain.c | 3 +-- 3 files changed, 5 insertions(+), 4 deletions(-)
Re: [PATCH 0/4] Relocate execve() sanity checks
On 5/19/20 2:17 PM, Kees Cook wrote: > On Tue, May 19, 2020 at 01:42:28PM -0500, Eric W. Biederman wrote: >> Kees Cook writes: >> >>> On Tue, May 19, 2020 at 12:41:27PM -0500, Eric W. Biederman wrote: Kees Cook writes: > and given the LSM hooks, I think the noexec check is too late as well. > (This is especially true for the coming O_MAYEXEC series, which will > absolutely need those tests earlier as well[1] -- the permission checking > is then in the correct place: during open, not exec.) I think the only > question is about leaving the redundant checks in fs/exec.c, which I > think are a cheap way to retain a sense of robustness. The trouble is when someone passes through changes one of the permission checks for whatever reason (misses that they are duplicated in another location) and things then fail in some very unexpected way. >>> >>> Do you think this series should drop the "late" checks in fs/exec.c? >>> Honestly, the largest motivation for me to move the checks earlier as >>> I've done is so that other things besides execve() can use FMODE_EXEC >>> during open() and receive the same sanity-checking as execve() (i.e the >>> O_MAYEXEC series -- the details are still under discussion but this >>> cleanup will be needed regardless). >> >> I think this series should drop the "late" checks in fs/exec.c It feels >> less error prone, and it feels like that would transform this into >> something Linus would be eager to merge because series becomes a cleanup >> that reduces line count. > > Yeah, that was my initial sense too. I just started to get nervous about > removing the long-standing exec sanity checks. ;) > >> I haven't been inside of open recently enough to remember if the >> location you are putting the check fundamentally makes sense. But the >> O_MAYEXEC bits make a pretty strong case that something of the sort >> needs to happen. > > Right. I *think* it's correct place for now, based on my understanding > of the call graph (which is why I included it in the commit logs). > >> I took a quick look but I can not see clearly where path_noexec >> and the regular file tests should go. >> >> I do see that you have code duplication with faccessat which suggests >> that you haven't put the checks in the right place. > > Yeah, I have notes on the similar call sites (which I concluded, perhaps > wrongly) to ignore: > > do_faccessat() > user_path_at(dfd, filename, lookup_flags, ); > if (acc_mode & MAY_EXEC path_noexec() > inode_permission(inode, mode | MAY_ACCESS); > > This appears to be strictly advisory, and the path_noexec() test is > there to, perhaps, avoid surprises when doing access() then fexecve()? > I would note, however, that that path-based LSMs appear to have no hook > in this call graph at all. I was expecting a call like: > > security_file_permission(..., mode | MAY_ACCESS) > > but I couldn't find one (or anything like it), so only > inode_permission() is being tested (which means also the existing > execve() late tests are missed, and the newly added S_ISREG() test from > do_dentry_open() is missed). > > sadly correct, its something that we intend to fix but haven't gotten to it > prctl_set_mm_exe_file() > err = -EACCESS; > if (!S_ISREG(inode->i_mode) || path_noexec(>f_path)) > goto exit; > err = inode_permission(inode, MAY_EXEC); > > This is similar (no path-based LSM hooks present, only inode_permission() > used for permission checking), but it is at least gated by CAP_SYS_ADMIN. > dito here > > And this bring me to a related question from my review: does > dentry_open() intentionally bypass security_inode_permission()? I.e. it > calls vfs_open() not do_open(): > > openat2(dfd, char * filename, open_how) > build_open_flags(open_how, open_flags) > do_filp_open(dfd, filename, open_flags) > path_openat(nameidata, open_flags, flags) > file = alloc_empty_file(open_flags, current_cred()); > do_open(nameidata, file, open_flags) > may_open(path, acc_mode, open_flag) > inode_permission(inode, MAY_OPEN | acc_mode) > security_inode_permission(inode, acc_mode) > vfs_open(path, file) > do_dentry_open(file, path->dentry->d_inode, open) > if (unlikely(f->f_flags & FMODE_EXEC && > !S_ISREG(inode->i_mode))) ... > security_file_open(f) > /* path-based LSMs check for open here >* and use FMODE_* flags to determine how a file > * is being opened. */ > open() > > vs > > dentry_open(path, flags, cred) > f = alloc_empty_file(flags, cred); > vfs_open(path, f); > > I would expect dentry_open() to mostly duplicate a bunch of > path_openat(), but it lacks the may_open() call, etc. >
Re: WARNING: suspicious RCU usage with PROVE_RCU_LIST=y
On 4/6/20 4:41 AM, Amol Grover wrote:
> Hello,
>
> With respect to the patch https://lore.kernel.org/patchwork/patch/1202512/
> I boot tested with CONFIG_PROVE_RCU_LIST=y and encountered a susppicious RCU
> usage warning in "security/apparmor/include/lib.h". I thought of going forward
> and fix it myself, however, while going through the stack trace and the actual
> code, I found that the function (__lookupn_profile) is required to be called
> with rcu_read_locK() but the splat proves it otherwise.
>
The comment could be updated to
* Requires: rcu_read_lock be held or the namespace->lock be held.
bascially there are reader paths which take the rcu_read_lock and then call
the __lookupn_profile() helper eg. aa_lookupn_profile() and the writer paths
the hold the ns lock, like in the splat
aa_replace_profiles() takes the ns lock
> [ 12.727582] =
> [ 12.727599] WARNING: suspicious RCU usage
> [ 12.727601] 5.5.4-stable #17 Tainted: GE
> [ 12.727602] -
> [ 12.727604] security/apparmor/include/lib.h:191 RCU-list traversed in
> non-reader section!!
> [ 12.727605]
>other info that might help us debug this:
>
> [ 12.727606]
>rcu_scheduler_active = 2, debug_locks = 1
> [ 12.727608] 2 locks held by apparmor_parser/506:
> [ 12.727609] #0: 9f0687562490 (sb_writers#10){.+.+}, at:
> vfs_write+0x140/0x1a0
> [ 12.727614] #1: 9f0687f09ca8 (>lock){+.+.}, at:
> aa_replace_profiles+0x17a/0xdd0
> [ 12.727619]
>stack backtrace:
> [ 12.727621] CPU: 3 PID: 506 Comm: apparmor_parser Tainted: GE
>5.5.4-stable #17
> [ 12.727622] Hardware name: Gigabyte Technology Co., Ltd.
> Z170-D3H/Z170-D3H-CF, BIOS F21 03/06/2017
> [ 12.727623] Call Trace:
> [ 12.727627] dump_stack+0x8f/0xd0
> [ 12.727630] __lookupn_profile+0x19c/0x1a0
> [ 12.727632] ? aa_unpack+0x51b/0x580
> [ 12.727636] __lookup_replace+0x34/0xc0
> [ 12.727640] aa_replace_profiles+0x2a0/0xdd0
> [ 12.727649] policy_update+0x106/0x370
> [ 12.727653] profile_replace+0xa3/0x110
> [ 12.727657] vfs_write+0xb9/0x1a0
> [ 12.727661] ksys_write+0x68/0xe0
> [ 12.727666] do_syscall_64+0x5c/0xe0
> [ 12.727669] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [ 12.727671] RIP: 0033:0x7ff83fec7f93
> [ 12.727673] Code: 75 05 48 83 c4 58 c3 e8 eb 41 ff ff 66 2e 0f 1f 84 00 00
> 00 00 00 90 64 8b 04 25 18 00 00 00 85 c0 75 14 b8 01 00 00 00 0f 05 <48> 3d
> 00 f0 ff ff 77 55 c3 0f 1f 40 00 48 83 ec 28 48 89 54 24 18
> [ 12.727674] RSP: 002b:7ffcebb5c398 EFLAGS: 0246 ORIG_RAX:
> 0001
> [ 12.727676] RAX: ffda RBX: 7131 RCX:
> 7ff83fec7f93
> [ 12.727677] RDX: 7131 RSI: 5610fd804a40 RDI:
> 0006
> [ 12.727678] RBP: 5610fd804a40 R08: 7131 R09:
> 5610fd802f38
> [ 12.727680] R10: fa8a R11: 0246 R12:
>
> [ 12.727681] R13: 0006 R14: 5610fd7dd490 R15:
> 7131
>
> Thanks
> Amol
>
Re: [PATCH -next] apparmor: Use true and false for bool variable
On 4/28/20 4:52 AM, Zou Wei wrote: > Fixes coccicheck warnings: > > security/apparmor/file.c:162:9-10: WARNING: return of 0/1 in function > 'is_deleted' with return type bool > security/apparmor/file.c:362:9-10: WARNING: return of 0/1 in function > 'xindex_is_subset' with return type bool > security/apparmor/policy_unpack.c:246:9-10: WARNING: return of 0/1 in > function 'unpack_X' with return type bool > security/apparmor/policy_unpack.c:292:9-10: WARNING: return of 0/1 in > function 'unpack_nameX' with return type bool > security/apparmor/policy_unpack.c:646:8-9: WARNING: return of 0/1 in function > 'unpack_rlimits' with return type bool > security/apparmor/policy_unpack.c:604:8-9: WARNING: return of 0/1 in function > 'unpack_secmark' with return type bool > security/apparmor/policy_unpack.c:538:8-9: WARNING: return of 0/1 in function > 'unpack_trans_table' with return type bool > security/apparmor/policy_unpack.c:327:9-10: WARNING: return of 0/1 in > function 'unpack_u32' with return type bool > security/apparmor/policy_unpack.c:345:9-10: WARNING: return of 0/1 in > function 'unpack_u64' with return type bool > security/apparmor/policy_unpack.c:309:9-10: WARNING: return of 0/1 in > function 'unpack_u8' with return type bool > security/apparmor/policy_unpack.c:568:8-9: WARNING: return of 0/1 in function > 'unpack_xattrs' with return type bool > security/apparmor/policy_unpack.c:1007:10-11: WARNING: return of 0/1 in > function 'verify_dfa_xindex' with return type bool > security/apparmor/policy_unpack.c:997:9-10: WARNING: return of 0/1 in > function 'verify_xindex' with return type bool > > Reported-by: Hulk Robot > Signed-off-by: Zou Wei thanks, applied to apparmor-next
Re: [PATCH] apparmor: Replace zero-length array with flexible-array
On 5/7/20 11:43 AM, Gustavo A. R. Silva wrote:
> The current codebase makes use of the zero-length array language
> extension to the C90 standard, but the preferred mechanism to declare
> variable-length types such as these ones is a flexible array member[1][2],
> introduced in C99:
>
> struct foo {
> int stuff;
> struct boo array[];
> };
>
> By making use of the mechanism above, we will get a compiler warning
> in case the flexible array does not occur last in the structure, which
> will help us prevent some kind of undefined behavior bugs from being
> inadvertently introduced[3] to the codebase from now on.
>
> Also, notice that, dynamic memory allocations won't be affected by
> this change:
>
> "Flexible array members have incomplete type, and so the sizeof operator
> may not be applied. As a quirk of the original implementation of
> zero-length arrays, sizeof evaluates to zero."[1]
>
> sizeof(flexible-array-member) triggers a warning because flexible array
> members have incomplete type[1]. There are some instances of code in
> which the sizeof operator is being incorrectly/erroneously applied to
> zero-length arrays and the result is zero. Such instances may be hiding
> some bugs. So, this work (flexible-array member conversions) will also
> help to get completely rid of those sorts of issues.
>
> This issue was found with the help of Coccinelle.
>
> [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> [2] https://github.com/KSPP/linux/issues/21
> [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
>
> Signed-off-by: Gustavo A. R. Silva
Acked-by: John Johansen
I have pulled this into my tree
Re: WARNING: suspicious RCU usage with PROVE_RCU_LIST=y
On 5/14/20 11:24 AM, Amol Grover wrote:
> On Mon, Apr 06, 2020 at 05:11:34PM +0530, Amol Grover wrote:
>> Hello,
>>
>> With respect to the patch https://lore.kernel.org/patchwork/patch/1202512/
>> I boot tested with CONFIG_PROVE_RCU_LIST=y and encountered a susppicious RCU
>> usage warning in "security/apparmor/include/lib.h". I thought of going
>> forward
>> and fix it myself, however, while going through the stack trace and the
>> actual
>> code, I found that the function (__lookupn_profile) is required to be called
>> with rcu_read_locK() but the splat proves it otherwise.
>>
>> [ 12.727582] =
>> [ 12.727599] WARNING: suspicious RCU usage
>> [ 12.727601] 5.5.4-stable #17 Tainted: GE
>> [ 12.727602] -
>> [ 12.727604] security/apparmor/include/lib.h:191 RCU-list traversed in
>> non-reader section!!
>> [ 12.727605]
>>other info that might help us debug this:
>>
>> [ 12.727606]
>>rcu_scheduler_active = 2, debug_locks = 1
>> [ 12.727608] 2 locks held by apparmor_parser/506:
>> [ 12.727609] #0: 9f0687562490 (sb_writers#10){.+.+}, at:
>> vfs_write+0x140/0x1a0
>> [ 12.727614] #1: 9f0687f09ca8 (>lock){+.+.}, at:
>> aa_replace_profiles+0x17a/0xdd0
>> [ 12.727619]
>>stack backtrace:
>> [ 12.727621] CPU: 3 PID: 506 Comm: apparmor_parser Tainted: GE
>> 5.5.4-stable #17
>> [ 12.727622] Hardware name: Gigabyte Technology Co., Ltd.
>> Z170-D3H/Z170-D3H-CF, BIOS F21 03/06/2017
>> [ 12.727623] Call Trace:
>> [ 12.727627] dump_stack+0x8f/0xd0
>> [ 12.727630] __lookupn_profile+0x19c/0x1a0
>> [ 12.727632] ? aa_unpack+0x51b/0x580
>> [ 12.727636] __lookup_replace+0x34/0xc0
>> [ 12.727640] aa_replace_profiles+0x2a0/0xdd0
>> [ 12.727649] policy_update+0x106/0x370
>> [ 12.727653] profile_replace+0xa3/0x110
>> [ 12.727657] vfs_write+0xb9/0x1a0
>> [ 12.727661] ksys_write+0x68/0xe0
>> [ 12.727666] do_syscall_64+0x5c/0xe0
>> [ 12.727669] entry_SYSCALL_64_after_hwframe+0x49/0xbe
>> [ 12.727671] RIP: 0033:0x7ff83fec7f93
>> [ 12.727673] Code: 75 05 48 83 c4 58 c3 e8 eb 41 ff ff 66 2e 0f 1f 84 00
>> 00 00 00 00 90 64 8b 04 25 18 00 00 00 85 c0 75 14 b8 01 00 00 00 0f 05 <48>
>> 3d 00 f0 ff ff 77 55 c3 0f 1f 40 00 48 83 ec 28 48 89 54 24 18
>> [ 12.727674] RSP: 002b:7ffcebb5c398 EFLAGS: 0246 ORIG_RAX:
>> 0001
>> [ 12.727676] RAX: ffda RBX: 7131 RCX:
>> 7ff83fec7f93
>> [ 12.727677] RDX: 7131 RSI: 5610fd804a40 RDI:
>> 0006
>> [ 12.727678] RBP: 5610fd804a40 R08: 7131 R09:
>> 5610fd802f38
>> [ 12.727680] R10: fa8a R11: 0246 R12:
>>
>> [ 12.727681] R13: 0006 R14: 5610fd7dd490 R15:
>> 7131
>>
>> Thanks
>> Amol
>
> Hello,
>
> Just a friendly request to please go through the above _bug_.
>
yep, thanks. I am looking into to it now.
Re: [PATCH v3 6/6] security: apparmor: default KUNIT_* fragments to KUNIT_ALL_TESTS
On 5/11/20 6:14 AM, Anders Roxell wrote: > This makes it easier to enable all KUnit fragments. > > Adding 'if !KUNIT_ALL_TESTS' so individual tests can not be turned off. > Therefore if KUNIT_ALL_TESTS is enabled that will hide the prompt in > menuconfig. > > Reviewed-by: David Gow > Signed-off-by: Anders Roxell Acked-by: John Johansen
Re: [PATCH] apparmor: Fix use-after-free in aa_audit_rule_init
On 10/20/19 7:16 AM, Markus Elfring wrote:
>> … But after this release the the return statement
>> tries to access the label field of the rule which results in
>> use-after-free. Before releaseing the rule, copy errNo and return it
>> after releasing rule.
>
Navid thanks for finding this, and Markus thanks for the review
> Please avoid a duplicate word and a typo in this change description.
> My preference would be a v2 version of the patch with the small clean-ups
that Markus has pointed out.
If I don't see a v2 this week I can pull this one in and do the revisions
myself adding a little fix-up note.
>
> …
>> +++ b/security/apparmor/audit.c
> …
>> @@ -197,8 +198,9 @@ int aa_audit_rule_init(u32 field, u32 op, char *rulestr,
>> void **vrule)
>> rule->label = aa_label_parse(_ns->unconfined->label, rulestr,
>> GFP_KERNEL, true, false);
>> if (IS_ERR(rule->label)) {
>> +err = rule->label;
>
> How do you think about to define the added local variable in this if branch
> directly?
>
> + int err = rule->label;
>
yes, since err isn't defined or in use else where this would be preferable
>> aa_audit_rule_free(rule);
>> -return PTR_ERR(rule->label);
>> +return PTR_ERR(err);
>> }
>>
>> *vrule = rule;
>
>
> Regards,
> Markus
>
Re: [WTF?] aafs_create_symlink() weirdness
On 9/23/19 7:03 PM, Al Viro wrote:
WTF indeed
>
> static struct dentry *aafs_create_symlink(const char *name,
> struct dentry *parent,
> const char *target,
> void *private,
> const struct inode_operations *iops)
> {
> struct dentry *dent;
> char *link = NULL;
>
> if (target) {
> if (!link)
> return ERR_PTR(-ENOMEM);
> }
>
> Er... That's an odd way to spell
> if (target)
> return ERR_PTR(-ENOMEM);
> (and an odd error value to use). Especially since all callers are passing
> NULL as target.
>
> Why is that code even there, why does that argument still exist and
> how many people have actually read that function?
>
It looks like 1180b4c757aa failed to drop it as part of the patch, but it
certainly should have. I can't say why it happened but regardless its an ugly
mistake. Thank you very much for catching this Al.
A patch to drop it is below or feel free to cons up an alternate version.
---
commit 5dbc63d4a0aa819be8ecf21a67a352dd377b0221
Author: John Johansen
Date: Tue Sep 24 09:46:33 2019 -0700
apparmor: remove useless aafs_create_symlink
1180b4c757aa ("apparmor: fix dangling symlinks to policy rawdata after
replacement") reworked how the rawdata symlink is handled but failed
to remove aafs_create_symlink which was reduced to a useless stub .
Fixes: 1180b4c757aa ("apparmor: fix dangling symlinks to policy rawdata
after replacement")
Reported-by: Al Viro
Signed-off-by: John Johansen
diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index 9c0e593e30aa..308c99ea3cf8 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -335,38 +335,6 @@ static struct dentry *aafs_create_dir(const char *name,
struct dentry *parent)
NULL);
}
-/**
- * aafs_create_symlink - create a symlink in the apparmorfs filesystem
- * @name: name of dentry to create
- * @parent: parent directory for this dentry
- * @target: if symlink, symlink target string
- * @private: private data
- * @iops: struct of inode_operations that should be used
- *
- * If @target parameter is %NULL, then the @iops parameter needs to be
- * setup to handle .readlink and .get_link inode_operations.
- */
-static struct dentry *aafs_create_symlink(const char *name,
- struct dentry *parent,
- const char *target,
- void *private,
- const struct inode_operations *iops)
-{
- struct dentry *dent;
- char *link = NULL;
-
- if (target) {
- if (!link)
- return ERR_PTR(-ENOMEM);
- }
- dent = aafs_create(name, S_IFLNK | 0444, parent, private, link, NULL,
- iops);
- if (IS_ERR(dent))
- kfree(link);
-
- return dent;
-}
-
/**
* aafs_remove - removes a file or directory from the apparmorfs filesystem
*
@@ -1757,25 +1725,25 @@ int __aafs_profile_mkdir(struct aa_profile *profile,
struct dentry *parent)
}
if (profile->rawdata) {
- dent = aafs_create_symlink("raw_sha1", dir, NULL,
- profile->label.proxy,
- _link_sha1_iops);
+ dent = aafs_create("raw_sha1", S_IFLNK | 0444, dir,
+ profile->label.proxy, NULL, NULL,
+ _link_sha1_iops);
if (IS_ERR(dent))
goto fail;
aa_get_proxy(profile->label.proxy);
profile->dents[AAFS_PROF_RAW_HASH] = dent;
- dent = aafs_create_symlink("raw_abi", dir, NULL,
- profile->label.proxy,
- _link_abi_iops);
+ dent = aafs_create("raw_abi", S_IFLNK | 0444, dir,
+ profile->label.proxy, NULL, NULL,
+ _link_abi_iops);
if (IS_ERR(dent))
goto fail;
aa_get_proxy(profile->label.proxy);
profile->dents[AAFS_PROF_RAW_ABI] = dent;
- dent = aafs_create_symlink("raw_data", dir, NULL,
- profile->label.proxy,
- _link_data_iops);
+ dent = aafs_create("raw_data", S_IFLNK | 0444, dir,
+
Re: [PATCH V34 00/29] Lockdown as an LSM
On 6/24/19 4:01 PM, James Morris wrote: > On Fri, 21 Jun 2019, Matthew Garrett wrote: > >> Minor updates over V33 - security_is_locked_down renamed to >> security_locked_down, return value of security_locked_down is returned >> in most cases, one unnecessary patch was dropped, couple of minor nits >> fixed. > > Thanks for the respin. > > We are still not resolved on granularity. Stephen has said he's not sure > if a useful policy can be constructed with just confidentiality and > integrity settings. I'd be interested to know JJ and Casey's thoughts on > lockdown policy flexibility wrt their respective LSMs. > > These are also "all or nothing" choices which may prevent deployment due > to a user needing to allow (presumably controlled or mitigated) exceptions > to the policy. > > I haven't gotten a chance to play with this the way I want to so there is still a lot of questions regarding its interaction with apparmor and its policy, but from what I have seen so far it is looking good. I expect the all or nothing choices may limit its deployments (we really need to play with this more to say) but we already face similar issues. There are options we provide at a distro level that we can't turn on by default, but we do recommend to more security conscious users. If lockdown was in kernel we would certainly make it available for our users, we have even had a few people ask about it.
[GIT PULL] apparmor bug fixes for v5.3-rc6
Hi Linus, Can you please pull the following bug fixes for apparmor Thanks! - John The following changes since commit 9e0babf2c06c73cda2c0cd37a1653d823adb40ec: Linux 5.2-rc5 (2019-06-16 08:49:45 -1000) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor tags/apparmor-pr-2019-06-18 for you to fetch changes up to 156e42996bd84eccb6acf319f19ce0cb140d00e3: apparmor: reset pos on failure to unpack for various functions (2019-06-18 16:04:16 -0700) + Bug Fixes - Fix PROFILE_MEDIATES for untrusted input - enforce nullbyte at end of tag string - reset pos on failure to unpack for various functions Jann Horn (1): apparmor: enforce nullbyte at end of tag string John Johansen (1): apparmor: fix PROFILE_MEDIATES for untrusted input Mike Salvatore (1): apparmor: reset pos on failure to unpack for various functions security/apparmor/include/policy.h | 11 - security/apparmor/policy_unpack.c | 49 +++--- 2 files changed, 50 insertions(+), 10 deletions(-)
[GIT PULL] apparmor bug fixes for v5.3-rc4
Hi Linus, Can you please pull the following bug fixes for apparmor Thanks! - John The following changes since commit b8a5afa418c1f5c8d7814ef829a88e60ae52f618: net: correct zerocopy refcnt with udp MSG_MORE (2019-05-31 06:40:10 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor tags/apparmor-pr-2019-06-06 for you to fetch changes up to dd60c38193b1cd8bc1cbde1425881cd5227ef466: apparmor: enforce nullbyte at end of tag string (2019-05-31 06:50:00 -0700) + Bug Fixes - Fix PROFILE_MEDIATES for untrusted input - enforce nullbyte at end of tag string Jann Horn (1): apparmor: enforce nullbyte at end of tag string John Johansen (1): apparmor: fix PROFILE_MEDIATES for untrusted input security/apparmor/include/policy.h | 11 ++- security/apparmor/policy_unpack.c | 2 +- 2 files changed, 11 insertions(+), 2 deletions(-)
Re: [PATCH] apparmor: enforce nullbyte at end of tag string
On 5/28/19 8:32 AM, Jann Horn wrote:
> A packed AppArmor policy contains null-terminated tag strings that are read
> by unpack_nameX(). However, unpack_nameX() uses string functions on them
> without ensuring that they are actually null-terminated, potentially
> leading to out-of-bounds accesses.
>
> Make sure that the tag string is null-terminated before passing it to
> strcmp().
>
> Cc: sta...@vger.kernel.org
> Signed-off-by: Jann Horn
gah! yes!
Acked-by: John Johansen
> ---
> Warning: The existence of this bug has not been verified at runtime, and
> the patch is compile-tested only. I noticed this while browsing through
> the code, but didn't want to spend the time necessary to figure out how
> to actually test this at runtime.
>
>
> security/apparmor/policy_unpack.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/security/apparmor/policy_unpack.c
> b/security/apparmor/policy_unpack.c
> index f6c2bcb2ab14..33041c4fb69f 100644
> --- a/security/apparmor/policy_unpack.c
> +++ b/security/apparmor/policy_unpack.c
> @@ -276,7 +276,7 @@ static bool unpack_nameX(struct aa_ext *e, enum aa_code
> code, const char *name)
> char *tag = NULL;
> size_t size = unpack_u16_chunk(e, );
> /* if a name is specified it must match. otherwise skip tag */
> - if (name && (!size || strcmp(name, tag)))
> + if (name && (!size || tag[size-1] != '\0' || strcmp(name, tag)))
> goto fail;
> } else if (name) {
> /* if a name is specified and there is no name tag fail */
>
Re: [PATCH] apparmor: Force type-casting of current->real_cred
On 4/23/19 9:53 AM, Bharath Vedartham wrote:
> This patch fixes the sparse warning:
> warning: cast removes address space '' of expression.
>
> Signed-off-by: Bharath Vedartham
Acked-by: John Johansen
I will pull this into my tree
> ---
> security/apparmor/lsm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 87500bd..36478c4 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -1529,7 +1529,7 @@ static int param_set_mode(const char *val, const struct
> kernel_param *kp)
> */
> static int __init set_init_ctx(void)
> {
> - struct cred *cred = (struct cred *)current->real_cred;
> + struct cred *cred = (__force struct cred *)current->real_cred;
>
> set_cred_label(cred, aa_get_label(ns_unconfined(root_ns)));
>
>
Acked-by: John Johansen
Re: kernel BUG at kernel/cred.c:434!
On 4/17/19 4:39 PM, Paul Moore wrote: > On Wed, Apr 17, 2019 at 12:27 PM Oleg Nesterov wrote: >> On 04/17, Paul Moore wrote: >>> >>> On Wed, Apr 17, 2019 at 10:57 AM Oleg Nesterov wrote: On 04/17, Paul Moore wrote: > > I'm tempted to simply return an error in selinux_setprocattr() if > the task's credentials are not the same as its real_cred; What about other modules? I have no idea what smack_setprocattr() is, but it too does prepare_creds/commit creds. it seems that the simplest workaround should simply add the additional cred == real_cred into proc_pid_attr_write(). >>> >>> Yes, that is simple, but I worry about what other LSMs might want to >>> do. While I believe failing if the effective creds are not the same >>> as the real_creds is okay for SELinux (possibly Smack too), I worry >>> about what other LSMs may want to do. After all, >>> proc_pid_attr_write() doesn't change the the creds itself, that is >>> something the specific LSMs do. >> >> Yes, but if proc_pid_attr_write() is called with cred != real_cred then >> something is already wrong? > > True, or at least I would think so. > > Looking at the current tree there are three LSMs which implement > setprocattr hooks: SELinux, Smack, and AppArmor. I know Casey has > already mentioned that he wasn't able to trigger the problem in Smack, > but looking at smack_setprocattr() I see the similar commit_creds() > usage so I would expect the same problem in Smack; what say you Casey? > Looking at apparmor_setprocattr(), it appears that it too could end > up calling commit_creds() via aa_set_current_hat(). > > Since it looks like all three LSMs which implement the setprocattr > hook are vulnerable I'm open to the idea that proc_pid_attr_write() is > a better choice for the cred != read_cred check, but I would want to > make sure John and Casey are okay with that. > > John? heh yeah, seems I messed up our check, we actually have if (current_cred() != current_real_cred()) return -EBUSY; on the change_profile path and missed it on the change_hat one. It makes sense to lift the check up earlier
Re: [PATCH] apparmor: fix spelling mistake "immutible" -> "immutable"
On 4/16/19 7:42 AM, Colin King wrote:
> From: Colin Ian King
>
> There is a spelling mistake in an information message string, fix it.
>
> Signed-off-by: Colin Ian King
Acked-by: John Johansen
I'll pull it into the apparmor tree
> ---
> security/apparmor/policy.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c
> index 71a3e6291478..04f2480e8374 100644
> --- a/security/apparmor/policy.c
> +++ b/security/apparmor/policy.c
> @@ -587,7 +587,7 @@ static int replacement_allowed(struct aa_profile
> *profile, int noreplace,
> {
> if (profile) {
> if (profile->label.flags & FLAG_IMMUTIBLE) {
> - *info = "cannot replace immutible profile";
> + *info = "cannot replace immutable profile";
> return -EPERM;
> } else if (noreplace) {
> *info = "profile already exists";
>
[GIT PULL] apparmor regression fix for v5.1-rc5
Hi Linus, Can you please pull the following regression fix for apparmor Thanks! - John The following changes since commit 771acc7e4a6e5dba779cb1a7fd851a164bc81033: Bluetooth: btusb: request wake pin with NOAUTOEN (2019-04-09 17:38:24 -1000) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor 5.1-regression-fix for you to fetch changes up to e33c1b9923775d17ad246946fe67fcb9be288677: apparmor: Restore Y/N in /sys for apparmor's "enabled" (2019-04-10 04:24:48 -0700) Kees Cook (1): apparmor: Restore Y/N in /sys for apparmor's "enabled" security/apparmor/lsm.c | 49 - 1 file changed, 48 insertions(+), 1 deletion(-)
Re: [PATCH] apparmor: Restore Y/N in /sys for apparmor's "enabled"
On 4/9/19 1:55 PM, Kees Cook wrote: > On Tue, Apr 9, 2019 at 1:12 PM James Morris wrote: >> Actually, JJ usually submits directly to Linus. > > Ah! Right; I forgot. John, can you take and send this? > yep, I'll send it up today
Re: [PATCH] apparmor: Restore Y/N in /sys for apparmor's "enabled"
On 4/9/19 1:11 PM, James Morris wrote:
> On Tue, 9 Apr 2019, Kees Cook wrote:
>
>> On Mon, Apr 8, 2019 at 11:21 PM David Rheinsberg
>> wrote:
>>>
>>> Hi
>>>
>>> On Mon, Apr 8, 2019 at 6:07 PM Kees Cook wrote:
Before commit c5459b829b71 ("LSM: Plumb visibility into optional "enabled"
state"), /sys/module/apparmor/parameters/enabled would show "Y" or "N"
since it was using the "bool" handler. After being changed to "int",
this switched to "1" or "0", breaking the userspace AppArmor detection
of dbus-broker. This restores the Y/N output while keeping the LSM
infrastructure happy.
Before:
$ cat /sys/module/apparmor/parameters/enabled
1
After:
$ cat /sys/module/apparmor/parameters/enabled
Y
Reported-by: David Rheinsberg
Link:
https://lkml.kernel.org/r/cadydso6k8vyb1eryt4g6+ehrlcvb68gabhvwuulkyjczcyn...@mail.gmail.com
Fixes: c5459b829b71 ("LSM: Plumb visibility into optional "enabled" state")
Signed-off-by: Kees Cook
---
This fix, if John is okay with it, is needed in v5.1 to correct the
userspace regression reported by David.
---
security/apparmor/lsm.c | 49 -
1 file changed, 48 insertions(+), 1 deletion(-)
>>>
>>> This looks good to me. Thanks a lot! If this makes v5.1, I will leave
>>> the apparmor-detection in dbus-broker as it is, unless someone asks me
>>> to parse 0/1 as well?
>>>
>>> I cannot judge whether the apparmor_initialized check is correct, but
>>> for the parameter parsing:
>>>
>>> Reviewed-by: David Rheinsberg
>>
>> Thanks!
>>
>> James, are you able to take this for v5.1 fixes?
>
> Actually, JJ usually submits directly to Linus.
>
yeah, I can push this up
Re: [PATCH] apparmor: Restore Y/N in /sys for apparmor's "enabled"
On 4/8/19 10:25 AM, Kees Cook wrote:
> On Mon, Apr 8, 2019 at 9:58 AM John Johansen
> wrote:
>>> +/* Can only be set before AppArmor is initialized (i.e. on boot cmdline).
>>> */
>>> +static int param_set_aaintbool(const char *val, const struct kernel_param
>>> *kp)
>>> +{
>>> + struct kernel_param kp_local;
>>> + bool value;
>>> + int error;
>>> +
>>> + if (apparmor_initialized)
>>> + return -EPERM;
>>> +
>> This isn't sufficient/correct. apparmor_initialized is only set after
>> apparmor has gone through and completed initialization. However if
>> apparmor is not selected as one of the LSMs to enable, then this check
>> won't stop apparmor_enabled from being set post boot.
>>
>> However with the apparmor_enabled param being 0444 and the
>> apparmor_enabled_setup() fn handling boot cmdline do with even need
>> the set parameter fn?
>
> Yup, that's true. I've gone and tested this, and yes, the 0444 is
> sufficient to protect the logic here (even if root chmods the inode).
> So the test here is redundant. However, very early in the threads
> about LSM boot cmdline enabling it was made clear that
> "apparmor.enabled=..." needed to stay working, which means the "set"
> op is still needed. (But I'm happy to do whatever you want here -- I
> was just trying to keep the functionality as it was.)
>
Right, though the legacy case that most document reference is apparmor=0/1
which is handled by __apparmor_enabled_setup()
still best not to break apparmor.enabled
> Should I send a v2 without the "initialized" check or is this okay to
> leave as-is with the redundant check?
>
redundant is fine, it will help protect against shooting ourselves in
the foot if some one ever tries changing the 0444
you can have my Acked-by: John Johansen
Re: [PATCH] apparmor: Restore Y/N in /sys for apparmor's "enabled"
On 4/8/19 9:07 AM, Kees Cook wrote:
> Before commit c5459b829b71 ("LSM: Plumb visibility into optional "enabled"
> state"), /sys/module/apparmor/parameters/enabled would show "Y" or "N"
> since it was using the "bool" handler. After being changed to "int",
> this switched to "1" or "0", breaking the userspace AppArmor detection
> of dbus-broker. This restores the Y/N output while keeping the LSM
> infrastructure happy.
>
> Before:
> $ cat /sys/module/apparmor/parameters/enabled
> 1
>
> After:
> $ cat /sys/module/apparmor/parameters/enabled
> Y
>
> Reported-by: David Rheinsberg
> Link:
> https://lkml.kernel.org/r/cadydso6k8vyb1eryt4g6+ehrlcvb68gabhvwuulkyjczcyn...@mail.gmail.com
> Fixes: c5459b829b71 ("LSM: Plumb visibility into optional "enabled" state")
> Signed-off-by: Kees Cook
> ---
> This fix, if John is okay with it, is needed in v5.1 to correct the
> userspace regression reported by David.
> ---
> security/apparmor/lsm.c | 49 -
> 1 file changed, 48 insertions(+), 1 deletion(-)
>
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 49d664ddff44..87500bde5a92 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -1336,9 +1336,16 @@ module_param_named(path_max, aa_g_path_max, aauint,
> S_IRUSR);
> bool aa_g_paranoid_load = true;
> module_param_named(paranoid_load, aa_g_paranoid_load, aabool, S_IRUGO);
>
> +static int param_get_aaintbool(char *buffer, const struct kernel_param *kp);
> +static int param_set_aaintbool(const char *val, const struct kernel_param
> *kp);
> +#define param_check_aaintbool param_check_int
> +static const struct kernel_param_ops param_ops_aaintbool = {
> + .set = param_set_aaintbool,
> + .get = param_get_aaintbool
> +};
> /* Boot time disable flag */
> static int apparmor_enabled __lsm_ro_after_init = 1;
> -module_param_named(enabled, apparmor_enabled, int, 0444);
> +module_param_named(enabled, apparmor_enabled, aaintbool, 0444);
>
> static int __init apparmor_enabled_setup(char *str)
> {
> @@ -1413,6 +1420,46 @@ static int param_get_aauint(char *buffer, const struct
> kernel_param *kp)
> return param_get_uint(buffer, kp);
> }
>
> +/* Can only be set before AppArmor is initialized (i.e. on boot cmdline). */
> +static int param_set_aaintbool(const char *val, const struct kernel_param
> *kp)
> +{
> + struct kernel_param kp_local;
> + bool value;
> + int error;
> +
> + if (apparmor_initialized)
> + return -EPERM;
> +
This isn't sufficient/correct. apparmor_initialized is only set after
apparmor has gone through and completed initialization. However if
apparmor is not selected as one of the LSMs to enable, then this check
won't stop apparmor_enabled from being set post boot.
However with the apparmor_enabled param being 0444 and the
apparmor_enabled_setup() fn handling boot cmdline do with even need
the set parameter fn?
> + /* Create local copy, with arg pointing to bool type. */
> + value = !!*((int *)kp->arg);
> + memcpy(_local, kp, sizeof(kp_local));
> + kp_local.arg =
> +
> + error = param_set_bool(val, _local);
> + if (!error)
> + *((int *)kp->arg) = *((bool *)kp_local.arg);
> + return error;
> +}
> +
> +/*
> + * To avoid changing /sys/module/apparmor/parameters/enabled from Y/N to
> + * 1/0, this converts the "int that is actually bool" back to bool for
> + * display in the /sys filesystem, while keeping it "int" for the LSM
> + * infrastructure.
> + */
> +static int param_get_aaintbool(char *buffer, const struct kernel_param *kp)
> +{
> + struct kernel_param kp_local;
> + bool value;
> +
> + /* Create local copy, with arg pointing to bool type. */
> + value = !!*((int *)kp->arg);
> + memcpy(_local, kp, sizeof(kp_local));
> + kp_local.arg =
> +
> + return param_get_bool(buffer, _local);
> +}
> +
> static int param_get_audit(char *buffer, const struct kernel_param *kp)
> {
> if (!apparmor_enabled)
>
[GIT PULL] apparmor updates for v5.1
Hi, Please pull these apparmor bugfixes for v5.1 Thanks! - John The following changes since commit 43aa09fee2f08c8d90a4f35d4c8c711362afcaee: apparmor: Fix warning about unused function apparmor_ipv6_postroute (2018-11-14 11:42:18 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor tags/apparmor-pr-2019-03-12 for you to fetch changes up to d8dbb581d4f86a2ac669c056fc71a28ebeb367f4: apparmor: fix double free when unpack of secmark rules fails (2019-03-12 03:48:02 -0700) + Bug Fixes - fix double when failing to unpack secmark rules in policy - fix leak of dentry when profile is removed Chris Coulson (1): apparmor: delete the dentry in aafs_remove() to avoid a leak John Johansen (1): apparmor: fix double free when unpack of secmark rules fails security/apparmor/apparmorfs.c| 1 + security/apparmor/policy_unpack.c | 1 + 2 files changed, 2 insertions(+)
Re: [PATCH] apparmor: fix build error undefined reference to zlib_*
On 2/12/19 1:48 AM, Anders Roxell wrote:
> With commit 876dd866c084 ("apparmor: Initial implementation of raw
> policy blob compression") and SECURITY_APPARMOR is set to '=y'
> ZLIB_DEFLATE must be enabled as well for the linker to see the symbols.
>
> aarch64-linux-gnu-ld: security/apparmor/policy_unpack.o: in function
> `deflate_compress':
> ../security/apparmor/policy_unpack.c:1030: undefined reference to
> `zlib_deflate_workspacesize'
> aarch64-linux-gnu-ld:
> ../security/apparmor/policy_unpack.c:1030:(.text+0xe20): relocation truncated
> to fit: R_AARCH64_CALL26 against undefined symbol `zlib_deflate_workspacesize'
> aarch64-linux-gnu-ld: ../security/apparmor/policy_unpack.c:1036: undefined
> reference to `zlib_deflateInit2'
> aarch64-linux-gnu-ld:
> ../security/apparmor/policy_unpack.c:1036:(.text+0xe60): relocation truncated
> to fit: R_AARCH64_CALL26 against undefined symbol `zlib_deflateInit2'
> aarch64-linux-gnu-ld: ../security/apparmor/policy_unpack.c:1053: undefined
> reference to `zlib_deflate'
> aarch64-linux-gnu-ld:
> ../security/apparmor/policy_unpack.c:1053:(.text+0xec0): relocation truncated
> to fit: R_AARCH64_CALL26 against undefined symbol `zlib_deflate'
> aarch64-linux-gnu-ld: ../security/apparmor/policy_unpack.c:1081: undefined
> reference to `zlib_deflateEnd'
> aarch64-linux-gnu-ld:
> ../security/apparmor/policy_unpack.c:1081:(.text+0xf90): relocation truncated
> to fit: R_AARCH64_CALL26 against undefined symbol `zlib_deflateEnd'
> aarch64-linux-gnu-ld: ../security/apparmor/policy_unpack.c:1081: undefined
> reference to `zlib_deflateEnd'
> aarch64-linux-gnu-ld:
> ../security/apparmor/policy_unpack.c:1081:(.text+0xfb4): relocation truncated
> to fit: R_AARCH64_CALL26 against undefined symbol `zlib_deflateEnd'
> aarch64-linux-gnu-ld: ../security/apparmor/policy_unpack.c:1081: undefined
> reference to `zlib_deflateEnd'
> aarch64-linux-gnu-ld:
> ../security/apparmor/policy_unpack.c:1081:(.text+0xfd8): relocation truncated
> to fit: R_AARCH64_CALL26 against undefined symbol `zlib_deflateEnd'
> make[1]: *** [Makefile:1023: vmlinux] Error 1
> make[1]: Target 'Image' not remade because of errors.
> make: *** [Makefile:152: sub-make] Error 2
> make: Target 'Image' not remade because of errors.
>
> Rework so when SECURITY_APPARMOR is set to '=y' ZLIB_INFLATE and ZLIB_DEFLATE
> gets
> selected, since both are used by the SECURITY_APPARMOR.
>
> Signed-off-by: Anders Roxell
yep thanks for the patch,
unfortunately this same fix landed in apparmor-next about 5 hours ago
> ---
> security/apparmor/Kconfig | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/security/apparmor/Kconfig b/security/apparmor/Kconfig
> index 3de21f46c82a..99c35e22c119 100644
> --- a/security/apparmor/Kconfig
> +++ b/security/apparmor/Kconfig
> @@ -5,6 +5,8 @@ config SECURITY_APPARMOR
> select SECURITY_PATH
> select SECURITYFS
> select SECURITY_NETWORK
> + select ZLIB_INFLATE
> + select ZLIB_DEFLATE
> default n
> help
> This enables the AppArmor security module.
>
[GIT PULL] apparmor fixes for 5.0-rc5
Can you please pull the following 2 bug fixes for apparmor The following changes since commit f17b5f06cb92ef2250513a1e154c47b78df07d40: Linux 5.0-rc4 (2019-01-27 15:18:05 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor tags/apparmor-pr-2019-02-01 for you to fetch changes up to d6d478aee003e19ef90321176552a8ad2929a47f: apparmor: Fix aa_label_build() error handling for failed merges (2019-02-01 08:01:39 -0800) Bug Fixes - Fix aa_label_build() error handling for failed merges - Fix warning about unused function apparmor_ipv6_postroute John Johansen (1): apparmor: Fix aa_label_build() error handling for failed merges Petr Vorel (1): apparmor: Fix warning about unused function apparmor_ipv6_postroute security/apparmor/domain.c | 5 - security/apparmor/lsm.c| 2 ++ 2 files changed, 6 insertions(+), 1 deletion(-)
Re: [PATCH] security: mark expected switch fall-throughs
On 1/24/19 6:56 PM, Gustavo A. R. Silva wrote: > In preparation to enabling -Wimplicit-fallthrough, mark switch > cases where we are expecting to fall through. > > This patch fixes the following warnings: > > security/integrity/ima/ima_appraise.c:116:26: warning: this statement may > fall through [-Wimplicit-fallthrough=] > security/integrity/ima/ima_template_lib.c:85:10: warning: this statement may > fall through [-Wimplicit-fallthrough=] > security/integrity/ima/ima_policy.c:940:18: warning: this statement may fall > through [-Wimplicit-fallthrough=] > security/integrity/ima/ima_policy.c:943:7: warning: this statement may fall > through [-Wimplicit-fallthrough=] > security/integrity/ima/ima_policy.c:972:21: warning: this statement may fall > through [-Wimplicit-fallthrough=] > security/integrity/ima/ima_policy.c:974:7: warning: this statement may fall > through [-Wimplicit-fallthrough=] > security/smack/smack_lsm.c:3391:9: warning: this statement may fall through > [-Wimplicit-fallthrough=] > security/apparmor/domain.c:569:6: warning: this statement may fall through > [-Wimplicit-fallthrough=] > > Warning level 3 was used: -Wimplicit-fallthrough=3 > > This patch is part of the ongoing efforts to enabling -Wimplicit-fallthrough. > > Signed-off-by: Gustavo A. R. Silva looks good to me Acked-by: John Johansen > --- > security/apparmor/domain.c| 2 +- > security/integrity/ima/ima_appraise.c | 1 + > security/integrity/ima/ima_policy.c | 4 > security/integrity/ima/ima_template_lib.c | 1 + > security/smack/smack_lsm.c| 3 +-- > 5 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c > index 726910bba84b..c7c619578095 100644 > --- a/security/apparmor/domain.c > +++ b/security/apparmor/domain.c > @@ -572,7 +572,7 @@ static struct aa_label *x_to_label(struct aa_profile > *profile, > stack = NULL; > break; > } > - /* fall through to X_NAME */ > + /* fall through - to X_NAME */ > case AA_X_NAME: > if (xindex & AA_X_CHILD) > /* released by caller */ > diff --git a/security/integrity/ima/ima_appraise.c > b/security/integrity/ima/ima_appraise.c > index a2baa85ea2f5..57daf30fb7d4 100644 > --- a/security/integrity/ima/ima_appraise.c > +++ b/security/integrity/ima/ima_appraise.c > @@ -114,6 +114,7 @@ static void ima_set_cache_status(struct > integrity_iint_cache *iint, > break; > case CREDS_CHECK: > iint->ima_creds_status = status; > + /* fall through */ > case FILE_CHECK: > case POST_SETATTR: > iint->ima_file_status = status; > diff --git a/security/integrity/ima/ima_policy.c > b/security/integrity/ima/ima_policy.c > index 8bc8a1c8cb3f..122797023bdb 100644 > --- a/security/integrity/ima/ima_policy.c > +++ b/security/integrity/ima/ima_policy.c > @@ -938,10 +938,12 @@ static int ima_parse_rule(char *rule, struct > ima_rule_entry *entry) > case Opt_uid_gt: > case Opt_euid_gt: > entry->uid_op = _gt; > + /* fall through */ > case Opt_uid_lt: > case Opt_euid_lt: > if ((token == Opt_uid_lt) || (token == Opt_euid_lt)) > entry->uid_op = _lt; > + /* fall through */ > case Opt_uid_eq: > case Opt_euid_eq: > uid_token = (token == Opt_uid_eq) || > @@ -970,9 +972,11 @@ static int ima_parse_rule(char *rule, struct > ima_rule_entry *entry) > break; > case Opt_fowner_gt: > entry->fowner_op = _gt; > + /* fall through */ > case Opt_fowner_lt: > if (token == Opt_fowner_lt) > entry->fowner_op = _lt; > + /* fall through */ > case Opt_fowner_eq: > ima_log_string_op(ab, "fowner", args[0].from, > entry->fowner_op); > diff --git a/security/integrity/ima/ima_template_lib.c > b/security/integrity/ima/ima_template_lib.c > index 43752002c222..513b457ae900 100644 > --- a/security/integrity/ima/ima_template_lib.c > +++ b/security/integrity/ima/ima_template_lib.c > @@ -83,6 +83,7 @@ static void ima_show_template_data_ascii(struct seq_file *m, > /* skip ':' and '\0' */ > buf_ptr += 2; >
Re: WARNING in apparmor_cred_free
On 1/16/19 1:14 PM, James Morris wrote:
> On Fri, 11 Jan 2019, Casey Schaufler wrote:
>
>> >From 47134986133c822e1d88860fa2b108f92c97a7ff Mon Sep 17 00:00:00 2001
>> From: Casey Schaufler
>> Date: Fri, 11 Jan 2019 17:31:50 -0800
>> Subject: [PATCH 1/2] LSM: Check for NULL cred-security on free
>>
>> Check that the cred security blob has been set before trying
>> to clean it up. There is a case during credential initialization
>> that could result in this.
>>
>> Signed-off-by: Casey Schaufler
>
> JJ: does this fix the problem?
>
sorry for not responding earlier, yes it does.
Acked-by: John Johansen
>> ---
>> security/security.c | 7 +++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/security/security.c b/security/security.c
>> index a618e22df5c6..7bffc86d4e87 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -1477,6 +1477,13 @@ int security_cred_alloc_blank(struct cred *cred,
>> gfp_t gfp)
>>
>> void security_cred_free(struct cred *cred)
>> {
>> +/*
>> + * There is a failure case in prepare_creds() that
>> + * may result in a call here with ->security being NULL.
>> + */
>> +if (unlikely(cred->security == NULL))
>> +return;
>> +
>> call_void_hook(cred_free, cred);
>>
>> kfree(cred->security);
>>
>
Re: [PATCH] security/apparmor/domain: use PTR_ERR_OR_ZERO
On 1/4/19 1:17 AM, Peng Hao wrote:
> The variable 'new' may be NULL, so use PTR_ERR_OR_ZERO instead
> of PTR_ERR.
>
> Signed-off-by: Peng Hao
yep that is a problem unfortunately the fix isn't quite right
we don't want to return 0 for an error here. Instead we can
do
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 08c88de0ffda..11975ec8d566 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -1444,7 +1444,10 @@ int aa_change_profile(const char *fqname, int flags)
new = aa_label_merge(label, target, GFP_KERNEL);
if (IS_ERR_OR_NULL(new)) {
info = "failed to build target label";
- error = PTR_ERR(new);
+ if (!new)
+ error = -ENOMEM;
+ else
+ error = PTR_ERR(new);
new = NULL;
perms.allow = 0;
goto audit;
I have pulled your patch into my apparmor tree with the above revision
> ---
> security/apparmor/domain.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index 08c88de..7663589 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -1444,7 +1444,7 @@ int aa_change_profile(const char *fqname, int flags)
> new = aa_label_merge(label, target, GFP_KERNEL);
> if (IS_ERR_OR_NULL(new)) {
> info = "failed to build target label";
> - error = PTR_ERR(new);
> + error = PTR_ERR_OR_ZERO(new);
> new = NULL;
> perms.allow = 0;
> goto audit;
>
Re: WARNING in apparmor_cred_free
On 1/11/19 2:11 PM, Casey Schaufler wrote: > On 1/11/2019 1:43 AM, syzbot wrote: >> Hello, >> >> syzbot found the following crash on: >> >> HEAD commit: b808822a75a3 Add linux-next specific files for 20190111 >> git tree: linux-next >> console output: https://syzkaller.appspot.com/x/log.txt?x=179c22f740 >> kernel config: https://syzkaller.appspot.com/x/.config?x=c052ead0aed5001b >> dashboard link: https://syzkaller.appspot.com/bug?extid=69ca07954461f189e808 >> compiler: gcc (GCC) 9.0.0 20181231 (experimental) >> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=162d947f40 >> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=139f6c3740 >> >> IMPORTANT: if you fix the bug, please add the following tag to the commit: >> Reported-by: syzbot+69ca07954461f189e...@syzkaller.appspotmail.com >> >> [ cut here ] >> AppArmor WARN cred_label: ((!blob)): >> WARNING: CPU: 0 PID: 0 at security/apparmor/include/cred.h:30 cred_label >> security/apparmor/include/cred.h:30 [inline] >> WARNING: CPU: 0 PID: 0 at security/apparmor/include/cred.h:30 >> apparmor_cred_free+0x12f/0x1a0 security/apparmor/lsm.c:62 >> Kernel panic - not syncing: panic_on_warn set ... >> CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.0.0-rc1-next-20190111 #10 >> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS >> Google 01/01/2011 >> Call Trace: >> >> __dump_stack lib/dump_stack.c:77 [inline] >> dump_stack+0x1db/0x2d0 lib/dump_stack.c:113 >> panic+0x2cb/0x65c kernel/panic.c:214 >> __warn.cold+0x20/0x48 kernel/panic.c:571 >> report_bug+0x263/0x2b0 lib/bug.c:186 >> fixup_bug arch/x86/kernel/traps.c:178 [inline] >> fixup_bug arch/x86/kernel/traps.c:173 [inline] >> do_error_trap+0x11b/0x200 arch/x86/kernel/traps.c:271 >> do_invalid_op+0x37/0x50 arch/x86/kernel/traps.c:290 >> invalid_op+0x14/0x20 arch/x86/entry/entry_64.S:973 >> RIP: 0010:cred_label security/apparmor/include/cred.h:30 [inline] >> RIP: 0010:apparmor_cred_free+0x12f/0x1a0 security/apparmor/lsm.c:62 >> Code: 7c 88 48 c7 c7 00 d0 7c 88 e8 fd 70 f2 fd 0f 0b eb a9 e8 54 3f 29 fe >> 48 c7 c6 c0 df 7c 88 48 c7 c7 00 d0 7c 88 e8 e1 70 f2 fd <0f> 0b 48 b8 00 00 >> 00 00 00 fc ff df 80 38 00 75 4a 4c 8b 2c 25 00 >> RSP: 0018:8880ae6079f8 EFLAGS: 00010286 >> RAX: RBX: RCX: >> RDX: 0100 RSI: 81687fa6 RDI: 0006 >> RBP: 8880ae607a18 R08: 8987dec0 R09: >> R10: R11: R12: 8880a86b3100 >> R13: 8880a86b3100 R14: 8880a86b3188 R15: dc00 >> security_cred_free+0x4b/0xf0 security/security.c:1490 > > The obvious thing to do is put a check in security_cred_free > for a NULL cred->security, in which case the LSM hooks > wouldn't get called. Right, but the question is should we? To my thinking we shouldn't ever have a cred without cred->security, unless the cred was allocated but a later step in its construction, say allocating ->security failed. In which case I'd rather see the cred directly freed and not call into security_cred_free() as I like being able to detect corrupt creds. We certainly can still do the check for security on only live creds but I would like to understand this particular failure better first > It's not clear to me how we got a cred > that doesn't have an allocated security blob. I have been trying to figure that one out as well. > >> put_cred_rcu+0x21f/0x6e0 kernel/cred.c:118 >> __rcu_reclaim kernel/rcu/rcu.h:240 [inline] >> rcu_do_batch kernel/rcu/tree.c:2486 [inline] >> invoke_rcu_callbacks kernel/rcu/tree.c:2799 [inline] >> rcu_core+0xc4a/0x1680 kernel/rcu/tree.c:2780 >> __do_softirq+0x30b/0xb11 kernel/softirq.c:292 >> invoke_softirq kernel/softirq.c:373 [inline] >> irq_exit+0x180/0x1d0 kernel/softirq.c:413 >> exiting_irq arch/x86/include/asm/apic.h:536 [inline] >> smp_apic_timer_interrupt+0x1b7/0x760 arch/x86/kernel/apic/apic.c:1062 >> apic_timer_interrupt+0xf/0x20 arch/x86/entry/entry_64.S:807 >> >> RIP: 0010:native_safe_halt+0x2/0x10 arch/x86/include/asm/irqflags.h:58 >> Code: ff ff ff 48 89 c7 48 89 45 d8 e8 79 6f d0 f9 48 8b 45 d8 e9 ce fe ff >> ff 48 89 df e8 68 6f d0 f9 eb 82 90 90 90 90 90 90 fb f4 0f 1f 00 66 2e >> 0f 1f 84 00 00 00 00 00 f4 c3 90 90 90 90 90 90 >> RSP: 0018:89807c60 EFLAGS: 0282 ORIG_RAX: ff13 >> RAX: 11325061 RBX: 11300f8f RCX: >> RDX: dc00 RSI: 0001 RDI: 8987e73c >> RBP: 89807d20 R08: 8987dec0 R09: >> R10: R11: R12: >> R13: 89807cf8 R14: R15: 899282f8 >> arch_cpu_idle+0x10/0x20 arch/x86/kernel/process.c:555 >> default_idle_call+0x36/0x90 kernel/sched/idle.c:93 >> cpuidle_idle_call kernel/sched/idle.c:153 [inline] >> do_idle+0x386/0x5d0
[GIT PULL] apparmor updates for v4.20
Hi, Please pull these apparmor changes for v4.20. Thanks! - John The following changes since commit fb7d1bcf1602b46f37ada72178516c01a250e434: Merge tag 'pci-v4.18-fixes-3' of git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci (2018-07-19 11:54:04 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor tags/apparmor-pr-2018-11-01 for you to fetch changes up to 566f52ece7bd1099d20dfe2f6f0801896643cf8f: apparmor: clean an indentation issue, remove extraneous space (2018-11-01 22:34:25 -0700) + Features/Improvements - replace spin_is_locked() with lockdep - add base support for secmark labeling and matching + Cleanups - clean an indentation issue, remove extraneous space - remove no-op permission check in policy_unpack - fix checkpatch missing spaces error in Parse secmark policy - fix network performance issue in aa_label_sk_perm + Bug fixes - add #ifdef checks for secmark filtering - fix an error code in __aa_create_ns() - don't try to replace stale label in ptrace checks - fix failure to audit context info in build_change_hat - check buffer bounds when mapping permissions mask - fully initialize aa_perms struct when answering userspace query - fix uninitialized value in aa_split_fqname Arnd Bergmann (1): apparmor: add #ifdef checks for secmark filtering Colin Ian King (1): apparmor: clean an indentation issue, remove extraneous space Dan Carpenter (1): apparmor: fix an error code in __aa_create_ns() Jann Horn (2): apparmor: don't try to replace stale label in ptrace access check apparmor: don't try to replace stale label in ptraceme check John Johansen (3): apparmor: Fix failure to audit context info in build_change_hat apparmor: remove no-op permission check in policy_unpack apparmor: fix checkpatch error in Parse secmark policy Lance Roy (1): apparmor: Replace spin_is_locked() with lockdep Matthew Garrett (3): apparmor: Add a wildcard secid apparmor: Parse secmark policy apparmor: Allow filtering based on secmark policy Tony Jones (1): apparmor: Fix network performance issue in aa_label_sk_perm Tyler Hicks (2): apparmor: Check buffer bounds when mapping permissions mask apparmor: Fully initialize aa_perms struct when answering userspace query Zubin Mithra (1): apparmor: Fix uninitialized value in aa_split_fqname security/apparmor/apparmorfs.c | 7 +- security/apparmor/domain.c | 2 +- security/apparmor/file.c | 5 +- security/apparmor/include/cred.h | 2 + security/apparmor/include/net.h| 10 +++ security/apparmor/include/perms.h | 3 +- security/apparmor/include/policy.h | 3 + security/apparmor/include/secid.h | 3 + security/apparmor/lib.c| 23 +-- security/apparmor/lsm.c| 130 +++-- security/apparmor/net.c| 83 +-- security/apparmor/policy.c | 3 + security/apparmor/policy_ns.c | 2 +- security/apparmor/policy_unpack.c | 93 +- security/apparmor/secid.c | 3 +- 15 files changed, 311 insertions(+), 61 deletions(-)
[GIT PULL] apparmor updates for v4.20
Hi, Please pull these apparmor changes for v4.20. Thanks! - John The following changes since commit fb7d1bcf1602b46f37ada72178516c01a250e434: Merge tag 'pci-v4.18-fixes-3' of git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci (2018-07-19 11:54:04 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor tags/apparmor-pr-2018-11-01 for you to fetch changes up to 566f52ece7bd1099d20dfe2f6f0801896643cf8f: apparmor: clean an indentation issue, remove extraneous space (2018-11-01 22:34:25 -0700) + Features/Improvements - replace spin_is_locked() with lockdep - add base support for secmark labeling and matching + Cleanups - clean an indentation issue, remove extraneous space - remove no-op permission check in policy_unpack - fix checkpatch missing spaces error in Parse secmark policy - fix network performance issue in aa_label_sk_perm + Bug fixes - add #ifdef checks for secmark filtering - fix an error code in __aa_create_ns() - don't try to replace stale label in ptrace checks - fix failure to audit context info in build_change_hat - check buffer bounds when mapping permissions mask - fully initialize aa_perms struct when answering userspace query - fix uninitialized value in aa_split_fqname Arnd Bergmann (1): apparmor: add #ifdef checks for secmark filtering Colin Ian King (1): apparmor: clean an indentation issue, remove extraneous space Dan Carpenter (1): apparmor: fix an error code in __aa_create_ns() Jann Horn (2): apparmor: don't try to replace stale label in ptrace access check apparmor: don't try to replace stale label in ptraceme check John Johansen (3): apparmor: Fix failure to audit context info in build_change_hat apparmor: remove no-op permission check in policy_unpack apparmor: fix checkpatch error in Parse secmark policy Lance Roy (1): apparmor: Replace spin_is_locked() with lockdep Matthew Garrett (3): apparmor: Add a wildcard secid apparmor: Parse secmark policy apparmor: Allow filtering based on secmark policy Tony Jones (1): apparmor: Fix network performance issue in aa_label_sk_perm Tyler Hicks (2): apparmor: Check buffer bounds when mapping permissions mask apparmor: Fully initialize aa_perms struct when answering userspace query Zubin Mithra (1): apparmor: Fix uninitialized value in aa_split_fqname security/apparmor/apparmorfs.c | 7 +- security/apparmor/domain.c | 2 +- security/apparmor/file.c | 5 +- security/apparmor/include/cred.h | 2 + security/apparmor/include/net.h| 10 +++ security/apparmor/include/perms.h | 3 +- security/apparmor/include/policy.h | 3 + security/apparmor/include/secid.h | 3 + security/apparmor/lib.c| 23 +-- security/apparmor/lsm.c| 130 +++-- security/apparmor/net.c| 83 +-- security/apparmor/policy.c | 3 + security/apparmor/policy_ns.c | 2 +- security/apparmor/policy_unpack.c | 93 +- security/apparmor/secid.c | 3 +- 15 files changed, 311 insertions(+), 61 deletions(-)
Re: [PATCH] apparmor: clean an indentation issue, remove extraneous space
On 10/30/18 7:11 AM, Colin King wrote: > From: Colin Ian King > > Trivial fix to clean up an indentation issue, remove space > > Signed-off-by: Colin Ian King Thanks Colin, I have pulled this into apparmor-next > --- > security/apparmor/apparmorfs.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c > index 658b85639006..33f89b3f28a4 100644 > --- a/security/apparmor/apparmorfs.c > +++ b/security/apparmor/apparmorfs.c > @@ -1744,7 +1744,7 @@ static int ns_rmdir_op(struct inode *dir, struct dentry > *dentry) > if (error) > return error; > > - parent = aa_get_ns(dir->i_private); > + parent = aa_get_ns(dir->i_private); > /* rmdir calls the generic securityfs functions to remove files >* from the apparmor dir. It is up to the apparmor ns locking >* to avoid races. >
Re: [PATCH] apparmor: clean an indentation issue, remove extraneous space
On 10/30/18 7:11 AM, Colin King wrote: > From: Colin Ian King > > Trivial fix to clean up an indentation issue, remove space > > Signed-off-by: Colin Ian King Thanks Colin, I have pulled this into apparmor-next > --- > security/apparmor/apparmorfs.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c > index 658b85639006..33f89b3f28a4 100644 > --- a/security/apparmor/apparmorfs.c > +++ b/security/apparmor/apparmorfs.c > @@ -1744,7 +1744,7 @@ static int ns_rmdir_op(struct inode *dir, struct dentry > *dentry) > if (error) > return error; > > - parent = aa_get_ns(dir->i_private); > + parent = aa_get_ns(dir->i_private); > /* rmdir calls the generic securityfs functions to remove files >* from the apparmor dir. It is up to the apparmor ns locking >* to avoid races. >
Re: [PATCH security-next v5 00/30] LSM: Explict ordering
On 10/12/2018 04:31 AM, Jordan Glover wrote: > ‐‐‐ Original Message ‐‐‐ > On Friday, October 12, 2018 2:26 AM, John Johansen > wrote: > >> On 10/11/2018 04:53 PM, Jordan Glover wrote: >> >>> ‐‐‐ Original Message ‐‐‐ >>> On Friday, October 12, 2018 1:09 AM, Kees Cook keesc...@chromium.org wrote: >>> >>>> We've had things sort of like this proposed, but if you can convince >>>> James and others, I'm all for it. I think the standing objection from >>>> James and John about this is that the results of booting with >>>> "lsm=something" ends up depending on CONFIG_LSM= for that distro. So >>>> you end up with different behaviors instead of a consistent behavior >>>> across all distros. >>> >>> Ok, I'll try :) >>> The final lsm string contains two parts: Kconfig "CONFIG_LSM=" and boot >>> param "lsm=". Changing even only one of those parts also changes the >>> final string. >>> In case of distros, it's the "CONFIG_LSM=" which changes. Even when "lsm=" >>> stays constant, the behavior will be different, example: >>> Distro A has: CONFIG_LSM=loadpin,integrity,selinux >>> Distro B has CONFIG_LSM=yama,loadpin,integrity,selinux >>> User on distro A wants to enable apparmor with: >>> lsm=loadpin,integrity,apparmor >>> which they do and add it to howto on wiki. >>> User on distro B want to enable apparmor, they found info on some wiki and >>> do: >>> lsm=loadpin,integrity,apparmor >>> Puff, yama got disabled! >>> Above example shows why I think "consistent behavior across all distros" >>> argument for current approach is flawed - because distros aren't >>> consistent. In my proposition the user will just use "lsm=apparmor" and >>> it will consistently enable apparmor on all distros which is what they >>> really wanted, but all pre-existing differences across distros will >>> remain unchanged. >> >> Are you sure about that? I have had more than one question about >> security=X resulting in a system with more than just X enabled. Ie why >> is yama enabled when I specifically set security to X. >> > > So, non-explicit list will match current "security=" behavior which users > are more familiar with. The current answer for this question is "because > your distro enabled it and you didn't disabled it. With non-explcit list > that answer will stay the same. > the current behavior is problematic leads to a configuration nightmare, and is the reason lsm= is proposed instead of just sticking with security= > With explicit list, the question will be "why is yama disabled when I > enabled AppArmor with lsm=apparmor". > yes that will happen as well > To ask both questions user have to know that something like "yama" exist > in first place. > yes. However when it comes to security I don't think its too insane to want to vet new modules before they become part of your configuration. This is something distros want to be able to do and something some users want. I am not claiming this is what all users will want, and it certainly isn't the best situation for the adoption of new lsms. But is a very understandable security policy stance. > As for question what users typically want you may look at search results > for "disable/enable yama" and "disable/enable apparmor/selinux". The > difference is several orders of magnitude. That's why I think typical user > just want to switch on/off one major lsm. I don't think anecdotal evidence > is representative here. > >> There will certainly be cases where what you describe is exactly what >> the user wants. The problem is an explosion of options isn't good >> for the user either. >> >> What I want at the moment is the discussion about different ways to >> enable LSMs to be split off so this work can move forward. >> >>> The current approach requires that everyone who dares to touch "lsm=" >>> knows about existence of all lsm, their enabled/disabled status on >>> target distro and their order. I doubt there are many people other >>> than recipients of this mail who fit for the above. >> >> Without having gotten a chance to review the current set of patches >> that was not what was discussed, it should only requires they know the >> set that they want. >> > > "it should only requires they know the set that they want" is very > hard requirement and I don't think most users will pass this. > Especially when sets like: > > lsm=yama,loadpin,integrity,apparmor > lsm=loadpin,integrity,yama,apparmor > > will behave differently. > yes, that is a problem and it highlights the complexity of the problem we are dealing with. Your proposal tries to hide the ordering issues from the user but they still suffer from the potentially different behavior of list ordering as it is moving the lsm around in the list. fwiw kees finally convinced me that having the order set separate from enablement in the kconfig is better for the user because of problems like this.
Re: [PATCH security-next v5 00/30] LSM: Explict ordering
On 10/12/2018 04:31 AM, Jordan Glover wrote: > ‐‐‐ Original Message ‐‐‐ > On Friday, October 12, 2018 2:26 AM, John Johansen > wrote: > >> On 10/11/2018 04:53 PM, Jordan Glover wrote: >> >>> ‐‐‐ Original Message ‐‐‐ >>> On Friday, October 12, 2018 1:09 AM, Kees Cook keesc...@chromium.org wrote: >>> >>>> We've had things sort of like this proposed, but if you can convince >>>> James and others, I'm all for it. I think the standing objection from >>>> James and John about this is that the results of booting with >>>> "lsm=something" ends up depending on CONFIG_LSM= for that distro. So >>>> you end up with different behaviors instead of a consistent behavior >>>> across all distros. >>> >>> Ok, I'll try :) >>> The final lsm string contains two parts: Kconfig "CONFIG_LSM=" and boot >>> param "lsm=". Changing even only one of those parts also changes the >>> final string. >>> In case of distros, it's the "CONFIG_LSM=" which changes. Even when "lsm=" >>> stays constant, the behavior will be different, example: >>> Distro A has: CONFIG_LSM=loadpin,integrity,selinux >>> Distro B has CONFIG_LSM=yama,loadpin,integrity,selinux >>> User on distro A wants to enable apparmor with: >>> lsm=loadpin,integrity,apparmor >>> which they do and add it to howto on wiki. >>> User on distro B want to enable apparmor, they found info on some wiki and >>> do: >>> lsm=loadpin,integrity,apparmor >>> Puff, yama got disabled! >>> Above example shows why I think "consistent behavior across all distros" >>> argument for current approach is flawed - because distros aren't >>> consistent. In my proposition the user will just use "lsm=apparmor" and >>> it will consistently enable apparmor on all distros which is what they >>> really wanted, but all pre-existing differences across distros will >>> remain unchanged. >> >> Are you sure about that? I have had more than one question about >> security=X resulting in a system with more than just X enabled. Ie why >> is yama enabled when I specifically set security to X. >> > > So, non-explicit list will match current "security=" behavior which users > are more familiar with. The current answer for this question is "because > your distro enabled it and you didn't disabled it. With non-explcit list > that answer will stay the same. > the current behavior is problematic leads to a configuration nightmare, and is the reason lsm= is proposed instead of just sticking with security= > With explicit list, the question will be "why is yama disabled when I > enabled AppArmor with lsm=apparmor". > yes that will happen as well > To ask both questions user have to know that something like "yama" exist > in first place. > yes. However when it comes to security I don't think its too insane to want to vet new modules before they become part of your configuration. This is something distros want to be able to do and something some users want. I am not claiming this is what all users will want, and it certainly isn't the best situation for the adoption of new lsms. But is a very understandable security policy stance. > As for question what users typically want you may look at search results > for "disable/enable yama" and "disable/enable apparmor/selinux". The > difference is several orders of magnitude. That's why I think typical user > just want to switch on/off one major lsm. I don't think anecdotal evidence > is representative here. > >> There will certainly be cases where what you describe is exactly what >> the user wants. The problem is an explosion of options isn't good >> for the user either. >> >> What I want at the moment is the discussion about different ways to >> enable LSMs to be split off so this work can move forward. >> >>> The current approach requires that everyone who dares to touch "lsm=" >>> knows about existence of all lsm, their enabled/disabled status on >>> target distro and their order. I doubt there are many people other >>> than recipients of this mail who fit for the above. >> >> Without having gotten a chance to review the current set of patches >> that was not what was discussed, it should only requires they know the >> set that they want. >> > > "it should only requires they know the set that they want" is very > hard requirement and I don't think most users will pass this. > Especially when sets like: > > lsm=yama,loadpin,integrity,apparmor > lsm=loadpin,integrity,yama,apparmor > > will behave differently. > yes, that is a problem and it highlights the complexity of the problem we are dealing with. Your proposal tries to hide the ordering issues from the user but they still suffer from the potentially different behavior of list ordering as it is moving the lsm around in the list. fwiw kees finally convinced me that having the order set separate from enablement in the kconfig is better for the user because of problems like this.
Re: [PATCH] apparmor: add #ifdef checks for secmark filtering
On 10/05/2018 09:11 AM, Arnd Bergmann wrote:
> The newly added code fails to build when either SECMARK or
> NETFILTER are disabled:
>
> security/apparmor/lsm.c: In function 'apparmor_socket_sock_rcv_skb':
> security/apparmor/lsm.c:1138:12: error: 'struct sk_buff' has no member named
> 'secmark'; did you mean 'mark'?
>
> security/apparmor/lsm.c:1671:21: error: 'struct nf_hook_state' declared
> inside parameter list will not be visible outside of this definition or
> declaration [-Werror]
>
> Add a set of #ifdef checks around it to only enable the code that
> we can compile and that makes sense in that configuration.
>
> Fixes: ab9f2115081a ("apparmor: Allow filtering based on secmark policy")
> Signed-off-by: Arnd Bergmann
Thanks Arnd, I have pulled this into apparmor-next
> ---
> security/apparmor/lsm.c | 10 ++
> security/apparmor/net.c | 2 ++
> 2 files changed, 12 insertions(+)
>
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 53201013c40e..b74b724d3e84 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -1123,6 +1123,7 @@ static int apparmor_socket_shutdown(struct socket
> *sock, int how)
> return aa_sock_perm(OP_SHUTDOWN, AA_MAY_SHUTDOWN, sock);
> }
>
> +#ifdef CONFIG_NETWORK_SECMARK
> /**
> * apparmor_socket_sock_recv_skb - check perms before associating skb to sk
> *
> @@ -1141,6 +1142,7 @@ static int apparmor_socket_sock_rcv_skb(struct sock
> *sk, struct sk_buff *skb)
> return apparmor_secmark_check(ctx->label, OP_RECVMSG, AA_MAY_RECEIVE,
> skb->secmark, sk);
> }
> +#endif
>
>
> static struct aa_label *sk_peer_label(struct sock *sk)
> @@ -1235,6 +1237,7 @@ static void apparmor_sock_graft(struct sock *sk, struct
> socket *parent)
> ctx->label = aa_get_current_label();
> }
>
> +#ifdef CONFIG_NETWORK_SECMARK
> static int apparmor_inet_conn_request(struct sock *sk, struct sk_buff *skb,
> struct request_sock *req)
> {
> @@ -1246,6 +1249,7 @@ static int apparmor_inet_conn_request(struct sock *sk,
> struct sk_buff *skb,
> return apparmor_secmark_check(ctx->label, OP_CONNECT, AA_MAY_CONNECT,
> skb->secmark, sk);
> }
> +#endif
>
> static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(ptrace_access_check, apparmor_ptrace_access_check),
> @@ -1304,13 +1308,17 @@ static struct security_hook_list apparmor_hooks[]
> __lsm_ro_after_init = {
> LSM_HOOK_INIT(socket_getsockopt, apparmor_socket_getsockopt),
> LSM_HOOK_INIT(socket_setsockopt, apparmor_socket_setsockopt),
> LSM_HOOK_INIT(socket_shutdown, apparmor_socket_shutdown),
> +#ifdef CONFIG_NETWORK_SECMARK
> LSM_HOOK_INIT(socket_sock_rcv_skb, apparmor_socket_sock_rcv_skb),
> +#endif
> LSM_HOOK_INIT(socket_getpeersec_stream,
> apparmor_socket_getpeersec_stream),
> LSM_HOOK_INIT(socket_getpeersec_dgram,
> apparmor_socket_getpeersec_dgram),
> LSM_HOOK_INIT(sock_graft, apparmor_sock_graft),
> +#ifdef CONFIG_NETWORK_SECMARK
> LSM_HOOK_INIT(inet_conn_request, apparmor_inet_conn_request),
> +#endif
>
> LSM_HOOK_INIT(cred_alloc_blank, apparmor_cred_alloc_blank),
> LSM_HOOK_INIT(cred_free, apparmor_cred_free),
> @@ -1666,6 +1674,7 @@ static inline int apparmor_init_sysctl(void)
> }
> #endif /* CONFIG_SYSCTL */
>
> +#if defined(CONFIG_NETFILTER) && defined(CONFIG_NETWORK_SECMARK)
> static unsigned int apparmor_ip_postroute(void *priv,
> struct sk_buff *skb,
> const struct nf_hook_state *state)
> @@ -1754,6 +1763,7 @@ static int __init apparmor_nf_ip_init(void)
> return 0;
> }
> __initcall(apparmor_nf_ip_init);
> +#endif
>
> static int __init apparmor_init(void)
> {
> diff --git a/security/apparmor/net.c b/security/apparmor/net.c
> index f9a678ce994f..c07fde444792 100644
> --- a/security/apparmor/net.c
> +++ b/security/apparmor/net.c
> @@ -190,6 +190,7 @@ int aa_sock_file_perm(struct aa_label *label, const char
> *op, u32 request,
> return aa_label_sk_perm(label, op, request, sock->sk);
> }
>
> +#ifdef CONFIG_NETWORK_SECMARK
> static int apparmor_secmark_init(struct aa_secmark *secmark)
> {
> struct aa_label *label;
> @@ -254,3 +255,4 @@ int apparmor_secmark_check(struct aa_label *label, char
> *op, u32 request,
> aa_secmark_perm(profile, request, secid,
> , sk));
> }
> +#endif
>
Re: [PATCH] apparmor: add #ifdef checks for secmark filtering
On 10/05/2018 09:11 AM, Arnd Bergmann wrote:
> The newly added code fails to build when either SECMARK or
> NETFILTER are disabled:
>
> security/apparmor/lsm.c: In function 'apparmor_socket_sock_rcv_skb':
> security/apparmor/lsm.c:1138:12: error: 'struct sk_buff' has no member named
> 'secmark'; did you mean 'mark'?
>
> security/apparmor/lsm.c:1671:21: error: 'struct nf_hook_state' declared
> inside parameter list will not be visible outside of this definition or
> declaration [-Werror]
>
> Add a set of #ifdef checks around it to only enable the code that
> we can compile and that makes sense in that configuration.
>
> Fixes: ab9f2115081a ("apparmor: Allow filtering based on secmark policy")
> Signed-off-by: Arnd Bergmann
Thanks Arnd, I have pulled this into apparmor-next
> ---
> security/apparmor/lsm.c | 10 ++
> security/apparmor/net.c | 2 ++
> 2 files changed, 12 insertions(+)
>
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 53201013c40e..b74b724d3e84 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -1123,6 +1123,7 @@ static int apparmor_socket_shutdown(struct socket
> *sock, int how)
> return aa_sock_perm(OP_SHUTDOWN, AA_MAY_SHUTDOWN, sock);
> }
>
> +#ifdef CONFIG_NETWORK_SECMARK
> /**
> * apparmor_socket_sock_recv_skb - check perms before associating skb to sk
> *
> @@ -1141,6 +1142,7 @@ static int apparmor_socket_sock_rcv_skb(struct sock
> *sk, struct sk_buff *skb)
> return apparmor_secmark_check(ctx->label, OP_RECVMSG, AA_MAY_RECEIVE,
> skb->secmark, sk);
> }
> +#endif
>
>
> static struct aa_label *sk_peer_label(struct sock *sk)
> @@ -1235,6 +1237,7 @@ static void apparmor_sock_graft(struct sock *sk, struct
> socket *parent)
> ctx->label = aa_get_current_label();
> }
>
> +#ifdef CONFIG_NETWORK_SECMARK
> static int apparmor_inet_conn_request(struct sock *sk, struct sk_buff *skb,
> struct request_sock *req)
> {
> @@ -1246,6 +1249,7 @@ static int apparmor_inet_conn_request(struct sock *sk,
> struct sk_buff *skb,
> return apparmor_secmark_check(ctx->label, OP_CONNECT, AA_MAY_CONNECT,
> skb->secmark, sk);
> }
> +#endif
>
> static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(ptrace_access_check, apparmor_ptrace_access_check),
> @@ -1304,13 +1308,17 @@ static struct security_hook_list apparmor_hooks[]
> __lsm_ro_after_init = {
> LSM_HOOK_INIT(socket_getsockopt, apparmor_socket_getsockopt),
> LSM_HOOK_INIT(socket_setsockopt, apparmor_socket_setsockopt),
> LSM_HOOK_INIT(socket_shutdown, apparmor_socket_shutdown),
> +#ifdef CONFIG_NETWORK_SECMARK
> LSM_HOOK_INIT(socket_sock_rcv_skb, apparmor_socket_sock_rcv_skb),
> +#endif
> LSM_HOOK_INIT(socket_getpeersec_stream,
> apparmor_socket_getpeersec_stream),
> LSM_HOOK_INIT(socket_getpeersec_dgram,
> apparmor_socket_getpeersec_dgram),
> LSM_HOOK_INIT(sock_graft, apparmor_sock_graft),
> +#ifdef CONFIG_NETWORK_SECMARK
> LSM_HOOK_INIT(inet_conn_request, apparmor_inet_conn_request),
> +#endif
>
> LSM_HOOK_INIT(cred_alloc_blank, apparmor_cred_alloc_blank),
> LSM_HOOK_INIT(cred_free, apparmor_cred_free),
> @@ -1666,6 +1674,7 @@ static inline int apparmor_init_sysctl(void)
> }
> #endif /* CONFIG_SYSCTL */
>
> +#if defined(CONFIG_NETFILTER) && defined(CONFIG_NETWORK_SECMARK)
> static unsigned int apparmor_ip_postroute(void *priv,
> struct sk_buff *skb,
> const struct nf_hook_state *state)
> @@ -1754,6 +1763,7 @@ static int __init apparmor_nf_ip_init(void)
> return 0;
> }
> __initcall(apparmor_nf_ip_init);
> +#endif
>
> static int __init apparmor_init(void)
> {
> diff --git a/security/apparmor/net.c b/security/apparmor/net.c
> index f9a678ce994f..c07fde444792 100644
> --- a/security/apparmor/net.c
> +++ b/security/apparmor/net.c
> @@ -190,6 +190,7 @@ int aa_sock_file_perm(struct aa_label *label, const char
> *op, u32 request,
> return aa_label_sk_perm(label, op, request, sock->sk);
> }
>
> +#ifdef CONFIG_NETWORK_SECMARK
> static int apparmor_secmark_init(struct aa_secmark *secmark)
> {
> struct aa_label *label;
> @@ -254,3 +255,4 @@ int apparmor_secmark_check(struct aa_label *label, char
> *op, u32 request,
> aa_secmark_perm(profile, request, secid,
> , sk));
> }
> +#endif
>
Re: [PATCH security-next v4 23/32] selinux: Remove boot parameter
On 10/02/2018 05:12 PM, Kees Cook wrote: > On Tue, Oct 2, 2018 at 5:05 PM, John Johansen > wrote: >> On 10/02/2018 04:54 PM, Kees Cook wrote: >>> That's not how I have it currently. It's a comma-separated a string, >>> including the reserved name "all". The default would just be >>> "CONFIG_LSM_ENABLE=all". Casey and I wanted this to have a way to >>> capture new LSMs by default at build-time. >>> >> >> I understand where you are coming from, but speaking with my distro >> hat on, that is not going to work. As a distro Ubuntu very much wants >> to be able to offer all the LSMs built in to the kernel so the user >> can select them. But very much wants to be able to specify a default >> supported subset that is enabled at boot. >> >> I expect RH and Suse will feel similarily. Speaking for Ubuntu if this >> isn't available as part of lsm stacking it will get distro patched in. > > Right. Ubuntu would do something like: > > CONFIG_LSM_ENABLE=yama,apparmor,integrity > > And that's why I wanted non-explicit lsm.enable, so that an end user > could just do: > > lsm.enable=loadpin > > to add loadpin. > > Perhaps we could have both? "lsm.enable=+loadpin" (add loadpin to > build default list) vs "lsm.enable=loadpin" (override build default > list with ONLY loadpin). > Maybe? I'm not sure what the best option is with all the competing requirements/desires. I need to think about it more and would like to see what others think.
Re: [PATCH security-next v4 23/32] selinux: Remove boot parameter
On 10/02/2018 05:12 PM, Kees Cook wrote: > On Tue, Oct 2, 2018 at 5:05 PM, John Johansen > wrote: >> On 10/02/2018 04:54 PM, Kees Cook wrote: >>> That's not how I have it currently. It's a comma-separated a string, >>> including the reserved name "all". The default would just be >>> "CONFIG_LSM_ENABLE=all". Casey and I wanted this to have a way to >>> capture new LSMs by default at build-time. >>> >> >> I understand where you are coming from, but speaking with my distro >> hat on, that is not going to work. As a distro Ubuntu very much wants >> to be able to offer all the LSMs built in to the kernel so the user >> can select them. But very much wants to be able to specify a default >> supported subset that is enabled at boot. >> >> I expect RH and Suse will feel similarily. Speaking for Ubuntu if this >> isn't available as part of lsm stacking it will get distro patched in. > > Right. Ubuntu would do something like: > > CONFIG_LSM_ENABLE=yama,apparmor,integrity > > And that's why I wanted non-explicit lsm.enable, so that an end user > could just do: > > lsm.enable=loadpin > > to add loadpin. > > Perhaps we could have both? "lsm.enable=+loadpin" (add loadpin to > build default list) vs "lsm.enable=loadpin" (override build default > list with ONLY loadpin). > Maybe? I'm not sure what the best option is with all the competing requirements/desires. I need to think about it more and would like to see what others think.
Re: [PATCH 15/16] apparmor: Replace spin_is_locked() with lockdep
On 10/02/2018 10:39 PM, Lance Roy wrote:
> lockdep_assert_held() is better suited to checking locking requirements,
> since it won't get confused when someone else holds the lock. This is
> also a step towards possibly removing spin_is_locked().
>
> Signed-off-by: Lance Roy
> Cc: John Johansen
> Cc: James Morris
> Cc: "Serge E. Hallyn"
> Cc:
Acked-by: John Johansen
> ---
> security/apparmor/file.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/security/apparmor/file.c b/security/apparmor/file.c
> index 4285943f7260..d0afed9ebd0e 100644
> --- a/security/apparmor/file.c
> +++ b/security/apparmor/file.c
> @@ -496,7 +496,7 @@ static void update_file_ctx(struct aa_file_ctx *fctx,
> struct aa_label *label,
> /* update caching of label on file_ctx */
> spin_lock(>lock);
> old = rcu_dereference_protected(fctx->label,
> - spin_is_locked(>lock));
> + lockdep_is_held(>lock));
> l = aa_label_merge(old, label, GFP_ATOMIC);
> if (l) {
> if (l != old) {
>
Re: [PATCH 15/16] apparmor: Replace spin_is_locked() with lockdep
On 10/02/2018 10:39 PM, Lance Roy wrote:
> lockdep_assert_held() is better suited to checking locking requirements,
> since it won't get confused when someone else holds the lock. This is
> also a step towards possibly removing spin_is_locked().
>
> Signed-off-by: Lance Roy
> Cc: John Johansen
> Cc: James Morris
> Cc: "Serge E. Hallyn"
> Cc:
Acked-by: John Johansen
> ---
> security/apparmor/file.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/security/apparmor/file.c b/security/apparmor/file.c
> index 4285943f7260..d0afed9ebd0e 100644
> --- a/security/apparmor/file.c
> +++ b/security/apparmor/file.c
> @@ -496,7 +496,7 @@ static void update_file_ctx(struct aa_file_ctx *fctx,
> struct aa_label *label,
> /* update caching of label on file_ctx */
> spin_lock(>lock);
> old = rcu_dereference_protected(fctx->label,
> - spin_is_locked(>lock));
> + lockdep_is_held(>lock));
> l = aa_label_merge(old, label, GFP_ATOMIC);
> if (l) {
> if (l != old) {
>
Re: [PATCH security-next v4 23/32] selinux: Remove boot parameter
On 10/02/2018 01:29 PM, Kees Cook wrote: > On Tue, Oct 2, 2018 at 12:47 PM, John Johansen > wrote: >> On 10/02/2018 12:17 PM, Kees Cook wrote: >>> I could define CONFIG_LSM_ENABLE as being "additive" to >>> SECURITY_APPARMOR_BOOTPARAM_VALUE and >>> SECURITY_SELINUX_BOOTPARAM_VALUE? >> >> Oh sure lets deal with my complaint about too many ways to configure >> this beast by adding yet another config option :P > > This is what v3 already does: SEC...BOOTPARAM_VALUE trumps ...LSM_ENABLE. > sure but I sent in a patch to kill SECURITY_APPARMOR_BOOTPARAM_VALUE because I really dislike the extra levels of config and getting rid of the SEC..BOOTPARAM_VALUE seems to be the easy way to fix it Now if only we can convince Paul and Stephen :) >> seriously though, please no. That just adds another layer of confusion >> even if it is only being foisted on the distro/builder > > You've already sent a patch removing > SECURITY_APPARMOR_BOOTPARAM_VALUE. If SELinux is fine to do that too, > then I think we'll be sorted out. I'll just need to make "lsm.enable=" > be an explicit list. (Do you have a problem with "lsm.disable=..." ?) > why yes, glad you asked If lsm.enabled is an explicit list lsm.disabled isn't required its a convenience option that can introduce confusion and conflicts. If both lsm.enabled and lsm.disabled are being used at the same time. I realize that some times the convenience of specifying lsm.disable=$LSM is easier than specifying an entire list of what should be enabled when you just want to disable a single LSM. I don't think the convenience is worth the potential confusion, but I don't feel strongly about it and realize others feel the other way. tldr: I can live with it, but don't like it if you are asking :)
Re: [PATCH security-next v4 23/32] selinux: Remove boot parameter
On 10/02/2018 01:29 PM, Kees Cook wrote: > On Tue, Oct 2, 2018 at 12:47 PM, John Johansen > wrote: >> On 10/02/2018 12:17 PM, Kees Cook wrote: >>> I could define CONFIG_LSM_ENABLE as being "additive" to >>> SECURITY_APPARMOR_BOOTPARAM_VALUE and >>> SECURITY_SELINUX_BOOTPARAM_VALUE? >> >> Oh sure lets deal with my complaint about too many ways to configure >> this beast by adding yet another config option :P > > This is what v3 already does: SEC...BOOTPARAM_VALUE trumps ...LSM_ENABLE. > sure but I sent in a patch to kill SECURITY_APPARMOR_BOOTPARAM_VALUE because I really dislike the extra levels of config and getting rid of the SEC..BOOTPARAM_VALUE seems to be the easy way to fix it Now if only we can convince Paul and Stephen :) >> seriously though, please no. That just adds another layer of confusion >> even if it is only being foisted on the distro/builder > > You've already sent a patch removing > SECURITY_APPARMOR_BOOTPARAM_VALUE. If SELinux is fine to do that too, > then I think we'll be sorted out. I'll just need to make "lsm.enable=" > be an explicit list. (Do you have a problem with "lsm.disable=..." ?) > why yes, glad you asked If lsm.enabled is an explicit list lsm.disabled isn't required its a convenience option that can introduce confusion and conflicts. If both lsm.enabled and lsm.disabled are being used at the same time. I realize that some times the convenience of specifying lsm.disable=$LSM is easier than specifying an entire list of what should be enabled when you just want to disable a single LSM. I don't think the convenience is worth the potential confusion, but I don't feel strongly about it and realize others feel the other way. tldr: I can live with it, but don't like it if you are asking :)
Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering
On 09/17/2018 05:45 PM, Kees Cook wrote:
> On Mon, Sep 17, 2018 at 5:24 PM, Casey Schaufler
> wrote:
>> On 9/17/2018 5:00 PM, Kees Cook wrote:
>>> The legacy per-LSM
>>> enable/disable ordering is the same, but ordering between
>>> lsm.enable/disable and the per-LSM options is NOT ordered. i.e. the
>>> precedent mentioned in the prior paragraph.
>>
>> That is, capability,yama,loadpin,
>
> Yeah, sorry, I didn't mean LSM order there, I meant the commandline
> order of appearance of the options. If you mix them, the last
> lsm.enable/disable for an LSM is the "real" setting, and the last
> $LSM.enabled= setting is the last of _that_ one.
>
>>> To support "security=", we'll still have some kind of legacy
>>> LSM_FLAG_MAJOR to perform implicit disabling of the non-operational
>>> other "major" LSMs. This means "security=$foo" will be a short-hand
>>> for "lsm.disable=all-LSM_FLAG_MAJOR-who-are-not-$foo". This will
>>> exactly match current behavior (i.e. "security=smack" and if smack
>>> fails initialization, we do not then fall back to another major).
>>
>> Right.
>
> Cool.
>
>>> I think we have to support runtime ordering for the reasons John
>>> specifies. Additionally, I have the sense that anything we can
>>> configure in Kconfig ultimately ends up being expressed at runtime
>>> too, so better to just make sure the design includes it now.
>>
>> Right.
>>
>>> What we have now:
>>>
>>> "first" then "order-doesn't-matter-minors" then "exclusive-major"
>>>
>>> - we can't change first.
>>> - exclusivity-ordering only matters in the face of enable/disable
>>> which we have solved now (?)
>>
>> I'm not sure where you get the conclusion we've solved this.
>> Today I can't say "lsm.enable=smack lsm.enable=apparmor", and
>> there's no mechanism to prevent that.
>>
>>> so, ordering can be totally arbitrary after "first" (but before some
>>> future "last"). We must not allow a token for "everything else" since
>>> that overlaps with enable/disable, so "everything else" stay implicit
>>> (I would argue a trailing implicit ordering).
>>
>> There's an assumption you're making that I'm not getting. Where does
>> this overlap between ordering and enable/disable come from?
>
> Handling exclusivity means the non-active LSMs are disabled. We had
> been saying "the other majors are disabled", but the concept of major
> will become arbitrary. If instead we move to "first exclusive wins
> among the exclusives", we still have the "the others are disabled"
> case. So exclusivity begets disabling.
>
>>> The one complication I see with ordering, then, is that if we change
>>> the exclusivity over time, we change what may be present on the
>>> system. For example, right now tomoyo is exclusive. Once we have
>>> blob-sharing, it doesn't need to be.
>>>
>>> so: lsm.order=tomoyo after this series means
>>> "capability,tomoyo,yama,loadpin,integrity", but when tomoyo becomes
>>> non-exclusive, suddenly we get
>>> "capability,tomoyo,yama,loadpin,{selinux,smack,apparmor},integrity".
>>> (i.e. if selinux is disabled then move on to trying smack, then
>>> apparmor, etc.)
>>
>> We're missing a description of what happens at build time.
>> It's hard to see what you expect to happen if I want to build in
>> all the major modules and don't plan to use the boot command line
>> options.
>>
>>> I would argue that this is a design feature (LSMs aren't left behind),
>>> and order of enabled exclusive LSMs "wins" the choice for the
>>> exclusivity (instead of operating "by name" the way "security="
>>> works).
>>
>> I think I see more, but I'm guessing. At build time it looks like
>> you're dropping the specification on the "major" module. We can't
>> do that because I want to build kernels that run Smack by default
>> but include SELinux for when I'm feeling less evil than normal.
>
> Do we need build time _ordering_, or can we just go with build time
depends on what you mean by build time ordering. Ordering like we
currently have based on link order, no. The ability to specify
the lsm order just like on the command line yes.
Distros are very much going to have a preferred order they want
LSMs to make supporting this more tractable when trying to deal
with issues.
> "first exclusive"? For the v1, I went with "first exclusive" from
> CONFIG_SECURITY_DEFAULT, and left the rest of the ordering up to the
> Makefile.
>
First exclusive from CONFIG_SECURITY_DEFAULT is fine for now, I am
more interested in making sure we get it right for when exclusivity
goes away.
Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering
On 09/17/2018 05:45 PM, Kees Cook wrote:
> On Mon, Sep 17, 2018 at 5:24 PM, Casey Schaufler
> wrote:
>> On 9/17/2018 5:00 PM, Kees Cook wrote:
>>> The legacy per-LSM
>>> enable/disable ordering is the same, but ordering between
>>> lsm.enable/disable and the per-LSM options is NOT ordered. i.e. the
>>> precedent mentioned in the prior paragraph.
>>
>> That is, capability,yama,loadpin,
>
> Yeah, sorry, I didn't mean LSM order there, I meant the commandline
> order of appearance of the options. If you mix them, the last
> lsm.enable/disable for an LSM is the "real" setting, and the last
> $LSM.enabled= setting is the last of _that_ one.
>
>>> To support "security=", we'll still have some kind of legacy
>>> LSM_FLAG_MAJOR to perform implicit disabling of the non-operational
>>> other "major" LSMs. This means "security=$foo" will be a short-hand
>>> for "lsm.disable=all-LSM_FLAG_MAJOR-who-are-not-$foo". This will
>>> exactly match current behavior (i.e. "security=smack" and if smack
>>> fails initialization, we do not then fall back to another major).
>>
>> Right.
>
> Cool.
>
>>> I think we have to support runtime ordering for the reasons John
>>> specifies. Additionally, I have the sense that anything we can
>>> configure in Kconfig ultimately ends up being expressed at runtime
>>> too, so better to just make sure the design includes it now.
>>
>> Right.
>>
>>> What we have now:
>>>
>>> "first" then "order-doesn't-matter-minors" then "exclusive-major"
>>>
>>> - we can't change first.
>>> - exclusivity-ordering only matters in the face of enable/disable
>>> which we have solved now (?)
>>
>> I'm not sure where you get the conclusion we've solved this.
>> Today I can't say "lsm.enable=smack lsm.enable=apparmor", and
>> there's no mechanism to prevent that.
>>
>>> so, ordering can be totally arbitrary after "first" (but before some
>>> future "last"). We must not allow a token for "everything else" since
>>> that overlaps with enable/disable, so "everything else" stay implicit
>>> (I would argue a trailing implicit ordering).
>>
>> There's an assumption you're making that I'm not getting. Where does
>> this overlap between ordering and enable/disable come from?
>
> Handling exclusivity means the non-active LSMs are disabled. We had
> been saying "the other majors are disabled", but the concept of major
> will become arbitrary. If instead we move to "first exclusive wins
> among the exclusives", we still have the "the others are disabled"
> case. So exclusivity begets disabling.
>
>>> The one complication I see with ordering, then, is that if we change
>>> the exclusivity over time, we change what may be present on the
>>> system. For example, right now tomoyo is exclusive. Once we have
>>> blob-sharing, it doesn't need to be.
>>>
>>> so: lsm.order=tomoyo after this series means
>>> "capability,tomoyo,yama,loadpin,integrity", but when tomoyo becomes
>>> non-exclusive, suddenly we get
>>> "capability,tomoyo,yama,loadpin,{selinux,smack,apparmor},integrity".
>>> (i.e. if selinux is disabled then move on to trying smack, then
>>> apparmor, etc.)
>>
>> We're missing a description of what happens at build time.
>> It's hard to see what you expect to happen if I want to build in
>> all the major modules and don't plan to use the boot command line
>> options.
>>
>>> I would argue that this is a design feature (LSMs aren't left behind),
>>> and order of enabled exclusive LSMs "wins" the choice for the
>>> exclusivity (instead of operating "by name" the way "security="
>>> works).
>>
>> I think I see more, but I'm guessing. At build time it looks like
>> you're dropping the specification on the "major" module. We can't
>> do that because I want to build kernels that run Smack by default
>> but include SELinux for when I'm feeling less evil than normal.
>
> Do we need build time _ordering_, or can we just go with build time
depends on what you mean by build time ordering. Ordering like we
currently have based on link order, no. The ability to specify
the lsm order just like on the command line yes.
Distros are very much going to have a preferred order they want
LSMs to make supporting this more tractable when trying to deal
with issues.
> "first exclusive"? For the v1, I went with "first exclusive" from
> CONFIG_SECURITY_DEFAULT, and left the rest of the ordering up to the
> Makefile.
>
First exclusive from CONFIG_SECURITY_DEFAULT is fine for now, I am
more interested in making sure we get it right for when exclusivity
goes away.
Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering
On 09/17/2018 04:20 PM, Kees Cook wrote: > On Mon, Sep 17, 2018 at 4:10 PM, Mickaël Salaün wrote: >> Landlock, because it target unprivileged users, should only be called >> after all other major (access-control) LSMs. The admin or distro must >> not be able to change that order in any way. This constraint doesn't >> apply to current LSMs, though. > > Good point! It will be easy to add LSM_ORDER_LAST, though, given the > machinery introduced in this series. > And when we have two LSMs that want to use that?
Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering
On 09/17/2018 04:20 PM, Kees Cook wrote: > On Mon, Sep 17, 2018 at 4:10 PM, Mickaël Salaün wrote: >> Landlock, because it target unprivileged users, should only be called >> after all other major (access-control) LSMs. The admin or distro must >> not be able to change that order in any way. This constraint doesn't >> apply to current LSMs, though. > > Good point! It will be easy to add LSM_ORDER_LAST, though, given the > machinery introduced in this series. > And when we have two LSMs that want to use that?
Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering
On 09/17/2018 04:10 PM, Mickaël Salaün wrote: > << snip >> > If "lsm.enable=apparmor lsm.disable=apparmor" is specified the last value > specified is used giving "lsm.disable=apparmor". > makes sense >>> >>> The rules for modification are pretty obvious. The downside is, as >>> you point out, that they don't address ordering. Maybe we address that >>> directly: >>> >>> lsm.order=*,tomoyo >>> >>> TOMOYO should be last. >>> >>> lsm.order=apparmor,* >>> >>> AppArmor should be first. >>> >>> >>> lsm.order=*,sara,selinux,* >>> >>> SELinux should come directly after SARA but we otherwise don't >>> care. >>> >>> lsm.order=smack,*,landlock,* >>> >>> Smack should be first and LandLock should come sometime later. >>> >>> lsm.order=*,yama,* >>> >>> Is meaningless. >>> >>> Modules not listed may go anywhere there is a "*" in the order. >>> An lsm.order= without a "*" is an error, and ignored. >>> If a module is specified in lsm.order but not built in it is ignored. >>> If a module is specified but disabled it is ignored. >>> The capability module goes first regardless. >>> >> >> I don't mind using lsm.order if we must but really do not like the '*' >> idea. It makes this way more complicated than it needs to be >> >> > > Landlock, because it target unprivileged users, should only be called > after all other major (access-control) LSMs. The admin or distro must > not be able to change that order in any way. This constraint doesn't > apply to current LSMs, though. > And yet another complication :) I don't know that we can enforce a strict only after all other LSMs. Imagine the hypothetical case of 2 LSMs targeting unprivileged users. Which one should be called first?
Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering
On 09/17/2018 04:10 PM, Mickaël Salaün wrote: > << snip >> > If "lsm.enable=apparmor lsm.disable=apparmor" is specified the last value > specified is used giving "lsm.disable=apparmor". > makes sense >>> >>> The rules for modification are pretty obvious. The downside is, as >>> you point out, that they don't address ordering. Maybe we address that >>> directly: >>> >>> lsm.order=*,tomoyo >>> >>> TOMOYO should be last. >>> >>> lsm.order=apparmor,* >>> >>> AppArmor should be first. >>> >>> >>> lsm.order=*,sara,selinux,* >>> >>> SELinux should come directly after SARA but we otherwise don't >>> care. >>> >>> lsm.order=smack,*,landlock,* >>> >>> Smack should be first and LandLock should come sometime later. >>> >>> lsm.order=*,yama,* >>> >>> Is meaningless. >>> >>> Modules not listed may go anywhere there is a "*" in the order. >>> An lsm.order= without a "*" is an error, and ignored. >>> If a module is specified in lsm.order but not built in it is ignored. >>> If a module is specified but disabled it is ignored. >>> The capability module goes first regardless. >>> >> >> I don't mind using lsm.order if we must but really do not like the '*' >> idea. It makes this way more complicated than it needs to be >> >> > > Landlock, because it target unprivileged users, should only be called > after all other major (access-control) LSMs. The admin or distro must > not be able to change that order in any way. This constraint doesn't > apply to current LSMs, though. > And yet another complication :) I don't know that we can enforce a strict only after all other LSMs. Imagine the hypothetical case of 2 LSMs targeting unprivileged users. Which one should be called first?
Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering
On 09/17/2018 02:57 PM, Casey Schaufler wrote:
> On 9/17/2018 12:55 PM, John Johansen wrote:
>> On 09/17/2018 12:23 PM, Casey Schaufler wrote:
>>> On 9/17/2018 11:14 AM, Kees Cook wrote:
>>>>> Keep security=$lsm with the existing exclusive behavior.
>>>>> Add lsm=$lsm1,...,$lsmN which requires a full list of modules
>>>>>
>>>>> If you want to be fancy (I don't!) you could add
>>>>>
>>>>> lsm.add=$lsm1,...,$lsmN which adds the modules to the stack
>>>>> lsm.delete=$lsm1,...,$lsmN which deletes modules from the stack
>>>> We've got two issues: ordering and enablement. It's been strongly
>>>> suggested that we should move away from per-LSM enable/disable flags
>>>> (to which I agree).
>>> I also agree. There are way too many ways to turn off some LSMs.
>>>
>> I wont disagree, but its largely because we didn't have this discussion
>> when we should have.
>
> True that.
>
>
>>>> If ordering should be separate from enablement (to
>>>> avoid the "booted kernel with new LSM built in, but my lsm="..." line
>>>> didn't include it so it's disabled case), then I think we need to
>>>> split the logic (otherwise we just reinvented "security=" with similar
>>>> problems).
>>> We could reduce the problem by declaring that LSM ordering is
>>> not something you can specify on the boot line. I can see value
>>> in specifying it when you build the kernel, but your circumstances
>>> would have to be pretty strange to change it at boot time.
>>>
>> if there is LSM ordering the getting
>>
>> lsm=B,A,C
>>
>> is not the behavior I would expect from specifying
>>
>> lsm=A,B,C
>
> Right. You'd expect that they'd be used in the order specified.
>
and yet you argue for something different ;)
>>>> Should "lsm=" allow arbitrary ordering? (I think yes.)
>>> I say no. Assume you can specify it at build time. When would
>>> you want to change the order? Why would you?
>>>
>> because maybe you care about the denial message from one LSM more than
>> you do from another. Since stacking is bail on first fail the order
>> could be important from an auditing POV
>
> I understand that a distribution would want to specify the order
> for support purposes and that a developer would want to specify
> the order to ensure reproducible behavior. But they are going to
> be controlling their kernel builds. I'm not suggesting that the
> order shouldn't be capable of build time specification. What I
> don't see is a reason to rearrange it at boot time.
>
Because not all users have the same priority as the distro. It can
also aid in debugging and testing of LSMs in a stacked situation.
>> Auditing is why apparmor's internal stacking is not bail on first
>> fail.
>
> Within a security module I get that. But we've already got the
> priority wrong for audit in general, because you only get to the
> LSM if the traditional code approves. Every guidance I ever got
true
> said you should do the MAC checks first, because you're much more
> concerned about getting audit records about MAC failures than DAC.
>
yep, wouldn't that be nice to have
>>>> Should "lsm=" imply implicit enable/disable? (I think no: unlisted
>>>> LSMs are implicitly auto-appended to the explicit list)
>>> If you want to add something that isn't there instead of making
>>> it explicit you want "lsm.enable=" not "lsm=".
>>>
>>>> So then we could have "lsm.enable=..." and "lsm.disable=...".
>>>>
>>>> If builtin list was:
>>>> capability,yama,loadpin,integrity,{selinux,smack,tomoyo,apparmor}
>>>> then:
>>>>
>>>> lsm.disable=loadpin lsm=smack
>>> Methinks this should be lsm.disable=loadpin lsm.enable=smack
>>>
>> that would only work if order is not important
>
> It works unless you want to change the order at boot, and
> I still don't see a use case for that.
see above
>
>>>> becomes
>>>>
>>>> capability,smack,yama,integrity
>>>>
>>>> and
>>>>
>>>> CONFIG_SECURITY_LOADPIN_DEFAULT_ENABLED=n
>>>> selinux.enable=0 lsm.add=loadpin lsm.disable=smack,tomoyo lsm=integrity
>>> Do you mean
>>> selinux.enable=0 lsm.enable=loadpin lsm.disable=smack,tomoyo
>>> lsm.enable=integrity
>>> selinux.enable=0 lsm.enable=loadp
Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering
On 09/17/2018 02:57 PM, Casey Schaufler wrote:
> On 9/17/2018 12:55 PM, John Johansen wrote:
>> On 09/17/2018 12:23 PM, Casey Schaufler wrote:
>>> On 9/17/2018 11:14 AM, Kees Cook wrote:
>>>>> Keep security=$lsm with the existing exclusive behavior.
>>>>> Add lsm=$lsm1,...,$lsmN which requires a full list of modules
>>>>>
>>>>> If you want to be fancy (I don't!) you could add
>>>>>
>>>>> lsm.add=$lsm1,...,$lsmN which adds the modules to the stack
>>>>> lsm.delete=$lsm1,...,$lsmN which deletes modules from the stack
>>>> We've got two issues: ordering and enablement. It's been strongly
>>>> suggested that we should move away from per-LSM enable/disable flags
>>>> (to which I agree).
>>> I also agree. There are way too many ways to turn off some LSMs.
>>>
>> I wont disagree, but its largely because we didn't have this discussion
>> when we should have.
>
> True that.
>
>
>>>> If ordering should be separate from enablement (to
>>>> avoid the "booted kernel with new LSM built in, but my lsm="..." line
>>>> didn't include it so it's disabled case), then I think we need to
>>>> split the logic (otherwise we just reinvented "security=" with similar
>>>> problems).
>>> We could reduce the problem by declaring that LSM ordering is
>>> not something you can specify on the boot line. I can see value
>>> in specifying it when you build the kernel, but your circumstances
>>> would have to be pretty strange to change it at boot time.
>>>
>> if there is LSM ordering the getting
>>
>> lsm=B,A,C
>>
>> is not the behavior I would expect from specifying
>>
>> lsm=A,B,C
>
> Right. You'd expect that they'd be used in the order specified.
>
and yet you argue for something different ;)
>>>> Should "lsm=" allow arbitrary ordering? (I think yes.)
>>> I say no. Assume you can specify it at build time. When would
>>> you want to change the order? Why would you?
>>>
>> because maybe you care about the denial message from one LSM more than
>> you do from another. Since stacking is bail on first fail the order
>> could be important from an auditing POV
>
> I understand that a distribution would want to specify the order
> for support purposes and that a developer would want to specify
> the order to ensure reproducible behavior. But they are going to
> be controlling their kernel builds. I'm not suggesting that the
> order shouldn't be capable of build time specification. What I
> don't see is a reason to rearrange it at boot time.
>
Because not all users have the same priority as the distro. It can
also aid in debugging and testing of LSMs in a stacked situation.
>> Auditing is why apparmor's internal stacking is not bail on first
>> fail.
>
> Within a security module I get that. But we've already got the
> priority wrong for audit in general, because you only get to the
> LSM if the traditional code approves. Every guidance I ever got
true
> said you should do the MAC checks first, because you're much more
> concerned about getting audit records about MAC failures than DAC.
>
yep, wouldn't that be nice to have
>>>> Should "lsm=" imply implicit enable/disable? (I think no: unlisted
>>>> LSMs are implicitly auto-appended to the explicit list)
>>> If you want to add something that isn't there instead of making
>>> it explicit you want "lsm.enable=" not "lsm=".
>>>
>>>> So then we could have "lsm.enable=..." and "lsm.disable=...".
>>>>
>>>> If builtin list was:
>>>> capability,yama,loadpin,integrity,{selinux,smack,tomoyo,apparmor}
>>>> then:
>>>>
>>>> lsm.disable=loadpin lsm=smack
>>> Methinks this should be lsm.disable=loadpin lsm.enable=smack
>>>
>> that would only work if order is not important
>
> It works unless you want to change the order at boot, and
> I still don't see a use case for that.
see above
>
>>>> becomes
>>>>
>>>> capability,smack,yama,integrity
>>>>
>>>> and
>>>>
>>>> CONFIG_SECURITY_LOADPIN_DEFAULT_ENABLED=n
>>>> selinux.enable=0 lsm.add=loadpin lsm.disable=smack,tomoyo lsm=integrity
>>> Do you mean
>>> selinux.enable=0 lsm.enable=loadpin lsm.disable=smack,tomoyo
>>> lsm.enable=integrity
>>> selinux.enable=0 lsm.enable=loadp
Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering
On 09/17/2018 12:23 PM, Casey Schaufler wrote:
> On 9/17/2018 11:14 AM, Kees Cook wrote:
>>
>>> Keep security=$lsm with the existing exclusive behavior.
>>> Add lsm=$lsm1,...,$lsmN which requires a full list of modules
>>>
>>> If you want to be fancy (I don't!) you could add
>>>
>>> lsm.add=$lsm1,...,$lsmN which adds the modules to the stack
>>> lsm.delete=$lsm1,...,$lsmN which deletes modules from the stack
>> We've got two issues: ordering and enablement. It's been strongly
>> suggested that we should move away from per-LSM enable/disable flags
>> (to which I agree).
>
> I also agree. There are way too many ways to turn off some LSMs.
>
I wont disagree, but its largely because we didn't have this discussion
when we should have.
>> If ordering should be separate from enablement (to
>> avoid the "booted kernel with new LSM built in, but my lsm="..." line
>> didn't include it so it's disabled case), then I think we need to
>> split the logic (otherwise we just reinvented "security=" with similar
>> problems).
>
> We could reduce the problem by declaring that LSM ordering is
> not something you can specify on the boot line. I can see value
> in specifying it when you build the kernel, but your circumstances
> would have to be pretty strange to change it at boot time.
>
if there is LSM ordering the getting
lsm=B,A,C
is not the behavior I would expect from specifying
lsm=A,B,C
>> Should "lsm=" allow arbitrary ordering? (I think yes.)
>
> I say no. Assume you can specify it at build time. When would
> you want to change the order? Why would you?
>
because maybe you care about the denial message from one LSM more than
you do from another. Since stacking is bail on first fail the order
could be important from an auditing POV
Auditing is why apparmor's internal stacking is not bail on first
fail.
>> Should "lsm=" imply implicit enable/disable? (I think no: unlisted
>> LSMs are implicitly auto-appended to the explicit list)
>
> If you want to add something that isn't there instead of making
> it explicit you want "lsm.enable=" not "lsm=".
>
>> So then we could have "lsm.enable=..." and "lsm.disable=...".
>>
>> If builtin list was:
>> capability,yama,loadpin,integrity,{selinux,smack,tomoyo,apparmor}
>> then:
>>
>> lsm.disable=loadpin lsm=smack
>
> Methinks this should be lsm.disable=loadpin lsm.enable=smack
>
that would only work if order is not important
>> becomes
>>
>> capability,smack,yama,integrity
>>
>> and
>>
>> CONFIG_SECURITY_LOADPIN_DEFAULT_ENABLED=n
>> selinux.enable=0 lsm.add=loadpin lsm.disable=smack,tomoyo lsm=integrity
>
> Do you mean
> selinux.enable=0 lsm.enable=loadpin lsm.disable=smack,tomoyo
> lsm.enable=integrity
> selinux.enable=0 lsm.enable=loadpin,integrity lsm.disable=smack,tomoyo
> selinux.enable=0 lsm.enable=loadpin lsm.enable=integrity
> lsm.disable=smack lsm.disable=tomoyo
>
>> becomes
>>
>> capability,integrity,yama,loadpin,apparmor
>>
>>
>> If "lsm=" _does_ imply enablement, then how does it interact with
>> per-LSM disabling? i.e. what does "apparmor.enabled=0
>> lsm=yama,apparmor" mean? If it means "turn on apparmor" how do I turn
>> on a CONFIG-default-off LSM without specifying all the other LSMs too?
>
> There should either be one option "lsm=", which is an explicit list or
> two, "lsm.enable=" and "lsm.disable", which modify the built in default.
>
maybe but this breaks with current behavior as their is a mismatch between
how the major lsms do selection/enablement and the minor ones.
I personally would prefer
lsm=
but that breaks how the minor lsms are currently enable
> In the "lsm=" case "apparmor.enabled=0" should be equivalent to leaving
> apparmor off the list, but it's up to the AppArmor code to do that.
>> If "lsm.enable=apparmor apparmor.enabled=0" is specified the explict wish
> of the security module is used, but it's up to the AppArmor code to do that.
>
current behavior
> If "lsm.disable=apparmor apparmor.enabled=1" is specified the infrastructure
> should have shut down AppArmor before it looked to see the
> "apparmor.enabled=1",
> so it will remain disabled.
>
yep, current behavior
> If "lsm.enable=apparmor lsm.disable=apparmor" is specified the last value
> specified is used giving "lsm.disable=apparmor".
>
makes sense
Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering
On 09/17/2018 12:23 PM, Casey Schaufler wrote:
> On 9/17/2018 11:14 AM, Kees Cook wrote:
>>
>>> Keep security=$lsm with the existing exclusive behavior.
>>> Add lsm=$lsm1,...,$lsmN which requires a full list of modules
>>>
>>> If you want to be fancy (I don't!) you could add
>>>
>>> lsm.add=$lsm1,...,$lsmN which adds the modules to the stack
>>> lsm.delete=$lsm1,...,$lsmN which deletes modules from the stack
>> We've got two issues: ordering and enablement. It's been strongly
>> suggested that we should move away from per-LSM enable/disable flags
>> (to which I agree).
>
> I also agree. There are way too many ways to turn off some LSMs.
>
I wont disagree, but its largely because we didn't have this discussion
when we should have.
>> If ordering should be separate from enablement (to
>> avoid the "booted kernel with new LSM built in, but my lsm="..." line
>> didn't include it so it's disabled case), then I think we need to
>> split the logic (otherwise we just reinvented "security=" with similar
>> problems).
>
> We could reduce the problem by declaring that LSM ordering is
> not something you can specify on the boot line. I can see value
> in specifying it when you build the kernel, but your circumstances
> would have to be pretty strange to change it at boot time.
>
if there is LSM ordering the getting
lsm=B,A,C
is not the behavior I would expect from specifying
lsm=A,B,C
>> Should "lsm=" allow arbitrary ordering? (I think yes.)
>
> I say no. Assume you can specify it at build time. When would
> you want to change the order? Why would you?
>
because maybe you care about the denial message from one LSM more than
you do from another. Since stacking is bail on first fail the order
could be important from an auditing POV
Auditing is why apparmor's internal stacking is not bail on first
fail.
>> Should "lsm=" imply implicit enable/disable? (I think no: unlisted
>> LSMs are implicitly auto-appended to the explicit list)
>
> If you want to add something that isn't there instead of making
> it explicit you want "lsm.enable=" not "lsm=".
>
>> So then we could have "lsm.enable=..." and "lsm.disable=...".
>>
>> If builtin list was:
>> capability,yama,loadpin,integrity,{selinux,smack,tomoyo,apparmor}
>> then:
>>
>> lsm.disable=loadpin lsm=smack
>
> Methinks this should be lsm.disable=loadpin lsm.enable=smack
>
that would only work if order is not important
>> becomes
>>
>> capability,smack,yama,integrity
>>
>> and
>>
>> CONFIG_SECURITY_LOADPIN_DEFAULT_ENABLED=n
>> selinux.enable=0 lsm.add=loadpin lsm.disable=smack,tomoyo lsm=integrity
>
> Do you mean
> selinux.enable=0 lsm.enable=loadpin lsm.disable=smack,tomoyo
> lsm.enable=integrity
> selinux.enable=0 lsm.enable=loadpin,integrity lsm.disable=smack,tomoyo
> selinux.enable=0 lsm.enable=loadpin lsm.enable=integrity
> lsm.disable=smack lsm.disable=tomoyo
>
>> becomes
>>
>> capability,integrity,yama,loadpin,apparmor
>>
>>
>> If "lsm=" _does_ imply enablement, then how does it interact with
>> per-LSM disabling? i.e. what does "apparmor.enabled=0
>> lsm=yama,apparmor" mean? If it means "turn on apparmor" how do I turn
>> on a CONFIG-default-off LSM without specifying all the other LSMs too?
>
> There should either be one option "lsm=", which is an explicit list or
> two, "lsm.enable=" and "lsm.disable", which modify the built in default.
>
maybe but this breaks with current behavior as their is a mismatch between
how the major lsms do selection/enablement and the minor ones.
I personally would prefer
lsm=
but that breaks how the minor lsms are currently enable
> In the "lsm=" case "apparmor.enabled=0" should be equivalent to leaving
> apparmor off the list, but it's up to the AppArmor code to do that.
>> If "lsm.enable=apparmor apparmor.enabled=0" is specified the explict wish
> of the security module is used, but it's up to the AppArmor code to do that.
>
current behavior
> If "lsm.disable=apparmor apparmor.enabled=1" is specified the infrastructure
> should have shut down AppArmor before it looked to see the
> "apparmor.enabled=1",
> so it will remain disabled.
>
yep, current behavior
> If "lsm.enable=apparmor lsm.disable=apparmor" is specified the last value
> specified is used giving "lsm.disable=apparmor".
>
makes sense
Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering
On 09/17/2018 11:14 AM, Kees Cook wrote:
> On Mon, Sep 17, 2018 at 10:13 AM, Casey Schaufler
> wrote:
>> TOMOYO uses the cred blob pointer. When the blob is shared TOMOYO
>> has to be allocated a pointer size chunk to store the pointer in.
>> Smack has the same behavior on file blobs.
>
> Oh dang, yes, I got confused over secid and other "extreme" shared things.
>
> So one change of my series would be to declare tomoyo as "exclusive" too.
>
>> Today the distinction is based on how the module registers hooks.
>> Modules that use blobs (including TOMOYO) use security_module_enable()
>> and those that don't just use security_add_hooks(). The "pick one"
>> policy is enforced in security_module_enable(), which is why you can
>> have as many non-blob users as you like. You could easily have a
>> non-blob using module that was exclusive simply by using
>> security_module_enable().
>
> True. With my removal of security_module_enable(), yes, it makes sense
> to mark all LSMs that were calling it before as exclusive, rather than
> focusing on whether they would be exclusive under the blob-sharing
> situation.
>
>> Keep security=$lsm with the existing exclusive behavior.
>> Add lsm=$lsm1,...,$lsmN which requires a full list of modules
>>
>> If you want to be fancy (I don't!) you could add
>>
>> lsm.add=$lsm1,...,$lsmN which adds the modules to the stack
>> lsm.delete=$lsm1,...,$lsmN which deletes modules from the stack
>
> We've got two issues: ordering and enablement. It's been strongly
> suggested that we should move away from per-LSM enable/disable flags
> (to which I agree). If ordering should be separate from enablement (to
> avoid the "booted kernel with new LSM built in, but my lsm="..." line
> didn't include it so it's disabled case), then I think we need to
> split the logic (otherwise we just reinvented "security=" with similar
> problems).
>
> Should "lsm=" allow arbitrary ordering? (I think yes.)
> yes
> Should "lsm=" imply implicit enable/disable? (I think no: unlisted
> LSMs are implicitly auto-appended to the explicit list)
>
maybe, adding $lsm to the list could possibly considered as enabling it,
but not having it there doesn't necessarily imply it isn't
> So then we could have "lsm.enable=..." and "lsm.disable=...".
>
> If builtin list was:
> capability,yama,loadpin,integrity,{selinux,smack,tomoyo,apparmor}
> then:
>
> lsm.disable=loadpin lsm=smack
>
> becomes
>
> capability,smack,yama,integrity
>
> and
>
> CONFIG_SECURITY_LOADPIN_DEFAULT_ENABLED=n
> selinux.enable=0 lsm.add=loadpin lsm.disable=smack,tomoyo lsm=integrity
>
> becomes
>
> capability,integrity,yama,loadpin,apparmor
>
>
> If "lsm=" _does_ imply enablement, then how does it interact with
> per-LSM disabling? i.e. what does "apparmor.enabled=0
> lsm=yama,apparmor" mean? If it means "turn on apparmor" how do I turn
> on a CONFIG-default-off LSM without specifying all the other LSMs too?
>
currently using
security=apparmor apparmor=0
means apparmor is the one given the chance to register but it declines
which means you just get capabilities. And with
# caveat not part of the current stacking patchset
security=selinux,apparmor apparmor=0
you end up with
capability,selinux
However apparmor=1 does not imply apparmor is the available LSM
that is
security=selinux apparmor=1
gives you
capability,selinux
if iirc selinux=X behaves the same way
However it is not clear to me whether this is the behavior that we
would want for $lsm.enabled, $lsm.disabled. It appears to be in
conflict with how yama, loadpin and IMA currently work.
Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering
On 09/17/2018 11:14 AM, Kees Cook wrote:
> On Mon, Sep 17, 2018 at 10:13 AM, Casey Schaufler
> wrote:
>> TOMOYO uses the cred blob pointer. When the blob is shared TOMOYO
>> has to be allocated a pointer size chunk to store the pointer in.
>> Smack has the same behavior on file blobs.
>
> Oh dang, yes, I got confused over secid and other "extreme" shared things.
>
> So one change of my series would be to declare tomoyo as "exclusive" too.
>
>> Today the distinction is based on how the module registers hooks.
>> Modules that use blobs (including TOMOYO) use security_module_enable()
>> and those that don't just use security_add_hooks(). The "pick one"
>> policy is enforced in security_module_enable(), which is why you can
>> have as many non-blob users as you like. You could easily have a
>> non-blob using module that was exclusive simply by using
>> security_module_enable().
>
> True. With my removal of security_module_enable(), yes, it makes sense
> to mark all LSMs that were calling it before as exclusive, rather than
> focusing on whether they would be exclusive under the blob-sharing
> situation.
>
>> Keep security=$lsm with the existing exclusive behavior.
>> Add lsm=$lsm1,...,$lsmN which requires a full list of modules
>>
>> If you want to be fancy (I don't!) you could add
>>
>> lsm.add=$lsm1,...,$lsmN which adds the modules to the stack
>> lsm.delete=$lsm1,...,$lsmN which deletes modules from the stack
>
> We've got two issues: ordering and enablement. It's been strongly
> suggested that we should move away from per-LSM enable/disable flags
> (to which I agree). If ordering should be separate from enablement (to
> avoid the "booted kernel with new LSM built in, but my lsm="..." line
> didn't include it so it's disabled case), then I think we need to
> split the logic (otherwise we just reinvented "security=" with similar
> problems).
>
> Should "lsm=" allow arbitrary ordering? (I think yes.)
> yes
> Should "lsm=" imply implicit enable/disable? (I think no: unlisted
> LSMs are implicitly auto-appended to the explicit list)
>
maybe, adding $lsm to the list could possibly considered as enabling it,
but not having it there doesn't necessarily imply it isn't
> So then we could have "lsm.enable=..." and "lsm.disable=...".
>
> If builtin list was:
> capability,yama,loadpin,integrity,{selinux,smack,tomoyo,apparmor}
> then:
>
> lsm.disable=loadpin lsm=smack
>
> becomes
>
> capability,smack,yama,integrity
>
> and
>
> CONFIG_SECURITY_LOADPIN_DEFAULT_ENABLED=n
> selinux.enable=0 lsm.add=loadpin lsm.disable=smack,tomoyo lsm=integrity
>
> becomes
>
> capability,integrity,yama,loadpin,apparmor
>
>
> If "lsm=" _does_ imply enablement, then how does it interact with
> per-LSM disabling? i.e. what does "apparmor.enabled=0
> lsm=yama,apparmor" mean? If it means "turn on apparmor" how do I turn
> on a CONFIG-default-off LSM without specifying all the other LSMs too?
>
currently using
security=apparmor apparmor=0
means apparmor is the one given the chance to register but it declines
which means you just get capabilities. And with
# caveat not part of the current stacking patchset
security=selinux,apparmor apparmor=0
you end up with
capability,selinux
However apparmor=1 does not imply apparmor is the available LSM
that is
security=selinux apparmor=1
gives you
capability,selinux
if iirc selinux=X behaves the same way
However it is not clear to me whether this is the behavior that we
would want for $lsm.enabled, $lsm.disabled. It appears to be in
conflict with how yama, loadpin and IMA currently work.
Re: [PATCH] apparmor: Fix network performance issue in aa_label_sk_perm
On 09/06/2018 09:33 PM, Tony Jones wrote:
> The netperf benchmark shows a 5.73% reduction in throughput for
> small (64 byte) transfers by unconfined tasks.
>
> DEFINE_AUDIT_SK() in aa_label_sk_perm() should not be performed
> unconditionally, rather only when the label is confined.
>
> netperf-tcp
> 56974a6fc^ 56974a6fc
> Min 64 563.48 ( 0.00%) 531.17 ( -5.73%)
> Min 128 1056.92 ( 0.00%) 999.44 ( -5.44%)
> Min 256 1945.95 ( 0.00%) 1867.97 ( -4.01%)
> Min 1024 6761.40 ( 0.00%) 6364.23 ( -5.87%)
> Min 2048 0.53 ( 0.00%)10606.20 ( -4.54%)
> Min 3312 13692.67 ( 0.00%)13158.41 ( -3.90%)
> Min 4096 14926.29 ( 0.00%)14457.46 ( -3.14%)
> Min 8192 18399.34 ( 0.00%)18091.65 ( -1.67%)
> Min 1638421384.13 ( 0.00%)21158.05 ( -1.06%)
> Hmean 64 564.96 ( 0.00%) 534.38 ( -5.41%)
> Hmean 128 1064.42 ( 0.00%) 1010.12 ( -5.10%)
> Hmean 256 1965.85 ( 0.00%) 1879.16 ( -4.41%)
> Hmean 1024 6839.77 ( 0.00%) 6478.70 ( -5.28%)
> Hmean 2048 11154.80 ( 0.00%)10671.13 ( -4.34%)
> Hmean 3312 13838.12 ( 0.00%)13249.01 ( -4.26%)
> Hmean 4096 15009.99 ( 0.00%)14561.36 ( -2.99%)
> Hmean 8192 18975.57 ( 0.00%)18326.54 ( -3.42%)
> Hmean 1638421440.44 ( 0.00%)21324.59 ( -0.54%)
> Stddev64 1.24 ( 0.00%)2.85 (-130.64%)
> Stddev128 4.51 ( 0.00%)6.53 ( -44.84%)
> Stddev256 11.67 ( 0.00%)8.50 ( 27.16%)
> Stddev102448.33 ( 0.00%) 75.07 ( -55.34%)
> Stddev204854.82 ( 0.00%) 65.16 ( -18.86%)
> Stddev3312 153.57 ( 0.00%) 56.29 ( 63.35%)
> Stddev4096 100.25 ( 0.00%) 88.50 ( 11.72%)
> Stddev8192 358.13 ( 0.00%) 169.99 ( 52.54%)
> Stddev16384 43.99 ( 0.00%) 141.82 (-222.39%)
>
> Signed-off-by: Tony Jones
> Fixes: 56974a6fcfef ("apparmor: add base infastructure for socket
> mediation")
hey Tony,
thanks for the patch, I am curious did you're investigation look
into what parts of DEFINE_AUDIT_SK are causing the issue?
regardless, I have pulled it into apparmor next
> ---
> security/apparmor/net.c | 15 +--
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/security/apparmor/net.c b/security/apparmor/net.c
> index bb24cfa0a164..d5d72dd1ca1f 100644
> --- a/security/apparmor/net.c
> +++ b/security/apparmor/net.c
> @@ -146,17 +146,20 @@ int aa_af_perm(struct aa_label *label, const char *op,
> u32 request, u16 family,
> static int aa_label_sk_perm(struct aa_label *label, const char *op, u32
> request,
> struct sock *sk)
> {
> - struct aa_profile *profile;
> - DEFINE_AUDIT_SK(sa, op, sk);
> + int error = 0;
>
> AA_BUG(!label);
> AA_BUG(!sk);
>
> - if (unconfined(label))
> - return 0;
> + if (!unconfined(label)) {
> + struct aa_profile *profile;
> + DEFINE_AUDIT_SK(sa, op, sk);
>
> - return fn_for_each_confined(label, profile,
> - aa_profile_af_sk_perm(profile, , request, sk));
> + error = fn_for_each_confined(label, profile,
> + aa_profile_af_sk_perm(profile, , request, sk));
> + }
> +
> + return error;
> }
>
> int aa_sk_perm(const char *op, u32 request, struct sock *sk)
>
Re: [PATCH] apparmor: Fix network performance issue in aa_label_sk_perm
On 09/06/2018 09:33 PM, Tony Jones wrote:
> The netperf benchmark shows a 5.73% reduction in throughput for
> small (64 byte) transfers by unconfined tasks.
>
> DEFINE_AUDIT_SK() in aa_label_sk_perm() should not be performed
> unconditionally, rather only when the label is confined.
>
> netperf-tcp
> 56974a6fc^ 56974a6fc
> Min 64 563.48 ( 0.00%) 531.17 ( -5.73%)
> Min 128 1056.92 ( 0.00%) 999.44 ( -5.44%)
> Min 256 1945.95 ( 0.00%) 1867.97 ( -4.01%)
> Min 1024 6761.40 ( 0.00%) 6364.23 ( -5.87%)
> Min 2048 0.53 ( 0.00%)10606.20 ( -4.54%)
> Min 3312 13692.67 ( 0.00%)13158.41 ( -3.90%)
> Min 4096 14926.29 ( 0.00%)14457.46 ( -3.14%)
> Min 8192 18399.34 ( 0.00%)18091.65 ( -1.67%)
> Min 1638421384.13 ( 0.00%)21158.05 ( -1.06%)
> Hmean 64 564.96 ( 0.00%) 534.38 ( -5.41%)
> Hmean 128 1064.42 ( 0.00%) 1010.12 ( -5.10%)
> Hmean 256 1965.85 ( 0.00%) 1879.16 ( -4.41%)
> Hmean 1024 6839.77 ( 0.00%) 6478.70 ( -5.28%)
> Hmean 2048 11154.80 ( 0.00%)10671.13 ( -4.34%)
> Hmean 3312 13838.12 ( 0.00%)13249.01 ( -4.26%)
> Hmean 4096 15009.99 ( 0.00%)14561.36 ( -2.99%)
> Hmean 8192 18975.57 ( 0.00%)18326.54 ( -3.42%)
> Hmean 1638421440.44 ( 0.00%)21324.59 ( -0.54%)
> Stddev64 1.24 ( 0.00%)2.85 (-130.64%)
> Stddev128 4.51 ( 0.00%)6.53 ( -44.84%)
> Stddev256 11.67 ( 0.00%)8.50 ( 27.16%)
> Stddev102448.33 ( 0.00%) 75.07 ( -55.34%)
> Stddev204854.82 ( 0.00%) 65.16 ( -18.86%)
> Stddev3312 153.57 ( 0.00%) 56.29 ( 63.35%)
> Stddev4096 100.25 ( 0.00%) 88.50 ( 11.72%)
> Stddev8192 358.13 ( 0.00%) 169.99 ( 52.54%)
> Stddev16384 43.99 ( 0.00%) 141.82 (-222.39%)
>
> Signed-off-by: Tony Jones
> Fixes: 56974a6fcfef ("apparmor: add base infastructure for socket
> mediation")
hey Tony,
thanks for the patch, I am curious did you're investigation look
into what parts of DEFINE_AUDIT_SK are causing the issue?
regardless, I have pulled it into apparmor next
> ---
> security/apparmor/net.c | 15 +--
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/security/apparmor/net.c b/security/apparmor/net.c
> index bb24cfa0a164..d5d72dd1ca1f 100644
> --- a/security/apparmor/net.c
> +++ b/security/apparmor/net.c
> @@ -146,17 +146,20 @@ int aa_af_perm(struct aa_label *label, const char *op,
> u32 request, u16 family,
> static int aa_label_sk_perm(struct aa_label *label, const char *op, u32
> request,
> struct sock *sk)
> {
> - struct aa_profile *profile;
> - DEFINE_AUDIT_SK(sa, op, sk);
> + int error = 0;
>
> AA_BUG(!label);
> AA_BUG(!sk);
>
> - if (unconfined(label))
> - return 0;
> + if (!unconfined(label)) {
> + struct aa_profile *profile;
> + DEFINE_AUDIT_SK(sa, op, sk);
>
> - return fn_for_each_confined(label, profile,
> - aa_profile_af_sk_perm(profile, , request, sk));
> + error = fn_for_each_confined(label, profile,
> + aa_profile_af_sk_perm(profile, , request, sk));
> + }
> +
> + return error;
> }
>
> int aa_sk_perm(const char *op, u32 request, struct sock *sk)
>
[GIT PULL] apparmor fix for v4.19-rc3
Hi, Please pull this fix for an issue syzbot discovered last week thanks - John The following changes since commit 57361846b52bc686112da6ca5368d11210796804: Linux 4.19-rc2 (2018-09-02 14:37:30 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor tags/apparmor-pr-2018-09-06 for you to fetch changes up to edf4e7b7b9104b58fddfcd073bd7dcc1585d5326: apparmor: fix bad debug check in apparmor_secid_to_secctx() (2018-09-03 11:15:29 -0700) - Fix for bad debug check when converting secids to secctx John Johansen (1): apparmor: fix bad debug check in apparmor_secid_to_secctx() security/apparmor/secid.c | 1 - 1 file changed, 1 deletion(-)
[GIT PULL] apparmor fix for v4.19-rc3
Hi, Please pull this fix for an issue syzbot discovered last week thanks - John The following changes since commit 57361846b52bc686112da6ca5368d11210796804: Linux 4.19-rc2 (2018-09-02 14:37:30 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor tags/apparmor-pr-2018-09-06 for you to fetch changes up to edf4e7b7b9104b58fddfcd073bd7dcc1585d5326: apparmor: fix bad debug check in apparmor_secid_to_secctx() (2018-09-03 11:15:29 -0700) - Fix for bad debug check when converting secids to secctx John Johansen (1): apparmor: fix bad debug check in apparmor_secid_to_secctx() security/apparmor/secid.c | 1 - 1 file changed, 1 deletion(-)
Re: [PATCH 0/8] CaitSith LSM module
On 09/01/2018 06:04 AM, Tetsuo Handa wrote: > On 2017/10/22 2:17, Casey Schaufler wrote: >>> As one year elapsed since I proposed CaitSith for upstream, I'd like to >>> hear the status again. I looked at >>> http://schd.ws/hosted_files/lss2017/8b/201709-LinuxSecuritySummit-Stacking.pdf >>> . >>> How is ETA for Security Module Stacking? Is it a half year or so? >> >> Assuming that I can keep working on stacking at my current level, >> and that we can work out a couple issues with audit and networking >> there is a possibility we're looking at mid 2018 for stacking. The >> increased interest in security module namespaces for containers is >> helping make stacking seem important to more people. >> >>> If it is likely take longer, should I resume proposing CaitSith for now >>> as one of "Minor modules" except security_module_enable() check added >>> until Security Module Stacking work completes? Or should I wait for >>> completion of stacking work? I want to know it, for recent proposals are >>> rather staying silent. >> >> I wouldn't wait if it was my project, but I have been known >> to be more aggressive than is good for me from time to time. >> > > It seems that stacking needs some more time. Manual with updated patch for > current source code is available at https://caitsith.osdn.jp/index2.html . > John, if you can resume reviewing, I'd like to propose CaitSith as one of > "Minor modules" except security_module_enable() check. > Hey Tetsuo, yes finishing up the Caitsith review is on my short list
Re: [PATCH 0/8] CaitSith LSM module
On 09/01/2018 06:04 AM, Tetsuo Handa wrote: > On 2017/10/22 2:17, Casey Schaufler wrote: >>> As one year elapsed since I proposed CaitSith for upstream, I'd like to >>> hear the status again. I looked at >>> http://schd.ws/hosted_files/lss2017/8b/201709-LinuxSecuritySummit-Stacking.pdf >>> . >>> How is ETA for Security Module Stacking? Is it a half year or so? >> >> Assuming that I can keep working on stacking at my current level, >> and that we can work out a couple issues with audit and networking >> there is a possibility we're looking at mid 2018 for stacking. The >> increased interest in security module namespaces for containers is >> helping make stacking seem important to more people. >> >>> If it is likely take longer, should I resume proposing CaitSith for now >>> as one of "Minor modules" except security_module_enable() check added >>> until Security Module Stacking work completes? Or should I wait for >>> completion of stacking work? I want to know it, for recent proposals are >>> rather staying silent. >> >> I wouldn't wait if it was my project, but I have been known >> to be more aggressive than is good for me from time to time. >> > > It seems that stacking needs some more time. Manual with updated patch for > current source code is available at https://caitsith.osdn.jp/index2.html . > John, if you can resume reviewing, I'd like to propose CaitSith as one of > "Minor modules" except security_module_enable() check. > Hey Tetsuo, yes finishing up the Caitsith review is on my short list
Re: WARNING in apparmor_secid_to_secctx
On 09/01/2018 09:33 PM, Dmitry Vyukov wrote: > On Sat, Sep 1, 2018 at 11:18 AM, John Johansen > wrote: >> On 08/29/2018 07:17 PM, syzbot wrote: >>> Hello, >>> >>> syzbot found the following crash on: >>> >>> HEAD commit:817e60a7a2bb Merge branch 'nfp-add-NFP5000-support' >>> git tree: net-next >>> console output: https://syzkaller.appspot.com/x/log.txt?x=1536d29640 >>> kernel config: https://syzkaller.appspot.com/x/.config?x=531a917630d2a492 >>> dashboard link: https://syzkaller.appspot.com/bug?extid=21016130b0580a9de3b5 >>> compiler: gcc (GCC) 8.0.1 20180413 (experimental) >>> >>> Unfortunately, I don't have any reproducer for this crash yet. >>> >>> IMPORTANT: if you fix the bug, please add the following tag to the commit: >>> Reported-by: syzbot+21016130b0580a9de...@syzkaller.appspotmail.com >>> >> >> << snip >> >> >> Patch sent directly to syzbot for testing > > Hi John, > > What do you mean? syzbot has not received any test requests for this, > and it would reply within half an hour or so. Where is that patch? > Hrmmm strange I followed the web instruction and attached the patch to the reply. The patch is below, its also available at git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor 4.18-syzbot-secid --- >From 22dad84baabf4174f11f5e9b34a05529084fa29c Mon Sep 17 00:00:00 2001 From: John Johansen Date: Sat, 1 Sep 2018 01:57:52 -0700 Subject: [PATCH] apparmor: fix apparmor_secid_to_secctx incorrect debug triggering WARN_ON apparmor_secid_to_secctx() has a bad debug statement tripping on a condition handle by the code. When kconfig SECURITY_APPARMOR_DEBUG is enabled the debug WARN_ON will trip when **secdata is NULL resulting in the following trace. [ cut here ] AppArmor WARN apparmor_secid_to_secctx: ((!secdata)): WARNING: CPU: 0 PID: 14826 at security/apparmor/secid.c:82 apparmor_secid_to_secctx+0x2b5/0x2f0 security/apparmor/secid.c:82 Kernel panic - not syncing: panic_on_warn set ... CPU: 0 PID: 14826 Comm: syz-executor1 Not tainted 4.19.0-rc1+ #193 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Call Trace: __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0x1c9/0x2b4 lib/dump_stack.c:113 panic+0x238/0x4e7 kernel/panic.c:184 __warn.cold.8+0x163/0x1ba kernel/panic.c:536 report_bug+0x252/0x2d0 lib/bug.c:186 fixup_bug arch/x86/kernel/traps.c:178 [inline] do_error_trap+0x1fc/0x4d0 arch/x86/kernel/traps.c:296 do_invalid_op+0x1b/0x20 arch/x86/kernel/traps.c:316 invalid_op+0x14/0x20 arch/x86/entry/entry_64.S:993 RIP: 0010:apparmor_secid_to_secctx+0x2b5/0x2f0 security/apparmor/secid.c:82 Code: c7 c7 40 66 58 87 e8 6a 6d 0f fe 0f 0b e9 6c fe ff ff e8 3e aa 44 fe 48 c7 c6 80 67 58 87 48 c7 c7 a0 65 58 87 e8 4b 6d 0f fe <0f> 0b e9 3f fe ff ff 48 89 df e8 fc a7 83 fe e9 ed fe ff ff bb f4 RSP: 0018:8801ba1bed10 EFLAGS: 00010286 RAX: RBX: 8801ba1beed0 RCX: c9000227e000 RDX: 00018482 RSI: 8163ac01 RDI: 0001 RBP: 8801ba1bed30 R08: 8801b80ec080 R09: ed003b603eca R10: ed003b603eca R11: 8801db01f657 R12: 0001 R13: R14: R15: 8801ba1beed0 security_secid_to_secctx+0x63/0xc0 security/security.c:1314 ctnetlink_secctx_size net/netfilter/nf_conntrack_netlink.c:621 [inline] ctnetlink_nlmsg_size net/netfilter/nf_conntrack_netlink.c:659 [inline] ctnetlink_conntrack_event+0x303/0x1470 net/netfilter/nf_conntrack_netlink.c:706 nf_conntrack_eventmask_report+0x55f/0x930 net/netfilter/nf_conntrack_ecache.c:151 nf_conntrack_event_report include/net/netfilter/nf_conntrack_ecache.h:112 [inline] nf_ct_delete+0x33c/0x5d0 net/netfilter/nf_conntrack_core.c:601 nf_ct_iterate_cleanup+0x48c/0x5e0 net/netfilter/nf_conntrack_core.c:1892 nf_ct_iterate_cleanup_net+0x23c/0x2d0 net/netfilter/nf_conntrack_core.c:1974 ctnetlink_flush_conntrack net/netfilter/nf_conntrack_netlink.c:1226 [inline] ctnetlink_del_conntrack+0x66c/0x850 net/netfilter/nf_conntrack_netlink.c:1258 nfnetlink_rcv_msg+0xd88/0x1070 net/netfilter/nfnetlink.c:228 netlink_rcv_skb+0x172/0x440 net/netlink/af_netlink.c:2454 nfnetlink_rcv+0x1c0/0x4d0 net/netfilter/nfnetlink.c:560 netlink_unicast_kernel net/netlink/af_netlink.c:1317 [inline] netlink_unicast+0x5a0/0x760 net/netlink/af_netlink.c:1343 netlink_sendmsg+0xa18/0xfc0 net/netlink/af_netlink.c:1908 sock_sendmsg_nosec net/socket.c:621 [inline] sock_sendmsg+0xd5/0x120 net/socket.c:631 ___sys_sendmsg+0x7fd/0x930 net/socket.c:2114 __sys_sendmsg+0x11d/0x290 net/socket.c:2152 __do_sys_sendmsg net/socket.c:2161 [inline] __se_sys_sendmsg net/socket.c:2159 [inline] __x64_sys_sendmsg+0x78/0xb0 net/socket.c:2159 do_syscall_64+0x1b9/0x820 arch/x86/e
Re: WARNING in apparmor_secid_to_secctx
On 09/01/2018 09:33 PM, Dmitry Vyukov wrote: > On Sat, Sep 1, 2018 at 11:18 AM, John Johansen > wrote: >> On 08/29/2018 07:17 PM, syzbot wrote: >>> Hello, >>> >>> syzbot found the following crash on: >>> >>> HEAD commit:817e60a7a2bb Merge branch 'nfp-add-NFP5000-support' >>> git tree: net-next >>> console output: https://syzkaller.appspot.com/x/log.txt?x=1536d29640 >>> kernel config: https://syzkaller.appspot.com/x/.config?x=531a917630d2a492 >>> dashboard link: https://syzkaller.appspot.com/bug?extid=21016130b0580a9de3b5 >>> compiler: gcc (GCC) 8.0.1 20180413 (experimental) >>> >>> Unfortunately, I don't have any reproducer for this crash yet. >>> >>> IMPORTANT: if you fix the bug, please add the following tag to the commit: >>> Reported-by: syzbot+21016130b0580a9de...@syzkaller.appspotmail.com >>> >> >> << snip >> >> >> Patch sent directly to syzbot for testing > > Hi John, > > What do you mean? syzbot has not received any test requests for this, > and it would reply within half an hour or so. Where is that patch? > Hrmmm strange I followed the web instruction and attached the patch to the reply. The patch is below, its also available at git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor 4.18-syzbot-secid --- >From 22dad84baabf4174f11f5e9b34a05529084fa29c Mon Sep 17 00:00:00 2001 From: John Johansen Date: Sat, 1 Sep 2018 01:57:52 -0700 Subject: [PATCH] apparmor: fix apparmor_secid_to_secctx incorrect debug triggering WARN_ON apparmor_secid_to_secctx() has a bad debug statement tripping on a condition handle by the code. When kconfig SECURITY_APPARMOR_DEBUG is enabled the debug WARN_ON will trip when **secdata is NULL resulting in the following trace. [ cut here ] AppArmor WARN apparmor_secid_to_secctx: ((!secdata)): WARNING: CPU: 0 PID: 14826 at security/apparmor/secid.c:82 apparmor_secid_to_secctx+0x2b5/0x2f0 security/apparmor/secid.c:82 Kernel panic - not syncing: panic_on_warn set ... CPU: 0 PID: 14826 Comm: syz-executor1 Not tainted 4.19.0-rc1+ #193 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Call Trace: __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0x1c9/0x2b4 lib/dump_stack.c:113 panic+0x238/0x4e7 kernel/panic.c:184 __warn.cold.8+0x163/0x1ba kernel/panic.c:536 report_bug+0x252/0x2d0 lib/bug.c:186 fixup_bug arch/x86/kernel/traps.c:178 [inline] do_error_trap+0x1fc/0x4d0 arch/x86/kernel/traps.c:296 do_invalid_op+0x1b/0x20 arch/x86/kernel/traps.c:316 invalid_op+0x14/0x20 arch/x86/entry/entry_64.S:993 RIP: 0010:apparmor_secid_to_secctx+0x2b5/0x2f0 security/apparmor/secid.c:82 Code: c7 c7 40 66 58 87 e8 6a 6d 0f fe 0f 0b e9 6c fe ff ff e8 3e aa 44 fe 48 c7 c6 80 67 58 87 48 c7 c7 a0 65 58 87 e8 4b 6d 0f fe <0f> 0b e9 3f fe ff ff 48 89 df e8 fc a7 83 fe e9 ed fe ff ff bb f4 RSP: 0018:8801ba1bed10 EFLAGS: 00010286 RAX: RBX: 8801ba1beed0 RCX: c9000227e000 RDX: 00018482 RSI: 8163ac01 RDI: 0001 RBP: 8801ba1bed30 R08: 8801b80ec080 R09: ed003b603eca R10: ed003b603eca R11: 8801db01f657 R12: 0001 R13: R14: R15: 8801ba1beed0 security_secid_to_secctx+0x63/0xc0 security/security.c:1314 ctnetlink_secctx_size net/netfilter/nf_conntrack_netlink.c:621 [inline] ctnetlink_nlmsg_size net/netfilter/nf_conntrack_netlink.c:659 [inline] ctnetlink_conntrack_event+0x303/0x1470 net/netfilter/nf_conntrack_netlink.c:706 nf_conntrack_eventmask_report+0x55f/0x930 net/netfilter/nf_conntrack_ecache.c:151 nf_conntrack_event_report include/net/netfilter/nf_conntrack_ecache.h:112 [inline] nf_ct_delete+0x33c/0x5d0 net/netfilter/nf_conntrack_core.c:601 nf_ct_iterate_cleanup+0x48c/0x5e0 net/netfilter/nf_conntrack_core.c:1892 nf_ct_iterate_cleanup_net+0x23c/0x2d0 net/netfilter/nf_conntrack_core.c:1974 ctnetlink_flush_conntrack net/netfilter/nf_conntrack_netlink.c:1226 [inline] ctnetlink_del_conntrack+0x66c/0x850 net/netfilter/nf_conntrack_netlink.c:1258 nfnetlink_rcv_msg+0xd88/0x1070 net/netfilter/nfnetlink.c:228 netlink_rcv_skb+0x172/0x440 net/netlink/af_netlink.c:2454 nfnetlink_rcv+0x1c0/0x4d0 net/netfilter/nfnetlink.c:560 netlink_unicast_kernel net/netlink/af_netlink.c:1317 [inline] netlink_unicast+0x5a0/0x760 net/netlink/af_netlink.c:1343 netlink_sendmsg+0xa18/0xfc0 net/netlink/af_netlink.c:1908 sock_sendmsg_nosec net/socket.c:621 [inline] sock_sendmsg+0xd5/0x120 net/socket.c:631 ___sys_sendmsg+0x7fd/0x930 net/socket.c:2114 __sys_sendmsg+0x11d/0x290 net/socket.c:2152 __do_sys_sendmsg net/socket.c:2161 [inline] __se_sys_sendmsg net/socket.c:2159 [inline] __x64_sys_sendmsg+0x78/0xb0 net/socket.c:2159 do_syscall_64+0x1b9/0x820 arch/x86/e
Re: WARNING in apparmor_secid_to_secctx
On 08/29/2018 07:17 PM, syzbot wrote: > Hello, > > syzbot found the following crash on: > > HEAD commit: 817e60a7a2bb Merge branch 'nfp-add-NFP5000-support' > git tree: net-next > console output: https://syzkaller.appspot.com/x/log.txt?x=1536d29640 > kernel config: https://syzkaller.appspot.com/x/.config?x=531a917630d2a492 > dashboard link: https://syzkaller.appspot.com/bug?extid=21016130b0580a9de3b5 > compiler: gcc (GCC) 8.0.1 20180413 (experimental) > > Unfortunately, I don't have any reproducer for this crash yet. > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > Reported-by: syzbot+21016130b0580a9de...@syzkaller.appspotmail.com > << snip >> Patch sent directly to syzbot for testing
Re: WARNING in apparmor_secid_to_secctx
On 08/29/2018 07:17 PM, syzbot wrote: > Hello, > > syzbot found the following crash on: > > HEAD commit: 817e60a7a2bb Merge branch 'nfp-add-NFP5000-support' > git tree: net-next > console output: https://syzkaller.appspot.com/x/log.txt?x=1536d29640 > kernel config: https://syzkaller.appspot.com/x/.config?x=531a917630d2a492 > dashboard link: https://syzkaller.appspot.com/bug?extid=21016130b0580a9de3b5 > compiler: gcc (GCC) 8.0.1 20180413 (experimental) > > Unfortunately, I don't have any reproducer for this crash yet. > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > Reported-by: syzbot+21016130b0580a9de...@syzkaller.appspotmail.com > << snip >> Patch sent directly to syzbot for testing
[GIT PULL] apparmor updates for v4.19
Hi, Please pull these apparmor changes for v4.19. There is nothing major this time just 4 bug fixes and a patch to remove so dead code. Thanks! - John The following changes since commit fb7d1bcf1602b46f37ada72178516c01a250e434: Merge tag 'pci-v4.18-fixes-3' of git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci (2018-07-19 11:54:04 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor tags/apparmor-pr-2018-08-23 for you to fetch changes up to c037bd615885f1d9d3bdb48531bace79fae1505d: apparmor: remove no-op permission check in policy_unpack (2018-08-22 18:44:42 -0700) + Cleanups - apparmor: remove no-op permission check in policy_unpack + Bug fixes - apparmor: fix an error code in __aa_create_ns() - apparmor: Fix failure to audit context info in build_change_hat - apparmor: Check buffer bounds when mapping permissions mask - apparmor: Fully initialize aa_perms struct when answering userspace query Dan Carpenter (1): apparmor: fix an error code in __aa_create_ns() John Johansen (2): apparmor: Fix failure to audit context info in build_change_hat apparmor: remove no-op permission check in policy_unpack Tyler Hicks (2): apparmor: Check buffer bounds when mapping permissions mask apparmor: Fully initialize aa_perms struct when answering userspace query security/apparmor/apparmorfs.c| 5 + security/apparmor/domain.c| 2 +- security/apparmor/file.c | 3 ++- security/apparmor/include/perms.h | 3 ++- security/apparmor/lib.c | 17 + security/apparmor/policy_ns.c | 2 +- security/apparmor/policy_unpack.c | 32 7 files changed, 20 insertions(+), 44 deletions(-)
[GIT PULL] apparmor updates for v4.19
Hi, Please pull these apparmor changes for v4.19. There is nothing major this time just 4 bug fixes and a patch to remove so dead code. Thanks! - John The following changes since commit fb7d1bcf1602b46f37ada72178516c01a250e434: Merge tag 'pci-v4.18-fixes-3' of git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci (2018-07-19 11:54:04 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor tags/apparmor-pr-2018-08-23 for you to fetch changes up to c037bd615885f1d9d3bdb48531bace79fae1505d: apparmor: remove no-op permission check in policy_unpack (2018-08-22 18:44:42 -0700) + Cleanups - apparmor: remove no-op permission check in policy_unpack + Bug fixes - apparmor: fix an error code in __aa_create_ns() - apparmor: Fix failure to audit context info in build_change_hat - apparmor: Check buffer bounds when mapping permissions mask - apparmor: Fully initialize aa_perms struct when answering userspace query Dan Carpenter (1): apparmor: fix an error code in __aa_create_ns() John Johansen (2): apparmor: Fix failure to audit context info in build_change_hat apparmor: remove no-op permission check in policy_unpack Tyler Hicks (2): apparmor: Check buffer bounds when mapping permissions mask apparmor: Fully initialize aa_perms struct when answering userspace query security/apparmor/apparmorfs.c| 5 + security/apparmor/domain.c| 2 +- security/apparmor/file.c | 3 ++- security/apparmor/include/perms.h | 3 ++- security/apparmor/lib.c | 17 + security/apparmor/policy_ns.c | 2 +- security/apparmor/policy_unpack.c | 32 7 files changed, 20 insertions(+), 44 deletions(-)
Re: [PATCH] apparmor: remove unused label
On 08/23/2018 07:09 AM, Arnd Bergmann wrote:
thank you for the patch, but a fix for this issue was pushed to apparmor-next
yesterday
> After the corresponding 'goto' was removed, we get a warning
> for the 'fail' label:
>
> security/apparmor/policy_unpack.c: In function 'unpack_dfa':
> security/apparmor/policy_unpack.c:426:1: error: label 'fail' defined but not
> used [-Werror=unused-label]
>
> Fixes: fb5841091f28 ("apparmor: remove no-op permission check in
> policy_unpack")
> Signed-off-by: Arnd Bergmann
> ---
> security/apparmor/policy_unpack.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/security/apparmor/policy_unpack.c
> b/security/apparmor/policy_unpack.c
> index 3647b5834ace..96d8cf68ce65 100644
> --- a/security/apparmor/policy_unpack.c
> +++ b/security/apparmor/policy_unpack.c
> @@ -423,7 +423,6 @@ static struct aa_dfa *unpack_dfa(struct aa_ext *e)
>
> return dfa;
>
> -fail:
> aa_put_dfa(dfa);
> return ERR_PTR(-EPROTO);
> }
>
Re: [PATCH] apparmor: remove unused label
On 08/23/2018 07:09 AM, Arnd Bergmann wrote:
thank you for the patch, but a fix for this issue was pushed to apparmor-next
yesterday
> After the corresponding 'goto' was removed, we get a warning
> for the 'fail' label:
>
> security/apparmor/policy_unpack.c: In function 'unpack_dfa':
> security/apparmor/policy_unpack.c:426:1: error: label 'fail' defined but not
> used [-Werror=unused-label]
>
> Fixes: fb5841091f28 ("apparmor: remove no-op permission check in
> policy_unpack")
> Signed-off-by: Arnd Bergmann
> ---
> security/apparmor/policy_unpack.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/security/apparmor/policy_unpack.c
> b/security/apparmor/policy_unpack.c
> index 3647b5834ace..96d8cf68ce65 100644
> --- a/security/apparmor/policy_unpack.c
> +++ b/security/apparmor/policy_unpack.c
> @@ -423,7 +423,6 @@ static struct aa_dfa *unpack_dfa(struct aa_ext *e)
>
> return dfa;
>
> -fail:
> aa_put_dfa(dfa);
> return ERR_PTR(-EPROTO);
> }
>
Re: [PATCH] apparmor: remove dead code
On 08/23/2018 06:42 AM, Gustavo A. R. Silva wrote:
thank you for the patch, but a fix for this issue was pushed to apparmor-next
yesterday
> Due to commit fb5841091f28 ("apparmor: remove no-op permission check
> in policy_unpack"), there is some leftover code.
>
> Coverity reports this issue as Structurally dead code. Fix this by
> removing such code.
>
> Addresses-Coverity-ID: 1472998 ("Structurally dead code")
> Fixes: fb5841091f28 ("apparmor: remove no-op permission check in
> policy_unpack")
> Signed-off-by: Gustavo A. R. Silva
> ---
> security/apparmor/policy_unpack.c | 4
> 1 file changed, 4 deletions(-)
>
> diff --git a/security/apparmor/policy_unpack.c
> b/security/apparmor/policy_unpack.c
> index 3647b58..21cb384 100644
> --- a/security/apparmor/policy_unpack.c
> +++ b/security/apparmor/policy_unpack.c
> @@ -422,10 +422,6 @@ static struct aa_dfa *unpack_dfa(struct aa_ext *e)
> }
>
> return dfa;
> -
> -fail:
> - aa_put_dfa(dfa);
> - return ERR_PTR(-EPROTO);
> }
>
> /**
>
Re: [PATCH] apparmor: remove dead code
On 08/23/2018 06:42 AM, Gustavo A. R. Silva wrote:
thank you for the patch, but a fix for this issue was pushed to apparmor-next
yesterday
> Due to commit fb5841091f28 ("apparmor: remove no-op permission check
> in policy_unpack"), there is some leftover code.
>
> Coverity reports this issue as Structurally dead code. Fix this by
> removing such code.
>
> Addresses-Coverity-ID: 1472998 ("Structurally dead code")
> Fixes: fb5841091f28 ("apparmor: remove no-op permission check in
> policy_unpack")
> Signed-off-by: Gustavo A. R. Silva
> ---
> security/apparmor/policy_unpack.c | 4
> 1 file changed, 4 deletions(-)
>
> diff --git a/security/apparmor/policy_unpack.c
> b/security/apparmor/policy_unpack.c
> index 3647b58..21cb384 100644
> --- a/security/apparmor/policy_unpack.c
> +++ b/security/apparmor/policy_unpack.c
> @@ -422,10 +422,6 @@ static struct aa_dfa *unpack_dfa(struct aa_ext *e)
> }
>
> return dfa;
> -
> -fail:
> - aa_put_dfa(dfa);
> - return ERR_PTR(-EPROTO);
> }
>
> /**
>
Re: linux-next: build warning after merge of the apparmor tree
On 08/22/2018 05:20 PM, Stephen Rothwell wrote:
> Hi John,
>
> After merging the apparmor tree, today's linux-next build (x86_64
> allmodconfig) produced this warning:
>
> security/apparmor/policy_unpack.c: In function 'unpack_dfa':
> security/apparmor/policy_unpack.c:426:1: warning: label 'fail' defined but
> not used [-Wunused-label]
> fail:
> ^~~~
>
> Introduced by commit
>
> fb5841091f28 ("apparmor: remove no-op permission check in policy_unpack")
>
sorry
fixed
Re: linux-next: build warning after merge of the apparmor tree
On 08/22/2018 05:20 PM, Stephen Rothwell wrote:
> Hi John,
>
> After merging the apparmor tree, today's linux-next build (x86_64
> allmodconfig) produced this warning:
>
> security/apparmor/policy_unpack.c: In function 'unpack_dfa':
> security/apparmor/policy_unpack.c:426:1: warning: label 'fail' defined but
> not used [-Wunused-label]
> fail:
> ^~~~
>
> Introduced by commit
>
> fb5841091f28 ("apparmor: remove no-op permission check in policy_unpack")
>
sorry
fixed
Re: [PATCH] apparmor: remove redundant pointer 'info'
On 07/14/2018 09:19 AM, Colin King wrote:
> From: Colin Ian King
>
> Pointer 'info' is being assigned but is never used hence it is
> redundant and can be removed.
>
> Cleans up clang warning:
> warning: variable 'info' set but not used [-Wunused-but-set-variable]
>
NAK,
real problem wrong fix, instead of deleting the additional context info
we need to be auditing it
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 098d546d8253..08c88de0ffda 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -1036,7 +1036,7 @@ static struct aa_label *build_change_hat(struct
aa_profile *profile,
audit:
aa_audit_file(profile, , OP_CHANGE_HAT, AA_MAY_CHANGEHAT,
name, hat ? hat->base.hname : NULL,
- hat ? >label : NULL, GLOBAL_ROOT_UID, NULL,
+ hat ? >label : NULL, GLOBAL_ROOT_UID, info,
error);
if (!hat || (error && error != -ENOENT))
return ERR_PTR(error);
I pushed this fix into apparmor-next
> Signed-off-by: Colin Ian King
> ---
> security/apparmor/domain.c | 6 +-
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index 098d546d8253..410d9ce09861 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -1006,7 +1006,6 @@ static struct aa_label *build_change_hat(struct
> aa_profile *profile,
>const char *name, bool sibling)
> {
> struct aa_profile *root, *hat = NULL;
> - const char *info = NULL;
> int error = 0;
>
> if (sibling && PROFILE_IS_HAT(profile)) {
> @@ -1014,7 +1013,6 @@ static struct aa_label *build_change_hat(struct
> aa_profile *profile,
> } else if (!sibling && !PROFILE_IS_HAT(profile)) {
> root = aa_get_profile(profile);
> } else {
> - info = "conflicting target types";
> error = -EPERM;
> goto audit;
> }
> @@ -1025,10 +1023,8 @@ static struct aa_label *build_change_hat(struct
> aa_profile *profile,
> if (COMPLAIN_MODE(profile)) {
> hat = aa_new_null_profile(profile, true, name,
> GFP_KERNEL);
> - if (!hat) {
> - info = "failed null profile create";
> + if (!hat)
> error = -ENOMEM;
> - }
> }
> }
> aa_put_profile(root);
>
Re: [PATCH] apparmor: remove redundant pointer 'info'
On 07/14/2018 09:19 AM, Colin King wrote:
> From: Colin Ian King
>
> Pointer 'info' is being assigned but is never used hence it is
> redundant and can be removed.
>
> Cleans up clang warning:
> warning: variable 'info' set but not used [-Wunused-but-set-variable]
>
NAK,
real problem wrong fix, instead of deleting the additional context info
we need to be auditing it
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 098d546d8253..08c88de0ffda 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -1036,7 +1036,7 @@ static struct aa_label *build_change_hat(struct
aa_profile *profile,
audit:
aa_audit_file(profile, , OP_CHANGE_HAT, AA_MAY_CHANGEHAT,
name, hat ? hat->base.hname : NULL,
- hat ? >label : NULL, GLOBAL_ROOT_UID, NULL,
+ hat ? >label : NULL, GLOBAL_ROOT_UID, info,
error);
if (!hat || (error && error != -ENOENT))
return ERR_PTR(error);
I pushed this fix into apparmor-next
> Signed-off-by: Colin Ian King
> ---
> security/apparmor/domain.c | 6 +-
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index 098d546d8253..410d9ce09861 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -1006,7 +1006,6 @@ static struct aa_label *build_change_hat(struct
> aa_profile *profile,
>const char *name, bool sibling)
> {
> struct aa_profile *root, *hat = NULL;
> - const char *info = NULL;
> int error = 0;
>
> if (sibling && PROFILE_IS_HAT(profile)) {
> @@ -1014,7 +1013,6 @@ static struct aa_label *build_change_hat(struct
> aa_profile *profile,
> } else if (!sibling && !PROFILE_IS_HAT(profile)) {
> root = aa_get_profile(profile);
> } else {
> - info = "conflicting target types";
> error = -EPERM;
> goto audit;
> }
> @@ -1025,10 +1023,8 @@ static struct aa_label *build_change_hat(struct
> aa_profile *profile,
> if (COMPLAIN_MODE(profile)) {
> hat = aa_new_null_profile(profile, true, name,
> GFP_KERNEL);
> - if (!hat) {
> - info = "failed null profile create";
> + if (!hat)
> error = -ENOMEM;
> - }
> }
> }
> aa_put_profile(root);
>
Re: [PATCH 2/2] apparmor: Fully initialize aa_perms struct when answering userspace query
On 07/05/2018 10:25 PM, Tyler Hicks wrote:
> Fully initialize the aa_perms struct in profile_query_cb() to avoid the
> potential of using an uninitialized struct member's value in a response
> to a query from userspace.
>
> Detected by CoverityScan CID#1415126 ("Uninitialized scalar variable")
>
> Fixes: 4f3b3f2d79a4 ("apparmor: add profile permission query ability">
> Signed-off-by: Tyler Hicks
I've pulled this into apparmor-next
> ---
> security/apparmor/apparmorfs.c | 5 +
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
> index 949dd8a48164..e09fe4d7307c 100644
> --- a/security/apparmor/apparmorfs.c
> +++ b/security/apparmor/apparmorfs.c
> @@ -603,7 +603,7 @@ static const struct file_operations
> aa_fs_ns_revision_fops = {
> static void profile_query_cb(struct aa_profile *profile, struct aa_perms
> *perms,
>const char *match_str, size_t match_len)
> {
> - struct aa_perms tmp;
> + struct aa_perms tmp = { };
> struct aa_dfa *dfa;
> unsigned int state = 0;
>
> @@ -613,7 +613,6 @@ static void profile_query_cb(struct aa_profile *profile,
> struct aa_perms *perms,
> dfa = profile->file.dfa;
> state = aa_dfa_match_len(dfa, profile->file.start,
>match_str + 1, match_len - 1);
> - tmp = nullperms;
> if (state) {
> struct path_cond cond = { };
>
> @@ -627,8 +626,6 @@ static void profile_query_cb(struct aa_profile *profile,
> struct aa_perms *perms,
>match_str, match_len);
> if (state)
> aa_compute_perms(dfa, state, );
> - else
> - tmp = nullperms;
> }
> aa_apply_modes_to_perms(profile, );
> aa_perms_accum_raw(perms, );
>
Re: [PATCH 2/2] apparmor: Fully initialize aa_perms struct when answering userspace query
On 07/05/2018 10:25 PM, Tyler Hicks wrote:
> Fully initialize the aa_perms struct in profile_query_cb() to avoid the
> potential of using an uninitialized struct member's value in a response
> to a query from userspace.
>
> Detected by CoverityScan CID#1415126 ("Uninitialized scalar variable")
>
> Fixes: 4f3b3f2d79a4 ("apparmor: add profile permission query ability">
> Signed-off-by: Tyler Hicks
I've pulled this into apparmor-next
> ---
> security/apparmor/apparmorfs.c | 5 +
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
> index 949dd8a48164..e09fe4d7307c 100644
> --- a/security/apparmor/apparmorfs.c
> +++ b/security/apparmor/apparmorfs.c
> @@ -603,7 +603,7 @@ static const struct file_operations
> aa_fs_ns_revision_fops = {
> static void profile_query_cb(struct aa_profile *profile, struct aa_perms
> *perms,
>const char *match_str, size_t match_len)
> {
> - struct aa_perms tmp;
> + struct aa_perms tmp = { };
> struct aa_dfa *dfa;
> unsigned int state = 0;
>
> @@ -613,7 +613,6 @@ static void profile_query_cb(struct aa_profile *profile,
> struct aa_perms *perms,
> dfa = profile->file.dfa;
> state = aa_dfa_match_len(dfa, profile->file.start,
>match_str + 1, match_len - 1);
> - tmp = nullperms;
> if (state) {
> struct path_cond cond = { };
>
> @@ -627,8 +626,6 @@ static void profile_query_cb(struct aa_profile *profile,
> struct aa_perms *perms,
>match_str, match_len);
> if (state)
> aa_compute_perms(dfa, state, );
> - else
> - tmp = nullperms;
> }
> aa_apply_modes_to_perms(profile, );
> aa_perms_accum_raw(perms, );
>
Re: [PATCH 1/2] apparmor: Check buffer bounds when mapping permissions mask
On 07/05/2018 10:25 PM, Tyler Hicks wrote:
> Don't read past the end of the buffer containing permissions
> characters or write past the end of the destination string.
>
> Detected by CoverityScan CID#1415361, 1415376 ("Out-of-bounds access")
>
> Fixes: e53cfe6c7caa ("apparmor: rework perm mapping to a slightly broader
> set")
> Signed-off-by: Tyler Hicks
I've pulled this into apparmor-next with a minor modification noted below
> security/apparmor/file.c | 3 ++-
> security/apparmor/include/perms.h | 3 ++-
> security/apparmor/lib.c | 17 +
> 3 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/security/apparmor/file.c b/security/apparmor/file.c
> index 224b2fef93ca..4285943f7260 100644
> --- a/security/apparmor/file.c
> +++ b/security/apparmor/file.c
> @@ -47,7 +47,8 @@ static void audit_file_mask(struct audit_buffer *ab, u32
> mask)
> {
> char str[10];
>
> - aa_perm_mask_to_str(str, aa_file_perm_chrs, map_mask_to_chr_mask(mask));
> + aa_perm_mask_to_str(str, sizeof(str), aa_file_perm_chrs,
> + map_mask_to_chr_mask(mask));
> audit_log_string(ab, str);
> }
>
> diff --git a/security/apparmor/include/perms.h
> b/security/apparmor/include/perms.h
> index 38aa6247d00f..b94ec114d1a4 100644
> --- a/security/apparmor/include/perms.h
> +++ b/security/apparmor/include/perms.h
> @@ -137,7 +137,8 @@ extern struct aa_perms allperms;
> xcheck(fn_for_each((L1), (P), (FN1)), fn_for_each((L2), (P), (FN2)))
>
>
> -void aa_perm_mask_to_str(char *str, const char *chrs, u32 mask);
> +void aa_perm_mask_to_str(char *str, size_t str_size, const char *chrs,
> + u32 mask);
> void aa_audit_perm_names(struct audit_buffer *ab, const char * const *names,
>u32 mask);
> void aa_audit_perm_mask(struct audit_buffer *ab, u32 mask, const char *chrs,
> diff --git a/security/apparmor/lib.c b/security/apparmor/lib.c
> index a7b3f681b80e..7ab368c3789b 100644
> --- a/security/apparmor/lib.c
> +++ b/security/apparmor/lib.c
> @@ -198,15 +198,24 @@ const char *aa_file_perm_names[] = {
> /**
> * aa_perm_mask_to_str - convert a perm mask to its short string
> * @str: character buffer to store string in (at least 10 characters)
> + * @str_size: size of the @str buffer
> + * @chrs: NUL-terminated character buffer of permission characters
> * @mask: permission mask to convert
> */
> -void aa_perm_mask_to_str(char *str, const char *chrs, u32 mask)
> +void aa_perm_mask_to_str(char *str, size_t str_size, const char *chrs, u32
> mask)
> {
> unsigned int i, perm = 1;
> + size_t num_chrs = strlen(chrs);
> +
> + for (i = 0; i < num_chrs; perm <<= 1, i++> +if (mask &
> perm) {
> + /* Ensure that one byte is left for NUL-termination */
> + if (WARN_ON_ONCE(str_size <= 1))
> + continue;
might as well break here
>
> - for (i = 0; i < 32; perm <<= 1, i++) {
> - if (mask & perm)
> *str++ = chrs[i];
> + str_size--;
> + }
> }
> *str = '\0';
> }
> @@ -236,7 +245,7 @@ void aa_audit_perm_mask(struct audit_buffer *ab, u32
> mask, const char *chrs,
>
> audit_log_format(ab, "\"");
> if ((mask & chrsmask) && chrs) {
> - aa_perm_mask_to_str(str, chrs, mask & chrsmask);
> + aa_perm_mask_to_str(str, sizeof(str), chrs, mask & chrsmask);
> mask &= ~chrsmask;
> audit_log_format(ab, "%s", str);
> if (mask & namesmask)
>
Re: [PATCH 1/2] apparmor: Check buffer bounds when mapping permissions mask
On 07/05/2018 10:25 PM, Tyler Hicks wrote:
> Don't read past the end of the buffer containing permissions
> characters or write past the end of the destination string.
>
> Detected by CoverityScan CID#1415361, 1415376 ("Out-of-bounds access")
>
> Fixes: e53cfe6c7caa ("apparmor: rework perm mapping to a slightly broader
> set")
> Signed-off-by: Tyler Hicks
I've pulled this into apparmor-next with a minor modification noted below
> security/apparmor/file.c | 3 ++-
> security/apparmor/include/perms.h | 3 ++-
> security/apparmor/lib.c | 17 +
> 3 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/security/apparmor/file.c b/security/apparmor/file.c
> index 224b2fef93ca..4285943f7260 100644
> --- a/security/apparmor/file.c
> +++ b/security/apparmor/file.c
> @@ -47,7 +47,8 @@ static void audit_file_mask(struct audit_buffer *ab, u32
> mask)
> {
> char str[10];
>
> - aa_perm_mask_to_str(str, aa_file_perm_chrs, map_mask_to_chr_mask(mask));
> + aa_perm_mask_to_str(str, sizeof(str), aa_file_perm_chrs,
> + map_mask_to_chr_mask(mask));
> audit_log_string(ab, str);
> }
>
> diff --git a/security/apparmor/include/perms.h
> b/security/apparmor/include/perms.h
> index 38aa6247d00f..b94ec114d1a4 100644
> --- a/security/apparmor/include/perms.h
> +++ b/security/apparmor/include/perms.h
> @@ -137,7 +137,8 @@ extern struct aa_perms allperms;
> xcheck(fn_for_each((L1), (P), (FN1)), fn_for_each((L2), (P), (FN2)))
>
>
> -void aa_perm_mask_to_str(char *str, const char *chrs, u32 mask);
> +void aa_perm_mask_to_str(char *str, size_t str_size, const char *chrs,
> + u32 mask);
> void aa_audit_perm_names(struct audit_buffer *ab, const char * const *names,
>u32 mask);
> void aa_audit_perm_mask(struct audit_buffer *ab, u32 mask, const char *chrs,
> diff --git a/security/apparmor/lib.c b/security/apparmor/lib.c
> index a7b3f681b80e..7ab368c3789b 100644
> --- a/security/apparmor/lib.c
> +++ b/security/apparmor/lib.c
> @@ -198,15 +198,24 @@ const char *aa_file_perm_names[] = {
> /**
> * aa_perm_mask_to_str - convert a perm mask to its short string
> * @str: character buffer to store string in (at least 10 characters)
> + * @str_size: size of the @str buffer
> + * @chrs: NUL-terminated character buffer of permission characters
> * @mask: permission mask to convert
> */
> -void aa_perm_mask_to_str(char *str, const char *chrs, u32 mask)
> +void aa_perm_mask_to_str(char *str, size_t str_size, const char *chrs, u32
> mask)
> {
> unsigned int i, perm = 1;
> + size_t num_chrs = strlen(chrs);
> +
> + for (i = 0; i < num_chrs; perm <<= 1, i++> +if (mask &
> perm) {
> + /* Ensure that one byte is left for NUL-termination */
> + if (WARN_ON_ONCE(str_size <= 1))
> + continue;
might as well break here
>
> - for (i = 0; i < 32; perm <<= 1, i++) {
> - if (mask & perm)
> *str++ = chrs[i];
> + str_size--;
> + }
> }
> *str = '\0';
> }
> @@ -236,7 +245,7 @@ void aa_audit_perm_mask(struct audit_buffer *ab, u32
> mask, const char *chrs,
>
> audit_log_format(ab, "\"");
> if ((mask & chrsmask) && chrs) {
> - aa_perm_mask_to_str(str, chrs, mask & chrsmask);
> + aa_perm_mask_to_str(str, sizeof(str), chrs, mask & chrsmask);
> mask &= ~chrsmask;
> audit_log_format(ab, "%s", str);
> if (mask & namesmask)
>
[GIT PULL] apparmor updates for v4.18
Hi, Please pull these apparmor changes for v4.18 Thanks! - John The following changes since commit 552c69b36ebd966186573b9c7a286b390935cce1: Merge tag 'v4.17-rc3' into apparmor-next (2018-05-02 00:38:52 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor tags/apparmor-pr-2018-06-13 for you to fetch changes up to 338d0be437ef10e247a35aed83dbab182cf406a2: apparmor: fix ptrace read check (2018-06-07 01:51:02 -0700) + Features - add support for mapping secids and using secctxes - add the ability to get a task's secid - add support for audit rule filtering + Cleanups - multiple typo fixes - Convert to use match_string() helper - update git and wiki locations in AppArmor docs - improve get_buffers macro by using get_cpu_ptr - Use an IDR to allocate apparmor secids + Bug fixes - fix '*seclen' is never less than zero - fix mediation of prlimit - fix memory leak when deduping profile load - fix ptrace read check - fix memory leak of rule on error exit path Andy Shevchenko (1): apparmor: Convert to use match_string() helper John Johansen (9): apparmor: add support for mapping secids and using secctxes apparmor: add the ability to get a task's secid apparmor: fix '*seclen' is never less than zero apparmor: improve get_buffers macro by using get_cpu_ptr apparmor: modify audit rule support to support profile stacks apparmor: fixup secid map conversion to using IDR apparmor: fix mediation of prlimit apparmor: fix memory leak when deduping profile load apparmor: fix ptrace read check Jordan Glover (1): apparmor: update git and wiki locations in AppArmor docs Matthew Garrett (1): apparmor: Add support for audit rule filtering Matthew Wilcox (1): apparmor: Use an IDR to allocate apparmor secids Tyler Hicks (1): apparmor: Fix memory leak of rule on error exit path Zygmunt Krynicki (7): apparmor: fix typo "loosen" apparmor: fix typo "comparison" apparmor: fix typo "replace" apparmor: fix typo "type" apparmor: fix typo "traverse" apparmor: fix typo "independent" apparmor: fix typo "preconfinement" Documentation/admin-guide/LSM/apparmor.rst | 6 +- security/apparmor/audit.c | 90 - security/apparmor/domain.c | 2 +- security/apparmor/include/audit.h | 6 ++ security/apparmor/include/label.h | 2 +- security/apparmor/include/path.h | 33 +++ security/apparmor/include/secid.h | 17 +++- security/apparmor/label.c | 15 ++- security/apparmor/lib.c| 2 +- security/apparmor/lsm.c| 50 +++--- security/apparmor/match.c | 2 +- security/apparmor/mount.c | 2 +- security/apparmor/policy.c | 7 +- security/apparmor/resource.c | 2 +- security/apparmor/secid.c | 151 + 15 files changed, 313 insertions(+), 74 deletions(-)
[GIT PULL] apparmor updates for v4.18
Hi, Please pull these apparmor changes for v4.18 Thanks! - John The following changes since commit 552c69b36ebd966186573b9c7a286b390935cce1: Merge tag 'v4.17-rc3' into apparmor-next (2018-05-02 00:38:52 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor tags/apparmor-pr-2018-06-13 for you to fetch changes up to 338d0be437ef10e247a35aed83dbab182cf406a2: apparmor: fix ptrace read check (2018-06-07 01:51:02 -0700) + Features - add support for mapping secids and using secctxes - add the ability to get a task's secid - add support for audit rule filtering + Cleanups - multiple typo fixes - Convert to use match_string() helper - update git and wiki locations in AppArmor docs - improve get_buffers macro by using get_cpu_ptr - Use an IDR to allocate apparmor secids + Bug fixes - fix '*seclen' is never less than zero - fix mediation of prlimit - fix memory leak when deduping profile load - fix ptrace read check - fix memory leak of rule on error exit path Andy Shevchenko (1): apparmor: Convert to use match_string() helper John Johansen (9): apparmor: add support for mapping secids and using secctxes apparmor: add the ability to get a task's secid apparmor: fix '*seclen' is never less than zero apparmor: improve get_buffers macro by using get_cpu_ptr apparmor: modify audit rule support to support profile stacks apparmor: fixup secid map conversion to using IDR apparmor: fix mediation of prlimit apparmor: fix memory leak when deduping profile load apparmor: fix ptrace read check Jordan Glover (1): apparmor: update git and wiki locations in AppArmor docs Matthew Garrett (1): apparmor: Add support for audit rule filtering Matthew Wilcox (1): apparmor: Use an IDR to allocate apparmor secids Tyler Hicks (1): apparmor: Fix memory leak of rule on error exit path Zygmunt Krynicki (7): apparmor: fix typo "loosen" apparmor: fix typo "comparison" apparmor: fix typo "replace" apparmor: fix typo "type" apparmor: fix typo "traverse" apparmor: fix typo "independent" apparmor: fix typo "preconfinement" Documentation/admin-guide/LSM/apparmor.rst | 6 +- security/apparmor/audit.c | 90 - security/apparmor/domain.c | 2 +- security/apparmor/include/audit.h | 6 ++ security/apparmor/include/label.h | 2 +- security/apparmor/include/path.h | 33 +++ security/apparmor/include/secid.h | 17 +++- security/apparmor/label.c | 15 ++- security/apparmor/lib.c| 2 +- security/apparmor/lsm.c| 50 +++--- security/apparmor/match.c | 2 +- security/apparmor/mount.c | 2 +- security/apparmor/policy.c | 7 +- security/apparmor/resource.c | 2 +- security/apparmor/secid.c | 151 + 15 files changed, 313 insertions(+), 74 deletions(-)
Re: [PATCH] Use an IDR to allocate apparmor secids
On 06/05/2018 04:47 AM, Matthew Wilcox wrote:
> On Mon, Jun 04, 2018 at 07:35:24PM -0700, John Johansen wrote:
>> On 06/04/2018 07:27 PM, Matthew Wilcox wrote:
>>> On Mon, Jun 04, 2018 at 06:27:09PM -0700, John Johansen wrote:
>>>> hey Mathew,
>>>>
>>>> I've pulled this into apparmor-next and done the retuning of
>>>> AA_SECID_INVALID a follow on patch. The reworking of the api to
>>>> return the specific error type can wait for another cycle.
>>>
>>> Oh ... here's what I currently have. I decided that AA_SECID_INVALID
>>> wasn't needed.
>>>
>> well not needed in the allocation path, but definitely needed and it
>> needs to be 0.
>>
>> This is for catching some uninitialized or freed and zeroed values.
>> The debug checks aren't in the current version, as they were
>> residing in another debug patch, but I will pull them out into their
>> own patch.
>
> With the IDR, I don't know if you need it for debug.
>
> BUG_ON(label != idr_find(_secids, label->secid))
>
> should do the trick.
>
its not so much the idr as the network and audit structs where the
secid gets used and then security system is asked to convert from
the secid to the secctx.
We need an invalid secid for when conversion result in an error,
partly because the returned error is ignored in some paths (eg. scm)
so we also make sure to have an invalid value.
Having it be zero helps us catch bugs on structs that are zero
but we miss initializing, and also works well with apparmor always
zeroing structs on free.
The debug patch didn't get included yet because the networking
code that make use of the secids hasn't landed yet (they are being
used in the audit path) so the debug patch ends up throwing a
lot of warning for the networking paths.
The patch I am testing on top of your patch is below
---
commit d5de3b1d21687c16df0a75b6309ab8481629a841
Author: John Johansen
Date: Mon Jun 4 19:44:59 2018 -0700
apparmor: cleanup secid map convertion to using idr
The idr convertion didn't handle an error case for when allocating a
mapping fails.
In addition it did not ensure that mappings did not allocate or use a
0 value, which is used as an invalid secid because it allows debug
code to detect when objects have not been correctly initialized or
freed too early.
Signed-off-by: John Johansen
diff --git a/security/apparmor/include/secid.h
b/security/apparmor/include/secid.h
index 686de8e50a79..dee6fa3b6081 100644
--- a/security/apparmor/include/secid.h
+++ b/security/apparmor/include/secid.h
@@ -28,8 +28,10 @@ int apparmor_secctx_to_secid(const char *secdata, u32
seclen, u32 *secid);
void apparmor_release_secctx(char *secdata, u32 seclen);
-u32 aa_alloc_secid(struct aa_label *label, gfp_t gfp);
+int aa_alloc_secid(struct aa_label *label, gfp_t gfp);
void aa_free_secid(u32 secid);
void aa_secid_update(u32 secid, struct aa_label *label);
+void aa_secids_init(void);
+
#endif /* __AA_SECID_H */
diff --git a/security/apparmor/label.c b/security/apparmor/label.c
index a17574df611b..ba11bdf9043a 100644
--- a/security/apparmor/label.c
+++ b/security/apparmor/label.c
@@ -407,8 +407,7 @@ bool aa_label_init(struct aa_label *label, int size, gfp_t
gfp)
AA_BUG(!label);
AA_BUG(size < 1);
- label->secid = aa_alloc_secid(label, gfp);
- if (label->secid == AA_SECID_INVALID)
+ if (aa_alloc_secid(label, gfp) < 0)
return false;
label->size = size; /* doesn't include null */
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index dab5409f2608..9ae7f9339513 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -1546,6 +1546,8 @@ static int __init apparmor_init(void)
return 0;
}
+ aa_secids_init();
+
error = aa_setup_dfa_engine();
if (error) {
AA_ERROR("Unable to setup dfa engine\n");
diff --git a/security/apparmor/secid.c b/security/apparmor/secid.c
index 3ad94b2ffbb2..ad6221e1f25f 100644
--- a/security/apparmor/secid.c
+++ b/security/apparmor/secid.c
@@ -33,6 +33,8 @@
* properly updating/freeing them
*/
+#define AA_FIRST_SECID 1
+
static DEFINE_IDR(aa_secids);
static DEFINE_SPINLOCK(secid_lock);
@@ -120,20 +122,30 @@ void apparmor_release_secctx(char *secdata, u32 seclen)
/**
* aa_alloc_secid - allocate a new secid for a profile
+ * @label: the label to allocate a secid for
+ * @gfp: memory allocation flags
+ *
+ * Returns: 0 with @label->secid initialized
+ * <0 returns error with @label->secid set to AA_SECID_INVALID
*/
-u32 aa_alloc_secid(struct aa_label *label, gfp_t gfp)
+int aa_alloc_secid(struct aa_label *label, gfp_t gfp)
{
unsigned long flags;
- u32 secid;
+ int
Re: [PATCH] Use an IDR to allocate apparmor secids
On 06/05/2018 04:47 AM, Matthew Wilcox wrote:
> On Mon, Jun 04, 2018 at 07:35:24PM -0700, John Johansen wrote:
>> On 06/04/2018 07:27 PM, Matthew Wilcox wrote:
>>> On Mon, Jun 04, 2018 at 06:27:09PM -0700, John Johansen wrote:
>>>> hey Mathew,
>>>>
>>>> I've pulled this into apparmor-next and done the retuning of
>>>> AA_SECID_INVALID a follow on patch. The reworking of the api to
>>>> return the specific error type can wait for another cycle.
>>>
>>> Oh ... here's what I currently have. I decided that AA_SECID_INVALID
>>> wasn't needed.
>>>
>> well not needed in the allocation path, but definitely needed and it
>> needs to be 0.
>>
>> This is for catching some uninitialized or freed and zeroed values.
>> The debug checks aren't in the current version, as they were
>> residing in another debug patch, but I will pull them out into their
>> own patch.
>
> With the IDR, I don't know if you need it for debug.
>
> BUG_ON(label != idr_find(_secids, label->secid))
>
> should do the trick.
>
its not so much the idr as the network and audit structs where the
secid gets used and then security system is asked to convert from
the secid to the secctx.
We need an invalid secid for when conversion result in an error,
partly because the returned error is ignored in some paths (eg. scm)
so we also make sure to have an invalid value.
Having it be zero helps us catch bugs on structs that are zero
but we miss initializing, and also works well with apparmor always
zeroing structs on free.
The debug patch didn't get included yet because the networking
code that make use of the secids hasn't landed yet (they are being
used in the audit path) so the debug patch ends up throwing a
lot of warning for the networking paths.
The patch I am testing on top of your patch is below
---
commit d5de3b1d21687c16df0a75b6309ab8481629a841
Author: John Johansen
Date: Mon Jun 4 19:44:59 2018 -0700
apparmor: cleanup secid map convertion to using idr
The idr convertion didn't handle an error case for when allocating a
mapping fails.
In addition it did not ensure that mappings did not allocate or use a
0 value, which is used as an invalid secid because it allows debug
code to detect when objects have not been correctly initialized or
freed too early.
Signed-off-by: John Johansen
diff --git a/security/apparmor/include/secid.h
b/security/apparmor/include/secid.h
index 686de8e50a79..dee6fa3b6081 100644
--- a/security/apparmor/include/secid.h
+++ b/security/apparmor/include/secid.h
@@ -28,8 +28,10 @@ int apparmor_secctx_to_secid(const char *secdata, u32
seclen, u32 *secid);
void apparmor_release_secctx(char *secdata, u32 seclen);
-u32 aa_alloc_secid(struct aa_label *label, gfp_t gfp);
+int aa_alloc_secid(struct aa_label *label, gfp_t gfp);
void aa_free_secid(u32 secid);
void aa_secid_update(u32 secid, struct aa_label *label);
+void aa_secids_init(void);
+
#endif /* __AA_SECID_H */
diff --git a/security/apparmor/label.c b/security/apparmor/label.c
index a17574df611b..ba11bdf9043a 100644
--- a/security/apparmor/label.c
+++ b/security/apparmor/label.c
@@ -407,8 +407,7 @@ bool aa_label_init(struct aa_label *label, int size, gfp_t
gfp)
AA_BUG(!label);
AA_BUG(size < 1);
- label->secid = aa_alloc_secid(label, gfp);
- if (label->secid == AA_SECID_INVALID)
+ if (aa_alloc_secid(label, gfp) < 0)
return false;
label->size = size; /* doesn't include null */
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index dab5409f2608..9ae7f9339513 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -1546,6 +1546,8 @@ static int __init apparmor_init(void)
return 0;
}
+ aa_secids_init();
+
error = aa_setup_dfa_engine();
if (error) {
AA_ERROR("Unable to setup dfa engine\n");
diff --git a/security/apparmor/secid.c b/security/apparmor/secid.c
index 3ad94b2ffbb2..ad6221e1f25f 100644
--- a/security/apparmor/secid.c
+++ b/security/apparmor/secid.c
@@ -33,6 +33,8 @@
* properly updating/freeing them
*/
+#define AA_FIRST_SECID 1
+
static DEFINE_IDR(aa_secids);
static DEFINE_SPINLOCK(secid_lock);
@@ -120,20 +122,30 @@ void apparmor_release_secctx(char *secdata, u32 seclen)
/**
* aa_alloc_secid - allocate a new secid for a profile
+ * @label: the label to allocate a secid for
+ * @gfp: memory allocation flags
+ *
+ * Returns: 0 with @label->secid initialized
+ * <0 returns error with @label->secid set to AA_SECID_INVALID
*/
-u32 aa_alloc_secid(struct aa_label *label, gfp_t gfp)
+int aa_alloc_secid(struct aa_label *label, gfp_t gfp)
{
unsigned long flags;
- u32 secid;
+ int
Re: [PATCH] Use an IDR to allocate apparmor secids
On 06/04/2018 07:27 PM, Matthew Wilcox wrote: > On Mon, Jun 04, 2018 at 06:27:09PM -0700, John Johansen wrote: >> hey Mathew, >> >> I've pulled this into apparmor-next and done the retuning of >> AA_SECID_INVALID a follow on patch. The reworking of the api to >> return the specific error type can wait for another cycle. > > Oh ... here's what I currently have. I decided that AA_SECID_INVALID > wasn't needed. > well not needed in the allocation path, but definitely needed and it needs to be 0. This is for catching some uninitialized or freed and zeroed values. The debug checks aren't in the current version, as they were residing in another debug patch, but I will pull them out into their own patch.
Re: [PATCH] Use an IDR to allocate apparmor secids
On 06/04/2018 07:27 PM, Matthew Wilcox wrote: > On Mon, Jun 04, 2018 at 06:27:09PM -0700, John Johansen wrote: >> hey Mathew, >> >> I've pulled this into apparmor-next and done the retuning of >> AA_SECID_INVALID a follow on patch. The reworking of the api to >> return the specific error type can wait for another cycle. > > Oh ... here's what I currently have. I decided that AA_SECID_INVALID > wasn't needed. > well not needed in the allocation path, but definitely needed and it needs to be 0. This is for catching some uninitialized or freed and zeroed values. The debug checks aren't in the current version, as they were residing in another debug patch, but I will pull them out into their own patch.
Re: [PATCH] Use an IDR to allocate apparmor secids
On 05/28/2018 10:01 AM, Matthew Wilcox wrote:
>
> ping?
>
> I have this queued up in my XArray tree. If I don't hear from you before
> -rc1, I'll be submitting it as part of the XArray conversion.
>
hey Mathew,
I've pulled this into apparmor-next and done the retuning of
AA_SECID_INVALID a follow on patch. The reworking of the api to
return the specific error type can wait for another cycle.
> On Tue, May 22, 2018 at 02:32:59AM -0700, Matthew Wilcox wrote:
>> Replace the custom usage of the radix tree to store a list of free IDs
>> with the IDR.
>>
>> Signed-off-by: Matthew Wilcox
>>
>> security/apparmor/secid.c | 114
>> --
>> 1 file changed, 11 insertions(+), 103 deletions(-)
>>
>> diff --git a/security/apparmor/secid.c b/security/apparmor/secid.c
>> index c2f0c1571156..3ad94b2ffbb2 100644
>> --- a/security/apparmor/secid.c
>> +++ b/security/apparmor/secid.c
>> @@ -18,6 +18,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>> #include
>>
>> @@ -30,18 +31,10 @@
>> /*
>> * secids - do not pin labels with a refcount. They rely on the label
>> * properly updating/freeing them
>> - *
>> - * A singly linked free list is used to track secids that have been
>> - * freed and reuse them before allocating new ones
>> */
>>
>> -#define FREE_LIST_HEAD 1
>> -
>> -static RADIX_TREE(aa_secids_map, GFP_ATOMIC);
>> +static DEFINE_IDR(aa_secids);
>> static DEFINE_SPINLOCK(secid_lock);
>> -static u32 alloced_secid = FREE_LIST_HEAD;
>> -static u32 free_list = FREE_LIST_HEAD;
>> -static unsigned long free_count;
>>
>> /*
>> * TODO: allow policy to reserve a secid range?
>> @@ -49,65 +42,6 @@ static unsigned long free_count;
>> * TODO: use secid_update in label replace
>> */
>>
>> -#define SECID_MAX U32_MAX
>> -
>> -/* TODO: mark free list as exceptional */
>> -static void *to_ptr(u32 secid)
>> -{
>> -return (void *)
>> -unsigned long) secid) << RADIX_TREE_EXCEPTIONAL_SHIFT));
>> -}
>> -
>> -static u32 to_secid(void *ptr)
>> -{
>> -return (u32) (((unsigned long) ptr) >> RADIX_TREE_EXCEPTIONAL_SHIFT);
>> -}
>> -
>> -
>> -/* TODO: tag free_list entries to mark them as different */
>> -static u32 __pop(struct aa_label *label)
>> -{
>> -u32 secid = free_list;
>> -void __rcu **slot;
>> -void *entry;
>> -
>> -if (free_list == FREE_LIST_HEAD)
>> -return AA_SECID_INVALID;
>> -
>> -slot = radix_tree_lookup_slot(_secids_map, secid);
>> -AA_BUG(!slot);
>> -entry = radix_tree_deref_slot_protected(slot, _lock);
>> -free_list = to_secid(entry);
>> -radix_tree_replace_slot(_secids_map, slot, label);
>> -free_count--;
>> -
>> -return secid;
>> -}
>> -
>> -static void __push(u32 secid)
>> -{
>> -void __rcu **slot;
>> -
>> -slot = radix_tree_lookup_slot(_secids_map, secid);
>> -AA_BUG(!slot);
>> -radix_tree_replace_slot(_secids_map, slot, to_ptr(free_list));
>> -free_list = secid;
>> -free_count++;
>> -}
>> -
>> -static struct aa_label * __secid_update(u32 secid, struct aa_label *label)
>> -{
>> -struct aa_label *old;
>> -void __rcu **slot;
>> -
>> -slot = radix_tree_lookup_slot(_secids_map, secid);
>> -AA_BUG(!slot);
>> -old = radix_tree_deref_slot_protected(slot, _lock);
>> -radix_tree_replace_slot(_secids_map, slot, label);
>> -
>> -return old;
>> -}
>> -
>> /**
>> * aa_secid_update - update a secid mapping to a new label
>> * @secid: secid to update
>> @@ -115,11 +49,10 @@ static struct aa_label * __secid_update(u32 secid,
>> struct aa_label *label)
>> */
>> void aa_secid_update(u32 secid, struct aa_label *label)
>> {
>> -struct aa_label *old;
>> unsigned long flags;
>>
>> spin_lock_irqsave(_lock, flags);
>> -old = __secid_update(secid, label);
>> +idr_replace(_secids, label, secid);
>> spin_unlock_irqrestore(_lock, flags);
>> }
>>
>> @@ -132,7 +65,7 @@ struct aa_label *aa_secid_to_label(u32 secid)
>> struct aa_label *label;
>>
>> rcu_read_lock();
>> -label = radix_tree_lookup(_secids_map, secid);
>> +label = idr_find(_secids, secid);
>> rcu_read_unlock();
>>
>> return label;
>> @@ -167,7 +100,6 @@ int apparmor_secid_to_secctx(u32 secid, char **secdata,
>> u32 *seclen)
>> return 0;
>> }
>>
>> -
>> int apparmor_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid)
>> {
>> struct aa_label *label;
>> @@ -186,7 +118,6 @@ void apparmor_release_secctx(char *secdata, u32 seclen)
>> kfree(secdata);
>> }
>>
>> -
>> /**
>> * aa_alloc_secid - allocate a new secid for a profile
>> */
>> @@ -195,35 +126,12 @@ u32 aa_alloc_secid(struct aa_label *label, gfp_t gfp)
>> unsigned long flags;
>> u32 secid;
>>
>> -/* racey, but at worst causes new allocation instead of reuse */
>> -if (free_list == FREE_LIST_HEAD) {
>> -bool preload = 0;
>> -int res;
>> -
>> -retry:
>> -
