Re: [PATCH v1 2/2] io_uring,audit: do not log IORING_OP_*GETXATTR
On Sat, Jan 28, 2023 at 12:26 PM Steve Grubb wrote:
> On Friday, January 27, 2023 5:43:02 PM EST Paul Moore wrote:
> > On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs wrote:
> > > Getting XATTRs is not particularly interesting security-wise.
> > >
> > > Suggested-by: Steve Grubb
> > > Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support")
> > > Signed-off-by: Richard Guy Briggs
> > > ---
> > > io_uring/opdef.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> >
> > Depending on your security policy, fetching file data, including
> > xattrs, can be interesting from a security perspective. As an
> > example, look at the SELinux file/getattr permission.
> >
> > https://github.com/SELinuxProject/selinux-notebook/blob/main/src/object_cla
> > sses_permissions.md#common-file-permissions
>
> We're mostly interested in setting attributes because that changes policy.
> Reading them is not interesting unless the access fails with EPERM.
See my earlier comments, SELinux does have provisions for caring about
reading xattrs, and now that I look at the rest of the LSMs it looks
like Smack cares about reading xattrs too. Regardless of whether a
given security policy cares about xattr access, the LSMs support
enforcing access on reading xattrs so we need to ensure the audit is
setup properly in these cases.
> I was updating the user space piece recently and saw there was a bunch of
> "new" operations. I was commenting that we need to audit 5 or 6 of the "new"
> operations such as IORING_OP_MKDIRATor IORING_OP_SETXATTR. But now that I see
> the patch, it looks like they are auditable and we can just let a couple be
> skipped. IORING_OP_MADVISE is not interesting as it just gives hiints about
> the expected access patterns of memory. If there were an equivalent of
> mprotect, that would be of interest, but not madvise.
Once again, as discussed previously, it is likely that skipping
auditing for IORING_OP_MADVISE is okay, but given that several of the
changes in this patchset were incorrect, I'd like a little more
thorough investigation before we skip auditing on madvise.
> There are some I'm not sure about such as IORING_OP_MSG_RING and
> IORING_OP_URING_CMD. What do they do?
Look at 4f57f06ce218 ("io_uring: add support for IORING_OP_MSG_RING
command") for the patch which added IORING_OP_MSG_RING as it has a
decent commit description. As for IORING_OP_URING_CMD, there were
lengthy discussions about it on the mailing lists (including audit)
back in March 2022 and then later in August on the LSM, SELinux, etc.
mailing lists when we landed some patches for it (there were no audit
changes). I also covered the IORING_OP_URING_CMD, albeit briefly, in
a presentation at LSS-EU last year:
https://www.youtube.com/watch?v=AaaH6skUEI8
https://www.paul-moore.com/docs/2022-lss_eu-iouring_lsm-pcmoore-r3.pdf
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v1 2/2] io_uring,audit: do not log IORING_OP_*GETXATTR
On Friday, January 27, 2023 5:43:02 PM EST Paul Moore wrote:
> On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs wrote:
> > Getting XATTRs is not particularly interesting security-wise.
> >
> > Suggested-by: Steve Grubb
> > Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support")
> > Signed-off-by: Richard Guy Briggs
> > ---
> > io_uring/opdef.c | 2 ++
> > 1 file changed, 2 insertions(+)
>
> Depending on your security policy, fetching file data, including
> xattrs, can be interesting from a security perspective. As an
> example, look at the SELinux file/getattr permission.
>
> https://github.com/SELinuxProject/selinux-notebook/blob/main/src/object_cla
> sses_permissions.md#common-file-permissions
We're mostly interested in setting attributes because that changes policy.
Reading them is not interesting unless the access fails with EPERM.
I was updating the user space piece recently and saw there was a bunch of
"new" operations. I was commenting that we need to audit 5 or 6 of the "new"
operations such as IORING_OP_MKDIRATor IORING_OP_SETXATTR. But now that I see
the patch, it looks like they are auditable and we can just let a couple be
skipped. IORING_OP_MADVISE is not interesting as it just gives hiints about
the expected access patterns of memory. If there were an equivalent of
mprotect, that would be of interest, but not madvise.
There are some I'm not sure about such as IORING_OP_MSG_RING and
IORING_OP_URING_CMD. What do they do?
-Steve
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v1 2/2] io_uring,audit: do not log IORING_OP_*GETXATTR
On 2023-01-27 19:06, Paul Moore wrote:
> On Fri, Jan 27, 2023 at 6:01 PM Richard Guy Briggs wrote:
> > On 2023-01-27 17:43, Paul Moore wrote:
> > > On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs
> > > wrote:
> > > > Getting XATTRs is not particularly interesting security-wise.
> > > >
> > > > Suggested-by: Steve Grubb
> > > > Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support")
> > > > Signed-off-by: Richard Guy Briggs
> > > > ---
> > > > io_uring/opdef.c | 2 ++
> > > > 1 file changed, 2 insertions(+)
> > >
> > > Depending on your security policy, fetching file data, including
> > > xattrs, can be interesting from a security perspective. As an
> > > example, look at the SELinux file/getattr permission.
> > >
> > > https://github.com/SELinuxProject/selinux-notebook/blob/main/src/object_classes_permissions.md#common-file-permissions
> >
> > The intent here is to lessen the impact of audit operations. Read and
> > Write were explicitly removed from io_uring auditing due to performance
> > concerns coupled with the denial of service implications from sheer
> > volume of records making other messages harder to locate. Those
> > operations are still possible for syscall auditing but they are strongly
> > discouraged for normal use.
>
> We need to balance security needs and performance needs. You are
> correct that general read() and write() operations are not audited,
> and generally not checked from a LSM perspective as the auditing and
> access control happens at open() time instead (access to fds is
> revalidated when they are passed). However, in the case of getxattr
> and fgetxattr, these are not normal file read operations, and do not
> go through the same code path in the kernel; there is a reason why we
> have xattr_permission() and security_inode_getxattr().
>
> We need to continue to audit IORING_OP_FGETXATTR and IORING_OP_GETXATTR.
Fair enough. This would be similar reasoning to send/recv vs
sendmsg/recvmsg. I'll drop this patch. Thanks for the reasoning and
feedback.
> 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
Re: [PATCH v1 2/2] io_uring,audit: do not log IORING_OP_*GETXATTR
On Fri, Jan 27, 2023 at 6:01 PM Richard Guy Briggs wrote:
> On 2023-01-27 17:43, Paul Moore wrote:
> > On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs wrote:
> > > Getting XATTRs is not particularly interesting security-wise.
> > >
> > > Suggested-by: Steve Grubb
> > > Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support")
> > > Signed-off-by: Richard Guy Briggs
> > > ---
> > > io_uring/opdef.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> >
> > Depending on your security policy, fetching file data, including
> > xattrs, can be interesting from a security perspective. As an
> > example, look at the SELinux file/getattr permission.
> >
> > https://github.com/SELinuxProject/selinux-notebook/blob/main/src/object_classes_permissions.md#common-file-permissions
>
> The intent here is to lessen the impact of audit operations. Read and
> Write were explicitly removed from io_uring auditing due to performance
> concerns coupled with the denial of service implications from sheer
> volume of records making other messages harder to locate. Those
> operations are still possible for syscall auditing but they are strongly
> discouraged for normal use.
We need to balance security needs and performance needs. You are
correct that general read() and write() operations are not audited,
and generally not checked from a LSM perspective as the auditing and
access control happens at open() time instead (access to fds is
revalidated when they are passed). However, in the case of getxattr
and fgetxattr, these are not normal file read operations, and do not
go through the same code path in the kernel; there is a reason why we
have xattr_permission() and security_inode_getxattr().
We need to continue to audit IORING_OP_FGETXATTR and IORING_OP_GETXATTR.
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v1 2/2] io_uring,audit: do not log IORING_OP_*GETXATTR
On 2023-01-27 17:43, Paul Moore wrote:
> On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs wrote:
> > Getting XATTRs is not particularly interesting security-wise.
> >
> > Suggested-by: Steve Grubb
> > Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support")
> > Signed-off-by: Richard Guy Briggs
> > ---
> > io_uring/opdef.c | 2 ++
> > 1 file changed, 2 insertions(+)
>
> Depending on your security policy, fetching file data, including
> xattrs, can be interesting from a security perspective. As an
> example, look at the SELinux file/getattr permission.
>
> https://github.com/SELinuxProject/selinux-notebook/blob/main/src/object_classes_permissions.md#common-file-permissions
The intent here is to lessen the impact of audit operations. Read and
Write were explicitly removed from io_uring auditing due to performance
concerns coupled with the denial of service implications from sheer
volume of records making other messages harder to locate. Those
operations are still possible for syscall auditing but they are strongly
discouraged for normal use.
If the frequency of getxattr io_uring ops is so infrequent as to be no
distraction, then this patch may be more of a liability than a benefit.
> > diff --git a/io_uring/opdef.c b/io_uring/opdef.c
> > index a2bf53b4a38a..f6bfe2cf078c 100644
> > --- a/io_uring/opdef.c
> > +++ b/io_uring/opdef.c
> > @@ -462,12 +462,14 @@ const struct io_op_def io_op_defs[] = {
> > },
> > [IORING_OP_FGETXATTR] = {
> > .needs_file = 1,
> > + .audit_skip = 1,
> > .name = "FGETXATTR",
> > .prep = io_fgetxattr_prep,
> > .issue = io_fgetxattr,
> > .cleanup= io_xattr_cleanup,
> > },
> > [IORING_OP_GETXATTR] = {
> > + .audit_skip = 1,
> > .name = "GETXATTR",
> > .prep = io_getxattr_prep,
> > .issue = io_getxattr,
> > --
> > 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
Re: [PATCH v1 2/2] io_uring,audit: do not log IORING_OP_*GETXATTR
On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs wrote:
>
> Getting XATTRs is not particularly interesting security-wise.
>
> Suggested-by: Steve Grubb
> Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support")
> Signed-off-by: Richard Guy Briggs
> ---
> io_uring/opdef.c | 2 ++
> 1 file changed, 2 insertions(+)
Depending on your security policy, fetching file data, including
xattrs, can be interesting from a security perspective. As an
example, look at the SELinux file/getattr permission.
https://github.com/SELinuxProject/selinux-notebook/blob/main/src/object_classes_permissions.md#common-file-permissions
> diff --git a/io_uring/opdef.c b/io_uring/opdef.c
> index a2bf53b4a38a..f6bfe2cf078c 100644
> --- a/io_uring/opdef.c
> +++ b/io_uring/opdef.c
> @@ -462,12 +462,14 @@ const struct io_op_def io_op_defs[] = {
> },
> [IORING_OP_FGETXATTR] = {
> .needs_file = 1,
> + .audit_skip = 1,
> .name = "FGETXATTR",
> .prep = io_fgetxattr_prep,
> .issue = io_fgetxattr,
> .cleanup= io_xattr_cleanup,
> },
> [IORING_OP_GETXATTR] = {
> + .audit_skip = 1,
> .name = "GETXATTR",
> .prep = io_getxattr_prep,
> .issue = io_getxattr,
> --
> 2.27.0
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
[PATCH v1 2/2] io_uring,audit: do not log IORING_OP_*GETXATTR
Getting XATTRs is not particularly interesting security-wise.
Suggested-by: Steve Grubb
Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support")
Signed-off-by: Richard Guy Briggs
---
io_uring/opdef.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/io_uring/opdef.c b/io_uring/opdef.c
index a2bf53b4a38a..f6bfe2cf078c 100644
--- a/io_uring/opdef.c
+++ b/io_uring/opdef.c
@@ -462,12 +462,14 @@ const struct io_op_def io_op_defs[] = {
},
[IORING_OP_FGETXATTR] = {
.needs_file = 1,
+ .audit_skip = 1,
.name = "FGETXATTR",
.prep = io_fgetxattr_prep,
.issue = io_fgetxattr,
.cleanup= io_xattr_cleanup,
},
[IORING_OP_GETXATTR] = {
+ .audit_skip = 1,
.name = "GETXATTR",
.prep = io_getxattr_prep,
.issue = io_getxattr,
--
2.27.0
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
