| Message ID | f602429ce0f419c2abc3ae5a0e705e1368ac5650.1674682056.git.rgb@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | two suggested iouring op audit updates | expand |
On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > Getting XATTRs is not particularly interesting security-wise. > > Suggested-by: Steve Grubb <sgrubb@redhat.com> > Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support") > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > --- > 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
On 2023-01-27 17:43, Paul Moore wrote: > On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > Getting XATTRs is not particularly interesting security-wise. > > > > Suggested-by: Steve Grubb <sgrubb@redhat.com> > > Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support") > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > --- > > 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 <rgb@redhat.com> 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
On 1/27/23 4: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 <rgb@redhat.com> wrote: >>> Getting XATTRs is not particularly interesting security-wise. >>> >>> Suggested-by: Steve Grubb <sgrubb@redhat.com> >>> Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support") >>> Signed-off-by: Richard Guy Briggs <rgb@redhat.com> >>> --- >>> 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. (audit list removed) Right now the xattr related functions are io-wq driven, and hence not super performance sensitive. But I'd greatly prefer to clean these up regardless, because once opcodes get upgraded from needing io-wq, then we don't have to go through the audit discussion at that point. Better to do it upfront, like now, regardless of expectation of frequency of calls.
On Fri, Jan 27, 2023 at 6:01 PM Richard Guy Briggs <rgb@redhat.com> wrote: > On 2023-01-27 17:43, Paul Moore wrote: > > On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > > Getting XATTRs is not particularly interesting security-wise. > > > > > > Suggested-by: Steve Grubb <sgrubb@redhat.com> > > > Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support") > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > > --- > > > 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.
On Fri, Jan 27, 2023 at 6:05 PM Jens Axboe <axboe@kernel.dk> wrote: > On 1/27/23 4: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 <rgb@redhat.com> wrote: > >>> Getting XATTRs is not particularly interesting security-wise. > >>> > >>> Suggested-by: Steve Grubb <sgrubb@redhat.com> > >>> Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support") > >>> Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > >>> --- > >>> 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. > > (audit list removed) > > Right now the xattr related functions are io-wq driven, and hence not > super performance sensitive. But I'd greatly prefer to clean these up > regardless, because once opcodes get upgraded from needing io-wq, then > we don't have to go through the audit discussion at that point. Better > to do it upfront, like now, regardless of expectation of frequency of > calls. See my reply to Richard, but unfortunately we need to continue to audit the getxattr ops.
On 2023-01-27 19:06, Paul Moore wrote: > On Fri, Jan 27, 2023 at 6:01 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > On 2023-01-27 17:43, Paul Moore wrote: > > > On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > > > Getting XATTRs is not particularly interesting security-wise. > > > > > > > > Suggested-by: Steve Grubb <sgrubb@redhat.com> > > > > Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support") > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > > > --- > > > > 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 <rgb@redhat.com> 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
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 <rgb@redhat.com> wrote: > > Getting XATTRs is not particularly interesting security-wise. > > > > Suggested-by: Steve Grubb <sgrubb@redhat.com> > > Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support") > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > --- > > 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
On Sat, Jan 28, 2023 at 12:26 PM Steve Grubb <sgrubb@redhat.com> 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 <rgb@redhat.com> wrote: > > > Getting XATTRs is not particularly interesting security-wise. > > > > > > Suggested-by: Steve Grubb <sgrubb@redhat.com> > > > Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support") > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > > --- > > > 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
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,
Getting XATTRs is not particularly interesting security-wise. Suggested-by: Steve Grubb <sgrubb@redhat.com> Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support") Signed-off-by: Richard Guy Briggs <rgb@redhat.com> --- io_uring/opdef.c | 2 ++ 1 file changed, 2 insertions(+)