Re: [RFC PATCH v9 06/16] ipe: add LSM hooks on execution and kernel read
On Tue, Jan 31, 2023 at 01:51:39PM +0100, Roberto Sassu wrote:
> On Mon, 2023-01-30 at 14:57 -0800, Fan Wu wrote:
> > +
> > +/**
> > + * ipe_mmap_file - ipe security hook function for mmap check.
> > + * @f: File being mmap'd. Can be NULL in the case of anonymous memory.
> > + * @reqprot: The requested protection on the mmap, passed from usermode.
> > + * @prot: The effective protection on the mmap, resolved from reqprot and
> > + * system configuration.
> > + * @flags: Unused.
> > + *
> > + * This hook is called when a file is loaded through the mmap
> > + * family of system calls.
> > + *
> > + * Return:
> > + * * 0 - OK
> > + * * !0- Error
> > + */
> > +int ipe_mmap_file(struct file *f, unsigned long reqprot, unsigned long
> > prot,
> > + unsigned long flags)
> > +{
> > + struct ipe_eval_ctx ctx = { 0 };
> > +
> > + if (prot & PROT_EXEC || reqprot & PROT_EXEC) {
>
> Since the kernel only adds flags and doesn't clear them, isn't safe to
> just consider prot? Oh, you mentioned it in the changelog, maybe just
> for ipe_file_mprotect().
>
Thanks for pointing that out, yes reqprot it indeed unnecessary, I will remove
this part in the next version.
> > + build_eval_ctx(&ctx, f, ipe_op_exec);
> > + return ipe_evaluate_event(&ctx);
> > + }
>
> Uhm, I think some considerations that IMA does for mmap() are relevant
> also for IPE.
>
> For example, look at mmap_violation_check(). It checks if there are
> writable mappings, and if yes, it denies the access.
>
> Similarly for mprotect(), is adding PROT_EXEC safe?
>
Yes, writable mapping might need to treat differently. But for the current
version
I think it is safe because currently we only support dmverity and fsverity,
they are inherently read-only.
But if in the future if there is a feature can support writable mapping, IPE
might
better provide user the flexibility to allow or deny execute writable mappings,
for example, adding a new property like file_writable=TRUE. Then user can deploy
a rule like op=EXECUTE file_writable=TRUE action=DENY to deny execute a writable
mapping.
> >
> > @@ -12,6 +13,11 @@ static struct lsm_blob_sizes ipe_blobs
> > __lsm_ro_after_init = {
> >
> > static struct security_hook_list ipe_hooks[] __lsm_ro_after_init = {
> > LSM_HOOK_INIT(sb_free_security, ipe_sb_free_security),
> > + LSM_HOOK_INIT(bprm_check_security, ipe_bprm_check_security),
> > + LSM_HOOK_INIT(mmap_file, ipe_mmap_file),
> > + LSM_HOOK_INIT(file_mprotect, ipe_file_mprotect),
> > + LSM_HOOK_INIT(kernel_read_file, ipe_kernel_read_file),
> > + LSM_HOOK_INIT(kernel_load_data, ipe_kernel_load_data),
> > };
>
> Uhm, maybe I would incorporate patch 1 with this.
>
> Roberto
This might not be possible because this patch has some dependencies on the
previous patches.
-Fan
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2] io_uring,audit: don't log IORING_OP_MADVISE
On Thursday, February 9, 2023 5:37:22 PM EST Paul Moore wrote: > On Thu, Feb 9, 2023 at 4:53 PM Richard Guy Briggs wrote: > > On 2023-02-01 16:18, Paul Moore wrote: > > > On Wed, Feb 1, 2023 at 3:34 PM Richard Guy Briggs wrote: > > > > fadvise and madvise both provide hints for caching or access pattern > > > > for file and memory respectively. Skip them. > > > > > > You forgot to update the first sentence in the commit description :/ > > > > I didn't forget. I updated that sentence to reflect the fact that the > > two should be treated similarly rather than differently. > > Ooookay. Can we at least agree that the commit description should be > rephrased to make it clear that the patch only adjusts madvise? Right > now I read the commit description and it sounds like you are adjusting > the behavior for both fadvise and madvise in this patch, which is not > true. > > > > I'm still looking for some type of statement that you've done some > > > homework on the IORING_OP_MADVISE case to ensure that it doesn't end > > > up calling into the LSM, see my previous emails on this. I need more > > > than "Steve told me to do this". > > > > > > I basically just want to see that some care and thought has gone into > > > this patch to verify it is correct and good. > > > > Steve suggested I look into a number of iouring ops. I looked at the > > description code and agreed that it wasn't necessary to audit madvise. > > The rationale for fadvise was detemined to have been conflated with > > fallocate and subsequently dropped. Steve also suggested a number of > > others and after investigation I decided that their current state was > > correct. *getxattr you've advised against, so it was dropped. It > > appears fewer modifications were necessary than originally suspected. > > My concern is that three of the four changes you initially proposed > were rejected, which gives me pause about the fourth. You mention > that based on your reading of madvise's description you feel auditing > isn't necessary - and you may be right - but based on our experience > so far with this patchset I would like to hear that you have properly > investigated all of the madvise code paths, and I would like that in > the commit description. I think you're being unnecessarily hard on this. Yes, the commit message might be touched up. But madvise is advisory in nature. It is not security relevant. And a grep through the security directory doesn't turn up any hooks. -Steve -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH v9 12/16] fsverity: consume builtin signature via LSM hook
On Wed, Feb 08, 2023 at 07:30:33PM -0800, Eric Biggers wrote:
> So disregarding the fact that using the fsverity builtin signatures still
> seems
> like a bad idea to me, here's a few comments on the diff itself:
>
Thanks for the review. I have verified the headers are indeed unnecessary,
I will remove them in the next version.
> On Mon, Jan 30, 2023 at 02:57:27PM -0800, Fan Wu wrote:
> > diff --git a/fs/verity/open.c b/fs/verity/open.c
> > index 81ff94442f7b..7e6fa52c0e9c 100644
> > --- a/fs/verity/open.c
> > +++ b/fs/verity/open.c
> > @@ -7,7 +7,9 @@
> >
> > #include "fsverity_private.h"
> >
> > +#include
> > #include
> > +#include
>
> There's no need to include .
>
> >
> > + if (err) {
> > + fsverity_err(inode, "Error %d verifying signature", err);
> > + goto out;
> > + }
>
> The above error message is unnecessary because fsverity_verify_signature()
> already prints an error message on failure.
>
> > +
> > + err = security_inode_setsecurity(inode, FS_VERITY_INODE_SEC_NAME,
> > desc->signature,
> > +le32_to_cpu(desc->sig_size), 0);
>
> This runs even if CONFIG_FS_VERITY_BUILTIN_SIGNATURES is disabled. Is that
> really the right behavior?
>
Yes the hook call should better depend on a KCONFIG. After second thought I
think it
should depend on CONFIG_IPE_PROP_FS_VERITY, which also indirectly introduces the
dependency on CONFIG_FS_VERITY_BUILTIN_SIGNATURES.
Currently security_inode_setsecurity only allows one LSM to save data with a
given name.
In our case IPE will be the only LSM that saves the signature.
I will update this part in the next version.
> Also a nit: please stick to the preferred line length of 80 characters.
> See Documentation/process/coding-style.rst
>
> > diff --git a/fs/verity/signature.c b/fs/verity/signature.c
> > index 143a530a8008..5d7b9496f9c4 100644
> > --- a/fs/verity/signature.c
> > +++ b/fs/verity/signature.c
> > @@ -9,6 +9,7 @@
> >
> > #include
> > #include
> > +#include
> > #include
> > #include
>
> This change is unnecessary.
>
> > diff --git a/include/linux/fsverity.h b/include/linux/fsverity.h
> > index 40f14e5fed9d..29e9888287ba 100644
> > --- a/include/linux/fsverity.h
> > +++ b/include/linux/fsverity.h
> > @@ -254,4 +254,6 @@ static inline bool fsverity_active(const struct inode
> > *inode)
> > return fsverity_get_info(inode) != NULL;
> > }
> >
> > +#define FS_VERITY_INODE_SEC_NAME "fsverity.inode-info"
>
> "inode-info" is very vague. Shouldn't it be named "builtin-sig" or something?
>
> - Eric
I agree this name works better, I will change it to "fsverity.builtin-sig".
-Fan
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2] io_uring,audit: don't log IORING_OP_MADVISE
On Thu, Feb 9, 2023 at 4:53 PM Richard Guy Briggs wrote: > On 2023-02-01 16:18, Paul Moore wrote: > > On Wed, Feb 1, 2023 at 3:34 PM Richard Guy Briggs wrote: > > > fadvise and madvise both provide hints for caching or access pattern for > > > file and memory respectively. Skip them. > > > > You forgot to update the first sentence in the commit description :/ > > I didn't forget. I updated that sentence to reflect the fact that the > two should be treated similarly rather than differently. Ooookay. Can we at least agree that the commit description should be rephrased to make it clear that the patch only adjusts madvise? Right now I read the commit description and it sounds like you are adjusting the behavior for both fadvise and madvise in this patch, which is not true. > > I'm still looking for some type of statement that you've done some > > homework on the IORING_OP_MADVISE case to ensure that it doesn't end > > up calling into the LSM, see my previous emails on this. I need more > > than "Steve told me to do this". > > > > I basically just want to see that some care and thought has gone into > > this patch to verify it is correct and good. > > Steve suggested I look into a number of iouring ops. I looked at the > description code and agreed that it wasn't necessary to audit madvise. > The rationale for fadvise was detemined to have been conflated with > fallocate and subsequently dropped. Steve also suggested a number of > others and after investigation I decided that their current state was > correct. *getxattr you've advised against, so it was dropped. It > appears fewer modifications were necessary than originally suspected. My concern is that three of the four changes you initially proposed were rejected, which gives me pause about the fourth. You mention that based on your reading of madvise's description you feel auditing isn't necessary - and you may be right - but based on our experience so far with this patchset I would like to hear that you have properly investigated all of the madvise code paths, and I would like that in the commit description. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2] io_uring,audit: don't log IORING_OP_MADVISE
On 2023-02-01 16:18, Paul Moore wrote:
> On Wed, Feb 1, 2023 at 3:34 PM Richard Guy Briggs wrote:
> > fadvise and madvise both provide hints for caching or access pattern for
> > file and memory respectively. Skip them.
>
> You forgot to update the first sentence in the commit description :/
I didn't forget. I updated that sentence to reflect the fact that the
two should be treated similarly rather than differently.
> I'm still looking for some type of statement that you've done some
> homework on the IORING_OP_MADVISE case to ensure that it doesn't end
> up calling into the LSM, see my previous emails on this. I need more
> than "Steve told me to do this".
>
> I basically just want to see that some care and thought has gone into
> this patch to verify it is correct and good.
Steve suggested I look into a number of iouring ops. I looked at the
description code and agreed that it wasn't necessary to audit madvise.
The rationale for fadvise was detemined to have been conflated with
fallocate and subsequently dropped. Steve also suggested a number of
others and after investigation I decided that their current state was
correct. *getxattr you've advised against, so it was dropped. It
appears fewer modifications were necessary than originally suspected.
> > Fixes: 5bd2182d58e9 ("audit,io_uring,io-wq: add some basic audit support to
> > io_uring")
> > Signed-off-by: Richard Guy Briggs
> > ---
> > changelog
> > v2:
> > - drop *GETXATTR patch
> > - drop FADVISE hunk
> >
> > io_uring/opdef.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/io_uring/opdef.c b/io_uring/opdef.c
> > index 3aa0d65c50e3..d3f36c633ceb 100644
> > --- a/io_uring/opdef.c
> > +++ b/io_uring/opdef.c
> > @@ -312,6 +312,7 @@ const struct io_op_def io_op_defs[] = {
> > .issue = io_fadvise,
> > },
> > [IORING_OP_MADVISE] = {
> > + .audit_skip = 1,
> > .name = "MADVISE",
> > .prep = io_madvise_prep,
> > .issue = io_madvise,
> > --
> > 2.27.0
>
> --
> paul-moore.com
>
- RGB
--
Richard Guy Briggs
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
audit-3.1 released
Hello, I've just released a new version of the audit daemon. It can be downloaded from http://people.redhat.com/sgrubb/audit. It will also be in rawhide soon. The ChangeLog is: - Disable ProtectControlGroups in auditd.service by default - Fix rule checking for exclude filter - Make audit_rule_syscallbyname_data work correctly outside of auditctl - Add new record types - Add io_uring support - Add support for new FANOTIFY record fields - Add keyword, this-hour, to ausearch/report start/end options - Add Requires.private to audit.pc file - Try to interpret OPENAT2 fields correctly ProtectControlGroups in auditd.service was preventing people from placing rules on /proc, so it's now off by default. The checks on the exclude filter were not matching the checks that kernel does. The checks were updated. io_uring support was added as was support for translating the new fields in the FANOTIFY record. The interpretation of the mode field in the OPENAT2 record was broken because it was expected to be in hex format like everywhere else, but instead it is decimal. That should be corrected. SHA256: b5cf3cdabb2786c08b1de3599a3b1a547e55f7a9f9c1eb2078f5b44cf44e8378 Please let me know if you run across any problems with this release. -Steve -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v7 0/3] fanotify: Allow user space to pass back additional audit info
On Wed 08-02-23 10:03:24, Paul Moore wrote: > On Wed, Feb 8, 2023 at 7:08 AM Jan Kara wrote: > > On Tue 07-02-23 09:54:11, Paul Moore wrote: > > > On Tue, Feb 7, 2023 at 7:09 AM Jan Kara wrote: > > > > On Fri 03-02-23 16:35:13, Richard Guy Briggs wrote: > > > > > The Fanotify API can be used for access control by requesting > > > > > permission > > > > > event notification. The user space tooling that uses it may have a > > > > > complicated policy that inherently contains additional context for the > > > > > decision. If this information were available in the audit trail, > > > > > policy > > > > > writers can close the loop on debugging policy. Also, if this > > > > > additional > > > > > information were available, it would enable the creation of tools that > > > > > can suggest changes to the policy similar to how audit2allow can help > > > > > refine labeled security. > > > > > > > > > > This patchset defines a new flag (FAN_INFO) and new extensions that > > > > > define additional information which are appended after the response > > > > > structure returned from user space on a permission event. The > > > > > appended > > > > > information is organized with headers containing a type and size that > > > > > can be delegated to interested subsystems. One new information type > > > > > is > > > > > defined to audit the triggering rule number. > > > > > > > > > > A newer kernel will work with an older userspace and an older kernel > > > > > will behave as expected and reject a newer userspace, leaving it up to > > > > > the newer userspace to test appropriately and adapt as necessary. > > > > > This > > > > > is done by providing a a fully-formed FAN_INFO extension but setting > > > > > the > > > > > fd to FAN_NOFD. On a capable kernel, it will succeed but issue no > > > > > audit > > > > > record, whereas on an older kernel it will fail. > > > > > > > > > > The audit function was updated to log the additional information in > > > > > the > > > > > AUDIT_FANOTIFY record. The following are examples of the new record > > > > > format: > > > > > type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 > > > > > fan_info=3137 subj_trust=3 obj_trust=5 > > > > > type=FANOTIFY msg=audit(1659730979.839:284): resp=1 fan_type=0 > > > > > fan_info=0 subj_trust=2 obj_trust=2 > > > > > > > > Thanks! I've applied this series to my tree. > > > > > > While I think this version of the patchset is fine, for future > > > reference it would have been nice if you had waited for my ACK on > > > patch 3/3; while Steve maintains his userspace tools, I'm the one > > > responsible for maintaining the Linux Kernel's audit subsystem. > > > > Aha, I'm sorry for that. I had the impression that on the last version of > > the series you've said you don't see anything for which the series should > > be respun so once Steve's objections where addressed and you were silent > > for a few days, I thought you consider the thing settled... My bad. > > That's understandable, especially given inconsistencies across > subsystems. If it helps, if I'm going to ACK something I make it > explicit with a proper 'Acked-by: ...' line in my reply; if I say > something looks good but there is no explicit ACK, there is usually > something outstanding that needs to be resolved, e.g. questions, > additional testing, etc. Ok, thanks for letting me now. Next time I'll wait for an explicit ack from you. This time, since everybody is fine with the actual patch, let's just move on ;). Honza -- Jan Kara SUSE Labs, CR -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH v9 12/16] fsverity: consume builtin signature via LSM hook
So disregarding the fact that using the fsverity builtin signatures still seems
like a bad idea to me, here's a few comments on the diff itself:
On Mon, Jan 30, 2023 at 02:57:27PM -0800, Fan Wu wrote:
> diff --git a/fs/verity/open.c b/fs/verity/open.c
> index 81ff94442f7b..7e6fa52c0e9c 100644
> --- a/fs/verity/open.c
> +++ b/fs/verity/open.c
> @@ -7,7 +7,9 @@
>
> #include "fsverity_private.h"
>
> +#include
> #include
> +#include
There's no need to include .
>
> static struct kmem_cache *fsverity_info_cachep;
>
> @@ -146,7 +148,7 @@ static int compute_file_digest(struct fsverity_hash_alg
> *hash_alg,
> * appended signature), and check the signature if present. The
> * fsverity_descriptor must have already undergone basic validation.
> */
> -struct fsverity_info *fsverity_create_info(const struct inode *inode,
> +struct fsverity_info *fsverity_create_info(struct inode *inode,
> struct fsverity_descriptor *desc)
> {
> struct fsverity_info *vi;
> @@ -182,6 +184,15 @@ struct fsverity_info *fsverity_create_info(const struct
> inode *inode,
>
> err = fsverity_verify_signature(vi, desc->signature,
> le32_to_cpu(desc->sig_size));
> + if (err) {
> + fsverity_err(inode, "Error %d verifying signature", err);
> + goto out;
> + }
The above error message is unnecessary because fsverity_verify_signature()
already prints an error message on failure.
> +
> + err = security_inode_setsecurity(inode, FS_VERITY_INODE_SEC_NAME,
> desc->signature,
> + le32_to_cpu(desc->sig_size), 0);
This runs even if CONFIG_FS_VERITY_BUILTIN_SIGNATURES is disabled. Is that
really the right behavior?
Also a nit: please stick to the preferred line length of 80 characters.
See Documentation/process/coding-style.rst
> diff --git a/fs/verity/signature.c b/fs/verity/signature.c
> index 143a530a8008..5d7b9496f9c4 100644
> --- a/fs/verity/signature.c
> +++ b/fs/verity/signature.c
> @@ -9,6 +9,7 @@
>
> #include
> #include
> +#include
> #include
> #include
This change is unnecessary.
> diff --git a/include/linux/fsverity.h b/include/linux/fsverity.h
> index 40f14e5fed9d..29e9888287ba 100644
> --- a/include/linux/fsverity.h
> +++ b/include/linux/fsverity.h
> @@ -254,4 +254,6 @@ static inline bool fsverity_active(const struct inode
> *inode)
> return fsverity_get_info(inode) != NULL;
> }
>
> +#define FS_VERITY_INODE_SEC_NAME "fsverity.inode-info"
"inode-info" is very vague. Shouldn't it be named "builtin-sig" or something?
- Eric
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
