Re: [PATCH 10/12] apparmorfs: Replace CURRENT_TIME with current_time()
On 04/07/2017 05:57 PM, Deepa Dinamani wrote: > CURRENT_TIME macro is not y2038 safe on 32 bit systems. > > The patch replaces all the uses of CURRENT_TIME by > current_time(). > > This is also in preparation for the patch that transitions > vfs timestamps to use 64 bit time and hence make them > y2038 safe. current_time() is also planned to be transitioned > to y2038 safe behavior along with this change. > > CURRENT_TIME macro will be deleted before merging the > aforementioned change. > > Signed-off-by: Deepa Dinamani This is all good, and I have no objections to it being merged for 4.12. If it isn't this change is already queued up for the apparmor 4.13 merge Acked-by: John Johansen > --- > 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 be0b498..4f6ac9d 100644 > --- a/security/apparmor/apparmorfs.c > +++ b/security/apparmor/apparmorfs.c > @@ -1357,7 +1357,7 @@ static int aa_mk_null_file(struct dentry *parent) > > inode->i_ino = get_next_ino(); > inode->i_mode = S_IFCHR | S_IRUGO | S_IWUGO; > - inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME; > + inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode); > init_special_inode(inode, S_IFCHR | S_IRUGO | S_IWUGO, > MKDEV(MEM_MAJOR, 3)); > d_instantiate(dentry, inode); > -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: LSM stacking in next for 6.1?
On 10/25/22 02:48, Tetsuo Handa wrote: On 2022/10/25 1:37, Casey Schaufler wrote: What I'm insisting is that "warrant the freedom to load loadable LSM modules without recompiling the whole kernel". Since security modules are optional and the LSM infrastructure itself is optional you can't ensure that any given kernel would support a loadable security module. Like I propose adding EXPORT_SYMBOL_GPL(security_hook_heads), I'm not taking about distributors who choose CONFIG_SECURITY=n. Adding EXPORT_SYMBOL_GPL(security_hook_heads) is the only way that can "allow LSM modules which distributors cannot support to be legally loaded". I believe that I've identified an alternative. It isn't easy or cheap. No. You are just handwaving/postponing the problem using something unknown that is not yet shown as a workable code. Anything that can be disabled via kernel config option cannot be an alternative. Uhmmm, loadable LSM modules if they ever happen will have a kernel config. If not distros will carry a patch just like they have for unprivileged user namespaces. Trying to force distros to allow out of tree code just isn't going to work. Quoting from https://lkml.kernel.org/r/2225aec6-f0f3-d38e-ee3c-6139a7c25...@i-love.sakura.ne.jp > Like Paul Moore said > > However, I will caution that it is becoming increasingly difficult for people > to find time to review potential new LSMs so it may a while to attract sufficient > comments and feedback. > > , being unable to legally use loadable LSMs deprives of chances to develop/try > new LSMs, and makes LSM interface more and more unattractive. The consequence > would be "The LSM interface is dead. We will give up implementing as LSMs." The biggest problem is that quite few developers show interest in loadable LSM modules. How many developers responded to this topic? Once the ability to allow loadable LSM modules is technically lost, nobody shall be able to revive it. You will be happy with ignoring poor people. You are already and completely trapped into "only in-tree and supported by distributors is correct" crap. no, Casey is not. He is trying to find a path forward to get LSM stacking upstream sooner than later. He has made proposals that admittedly you have not liked, but he has at least tried to propose ideas that could work within the insane set of constraints. Of course the upstream kernel isn't going to have LSM IDs for out-of-tree security modules. That's one of many reasons loadable modules are going to have to be treated differently from built-in modules, if they're allowed at all. Then, I have to hate your idea of having fixed sized array. Nacked-by: Tetsuo Handa -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: LSM stacking in next for 6.1?
On 10/26/22 03:19, Tetsuo Handa wrote: On 2022/10/26 7:41, Casey Schaufler wrote: You need a built-in LSM that loads and manages loadable security modules. That is no longer loadable LSM modules. A loadable LSM module must be capable of loading any code and using any interface that is allowed to loadable kernel modules using /sbin/insmod command. That is my understanding of what you have promised (and the reason I am allowing you to continue working on LSM stacking before I make CONFIG_SECURITY_TOMOYO=m). Tetsuo, think of it this way. LSM stacking is going to make it much easier for new LSM modules because they won't automatically be excluded because one of the other LSMs is needed. The problem of loadable LSM modules is orthogonal, and Casey shouldn't need to solve it in this patch series. That is further work to be taken up by another, as Casey has clearly stated its work he is not interested in doing. However the real problem you are trying to solve won't be solved by loadable LSM modules, though they may help. Just having loadable LSMs modules won't mean a distro will build an LSM as a loadable module instead of disabling it, nor does it mean a distro will allow loading an out of tree LSM module. Even if the upstream kernel doesn't provide an option to block loading them, distros will. -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: LSM stacking in next for 6.1?
On 10/29/22 21:03, Tetsuo Handa wrote: On 2022/10/28 19:14, John Johansen wrote: On 10/26/22 03:19, Tetsuo Handa wrote: On 2022/10/26 7:41, Casey Schaufler wrote: You need a built-in LSM that loads and manages loadable security modules. That is no longer loadable LSM modules. A loadable LSM module must be capable of loading any code and using any interface that is allowed to loadable kernel modules using /sbin/insmod command. That is my understanding of what you have promised (and the reason I am allowing you to continue working on LSM stacking before I make CONFIG_SECURITY_TOMOYO=m). Tetsuo, think of it this way. LSM stacking is going to make it much easier for new LSM modules because they won't automatically be excluded because one of the other LSMs is needed. The problem of loadable LSM modules is orthogonal, and Casey shouldn't need to solve it in this patch series. That is further work to be taken up by another, as Casey has clearly stated its work he is not interested in doing. However the real problem you are trying to solve won't be solved by loadable LSM modules, though they may help. Just having loadable LSMs modules won't mean a distro will build an LSM as a loadable module instead of disabling it, nor does it mean a distro will allow loading an out of tree LSM module. Even if the upstream kernel doesn't provide an option to block loading them, distros will. What do you think the background of Ultimately the challenge is getting userspace developers to accept a change that is not part of the upstream Linux Kernel and thus not guaranteed under the usual "don't break userspace" kernel API promise. is? I consider that the reason is because We do care about userspace programs and users who are using userspace programs. . If we don't care about userspace and users, we would not mind breaking APIs. Like it or not kernel developers draw a very distinct line between userspace APIs and what is considered kernel code. This reasoning is more stronger than "we don't care about out-of-tree code" reasoning. Look many developers really just don't care about out of tree code, and others well I won't say they don't care but its impossible task to monitor and think about how the broad swath of different out of tree code bits could be affected by kernel changes. So the only practical thing to do is make changes based on what is in tree and let out of tree projects deal with making the adjustments needed to adapt to the changing kernel. This comes about because the kernel does NOT provide a stable ABI for out of tree code. This is not a technical position as other projects do provide stable ABIs for out of tree code, with varying degrees of success. It is a community/political position that is certainly informed by technical merits but also other considerations, like GPL, experience on the costs of maintaining stable ABIs and a desire to where possible push for code inclusion in the kernel. Distributors have rights to block loading loadable kernel modules which are not included in upstream kernels. But for example Red Hat is not excising the rights to block loading loadable kernel modules (e.g. https://access.redhat.com/solutions/1376133 ). What do you think about the reasons of not blocking loading loadable kernel modules which are not included in upstream kernels? I consider that the reason is because Allowing loadable kernel modules which cannot be supported by distributors to exist and to be loaded into distributor kernels is more beneficial for users. For some users yes. I am not saying a distro will want to block it for everyone one every kernel just that they need the option. Ubuntu has taken the position to try to be as inclusive as possible on the base distro kernels but even with that position we have special kernels that take a very different approach. That is, we have been allowing out-of-tree kernel code to exist because out-of-tree kernel code can be beneficial for users despite distributors cannot afford supporting out-of-tree kernel code. yes, no argument If you really think that we have the rights to lock out out-of-tree kernel code and/or disable loading of out-of-tree kernel code via /sbin/insmod, firstly achieve (1) Disallow loading of non-GPL kernel modules. (It is a trivial change.) I have no desire to get involved the that debate :) (2) Disallow loading of out-of-tree kernel code via /sbin/insmod . Depending on the kernel we do it either by disabling the loading of modules or via requiring modules to be signed. We have kernels that do this. (I don't know how we can technically enforce such change. Unlike not assigning LSM id value to LSM modules, we need to somehow be able to check whether an arbitrary file is in-tree (e.g. forbid "git add some_file") and unmodified (e.g. f
Re: [PATCH v19 13/23] LSM: Specify which LSM to display
On 7/27/20 1:36 PM, James Morris wrote: > On Fri, 24 Jul 2020, 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 > > jj: do you have any review/feedback on this? > yeah, I am working my way through it today -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v19 16/23] LSM: Use lsmcontext in security_inode_getsecctx
On 7/24/20 1:32 PM, Casey Schaufler wrote:
> Change the security_inode_getsecctx() interface to fill
> a lsmcontext structure instead of data and length pointers.
> This provides the information about which LSM created the
> context so that security_release_secctx() can use the
> correct hook.
>
Reviewed-by: John Johansen
> Acked-by: Stephen Smalley
> Signed-off-by: Casey Schaufler
> ---
> fs/nfsd/nfs4xdr.c| 23 +--
> include/linux/security.h | 5 +++--
> security/security.c | 13 +++--
> 3 files changed, 23 insertions(+), 18 deletions(-)
>
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 61d6b8a0e8f0..6673221d5606 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -2379,11 +2379,11 @@ nfsd4_encode_layout_types(struct xdr_stream *xdr, u32
> layout_types)
> #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> static inline __be32
> nfsd4_encode_security_label(struct xdr_stream *xdr, struct svc_rqst *rqstp,
> - void *context, int len)
> + struct lsmcontext *context)
> {
> __be32 *p;
>
> - p = xdr_reserve_space(xdr, len + 4 + 4 + 4);
> + p = xdr_reserve_space(xdr, context->len + 4 + 4 + 4);
> if (!p)
> return nfserr_resource;
>
> @@ -2393,13 +2393,13 @@ nfsd4_encode_security_label(struct xdr_stream *xdr,
> struct svc_rqst *rqstp,
>*/
> *p++ = cpu_to_be32(0); /* lfs */
> *p++ = cpu_to_be32(0); /* pi */
> - p = xdr_encode_opaque(p, context, len);
> + p = xdr_encode_opaque(p, context->context, context->len);
> return 0;
> }
> #else
> static inline __be32
> nfsd4_encode_security_label(struct xdr_stream *xdr, struct svc_rqst *rqstp,
> - void *context, int len)
> + struct lsmcontext *context)
> { return 0; }
> #endif
>
> @@ -2496,9 +2496,7 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct
> svc_fh *fhp,
> int err;
> struct nfs4_acl *acl = NULL;
> #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> - struct lsmcontext scaff; /* scaffolding */
> - void *context = NULL;
> - int contextlen;
> + struct lsmcontext context = { };
> #endif
> bool contextsupport = false;
> struct nfsd4_compoundres *resp = rqstp->rq_resp;
> @@ -2556,7 +2554,7 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct
> svc_fh *fhp,
>bmval0 & FATTR4_WORD0_SUPPORTED_ATTRS) {
> if (exp->ex_flags & NFSEXP_SECURITY_LABEL)
> err = security_inode_getsecctx(d_inode(dentry),
> - &context, &contextlen);
> +&context);
> else
> err = -EOPNOTSUPP;
> contextsupport = (err == 0);
> @@ -2986,8 +2984,7 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct
> svc_fh *fhp,
>
> #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> if (bmval2 & FATTR4_WORD2_SECURITY_LABEL) {
> - status = nfsd4_encode_security_label(xdr, rqstp, context,
> - contextlen);
> + status = nfsd4_encode_security_label(xdr, rqstp, &context);
> if (status)
> goto out;
> }
> @@ -2999,10 +2996,8 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct
> svc_fh *fhp,
>
> out:
> #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> - if (context) {
> - lsmcontext_init(&scaff, context, contextlen, 0); /*scaffolding*/
> - security_release_secctx(&scaff);
> - }
> + if (context.context)
> + security_release_secctx(&context);
> #endif /* CONFIG_NFSD_V4_SECURITY_LABEL */
> kfree(acl);
> if (tempfh) {
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 43f8a2660d37..02dc3b5ef57b 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -560,7 +560,7 @@ void security_release_secctx(struct lsmcontext *cp);
> void security_inode_invalidate_secctx(struct inode *inode);
> int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
> int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
> -int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
> +int security_inode_getsecctx(struct inode *inode, struct lsmcontext *cp);
> int security_locked_down(enum lockdown_reason what);
> #else /* CONFIG_SECURITY */
>
> @@ -1399,7 +1399,8 @@ static inline int security_inode_setsecctx(struct
> dentry *
Re: [PATCH v19 15/23] LSM: Use lsmcontext in security_secid_to_secctx
On 7/24/20 1:32 PM, Casey Schaufler wrote:
> Replace the (secctx,seclen) pointer pair with a single
> lsmcontext pointer to allow return of the LSM identifier
> along with the context and context length. This allows
> security_release_secctx() to know how to release the
> context. Callers have been modified to use or save the
> returned data from the new structure.
>
> Reviewed-by: Kees Cook
> Acked-by: Stephen Smalley
> Acked-by: Paul Moore
> Signed-off-by: Casey Schaufler
> Cc: net...@vger.kernel.org
> ---
> drivers/android/binder.c| 26 +++-
> include/linux/security.h| 4 +--
> include/net/scm.h | 10 ++-
> kernel/audit.c | 35 --
> kernel/auditsc.c| 31 +++
> net/ipv4/ip_sockglue.c | 7 ++---
> net/netfilter/nf_conntrack_netlink.c| 18 +--
> net/netfilter/nf_conntrack_standalone.c | 7 ++---
> net/netfilter/nfnetlink_queue.c | 5 +++-
> net/netlabel/netlabel_unlabeled.c | 40 -
> net/netlabel/netlabel_user.c| 7 ++---
> security/security.c | 10 +--
> 12 files changed, 76 insertions(+), 124 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index b7ab206f8bb3..ceb5987c7d76 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -2861,9 +2861,7 @@ static void binder_transaction(struct binder_proc *proc,
> binder_size_t last_fixup_min_off = 0;
> struct binder_context *context = proc->context;
> int t_debug_id = atomic_inc_return(&binder_last_id);
> - char *secctx = NULL;
> - u32 secctx_sz = 0;
> - struct lsmcontext scaff; /* scaffolding */
> + struct lsmcontext lsmctx = { };
>
> e = binder_transaction_log_add(&binder_transaction_log);
> e->debug_id = t_debug_id;
> @@ -3111,14 +3109,14 @@ static void binder_transaction(struct binder_proc
> *proc,
> size_t added_size;
>
> security_task_getsecid(proc->tsk, &blob);
> - ret = security_secid_to_secctx(&blob, &secctx, &secctx_sz);
> + ret = security_secid_to_secctx(&blob, &lsmctx);
> if (ret) {
> return_error = BR_FAILED_REPLY;
> return_error_param = ret;
> return_error_line = __LINE__;
> goto err_get_secctx_failed;
> }
> - added_size = ALIGN(secctx_sz, sizeof(u64));
> + added_size = ALIGN(lsmctx.len, sizeof(u64));
> extra_buffers_size += added_size;
> if (extra_buffers_size < added_size) {
> /* integer overflow of extra_buffers_size */
> @@ -3145,24 +3143,22 @@ static void binder_transaction(struct binder_proc
> *proc,
> t->buffer = NULL;
> goto err_binder_alloc_buf_failed;
> }
> - if (secctx) {
> + if (lsmctx.context) {
> int err;
> size_t buf_offset = ALIGN(tr->data_size, sizeof(void *)) +
> ALIGN(tr->offsets_size, sizeof(void *)) +
> ALIGN(extra_buffers_size, sizeof(void *)) -
> - ALIGN(secctx_sz, sizeof(u64));
> + ALIGN(lsmctx.len, sizeof(u64));
>
> t->security_ctx = (uintptr_t)t->buffer->user_data + buf_offset;
> err = binder_alloc_copy_to_buffer(&target_proc->alloc,
> t->buffer, buf_offset,
> - secctx, secctx_sz);
> + lsmctx.context, lsmctx.len);
> if (err) {
> t->security_ctx = 0;
> WARN_ON(1);
> }
> - lsmcontext_init(&scaff, secctx, secctx_sz, 0);
> - security_release_secctx(&scaff);
> - secctx = NULL;
> + security_release_secctx(&lsmctx);
> }
> t->buffer->debug_id = t->debug_id;
> t->buffer->transaction = t;
> @@ -3218,7 +3214,7 @@ static void binder_transaction(struct binder_proc *proc,
> off_end_offset = off_start_offset + tr->offsets_size;
> sg_buf_offset = ALIGN(off_end_offset, sizeof(void *));
> sg_buf_end_offset = sg_buf_offset + extra_buffers_size -
> - ALIGN(secctx_sz, sizeof(u64));
> + ALIGN(lsmctx.len, sizeof(u64));
> off_min = 0;
> for (buffer_offset = off_start_offset; buffer_offset < off_end_offset;
>buffer_offset += sizeof(binder_size_t)) {
> @@ -3494,10 +3490,8 @@ static void binder_transaction(struct binder_proc
> *proc,
> binder_alloc_free_buf(&target_proc->alloc, t->buffer);
> err_binder_alloc_buf_failed:
> err_bad_extra_size:
> - if (secctx) {
> -
Re: [PATCH v19 04/23] LSM: Use lsmblob in security_kernel_act_as
On 7/24/20 1:32 PM, Casey Schaufler wrote:
> Change the security_kernel_act_as interface to use a lsmblob
> structure in place of the single u32 secid in support of
> module stacking. Change its only caller, set_security_override,
> to do the same. Change that one's only caller,
> set_security_override_from_ctx, to call it with the new
> parameter type.
>
> The security module hook is unchanged, still taking a secid.
> The infrastructure passes the correct entry from the lsmblob.
> lsmblob_init() is used to fill the lsmblob structure, however
> this will be removed later in the series when security_secctx_to_secid()
> is undated to provide a lsmblob instead of a secid.
>
fix ^ "undated" to updated
> Reviewed-by: Kees Cook
> Reviewed-by: John Johansen
> Acked-by: Stephen Smalley
> Signed-off-by: Casey Schaufler
> ---
> include/linux/cred.h | 3 ++-
> include/linux/security.h | 5 +++--
> kernel/cred.c| 10 ++
> security/security.c | 14 --
> 4 files changed, 23 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/cred.h b/include/linux/cred.h
> index 18639c069263..03ae0182cba6 100644
> --- a/include/linux/cred.h
> +++ b/include/linux/cred.h
> @@ -18,6 +18,7 @@
>
> struct cred;
> struct inode;
> +struct lsmblob;
>
> /*
> * COW Supplementary groups list
> @@ -165,7 +166,7 @@ extern const struct cred *override_creds(const struct
> cred *);
> extern void revert_creds(const struct cred *);
> extern struct cred *prepare_kernel_cred(struct task_struct *);
> extern int change_create_files_as(struct cred *, struct inode *);
> -extern int set_security_override(struct cred *, u32);
> +extern int set_security_override(struct cred *, struct lsmblob *);
> extern int set_security_override_from_ctx(struct cred *, const char *);
> extern int set_create_files_as(struct cred *, struct inode *);
> extern int cred_fscmp(const struct cred *, const struct cred *);
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 294410533b51..6d403a522918 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -441,7 +441,7 @@ void security_cred_free(struct cred *cred);
> int security_prepare_creds(struct cred *new, const struct cred *old, gfp_t
> gfp);
> void security_transfer_creds(struct cred *new, const struct cred *old);
> void security_cred_getsecid(const struct cred *c, u32 *secid);
> -int security_kernel_act_as(struct cred *new, u32 secid);
> +int security_kernel_act_as(struct cred *new, struct lsmblob *blob);
> int security_kernel_create_files_as(struct cred *new, struct inode *inode);
> int security_kernel_module_request(char *kmod_name);
> int security_kernel_load_data(enum kernel_load_data_id id);
> @@ -1055,7 +1055,8 @@ static inline void security_transfer_creds(struct cred
> *new,
> {
> }
>
> -static inline int security_kernel_act_as(struct cred *cred, u32 secid)
> +static inline int security_kernel_act_as(struct cred *cred,
> + struct lsmblob *blob)
> {
> return 0;
> }
> diff --git a/kernel/cred.c b/kernel/cred.c
> index 421b1149c651..22e0e7cbefde 100644
> --- a/kernel/cred.c
> +++ b/kernel/cred.c
> @@ -733,14 +733,14 @@ EXPORT_SYMBOL(prepare_kernel_cred);
> /**
> * set_security_override - Set the security ID in a set of credentials
> * @new: The credentials to alter
> - * @secid: The LSM security ID to set
> + * @blob: The LSM security information to set
> *
> * Set the LSM security ID in a set of credentials so that the subjective
> * security is overridden when an alternative set of credentials is used.
> */
> -int set_security_override(struct cred *new, u32 secid)
> +int set_security_override(struct cred *new, struct lsmblob *blob)
> {
> - return security_kernel_act_as(new, secid);
> + return security_kernel_act_as(new, blob);
> }
> EXPORT_SYMBOL(set_security_override);
>
> @@ -756,6 +756,7 @@ EXPORT_SYMBOL(set_security_override);
> */
> int set_security_override_from_ctx(struct cred *new, const char *secctx)
> {
> + struct lsmblob blob;
> u32 secid;
> int ret;
>
> @@ -763,7 +764,8 @@ int set_security_override_from_ctx(struct cred *new,
> const char *secctx)
> if (ret < 0)
> return ret;
>
> - return set_security_override(new, secid);
> + lsmblob_init(&blob, secid);
> + return set_security_override(new, &blob);
> }
> EXPORT_SYMBOL(set_security_override_from_ctx);
>
> diff --git a/security/security.c b/security/security.c
> index f9a249a93215..d6d882b1f7d5 100644
> --- a/security/security.c
> +
Re: [PATCH v19 05/23] net: Prepare UDS for security module stacking
On 7/24/20 1:32 PM, Casey Schaufler wrote:
> Change the data used in UDS SO_PEERSEC processing from a
> secid to a more general struct lsmblob. Update the
> security_socket_getpeersec_dgram() interface to use the
> lsmblob. There is a small amount of scaffolding code
> that will come out when the security_secid_to_secctx()
> code is brought in line with the lsmblob.
>
> Signed-off-by: Casey Schaufler
Reviewed-by: John Johansen
> ---
> include/linux/security.h | 7 +--
> include/net/af_unix.h| 2 +-
> include/net/scm.h| 8 +---
> net/ipv4/ip_sockglue.c | 8 +---
> net/unix/af_unix.c | 6 +++---
> security/security.c | 18 +++---
> 6 files changed, 34 insertions(+), 15 deletions(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 6d403a522918..d81e8886d799 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1397,7 +1397,8 @@ int security_socket_shutdown(struct socket *sock, int
> how);
> int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb);
> int security_socket_getpeersec_stream(struct socket *sock, char __user
> *optval,
> int __user *optlen, unsigned len);
> -int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff
> *skb, u32 *secid);
> +int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff
> *skb,
> + struct lsmblob *blob);
> int security_sk_alloc(struct sock *sk, int family, gfp_t priority);
> void security_sk_free(struct sock *sk);
> void security_sk_clone(const struct sock *sk, struct sock *newsk);
> @@ -1535,7 +1536,9 @@ static inline int
> security_socket_getpeersec_stream(struct socket *sock, char __
> return -ENOPROTOOPT;
> }
>
> -static inline int security_socket_getpeersec_dgram(struct socket *sock,
> struct sk_buff *skb, u32 *secid)
> +static inline int security_socket_getpeersec_dgram(struct socket *sock,
> +struct sk_buff *skb,
> +struct lsmblob *blob)
> {
> return -ENOPROTOOPT;
> }
> diff --git a/include/net/af_unix.h b/include/net/af_unix.h
> index f42fdddecd41..a86da0cb5ec1 100644
> --- a/include/net/af_unix.h
> +++ b/include/net/af_unix.h
> @@ -36,7 +36,7 @@ struct unix_skb_parms {
> kgid_t gid;
> struct scm_fp_list *fp;/* Passed files */
> #ifdef CONFIG_SECURITY_NETWORK
> - u32 secid; /* Security ID */
> + struct lsmblob lsmblob;/* Security LSM data*/
> #endif
> u32 consumed;
> } __randomize_layout;
> diff --git a/include/net/scm.h b/include/net/scm.h
> index 1ce365f4c256..e2e71c4bf9d0 100644
> --- a/include/net/scm.h
> +++ b/include/net/scm.h
> @@ -33,7 +33,7 @@ struct scm_cookie {
> struct scm_fp_list *fp;/* Passed files */
> struct scm_credscreds; /* Skb credentials */
> #ifdef CONFIG_SECURITY_NETWORK
> - u32 secid; /* Passed security ID */
> + struct lsmblob lsmblob;/* Passed LSM data */
> #endif
> };
>
> @@ -46,7 +46,7 @@ struct scm_fp_list *scm_fp_dup(struct scm_fp_list *fpl);
> #ifdef CONFIG_SECURITY_NETWORK
> static __inline__ void unix_get_peersec_dgram(struct socket *sock, struct
> scm_cookie *scm)
> {
> - security_socket_getpeersec_dgram(sock, NULL, &scm->secid);
> + security_socket_getpeersec_dgram(sock, NULL, &scm->lsmblob);
> }
> #else
> static __inline__ void unix_get_peersec_dgram(struct socket *sock, struct
> scm_cookie *scm)
> @@ -97,7 +97,9 @@ static inline void scm_passec(struct socket *sock, struct
> msghdr *msg, struct sc
> int err;
>
> if (test_bit(SOCK_PASSSEC, &sock->flags)) {
> - err = security_secid_to_secctx(scm->secid, &secdata, &seclen);
> + /* Scaffolding - it has to be element 0 for now */
> + err = security_secid_to_secctx(scm->lsmblob.secid[0],
> +&secdata, &seclen);
>
> if (!err) {
> put_cmsg(msg, SOL_SOCKET, SCM_SECURITY, seclen,
> secdata);
> diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
> index 84ec3703c909..3ea1103b4c29 100644
> --- a/net/ipv4/ip_sockglue.c
> +++ b/net/ipv4/ip_sockglue.c
> @@ -130,15 +130,17 @@ static void ip_cmsg_recv_checksum(struct msghdr *msg,
> struct sk_buff *skb,
>
> static
Re: [PATCH v19 06/23] LSM: Use lsmblob in security_secctx_to_secid
On 7/24/20 1:32 PM, Casey Schaufler wrote:
> Change security_secctx_to_secid() to fill in a lsmblob instead
> of a u32 secid. Multiple LSMs may be able to interpret the
> string, and this allows for setting whichever secid is
> appropriate. Change security_secmark_relabel_packet() to use a
> lsmblob instead of a u32 secid. In some other cases there is
> scaffolding where interfaces have yet to be converted.
>
> Reviewed-by: Kees Cook
> Signed-off-by: Casey Schaufler
> Cc: net...@vger.kernel.org
one comment below, but its a nice to have so
Reviewed-by: John Johansen
> ---
> include/linux/security.h | 30 +++
> include/net/scm.h | 7 +--
> kernel/cred.c | 4 +---
> net/ipv4/ip_sockglue.c| 6 --
> net/netfilter/nft_meta.c | 18 +---
> net/netfilter/xt_SECMARK.c| 9 ++--
> net/netlabel/netlabel_unlabeled.c | 23 +
> security/security.c | 34 ++-
> 8 files changed, 98 insertions(+), 33 deletions(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index d81e8886d799..98176faaaba5 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -189,6 +189,27 @@ static inline bool lsmblob_equal(struct lsmblob *bloba,
> struct lsmblob *blobb)
> return !memcmp(bloba, blobb, sizeof(*bloba));
> }
>
> +/**
> + * lsmblob_value - find the first non-zero value in an lsmblob structure.
> + * @blob: Pointer to the data
> + *
> + * This needs to be used with extreme caution, as the cases where
> + * it is appropriate are rare.
> + *
> + * Return the first secid value set in the lsmblob.
> + * There should only be one.
It would be really nice if we could have an LSM debug config, that would
do things like checking there is indeed only one value when this fn
is called.
> + */
> +static inline u32 lsmblob_value(const struct lsmblob *blob)
> +{
> + int i;
> +
> + for (i = 0; i < LSMBLOB_ENTRIES; i++)
> + if (blob->secid[i])
> + return blob->secid[i];
> +
> + return 0;
> +}
> +
> /* These functions are in security/commoncap.c */
> extern int cap_capable(const struct cred *cred, struct user_namespace *ns,
> int cap, unsigned int opts);
> @@ -502,7 +523,8 @@ int security_setprocattr(const char *lsm, const char
> *name, void *value,
> int security_netlink_send(struct sock *sk, struct sk_buff *skb);
> int security_ismaclabel(const char *name);
> int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
> -int security_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid);
> +int security_secctx_to_secid(const char *secdata, u32 seclen,
> + struct lsmblob *blob);
> void security_release_secctx(char *secdata, u32 seclen);
> void security_inode_invalidate_secctx(struct inode *inode);
> int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
> @@ -1321,7 +1343,7 @@ static inline int security_secid_to_secctx(u32 secid,
> char **secdata, u32 *secle
>
> static inline int security_secctx_to_secid(const char *secdata,
> u32 seclen,
> -u32 *secid)
> +struct lsmblob *blob)
> {
> return -EOPNOTSUPP;
> }
> @@ -1411,7 +1433,7 @@ void security_inet_csk_clone(struct sock *newsk,
> const struct request_sock *req);
> void security_inet_conn_established(struct sock *sk,
> struct sk_buff *skb);
> -int security_secmark_relabel_packet(u32 secid);
> +int security_secmark_relabel_packet(struct lsmblob *blob);
> void security_secmark_refcount_inc(void);
> void security_secmark_refcount_dec(void);
> int security_tun_dev_alloc_security(void **security);
> @@ -1584,7 +1606,7 @@ static inline void
> security_inet_conn_established(struct sock *sk,
> {
> }
>
> -static inline int security_secmark_relabel_packet(u32 secid)
> +static inline int security_secmark_relabel_packet(struct lsmblob *blob)
> {
> return 0;
> }
> diff --git a/include/net/scm.h b/include/net/scm.h
> index e2e71c4bf9d0..c09f2dfeec88 100644
> --- a/include/net/scm.h
> +++ b/include/net/scm.h
> @@ -97,8 +97,11 @@ static inline void scm_passec(struct socket *sock, struct
> msghdr *msg, struct sc
> int err;
>
> if (test_bit(SOCK_PASSSEC, &sock->flags)) {
> - /* Scaffolding - it has to be element 0 for now */
> - err = security_secid_to_secctx(scm->lsmblob.secid[0
Re: [PATCH v19 02/23] LSM: Create and manage the lsmblob data structure.
On 7/24/20 1:32 PM, Casey Schaufler wrote:
> When more than one security module is exporting data to
> audit and networking sub-systems a single 32 bit integer
> is no longer sufficient to represent the data. Add a
> structure to be used instead.
>
> The lsmblob structure is currently an array of
> u32 "secids". There is an entry for each of the
> security modules built into the system that would
> use secids if active. The system assigns the module
> a "slot" when it registers hooks. If modules are
> compiled in but not registered there will be unused
> slots.
>
> A new lsm_id structure, which contains the name
> of the LSM and its slot number, is created. There
> is an instance for each LSM, which assigns the name
> and passes it to the infrastructure to set the slot.
>
> The audit rules data is expanded to use an array of
> security module data rather than a single instance.
> Because IMA uses the audit rule functions it is
> affected as well.
>
assuming the config issue Stephan found is fixed
you can have my
Acked-by: John Johansen
> Acked-by: Stephen Smalley
> Acked-by: Paul Moore
> Signed-off-by: Casey Schaufler
> ---
> include/linux/audit.h | 4 +-
> include/linux/lsm_hooks.h | 12 -
> include/linux/security.h| 66 --
> kernel/auditfilter.c| 24 +-
> kernel/auditsc.c| 12 ++---
> security/apparmor/lsm.c | 7 ++-
> security/commoncap.c| 7 ++-
> security/integrity/ima/ima_policy.c | 40 +++-
> security/loadpin/loadpin.c | 8 +++-
> security/lockdown/lockdown.c| 7 ++-
> security/safesetid/lsm.c| 8 +++-
> security/security.c | 72 -
> security/selinux/hooks.c| 8 +++-
> security/smack/smack_lsm.c | 7 ++-
> security/tomoyo/tomoyo.c| 8 +++-
> security/yama/yama_lsm.c| 7 ++-
> 16 files changed, 242 insertions(+), 55 deletions(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 3fcd9ee49734..aabbbe6d9296 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -11,6 +11,7 @@
>
> #include
> #include
> +#include
> #include
>
> #define AUDIT_INO_UNSET ((unsigned long)-1)
> @@ -64,8 +65,9 @@ struct audit_field {
> kuid_t uid;
> kgid_t gid;
> struct {
> + boollsm_isset;
> char*lsm_str;
> - void*lsm_rule;
> + void*lsm_rules[LSMBLOB_ENTRIES];
> };
> };
> u32 op;
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index b4bcafc79e0b..c9f792066d86 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1532,6 +1532,14 @@ struct security_hook_heads {
> #undef LSM_HOOK
> } __randomize_layout;
>
> +/*
> + * Information that identifies a security module.
> + */
> +struct lsm_id {
> + const char *lsm; /* Name of the LSM */
> + int slot; /* Slot in lsmblob if one is allocated */
> +};
> +
> /*
> * Security module hook list structure.
> * For use with generic list macros for common operations.
> @@ -1540,7 +1548,7 @@ struct security_hook_list {
> struct hlist_node list;
> struct hlist_head *head;
> union security_list_options hook;
> - char*lsm;
> + struct lsm_id *lsmid;
> } __randomize_layout;
>
> /*
> @@ -1575,7 +1583,7 @@ extern struct security_hook_heads security_hook_heads;
> extern char *lsm_names;
>
> extern void security_add_hooks(struct security_hook_list *hooks, int count,
> - char *lsm);
> +struct lsm_id *lsmid);
>
> #define LSM_FLAG_LEGACY_MAJORBIT(0)
> #define LSM_FLAG_EXCLUSIVE BIT(1)
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 0a0a03b36a3b..591dae299c6f 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -131,6 +131,64 @@ enum lockdown_reason {
>
> extern const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1];
>
> +/*
> + * Data exported by the security modules
> + *
> + * Any LSM that provides secid or secctx based hooks must be included.
> + */
> +#define LSMBLOB_ENTRIES ( \
> + (IS_ENABL
Re: [PATCH v19 13/23] LSM: Specify which LSM to display
On 7/24/20 1:32 PM, 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
> ---
> 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 | 167 ---
> security/selinux/hooks.c | 11 ++
> security/selinux/include/classmap.h | 2 +-
> security/smack/smack_lsm.c | 7 ++
> 8 files changed, 221 insertions(+), 19 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index d86c0afc8a85..40471a12ced2 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2794,6 +2794,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),
> #ifdef CONFIG_SECURITY_SMACK
> DIR("smack",0555,
> proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops),
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index c9f792066d86..6908fa03cf31 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1647,4 +1647,21 @@ static inline void security_delete_hooks(struct
> security_hook_list *hooks,
>
> extern int lsm_inode_alloc(struct inode *inode);
>
> +/**
> + * lsm_task_display - the "display" LSM for this task
> + * @task: The task to report on
> + *
> + * Returns the task's display LSM slot.
> + */
> +static inline int lsm_task_display(struct task_struct *task)
> +{
> +#ifdef CONFIG_SECURITY
> + int *display = task->security;
> +
> + if (display)
> + return *display;
> +#endif
> + return LSMBLOB_INVALID;
> +}
> +
> #endif /* ! __LINUX_LSM_HOOKS_H */
> diff --git a/security/apparmor/include/apparmor.h
> b/security/apparmor/include/apparmor.h
> index 1fbabdb565a8..b1622fcb4394 100644
> --- a/security/apparmor/include/apparmor.h
> +++ b/security/apparmor/include/apparmor.h
> @@ -28,8 +28,9 @@
> #define AA_CLASS_SIGNAL 10
> #define AA_CLASS_NET 14
> #define AA_CLASS_LABEL 16
> +#define AA_CLASS_DISPLAY_LSM 17
>
> -#define AA_CLASS_LASTAA_CLASS_LABEL
> +#define AA_CLASS_LASTAA_CLASS_DISPLAY_LSM
>
> /* Control parameters settable through module/boot flags */
> extern enum audit_mode aa_g_audit;
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 432915c1d427..31a6f11890f1 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -612,6 +612,25 @@ static int apparmor_getprocattr(struct task_struct
> *task, char *name,
> return error;
> }
>
> +
> +static int profile_display_lsm(struct aa_profile *profile,
> +struct common_audit_data *sa)
> +{
> + struct aa_perms perms = { };
> + unsigned int state;
> +
> + state = PROFILE_MEDIATES(profile, AA_CLASS_DISPLAY_LSM);
> + if (state) {
> + aa_compute_perms(profile->policy.dfa, state, &perms);
> + aa_apply_modes_to_perms(profile, &perms);
> + aad(sa)->label = &profile->label;
> +
> + return aa_check_perms(profile, &perms, AA_MAY_WRITE, sa, NULL);
> + }
> +
> + return 0;
> +}
> +
> static int apparmor_setprocattr(const char *name, void *value,
> size_t size)
> {
> @@ -623,6 +642,19 @@ static int apparmor_setprocattr(const char *name, void
> *value,
>
Re: [PATCH v19 06/23] LSM: Use lsmblob in security_secctx_to_secid
On 7/28/20 4:41 PM, Casey Schaufler wrote: > On 7/28/2020 4:11 AM, John Johansen wrote: >> On 7/24/20 1:32 PM, Casey Schaufler wrote: >>> Change security_secctx_to_secid() to fill in a lsmblob instead >>> of a u32 secid. Multiple LSMs may be able to interpret the >>> string, and this allows for setting whichever secid is >>> appropriate. Change security_secmark_relabel_packet() to use a >>> lsmblob instead of a u32 secid. In some other cases there is >>> scaffolding where interfaces have yet to be converted. >>> >>> Reviewed-by: Kees Cook >>> Signed-off-by: Casey Schaufler >>> Cc: net...@vger.kernel.org >> one comment below, but its a nice to have so >> >> Reviewed-by: John Johansen >> >> >>> --- >>> include/linux/security.h | 30 +++ >>> include/net/scm.h | 7 +-- >>> kernel/cred.c | 4 +--- >>> net/ipv4/ip_sockglue.c| 6 -- >>> net/netfilter/nft_meta.c | 18 +--- >>> net/netfilter/xt_SECMARK.c| 9 ++-- >>> net/netlabel/netlabel_unlabeled.c | 23 + >>> security/security.c | 34 ++- >>> 8 files changed, 98 insertions(+), 33 deletions(-) >>> >>> diff --git a/include/linux/security.h b/include/linux/security.h >>> index d81e8886d799..98176faaaba5 100644 >>> --- a/include/linux/security.h >>> +++ b/include/linux/security.h >>> @@ -189,6 +189,27 @@ static inline bool lsmblob_equal(struct lsmblob >>> *bloba, struct lsmblob *blobb) >>> return !memcmp(bloba, blobb, sizeof(*bloba)); >>> } >>> >>> +/** >>> + * lsmblob_value - find the first non-zero value in an lsmblob structure. >>> + * @blob: Pointer to the data >>> + * >>> + * This needs to be used with extreme caution, as the cases where >>> + * it is appropriate are rare. >>> + * >>> + * Return the first secid value set in the lsmblob. >>> + * There should only be one. >> It would be really nice if we could have an LSM debug config, that would >> do things like checking there is indeed only one value when this fn >> is called. > > I can't see a CONFIG_LSM_DEBUG for this alone, but if you have > other places you'd like to see it I'm open to it. > yeah there are a few other places, this really isn't a requirement just a thought while I was going through these again. I will have to spend some time chasing them down. Maybe even cobble together a patch -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v19 19/23] LSM: Verify LSM display sanity in binder
On 7/24/20 1:32 PM, Casey Schaufler wrote:
> Verify that the tasks on the ends of a binder transaction
> use the same "display" security module. This prevents confusion
> of security "contexts".
>
Reviewed-by: John Johansen
> Reviewed-by: Kees Cook
> Acked-by: Stephen Smalley
> Signed-off-by: Casey Schaufler
> ---
> security/security.c | 29 +
> 1 file changed, 29 insertions(+)
>
> diff --git a/security/security.c b/security/security.c
> index ddbaf2073b02..95b48721fb17 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -788,9 +788,38 @@ int security_binder_set_context_mgr(struct task_struct
> *mgr)
> return call_int_hook(binder_set_context_mgr, 0, mgr);
> }
>
> +/**
> + * security_binder_transaction - Binder driver transaction check
> + * @from: source of the transaction
> + * @to: destination of the transaction
> + *
> + * Verify that the tasks have the same LSM "display", then
> + * call the security module hooks.
> + *
> + * Returns -EINVAL if the displays don't match, or the
> + * result of the security module checks.
> + */
> int security_binder_transaction(struct task_struct *from,
> struct task_struct *to)
> {
> + int from_display = lsm_task_display(from);
> + int to_display = lsm_task_display(to);
> +
> + /*
> + * If the display is LSMBLOB_INVALID the first module that has
> + * an entry is used. This will be in the 0 slot.
> + *
> + * This is currently only required if the server has requested
> + * peer contexts, but it would be unwieldly to have too much of
> + * the binder driver detail here.
> + */
> + if (from_display == LSMBLOB_INVALID)
> + from_display = 0;
> + if (to_display == LSMBLOB_INVALID)
> + to_display = 0;
> + if (from_display != to_display)
> + return -EINVAL;
> +
> return call_int_hook(binder_transaction, 0, from, to);
> }
>
>
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v19 22/23] LSM: Add /proc attr entry for full LSM context
On 7/24/20 1:32 PM, Casey Schaufler wrote:
> Add an entry /proc/.../attr/context which displays the full
> process security "context" in compound format:
> lsm1\0value\0lsm2\0value\0...
> This entry is not writable.
>
> A security module may decide that its policy does not allow
> this information to be displayed. In this case none of the
> information will be displayed.
>
> Reviewed-by: Kees Cook
> Signed-off-by: Casey Schaufler
> Cc: linux-...@vger.kernel.org
> ---
> Documentation/security/lsm.rst | 28 +++
> fs/proc/base.c | 1 +
> include/linux/lsm_hooks.h| 6 +++
> security/apparmor/include/procattr.h | 2 +-
> security/apparmor/lsm.c | 8 +++-
> security/apparmor/procattr.c | 22 +
> security/security.c | 70
> security/selinux/hooks.c | 2 +-
> security/smack/smack_lsm.c | 2 +-
> 9 files changed, 126 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/security/lsm.rst b/Documentation/security/lsm.rst
> index 6a2a2e973080..fd4c87358d54 100644
> --- a/Documentation/security/lsm.rst
> +++ b/Documentation/security/lsm.rst
> @@ -129,3 +129,31 @@ to identify it as the first security module to be
> registered.
> The capabilities security module does not use the general security
> blobs, unlike other modules. The reasons are historical and are
> based on overhead, complexity and performance concerns.
> +
> +LSM External Interfaces
> +===
> +
> +The LSM infrastructure does not generally provide external interfaces.
> +The individual security modules provide what external interfaces they
> +require.
> +
> +The file ``/sys/kernel/security/lsm`` provides a comma
> +separated list of the active security modules.
> +
> +The file ``/proc/pid/attr/display`` contains the name of the security
> +module for which the ``/proc/pid/attr/current`` interface will
> +apply. This interface can be written to.
> +
> +The infrastructure does provide an interface for the special
> +case where multiple security modules provide a process context.
> +This is provided in compound context format.
> +
> +- `lsm\0value\0lsm\0value\0`
> +
> +The `lsm` and `value` fields are nul terminated bytestrings.
> +Each field may contain whitespace or non-printable characters.
> +The nul bytes are included in the size of a compound context.
> +The context ``Bell\0Secret\0Biba\0Loose\0`` has a size of 23.
> +
> +The file ``/proc/pid/attr/context`` provides the security
> +context of the identified process.
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 40471a12ced2..ba8b0316e999 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2795,6 +2795,7 @@ static const struct pid_entry attr_dir_stuff[] = {
> ATTR(NULL, "keycreate", 0666),
> ATTR(NULL, "sockcreate",0666),
> ATTR(NULL, "display", 0666),
> + ATTR(NULL, "context", 0444),
> #ifdef CONFIG_SECURITY_SMACK
> DIR("smack",0555,
> proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops),
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 6908fa03cf31..5be04dacc17a 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1359,6 +1359,12 @@
> * @pages contains the number of pages.
> * Return 0 if permission is granted.
> *
> + * @getprocattr:
> + * Provide the named process attribute for display in special files in
> + * the /proc/.../attr directory. Attribute naming and the data displayed
> + * is at the discretion of the security modules. The exception is the
> + * "context" attribute, which will contain the security context of the
> + * task as a nul terminated text string without trailing whitespace.
> * @ismaclabel:
> * Check if the extended attribute specified by @name
> * represents a MAC label. Returns 1 if name is a MAC
> diff --git a/security/apparmor/include/procattr.h
> b/security/apparmor/include/procattr.h
> index 31689437e0e1..03dbfdb2f2c0 100644
> --- a/security/apparmor/include/procattr.h
> +++ b/security/apparmor/include/procattr.h
> @@ -11,7 +11,7 @@
> #ifndef __AA_PROCATTR_H
> #define __AA_PROCATTR_H
>
> -int aa_getprocattr(struct aa_label *label, char **string);
> +int aa_getprocattr(struct aa_label *label, char **string, bool newline);
> int aa_setprocattr_changehat(char *args, size_t size, int flags);
>
> #endif /* __AA_PROCATTR_H */
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 31a6f11890f1..7ce570b0f491 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -593,6 +593,7 @@ static int apparmor_getprocattr(struct task_struct *task,
> char *name,
> const struct cred *cred = get_task_cred(task);
> struct aa_task_ctx *ctx = task_ctx(current);
> struct aa_label *label = NULL;
> + bool newline = true;
>
> if (strcm
Re: [PATCH v19 23/23] AppArmor: Remove the exclusive flag
On 7/24/20 1:32 PM, Casey Schaufler wrote:
> With the inclusion of the "display" process attribute
> mechanism AppArmor no longer needs to be treated as an
> "exclusive" security module. Remove the flag that indicates
> it is exclusive. Remove the stub getpeersec_dgram AppArmor
> hook as it has no effect in the single LSM case and
> interferes in the multiple LSM case.
>
probably should change this to
Acked-by: John Johansen
> Acked-by: Stephen Smalley
> Reviewed-by: Kees Cook
> Reviewed-by: John Johansen
> Signed-off-by: Casey Schaufler
> ---
> security/apparmor/lsm.c | 20 +---
> 1 file changed, 1 insertion(+), 19 deletions(-)
>
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 7ce570b0f491..4b7cbe9bb1be 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -1129,22 +1129,6 @@ static int apparmor_socket_getpeersec_stream(struct
> socket *sock,
> return error;
> }
>
> -/**
> - * apparmor_socket_getpeersec_dgram - get security label of packet
> - * @sock: the peer socket
> - * @skb: packet data
> - * @secid: pointer to where to put the secid of the packet
> - *
> - * Sets the netlabel socket state on sk from parent
> - */
> -static int apparmor_socket_getpeersec_dgram(struct socket *sock,
> - struct sk_buff *skb, u32 *secid)
> -
> -{
> - /* TODO: requires secid support */
> - return -ENOPROTOOPT;
> -}
> -
> /**
> * apparmor_sock_graft - Initialize newly created socket
> * @sk: child sock
> @@ -1248,8 +1232,6 @@ static struct security_hook_list apparmor_hooks[]
> __lsm_ro_after_init = {
> #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),
> @@ -1918,7 +1900,7 @@ static int __init apparmor_init(void)
>
> DEFINE_LSM(apparmor) = {
> .name = "apparmor",
> - .flags = LSM_FLAG_LEGACY_MAJOR | LSM_FLAG_EXCLUSIVE,
> + .flags = LSM_FLAG_LEGACY_MAJOR,
> .enabled = &apparmor_enabled,
> .blobs = &apparmor_blob_sizes,
> .init = apparmor_init,
>
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v19 22/23] LSM: Add /proc attr entry for full LSM context
On 7/30/20 1:44 PM, Casey Schaufler wrote:
> On 7/30/2020 3:03 AM, John Johansen wrote:
>> On 7/24/20 1:32 PM, Casey Schaufler wrote:
>>> Add an entry /proc/.../attr/context which displays the full
>>> process security "context" in compound format:
>>> lsm1\0value\0lsm2\0value\0...
>>> This entry is not writable.
>>>
>>> A security module may decide that its policy does not allow
>>> this information to be displayed. In this case none of the
>>> information will be displayed.
>>>
>>> Reviewed-by: Kees Cook
>>> Signed-off-by: Casey Schaufler
>>> Cc: linux-...@vger.kernel.org
>>> ---
>>> Documentation/security/lsm.rst | 28 +++
>>> fs/proc/base.c | 1 +
>>> include/linux/lsm_hooks.h| 6 +++
>>> security/apparmor/include/procattr.h | 2 +-
>>> security/apparmor/lsm.c | 8 +++-
>>> security/apparmor/procattr.c | 22 +
>>> security/security.c | 70
>>> security/selinux/hooks.c | 2 +-
>>> security/smack/smack_lsm.c | 2 +-
>>> 9 files changed, 126 insertions(+), 15 deletions(-)
>
>
>
>>>
>>> /**
>>> diff --git a/security/security.c b/security/security.c
>>> index d35e578fa45b..bce6be720401 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -754,6 +754,48 @@ static void __init lsm_early_task(struct task_struct
>>> *task)
>>> panic("%s: Early task alloc failed.\n", __func__);
>>> }
>>>
>>> +/**
>>> + * append_ctx - append a lsm/context pair to a compound context
>>> + * @ctx: the existing compound context
>>> + * @ctxlen: size of the old context, including terminating nul byte
>>> + * @lsm: new lsm name, nul terminated
>>> + * @new: new context, possibly nul terminated
>>> + * @newlen: maximum size of @new
>>> + *
>>> + * replace @ctx with a new compound context, appending @newlsm and @new
>>> + * to @ctx. On exit the new data replaces the old, which is freed.
>>> + * @ctxlen is set to the new size, which includes a trailing nul byte.
>>> + *
>>> + * Returns 0 on success, -ENOMEM if no memory is available.
>>> + */
>>> +static int append_ctx(char **ctx, int *ctxlen, const char *lsm, char *new,
>>> + int newlen)
>>> +{
>>> + char *final;
>>> + size_t llen;
>>> +
>>> + llen = strlen(lsm) + 1;
>>> + /*
>>> +* A security module may or may not provide a trailing nul on
>>> +* when returning a security context. There is no definition
>>> +* of which it should be, and there are modules that do it
>>> +* each way.
>>> +*/
>>> + newlen = strnlen(new, newlen) + 1;
>>> +
>>> + final = kzalloc(*ctxlen + llen + newlen, GFP_KERNEL);
>>> + if (final == NULL)
>>> + return -ENOMEM;
>>> + if (*ctxlen)
>>> + memcpy(final, *ctx, *ctxlen);
>>> + memcpy(final + *ctxlen, lsm, llen);
>>> + memcpy(final + *ctxlen + llen, new, newlen);
>> if @new doesn't have a newline appended at its end this will read 1 byte
>> passed the end of the @new buffer. Nor will the result have a trailing
>> \0 as expected unless we get lucky.
>
> @new will never have a newline at the end. The trailing nul comes
> from the allocation being done with kzalloc(). This function has to
> be considered in the context of its caller.
>
ugh, sorry not trailing newline, I meant trailing \0. The problem isn't
the kzalloc, the target has the space. It is the source @new. It is
dangerous to assume that the @new buffer has a null byte after its
declared length. Which is potentially what we are doing if @new
doesn't have an embedded null byte. In that case strlen(new, newlen)
will then return newlen and we add 1 to it.
which means in the memcpy we are copying an extra byte beyond what
was declared to exist in @new.
>>
>>
>>> + kfree(*ctx);
>>> + *ctx = final;
>>> + *ctxlen = *ctxlen + llen + newlen;
>>> + return 0;
>>> +}
>>> +
>>> /*
>>> * The default value of the LSM hook is defined in linux/lsm_hook_defs.h
>>> and
>>> * can be accessed with:
>>> @@ -2124,6 +2166,10 @@ int security_getprocattr(struct task_struct *p,
>>> const char *lsm, char *name,
>
Re: [PATCH v20 20/23] Audit: Add new record for multiple process LSM attributes
On 9/3/20 9:32 AM, James Morris wrote:
> On Wed, 26 Aug 2020, Casey Schaufler wrote:
>
>> Create a new audit record type to contain the subject information
>> when there are multiple security modules that require such data.
>> This record is linked with the same timestamp and serial number.
>> The record is produced only in cases where there is more than one
>> security module with a process "context".
>>
>> Before this change the only audit events that required multiple
>> records were syscall events. Several non-syscall events include
>> subject contexts, so the use of audit_context data has been expanded
>> as necessary.
>>
>> Signed-off-by: Casey Schaufler
>> Cc: linux-audit@redhat.com
>
> Paul, can you review/ack the audit changes?
>
I am working on reviewing them as well, but I can't/won't ack them.
>> ---
>> drivers/android/binder.c| 2 +-
>> include/linux/audit.h | 13 +++-
>> include/linux/security.h| 18 -
>> include/net/netlabel.h | 2 +-
>> include/net/scm.h | 3 +-
>> include/net/xfrm.h | 4 +-
>> include/uapi/linux/audit.h | 1 +
>> kernel/audit.c | 89 ++---
>> kernel/auditfilter.c| 2 +-
>> kernel/auditsc.c| 87 ++--
>> net/ipv4/ip_sockglue.c | 2 +-
>> net/netfilter/nf_conntrack_netlink.c| 4 +-
>> net/netfilter/nf_conntrack_standalone.c | 2 +-
>> net/netfilter/nfnetlink_queue.c | 2 +-
>> net/netlabel/netlabel_unlabeled.c | 16 ++---
>> net/netlabel/netlabel_user.c| 12 ++--
>> net/netlabel/netlabel_user.h| 6 +-
>> security/integrity/integrity_audit.c| 2 +-
>> security/security.c | 73 +++-
>> security/smack/smackfs.c| 3 +-
>> 20 files changed, 259 insertions(+), 84 deletions(-)
>>
>> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
>> index 0bde1b96680e..93781dad0c28 100644
>> --- a/drivers/android/binder.c
>> +++ b/drivers/android/binder.c
>> @@ -3113,7 +3113,7 @@ static void binder_transaction(struct binder_proc
>> *proc,
>> size_t added_size;
>>
>> security_task_getsecid(proc->tsk, &blob);
>> -ret = security_secid_to_secctx(&blob, &lsmctx);
>> +ret = security_secid_to_secctx(&blob, &lsmctx, LSMBLOB_DISPLAY);
>> if (ret) {
>> return_error = BR_FAILED_REPLY;
>> return_error_param = ret;
>> diff --git a/include/linux/audit.h b/include/linux/audit.h
>> index ba1cd38d601b..fe027df0d9a8 100644
>> --- a/include/linux/audit.h
>> +++ b/include/linux/audit.h
>> @@ -186,7 +186,9 @@ extern void audit_log_path_denied(int
>> type,
>>const char *operation);
>> extern void audit_log_lost(const char *message);
>>
>> -extern int audit_log_task_context(struct audit_buffer *ab);
>> +extern void audit_log_lsm(struct lsmblob *blob, bool exiting);
>> +extern int audit_log_task_context(struct audit_buffer *ab,
>> + struct lsmblob *blob);
>> extern void audit_log_task_info(struct audit_buffer *ab);
>>
>> extern int audit_update_lsm_rules(void);
>> @@ -246,7 +248,10 @@ static inline void audit_log_key(struct audit_buffer
>> *ab, char *key)
>> { }
>> static inline void audit_log_path_denied(int type, const char *operation)
>> { }
>> -static inline int audit_log_task_context(struct audit_buffer *ab)
>> +static inline void audit_log_lsm(struct lsmblob *blob, bool exiting)
>> +{ }
>> +static inline int audit_log_task_context(struct audit_buffer *ab,
>> + struct lsmblob *blob);
>> {
>> return 0;
>> }
>> @@ -305,6 +310,7 @@ extern void audit_seccomp(unsigned long syscall, long
>> signr, int code);
>> extern void audit_seccomp_actions_logged(const char *names,
>> const char *old_names, int res);
>> extern void __audit_ptrace(struct task_struct *t);
>> +extern void audit_stamp_context(struct audit_context *ctx);
>>
>> static inline void audit_set_context(struct task_struct *task, struct
>> audit_context *ctx)
>> {
>> @@ -682,6 +688,9 @@ static inline void audit_ntp_log(const struct
>> audit_ntp_data *ad)
>> static inline void audit_ptrace(struct task_struct *t)
>> { }
>>
>> +static inline void audit_stamp_context(struct audit_context *ctx)
>> +{ }
>> +
>> static inline void audit_log_nfcfg(const char *name, u8 af,
>> unsigned int nentries,
>> enum audit_nfcfgop op, gfp_t gfp)
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index 40260bfc3a0d..3cbe24be1563 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/sec
Re: [PATCH v20 05/23] net: Prepare UDS for security module stacking
On 9/5/20 11:13 AM, Casey Schaufler wrote: > On 9/5/2020 6:25 AM, Paul Moore wrote: >> On Fri, Sep 4, 2020 at 7:58 PM Casey Schaufler >> wrote: >>> On 9/4/2020 2:53 PM, Paul Moore wrote: On Fri, Sep 4, 2020 at 5:35 PM Casey Schaufler wrote: > On 9/4/2020 1:08 PM, Paul Moore wrote: >> ... >> I understand the concerns you mention, they are all valid as far as I'm concerned, but I think we are going to get burned by this code as it currently stands. >>> Yes, I can see that. We're getting burned by the non-extensibility >>> of secids. It will take someone smarter than me to figure out how to >>> fit N secids into 32bits without danger of either failure or memory >>> allocation. >> Sooo what are the next steps here? It sounds like there is some >> agreement that the currently proposed unix_skb_params approach is a >> problem, but it also sounds like you just want to merge it anyway? > > There are real problems with all the approaches. This is by far the > least invasive of the lot. If this is acceptable for now I will commit > to including the dynamic allocation version in the full stacking > (e.g. Smack + SELinux) stage. If it isn't, well, this stage is going > to take even longer than it already has. Sigh. > > >> I was sorta hoping for something a bit better. > > I will be looking at alternatives. I am very much open to suggestions. > I'm not even 100% convinced that Stephen's objections to my separate > allocation strategy outweigh its advantages. If you have an opinion on > that, I'd love to hear it. > fwiw I prefer the separate allocation strategy, but as you have already said it trading off one set of problems for another. I would rather see this move forward and one set of trade offs isn't significantly worse than the other to me so, either wfm. -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v20 05/23] net: Prepare UDS for security module stacking
On 9/8/20 6:35 AM, Stephen Smalley wrote: > On Mon, Sep 7, 2020 at 9:28 PM Stephen Smalley > wrote: >> >> On Sat, Sep 5, 2020 at 3:07 PM John Johansen >> wrote: >>> >>> On 9/5/20 11:13 AM, Casey Schaufler wrote: >>>> On 9/5/2020 6:25 AM, Paul Moore wrote: >>>>> On Fri, Sep 4, 2020 at 7:58 PM Casey Schaufler >>>>> wrote: >>>>>> On 9/4/2020 2:53 PM, Paul Moore wrote: >>>>>>> On Fri, Sep 4, 2020 at 5:35 PM Casey Schaufler >>>>>>> wrote: >>>>>>>> On 9/4/2020 1:08 PM, Paul Moore wrote: >>>>> ... >>>>> >>>>>>> I understand the concerns you mention, they are all valid as far as >>>>>>> I'm concerned, but I think we are going to get burned by this code as >>>>>>> it currently stands. >>>>>> Yes, I can see that. We're getting burned by the non-extensibility >>>>>> of secids. It will take someone smarter than me to figure out how to >>>>>> fit N secids into 32bits without danger of either failure or memory >>>>>> allocation. >>>>> Sooo what are the next steps here? It sounds like there is some >>>>> agreement that the currently proposed unix_skb_params approach is a >>>>> problem, but it also sounds like you just want to merge it anyway? >>>> >>>> There are real problems with all the approaches. This is by far the >>>> least invasive of the lot. If this is acceptable for now I will commit >>>> to including the dynamic allocation version in the full stacking >>>> (e.g. Smack + SELinux) stage. If it isn't, well, this stage is going >>>> to take even longer than it already has. Sigh. >>>> >>>> >>>>> I was sorta hoping for something a bit better. >>>> >>>> I will be looking at alternatives. I am very much open to suggestions. >>>> I'm not even 100% convinced that Stephen's objections to my separate >>>> allocation strategy outweigh its advantages. If you have an opinion on >>>> that, I'd love to hear it. >>>> >>> >>> fwiw I prefer the separate allocation strategy, but as you have already >>> said it trading off one set of problems for another. I would rather see >>> this move forward and one set of trade offs isn't significantly worse >>> than the other to me so, either wfm. >> >> I remain unclear that AppArmor needs this patch at all even when >> support for SO_PEERSEC lands. >> Contrary to the patch description, it is about supporting SCM_SECURITY >> for datagram not SO_PEERSEC. And I don't know of any actual users of >> SCM_SECURITY even for SELinux, just SO_PEERSEC. > > I remembered that systemd once tried using SCM_SECURITY but that was a > bug since systemd was using it with stream sockets and that wasn't > supported by the kernel at the time, > https://bugzilla.redhat.com/show_bug.cgi?id=1224211, so systemd > switched over to using SO_PEERSEC. Subsequently I did fix > SCM_SECURITY to work with stream sockets via kernel commit > 37a9a8df8ce9de6ea73349c9ac8bdf6ba4ec4f70 but SO_PEERSEC is still > preferred. Looking around, I see that there is still one usage of > SCM_SECURITY in systemd-journald but it doesn't seem to be required > (if provided, journald will pass the label along but nothing seems to > depend on it AFAICT). In any event, I don't believe this patch is > needed to support stacking AppArmor. > correct it is not currently needed. I have been playing with code to handle it but it is not upstream yet. Regardless this is something that will need to be solved at some point. -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v20 05/23] net: Prepare UDS for security module stacking
On 9/8/20 4:37 PM, Casey Schaufler wrote: > On 9/8/2020 6:35 AM, Stephen Smalley wrote: >> On Mon, Sep 7, 2020 at 9:28 PM Stephen Smalley >> wrote: >>> On Sat, Sep 5, 2020 at 3:07 PM John Johansen >>> wrote: >>>> On 9/5/20 11:13 AM, Casey Schaufler wrote: >>>>> On 9/5/2020 6:25 AM, Paul Moore wrote: >>>>>> On Fri, Sep 4, 2020 at 7:58 PM Casey Schaufler >>>>>> wrote: >>>>>>> On 9/4/2020 2:53 PM, Paul Moore wrote: >>>>>>>> On Fri, Sep 4, 2020 at 5:35 PM Casey Schaufler >>>>>>>> wrote: >>>>>>>>> On 9/4/2020 1:08 PM, Paul Moore wrote: >>>>>> ... >>>>>> >>>>>>>> I understand the concerns you mention, they are all valid as far as >>>>>>>> I'm concerned, but I think we are going to get burned by this code as >>>>>>>> it currently stands. >>>>>>> Yes, I can see that. We're getting burned by the non-extensibility >>>>>>> of secids. It will take someone smarter than me to figure out how to >>>>>>> fit N secids into 32bits without danger of either failure or memory >>>>>>> allocation. >>>>>> Sooo what are the next steps here? It sounds like there is some >>>>>> agreement that the currently proposed unix_skb_params approach is a >>>>>> problem, but it also sounds like you just want to merge it anyway? >>>>> There are real problems with all the approaches. This is by far the >>>>> least invasive of the lot. If this is acceptable for now I will commit >>>>> to including the dynamic allocation version in the full stacking >>>>> (e.g. Smack + SELinux) stage. If it isn't, well, this stage is going >>>>> to take even longer than it already has. Sigh. >>>>> >>>>> >>>>>> I was sorta hoping for something a bit better. >>>>> I will be looking at alternatives. I am very much open to suggestions. >>>>> I'm not even 100% convinced that Stephen's objections to my separate >>>>> allocation strategy outweigh its advantages. If you have an opinion on >>>>> that, I'd love to hear it. >>>>> >>>> fwiw I prefer the separate allocation strategy, but as you have already >>>> said it trading off one set of problems for another. I would rather see >>>> this move forward and one set of trade offs isn't significantly worse >>>> than the other to me so, either wfm. >>> I remain unclear that AppArmor needs this patch at all even when >>> support for SO_PEERSEC lands. >>> Contrary to the patch description, it is about supporting SCM_SECURITY >>> for datagram not SO_PEERSEC. And I don't know of any actual users of >>> SCM_SECURITY even for SELinux, just SO_PEERSEC. >> I remembered that systemd once tried using SCM_SECURITY but that was a >> bug since systemd was using it with stream sockets and that wasn't >> supported by the kernel at the time, >> https://bugzilla.redhat.com/show_bug.cgi?id=1224211, so systemd >> switched over to using SO_PEERSEC. Subsequently I did fix >> SCM_SECURITY to work with stream sockets via kernel commit >> 37a9a8df8ce9de6ea73349c9ac8bdf6ba4ec4f70 but SO_PEERSEC is still >> preferred. Looking around, I see that there is still one usage of >> SCM_SECURITY in systemd-journald but it doesn't seem to be required >> (if provided, journald will pass the label along but nothing seems to >> depend on it AFAICT). In any event, I don't believe this patch is >> needed to support stacking AppArmor. > > Stephen is, as is so often the case, correct. AppArmor has a stub > socket_getpeersec_dgram() that gets removed in patch 23. If I remove right but as I said before this is coming, I have been playing with it and have code. So the series doesn't need it today but sooner than later it will be needed > it earlier and throw in a touch of scaffolding for secid_to_secctx() > we can leave the secid as is for now. This can't be the final solution > as AppArmor will be using the hook someday and we still have the all > modules case to worry about for the next phase. It also assumes that > The BPF module isn't going to suddenly sprout a security context. > Yep this is something that needs to be dealt with, whether it is now or kicked down the road a little further ... -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v20 05/23] net: Prepare UDS for security module stacking
On 9/9/20 11:19 AM, Casey Schaufler wrote: > On 9/9/2020 6:19 AM, Stephen Smalley wrote: >> On Tue, Sep 8, 2020 at 8:21 PM John Johansen >> wrote: >>> On 9/8/20 4:37 PM, Casey Schaufler wrote: >>>> On 9/8/2020 6:35 AM, Stephen Smalley wrote: >>>>> On Mon, Sep 7, 2020 at 9:28 PM Stephen Smalley >>>>> wrote: >>>>>> On Sat, Sep 5, 2020 at 3:07 PM John Johansen >>>>>> wrote: >>>>>>> On 9/5/20 11:13 AM, Casey Schaufler wrote: >>>>>>>> On 9/5/2020 6:25 AM, Paul Moore wrote: >>>>>>>>> On Fri, Sep 4, 2020 at 7:58 PM Casey Schaufler >>>>>>>>> wrote: >>>>>>>>>> On 9/4/2020 2:53 PM, Paul Moore wrote: >>>>>>>>>>> On Fri, Sep 4, 2020 at 5:35 PM Casey Schaufler >>>>>>>>>>> wrote: >>>>>>>>>>>> On 9/4/2020 1:08 PM, Paul Moore wrote: >>>>>>>>> ... >>>>>>>>> >>>>>>>>>>> I understand the concerns you mention, they are all valid as far as >>>>>>>>>>> I'm concerned, but I think we are going to get burned by this code >>>>>>>>>>> as >>>>>>>>>>> it currently stands. >>>>>>>>>> Yes, I can see that. We're getting burned by the non-extensibility >>>>>>>>>> of secids. It will take someone smarter than me to figure out how to >>>>>>>>>> fit N secids into 32bits without danger of either failure or memory >>>>>>>>>> allocation. >>>>>>>>> Sooo what are the next steps here? It sounds like there is some >>>>>>>>> agreement that the currently proposed unix_skb_params approach is a >>>>>>>>> problem, but it also sounds like you just want to merge it anyway? >>>>>>>> There are real problems with all the approaches. This is by far the >>>>>>>> least invasive of the lot. If this is acceptable for now I will commit >>>>>>>> to including the dynamic allocation version in the full stacking >>>>>>>> (e.g. Smack + SELinux) stage. If it isn't, well, this stage is going >>>>>>>> to take even longer than it already has. Sigh. >>>>>>>> >>>>>>>> >>>>>>>>> I was sorta hoping for something a bit better. >>>>>>>> I will be looking at alternatives. I am very much open to suggestions. >>>>>>>> I'm not even 100% convinced that Stephen's objections to my separate >>>>>>>> allocation strategy outweigh its advantages. If you have an opinion on >>>>>>>> that, I'd love to hear it. >>>>>>>> >>>>>>> fwiw I prefer the separate allocation strategy, but as you have already >>>>>>> said it trading off one set of problems for another. I would rather see >>>>>>> this move forward and one set of trade offs isn't significantly worse >>>>>>> than the other to me so, either wfm. >>>>>> I remain unclear that AppArmor needs this patch at all even when >>>>>> support for SO_PEERSEC lands. >>>>>> Contrary to the patch description, it is about supporting SCM_SECURITY >>>>>> for datagram not SO_PEERSEC. And I don't know of any actual users of >>>>>> SCM_SECURITY even for SELinux, just SO_PEERSEC. >>>>> I remembered that systemd once tried using SCM_SECURITY but that was a >>>>> bug since systemd was using it with stream sockets and that wasn't >>>>> supported by the kernel at the time, >>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1224211, so systemd >>>>> switched over to using SO_PEERSEC. Subsequently I did fix >>>>> SCM_SECURITY to work with stream sockets via kernel commit >>>>> 37a9a8df8ce9de6ea73349c9ac8bdf6ba4ec4f70 but SO_PEERSEC is still >>>>> preferred. Looking around, I see that there is still one usage of >>>>> SCM_SECURITY in systemd-journald but it doesn't seem to be required >>>>> (if provided, journald will pass the label along but nothing seems to >>>>> depend on it AFAICT). In any event, I don't believe this patch is >>>>> needed to support stacking AppArmor. >>>> Stephen is, as is so often the case, correct. AppArmor has a stub >>>> socket_getpeersec_dgram() that gets removed in patch 23. If I remove >>> right but as I said before this is coming, I have been playing with >>> it and have code. So the series doesn't need it today but sooner than >>> later it will be needed > > Is sooner like 5.10, or 5.15? It matters. > I can split SCM_SECURITY off from the rest of the unix mediation and push it off for a while. So lets call it 5.15 or later. >> I don't understand why. Is there a userspace component that relies on >> SCM_SECURITY today for anything real (more than just blindly passing >> it along and maybe writing to a log somewhere)? And this doesn't >> provide support for a composite SCM_SECURITY or SCM_CONTEXT, so it >> doesn't really solve the stacking problem for it anyway. What am I >> missing? Why do you care about this patch? -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v20 05/23] net: Prepare UDS for security module stacking
On 9/9/20 6:19 AM, Stephen Smalley wrote: > On Tue, Sep 8, 2020 at 8:21 PM John Johansen > wrote: >> >> On 9/8/20 4:37 PM, Casey Schaufler wrote: >>> On 9/8/2020 6:35 AM, Stephen Smalley wrote: >>>> On Mon, Sep 7, 2020 at 9:28 PM Stephen Smalley >>>> wrote: >>>>> On Sat, Sep 5, 2020 at 3:07 PM John Johansen >>>>> wrote: >>>>>> On 9/5/20 11:13 AM, Casey Schaufler wrote: >>>>>>> On 9/5/2020 6:25 AM, Paul Moore wrote: >>>>>>>> On Fri, Sep 4, 2020 at 7:58 PM Casey Schaufler >>>>>>>> wrote: >>>>>>>>> On 9/4/2020 2:53 PM, Paul Moore wrote: >>>>>>>>>> On Fri, Sep 4, 2020 at 5:35 PM Casey Schaufler >>>>>>>>>> wrote: >>>>>>>>>>> On 9/4/2020 1:08 PM, Paul Moore wrote: >>>>>>>> ... >>>>>>>> >>>>>>>>>> I understand the concerns you mention, they are all valid as far as >>>>>>>>>> I'm concerned, but I think we are going to get burned by this code as >>>>>>>>>> it currently stands. >>>>>>>>> Yes, I can see that. We're getting burned by the non-extensibility >>>>>>>>> of secids. It will take someone smarter than me to figure out how to >>>>>>>>> fit N secids into 32bits without danger of either failure or memory >>>>>>>>> allocation. >>>>>>>> Sooo what are the next steps here? It sounds like there is some >>>>>>>> agreement that the currently proposed unix_skb_params approach is a >>>>>>>> problem, but it also sounds like you just want to merge it anyway? >>>>>>> There are real problems with all the approaches. This is by far the >>>>>>> least invasive of the lot. If this is acceptable for now I will commit >>>>>>> to including the dynamic allocation version in the full stacking >>>>>>> (e.g. Smack + SELinux) stage. If it isn't, well, this stage is going >>>>>>> to take even longer than it already has. Sigh. >>>>>>> >>>>>>> >>>>>>>> I was sorta hoping for something a bit better. >>>>>>> I will be looking at alternatives. I am very much open to suggestions. >>>>>>> I'm not even 100% convinced that Stephen's objections to my separate >>>>>>> allocation strategy outweigh its advantages. If you have an opinion on >>>>>>> that, I'd love to hear it. >>>>>>> >>>>>> fwiw I prefer the separate allocation strategy, but as you have already >>>>>> said it trading off one set of problems for another. I would rather see >>>>>> this move forward and one set of trade offs isn't significantly worse >>>>>> than the other to me so, either wfm. >>>>> I remain unclear that AppArmor needs this patch at all even when >>>>> support for SO_PEERSEC lands. >>>>> Contrary to the patch description, it is about supporting SCM_SECURITY >>>>> for datagram not SO_PEERSEC. And I don't know of any actual users of >>>>> SCM_SECURITY even for SELinux, just SO_PEERSEC. >>>> I remembered that systemd once tried using SCM_SECURITY but that was a >>>> bug since systemd was using it with stream sockets and that wasn't >>>> supported by the kernel at the time, >>>> https://bugzilla.redhat.com/show_bug.cgi?id=1224211, so systemd >>>> switched over to using SO_PEERSEC. Subsequently I did fix >>>> SCM_SECURITY to work with stream sockets via kernel commit >>>> 37a9a8df8ce9de6ea73349c9ac8bdf6ba4ec4f70 but SO_PEERSEC is still >>>> preferred. Looking around, I see that there is still one usage of >>>> SCM_SECURITY in systemd-journald but it doesn't seem to be required >>>> (if provided, journald will pass the label along but nothing seems to >>>> depend on it AFAICT). In any event, I don't believe this patch is >>>> needed to support stacking AppArmor. >>> >>> Stephen is, as is so often the case, correct. AppArmor has a stub >>> socket_getpeersec_dgram() that gets removed in patch 23. If I remove >> >> right but as I said before this is coming, I have been playing with >> it and have code. So the series doesn't need it today but sooner than >> later it will be needed > > I don't understand why. Is there a userspace component that relies on > SCM_SECURITY today for anything real (more than just blindly passing > it along and maybe writing to a log somewhere)? And this doesn't > provide support for a composite SCM_SECURITY or SCM_CONTEXT, so it > doesn't really solve the stacking problem for it anyway. What am I > missing? Why do you care about this patch? > personally I don't atm, but there are people who do care about this in there logs, whether they should or shouldn't is an entirely different question. Long term there may be some uses for it that I care about or "have to care about." For now Casey can drop it from this series. -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v21 00/23] LSM: Module stacking for AppArmor
A backport of v20 of this series has landed in the ubuntu 20.10 kernel. It defaults to just apparmor as the major LSM so stacking of apparmor with selinux or smack is not being tested by default, but it is exercising the LSM changes. On 10/12/20 1:19 PM, Casey Schaufler wrote: > This patchset provides the changes required for > the AppArmor security module to stack safely with any other. > > v21: Rebase to 5.9-rc4 > Incorporate feedback from v20 > - Further revert UDS SO_PEERSEC to use scaffolding around >the interfaces that use lsmblobs and store only a single >secid. The possibility of multiple security modules >requiring data here is still a future problem. > - Incorporate Richard Guy Briggs' non-syscall auxiliary >records patch (patch 0019-0021) in place of my "supplimental" >records implementation. [I'm not sure I've given proper >attestation. I will correct as appropriate] > v20: Rebase to 5.9-rc1 > Change the BPF security module to use the lsmblob data. (patch 0002) > Repair length logic in subject label processing (patch 0015) > Handle -EINVAL from the empty BPF setprocattr hook (patch 0020) > Correct length processing in append_ctx() (patch 0022) > v19: Rebase to 5.8-rc6 > Incorporate feedback from v18 > - Revert UDS SO_PEERSEC implementation to use lsmblobs >directly, rather than allocating as needed. The correct >treatment of out-of-memory conditions in the later case >is difficult to define. (patch 0005) > - Use a size_t in append_ctx() (patch 0021) > - Fix a memory leak when creating compound contexts. (patch 0021) > Fix build error when CONFIG_SECURITY isn't set (patch 0013) > Fix build error when CONFIG_SECURITY isn't set (patch 0020) > Fix build error when CONFIG_SECURITY isn't set (patch 0021) > > v18: Rebase to 5.8-rc3 > Incorporate feedback from v17 > - Null pointer checking in UDS (patch 0005) > Match changes in IMA code (patch 0012) > Fix the behavior of LSM context supplimental audit > records so that there's always exactly one when it's > appropriate for there to be one. This is a substantial > change that requires extention of the audit_context beyond > syscall events. (patch 0020) > > v17: Rebase to 5.7-rc4 > > v16: Rebase to 5.6 > Incorporate feedback from v15 - Thanks Stephen, Mimi and Paul > - Generally improve commit messages WRT scaffolding > - Comment ima_lsm_isset() (patch 0002) > - Some question may remain on IMA warning (patch 0002) > - Mark lsm_slot as __lsm_ro_after_init not __init_data (patch 0002) > - Change name of lsmblob variable in ima_match_rules() (patch 0003) > - Instead of putting a struct lsmblob into the unix_skb_parms >structure put a pointer to an allocated instance. There is >currently only space for 5 u32's in unix_skb_parms and it is >likely to get even tighter. Fortunately, the lifecycle >management of the allocated lsmblob is simple. (patch 0005) > - Dropped Acks due to the above change (patch 0005) > - Improved commentary on secmark labeling scaffolding. (patch 0006) > - Reduced secmark related labeling scaffolding. (patch 0006) > - Replace use of the zeroth entry of an lsmblob in scaffolding >with a function lsmblob_value() to hopefully make it less >obscure. (patch 0006) > - Convert security_secmark_relabel_packet to use lsmblob as >this reduces much of the most contentious scaffolding. (patch 0006) > - Dropped Acks due to the above change (patch 0006) > - Added BUILD_BUG_ON() for CIPSO tag 6. (patch 0018) > - Reworked audit subject information. Instead of adding fields in >the middle of existing records add a new record to the event. When >a separate record is required use subj="?". (patch 0020) > - Dropped Acks due to the above change (patch 0020) > - Reworked audit object information. Instead of adding fields in >the middle of existing records add a new record to the event. When >a separate record is required use obj="?". (patch 0021) > - Dropped Acks due to the above change (patch 0021) > - Enhanced documentation (patch 0022) > - Removed unnecessary error code check in security_getprocattr() >(patch 0021) > > v15: Rebase to 5.6-rc1 > - Revise IMA data use (patch 0002) > Incorporate feedback from v14 > - Fix lockdown module registration naming (patch 0002) > - Revise how /proc/self/attr/context is gathered. (patch 0022) > - Revise access modes on /proc/self/attr/context. (patch 0022) > - Revise documentation on LSM external interfaces. (patch 0022) > > v14: Rebase to 5.5-rc5 > Incorporate feedback from v13 > - Use an array of audit rules (patch 0002) > - Significant change, removed Acks (patch 0002) > - Remove unneeded
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
>
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
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
>>>
>
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH 3/4] smack: differentiate between subjective and objective task credentials
On 2/19/21 3:29 PM, Paul Moore wrote:
> With the split of the security_task_getsecid() into subjective and
> objective variants it's time to update Smack to ensure it is using
> the correct task creds.
>
> Signed-off-by: Paul Moore
first pass looks good to me
> ---
> security/smack/smack.h | 18 +-
> security/smack/smack_lsm.c | 40 +++-
> 2 files changed, 44 insertions(+), 14 deletions(-)
>
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index a9768b12716bf..08f9cb80655ce 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -383,7 +383,23 @@ static inline struct smack_known *smk_of_task(const
> struct task_smack *tsp)
> return tsp->smk_task;
> }
>
> -static inline struct smack_known *smk_of_task_struct(
> +static inline struct smack_known *smk_of_task_struct_subj(
> + const struct task_struct *t)
> +{
> + struct smack_known *skp;
> + const struct cred *cred;
> +
> + rcu_read_lock();
> +
> + cred = rcu_dereference(t->cred);
> + skp = smk_of_task(smack_cred(cred));
> +
> + rcu_read_unlock();
> +
> + return skp;
> +}
> +
> +static inline struct smack_known *smk_of_task_struct_obj(
> const struct task_struct *t)
> {
> struct smack_known *skp;
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 2bb354ef2c4a9..ea1a82742e8ba 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -159,7 +159,7 @@ static int smk_bu_current(char *note, struct smack_known
> *oskp,
> static int smk_bu_task(struct task_struct *otp, int mode, int rc)
> {
> struct task_smack *tsp = smack_cred(current_cred());
> - struct smack_known *smk_task = smk_of_task_struct(otp);
> + struct smack_known *smk_task = smk_of_task_struct_obj(otp);
> char acc[SMK_NUM_ACCESS_TYPE + 1];
>
> if (rc <= 0)
> @@ -479,7 +479,7 @@ static int smack_ptrace_access_check(struct task_struct
> *ctp, unsigned int mode)
> {
> struct smack_known *skp;
>
> - skp = smk_of_task_struct(ctp);
> + skp = smk_of_task_struct_obj(ctp);
>
> return smk_ptrace_rule_check(current, skp, mode, __func__);
> }
> @@ -2031,7 +2031,7 @@ static int smk_curacc_on_task(struct task_struct *p,
> int access,
> const char *caller)
> {
> struct smk_audit_info ad;
> - struct smack_known *skp = smk_of_task_struct(p);
> + struct smack_known *skp = smk_of_task_struct_subj(p);
> int rc;
>
> smk_ad_init(&ad, caller, LSM_AUDIT_DATA_TASK);
> @@ -2076,15 +2076,29 @@ static int smack_task_getsid(struct task_struct *p)
> }
>
> /**
> - * smack_task_getsecid - get the secid of the task
> - * @p: the object task
> + * smack_task_getsecid_subj - get the subjective secid of the task
> + * @p: the task
> * @secid: where to put the result
> *
> - * Sets the secid to contain a u32 version of the smack label.
> + * Sets the secid to contain a u32 version of the task's subjective smack
> label.
> + */
> +static void smack_task_getsecid_subj(struct task_struct *p, u32 *secid)
> +{
> + struct smack_known *skp = smk_of_task_struct_subj(p);
> +
> + *secid = skp->smk_secid;
> +}
> +
> +/**
> + * smack_task_getsecid_obj - get the objective secid of the task
> + * @p: the task
> + * @secid: where to put the result
> + *
> + * Sets the secid to contain a u32 version of the task's objective smack
> label.
> */
> -static void smack_task_getsecid(struct task_struct *p, u32 *secid)
> +static void smack_task_getsecid_obj(struct task_struct *p, u32 *secid)
> {
> - struct smack_known *skp = smk_of_task_struct(p);
> + struct smack_known *skp = smk_of_task_struct_obj(p);
>
> *secid = skp->smk_secid;
> }
> @@ -2172,7 +2186,7 @@ static int smack_task_kill(struct task_struct *p,
> struct kernel_siginfo *info,
> {
> struct smk_audit_info ad;
> struct smack_known *skp;
> - struct smack_known *tkp = smk_of_task_struct(p);
> + struct smack_known *tkp = smk_of_task_struct_obj(p);
> int rc;
>
> if (!sig)
> @@ -2210,7 +2224,7 @@ static int smack_task_kill(struct task_struct *p,
> struct kernel_siginfo *info,
> static void smack_task_to_inode(struct task_struct *p, struct inode *inode)
> {
> struct inode_smack *isp = smack_inode(inode);
> - struct smack_known *skp = smk_of_task_struct(p);
> + struct smack_known *skp = smk_of_task_struct_obj(p);
>
> isp->smk_inode = skp;
> isp->smk_flags |= SMK_INODE_INSTANT;
> @@ -3481,7 +3495,7 @@ static void smack_d_instantiate(struct dentry
> *opt_dentry, struct inode *inode)
> */
> static int smack_getprocattr(struct task_struct *p, char *name, char **value)
> {
> - struct smack_known *skp = smk_of_task_struct(p);
> + struct smack_known *skp = smk_of_task_struct_subj(p);
> char *cp;
>
Re: [RFC PATCH 2/4] selinux: clarify task subjective and objective credentials
On 2/19/21 3:29 PM, Paul Moore wrote:
> SELinux has a function, task_sid(), which returns the task's
> objective credentials, but unfortunately is used in a few places
> where the subjective task credentials should be used. Most notably
> in the new security_task_getsecid_subj() LSM hook.
>
> This patch fixes this and attempts to make things more obvious by
> introducing a new function, task_sid_subj(), and renaming the
> existing task_sid() function to task_sid_obj().
>
> Signed-off-by: Paul Moore
This looks good, though I have some questions around whether _subj use is
correct in selinux_binder_transaction(). So off to learn more binder
> ---
> security/selinux/hooks.c | 85
> +++---
> 1 file changed, 49 insertions(+), 36 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f311541c4972e..1c53000d28e37 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -229,10 +229,23 @@ static inline u32 cred_sid(const struct cred *cred)
> return tsec->sid;
> }
>
> +/*
> + * get the subjective security ID of a task
> + */
> +static inline u32 task_sid_subj(const struct task_struct *task)
> +{
> + u32 sid;
> +
> + rcu_read_lock();
> + sid = cred_sid(rcu_dereference(task->cred));
> + rcu_read_unlock();
> + return sid;
> +}
> +
> /*
> * get the objective security ID of a task
> */
> -static inline u32 task_sid(const struct task_struct *task)
> +static inline u32 task_sid_obj(const struct task_struct *task)
> {
> u32 sid;
>
> @@ -2034,11 +2047,8 @@ static inline u32 open_file_to_av(struct file *file)
>
> static int selinux_binder_set_context_mgr(struct task_struct *mgr)
> {
> - u32 mysid = current_sid();
> - u32 mgrsid = task_sid(mgr);
> -
> return avc_has_perm(&selinux_state,
> - mysid, mgrsid, SECCLASS_BINDER,
> + current_sid(), task_sid_obj(mgr), SECCLASS_BINDER,
> BINDER__SET_CONTEXT_MGR, NULL);
> }
>
> @@ -2046,8 +2056,8 @@ static int selinux_binder_transaction(struct
> task_struct *from,
> struct task_struct *to)
> {
> u32 mysid = current_sid();
> - u32 fromsid = task_sid(from);
> - u32 tosid = task_sid(to);
> + u32 fromsid = task_sid_subj(from);
> + u32 tosid = task_sid_subj(to);
> int rc;
>
> if (mysid != fromsid) {
> @@ -2066,11 +2076,9 @@ static int selinux_binder_transaction(struct
> task_struct *from,
> static int selinux_binder_transfer_binder(struct task_struct *from,
> struct task_struct *to)
> {
> - u32 fromsid = task_sid(from);
> - u32 tosid = task_sid(to);
> -
> return avc_has_perm(&selinux_state,
> - fromsid, tosid, SECCLASS_BINDER, BINDER__TRANSFER,
> + task_sid_subj(from), task_sid_obj(to),
> + SECCLASS_BINDER, BINDER__TRANSFER,
> NULL);
> }
>
> @@ -2078,7 +2086,7 @@ static int selinux_binder_transfer_file(struct
> task_struct *from,
> struct task_struct *to,
> struct file *file)
> {
> - u32 sid = task_sid(to);
> + u32 sid = task_sid_subj(to);
> struct file_security_struct *fsec = selinux_file(file);
> struct dentry *dentry = file->f_path.dentry;
> struct inode_security_struct *isec;
> @@ -2114,10 +2122,10 @@ static int selinux_binder_transfer_file(struct
> task_struct *from,
> }
>
> static int selinux_ptrace_access_check(struct task_struct *child,
> - unsigned int mode)
> +unsigned int mode)
> {
> u32 sid = current_sid();
> - u32 csid = task_sid(child);
> + u32 csid = task_sid_obj(child);
>
> if (mode & PTRACE_MODE_READ)
> return avc_has_perm(&selinux_state,
> @@ -2130,15 +2138,15 @@ static int selinux_ptrace_access_check(struct
> task_struct *child,
> static int selinux_ptrace_traceme(struct task_struct *parent)
> {
> return avc_has_perm(&selinux_state,
> - task_sid(parent), current_sid(), SECCLASS_PROCESS,
> - PROCESS__PTRACE, NULL);
> + task_sid_subj(parent), task_sid_obj(current),
> + SECCLASS_PROCESS, PROCESS__PTRACE, NULL);
> }
>
> static int selinux_capget(struct task_struct *target, kernel_cap_t
> *effective,
> kernel_cap_t *inheritable, kernel_cap_t *permitted)
> {
> return avc_has_perm(&selinux_state,
> - current_sid(), task_sid(target), SECCLASS_PROCESS,
> + current_sid(), task_sid_obj(target),
> SECCLASS_PROCESS,
> PROCESS__GETCAP, NULL);
> }
>
> @@ -2263,7 +2271,7 @@ static u32 ptrace
Re: [RFC PATCH 4/4] apparmor: differentiate between subjective and objective task credentials
On 2/19/21 3:29 PM, Paul Moore wrote:
> With the split of the security_task_getsecid() into subjective and
> objective variants it's time to update AppArmor to ensure it is
> using the correct task creds.
>
> Signed-off-by: Paul Moore
This has a couple problems, that I will work on addressing
> ---
> security/apparmor/domain.c |2 +-
> security/apparmor/include/cred.h | 19 ---
> security/apparmor/include/task.h |3 ++-
> security/apparmor/lsm.c | 23 +++
> security/apparmor/task.c | 23 ---
> 5 files changed, 54 insertions(+), 16 deletions(-)
>
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index f919ebd042fd2..9ed00b8dcdf0c 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -67,7 +67,7 @@ static int may_change_ptraced_domain(struct aa_label
> *to_label,
> tracer = ptrace_parent(current);
> if (tracer)
> /* released below */
> - tracerl = aa_get_task_label(tracer);
> + tracerl = aa_get_task_label_subj(tracer);
>
> /* not ptraced */
> if (!tracer || unconfined(tracerl))
> diff --git a/security/apparmor/include/cred.h
> b/security/apparmor/include/cred.h
> index 0b9ae4804ef73..43c21ef5568ab 100644
> --- a/security/apparmor/include/cred.h
> +++ b/security/apparmor/include/cred.h
> @@ -64,14 +64,27 @@ static inline struct aa_label
> *aa_get_newest_cred_label(const struct cred *cred)
> }
>
> /**
> - * __aa_task_raw_label - retrieve another task's label
> + * __aa_task_raw_label_subj - retrieve another task's subjective label
> * @task: task to query (NOT NULL)
> *
> - * Returns: @task's label without incrementing its ref count
> + * Returns: @task's subjective label without incrementing its ref count
> *
> * If @task != current needs to be called in RCU safe critical section
> */
> -static inline struct aa_label *__aa_task_raw_label(struct task_struct *task)
> +static inline struct aa_label *__aa_task_raw_label_subj(struct task_struct
> *task)
> +{
> + return aa_cred_raw_label(rcu_dereference(task->cred));
> +}
> +
> +/**
> + * __aa_task_raw_label_obj - retrieve another task's objective label
> + * @task: task to query (NOT NULL)
> + *
> + * Returns: @task's objective label without incrementing its ref count
> + *
> + * If @task != current needs to be called in RCU safe critical section
> + */
> +static inline struct aa_label *__aa_task_raw_label_obj(struct task_struct
> *task)
> {
> return aa_cred_raw_label(__task_cred(task));
> }
> diff --git a/security/apparmor/include/task.h
> b/security/apparmor/include/task.h
> index f13d12373b25e..27a2961558555 100644
> --- a/security/apparmor/include/task.h
> +++ b/security/apparmor/include/task.h
> @@ -33,7 +33,8 @@ int aa_replace_current_label(struct aa_label *label);
> int aa_set_current_onexec(struct aa_label *label, bool stack);
> int aa_set_current_hat(struct aa_label *label, u64 token);
> int aa_restore_previous_label(u64 cookie);
> -struct aa_label *aa_get_task_label(struct task_struct *task);
> +struct aa_label *aa_get_task_label_subj(struct task_struct *task);
> +struct aa_label *aa_get_task_label_obj(struct task_struct *task);
>
> /**
> * aa_free_task_ctx - free a task_ctx
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 15e37b9132679..38430851675b9 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -119,7 +119,7 @@ static int apparmor_ptrace_access_check(struct
> task_struct *child,
> int error;
>
> tracer = __begin_current_label_crit_section();
> - tracee = aa_get_task_label(child);
> + tracee = aa_get_task_label_obj(child);
> error = aa_may_ptrace(tracer, tracee,
> (mode & PTRACE_MODE_READ) ? AA_PTRACE_READ
> : AA_PTRACE_TRACE);
> @@ -135,7 +135,7 @@ static int apparmor_ptrace_traceme(struct task_struct
> *parent)
> int error;
>
> tracee = __begin_current_label_crit_section();
> - tracer = aa_get_task_label(parent);
> + tracer = aa_get_task_label_subj(parent);
> error = aa_may_ptrace(tracer, tracee, AA_PTRACE_TRACE);
> aa_put_label(tracer);
> __end_current_label_crit_section(tracee);
> @@ -719,9 +719,16 @@ static void apparmor_bprm_committed_creds(struct
> linux_binprm *bprm)
> return;
> }
>
> -static void apparmor_task_getsecid(struct task_struct *p, u32 *secid)
> +static void apparmor_task_getsecid_subj(struct task_struct *p, u32 *secid)
> {
> - struct aa_label *label = aa_get_task_label(p);
> + struct aa_label *label = aa_get_task_label_subj(p);
> + *secid = label->secid;
> + aa_put_label(label);
> +}
> +
> +static void apparmor_task_getsecid_obj(struct task_struct *p, u32 *secid)
> +{
> + struct aa_label *label = aa_get_task_label_obj(p);
> *secid = label->secid;
>
Re: [RFC PATCH 1/4] lsm: separate security_task_getsecid() into subjective and objective variants
On 2/19/21 3:29 PM, Paul Moore wrote:
> Of the three LSMs that implement the security_task_getsecid() LSM
> hook, all three LSMs provide the task's objective security
> credentials. This turns out to be unfortunate as most of the hook's
> callers seem to expect the task's subjective credentials, although
> a small handful of callers do correctly expect the objective
> credentials.
>
> This patch is the first step towards fixing the problem: it splits
> the existing security_task_getsecid() hook into two variants, one
> for the subjective creds, one for the objective creds.
>
> void security_task_getsecid_subj(struct task_struct *p,
> u32 *secid);
> void security_task_getsecid_obj(struct task_struct *p,
> u32 *secid);
>
> While this patch does fix all of the callers to use the correct
> variant, in order to keep this patch focused on the callers and to
> ease review, the LSMs continue to use the same implementation for
> both hooks. The net effect is that this patch should not change
> the behavior of the kernel in any way, it will be up to the latter
> LSM specific patches in this series to change the hook
> implementations and return the correct credentials.
>
> Signed-off-by: Paul Moore
So far this looks good. I want to take another stab at it and give
it some testing
> ---
> drivers/android/binder.c |2 +-
> include/linux/cred.h |2 +-
> include/linux/lsm_hook_defs.h |5 -
> include/linux/lsm_hooks.h |8 ++--
> include/linux/security.h | 10 --
> kernel/audit.c|4 ++--
> kernel/auditfilter.c |3 ++-
> kernel/auditsc.c |8
> net/netlabel/netlabel_unlabeled.c |2 +-
> net/netlabel/netlabel_user.h |2 +-
> security/apparmor/lsm.c |3 ++-
> security/integrity/ima/ima_appraise.c |2 +-
> security/integrity/ima/ima_main.c | 14 +++---
> security/security.c | 13 ++---
> security/selinux/hooks.c |3 ++-
> security/smack/smack_lsm.c|3 ++-
> 16 files changed, 54 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index c119736ca56ac..39d501261108d 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -2700,7 +2700,7 @@ static void binder_transaction(struct binder_proc *proc,
> u32 secid;
> size_t added_size;
>
> - security_task_getsecid(proc->tsk, &secid);
> + security_task_getsecid_subj(proc->tsk, &secid);
> ret = security_secid_to_secctx(secid, &secctx, &secctx_sz);
> if (ret) {
> return_error = BR_FAILED_REPLY;
> diff --git a/include/linux/cred.h b/include/linux/cred.h
> index 18639c069263f..42b9d88d9a565 100644
> --- a/include/linux/cred.h
> +++ b/include/linux/cred.h
> @@ -140,7 +140,7 @@ struct cred {
> struct key *request_key_auth; /* assumed request_key authority */
> #endif
> #ifdef CONFIG_SECURITY
> - void*security; /* subjective LSM security */
> + void*security; /* LSM security */
> #endif
> struct user_struct *user; /* real user ID subscription */
> struct user_namespace *user_ns; /* user_ns the caps and keyrings are
> relative to. */
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index dfd261dcbcb04..1490a185135a0 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -200,7 +200,10 @@ LSM_HOOK(int, 0, task_fix_setgid, struct cred *new,
> const struct cred * old,
> LSM_HOOK(int, 0, task_setpgid, struct task_struct *p, pid_t pgid)
> LSM_HOOK(int, 0, task_getpgid, struct task_struct *p)
> LSM_HOOK(int, 0, task_getsid, struct task_struct *p)
> -LSM_HOOK(void, LSM_RET_VOID, task_getsecid, struct task_struct *p, u32
> *secid)
> +LSM_HOOK(void, LSM_RET_VOID, task_getsecid_subj,
> + struct task_struct *p, u32 *secid)
> +LSM_HOOK(void, LSM_RET_VOID, task_getsecid_obj,
> + struct task_struct *p, u32 *secid)
> LSM_HOOK(int, 0, task_setnice, struct task_struct *p, int nice)
> LSM_HOOK(int, 0, task_setioprio, struct task_struct *p, int ioprio)
> LSM_HOOK(int, 0, task_getioprio, struct task_struct *p)
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index bdfc8a76a4f79..13d2a9a6f2014 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -706,8 +706,12 @@
> * @p.
> * @p contains the task_struct for the process.
> * Return 0 if permission is granted.
> - * @task_getsecid:
> - * Retrieve the security identifier of the process @p.
> + * @task_getsecid_subj:
> + * Retrieve the subjective security identifier of the process @p.
> + * @p contains the t
Re: [RFC PATCH 1/4] lsm: separate security_task_getsecid() into subjective and objective variants
On 3/9/21 4:28 PM, Paul Moore wrote: > On Wed, Mar 3, 2021 at 7:44 PM Paul Moore wrote: >> On Sun, Feb 21, 2021 at 7:51 AM John Johansen >> wrote: >>> On 2/19/21 3:29 PM, Paul Moore wrote: >>>> Of the three LSMs that implement the security_task_getsecid() LSM >>>> hook, all three LSMs provide the task's objective security >>>> credentials. This turns out to be unfortunate as most of the hook's >>>> callers seem to expect the task's subjective credentials, although >>>> a small handful of callers do correctly expect the objective >>>> credentials. >>>> >>>> This patch is the first step towards fixing the problem: it splits >>>> the existing security_task_getsecid() hook into two variants, one >>>> for the subjective creds, one for the objective creds. >>>> >>>> void security_task_getsecid_subj(struct task_struct *p, >>>> u32 *secid); >>>> void security_task_getsecid_obj(struct task_struct *p, >>>> u32 *secid); >>>> >>>> While this patch does fix all of the callers to use the correct >>>> variant, in order to keep this patch focused on the callers and to >>>> ease review, the LSMs continue to use the same implementation for >>>> both hooks. The net effect is that this patch should not change >>>> the behavior of the kernel in any way, it will be up to the latter >>>> LSM specific patches in this series to change the hook >>>> implementations and return the correct credentials. >>>> >>>> Signed-off-by: Paul Moore >>> >>> So far this looks good. I want to take another stab at it and give >>> it some testing >> >> Checking in as I know you said you needed to fix/release the AppArmor >> patch in this series ... is this patch still looking okay to you? If >> so, can I get an ACK at least on this patch? > > Hi John, > > Any objections if I merge the LSM, SELinux, and Smack patches into the > selinux/next tree so that we can start getting some wider testing? If > I leave out my poor attempt at an AppArmor patch, the current in-tree > AppArmor code should behave exactly as it does today with the > apparmor_task_getsecid() function handling both the subjective and > objective creds. I can always merge the AppArmor patch later when you > have it ready, or you can merge it via your AppArmor tree at a later > date. > I have some questions around selinux and binder but I don't have any objections to you merging, we can always drop fixes on top if they are necessary -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH 1/4] lsm: separate security_task_getsecid() into subjective and objective variants
On 2/19/21 3:29 PM, Paul Moore wrote:
> Of the three LSMs that implement the security_task_getsecid() LSM
> hook, all three LSMs provide the task's objective security
> credentials. This turns out to be unfortunate as most of the hook's
> callers seem to expect the task's subjective credentials, although
> a small handful of callers do correctly expect the objective
> credentials.
>
> This patch is the first step towards fixing the problem: it splits
> the existing security_task_getsecid() hook into two variants, one
> for the subjective creds, one for the objective creds.
>
> void security_task_getsecid_subj(struct task_struct *p,
> u32 *secid);
> void security_task_getsecid_obj(struct task_struct *p,
> u32 *secid);
>
> While this patch does fix all of the callers to use the correct
> variant, in order to keep this patch focused on the callers and to
> ease review, the LSMs continue to use the same implementation for
> both hooks. The net effect is that this patch should not change
> the behavior of the kernel in any way, it will be up to the latter
> LSM specific patches in this series to change the hook
> implementations and return the correct credentials.
>
> Signed-off-by: Paul Moore
Reviewed-by: John Johansen
> ---
> drivers/android/binder.c |2 +-
> include/linux/cred.h |2 +-
> include/linux/lsm_hook_defs.h |5 -
> include/linux/lsm_hooks.h |8 ++--
> include/linux/security.h | 10 --
> kernel/audit.c|4 ++--
> kernel/auditfilter.c |3 ++-
> kernel/auditsc.c |8
> net/netlabel/netlabel_unlabeled.c |2 +-
> net/netlabel/netlabel_user.h |2 +-
> security/apparmor/lsm.c |3 ++-
> security/integrity/ima/ima_appraise.c |2 +-
> security/integrity/ima/ima_main.c | 14 +++---
> security/security.c | 13 ++---
> security/selinux/hooks.c |3 ++-
> security/smack/smack_lsm.c|3 ++-
> 16 files changed, 54 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index c119736ca56ac..39d501261108d 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -2700,7 +2700,7 @@ static void binder_transaction(struct binder_proc *proc,
> u32 secid;
> size_t added_size;
>
> - security_task_getsecid(proc->tsk, &secid);
> + security_task_getsecid_subj(proc->tsk, &secid);
> ret = security_secid_to_secctx(secid, &secctx, &secctx_sz);
> if (ret) {
> return_error = BR_FAILED_REPLY;
> diff --git a/include/linux/cred.h b/include/linux/cred.h
> index 18639c069263f..42b9d88d9a565 100644
> --- a/include/linux/cred.h
> +++ b/include/linux/cred.h
> @@ -140,7 +140,7 @@ struct cred {
> struct key *request_key_auth; /* assumed request_key authority */
> #endif
> #ifdef CONFIG_SECURITY
> - void*security; /* subjective LSM security */
> + void*security; /* LSM security */
> #endif
> struct user_struct *user; /* real user ID subscription */
> struct user_namespace *user_ns; /* user_ns the caps and keyrings are
> relative to. */
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index dfd261dcbcb04..1490a185135a0 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -200,7 +200,10 @@ LSM_HOOK(int, 0, task_fix_setgid, struct cred *new,
> const struct cred * old,
> LSM_HOOK(int, 0, task_setpgid, struct task_struct *p, pid_t pgid)
> LSM_HOOK(int, 0, task_getpgid, struct task_struct *p)
> LSM_HOOK(int, 0, task_getsid, struct task_struct *p)
> -LSM_HOOK(void, LSM_RET_VOID, task_getsecid, struct task_struct *p, u32
> *secid)
> +LSM_HOOK(void, LSM_RET_VOID, task_getsecid_subj,
> + struct task_struct *p, u32 *secid)
> +LSM_HOOK(void, LSM_RET_VOID, task_getsecid_obj,
> + struct task_struct *p, u32 *secid)
> LSM_HOOK(int, 0, task_setnice, struct task_struct *p, int nice)
> LSM_HOOK(int, 0, task_setioprio, struct task_struct *p, int ioprio)
> LSM_HOOK(int, 0, task_getioprio, struct task_struct *p)
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index bdfc8a76a4f79..13d2a9a6f2014 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -706,8 +706,12 @@
> * @p.
> * @p contains the tas
Re: [RFC PATCH 3/4] smack: differentiate between subjective and objective task credentials
On 2/19/21 3:29 PM, Paul Moore wrote:
> With the split of the security_task_getsecid() into subjective and
> objective variants it's time to update Smack to ensure it is using
> the correct task creds.
>
> Signed-off-by: Paul Moore
Reviewed-by: John Johansen
> ---
> security/smack/smack.h | 18 +-
> security/smack/smack_lsm.c | 40 +++-
> 2 files changed, 44 insertions(+), 14 deletions(-)
>
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index a9768b12716bf..08f9cb80655ce 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -383,7 +383,23 @@ static inline struct smack_known *smk_of_task(const
> struct task_smack *tsp)
> return tsp->smk_task;
> }
>
> -static inline struct smack_known *smk_of_task_struct(
> +static inline struct smack_known *smk_of_task_struct_subj(
> + const struct task_struct *t)
> +{
> + struct smack_known *skp;
> + const struct cred *cred;
> +
> + rcu_read_lock();
> +
> + cred = rcu_dereference(t->cred);
> + skp = smk_of_task(smack_cred(cred));
> +
> + rcu_read_unlock();
> +
> + return skp;
> +}
> +
> +static inline struct smack_known *smk_of_task_struct_obj(
> const struct task_struct *t)
> {
> struct smack_known *skp;
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 2bb354ef2c4a9..ea1a82742e8ba 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -159,7 +159,7 @@ static int smk_bu_current(char *note, struct smack_known
> *oskp,
> static int smk_bu_task(struct task_struct *otp, int mode, int rc)
> {
> struct task_smack *tsp = smack_cred(current_cred());
> - struct smack_known *smk_task = smk_of_task_struct(otp);
> + struct smack_known *smk_task = smk_of_task_struct_obj(otp);
> char acc[SMK_NUM_ACCESS_TYPE + 1];
>
> if (rc <= 0)
> @@ -479,7 +479,7 @@ static int smack_ptrace_access_check(struct task_struct
> *ctp, unsigned int mode)
> {
> struct smack_known *skp;
>
> - skp = smk_of_task_struct(ctp);
> + skp = smk_of_task_struct_obj(ctp);
>
> return smk_ptrace_rule_check(current, skp, mode, __func__);
> }
> @@ -2031,7 +2031,7 @@ static int smk_curacc_on_task(struct task_struct *p,
> int access,
> const char *caller)
> {
> struct smk_audit_info ad;
> - struct smack_known *skp = smk_of_task_struct(p);
> + struct smack_known *skp = smk_of_task_struct_subj(p);
> int rc;
>
> smk_ad_init(&ad, caller, LSM_AUDIT_DATA_TASK);
> @@ -2076,15 +2076,29 @@ static int smack_task_getsid(struct task_struct *p)
> }
>
> /**
> - * smack_task_getsecid - get the secid of the task
> - * @p: the object task
> + * smack_task_getsecid_subj - get the subjective secid of the task
> + * @p: the task
> * @secid: where to put the result
> *
> - * Sets the secid to contain a u32 version of the smack label.
> + * Sets the secid to contain a u32 version of the task's subjective smack
> label.
> + */
> +static void smack_task_getsecid_subj(struct task_struct *p, u32 *secid)
> +{
> + struct smack_known *skp = smk_of_task_struct_subj(p);
> +
> + *secid = skp->smk_secid;
> +}
> +
> +/**
> + * smack_task_getsecid_obj - get the objective secid of the task
> + * @p: the task
> + * @secid: where to put the result
> + *
> + * Sets the secid to contain a u32 version of the task's objective smack
> label.
> */
> -static void smack_task_getsecid(struct task_struct *p, u32 *secid)
> +static void smack_task_getsecid_obj(struct task_struct *p, u32 *secid)
> {
> - struct smack_known *skp = smk_of_task_struct(p);
> + struct smack_known *skp = smk_of_task_struct_obj(p);
>
> *secid = skp->smk_secid;
> }
> @@ -2172,7 +2186,7 @@ static int smack_task_kill(struct task_struct *p,
> struct kernel_siginfo *info,
> {
> struct smk_audit_info ad;
> struct smack_known *skp;
> - struct smack_known *tkp = smk_of_task_struct(p);
> + struct smack_known *tkp = smk_of_task_struct_obj(p);
> int rc;
>
> if (!sig)
> @@ -2210,7 +2224,7 @@ static int smack_task_kill(struct task_struct *p,
> struct kernel_siginfo *info,
> static void smack_task_to_inode(struct task_struct *p, struct inode *inode)
> {
> struct inode_smack *isp = smack_inode(inode);
> - struct smack_known *skp = smk_of_task_struct(p);
> + struct smack_known *skp = smk_of_task_struct_obj(
Re: [RFC PATCH 2/4] selinux: clarify task subjective and objective credentials
On 2/19/21 3:29 PM, Paul Moore wrote:
> SELinux has a function, task_sid(), which returns the task's
> objective credentials, but unfortunately is used in a few places
> where the subjective task credentials should be used. Most notably
> in the new security_task_getsecid_subj() LSM hook.
>
> This patch fixes this and attempts to make things more obvious by
> introducing a new function, task_sid_subj(), and renaming the
> existing task_sid() function to task_sid_obj().
>
> Signed-off-by: Paul Moore
I have a couple of questions below but the rest looks good
> ---
> security/selinux/hooks.c | 85
> +++---
> 1 file changed, 49 insertions(+), 36 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f311541c4972e..1c53000d28e37 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -229,10 +229,23 @@ static inline u32 cred_sid(const struct cred *cred)
> return tsec->sid;
> }
>
> +/*
> + * get the subjective security ID of a task
> + */
> +static inline u32 task_sid_subj(const struct task_struct *task)
> +{
> + u32 sid;
> +
> + rcu_read_lock();
> + sid = cred_sid(rcu_dereference(task->cred));
> + rcu_read_unlock();
> + return sid;
> +}
> +
> /*
> * get the objective security ID of a task
> */
> -static inline u32 task_sid(const struct task_struct *task)
> +static inline u32 task_sid_obj(const struct task_struct *task)
> {
> u32 sid;
>
> @@ -2034,11 +2047,8 @@ static inline u32 open_file_to_av(struct file *file)
>
> static int selinux_binder_set_context_mgr(struct task_struct *mgr)
> {
> - u32 mysid = current_sid();
> - u32 mgrsid = task_sid(mgr);
> -
> return avc_has_perm(&selinux_state,
> - mysid, mgrsid, SECCLASS_BINDER,
> + current_sid(), task_sid_obj(mgr), SECCLASS_BINDER,
> BINDER__SET_CONTEXT_MGR, NULL);
> }
>
> @@ -2046,8 +2056,8 @@ static int selinux_binder_transaction(struct
> task_struct *from,
> struct task_struct *to)
> {
> u32 mysid = current_sid();
> - u32 fromsid = task_sid(from);
> - u32 tosid = task_sid(to);
> + u32 fromsid = task_sid_subj(from);
fromsid potentially gets used as both the subject and the object the following
permission checks. It makes sense to use the same cred for both checks but
what I am not sure about yet is whether its actually safe to use the subject
sid when the task isn't current.
ie. I am still trying to determine if there is a race here between the
transaction
request and the permission check.
> + u32 tosid = task_sid_subj(to);
its not clear to me that using the subj for to is correct
> int rc;
>
> if (mysid != fromsid) {
> @@ -2066,11 +2076,9 @@ static int selinux_binder_transaction(struct
> task_struct *from,
> static int selinux_binder_transfer_binder(struct task_struct *from,
> struct task_struct *to)
> {
> - u32 fromsid = task_sid(from);
> - u32 tosid = task_sid(to);
> -
> return avc_has_perm(&selinux_state,
> - fromsid, tosid, SECCLASS_BINDER, BINDER__TRANSFER,
> + task_sid_subj(from), task_sid_obj(to),
> + SECCLASS_BINDER, BINDER__TRANSFER,
> NULL);
> }
>
> @@ -2078,7 +2086,7 @@ static int selinux_binder_transfer_file(struct
> task_struct *from,
> struct task_struct *to,
> struct file *file)
> {
> - u32 sid = task_sid(to);
> + u32 sid = task_sid_subj(to);
same question about safety here
> struct file_security_struct *fsec = selinux_file(file);
> struct dentry *dentry = file->f_path.dentry;
> struct inode_security_struct *isec;
> @@ -2114,10 +2122,10 @@ static int selinux_binder_transfer_file(struct
> task_struct *from,
> }
>
> static int selinux_ptrace_access_check(struct task_struct *child,
> - unsigned int mode)
> +unsigned int mode)
> {
> u32 sid = current_sid();
> - u32 csid = task_sid(child);
> + u32 csid = task_sid_obj(child);
>
> if (mode & PTRACE_MODE_READ)
> return avc_has_perm(&selinux_state,
> @@ -2130,15 +2138,15 @@ static int selinux_ptrace_access_check(struct
> task_struct *child,
> static int selinux_ptrace_traceme(struct task_struct *parent)
> {
> return avc_has_perm(&selinux_state,
> - task_sid(parent), current_sid(), SECCLASS_PROCESS,
> - PROCESS__PTRACE, NULL);
> + task_sid_subj(parent), task_sid_obj(current),
> + SECCLASS_PROCESS, PROCESS__PTRACE, NULL);
> }
>
> static int selinux_capget(struct task_struct *target, kernel_cap_t
> *effective,
>
Re: [PATCH v35 05/29] IMA: avoid label collisions with stacked LSMs
On 4/18/22 07:59, Casey Schaufler wrote:
> Integrity measurement may filter on security module information
> and needs to be clear in the case of multiple active security
> modules which applies. Provide a boot option ima_rules_lsm= to
> allow the user to specify an active security module to apply
> filters to. If not specified, use the first registered module
> that supports the audit_rule_match() LSM hook. Allow the user
> to specify in the IMA policy an lsm= option to specify the
> security module to use for a particular rule.
>
> Signed-off-by: Casey Schaufler
Reviewed-by: John Johansen
> To: Mimi Zohar
> To: linux-integr...@vger.kernel.org
> ---
> Documentation/ABI/testing/ima_policy | 8 -
> include/linux/security.h | 14
> security/integrity/ima/ima_policy.c | 51
> security/security.c | 35 +++
> 4 files changed, 89 insertions(+), 19 deletions(-)
>
> diff --git a/Documentation/ABI/testing/ima_policy
> b/Documentation/ABI/testing/ima_policy
> index 839fab811b18..64863e9d87ea 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -26,7 +26,7 @@ Description:
> [uid=] [euid=] [gid=] [egid=]
> [fowner=] [fgroup=]]
> lsm:[[subj_user=] [subj_role=] [subj_type=]
> - [obj_user=] [obj_role=] [obj_type=]]
> + [obj_user=] [obj_role=] [obj_type=]] [lsm=]
> option: [[appraise_type=]] [template=] [permit_directio]
> [appraise_flag=] [appraise_algos=] [keyrings=]
> base:
> @@ -126,6 +126,12 @@ Description:
>
> measure subj_user=_ func=FILE_CHECK mask=MAY_READ
>
> + It is possible to explicitly specify which security
> + module a rule applies to using lsm=. If the security
> + module specified is not active on the system the rule
> + will be rejected. If lsm= is not specified the first
> + security module registered on the system will be assumed.
> +
> Example of measure rules using alternate PCRs::
>
> measure func=KEXEC_KERNEL_CHECK pcr=4
> diff --git a/include/linux/security.h b/include/linux/security.h
> index d00870d2b416..3666eddad59a 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1985,25 +1985,27 @@ static inline void security_audit_rule_free(struct
> audit_lsm_rules *lsmrules)
> #endif /* CONFIG_AUDIT */
>
> #if defined(CONFIG_IMA_LSM_RULES) && defined(CONFIG_SECURITY)
> -int ima_filter_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule);
> -int ima_filter_rule_match(u32 secid, u32 field, u32 op, void *lsmrule);
> -void ima_filter_rule_free(void *lsmrule);
> +int ima_filter_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule,
> + int lsmslot);
> +int ima_filter_rule_match(u32 secid, u32 field, u32 op, void *lsmrule,
> + int lsmslot);
> +void ima_filter_rule_free(void *lsmrule, int lsmslot);
>
> #else
>
> static inline int ima_filter_rule_init(u32 field, u32 op, char *rulestr,
> -void **lsmrule)
> +void **lsmrule, int lsmslot)
> {
> return 0;
> }
>
> static inline int ima_filter_rule_match(u32 secid, u32 field, u32 op,
> - void *lsmrule)
> + void *lsmrule, int lsmslot)
> {
> return 0;
> }
>
> -static inline void ima_filter_rule_free(void *lsmrule)
> +static inline void ima_filter_rule_free(void *lsmrule, int lsmslot)
> { }
>
> #endif /* defined(CONFIG_IMA_LSM_RULES) && defined(CONFIG_SECURITY) */
> diff --git a/security/integrity/ima/ima_policy.c
> b/security/integrity/ima/ima_policy.c
> index eea6e92500b8..97470354c8ae 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -89,6 +89,7 @@ struct ima_rule_entry {
> bool (*fgroup_op)(kgid_t cred_gid, kgid_t rule_gid); /* gid_eq(),
> gid_gt(), gid_lt() */
> int pcr;
> unsigned int allowed_algos; /* bitfield of allowed hash algorithms */
> + int which; /* which LSM rule applies to */
> struct {
> void *rule; /* LSM file metadata specific */
> char *args_p; /* audit value */
> @@ -285,6 +286,20 @@ static int __init default_appraise_policy_setup(char
> *str)
> }
> __
Re: [PATCH v35 01/29] integrity: disassociate ima_filter_rule from security_audit_rule
On 4/18/22 07:59, Casey Schaufler wrote:
> Create real functions for the ima_filter_rule interfaces.
> These replace #defines that obscure the reuse of audit
> interfaces. The new fuctions are put in security.c because
> they use security module registered hooks that we don't
> want exported.
>
> Signed-off-by: Casey Schaufler
> Acked-by: Paul Moore
Reviewed-by: John Johansen
> ---
> include/linux/security.h | 24
> security/integrity/ima/ima.h | 26 --
> security/security.c | 21 +
> 3 files changed, 45 insertions(+), 26 deletions(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 25b3ef71f495..2986342dad41 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1917,6 +1917,30 @@ static inline void security_audit_rule_free(void
> *lsmrule)
> #endif /* CONFIG_SECURITY */
> #endif /* CONFIG_AUDIT */
>
> +#if defined(CONFIG_IMA_LSM_RULES) && defined(CONFIG_SECURITY)
> +int ima_filter_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule);
> +int ima_filter_rule_match(u32 secid, u32 field, u32 op, void *lsmrule);
> +void ima_filter_rule_free(void *lsmrule);
> +
> +#else
> +
> +static inline int ima_filter_rule_init(u32 field, u32 op, char *rulestr,
> +void **lsmrule)
> +{
> + return 0;
> +}
> +
> +static inline int ima_filter_rule_match(u32 secid, u32 field, u32 op,
> + void *lsmrule)
> +{
> + return 0;
> +}
> +
> +static inline void ima_filter_rule_free(void *lsmrule)
> +{ }
> +
> +#endif /* defined(CONFIG_IMA_LSM_RULES) && defined(CONFIG_SECURITY) */
> +
> #ifdef CONFIG_SECURITYFS
>
> extern struct dentry *securityfs_create_file(const char *name, umode_t mode,
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index be965a8715e4..1b5d70ac2dc9 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -418,32 +418,6 @@ static inline void ima_free_modsig(struct modsig *modsig)
> }
> #endif /* CONFIG_IMA_APPRAISE_MODSIG */
>
> -/* LSM based policy rules require audit */
> -#ifdef CONFIG_IMA_LSM_RULES
> -
> -#define ima_filter_rule_init security_audit_rule_init
> -#define ima_filter_rule_free security_audit_rule_free
> -#define ima_filter_rule_match security_audit_rule_match
> -
> -#else
> -
> -static inline int ima_filter_rule_init(u32 field, u32 op, char *rulestr,
> -void **lsmrule)
> -{
> - return -EINVAL;
> -}
> -
> -static inline void ima_filter_rule_free(void *lsmrule)
> -{
> -}
> -
> -static inline int ima_filter_rule_match(u32 secid, u32 field, u32 op,
> - void *lsmrule)
> -{
> - return -EINVAL;
> -}
> -#endif /* CONFIG_IMA_LSM_RULES */
> -
> #ifdef CONFIG_IMA_READ_POLICY
> #define POLICY_FILE_FLAGS (S_IWUSR | S_IRUSR)
> #else
> diff --git a/security/security.c b/security/security.c
> index b7cf5cbfdc67..22543fdb6041 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2586,6 +2586,27 @@ int security_audit_rule_match(u32 secid, u32 field,
> u32 op, void *lsmrule)
> }
> #endif /* CONFIG_AUDIT */
>
> +#ifdef CONFIG_IMA_LSM_RULES
> +/*
> + * The integrity subsystem uses the same hooks as
> + * the audit subsystem.
> + */
> +int ima_filter_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule)
> +{
> + return call_int_hook(audit_rule_init, 0, field, op, rulestr, lsmrule);
> +}
> +
> +void ima_filter_rule_free(void *lsmrule)
> +{
> + call_void_hook(audit_rule_free, lsmrule);
> +}
> +
> +int ima_filter_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
> +{
> + return call_int_hook(audit_rule_match, 0, secid, field, op, lsmrule);
> +}
> +#endif /* CONFIG_IMA_LSM_RULES */
> +
> #ifdef CONFIG_BPF_SYSCALL
> int security_bpf(int cmd, union bpf_attr *attr, unsigned int size)
> {
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v35 06/29] LSM: Use lsmblob in security_audit_rule_match
On 4/18/22 07:59, Casey Schaufler wrote:
> Change the secid parameter of security_audit_rule_match
> to a lsmblob structure pointer. Pass the entry from the
> lsmblob structure for the approprite slot to the LSM hook.
>
> Change the users of security_audit_rule_match to use the
> lsmblob instead of a u32. The scaffolding function lsmblob_init()
> fills the blob with the value of the old secid, ensuring that
> it is available to the appropriate module hook. The sources of
> the secid, security_task_getsecid() and security_inode_getsecid(),
> will be converted to use the blob structure later in the series.
> At the point the use of lsmblob_init() is dropped.
>
> Signed-off-by: Casey Schaufler
> Acked-by: Paul Moore
Reviewed-by: John Johansen
> Cc: linux-audit@redhat.com
> ---
> include/linux/security.h | 5 +++--
> kernel/auditfilter.c | 6 --
> kernel/auditsc.c | 16 +++-
> security/security.c | 5 +++--
> 4 files changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 3666eddad59a..ee5d14dac65f 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1955,7 +1955,7 @@ static inline int security_key_getsecurity(struct key
> *key, char **_buffer)
> int security_audit_rule_init(u32 field, u32 op, char *rulestr,
>struct audit_lsm_rules *lsmrules);
> int security_audit_rule_known(struct audit_krule *krule);
> -int security_audit_rule_match(u32 secid, u32 field, u32 op,
> +int security_audit_rule_match(struct lsmblob *blob, u32 field, u32 op,
> struct audit_lsm_rules *lsmrules);
> void security_audit_rule_free(struct audit_lsm_rules *lsmrules);
>
> @@ -1972,7 +1972,8 @@ static inline int security_audit_rule_known(struct
> audit_krule *krule)
> return 0;
> }
>
> -static inline int security_audit_rule_match(u32 secid, u32 field, u32 op,
> +static inline int security_audit_rule_match(struct lsmblob *blob,
> + u32 field, u32 op,
> struct audit_lsm_rules *lsmrules)
> {
> return 0;
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index de75bd6ad866..15cd4fe35e9c 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -1337,6 +1337,7 @@ int audit_filter(int msgtype, unsigned int listtype)
>
> for (i = 0; i < e->rule.field_count; i++) {
> struct audit_field *f = &e->rule.fields[i];
> + struct lsmblob blob;
> pid_t pid;
> u32 sid;
>
> @@ -1369,8 +1370,9 @@ int audit_filter(int msgtype, unsigned int listtype)
> case AUDIT_SUBJ_CLR:
> if (f->lsm_str) {
> security_current_getsecid_subj(&sid);
> - result = security_audit_rule_match(sid,
> -f->type, f->op,
> + lsmblob_init(&blob, sid);
> + result = security_audit_rule_match(
> +&blob, f->type, f->op,
> &f->lsm_rules);
> }
> break;
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index d272b5cf18a8..a9d5bfa37cb3 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -468,6 +468,7 @@ static int audit_filter_rules(struct task_struct *tsk,
> const struct cred *cred;
> int i, need_sid = 1;
> u32 sid;
> + struct lsmblob blob;
> unsigned int sessionid;
>
> if (ctx && rule->prio <= ctx->prio)
> @@ -678,8 +679,10 @@ static int audit_filter_rules(struct task_struct *tsk,
> security_current_getsecid_subj(&sid);
> need_sid = 0;
> }
> - result = security_audit_rule_match(sid, f->type,
> - f->op, &f->lsm_rules);
> + lsmblob_init(&blob, sid);
> + result = security_audit_rule_match(&blob,
> + f->type, f->op,
> + &f->lsm_rules);
> }
> break;
> cas
Re: [PATCH v35 04/29] LSM: provide lsm name and id slot mappings
On 4/18/22 07:59, Casey Schaufler wrote:
> Provide interfaces to map LSM slot numbers and LSM names.
> Update the LSM registration code to save this information.
>
> Acked-by: Paul Moore
> Reviewed-by: Kees Cook
> Signed-off-by: Casey Schaufler
Reviewed-by: John Johansen
> ---
> include/linux/security.h | 4
> security/security.c | 45
> 2 files changed, 49 insertions(+)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index ed51baa94a30..d00870d2b416 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -195,6 +195,10 @@ static inline bool lsmblob_equal(const struct lsmblob
> *bloba,
> return !memcmp(bloba, blobb, sizeof(*bloba));
> }
>
> +/* Map lsm names to blob slot numbers */
> +extern int lsm_name_to_slot(char *name);
> +extern const char *lsm_slot_to_name(int slot);
> +
> /* These functions are in security/commoncap.c */
> extern int cap_capable(const struct cred *cred, struct user_namespace *ns,
> int cap, unsigned int opts);
> diff --git a/security/security.c b/security/security.c
> index 49fa61028da2..d1ddbb857af1 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -477,6 +477,50 @@ static int lsm_append(const char *new, char **result)
> * Current index to use while initializing the lsmblob secid list.
> */
> static int lsm_slot __lsm_ro_after_init;
> +static struct lsm_id *lsm_slotlist[LSMBLOB_ENTRIES] __lsm_ro_after_init;
> +
> +/**
> + * lsm_name_to_slot - Report the slot number for a security module
> + * @name: name of the security module
> + *
> + * Look up the slot number for the named security module.
> + * Returns the slot number or LSMBLOB_INVALID if @name is not
> + * a registered security module name.
> + */
> +int lsm_name_to_slot(char *name)
> +{
> + int i;
> +
> + for (i = 0; i < lsm_slot; i++)
> + if (strcmp(lsm_slotlist[i]->lsm, name) == 0)
> + return i;
> +
> + return LSMBLOB_INVALID;
> +}
> +
> +/**
> + * lsm_slot_to_name - Get the name of the security module in a slot
> + * @slot: index into the interface LSM slot list.
> + *
> + * Provide the name of the security module associated with
> + * a interface LSM slot.
> + *
> + * If @slot is LSMBLOB_INVALID return the value
> + * for slot 0 if it has been set, otherwise NULL.
> + *
> + * Returns a pointer to the name string or NULL.
> + */
> +const char *lsm_slot_to_name(int slot)
> +{
> + if (slot == LSMBLOB_INVALID)
> + slot = 0;
> + else if (slot >= LSMBLOB_ENTRIES || slot < 0)
> + return NULL;
> +
> + if (lsm_slotlist[slot] == NULL)
> + return NULL;
> + return lsm_slotlist[slot]->lsm;
> +}
>
> /**
> * security_add_hooks - Add a modules hooks to the hook lists.
> @@ -498,6 +542,7 @@ void __init security_add_hooks(struct security_hook_list
> *hooks, int count,
> if (lsmid->slot == LSMBLOB_NEEDED) {
> if (lsm_slot >= LSMBLOB_ENTRIES)
> panic("%s Too many LSMs registered.\n", __func__);
> + lsm_slotlist[lsm_slot] = lsmid;
> lsmid->slot = lsm_slot++;
> init_debug("%s assigned lsmblob slot %d\n", lsmid->lsm,
> lsmid->slot);
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v35 28/29] LSM: Add /proc attr entry for full LSM context
On 4/18/22 07:59, Casey Schaufler wrote:
> Add an entry /proc/.../attr/context which displays the full
> process security "context" in compound format:
> lsm1\0value\0lsm2\0value\0...
> This entry is not writable.
>
> A security module may decide that its policy does not allow
> this information to be displayed. In this case none of the
> information will be displayed.
>
> Reviewed-by: Kees Cook
> Signed-off-by: Casey Schaufler
Acked-by: John Johansen
> Cc: linux-...@vger.kernel.org
> Cc: linux-...@vger.kernel.org
> ---
> Documentation/security/lsm.rst | 14 +
> fs/proc/base.c | 1 +
> include/linux/lsm_hooks.h| 6 +++
> security/apparmor/include/procattr.h | 2 +-
> security/apparmor/lsm.c | 8 ++-
> security/apparmor/procattr.c | 22
> security/security.c | 79
> security/selinux/hooks.c | 2 +-
> security/smack/smack_lsm.c | 2 +-
> 9 files changed, 121 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/security/lsm.rst b/Documentation/security/lsm.rst
> index b77b4a540391..070225ae6ceb 100644
> --- a/Documentation/security/lsm.rst
> +++ b/Documentation/security/lsm.rst
> @@ -143,3 +143,17 @@ separated list of the active security modules.
> The file ``/proc/pid/attr/interface_lsm`` contains the name of the security
> module for which the ``/proc/pid/attr/current`` interface will
> apply. This interface can be written to.
> +
> +The infrastructure does provide an interface for the special
> +case where multiple security modules provide a process context.
> +This is provided in compound context format.
> +
> +- `lsm\0value\0lsm\0value\0`
> +
> +The `lsm` and `value` fields are NUL-terminated bytestrings.
> +Each field may contain whitespace or non-printable characters.
> +The NUL bytes are included in the size of a compound context.
> +The context ``Bell\0Secret\0Biba\0Loose\0`` has a size of 23.
> +
> +The file ``/proc/pid/attr/context`` provides the security
> +context of the identified process.
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index f2d15348bdff..f8aed4404e7e 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2828,6 +2828,7 @@ static const struct pid_entry attr_dir_stuff[] = {
> ATTR(NULL, "keycreate", 0666),
> ATTR(NULL, "sockcreate",0666),
> ATTR(NULL, "interface_lsm", 0666),
> + ATTR(NULL, "context", 0444),
> #ifdef CONFIG_SECURITY_SMACK
> DIR("smack",0555,
> proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops),
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index fd63ae215104..425538ebc606 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1401,6 +1401,12 @@
> * @pages contains the number of pages.
> * Return 0 if permission is granted.
> *
> + * @getprocattr:
> + * Provide the named process attribute for display in special files in
> + * the /proc/.../attr directory. Attribute naming and the data displayed
> + * is at the discretion of the security modules. The exception is the
> + * "context" attribute, which will contain the security context of the
> + * task as a nul terminated text string without trailing whitespace.
> * @ismaclabel:
> * Check if the extended attribute specified by @name
> * represents a MAC label. Returns 1 if name is a MAC
> diff --git a/security/apparmor/include/procattr.h
> b/security/apparmor/include/procattr.h
> index 31689437e0e1..03dbfdb2f2c0 100644
> --- a/security/apparmor/include/procattr.h
> +++ b/security/apparmor/include/procattr.h
> @@ -11,7 +11,7 @@
> #ifndef __AA_PROCATTR_H
> #define __AA_PROCATTR_H
>
> -int aa_getprocattr(struct aa_label *label, char **string);
> +int aa_getprocattr(struct aa_label *label, char **string, bool newline);
> int aa_setprocattr_changehat(char *args, size_t size, int flags);
>
> #endif /* __AA_PROCATTR_H */
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 29181bc8c693..1ee58c1491ab 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -602,6 +602,7 @@ static int apparmor_getprocattr(struct task_struct *task,
> char *name,
> const struct cred *cred = get_task_cred(task);
> struct aa_task_ctx *ctx = task_ctx(current);
> struct aa_label *label = NULL;
> + bool newline = true;
>
> if (strcmp(name, "current") == 0)
> label = aa_get_newest_label(cred_label(cred));
> @
Re: [PATCH v35 24/29] LSM: Add a function to report multiple LSMs
On 4/18/22 07:59, Casey Schaufler wrote:
> Add a new boolean function lsm_multiple_contexts() to
> identify when multiple security modules provide security
> context strings.
>
> Signed-off-by: Casey Schaufler
Reviewed-by: John Johansen
> ---
> include/linux/security.h | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 2150016492be..3fab84220f88 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -232,6 +232,15 @@ static inline bool lsmblob_equal(const struct lsmblob
> *bloba,
> extern int lsm_name_to_slot(char *name);
> extern const char *lsm_slot_to_name(int slot);
>
> +static inline bool lsm_multiple_contexts(void)
> +{
> +#ifdef CONFIG_SECURITY
> + return lsm_slot_to_name(1) != NULL;
> +#else
> + return false;
> +#endif
> +}
> +
> /**
> * lsmblob_value - find the first non-zero value in an lsmblob structure.
> * @blob: Pointer to the data
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v35 22/29] Audit: Keep multiple LSM data in audit_names
On 4/18/22 07:59, Casey Schaufler wrote:
> Replace the osid field in the audit_names structure
> with a lsmblob structure. This accomodates the use
> of an lsmblob in security_audit_rule_match() and
> security_inode_getsecid().
>
> Signed-off-by: Casey Schaufler
> Acked-by: Paul Moore
> ---
> kernel/audit.h | 2 +-
> kernel/auditsc.c | 22 --
> 2 files changed, 9 insertions(+), 15 deletions(-)
>
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 316fac62d5f7..4af63e7dde17 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -82,7 +82,7 @@ struct audit_names {
> kuid_t uid;
> kgid_t gid;
> dev_t rdev;
> - u32 osid;
> + struct lsmblob lsmblob;
> struct audit_cap_data fcap;
> unsigned intfcap_ver;
> unsigned char type; /* record type */
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 231631f61550..6fe9f2525fc1 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -700,17 +700,16 @@ static int audit_filter_rules(struct task_struct *tsk,
>* lsmblob, which happens later in
>* this patch set.
>*/
> - lsmblob_init(&blob, name->osid);
> result = security_audit_rule_match(
> - &blob,
> + &name->lsmblob,
> f->type,
> f->op,
> &f->lsm_rules);
> } else if (ctx) {
> list_for_each_entry(n,
> &ctx->names_list, list) {
> - lsmblob_init(&blob, n->osid);
> if (security_audit_rule_match(
> - &blob, f->type, f->op,
> + &n->lsmblob,
> + f->type, f->op,
> &f->lsm_rules)) {
> ++result;
> break;
> @@ -1589,13 +1588,12 @@ static void audit_log_name(struct audit_context
> *context, struct audit_names *n,
>from_kgid(&init_user_ns, n->gid),
>MAJOR(n->rdev),
>MINOR(n->rdev));
> - if (n->osid != 0) {
> - struct lsmblob blob;
> + if (lsmblob_is_set(&n->lsmblob)) {
> struct lsmcontext lsmctx;
>
> - lsmblob_init(&blob, n->osid);
> - if (security_secid_to_secctx(&blob, &lsmctx, LSMBLOB_FIRST)) {
> - audit_log_format(ab, " osid=%u", n->osid);
> + if (security_secid_to_secctx(&n->lsmblob, &lsmctx,
> + LSMBLOB_FIRST)) {
> + audit_log_format(ab, " osid=?");
is there something better we can do here? This feels like a regression
> if (call_panic)
> *call_panic = 2;
> } else {
> @@ -2297,17 +2295,13 @@ static void audit_copy_inode(struct audit_names *name,
>const struct dentry *dentry,
>struct inode *inode, unsigned int flags)
> {
> - struct lsmblob blob;
> -
> name->ino = inode->i_ino;
> name->dev = inode->i_sb->s_dev;
> name->mode = inode->i_mode;
> name->uid = inode->i_uid;
> name->gid = inode->i_gid;
> name->rdev = inode->i_rdev;
> - security_inode_getsecid(inode, &blob);
> - /* scaffolding until osid is updated */
> - name->osid = lsmblob_first(&blob);
> + security_inode_getsecid(inode, &name->lsmblob);
> if (flags & AUDIT_INODE_NOEVAL) {
> name->fcap_ver = -1;
> return;
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v35 23/29] Audit: Create audit_stamp structure
On 4/18/22 07:59, Casey Schaufler wrote:
> Replace the timestamp and serial number pair used in audit records
> with a structure containing the two elements.
>
> Signed-off-by: Casey Schaufler
> Acked-by: Paul Moore
> ---
> kernel/audit.c | 17 +
> kernel/audit.h | 12 +---
> kernel/auditsc.c | 22 +-
> 3 files changed, 27 insertions(+), 24 deletions(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 28ff7a5f90bd..6b6c089512f7 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1822,11 +1822,11 @@ unsigned int audit_serial(void)
> }
>
> static inline void audit_get_stamp(struct audit_context *ctx,
> -struct timespec64 *t, unsigned int *serial)
> +struct audit_stamp *stamp)
> {
> - if (!ctx || !auditsc_get_stamp(ctx, t, serial)) {
> - ktime_get_coarse_real_ts64(t);
> - *serial = audit_serial();
> + if (!ctx || !auditsc_get_stamp(ctx, stamp)) {
> + ktime_get_coarse_real_ts64(&stamp->ctime);
> + stamp->serial = audit_serial();
> }
> }
>
> @@ -1849,8 +1849,7 @@ struct audit_buffer *audit_log_start(struct
> audit_context *ctx, gfp_t gfp_mask,
>int type)
> {
> struct audit_buffer *ab;
> - struct timespec64 t;
> - unsigned int serial;
> + struct audit_stamp stamp;
>
> if (audit_initialized != AUDIT_INITIALIZED)
> return NULL;
> @@ -1905,12 +1904,14 @@ struct audit_buffer *audit_log_start(struct
> audit_context *ctx, gfp_t gfp_mask,
> return NULL;
> }
>
> - audit_get_stamp(ab->ctx, &t, &serial);
> + audit_get_stamp(ab->ctx, &stamp);
> /* cancel dummy context to enable supporting records */
> if (ctx)
> ctx->dummy = 0;
> audit_log_format(ab, "audit(%llu.%03lu:%u): ",
> - (unsigned long long)t.tv_sec, t.tv_nsec/100,
> serial);
> + (unsigned long long)stamp.ctime.tv_sec,
> + stamp.ctime.tv_nsec/100,
> + stamp.serial);
>
> return ab;
> }
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 4af63e7dde17..260dab6e0e15 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -99,6 +99,12 @@ struct audit_proctitle {
> char*value; /* the cmdline field */
> };
>
> +/* A timestamp/serial pair to identify an event */
> +struct audit_stamp {
> + struct timespec64 ctime; /* time of syscall entry */
> + unsigned intserial; /* serial number for record */
> +};
> +
> /* The per-task audit context. */
> struct audit_context {
> int dummy; /* must be the first element */
> @@ -108,10 +114,10 @@ struct audit_context {
> AUDIT_CTX_URING,/* in use by io_uring */
> } context;
> enum audit_statestate, current_state;
> + struct audit_stamp stamp; /* event identifier */
> unsigned intserial; /* serial number for record */
shouldn't we be dropping serial from the audit_context, since we have
moved it into the audit_stamp?
> int major; /* syscall number */
> int uring_op; /* uring operation */
> - struct timespec64 ctime; /* time of syscall entry */
> unsigned long argv[4];/* syscall arguments */
> longreturn_code;/* syscall return code */
> u64 prio;
> @@ -265,7 +271,7 @@ extern void audit_put_tty(struct tty_struct *tty);
> #ifdef CONFIG_AUDITSYSCALL
> extern unsigned int audit_serial(void);
> extern int auditsc_get_stamp(struct audit_context *ctx,
> - struct timespec64 *t, unsigned int *serial);
> + struct audit_stamp *stamp);
>
> extern void audit_put_watch(struct audit_watch *watch);
> extern void audit_get_watch(struct audit_watch *watch);
> @@ -306,7 +312,7 @@ extern void audit_filter_inodes(struct task_struct *tsk,
> struct audit_context *ctx);
> extern struct list_head *audit_killed_trees(void);
> #else /* CONFIG_AUDITSYSCALL */
> -#define auditsc_get_stamp(c, t, s) 0
> +#define auditsc_get_stamp(c, s) 0
> #define audit_put_watch(w) do { } while (0)
> #define audit_get_watch(w) do { } while (0)
> #define audit_to_watch(k, p, l, o) (-EINVAL)
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 6fe9f2525fc1..557713954a69 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -992,10 +992,10 @@ static void audit_reset_context(struct audit_context
> *ctx)
>*/
>
> ctx->current_state = ctx->state;
> - ctx->serial = 0;
> + ctx->stamp.serial = 0;
> ctx->major = 0;
> ctx->uring_op = 0;
> - ctx->ctime = (struct timespec64){ .tv_sec = 0, .tv_nsec = 0 };
> + ctx->stamp.ctime = (struct timespec64){ .tv_s
Re: [PATCH v35 21/29] LSM: Extend security_secid_to_secctx to include module selection
On 4/18/22 07:59, Casey Schaufler wrote:
> Add a parameter to security_secid_to_secctx() to identify
> which of the security modules that may be active should
> provide the security context. If the parameter is greater
> than or equal to zero, the security module associated with
> that LSM "slot" is used. If the value is LSMBLOB_DISPLAY
> the "interface lsm" is used. If the value is LSMBLOB_FIRST
> the first security module providing a hook is used.
>
So the patch does change behavior from previously doing
effectively LSMBLOB_DISPLAY everywhere to using LSMBLOB_FIRST
in certain cases. I think the reason for the change needs
to called out. I think a note in the patch description
would do.
> Signed-off-by: Casey Schaufler
> ---
> drivers/android/binder.c| 2 +-
> include/linux/security.h| 7 +--
> include/net/scm.h | 2 +-
> kernel/audit.c | 4 ++--
> kernel/auditsc.c| 7 ---
> net/ipv4/ip_sockglue.c | 2 +-
> net/netfilter/nf_conntrack_netlink.c| 4 ++--
> net/netfilter/nf_conntrack_standalone.c | 2 +-
> net/netfilter/nfnetlink_queue.c | 2 +-
> net/netlabel/netlabel_unlabeled.c | 11 +++
> net/netlabel/netlabel_user.c| 2 +-
> security/security.c | 20 ++--
> 12 files changed, 44 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 259f5e38e6ba..d59c4ebf7e22 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -2983,7 +2983,7 @@ static void binder_transaction(struct binder_proc *proc,
> size_t added_size;
>
> security_cred_getsecid(proc->cred, &blob);
> - ret = security_secid_to_secctx(&blob, &lsmctx);
> + ret = security_secid_to_secctx(&blob, &lsmctx, LSMBLOB_DISPLAY);
> if (ret) {
> return_error = BR_FAILED_REPLY;
> return_error_param = ret;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index dc66f3f48456..2150016492be 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -184,6 +184,8 @@ struct lsmblob {
> #define LSMBLOB_INVALID -1 /* Not a valid LSM slot number
> */
> #define LSMBLOB_NEEDED -2 /* Slot requested on
> initialization */
> #define LSMBLOB_NOT_NEEDED -3 /* Slot not requested */
> +#define LSMBLOB_DISPLAY -4 /* Use the "interface_lsm" slot
> */
> +#define LSMBLOB_FIRST-5 /* Use the first slot */
>
> /**
> * lsmblob_init - initialize a lsmblob structure
> @@ -615,7 +617,8 @@ int security_setprocattr(const char *lsm, const char
> *name, void *value,
>size_t size);
> int security_netlink_send(struct sock *sk, struct sk_buff *skb);
> int security_ismaclabel(const char *name);
> -int security_secid_to_secctx(struct lsmblob *blob, struct lsmcontext *cp);
> +int security_secid_to_secctx(struct lsmblob *blob, struct lsmcontext *cp,
> + int ilsm);
> int security_secctx_to_secid(const char *secdata, u32 seclen,
>struct lsmblob *blob);
> void security_release_secctx(struct lsmcontext *cp);
> @@ -1470,7 +1473,7 @@ static inline int security_ismaclabel(const char *name)
> }
>
> static inline int security_secid_to_secctx(struct lsmblob *blob,
> -struct lsmcontext *cp)
> +struct lsmcontext *cp, int ilsm)
> {
> return -EOPNOTSUPP;
> }
> diff --git a/include/net/scm.h b/include/net/scm.h
> index b77a52f93389..f4d567d4885e 100644
> --- a/include/net/scm.h
> +++ b/include/net/scm.h
> @@ -101,7 +101,7 @@ static inline void scm_passec(struct socket *sock, struct
> msghdr *msg, struct sc
>* and the infrastructure will know which it is.
>*/
> lsmblob_init(&lb, scm->secid);
> - err = security_secid_to_secctx(&lb, &context);
> + err = security_secid_to_secctx(&lb, &context, LSMBLOB_DISPLAY);
>
> if (!err) {
> put_cmsg(msg, SOL_SOCKET, SCM_SECURITY, context.len,
> diff --git a/kernel/audit.c b/kernel/audit.c
> index a885ebdbb91e..28ff7a5f90bd 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1464,7 +1464,7 @@ static int audit_receive_msg(struct sk_buff *skb,
> struct nlmsghdr *nlh)
>
> if (lsmblob_is_set(&audit_sig_lsm)) {
> err = security_secid_to_secctx(&audit_sig_lsm,
> -&context);
> +&context, LSMBLOB_FIRST);
> if (err)
> return err;
> }
> @@ -2176,7 +2176,7 @@ int audit_log_tas
Re: [PATCH v35 26/29] Audit: Add record for multiple task security contexts
On 4/18/22 07:59, Casey Schaufler wrote:
> Create a new audit record AUDIT_MAC_TASK_CONTEXTS.
> An example of the MAC_TASK_CONTEXTS (1420) record is:
>
> type=MAC_TASK_CONTEXTS[1420]
> msg=audit(1600880931.832:113)
> subj_apparmor=unconfined
> subj_smack=_
>
> When an audit event includes a AUDIT_MAC_TASK_CONTEXTS record
> the "subj=" field in other records in the event will be "subj=?".
> An AUDIT_MAC_TASK_CONTEXTS record is supplied when the system has
> multiple security modules that may make access decisions based
> on a subject security context.
>
> Functions are created to manage the skb list in the audit_buffer.
>
> Signed-off-by: Casey Schaufler
Besides moving the aux fns, and the whining below
Reviewed-by: John Johansen
> ---
> include/uapi/linux/audit.h | 1 +
> kernel/audit.c | 93 +++---
> 2 files changed, 88 insertions(+), 6 deletions(-)
>
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 8eda133ca4c1..af0aaccfaf57 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -143,6 +143,7 @@
> #define AUDIT_MAC_UNLBL_STCDEL 1417/* NetLabel: del a static label
> */
> #define AUDIT_MAC_CALIPSO_ADD1418/* NetLabel: add CALIPSO DOI
> entry */
> #define AUDIT_MAC_CALIPSO_DEL1419/* NetLabel: del CALIPSO DOI
> entry */
> +#define AUDIT_MAC_TASK_CONTEXTS 1420/* Multiple LSM task contexts */
>
> #define AUDIT_FIRST_KERN_ANOM_MSG 1700
> #define AUDIT_LAST_KERN_ANOM_MSG1799
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 4d44c05053b0..8ed2d717c217 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2175,8 +2175,61 @@ void audit_log_key(struct audit_buffer *ab, char *key)
> audit_log_format(ab, "(null)");
> }
>
> +/**
> + * audit_buffer_aux_new - Add an aux record buffer to the skb list
> + * @ab: audit_buffer
> + * @type: message type
> + *
> + * Aux records are allocated and added to the skb list of
> + * the "main" record. The ab->skb is reset to point to the
> + * aux record on its creation. When the aux record in complete
> + * ab->skb has to be reset to point to the "main" record.
> + * This allows the audit_log_ functions to be ignorant of
> + * which kind of record it is logging to. It also avoids adding
> + * special data for aux records.
> + *
> + * On success ab->skb will point to the new aux record.
> + * Returns 0 on success, -ENOMEM should allocation fail.
> + */
> +static int audit_buffer_aux_new(struct audit_buffer *ab, int type)
> +{
> + WARN_ON(ab->skb != skb_peek(&ab->skb_list));
> +
> + ab->skb = nlmsg_new(AUDIT_BUFSIZ, ab->gfp_mask);
> + if (!ab->skb)
> + goto err;
> + if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0))
> + goto err;
> + skb_queue_tail(&ab->skb_list, ab->skb);
> +
> + audit_log_format(ab, "audit(%llu.%03lu:%u): ",
> + (unsigned long long)ab->stamp.ctime.tv_sec,
> + ab->stamp.ctime.tv_nsec/100,
> + ab->stamp.serial);
> +
> + return 0;
> +
> +err:
> + kfree_skb(ab->skb);
> + ab->skb = skb_peek(&ab->skb_list);
> + return -ENOMEM;
> +}
> +
> +/**
> + * audit_buffer_aux_end - Switch back to the "main" record from an aux record
> + * @ab: audit_buffer
> + *
> + * Restores the "main" audit record to ab->skb.
> + */
> +static void audit_buffer_aux_end(struct audit_buffer *ab)
> +{
> + ab->skb = skb_peek(&ab->skb_list);
> +}
> +
> +
> int audit_log_task_context(struct audit_buffer *ab)
> {
> + int i;
> int error;
> struct lsmblob blob;
> struct lsmcontext context;
> @@ -2185,16 +2238,44 @@ int audit_log_task_context(struct audit_buffer *ab)
> if (!lsmblob_is_set(&blob))
> return 0;
>
> - error = security_secid_to_secctx(&blob, &context, LSMBLOB_FIRST);
> + if (!lsm_multiple_contexts()) {
> + error = security_secid_to_secctx(&blob, &context,
> + LSMBLOB_FIRST);
> + if (error) {
> + if (error != -EINVAL)
> + goto error_path;
> + return 0;
> + }
>
> - if (error) {
> - if (error != -EINVAL)
> + audit_log_format(ab, " subj=%s", context.context);
> +
Re: [PATCH v35 25/29] Audit: Allow multiple records in an audit_buffer
On 4/18/22 07:59, Casey Schaufler wrote:
> Replace the single skb pointer in an audit_buffer with
> a list of skb pointers. Add the audit_stamp information
> to the audit_buffer as there's no guarantee that there
> will be an audit_context containing the stamp associated
> with the event. At audit_log_end() time create auxiliary
> records (none are currently defined) as have been added
> to the list.
>
> Suggested-by: Paul Moore
> Signed-off-by: Casey Schaufler
I agree with Paul that audit_buffer_aux_new() and
audit_buffer_aux_end() belong in this patch
> ---
> kernel/audit.c | 62 +++---
> 1 file changed, 39 insertions(+), 23 deletions(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 6b6c089512f7..4d44c05053b0 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -197,8 +197,10 @@ static struct audit_ctl_mutex {
> * to place it on a transmit queue. Multiple audit_buffers can be in
> * use simultaneously. */
> struct audit_buffer {
> - struct sk_buff *skb; /* formatted skb ready to send */
> + struct sk_buff *skb; /* the skb for audit_log functions */
> + struct sk_buff_head skb_list; /* formatted skbs, ready to send */
> struct audit_context *ctx; /* NULL or associated context */
> + struct audit_stamp stamp; /* audit stamp for these records */
> gfp_tgfp_mask;
> };
>
> @@ -1765,10 +1767,13 @@ __setup("audit_backlog_limit=",
> audit_backlog_limit_set);
>
> static void audit_buffer_free(struct audit_buffer *ab)
> {
> + struct sk_buff *skb;
> +
> if (!ab)
> return;
>
> - kfree_skb(ab->skb);
> + while((skb = skb_dequeue(&ab->skb_list)))
> + kfree_skb(skb);
we still have and ab->skb can we have a debug check that its freed by walking
the queue?
> kmem_cache_free(audit_buffer_cache, ab);
> }
>
> @@ -1784,8 +1789,12 @@ static struct audit_buffer *audit_buffer_alloc(struct
> audit_context *ctx,
> ab->skb = nlmsg_new(AUDIT_BUFSIZ, gfp_mask);
> if (!ab->skb)
> goto err;
> +
> + skb_queue_head_init(&ab->skb_list);
> + skb_queue_tail(&ab->skb_list, ab->skb);
> +
> if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0))
> - goto err;
> + kfree_skb(ab->skb);
why is this no longer an error?
>
> ab->ctx = ctx;
> ab->gfp_mask = gfp_mask;
> @@ -1849,7 +1858,6 @@ struct audit_buffer *audit_log_start(struct
> audit_context *ctx, gfp_t gfp_mask,
>int type)
> {
> struct audit_buffer *ab;
> - struct audit_stamp stamp;
>
> if (audit_initialized != AUDIT_INITIALIZED)
> return NULL;
> @@ -1904,14 +1912,14 @@ struct audit_buffer *audit_log_start(struct
> audit_context *ctx, gfp_t gfp_mask,
> return NULL;
> }
>
> - audit_get_stamp(ab->ctx, &stamp);
> + audit_get_stamp(ab->ctx, &ab->stamp);
> /* cancel dummy context to enable supporting records */
> if (ctx)
> ctx->dummy = 0;
> audit_log_format(ab, "audit(%llu.%03lu:%u): ",
> - (unsigned long long)stamp.ctime.tv_sec,
> - stamp.ctime.tv_nsec/100,
> - stamp.serial);
> + (unsigned long long)ab->stamp.ctime.tv_sec,
> + ab->stamp.ctime.tv_nsec/100,
> + ab->stamp.serial);
>
> return ab;
> }
> @@ -2402,26 +2410,14 @@ int audit_signal_info(int sig, struct task_struct *t)
> }
>
> /**
> - * audit_log_end - end one audit record
> - * @ab: the audit_buffer
> - *
> - * We can not do a netlink send inside an irq context because it blocks (last
> - * arg, flags, is not set to MSG_DONTWAIT), so the audit buffer is placed on
> a
> - * queue and a kthread is scheduled to remove them from the queue outside the
> - * irq context. May be called in any context.
> + * __audit_log_end - enqueue one audit record
> + * @skb: the buffer to send
> */
> -void audit_log_end(struct audit_buffer *ab)
> +static void __audit_log_end(struct sk_buff *skb)
> {
> - struct sk_buff *skb;
> struct nlmsghdr *nlh;
>
> - if (!ab)
> - return;
> -
> if (audit_rate_check()) {
> - skb = ab->skb;
> - ab->skb = NULL;
> -
> /* setup the netlink header, see the comments in
>* kauditd_send_multicast_skb() for length quirks */
> nlh = nlmsg_hdr(skb);
> @@ -2432,6 +2428,26 @@ void audit_log_end(struct audit_buffer *ab)
> wake_up_interruptible(&kauditd_wait);
> } else
> audit_log_lost("rate limit exceeded");
> +}
> +
> +/**
> + * audit_log_end - end one audit record
> + * @ab: the audit_buffer
> + *
> + * We can not do a netlink send inside an irq context because it blocks (last
> + * arg, flags, is not set to MSG_DONTWAIT), so the audit buffer
Re: [PATCH v35 27/29] Audit: Add record for multiple object contexts
On 4/18/22 07:59, Casey Schaufler wrote:
> Create a new audit record AUDIT_MAC_OBJ_CONTEXTS.
> An example of the MAC_OBJ_CONTEXTS (1421) record is:
>
> type=MAC_OBJ_CONTEXTS[1421]
> msg=audit(1601152467.009:1050):
> obj_selinux=unconfined_u:object_r:user_home_t:s0
>
> When an audit event includes a AUDIT_MAC_OBJ_CONTEXTS record
> the "obj=" field in other records in the event will be "obj=?".
> An AUDIT_MAC_OBJ_CONTEXTS record is supplied when the system has
> multiple security modules that may make access decisions based
> on an object security context.
>
> Signed-off-by: Casey Schaufler
> ---
> include/linux/audit.h | 5 +++
> include/uapi/linux/audit.h | 1 +
> kernel/audit.c | 47 +++
> kernel/auditsc.c | 79 --
> 4 files changed, 77 insertions(+), 55 deletions(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 14849d5f84b4..1b05eb2dbe77 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -191,6 +191,8 @@ extern void audit_log_path_denied(int
> type,
> const char *operation);
> extern void audit_log_lost(const char *message);
>
> +extern void audit_log_object_context(struct audit_buffer *ab,
> + struct lsmblob *blob);
> extern int audit_log_task_context(struct audit_buffer *ab);
> extern void audit_log_task_info(struct audit_buffer *ab);
>
> @@ -251,6 +253,9 @@ static inline void audit_log_key(struct audit_buffer *ab,
> char *key)
> { }
> static inline void audit_log_path_denied(int type, const char *operation)
> { }
> +static inline void audit_log_object_context(struct audit_buffer *ab,
> + struct lsmblob *blob)
> +{ }
> static inline int audit_log_task_context(struct audit_buffer *ab)
> {
> return 0;
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index af0aaccfaf57..d25d76b29e3c 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -144,6 +144,7 @@
> #define AUDIT_MAC_CALIPSO_ADD1418/* NetLabel: add CALIPSO DOI
> entry */
> #define AUDIT_MAC_CALIPSO_DEL1419/* NetLabel: del CALIPSO DOI
> entry */
> #define AUDIT_MAC_TASK_CONTEXTS 1420/* Multiple LSM task contexts */
> +#define AUDIT_MAC_OBJ_CONTEXTS 1421/* Multiple LSM objext contexts
> */
>
> #define AUDIT_FIRST_KERN_ANOM_MSG 1700
> #define AUDIT_LAST_KERN_ANOM_MSG1799
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 8ed2d717c217..a8c3ec6ba60b 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2226,6 +2226,53 @@ static void audit_buffer_aux_end(struct audit_buffer
> *ab)
> ab->skb = skb_peek(&ab->skb_list);
> }
>
> +void audit_log_object_context(struct audit_buffer *ab, struct lsmblob *blob)
> +{
> + int i;
> + int error;
> + struct lsmcontext context;
> +
> + if (!lsm_multiple_contexts()) {
> + error = security_secid_to_secctx(blob, &context, LSMBLOB_FIRST);
> + if (error) {
> + if (error != -EINVAL)
> + goto error_path;
> + return;
> + }
> + audit_log_format(ab, " obj=%s", context.context);
> + security_release_secctx(&context);
> + } else {
> + audit_log_format(ab, " obj=?");
> + error = audit_buffer_aux_new(ab, AUDIT_MAC_OBJ_CONTEXTS);
> + if (error)
> + goto error_path;
> +
> + for (i = 0; i < LSMBLOB_ENTRIES; i++) {
> + if (blob->secid[i] == 0)
> + continue;
> + error = security_secid_to_secctx(blob, &context, i);
> + if (error) {
> + audit_log_format(ab, "%sobj_%s=?",
> + i ? " " : "",
> + lsm_slot_to_name(i));
> + if (error != -EINVAL)
> + audit_panic("error in
> audit_log_object_context");
> + } else {
> + audit_log_format(ab, "%sobj_%s=%s",
> + i ? " " : "",
> + lsm_slot_to_name(i),
> + context.context);
> + security_release_secctx(&context);
> + }
> + }
> +
> + audit_buffer_aux_end(ab);
> + }
> + return;
> +
> +error_path:
> + audit_panic("error in audit_log_object_context");
This moves the audit_panic around, so certain operations are not
done before the call. I am currently not sure of the implications.
Paul?
> +}
>
> int audit_log_task_context
Re: [PATCH v35 23/29] Audit: Create audit_stamp structure
On 4/26/22 11:03, Paul Moore wrote:
> On Mon, Apr 25, 2022 at 7:31 PM John Johansen
> wrote:
>> On 4/18/22 07:59, Casey Schaufler wrote:
>>> Replace the timestamp and serial number pair used in audit records
>>> with a structure containing the two elements.
>>>
>>> Signed-off-by: Casey Schaufler
>>> Acked-by: Paul Moore
>>> ---
>>> kernel/audit.c | 17 +
>>> kernel/audit.h | 12 +---
>>> kernel/auditsc.c | 22 +-
>>> 3 files changed, 27 insertions(+), 24 deletions(-)
>
> ...
>
>>> diff --git a/kernel/audit.h b/kernel/audit.h
>>> index 4af63e7dde17..260dab6e0e15 100644
>>> --- a/kernel/audit.h
>>> +++ b/kernel/audit.h
>>> @@ -108,10 +114,10 @@ struct audit_context {
>>> AUDIT_CTX_URING,/* in use by io_uring */
>>> } context;
>>> enum audit_statestate, current_state;
>>> + struct audit_stamp stamp; /* event identifier */
>>> unsigned intserial; /* serial number for record */
>>
>> shouldn't we be dropping serial from the audit_context, since we have
>> moved it into the audit_stamp?
>
> Unless we make some significant changes to audit_log_start() we still
> need to preserve a timestamp in the audit_context so that regularly
> associated audit records can share a common timestamp (which is what
> groups multiple records into a single "event").
>
sure, but the patch changes things to use ctx->stamp.serial instead of
ctx->serial. Eg. in audit_reset_context() we have
- ctx->serial = 0;
+ ctx->stamp.serial = 0;
I don't see a reason why we need both ctx->serial and ctx->stamp.serial
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v35 25/29] Audit: Allow multiple records in an audit_buffer
On 4/26/22 11:12, Paul Moore wrote:
> On Mon, Apr 25, 2022 at 9:06 PM John Johansen
> wrote:
>> On 4/18/22 07:59, Casey Schaufler wrote:
>>> Replace the single skb pointer in an audit_buffer with
>>> a list of skb pointers. Add the audit_stamp information
>>> to the audit_buffer as there's no guarantee that there
>>> will be an audit_context containing the stamp associated
>>> with the event. At audit_log_end() time create auxiliary
>>> records (none are currently defined) as have been added
>>> to the list.
>>>
>>> Suggested-by: Paul Moore
>>> Signed-off-by: Casey Schaufler
>>
>> I agree with Paul that audit_buffer_aux_new() and
>> audit_buffer_aux_end() belong in this patch
>>
>>
>>> ---
>>> kernel/audit.c | 62 +++---
>>> 1 file changed, 39 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/kernel/audit.c b/kernel/audit.c
>>> index 6b6c089512f7..4d44c05053b0 100644
>>> --- a/kernel/audit.c
>>> +++ b/kernel/audit.c
>>> @@ -197,8 +197,10 @@ static struct audit_ctl_mutex {
>>> * to place it on a transmit queue. Multiple audit_buffers can be in
>>> * use simultaneously. */
>>> struct audit_buffer {
>>> - struct sk_buff *skb; /* formatted skb ready to send */
>>> + struct sk_buff *skb; /* the skb for audit_log functions */
>>> + struct sk_buff_head skb_list; /* formatted skbs, ready to send */
>>> struct audit_context *ctx; /* NULL or associated context */
>>> + struct audit_stamp stamp; /* audit stamp for these records */
>>> gfp_tgfp_mask;
>>> };
>>>
>>> @@ -1765,10 +1767,13 @@ __setup("audit_backlog_limit=",
>>> audit_backlog_limit_set);
>>>
>>> static void audit_buffer_free(struct audit_buffer *ab)
>>> {
>>> + struct sk_buff *skb;
>>> +
>>> if (!ab)
>>> return;
>>>
>>> - kfree_skb(ab->skb);
>>> + while((skb = skb_dequeue(&ab->skb_list)))
>>> + kfree_skb(skb);
>>
>> we still have and ab->skb can we have a debug check that its freed by
>> walking the queue?
>
> By definition ab->skb is always going to point at something on the
> list, if it doesn't we are likely to have failures elsewhere. The
> structure definition is private to kernel/audit.c and the
> allocation/creation is handled by an allocator function which always
> adds the new skb to the list so I think we're okay.
>
yeah I got that eventually, though it wasn't immediately obvious
> We could add additional checks, but with audit performance already a
> hot topic I would prefer to draw the debug-check line at input coming
> from outside the audit subsystem.
>
and that is why I asked for a debug check. But its not a hard requirement
just a nice to have because I have been bitten by internal consistency
issues all to often.
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v35 26/29] Audit: Add record for multiple task security contexts
On 4/26/22 11:15, Paul Moore wrote:
> On Mon, Apr 25, 2022 at 9:08 PM John Johansen
> wrote:
>> On 4/18/22 07:59, Casey Schaufler wrote:
>>> Create a new audit record AUDIT_MAC_TASK_CONTEXTS.
>>> An example of the MAC_TASK_CONTEXTS (1420) record is:
>>>
>>> type=MAC_TASK_CONTEXTS[1420]
>>> msg=audit(1600880931.832:113)
>>> subj_apparmor=unconfined
>>> subj_smack=_
>>>
>>> When an audit event includes a AUDIT_MAC_TASK_CONTEXTS record
>>> the "subj=" field in other records in the event will be "subj=?".
>>> An AUDIT_MAC_TASK_CONTEXTS record is supplied when the system has
>>> multiple security modules that may make access decisions based
>>> on a subject security context.
>>>
>>> Functions are created to manage the skb list in the audit_buffer.
>>>
>>> Signed-off-by: Casey Schaufler
>>
>> Besides moving the aux fns, and the whining below
>> Reviewed-by: John Johansen
>
> ...
>
>>> diff --git a/kernel/audit.c b/kernel/audit.c
>>> index 4d44c05053b0..8ed2d717c217 100644
>>> --- a/kernel/audit.c
>>> +++ b/kernel/audit.c
>>> @@ -2185,16 +2238,44 @@ int audit_log_task_context(struct audit_buffer *ab)
>>> if (!lsmblob_is_set(&blob))
>>> return 0;
>>>
>>> - error = security_secid_to_secctx(&blob, &context, LSMBLOB_FIRST);
>>> + if (!lsm_multiple_contexts()) {
>>> + error = security_secid_to_secctx(&blob, &context,
>>> + LSMBLOB_FIRST);
>>> + if (error) {
>>> + if (error != -EINVAL)
>>> + goto error_path;
>>> + return 0;
>>> + }
>>>
>>> - if (error) {
>>> - if (error != -EINVAL)
>>> + audit_log_format(ab, " subj=%s", context.context);
>>> + security_release_secctx(&context);
>>> + } else {
>>> + /* Multiple LSMs provide contexts. Include an aux record. */
>>> + audit_log_format(ab, " subj=?");
>>
>> just me whining, you sure we can't just drop subj= here
>
> Have I recently given you my "the audit code is crap" speech? ;)
>
hehehe, I get it, something about glass houses and stones. the whole newline
mess in path 28/29 that I would dearly love to drop.
> I more or less answered this with my comments on the earlier patch,
> but we need to keep this around for compatibility. It will get better
> in the future.
>
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v35 27/29] Audit: Add record for multiple object contexts
On 4/26/22 11:57, Paul Moore wrote:
> On Mon, Apr 25, 2022 at 11:38 PM John Johansen
> wrote:
>> On 4/18/22 07:59, Casey Schaufler wrote:
>>> Create a new audit record AUDIT_MAC_OBJ_CONTEXTS.
>>> An example of the MAC_OBJ_CONTEXTS (1421) record is:
>>>
>>> type=MAC_OBJ_CONTEXTS[1421]
>>> msg=audit(1601152467.009:1050):
>>> obj_selinux=unconfined_u:object_r:user_home_t:s0
>>>
>>> When an audit event includes a AUDIT_MAC_OBJ_CONTEXTS record
>>> the "obj=" field in other records in the event will be "obj=?".
>>> An AUDIT_MAC_OBJ_CONTEXTS record is supplied when the system has
>>> multiple security modules that may make access decisions based
>>> on an object security context.
>>>
>>> Signed-off-by: Casey Schaufler
>>> ---
>>> include/linux/audit.h | 5 +++
>>> include/uapi/linux/audit.h | 1 +
>>> kernel/audit.c | 47 +++
>>> kernel/auditsc.c | 79 --
>>> 4 files changed, 77 insertions(+), 55 deletions(-)
>
> ...
>
>>> diff --git a/kernel/audit.c b/kernel/audit.c
>>> index 8ed2d717c217..a8c3ec6ba60b 100644
>>> --- a/kernel/audit.c
>>> +++ b/kernel/audit.c
>>> @@ -2226,6 +2226,53 @@ static void audit_buffer_aux_end(struct audit_buffer
>>> *ab)
>>> ab->skb = skb_peek(&ab->skb_list);
>>> }
>>>
>>> +void audit_log_object_context(struct audit_buffer *ab, struct lsmblob
>>> *blob)
>>> +{
>>> + int i;
>>> + int error;
>>> + struct lsmcontext context;
>>> +
>>> + if (!lsm_multiple_contexts()) {
>>> + error = security_secid_to_secctx(blob, &context,
>>> LSMBLOB_FIRST);
>>> + if (error) {
>>> + if (error != -EINVAL)
>>> + goto error_path;
>>> + return;
>>> + }
>>> + audit_log_format(ab, " obj=%s", context.context);
>>> + security_release_secctx(&context);
>>> + } else {
>>> + audit_log_format(ab, " obj=?");
>>> + error = audit_buffer_aux_new(ab, AUDIT_MAC_OBJ_CONTEXTS);
>>> + if (error)
>>> + goto error_path;
>>> +
>>> + for (i = 0; i < LSMBLOB_ENTRIES; i++) {
>>> + if (blob->secid[i] == 0)
>>> + continue;
>>> + error = security_secid_to_secctx(blob, &context, i);
>>> + if (error) {
>>> + audit_log_format(ab, "%sobj_%s=?",
>>> + i ? " " : "",
>>> + lsm_slot_to_name(i));
>>> + if (error != -EINVAL)
>>> + audit_panic("error in
>>> audit_log_object_context");
>>> + } else {
>>> + audit_log_format(ab, "%sobj_%s=%s",
>>> + i ? " " : "",
>>> + lsm_slot_to_name(i),
>>> + context.context);
>>> + security_release_secctx(&context);
>>> + }
>>> + }
>>> +
>>> + audit_buffer_aux_end(ab);
>>> + }
>>> + return;
>>> +
>>> +error_path:
>>> + audit_panic("error in audit_log_object_context");
>>
>> This moves the audit_panic around, so certain operations are not
>> done before the call. I am currently not sure of the implications.
>
> Short version: It's okay.
>
> Longer version: The audit_panic() call is either going to panic the
> kernel (NOT the default), do a pr_err(), or essentially be a no-op.
> In the case of the full blown kernel panic we don't really care, the
> system is going to die before there is any chance of this record in
> progress getting logged. In the case of a pr_err() or no-op the key
> part is making sure we leave the audit_buffer in a consistent state so
> that we preserve whatever information is already present. In the
> !lsm_multiple_
Re: [PATCH v35 08/29] LSM: Use lsmblob in security_secctx_to_secid
On 4/18/22 07:59, Casey Schaufler wrote:
> Change the security_secctx_to_secid interface to use a lsmblob
> structure in place of the single u32 secid in support of
> module stacking. Change its callers to do the same.
>
> The security module hook is unchanged, still passing back a secid.
> The infrastructure passes the correct entry from the lsmblob.
>
> Acked-by: Paul Moore
> Reviewed-by: Kees Cook
> Signed-off-by: Casey Schaufler
> Cc: net...@vger.kernel.org
> Cc: netfilter-de...@vger.kernel.org
> To: Pablo Neira Ayuso
Reviewed-by: John Johansen
> ---
> include/linux/security.h | 26 ++--
> kernel/cred.c | 4 +---
> net/netfilter/nft_meta.c | 10
> net/netfilter/xt_SECMARK.c| 7 +-
> net/netlabel/netlabel_unlabeled.c | 23 +++---
> security/security.c | 40 ++-
> 6 files changed, 85 insertions(+), 25 deletions(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 68ab0add23d3..57879f0b9f89 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -199,6 +199,27 @@ static inline bool lsmblob_equal(const struct lsmblob
> *bloba,
> extern int lsm_name_to_slot(char *name);
> extern const char *lsm_slot_to_name(int slot);
>
> +/**
> + * lsmblob_value - find the first non-zero value in an lsmblob structure.
> + * @blob: Pointer to the data
> + *
> + * This needs to be used with extreme caution, as the cases where
> + * it is appropriate are rare.
> + *
> + * Return the first secid value set in the lsmblob.
> + * There should only be one.
> + */
> +static inline u32 lsmblob_value(const struct lsmblob *blob)
> +{
> + int i;
> +
> + for (i = 0; i < LSMBLOB_ENTRIES; i++)
> + if (blob->secid[i])
> + return blob->secid[i];
> +
> + return 0;
> +}
> +
> /* These functions are in security/commoncap.c */
> extern int cap_capable(const struct cred *cred, struct user_namespace *ns,
> int cap, unsigned int opts);
> @@ -529,7 +550,8 @@ int security_setprocattr(const char *lsm, const char
> *name, void *value,
> int security_netlink_send(struct sock *sk, struct sk_buff *skb);
> int security_ismaclabel(const char *name);
> int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
> -int security_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid);
> +int security_secctx_to_secid(const char *secdata, u32 seclen,
> + struct lsmblob *blob);
> void security_release_secctx(char *secdata, u32 seclen);
> void security_inode_invalidate_secctx(struct inode *inode);
> int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
> @@ -1384,7 +1406,7 @@ static inline int security_secid_to_secctx(u32 secid,
> char **secdata, u32 *secle
>
> static inline int security_secctx_to_secid(const char *secdata,
> u32 seclen,
> -u32 *secid)
> +struct lsmblob *blob)
> {
> return -EOPNOTSUPP;
> }
> diff --git a/kernel/cred.c b/kernel/cred.c
> index 3925d38f49f4..adea727744f4 100644
> --- a/kernel/cred.c
> +++ b/kernel/cred.c
> @@ -791,14 +791,12 @@ EXPORT_SYMBOL(set_security_override);
> int set_security_override_from_ctx(struct cred *new, const char *secctx)
> {
> struct lsmblob blob;
> - u32 secid;
> int ret;
>
> - ret = security_secctx_to_secid(secctx, strlen(secctx), &secid);
> + ret = security_secctx_to_secid(secctx, strlen(secctx), &blob);
> if (ret < 0)
> return ret;
>
> - lsmblob_init(&blob, secid);
> return set_security_override(new, &blob);
> }
> EXPORT_SYMBOL(set_security_override_from_ctx);
> diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c
> index ac4859241e17..fc0028c9e33d 100644
> --- a/net/netfilter/nft_meta.c
> +++ b/net/netfilter/nft_meta.c
> @@ -860,21 +860,21 @@ static const struct nla_policy
> nft_secmark_policy[NFTA_SECMARK_MAX + 1] = {
>
> static int nft_secmark_compute_secid(struct nft_secmark *priv)
> {
> - u32 tmp_secid = 0;
> + struct lsmblob blob;
> int err;
>
> - err = security_secctx_to_secid(priv->ctx, strlen(priv->ctx),
> &tmp_secid);
> + err = security_secctx_to_secid(priv->ctx, strlen(priv->ctx), &blob);
> if (err)
> return err;
>
> - if (!tmp_secid)
> + if (!lsmblob_is_set(&blob))
> return -ENOENT;
>
>
Re: [PATCH v35 03/29] LSM: Add the lsmblob data structure.
On 4/18/22 07:59, Casey Schaufler wrote:
> When more than one security module is exporting data to
> audit and networking sub-systems a single 32 bit integer
> is no longer sufficient to represent the data. Add a
> structure to be used instead.
>
> The lsmblob structure is currently an array of
> u32 "secids". There is an entry for each of the
> security modules built into the system that would
> use secids if active. The system assigns the module
> a "slot" when it registers hooks. If modules are
> compiled in but not registered there will be unused
> slots.
>
> A new lsm_id structure, which contains the name
> of the LSM and its slot number, is created. There
> is an instance for each LSM, which assigns the name
> and passes it to the infrastructure to set the slot.
>
> The audit rules data is expanded to use an array of
> security module data rather than a single instance.
> A new structure audit_lsm_rules is defined to avoid the
> confusion which commonly accompanies the use of
> void ** parameters.
>
> Signed-off-by: Casey Schaufler
> Reviewed-by: Mickaël Salaün
small nit below, otherwise Reviewed-by: John Johansen
> ---
> include/linux/audit.h| 10 -
> include/linux/lsm_hooks.h| 12 +-
> include/linux/security.h | 75 ++---
> kernel/auditfilter.c | 23 +-
> kernel/auditsc.c | 17 +++-
> security/apparmor/lsm.c | 7 ++-
> security/bpf/hooks.c | 12 +-
> security/commoncap.c | 7 ++-
> security/landlock/cred.c | 2 +-
> security/landlock/fs.c | 2 +-
> security/landlock/ptrace.c | 2 +-
> security/landlock/setup.c| 5 +++
> security/landlock/setup.h| 1 +
> security/loadpin/loadpin.c | 8 +++-
> security/lockdown/lockdown.c | 7 ++-
> security/safesetid/lsm.c | 8 +++-
> security/security.c | 82 ++--
> security/selinux/hooks.c | 8 +++-
> security/smack/smack_lsm.c | 7 ++-
> security/tomoyo/tomoyo.c | 8 +++-
> security/yama/yama_lsm.c | 7 ++-
> 21 files changed, 254 insertions(+), 56 deletions(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index d06134ac6245..14849d5f84b4 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -11,6 +11,7 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -59,6 +60,10 @@ struct audit_krule {
> /* Flag to indicate legacy AUDIT_LOGINUID unset usage */
> #define AUDIT_LOGINUID_LEGACY0x1
>
> +struct audit_lsm_rules {
> + void*rule[LSMBLOB_ENTRIES];
> +};
> +
> struct audit_field {
> u32 type;
> union {
> @@ -66,8 +71,9 @@ struct audit_field {
> kuid_t uid;
> kgid_t gid;
> struct {
> - char*lsm_str;
> - void*lsm_rule;
> + boollsm_isset;
lsm_isset is unused in this patch, so it shouldn't be added here
> + char*lsm_str;
> + struct audit_lsm_rules lsm_rules;
> };
> };
> u32 op;
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 14d88e1312eb..fd63ae215104 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1587,6 +1587,14 @@ struct security_hook_heads {
> #undef LSM_HOOK
> } __randomize_layout;
>
> +/*
> + * Information that identifies a security module.
> + */
> +struct lsm_id {
> + const char *lsm; /* Name of the LSM */
> + int slot; /* Slot in lsmblob if one is allocated */
> +};
> +
> /*
> * Security module hook list structure.
> * For use with generic list macros for common operations.
> @@ -1595,7 +1603,7 @@ struct security_hook_list {
> struct hlist_node list;
> struct hlist_head *head;
> union security_list_options hook;
> - char*lsm;
> + struct lsm_id *lsmid;
> } __randomize_layout;
>
> /*
> @@ -1631,7 +1639,7 @@ extern struct security_hook_heads security_hook_heads;
> extern char *lsm_names;
>
> extern void security_add_hooks(struct security_hook_list *hooks, int count,
> - char *lsm);
> +struct lsm_id *lsmid);
>
> #define LSM_FLAG_LEGACY_MAJORBIT(0)
> #define
Re: [PATCH v37 00/33] LSM: Module stacking for AppArmor
On 6/27/22 17:55, Casey Schaufler wrote: This patchset provides the changes required for the AppArmor security module to stack safely with any other. There are additional changes required for SELinux and Smack to coexist. These are primarily in the networking code and will be addressed after these changes are upstream. v37: Rebase to 5.19-rc3 - Audit changes should be complete, all comments have been addressed. - Address indexing an empty array for the case where no built in security modules require data in struct lsmblob. - Fix a few checkpatch complaints. v36: Rebase to 5.19-rc1 - Yet another rework of the audit changes. Rearranging how the timestamp is managed allows auxiliary records to be generated correctly with a minimum of fuss. - In the end no LSM interface scaffolding remains. Secids have been replaced with lsmblob structures in all cases, including IMA and NetLabel. <> https://github.com/cschaufler/lsm-stacking.git#stack-5.19-rc3-v37 hey Casey, I am not finding v37 in your public github tree, the newest I see is v36 and v36-a both based on 5.19-rc1. Can you make sure v37 is pushed? thanks john -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: LSM stacking in next for 6.1?
On 8/2/22 17:56, Paul Moore wrote: On Tue, Aug 2, 2022 at 8:01 PM Casey Schaufler wrote: I would like very much to get v38 or v39 of the LSM stacking for Apparmor patch set in the LSM next branch for 6.1. The audit changes have polished up nicely and I believe that all comments on the integrity code have been addressed. The interface_lsm mechanism has been beaten to a frothy peak. There are serious binder changes, but I think they address issues beyond the needs of stacking. Changes outside these areas are pretty well limited to LSM interface improvements. The LSM stacking patches are near the very top of my list to review once the merge window clears, the io_uring fixes are in (bug fix), and SCTP is somewhat sane again (bug fix). I'm hopeful that the io_uring and SCTP stuff can be finished up in the next week or two. Since I'm the designated first stuckee now for the stacking stuff I want to go back through everything with fresh eyes, which probably isn't a bad idea since it has been a while since I looked at the full patchset from bottom to top. I can tell you that I've never been really excited about the /proc changes, and believe it or not I've been thinking about those a fair amount since James asked me to start maintaining the LSM. I don't want to get into any detail until I've had a chance to look over everything again, but just a heads-up that I'm not too excited about those bits. I am slowly working my way through the complete stack of patches again as well. I have pulled them into a test branch for Ubuntu 22.10 and the plan is to get them out into our -proposed kernels for broader testing in the next couple of weeks -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: LSM stacking in next for 6.1?
On 9/6/22 17:39, Casey Schaufler wrote: On 9/6/2022 5:10 PM, John Johansen wrote: sorry I am wa behind on this, so starting from here On 9/6/22 16:24, Paul Moore wrote: I can't currently in good conscience defend the kernel/userspace combined label interfaces as "good", especially when we have a very rare opportunity to do better. so I am going to grab and hold onto Further, I think we can add the new syscall API separately from the LSM stacking changes as they do have standalone value. what I think Paul is saying is we can move the LSM stacking patches forward by removing the combined label interface. Do you mean /proc/self/attr/interface_lsm? /proc/.../attr/context? /proc/.../attr/context is the combined label interface. /proc/self/attr/interface_lsm is an interesting question. Its not a combined label interface, instead it is a new interface that allows controlling of which LSM the task get to see on the old /proc/.../attr/* interface. Loosing it would hurt (its a useful tool and is currently necessary for the SElinux host + AppArmor in container use case) but I think if that is cost to move forward dropping it at least for now would be worth it. They won't be as useful but it would be a huge step forward, and the next step could be the syscall API. -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: LSM stacking in next for 6.1?
sorry I am wa behind on this, so starting from here On 9/6/22 16:24, Paul Moore wrote: On Fri, Sep 2, 2022 at 7:14 PM Casey Schaufler wrote: On 9/2/2022 2:30 PM, Paul Moore wrote: On Tue, Aug 2, 2022 at 8:56 PM Paul Moore wrote: On Tue, Aug 2, 2022 at 8:01 PM Casey Schaufler wrote: I would like very much to get v38 or v39 of the LSM stacking for Apparmor patch set in the LSM next branch for 6.1. The audit changes have polished up nicely and I believe that all comments on the integrity code have been addressed. The interface_lsm mechanism has been beaten to a frothy peak. There are serious binder changes, but I think they address issues beyond the needs of stacking. Changes outside these areas are pretty well limited to LSM interface improvements. The LSM stacking patches are near the very top of my list to review once the merge window clears, the io_uring fixes are in (bug fix), and SCTP is somewhat sane again (bug fix). I'm hopeful that the io_uring and SCTP stuff can be finished up in the next week or two. Since I'm the designated first stuckee now for the stacking stuff I want to go back through everything with fresh eyes, which probably isn't a bad idea since it has been a while since I looked at the full patchset from bottom to top. I can tell you that I've never been really excited about the /proc changes, and believe it or not I've been thinking about those a fair amount since James asked me to start maintaining the LSM. I don't want to get into any detail until I've had a chance to look over everything again, but just a heads-up that I'm not too excited about those bits. As I mentioned above, I don't really like the stuff that one has to do to support LSM stacking on the existing /proc interfaces, the "label1\0label2\labelN\0" hack is probably the best (only?) option we have for retrofitting multiple LSMs into those interfaces and I think we can all agree it's not a great API. Considering that applications that wish to become simultaneous multi-LSM aware are going to need modification anyway, let's take a step back and see if we can do this with a more sensible API. This is a compound problem. Some applications, including systemd and dbus, will require modification to completely support multiple concurrent LSMs in the long term. This will certainly be the case should someone be wild and crazy enough to use Smack and SELinux together. Even with the (Smack or SELinux) and AppArmor case the ps(1) command should be educated about the possibility of multiple "current" values. However, in a container world, where an Android container can run on an Ubuntu system, the presence of AppArmor on the base system is completely uninteresting to the SELinux aware applications in the container. This is a real use case. If you are running AppArmor on the host system and SELinux in a container you are likely going to have some *very* bizarre behavior as the SELinux policy you load in the container will apply to the entire system, including processes which started *before* the SELinux policy was loaded. While I understand the point you are trying to make, I don't believe the example you chose is going to work without a lot of other changes. correct but the reverse does work. Where SELinux is running on the host and AppArmor in the container. Obviously that is the simplified version AppArmor is running on the host too but unconfined, has setup a policy namespace for the container and is self stacking (bounding). Regardless of the above example, I want to be clear that I'm not suggesting changes to the /proc interfaces. Existing LSM aware applications that use procfs for information would continue to work as expected, it would just be the simul-multi-LSM aware applications that would need to transition to the new syscall API to get all of the LSM labels. I can live with that. AppArmor and Smack have already landed private interfaces and prefer them over the shared interface if they are available. I think it's time to think about a proper set of LSM syscalls. At the very least we need a liblsm that preforms a number of useful functions, like identifying what security modules are available, validating "contexts", fetching "contexts" from files and processes and that sort of thing. Whether it is built on syscalls or /proc and getxattr() is a matter of debate and taste. Why is it a forgone conclusion that a library would be necessary for basic operations? If the kernel/userspace API is sane to begin with we could probably either significantly reduce or eliminate the need for additional libraries. I really want us to attempt to come up with a decent kernel/userspace API to begin with as opposed to using the excuse of a userspace library to hide API sins that never should have been committed. I don't think its a foregone conclusion, its just that it has been discussed several times, and all the interfaces have been nasty and prebaked userspace code would be really nice to have. T
Re: LSM stacking in next for 6.1?
On 9/7/22 09:41, Casey Schaufler wrote: On 9/7/2022 7:41 AM, Paul Moore wrote: On Tue, Sep 6, 2022 at 8:10 PM John Johansen wrote: On 9/6/22 16:24, Paul Moore wrote: On Fri, Sep 2, 2022 at 7:14 PM Casey Schaufler wrote: On 9/2/2022 2:30 PM, Paul Moore wrote: On Tue, Aug 2, 2022 at 8:56 PM Paul Moore wrote: On Tue, Aug 2, 2022 at 8:01 PM Casey Schaufler wrote: .. If you are running AppArmor on the host system and SELinux in a container you are likely going to have some *very* bizarre behavior as the SELinux policy you load in the container will apply to the entire system, including processes which started *before* the SELinux policy was loaded. While I understand the point you are trying to make, I don't believe the example you chose is going to work without a lot of other changes. correct but the reverse does work ... Sure, that doesn't surprise me, but that isn't the example Casey brought up. I said that I'm not sure how they go about doing Android on Ubuntu. I brought it up because I've seen it. LSM stacking for that use case is necessary but insufficient. At a minimum SELinux would need bounding, and realistically some other gymnastics. I don't hold out hope of it happening soon if ever. I have told the anbox people such. At the momement anbox disables SELinux when run in a container https://github.com/anbox/platform_system_core/commit/71907fc5e7833866be6ae3c120c602974edf8322 there has been work on using a VM instead so that they can have SELinux but I am not current on how/when that is used. Where Canonical is interested in LSM stacking is running snaps with apparmor confinement on top of SELinux distros. I know snaps aren't popular but it is a much more realistic and attainable use case for LSM stacking. -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: LSM stacking in next for 6.1?
On 9/7/22 16:53, Casey Schaufler wrote: On 9/7/2022 4:27 PM, Paul Moore wrote: On Wed, Sep 7, 2022 at 12:42 PM Casey Schaufler wrote: On 9/7/2022 7:41 AM, Paul Moore wrote: On Tue, Sep 6, 2022 at 8:10 PM John Johansen wrote: On 9/6/22 16:24, Paul Moore wrote: On Fri, Sep 2, 2022 at 7:14 PM Casey Schaufler wrote: On 9/2/2022 2:30 PM, Paul Moore wrote: On Tue, Aug 2, 2022 at 8:56 PM Paul Moore wrote: On Tue, Aug 2, 2022 at 8:01 PM Casey Schaufler wrote: .. If you are running AppArmor on the host system and SELinux in a container you are likely going to have some *very* bizarre behavior as the SELinux policy you load in the container will apply to the entire system, including processes which started *before* the SELinux policy was loaded. While I understand the point you are trying to make, I don't believe the example you chose is going to work without a lot of other changes. correct but the reverse does work ... Sure, that doesn't surprise me, but that isn't the example Casey brought up. I said that I'm not sure how they go about doing Android on Ubuntu. I brought it up because I've seen it. Addressed in the other portion of this thread, but the quick response here is: No, you were mistaken, that was not proper Android, SELinux was disabled. I'm sympathetic that this work has been going on for some time, but that is no reason to push through a bad design; look at the ACKs/reviews on the combined-label patches vs the others in the series, that's a pretty good indication that no one is really excited about that design. The kernel developers aren't the consumers of these APIs. There has been considerable feedback from system application developers on the interfaces. This included dbus and systemd. Kernel developers aren't interested in these APIs because they don't care one way or the other. That, and as you are painfully aware, some of them are really busy on their own projects. Yes, everyone is busy yet they found time to ACK/review the other patches in the patchset. I'm not buying the "busy" argument here. Yes, you are also correct in that the kernel devs are not likely to be the main consumers of any kernel/userspace API, but we do have to support these APIs and find ways to handle the inevitable misuse and abuse of these APIs. Further, while I do believe that you've talked to some application developers about the current proposed API, I'm reasonably confident that it isn't the only API they would be happy supporting in their applications. As far as kernel developers not being interested in these APIs, I think the recent conversation in this thread proves the exact opposite ;) Am I really happy with the "hideous" format? Yeah, because it makes the end user happy. Am I happy with interface_lsm? Other than the name, which was the result of feedback, yes, because it in the simplest way to accomplish the goal. I am curious about what you think is "bad" about the current design. The interfaces aren't exciting. They don't need to be. I don't even know what an exciting interface would look like ... ? io_uring? :) I just want an interface that is clearly defined, has reasonable capacity to be extended in the future as needed, and is easy(ish) to use and support over extended periods of time (both from a kernel and userspace perspective). The "smack_label\0apparmor_label\0selinux_label\0future_lsm_label\0" string interface is none of these things. In this we disagree It is not clearly defined as it requires other interfaces to associate the labels with the correct LSMs. The label follows the lsm name directly. What other association is required? its a serialized message format, with all the data in the message instead of pointer to external memory. If you want nicer to handle you wrap a lib around it, this is pretty common. That is why I don't see it as that different from the syscall. It has no provision for extension beyond adding a new LSM. I grant that. But the purpose of the format is to present LSM/context pairs. What other information would make sense? We could add an "aux" field, but that seems somewhat arbitrary. or you know a header that gives potential future, also potentially a count, ... The ease-of-use quality is a bit subjective, but it does need another interface to use properly and it requires string parsing which history has shown to be an issue time and time again (although it is relatively simple here). There was a lot of discussion regarding that. My proposed apparmor="unconfined",smack="User" format was panned for those same reasons. The nil byte format has been used elsewhere and suggested for that reason. At this level the lib provides the ease of use, but I think that is what we are going to need with a syscall too, as marshalling variable number/length
Re: LSM stacking in next for 6.1?
On 9/8/22 11:05, Casey Schaufler wrote: On 9/7/2022 8:57 PM, Paul Moore wrote: On Wed, Sep 7, 2022 at 7:53 PM Casey Schaufler wrote: On 9/7/2022 4:27 PM, Paul Moore wrote: .. I just want an interface that is clearly defined, has reasonable capacity to be extended in the future as needed, and is easy(ish) to use and support over extended periods of time (both from a kernel and userspace perspective). The "smack_label\0apparmor_label\0selinux_label\0future_lsm_label\0" string interface is none of these things. That wasn't the proposal. The proposal was "smack\0smack_label\0apparmor\0apparmor_label\0future_lsm\0future_lsm_label\0" In this we disagree I think we can both agree that there is a subjective aspect to this argument and it may be that we never reach agreement on the "best" approach, if there even is such a thing. What I am trying to do here is describe a path that would allow me to be more comfortable merging the LSM stacking functionality; I don't think you've had a clearly defined path, of any sort, towards getting these patches merged, which is a problem and I suspect the source of some of the frustration. My comments, as objectionable as you may find them to be, are intended to help you move forward with these patches. OK. Let's get'er done. It is not clearly defined as it requires other interfaces to associate the labels with the correct LSMs. The label follows the lsm name directly. What other association is required? You need to know the order of the LSMs in order to interpret/de-serialize the string. That's true for the string you included, but not for what I had actually proposed. The ease-of-use quality is a bit subjective, but it does need another interface to use properly and it requires string parsing which history has shown to be an issue time and time again (although it is relatively simple here). There was a lot of discussion regarding that. My proposed apparmor="unconfined",smack="User" format was panned for those same reasons. The nil byte format has been used elsewhere and suggested for that reason. Based on what I recall from those discussions, it was my impression the nil byte label delimiter was suggested simply because no one was entertaining the idea of using something other than the existing procfs interface. It is my opinion that we've taken that interface about as far as it can go, and while it needs to stay intact for compatibility reasons, the LSM stacking functionality should move to a different API that is better suited for it. It's going to raise its ugly head again with SO_PEERCONTEXT for the SELinux+Smack case. But we can cross that bridge when we come to it. AppArmor too, I am working on revising the out of tree af_unix mediation Once again, the syscall example I tossed out was a quick strawman, but it had clearly separated and defined labels conveyed in data structures that didn't require string parsing to find the label associated with an LSM. True, but it uses pointers internally and you can't do that in memory you're sending up from the system. What comes from the syscall has to look something like the nil byte format. The strawman would have to do the same sort of parsing in userspace that you are objecting to. Then we change the strawman. That's pretty much the definition of a strawman example, it's something you toss out as starting point for discussion and future improvements. If it was something much more fully developed I would have submitted a patch sheesh. Fair enough. In case it helps spur your imagination, here is a revised strawman: /** * struct lsm_ctx - LSM context * @id: the LSM id number, see LSM_ID_XXX A LSM ID hard coded in a kernel header makes it harder to develop new security modules. The security module can't be self contained. I say that, but I acknowledge that I've done the same kind of thing with the definition of the struct lsmblob. That isn't part of an external API however. It may also interfere with Tetsuo's long standing request that we don't do things that prevent the possibility of loadable security modules at some point in the future. I will also mention the out-of-tree security module objection, not because I care, but because someone most likely will bring it up. On the other hand, there's no great way to include two variable length strings in a structure like this. So unless we adopt something as ugly as the nil byte scheme this is supposed to replace I expect we're stuck with an LSM ID. well at a minimum we shouldn't export the kernel internal LSM_ID if its exposed to userspace it needs to be something that can live with for a long time - Fixed length strings, which really are just a long LSM ID, Say 8 bytes. Can still even look human readable. For most* LSMs this could just be their name. * only safesetid and capability don't fit atm - and certainly uglier, for variable length use an index for one of the variable length strings, with an em
Re: LSM stacking in next for 6.1?
On 9/14/22 06:57, Tetsuo Handa wrote: On 2022/09/13 23:45, Casey Schaufler wrote: . A security module that manages loadable LSM modules cannot give us a good answer if there is a kernel config option to disable the manager security module. The community that is absolutely opposed to loadable modules will disagree. Who are members of that community? Hiding security_hook_heads from /proc/kallsyms has no value from security perspective, for malicious loadable kernel modules can calculate the address of security_hook_heads based on addresses of relevant functions and byte-code in the relevant functions. Keeping __lsm_ro_after_init might have a little value, but at the same time it might make kernel less secure (or more prone to memory corruption) due to the need to pass rodata=0 kernel command line option when a loadable module LSM is loaded. The kernel config option and distribution's policy are preventing users from using non-builtin LSMs in distributor's kernels. It is a trivial task to make TOMOYO work in distributor's kernels if above-mentioned changes are accepted. You should be able to use TOMOYO as a built-in along side other security modules today. Aside from getting the distribution to include it in their kernel configuration, which is admittedly no mean feat, and getting any user-space you need included, you should already have what you need. That's a chicken-and-egg problem. Yes, we can use TOMOYO as a built-in along side other security modules for _user-built_ kernels. But no, we can't use TOMOYO for _distributor-built_ kernels (namely, Fedora/CentOS Stream/RHEL kernels). https://bugzilla.redhat.com/show_bug.cgi?id=542986 Ten years ago they said "Don't want to, aren't going to". Sadly, I doubt there would be a different attitude today. The decision to support a security module in a distribution is serious. I can definitely see how Redhat would have their hands full supporting SELinux. Please distinguish the difference between "enable" and "support" at https://bugzilla.redhat.com/show_bug.cgi?id=542986#c7 . (By the way, I hate the word "support", for nobody can share agreed definition.) "enable" is something like "available", "allow to exist". "support" is something like "guaranteed", "provide efforts for fixing bugs". However, in the Red Hat's world, "enable" == "support". The kernel config options enabled by Red Hat is supported by Red Hat, and the kernel config options Red Hat cannot support cannot be enabled by Red Hat. On the contrary, in the vanilla kernel's world, the in-tree version of TOMOYO cannot be built as a loadable module LSM. And it is impossible to built TOMOYO as a loadable module LSM (so that TOMOYO can be used without the "support" by Red Hat). As a result, users cannot try LSMs (either in-tree or out-of-tree) other than SELinux. The negative effect is not limited to TOMOYO. Like Paul Moore said However, I will caution that it is becoming increasingly difficult for people to find time to review potential new LSMs so it may a while to attract sufficient comments and feedback. , being unable to legally use loadable LSMs deprives of chances to develop/try new LSMs, and makes LSM interface more and more unattractive. The consequence would be "The LSM interface is dead. We will give up implementing as LSMs." It is exactly "only in-tree and supported by distributors is correct" crap. for some users, but having a very well defined support surface also has its place. From a distro POV support is expensive and its amazing what users will do and try to hide while trying to get support. Personally I prefer splitting enable and support but there are situations where that isn't even allowed (some certifications). So I can understand where they are coming from. It just sucks for the users and projects that aren't "supported". I don't like closed-source kernel modules that rewrite syscall tables (e.g. used by AntiVirus), for I can't analyze problems when something went wrong. Does anyone? If LSMs were available to open-source out-of-tree kernel modules, this situation could be improved. you are more optimistic than I am. What makes you think a distro like RH will enable loading out-of-tree kernel modules if they aren't enabling TOMOYO that is already in the kernel. If loadable LSM modules are allowed, there will likely be a kernel config to disable them and there will definitely be an interface that allows blocking them. So whether via config option or run time control I don't see RH allowing them. I think that syzbot is the most aggressive tester of TOMOYO security module. But how many bugs did syzbot found in TOMOYO? How many distributors that enabled TOMOYO in their kernels got bug reports regarding TOMOYO? There might be reports like "When do you start providing ready-made policy configurations?", but what Josh Boyer worried at https://bugzilla.redhat.com/show_bug.cgi?id=542986#c8 Simply put, we do not have the time to
Re: LSM stacking in next for 6.1?
On 9/15/22 07:27, Tetsuo Handa wrote: On 2022/09/15 0:50, Casey Schaufler wrote: On 9/14/2022 6:57 AM, Tetsuo Handa wrote: Please distinguish the difference between "enable" and "support" at https://bugzilla.redhat.com/show_bug.cgi?id=542986#c7 . (By the way, I hate the word "support", for nobody can share agreed definition.) "enable" is something like "available", "allow to exist". "support" is something like "guaranteed", "provide efforts for fixing bugs". However, in the Red Hat's world, "enable" == "support". The kernel config options enabled by Red Hat is supported by Red Hat, and the kernel config options Red Hat cannot support cannot be enabled by Red Hat. The "enable" == "support" model in consistent with the expectations of paying customers. Regarding CONFIG_MODULES=y, "Vendor-A enables module-A" == "Vendor-A provides support for module-A" and "Vendor-B enables module-B" == "Vendor-B provides support for module-B". Regarding CONFIG_SECURITY=y (namely in the RH world), "Distributor-A enables LSM-A" == "Distributor-A provides support for LSM-A". However, "Distributor-A does not enable LSM-B" == "Some vendor is impossible to provide support for LSM-B". "Distributor-A does not enable module-B" == "Distributor-A is not responsible for providing support for module-B" and "Vendor-B enables LSM-B" == "Vendor-B provides support for LSM-B" are what I expect. Current LSM interface does not allow LSM-B to exist in Distributor-A's systems. The "enable" == "support" model should be allowed for LSM interface as well. What a strange asymmetry rule! On the contrary, in the vanilla kernel's world, the in-tree version of TOMOYO cannot be built as a loadable module LSM. And it is impossible to built TOMOYO as a loadable module LSM (so that TOMOYO can be used without the "support" by Red Hat). As a result, users cannot try LSMs (either in-tree or out-of-tree) other than SELinux. That is correct. Redhat has chosen to support only SELinux. If you want TOMOYO to be enabled in a distribution you need to sell the value to a distributor (really, really hard) Or (not recommended) become one yourself. What I'm asking is "allow non-SELinux to exist in RH systems". I'm not asking RH to "provide efforts for fixing non-SELinux". Being able to build in-tree version of TOMOYO via "make M=security/tomoyo" releases RH from the "enable" == "support" spell. I am sympathetic, I want this too. But RH choices are not a technical problem, they could easily enable and not support other LSMs (eg. Ubuntu does). It is a political problem and I don't see loadable LSMs changing this. -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
[RFC] include audit type in audit message when using printk
Currently audit drops the audit type when an audit message goes through
printk instead of the audit deamon. This is a minor annoyance in
that the audit type is no longer part of the message and the information
the audit type conveys needs to be carried in, or derived from the
message data.
The attached patch includes the type number as part of the printk.
Admittedly it isn't the type name that the audit deamon provides but I
think this is better than dropping the type completely.
---
kernel/audit.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1354,7 +1354,8 @@ void audit_log_end(struct audit_buffer *
ab->skb = NULL;
wake_up_interruptible(&kauditd_wait);
} else {
- printk(KERN_NOTICE "%s\n", ab->skb->data +
NLMSG_SPACE(0));
+ struct nlmsghdr *nlh = nlmsg_hdr(ab->skb);
+ printk(KERN_NOTICE "type=%d %s\n", nlh->nlmsg_type,
ab->skb->data + NLMSG_SPACE(0));
}
}
audit_buffer_free(ab);
pgpZ6xpS9Uk61.pgp
Description: PGP signature
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH 03/3] apparmor: remove parent task info from audit logging
The reporting of the parent task info is a vestage from old versions of
apparmor. The need for this information was removed by unique null-
profiles before apparmor was upstreamed so remove this info from logging.
Signed-off-by: John Johansen
---
security/apparmor/audit.c | 6 --
security/apparmor/include/audit.h | 1 -
2 files changed, 7 deletions(-)
diff --git a/security/apparmor/audit.c b/security/apparmor/audit.c
index e32c448..89c7865 100644
--- a/security/apparmor/audit.c
+++ b/security/apparmor/audit.c
@@ -111,7 +111,6 @@ static const char *const aa_audit_type[] = {
static void audit_pre(struct audit_buffer *ab, void *ca)
{
struct common_audit_data *sa = ca;
- struct task_struct *tsk = sa->u.tsk ? sa->u.tsk : current;
if (aa_g_audit_header) {
audit_log_format(ab, "apparmor=");
@@ -132,11 +131,6 @@ static void audit_pre(struct audit_buffer *ab, void *ca)
if (sa->aad->profile) {
struct aa_profile *profile = sa->aad->profile;
- pid_t pid;
- rcu_read_lock();
- pid = rcu_dereference(tsk->real_parent)->pid;
- rcu_read_unlock();
- audit_log_format(ab, " parent=%d", pid);
if (profile->ns != root_ns) {
audit_log_format(ab, " namespace=");
audit_log_untrustedstring(ab, profile->ns->base.hname);
diff --git a/security/apparmor/include/audit.h
b/security/apparmor/include/audit.h
index 69d8cae..cf65443 100644
--- a/security/apparmor/include/audit.h
+++ b/security/apparmor/include/audit.h
@@ -110,7 +110,6 @@ struct apparmor_audit_data {
void *profile;
const char *name;
const char *info;
- struct task_struct *tsk;
union {
void *target;
struct {
--
1.8.3.2
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH 1/3] apparmor: fix capability to not use the current task, during reporting
Mediation is based off of the cred but auditing includes the current
task which may not be related to the actual request.
Signed-off-by: John Johansen
---
security/apparmor/capability.c | 15 +--
security/apparmor/domain.c | 2 +-
security/apparmor/include/capability.h | 5 ++---
security/apparmor/include/ipc.h| 4 ++--
security/apparmor/ipc.c| 9 -
security/apparmor/lsm.c| 2 +-
6 files changed, 15 insertions(+), 22 deletions(-)
diff --git a/security/apparmor/capability.c b/security/apparmor/capability.c
index 887a5e9..98a73eb 100644
--- a/security/apparmor/capability.c
+++ b/security/apparmor/capability.c
@@ -48,8 +48,7 @@ static void audit_cb(struct audit_buffer *ab, void *va)
/**
* audit_caps - audit a capability
- * @profile: profile confining task (NOT NULL)
- * @task: task capability test was performed against (NOT NULL)
+ * @profile: profile being tested for confinement (NOT NULL)
* @cap: capability tested
* @error: error code returned by test
*
@@ -58,8 +57,7 @@ static void audit_cb(struct audit_buffer *ab, void *va)
*
* Returns: 0 or sa->error on success, error code on failure
*/
-static int audit_caps(struct aa_profile *profile, struct task_struct *task,
- int cap, int error)
+static int audit_caps(struct aa_profile *profile, int cap, int error)
{
struct audit_cache *ent;
int type = AUDIT_APPARMOR_AUTO;
@@ -68,7 +66,6 @@ static int audit_caps(struct aa_profile *profile, struct
task_struct *task,
sa.type = LSM_AUDIT_DATA_CAP;
sa.aad = &aad;
sa.u.cap = cap;
- sa.aad->tsk = task;
sa.aad->op = OP_CAPABLE;
sa.aad->error = error;
@@ -119,8 +116,7 @@ static int profile_capable(struct aa_profile *profile, int
cap)
/**
* aa_capable - test permission to use capability
- * @task: task doing capability test against (NOT NULL)
- * @profile: profile confining @task (NOT NULL)
+ * @profile: profile being tested against (NOT NULL)
* @cap: capability to be tested
* @audit: whether an audit record should be generated
*
@@ -128,8 +124,7 @@ static int profile_capable(struct aa_profile *profile, int
cap)
*
* Returns: 0 on success, or else an error code.
*/
-int aa_capable(struct task_struct *task, struct aa_profile *profile, int cap,
- int audit)
+int aa_capable(struct aa_profile *profile, int cap, int audit)
{
int error = profile_capable(profile, cap);
@@ -139,5 +134,5 @@ int aa_capable(struct task_struct *task, struct aa_profile
*profile, int cap,
return error;
}
- return audit_caps(profile, task, cap, error);
+ return audit_caps(profile, cap, error);
}
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 01b7bd6..f037c57 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -75,7 +75,7 @@ static int may_change_ptraced_domain(struct task_struct *task,
if (!tracer || unconfined(tracerp))
goto out;
- error = aa_may_ptrace(tracer, tracerp, to_profile, PTRACE_MODE_ATTACH);
+ error = aa_may_ptrace(tracerp, to_profile, PTRACE_MODE_ATTACH);
out:
rcu_read_unlock();
diff --git a/security/apparmor/include/capability.h
b/security/apparmor/include/capability.h
index c24d295..e4fea19 100644
--- a/security/apparmor/include/capability.h
+++ b/security/apparmor/include/capability.h
@@ -4,7 +4,7 @@
* This file contains AppArmor capability mediation definitions.
*
* Copyright (C) 1998-2008 Novell/SUSE
- * Copyright 2009-2010 Canonical Ltd.
+ * Copyright 2009-2013 Canonical Ltd.
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License as
@@ -34,8 +34,7 @@ struct aa_caps {
kernel_cap_t extended;
};
-int aa_capable(struct task_struct *task, struct aa_profile *profile, int cap,
- int audit);
+int aa_capable(struct aa_profile *profile, int cap, int audit);
static inline void aa_free_cap_rules(struct aa_caps *caps)
{
diff --git a/security/apparmor/include/ipc.h b/security/apparmor/include/ipc.h
index aeda0fb..288ca76 100644
--- a/security/apparmor/include/ipc.h
+++ b/security/apparmor/include/ipc.h
@@ -19,8 +19,8 @@
struct aa_profile;
-int aa_may_ptrace(struct task_struct *tracer_task, struct aa_profile *tracer,
- struct aa_profile *tracee, unsigned int mode);
+int aa_may_ptrace(struct aa_profile *tracer, struct aa_profile *tracee,
+ unsigned int mode);
int aa_ptrace(struct task_struct *tracer, struct task_struct *tracee,
unsigned int mode);
diff --git a/security/apparmor/ipc.c b/security/apparmor/ipc.c
index c51d226..777ac1c 100644
--- a/security/apparmor/ipc.c
+++ b/security/apparmor/ipc.c
@@ -54,15 +54,14 @@ static int aa_audit_ptrace(struct aa_profile *profile,
/**
* aa_may_ptrace - tes
Re: [PATCH 03/12] pid: get ppid pid_t of task in init_pid_ns safely
On 08/30/2013 12:56 PM, Richard Guy Briggs wrote:
> On Tue, Aug 27, 2013 at 07:21:55PM +0200, Oleg Nesterov wrote:
>> On 08/20, Richard Guy Briggs wrote:
>>>
>>> Added the functions
>>> task_ppid()
>>> task_ppid_nr_ns()
>>> task_ppid_nr_init_ns()
>>> to safely abstract the lookup of the PPID
>>
>> but it is not safe.
>>
>>> +static inline struct pid *task_ppid(struct task_struct *task)
>>> +{
>>> + return task_tgid(rcu_dereference(current->real_parent));
>> ^^^
>> task?
>
> Yup, thanks for those two catches.
>
>>> + rcu_read_unlock();
>>
>> And why this is safe?
>>
>> rcu_read_lock() can't help if tsk was already dead _before_ it takes
>> the rcu lock. ->real_parent can point the already freed/reused/unmapped
>> memory.
>
> Does it not bump a refcount if it is holding a pointer to it? So the
> parent task might be dead, but it won't cause a pointer dereference
> issue.
>
>> This is safe if, for example, the caller alredy holds rcu_read_lock()
>> and tsk was found by find_task_by*(), or tsk is current.
>
> Fair enough, I'll have a more careful look at this. Thanks.
>
> Most of the instances are current, but the one called from apparmour is
> stored. I've just learned that this is bad and someone else just chimed
> in that they have a patch to remove it...
the apparmor case isn't actually stored long term. The stored task will be
a parameter that was passed into an lsm hook and the buffer that it is
stored in dies before the hook is done. Its temporarily stored in the
struct so that it can be passed into the lsm_audit fn, and printed into an
allocated audit buffer. The text version in the audit buffer is what will
exist beyond the hook.
There are three patches, I'll reply them below once I have finished rebasing
them to apply to the current tree instead of my dev tree.
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH 2/3] apparmor: remove tsk field from the apparmor_audit_struct
Now that aa_capabile no longer sets the task field it can be removed
and the lsm_audit version of the field can be used.
Signed-off-by: John Johansen
---
security/apparmor/audit.c | 10 ++
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/security/apparmor/audit.c b/security/apparmor/audit.c
index 031d2d9..e32c448 100644
--- a/security/apparmor/audit.c
+++ b/security/apparmor/audit.c
@@ -111,7 +111,7 @@ static const char *const aa_audit_type[] = {
static void audit_pre(struct audit_buffer *ab, void *ca)
{
struct common_audit_data *sa = ca;
- struct task_struct *tsk = sa->aad->tsk ? sa->aad->tsk : current;
+ struct task_struct *tsk = sa->u.tsk ? sa->u.tsk : current;
if (aa_g_audit_header) {
audit_log_format(ab, "apparmor=");
@@ -149,12 +149,6 @@ static void audit_pre(struct audit_buffer *ab, void *ca)
audit_log_format(ab, " name=");
audit_log_untrustedstring(ab, sa->aad->name);
}
-
- if (sa->aad->tsk) {
- audit_log_format(ab, " pid=%d comm=", tsk->pid);
- audit_log_untrustedstring(ab, tsk->comm);
- }
-
}
/**
@@ -212,7 +206,7 @@ int aa_audit(int type, struct aa_profile *profile, gfp_t
gfp,
if (sa->aad->type == AUDIT_APPARMOR_KILL)
(void)send_sig_info(SIGKILL, NULL,
- sa->aad->tsk ? sa->aad->tsk : current);
+ sa->u.tsk ? sa->u.tsk : current);
if (sa->aad->type == AUDIT_APPARMOR_ALLOWED)
return complain_error(sa->aad->error);
--
1.8.3.2
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH 03/12] pid: get ppid pid_t of task in init_pid_ns safely
On 12/11/2013 06:47 AM, Richard Guy Briggs wrote: > On Tue, Sep 03, 2013 at 02:31:59PM -0400, Richard Guy Briggs wrote: >> On Fri, Aug 30, 2013 at 01:37:09PM -0700, John Johansen wrote: >>> On 08/30/2013 12:56 PM, Richard Guy Briggs wrote: >>>> On Tue, Aug 27, 2013 at 07:21:55PM +0200, Oleg Nesterov wrote: >>>>> On 08/20, Richard Guy Briggs wrote: >>>> Most of the instances are current, but the one called from apparmour is >>>> stored. I've just learned that this is bad and someone else just chimed >>>> in that they have a patch to remove it... >>> >>> the apparmor case isn't actually stored long term. The stored task will be >>> a parameter that was passed into an lsm hook and the buffer that it is >>> stored in dies before the hook is done. Its temporarily stored in the >>> struct so that it can be passed into the lsm_audit fn, and printed into an >>> allocated audit buffer. The text version in the audit buffer is what will >>> exist beyond the hook. >>> >>> There are three patches, I'll reply them below once I have finished rebasing >>> them to apply to the current tree instead of my dev tree. >> >> John, thanks for this context and fix. That helps simplify things. > > John, What's the status of this set of 3 patches? I don't see them > upstream. > they are part of the security tree merge in 3.13 51775fe apparmor: remove the "task" arg from may_change_ptraced_domain() 4a7fc30 apparmor: remove parent task info from audit logging 61e3fb8 apparmor: remove tsk field from the apparmor_audit_struct dd0c6e8 apparmor: fix capability to not use the current task, during reporting -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v3 21/24] Audit: Store LSM audit information in an lsmblob
On 6/24/19 6:46 PM, Paul Moore wrote:
> On Mon, Jun 24, 2019 at 9:01 PM Casey Schaufler
> wrote:
>> On 6/24/2019 2:33 PM, John Johansen wrote:
>>> On 6/21/19 11:52 AM, Casey Schaufler wrote:
>>>> Change the audit code to store full lsmblob data instead of
>>>> a single u32 secid. This allows for multiple security modules
>>>> to use the audit system at the same time. It also allows the
>>>> removal of scaffolding code that was included during the
>>>> revision of LSM interfaces.
>>>>
>>>> Signed-off-by: Casey Schaufler
>>> I know Kees raised this too, but I haven't seen a reply
>>>
>>> Eric (Paul is already CCed): I have directly added you because of
>>> the question below.
>>>
>>> In summary there isn't necessarily a single secid any more, and
>>> we need to know whether dropping the logging of the secid or
>>> logging all secids is the correct action.
>>
>> It is to be considered that this is an error case. If
>> everything is working normally you should have produced
>> a secctx previously, which you'll have included in the
>> audit record. Including the secid in the record ought to
>> be pointless, as the secid is strictly an internal token
>> with no meaning outside the running kernel. You are providing
>> no security relevant information by providing the secid.
>> I will grant the possibility that the secid might be useful
>> in debugging, but for that a pr_warn is more appropriate
>> than a field in the audit record.
>
> FWIW, this probably should have been CC'd to the audit list.
>
hrmm indeed, sorry
> I agree that this is an error case (security_secid_to_secctx() failed
> to resolve the secid) and further that logging the secid, or a
> collection of secids, has little value the way things currently work.
> Since secids are a private kernel implementation detail, we don't
> really display them outside the context of the kernel, including in
> the audit logs. Recording a secid in this case doesn't provide
> anything meaningful since secids aren't recorded in the audit record
> stream, only the secctxs, and there is no "magic decoder ring" to go
> between the two in the audit logs, or anywhere else in userspace for
> that matter.
>
Okay, thanks. Casey I am good with just a pr_warn here. I just didn't
have context of why it was going to the audit_log and didn't want
to change that without some more input.
>>>> ---
>>>> kernel/audit.h | 6 +++---
>>>> kernel/auditsc.c | 38 +++---
>>>> 2 files changed, 14 insertions(+), 30 deletions(-)
>
> ...
>
>>>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>>>> index 0478680cd0a8..d3ad13f11788 100644
>>>> --- a/kernel/auditsc.c
>>>> +++ b/kernel/auditsc.c
>>>> @@ -1187,21 +1184,18 @@ static void show_special(struct audit_context
>>>> *context, int *call_panic)
>>>> context->socketcall.args[i]);
>>>> break; }
>>>> case AUDIT_IPC: {
>>>> -u32 osid = context->ipc.osid;
>>>> +struct lsmblob *olsm = &context->ipc.olsm;
>>>>
>>>> audit_log_format(ab, "ouid=%u ogid=%u mode=%#ho",
>>>> from_kuid(&init_user_ns, context->ipc.uid),
>>>> from_kgid(&init_user_ns, context->ipc.gid),
>>>> context->ipc.mode);
>>>> -if (osid) {
>>>> +if (lsmblob_is_set(olsm)) {
>>>> struct lsmcontext lsmcxt;
>>>> -struct lsmblob blob;
>>>>
>>>> -lsmblob_init(&blob, osid);
>>>> -if (security_secid_to_secctx(&blob, &lsmcxt)) {
>>>> -audit_log_format(ab, " osid=%u", osid);
>>> I am not comfortable just dropping this I would think logging all secids is
>>> the
>>> correct action here.
>>>
>>>
>>>> +if (security_secid_to_secctx(olsm, &lsmcxt))
>>>> *call_panic = 1;
>>>> -} else {
>>>> +else {
>>>> audit_log_format(ab, " obj=%s",
>>>> lsmcxt.context);
>>>> security_release_secctx(&lsmcxt);
>>>> }
>
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v5 15/23] LSM: Specify which LSM to display
On 7/9/19 2:34 PM, Stephen Smalley wrote: > On 7/9/19 5:18 PM, Casey Schaufler wrote: >> On 7/9/2019 11:12 AM, Stephen Smalley wrote: >>> On 7/9/19 1:51 PM, Casey Schaufler wrote: On 7/9/2019 10:13 AM, Stephen Smalley wrote: > On 7/3/19 5:25 PM, Casey Schaufler wrote: >> Create a new entry "display" in /proc/.../attr for controlling >> which LSM security information is displayed for a process. >> 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() to get the display >> slot for a task_struct. > > As I explained previously, this is a security hole waiting to happen. It > still permits a process to affect the output of audit, alter the result > of reading or writing /proc/self/attr nodes even by > setuid/setgid/file-caps/context-changing programs, alter the contexts > generated in netlink messages delivered to other processes (I think?), > and possibly other effects beyond affecting the process' own view of > things. I would very much like some feedback regarding which of the possible formats for putting multiple subject contexts in audit records would be preferred: lsm=selinux,subj=xyzzy_t lsm=smack,subj=Xyzzy lsm=selinux,smack subj=xyzzy_t,Xyzzy subj="selinux='xyzzy_t',smack='Xyzzy'" >>> >>> (cc'd linux-audit mailing list) >>> Or something else. Free bikeshedding! I don't see how you have a problem with netlink. My look at what's in the kernel didn't expose anything, but I am willing to be educated. >>> >>> I haven't traced through it in detail, but it wasn't clear to me that the >>> security_secid_to_secctx() call always occurs in the context of the >>> receiving process (and hence use its display value). If not, then the >>> display of the sender can affect what is reported to the receiver; hence, >>> there is a forgery concern similar to the binder issue. It would be >>> cleaner if we didn't alter the default behavior of >>> security_secid_to_secctx() and security_secctx_to_secid() and instead >>> introduced new hooks for any case where we truly want the display to take >>> effect. >> >> If the context is generated by security_secid_to_secctx() we >> retain the slot number of the module that created it in lsmcontext. >> We have to to ensure it is released correctly. If the potential >> issue you're describing for netlink does in fact occur, we can check >> the slot in lsmcontext to verify that it is the same. >> >> security_secid_to_secctx() is called nowhere in net/netlink, >> at least not that grep finds. Where are you seeing this potential >> problem? > > Look under net/netfilter. > >> >>> > Before: > $ id > uid=1002(sds2) gid=1002(sds2) groups=1002(sds2) > context=staff_u:staff_r:staff_t:s0-s0:c0.c1023 > $ su > Password: > su: Authentication failure > > syscall audit record: > type=SYSCALL msg=audit(07/09/2019 11:52:49.784:365) : arch=x86_64 > syscall=openat > success=no exit=EACCES(Permission denied) a0=0xff9c > a1=0x560897e58e00 a2=O_ > WRONLY a3=0x0 items=1 ppid=3258 pid=3781 auid=sds2 uid=sds2 gid=sds2 > euid=root s > uid=root fsuid=root egid=sds2 sgid=sds2 fsgid=sds2 tty=pts2 ses=6 comm=su > exe=/usr/bin/su subj=staff_u:staff_r:staff_t:s0-s0:c0.c1023 key=(null) > > After: > $ id > uid=1002(sds2) gid=1002(sds2) groups=1002(sds2) > context=staff_u:staff_r:staff_t:s0-s0:c0.c1023 > $ echo apparmor > /proc/self/attr/display > $ su > Password: > su: Authentication failure > > audit record: > type=SYSCALL msg=audit(07/09/2019 12:05:32.402:406) : arch=x86_64 > syscall=openat success=no exit=EACCES(Permission denied) a0=0xff9c > a1=0x556b41e1ae00 a2=O_WRONLY a3=0x0 items=1 ppid=3258 pid=9426 auid=sds2 > uid=sds2 gid=sds2 euid=root suid=root fsuid=root egid=sds2 sgid=sds2 > fsgid=sds2 tty=pts2 ses=6 comm=su exe=/usr/bin/su subj==unconfined > key=(null) > > NB The subj= field of the SYSCALL audit record is longer accurate and is > potentially under the control of a process that would not be authorized > to set its subject label to that value by SELinux. It's still accurate, it's just not complete. It's a matter of how best to complete it. > > Now, let's play with userspace. > > Before: > # id > uid=0(root) gid=0(root) groups=0(root) > context=staff_u:staff_r:staff_t:s0-s0:c0.c1023 > # passwd root > passwd: SELinux deny access due to security policy. > > audit record: > type=USER_AVC msg=audit(07/09/2019 12:24:35.135:812) : pid=12693 uid=root
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.
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
