* selinux_netlink_send changes program behavior
@ 2020-04-24 8:27 Dmitry Vyukov
2020-04-24 21:51 ` Paul Moore
0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Vyukov @ 2020-04-24 8:27 UTC (permalink / raw)
To: Paul Moore, stephen.smalley.work, Eric Paris, selinux
Cc: LKML, syzkaller, Willem de Bruijn
Hi SELinux maintainers,
We've hit a case where a developer wasn't able to reproduce a kernel
bug, it turned out to be a difference in behavior between SELinux and
non-SELinux kernels.
Condensed version: a program does sendmmsg on netlink socket with 2
mmsghdr's, first is completely empty/zeros, second contains some
actual payload. Without SELinux the first mmsghdr is treated as no-op
and the kernel processes the second one (triggers bug). However the
SELinux hook does:
static int selinux_netlink_send(struct sock *sk, struct sk_buff *skb)
{
if (skb->len < NLMSG_HDRLEN) {
err = -EINVAL;
goto out;
}
and fails processing on the first empty mmsghdr (does not happen
without SELinux).
Is this difference in behavior intentional/acceptable/should be fixed?
Thanks
FTR, the C program is:
https://gist.githubusercontent.com/dvyukov/dda1c547ca9121817159d29afa72aea2/raw/41b021d722947df4d8c48e2fc783591b44671ceb/gistfile1.txt
kernel config:
https://gist.githubusercontent.com/dvyukov/08bf2c2fd873a84a2c4c771740716183/raw/78fb3b1063b7ae37625468f32868869edbd1bd19/gistfile1.txt
on upstream commit 50cc09c1 it triggers a KASAN bug without SELinux,
but does not with SELinux.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: selinux_netlink_send changes program behavior 2020-04-24 8:27 selinux_netlink_send changes program behavior Dmitry Vyukov @ 2020-04-24 21:51 ` Paul Moore 2020-04-25 5:14 ` Dmitry Vyukov 0 siblings, 1 reply; 6+ messages in thread From: Paul Moore @ 2020-04-24 21:51 UTC (permalink / raw) To: Dmitry Vyukov Cc: Stephen Smalley, Eric Paris, selinux, LKML, syzkaller, Willem de Bruijn On Fri, Apr 24, 2020 at 4:27 AM Dmitry Vyukov <dvyukov@google.com> wrote: > Hi SELinux maintainers, > > We've hit a case where a developer wasn't able to reproduce a kernel > bug, it turned out to be a difference in behavior between SELinux and > non-SELinux kernels. > Condensed version: a program does sendmmsg on netlink socket with 2 > mmsghdr's, first is completely empty/zeros, second contains some > actual payload. Without SELinux the first mmsghdr is treated as no-op > and the kernel processes the second one (triggers bug). However the > SELinux hook does: > > static int selinux_netlink_send(struct sock *sk, struct sk_buff *skb) > { > if (skb->len < NLMSG_HDRLEN) { > err = -EINVAL; > goto out; > } > > and fails processing on the first empty mmsghdr (does not happen > without SELinux). > > Is this difference in behavior intentional/acceptable/should be fixed? From a practical perspective, SELinux is always going to need to do a length check as it needs to peek into the netlink message header for the message type so it can map that to the associated SELinux permissions. So in that sense, the behavior is intentional and desired; however from a bug-for-bug compatibility perspective ... not so much. Ultimately, my it's-Friday-and-it's-been-a-long-week-ending-in-a-long-day thought is that this was a buggy operation to begin with and the bug was just caught in different parts of the kernel, depending on how it was configured. It may not be ideal, but I can think of worse things (and arguably SELinux is doing the Right Thing). -- paul moore www.paul-moore.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: selinux_netlink_send changes program behavior 2020-04-24 21:51 ` Paul Moore @ 2020-04-25 5:14 ` Dmitry Vyukov 2020-04-25 11:42 ` Paul Moore 0 siblings, 1 reply; 6+ messages in thread From: Dmitry Vyukov @ 2020-04-25 5:14 UTC (permalink / raw) To: Paul Moore, David Miller, kuba, netdev Cc: Stephen Smalley, Eric Paris, selinux, LKML, syzkaller, Willem de Bruijn On Fri, Apr 24, 2020 at 11:51 PM Paul Moore <paul@paul-moore.com> wrote: > > On Fri, Apr 24, 2020 at 4:27 AM Dmitry Vyukov <dvyukov@google.com> wrote: > > Hi SELinux maintainers, > > > > We've hit a case where a developer wasn't able to reproduce a kernel > > bug, it turned out to be a difference in behavior between SELinux and > > non-SELinux kernels. > > Condensed version: a program does sendmmsg on netlink socket with 2 > > mmsghdr's, first is completely empty/zeros, second contains some > > actual payload. Without SELinux the first mmsghdr is treated as no-op > > and the kernel processes the second one (triggers bug). However the > > SELinux hook does: > > > > static int selinux_netlink_send(struct sock *sk, struct sk_buff *skb) > > { > > if (skb->len < NLMSG_HDRLEN) { > > err = -EINVAL; > > goto out; > > } > > > > and fails processing on the first empty mmsghdr (does not happen > > without SELinux). > > > > Is this difference in behavior intentional/acceptable/should be fixed? > > From a practical perspective, SELinux is always going to need to do a > length check as it needs to peek into the netlink message header for > the message type so it can map that to the associated SELinux > permissions. So in that sense, the behavior is intentional and > desired; however from a bug-for-bug compatibility perspective ... not > so much. > > Ultimately, my it's-Friday-and-it's-been-a-long-week-ending-in-a-long-day > thought is that this was a buggy operation to begin with and the bug > was just caught in different parts of the kernel, depending on how it > was configured. It may not be ideal, but I can think of worse things > (and arguably SELinux is doing the Right Thing). +netlink maintainers for intended semantics of empty netlink messages If it's a bug, or intended behavior depends on the intended behavior... which I assume is not documented anywhere officially. However, most of the netlink families use netlink_rcv_skb, which does: int netlink_rcv_skb(struct sk_buff *skb, int (*cb)(struct sk_buff *, struct nlmsghdr *, struct netlink_ext_ack *)) { ... while (skb->len >= nlmsg_total_size(0)) { ... skb_pull(skb, msglen); } return 0; } 1. How intentional is this while loop logic vs sloppy error checking? 2. netlink_rcv_skb seems to be able to handle 2+ messages in the same skb, while selinux_netlink_send only checks the first one... so can I skip SELinux checks by putting a malicious message after a permitted one?.. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: selinux_netlink_send changes program behavior 2020-04-25 5:14 ` Dmitry Vyukov @ 2020-04-25 11:42 ` Paul Moore 2020-04-25 12:00 ` Dmitry Vyukov 0 siblings, 1 reply; 6+ messages in thread From: Paul Moore @ 2020-04-25 11:42 UTC (permalink / raw) To: Dmitry Vyukov, David Miller, kuba, netdev Cc: Stephen Smalley, Eric Paris, selinux, LKML, syzkaller, Willem de Bruijn On April 25, 2020 1:15:05 AM Dmitry Vyukov <dvyukov@google.com> wrote: > On Fri, Apr 24, 2020 at 11:51 PM Paul Moore <paul@paul-moore.com> wrote: >> >> On Fri, Apr 24, 2020 at 4:27 AM Dmitry Vyukov <dvyukov@google.com> wrote: >>> Hi SELinux maintainers, >>> >>> We've hit a case where a developer wasn't able to reproduce a kernel >>> bug, it turned out to be a difference in behavior between SELinux and >>> non-SELinux kernels. >>> Condensed version: a program does sendmmsg on netlink socket with 2 >>> mmsghdr's, first is completely empty/zeros, second contains some >>> actual payload. Without SELinux the first mmsghdr is treated as no-op >>> and the kernel processes the second one (triggers bug). However the >>> SELinux hook does: >>> >>> static int selinux_netlink_send(struct sock *sk, struct sk_buff *skb) >>> { >>> if (skb->len < NLMSG_HDRLEN) { >>> err = -EINVAL; >>> goto out; >>> } >>> >>> and fails processing on the first empty mmsghdr (does not happen >>> without SELinux). >>> >>> Is this difference in behavior intentional/acceptable/should be fixed? >> >> From a practical perspective, SELinux is always going to need to do a >> length check as it needs to peek into the netlink message header for >> the message type so it can map that to the associated SELinux >> permissions. So in that sense, the behavior is intentional and >> desired; however from a bug-for-bug compatibility perspective ... not >> so much. >> >> Ultimately, my it's-Friday-and-it's-been-a-long-week-ending-in-a-long-day >> thought is that this was a buggy operation to begin with and the bug >> was just caught in different parts of the kernel, depending on how it >> was configured. It may not be ideal, but I can think of worse things >> (and arguably SELinux is doing the Right Thing). > > +netlink maintainers for intended semantics of empty netlink messages > > If it's a bug, or intended behavior depends on the intended > behavior... which I assume is not documented anywhere officially. Your original email gave the impression that there was a big in the non-SELinux case; if that is not the case my response changes. > However, most of the netlink families use netlink_rcv_skb, which does: > > int netlink_rcv_skb(struct sk_buff *skb, int (*cb)(struct sk_buff *, > struct nlmsghdr *, > struct netlink_ext_ack *)) > { > ... > while (skb->len >= nlmsg_total_size(0)) { > ... > skb_pull(skb, msglen); > } > return 0; > } > > 1. How intentional is this while loop logic vs sloppy error checking? > 2. netlink_rcv_skb seems to be able to handle 2+ messages in the same > skb, while selinux_netlink_send only checks the first one... so can I > skip SELinux checks by putting a malicious message after a permitted > one?.. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: selinux_netlink_send changes program behavior 2020-04-25 11:42 ` Paul Moore @ 2020-04-25 12:00 ` Dmitry Vyukov 2020-04-28 1:53 ` Paul Moore 0 siblings, 1 reply; 6+ messages in thread From: Dmitry Vyukov @ 2020-04-25 12:00 UTC (permalink / raw) To: Paul Moore Cc: David Miller, kuba, netdev, Stephen Smalley, Eric Paris, selinux, LKML, syzkaller, Willem de Bruijn On Sat, Apr 25, 2020 at 1:42 PM Paul Moore <paul@paul-moore.com> wrote: > >> On Fri, Apr 24, 2020 at 4:27 AM Dmitry Vyukov <dvyukov@google.com> wrote: > >>> Hi SELinux maintainers, > >>> > >>> We've hit a case where a developer wasn't able to reproduce a kernel > >>> bug, it turned out to be a difference in behavior between SELinux and > >>> non-SELinux kernels. > >>> Condensed version: a program does sendmmsg on netlink socket with 2 > >>> mmsghdr's, first is completely empty/zeros, second contains some > >>> actual payload. Without SELinux the first mmsghdr is treated as no-op > >>> and the kernel processes the second one (triggers bug). However the > >>> SELinux hook does: > >>> > >>> static int selinux_netlink_send(struct sock *sk, struct sk_buff *skb) > >>> { > >>> if (skb->len < NLMSG_HDRLEN) { > >>> err = -EINVAL; > >>> goto out; > >>> } > >>> > >>> and fails processing on the first empty mmsghdr (does not happen > >>> without SELinux). > >>> > >>> Is this difference in behavior intentional/acceptable/should be fixed? > >> > >> From a practical perspective, SELinux is always going to need to do a > >> length check as it needs to peek into the netlink message header for > >> the message type so it can map that to the associated SELinux > >> permissions. So in that sense, the behavior is intentional and > >> desired; however from a bug-for-bug compatibility perspective ... not > >> so much. > >> > >> Ultimately, my it's-Friday-and-it's-been-a-long-week-ending-in-a-long-day > >> thought is that this was a buggy operation to begin with and the bug > >> was just caught in different parts of the kernel, depending on how it > >> was configured. It may not be ideal, but I can think of worse things > >> (and arguably SELinux is doing the Right Thing). > > > > +netlink maintainers for intended semantics of empty netlink messages > > > > If it's a bug, or intended behavior depends on the intended > > behavior... which I assume is not documented anywhere officially. > > Your original email gave the impression that there was a big in the non-SELinux case; if that is not the case my response changes. There is no bug... Well, there is a crash, but it is somewhere in the routing subsystem and is caused by the contents of the second netlink message. This is totally unrelated to this SELinux check and that crash is totally reproducible with SELinux as well if we just don't send the first empty message. The crux is really a difference in behavior in SELinux and non-SELinux cases. > > However, most of the netlink families use netlink_rcv_skb, which does: > > > > int netlink_rcv_skb(struct sk_buff *skb, int (*cb)(struct sk_buff *, > > struct nlmsghdr *, > > struct netlink_ext_ack *)) > > { > > ... > > while (skb->len >= nlmsg_total_size(0)) { > > ... > > skb_pull(skb, msglen); > > } > > return 0; > > } > > > > 1. How intentional is this while loop logic vs sloppy error checking? > > 2. netlink_rcv_skb seems to be able to handle 2+ messages in the same > > skb, while selinux_netlink_send only checks the first one... so can I > > skip SELinux checks by putting a malicious message after a permitted > > one?.. > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: selinux_netlink_send changes program behavior 2020-04-25 12:00 ` Dmitry Vyukov @ 2020-04-28 1:53 ` Paul Moore 0 siblings, 0 replies; 6+ messages in thread From: Paul Moore @ 2020-04-28 1:53 UTC (permalink / raw) To: Dmitry Vyukov Cc: David Miller, kuba, netdev, Stephen Smalley, Eric Paris, selinux, LKML, syzkaller, Willem de Bruijn On Sat, Apr 25, 2020 at 8:01 AM Dmitry Vyukov <dvyukov@google.com> wrote: > On Sat, Apr 25, 2020 at 1:42 PM Paul Moore <paul@paul-moore.com> wrote: > > >> On Fri, Apr 24, 2020 at 4:27 AM Dmitry Vyukov <dvyukov@google.com> wrote: > > >>> Hi SELinux maintainers, > > >>> > > >>> We've hit a case where a developer wasn't able to reproduce a kernel > > >>> bug, it turned out to be a difference in behavior between SELinux and > > >>> non-SELinux kernels. > > >>> Condensed version: a program does sendmmsg on netlink socket with 2 > > >>> mmsghdr's, first is completely empty/zeros, second contains some > > >>> actual payload. Without SELinux the first mmsghdr is treated as no-op > > >>> and the kernel processes the second one (triggers bug). However the > > >>> SELinux hook does: > > >>> > > >>> static int selinux_netlink_send(struct sock *sk, struct sk_buff *skb) > > >>> { > > >>> if (skb->len < NLMSG_HDRLEN) { > > >>> err = -EINVAL; > > >>> goto out; > > >>> } > > >>> > > >>> and fails processing on the first empty mmsghdr (does not happen > > >>> without SELinux). > > >>> > > >>> Is this difference in behavior intentional/acceptable/should be fixed? > > >> > > >> From a practical perspective, SELinux is always going to need to do a > > >> length check as it needs to peek into the netlink message header for > > >> the message type so it can map that to the associated SELinux > > >> permissions. So in that sense, the behavior is intentional and > > >> desired; however from a bug-for-bug compatibility perspective ... not > > >> so much. > > >> > > >> Ultimately, my it's-Friday-and-it's-been-a-long-week-ending-in-a-long-day > > >> thought is that this was a buggy operation to begin with and the bug > > >> was just caught in different parts of the kernel, depending on how it > > >> was configured. It may not be ideal, but I can think of worse things > > >> (and arguably SELinux is doing the Right Thing). > > > > > > +netlink maintainers for intended semantics of empty netlink messages > > > > > > If it's a bug, or intended behavior depends on the intended > > > behavior... which I assume is not documented anywhere officially. > > > > Your original email gave the impression that there was a big in the non-SELinux case; if that is not the case my response changes. > > > There is no bug... Well, there is a crash, but it is somewhere in the > routing subsystem and is caused by the contents of the second netlink > message. This is totally unrelated to this SELinux check and that > crash is totally reproducible with SELinux as well if we just don't > send the first empty message. > The crux is really a difference in behavior in SELinux and non-SELinux cases. We're taking a closer look from the SELinux side of things. -- paul moore www.paul-moore.com ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-04-28 1:54 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-24 8:27 selinux_netlink_send changes program behavior Dmitry Vyukov 2020-04-24 21:51 ` Paul Moore 2020-04-25 5:14 ` Dmitry Vyukov 2020-04-25 11:42 ` Paul Moore 2020-04-25 12:00 ` Dmitry Vyukov 2020-04-28 1:53 ` Paul Moore
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).