| Message ID | 20211104195949.135374-1-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [net] selinux: fix SCTP client peeloff socket labeling | expand |
On Thu, Nov 4, 2021 at 3:59 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > The commit referenced in the "Fixes" tag mistakenly attempted to > preserve the label of the peeloff socket that had been set in > selinux_socket_post_create() in the case of a client socket. However, > the peeloff socket should in fact just inherit the label from the parent > socket. In practice these labels are usually the same, but they may > differ when setsockcreatecon(3) or socket type transition rules are > involved. > > The fact that selinux_socket_[post_]create() are called on the peeloff > socket is actually not what should be happening (it is a side effect of > sctp_do_peeloff() using socket_create() to create the socket, which > calls the aforementioned LSM hooks). A patch to fix this is being worked > on. > > In the meanwhile, at least fix sctp_assoc_established() to set > asoc->secid to the socket's sid and selinux_sctp_sk_clone() to > unconditionally get the peeloff socket's sid from asoc->secid, which > will ensure that the peeloff socket gets the right label in case of both > client and server SCTP socket. The label set by > selinux_socket_post_create() will be simply overwritten in both cases, > as was the case before the commit this patch is fixing. > > Passed both the selinux-testsuite (with client peeloff tests added) and > the SCTP functional test suite from lksctp-tools. > > Fixes: e7310c94024c ("security: implement sctp_assoc_established hook in selinux") > Based-on-patch-by: Xin Long <lucien.xin@gmail.com> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > --- > > As agreed with Xin Long, I'm posting this fix up instead of him. I am > now fairly convinced that this is the right way to deal with the > immediate problem of client peeloff socket labeling. I'll work on > addressing the side problem regarding selinux_socket_post_create() > being called on the peeloff sockets separately. > > Please don't merge this patch without an ack from Paul, as it seems > we haven't reached an overall consensus yet. > > security/selinux/hooks.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) When we change things as significantly as we are doing here, i.e. shifting some of the labeling away from the endpoint to the association, I much rather we do it as a chunk/patchset so that we can review it in a consistent manner. Some of that has gone out the door here because of what I view as recklessness on the part of the netdev folks, but that doesn't mean we need to abandon all order. Let's get all the fixes and repairs queued up in a single patchset so that we can fully see what the end result of these changes are going to look like. Further, I think it would be good if at least one of the patches has a very clear explanation in the commit description (not the cover letter, I want to see this in the git log) of what happens with respect to labeling on the server side, the client side, during socket peeloffs on both ends, and how multiple associations are handled. My hope is that this should give us all a more consistent view, which will be very important moving forward if netdev is going to act independent of the other subsystems.
From: Paul Moore <paul@paul-moore.com> Date: Sun, 7 Nov 2021 09:12:57 -0500 > > When we change things as significantly as we are doing here, i.e. > shifting some of the labeling away from the endpoint to the > association, I much rather we do it as a chunk/patchset so that we can > review it in a consistent manner. Some of that has gone out the door > here because of what I view as recklessness on the part of the netdev > folks, but that doesn't mean we need to abandon all order. Let's get > all the fixes and repairs queued up in a single patchset so that we > can fully see what the end result of these changes are going to look > like. Further, I think it would be good if at least one of the > patches has a very clear explanation in the commit description (not > the cover letter, I want to see this in the git log) of what happens Cover letters show up in the merge commit log message for the patch series so they show up in the git commit log. Paul, please stop being so difficult and let's fix this. Thank you.
On Sun, Nov 7, 2021 at 2:10 PM David Miller <davem@davemloft.net> wrote: > From: Paul Moore <paul@paul-moore.com> > Date: Sun, 7 Nov 2021 09:12:57 -0500 > > When we change things as significantly as we are doing here, i.e. > > shifting some of the labeling away from the endpoint to the > > association, I much rather we do it as a chunk/patchset so that we can > > review it in a consistent manner. Some of that has gone out the door > > here because of what I view as recklessness on the part of the netdev > > folks, but that doesn't mean we need to abandon all order. Let's get > > all the fixes and repairs queued up in a single patchset so that we > > can fully see what the end result of these changes are going to look > > like. Further, I think it would be good if at least one of the > > patches has a very clear explanation in the commit description (not > > the cover letter, I want to see this in the git log) of what happens > > Cover letters show up in the merge commit log message for the patch > series so they show up in the git commit log. That assumes the patch(set) is merged and not applied directly from email, patchwork, etc. I try not to make too many assumptions about how patches end up in various trees as everyone/every-tree is a bit different; including the details in a commit description has been the safest route in my experience. Regardless, the key is that info gets into the git log in a way that is easily discoverable, the exact mechanism is less important as far as I'm concerned. > Paul, please stop being so difficult and let's fix this. David, please look at the associated threads and see that we *are* working on fixing this; no one likes broken code, we all want this fixed. As far as being difficult is concerned, I can assure that from my perspective the individual who merged a patchset in less than 24 hours (during the initial days of the merge window) without an ACK from the affected maintainers and then refused to revert the patches when asked is the difficult party, but I guess we all have our different opinions. Best wishes and warmest regards.
On Thu, 4 Nov 2021 20:59:49 +0100 Ondrej Mosnacek wrote: > As agreed with Xin Long, I'm posting this fix up instead of him. I am > now fairly convinced that this is the right way to deal with the > immediate problem of client peeloff socket labeling. I'll work on > addressing the side problem regarding selinux_socket_post_create() > being called on the peeloff sockets separately. IIUC Paul would like to see this part to come up in the same series. Any predictions when such series would materialize?
On Tue, Nov 9, 2021 at 9:21 AM Jakub Kicinski <kuba@kernel.org> wrote: > On Thu, 4 Nov 2021 20:59:49 +0100 Ondrej Mosnacek wrote: > > As agreed with Xin Long, I'm posting this fix up instead of him. I am > > now fairly convinced that this is the right way to deal with the > > immediate problem of client peeloff socket labeling. I'll work on > > addressing the side problem regarding selinux_socket_post_create() > > being called on the peeloff sockets separately. > > IIUC Paul would like to see this part to come up in the same series. Just to reaffirm the IIUC part - yes, your understanding is correct.
On Tue, Nov 9, 2021 at 4:00 PM Paul Moore <paul@paul-moore.com> wrote: > On Tue, Nov 9, 2021 at 9:21 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 4 Nov 2021 20:59:49 +0100 Ondrej Mosnacek wrote: > > > As agreed with Xin Long, I'm posting this fix up instead of him. I am > > > now fairly convinced that this is the right way to deal with the > > > immediate problem of client peeloff socket labeling. I'll work on > > > addressing the side problem regarding selinux_socket_post_create() > > > being called on the peeloff sockets separately. > > > > IIUC Paul would like to see this part to come up in the same series. > > Just to reaffirm the IIUC part - yes, your understanding is correct. The more I'm reading these threads, the more I'm getting confused... Do you insist on resending the whole original series with modifications? Or actual revert patches + the new patches? Or is it enough to revert/resend only the patches that need changes? Do you also insist on the selinux_socket_post_create() thing to be fixed in the same series? Note that the original patches are still in the net.git tree and it doesn't seem like Dave will want to rebase them away, so it seems explicit reverting is the only way to "respin" the series... Regardless of the answers, this thing has rabbithole'd too much and I'm already past my free cycles to dedicate to this, so I think it will take me (and Xin) some time to prepare the corrected and re-documented patches. Moreover, I think I realized how to to deal with the peer_secid-vs.-multiple-assocs-on-one-socket problem that Xin mentions in patch 4/4, fixing which can't really be split out into a separate patch and will need some test coverage, so I don't think I can rush this up at this point... In the short term, I'd suggest either reverting patches 3/4 and 4/4 (which are the only ones that would need re-doing; the first two are good changes on their own) or leaving everything as is (it's not functionally worse than what we had before...) and waiting for the proper fixes. -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
On Thu, Nov 11, 2021 at 7:59 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > On Tue, Nov 9, 2021 at 4:00 PM Paul Moore <paul@paul-moore.com> wrote: > > On Tue, Nov 9, 2021 at 9:21 AM Jakub Kicinski <kuba@kernel.org> wrote: > > > On Thu, 4 Nov 2021 20:59:49 +0100 Ondrej Mosnacek wrote: > > > > As agreed with Xin Long, I'm posting this fix up instead of him. I am > > > > now fairly convinced that this is the right way to deal with the > > > > immediate problem of client peeloff socket labeling. I'll work on > > > > addressing the side problem regarding selinux_socket_post_create() > > > > being called on the peeloff sockets separately. > > > > > > IIUC Paul would like to see this part to come up in the same series. > > > > Just to reaffirm the IIUC part - yes, your understanding is correct. > > The more I'm reading these threads, the more I'm getting confused... > Do you insist on resending the whole original series with > modifications? Or actual revert patches + the new patches? Or is it > enough to revert/resend only the patches that need changes? Do you > also insist on the selinux_socket_post_create() thing to be fixed in > the same series? Note that the original patches are still in the > net.git tree and it doesn't seem like Dave will want to rebase them > away, so it seems explicit reverting is the only way to "respin" the > series... DaveM is stubbornly rejecting the revert requests so for now I would continue to base any patches on top of the netdev tree. If that changes we can reconcile any changes as necessary, that should not be a major issue. As far as what I would like to see from the patches, ignoring the commit description vs cover letter discussion, I would like to see patches that fix all of the known LSM/SELinux/SCTP problems as have been discussed over the past couple of weeks. Even beyond this particular issue I generally disapprove of partial fixes to known problems; I would rather see us sort out all of the issues in a single patchset so that we can review everything in a sane manner. In this particular case things are a bit more complicated because of the current state of the patches in the netdev tree, but as mentioned above just treat the netdev tree as broken and base your patches on that with all of the necessary "Fixes:" metadata and the like. > Regardless of the answers, this thing has rabbithole'd too much and > I'm already past my free cycles to dedicate to this, so I think it > will take me (and Xin) some time to prepare the corrected and > re-documented patches. Moreover, I think I realized how to to deal > with the peer_secid-vs.-multiple-assocs-on-one-socket problem that Xin > mentions in patch 4/4, fixing which can't really be split out into a > separate patch and will need some test coverage, so I don't think I > can rush this up at this point... It's not clear to me from your comments above if this is something you are currently working on, planning to work on soon, or giving up on in the near term. Are we able to rely on you for a patchset to fix this or are you unable to see this through at this time? > In the short term, I'd suggest > either reverting patches 3/4 and 4/4 (which are the only ones that > would need re-doing; the first two are good changes on their own) or > leaving everything as is (it's not functionally worse than what we had > before...) and waiting for the proper fixes. DaveM has thus far failed to honor the revert request so it doesn't appear that reverting 3/4 and 4/4 is an option. In the near term that leaves us with the other two options: leave it as-is or fix it properly. I am firmly in the fix it properly camp, regardless of the revert state, so that is the direction I would like to see things go.
On Thu, Nov 11, 2021 at 4:44 PM Paul Moore <paul@paul-moore.com> wrote: > On Thu, Nov 11, 2021 at 7:59 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Tue, Nov 9, 2021 at 4:00 PM Paul Moore <paul@paul-moore.com> wrote: > > > On Tue, Nov 9, 2021 at 9:21 AM Jakub Kicinski <kuba@kernel.org> wrote: > > > > On Thu, 4 Nov 2021 20:59:49 +0100 Ondrej Mosnacek wrote: > > > > > As agreed with Xin Long, I'm posting this fix up instead of him. I am > > > > > now fairly convinced that this is the right way to deal with the > > > > > immediate problem of client peeloff socket labeling. I'll work on > > > > > addressing the side problem regarding selinux_socket_post_create() > > > > > being called on the peeloff sockets separately. > > > > > > > > IIUC Paul would like to see this part to come up in the same series. > > > > > > Just to reaffirm the IIUC part - yes, your understanding is correct. > > > > The more I'm reading these threads, the more I'm getting confused... > > Do you insist on resending the whole original series with > > modifications? Or actual revert patches + the new patches? Or is it > > enough to revert/resend only the patches that need changes? Do you > > also insist on the selinux_socket_post_create() thing to be fixed in > > the same series? Note that the original patches are still in the > > net.git tree and it doesn't seem like Dave will want to rebase them > > away, so it seems explicit reverting is the only way to "respin" the > > series... > > DaveM is stubbornly rejecting the revert requests so for now I would > continue to base any patches on top of the netdev tree. If that > changes we can reconcile any changes as necessary, that should not be > a major issue. > > As far as what I would like to see from the patches, ignoring the > commit description vs cover letter discussion, I would like to see > patches that fix all of the known LSM/SELinux/SCTP problems as have > been discussed over the past couple of weeks. Even beyond this > particular issue I generally disapprove of partial fixes to known > problems; I would rather see us sort out all of the issues in a single > patchset so that we can review everything in a sane manner. In this > particular case things are a bit more complicated because of the > current state of the patches in the netdev tree, but as mentioned > above just treat the netdev tree as broken and base your patches on > that with all of the necessary "Fixes:" metadata and the like. Hm, okay, that isn't what I was branch-predicting from your other responses, but works for me. > > Regardless of the answers, this thing has rabbithole'd too much and > > I'm already past my free cycles to dedicate to this, so I think it > > will take me (and Xin) some time to prepare the corrected and > > re-documented patches. Moreover, I think I realized how to to deal > > with the peer_secid-vs.-multiple-assocs-on-one-socket problem that Xin > > mentions in patch 4/4, fixing which can't really be split out into a > > separate patch and will need some test coverage, so I don't think I > > can rush this up at this point... > > It's not clear to me from your comments above if this is something you > are currently working on, planning to work on soon, or giving up on in > the near term. Are we able to rely on you for a patchset to fix this > or are you unable to see this through at this time? I'm still working on it, but I'll need to juggle it with other things now and I might need to defer it completely at some point. I don't think I'll have all the patches ready sooner than two weeks from now. My point was to explain that I'm unable to provide a proper fix in the short term and don't want to block the netdev maintainers in case they were waiting for this to get resolved before e.g. sending a PR to Linus. -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
On Fri, Nov 12, 2021 at 4:53 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Thu, Nov 11, 2021 at 4:44 PM Paul Moore <paul@paul-moore.com> wrote: > > On Thu, Nov 11, 2021 at 7:59 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > On Tue, Nov 9, 2021 at 4:00 PM Paul Moore <paul@paul-moore.com> wrote: > > > > On Tue, Nov 9, 2021 at 9:21 AM Jakub Kicinski <kuba@kernel.org> wrote: > > > > > On Thu, 4 Nov 2021 20:59:49 +0100 Ondrej Mosnacek wrote: > > > > > > As agreed with Xin Long, I'm posting this fix up instead of him. I am > > > > > > now fairly convinced that this is the right way to deal with the > > > > > > immediate problem of client peeloff socket labeling. I'll work on > > > > > > addressing the side problem regarding selinux_socket_post_create() > > > > > > being called on the peeloff sockets separately. > > > > > > > > > > IIUC Paul would like to see this part to come up in the same series. > > > > > > > > Just to reaffirm the IIUC part - yes, your understanding is correct. > > > > > > The more I'm reading these threads, the more I'm getting confused... > > > Do you insist on resending the whole original series with > > > modifications? Or actual revert patches + the new patches? Or is it > > > enough to revert/resend only the patches that need changes? Do you > > > also insist on the selinux_socket_post_create() thing to be fixed in > > > the same series? Note that the original patches are still in the > > > net.git tree and it doesn't seem like Dave will want to rebase them > > > away, so it seems explicit reverting is the only way to "respin" the > > > series... > > > > DaveM is stubbornly rejecting the revert requests so for now I would > > continue to base any patches on top of the netdev tree. If that > > changes we can reconcile any changes as necessary, that should not be > > a major issue. > > > > As far as what I would like to see from the patches, ignoring the > > commit description vs cover letter discussion, I would like to see > > patches that fix all of the known LSM/SELinux/SCTP problems as have > > been discussed over the past couple of weeks. Even beyond this > > particular issue I generally disapprove of partial fixes to known > > problems; I would rather see us sort out all of the issues in a single > > patchset so that we can review everything in a sane manner. In this > > particular case things are a bit more complicated because of the > > current state of the patches in the netdev tree, but as mentioned > > above just treat the netdev tree as broken and base your patches on > > that with all of the necessary "Fixes:" metadata and the like. > > Hm, okay, that isn't what I was branch-predicting from your other > responses, but works for me. If the past few years has shown us anything it is that branch prediction is ... problematic :) In an attempt to make my thinking a bit more clear, I see this as two issues: the first, and most important, is ultimately getting the SCTP/LSM/SELinux controls working properly, e.g. all the fixes you are currently working on; the second issue is the current state of the various kernel trees. Regardless of what happens with the second issue, I don't want it to detract from efforts on the first; making sure we fix the bugs is paramount. As you work on the patches to fix things, my suggestion is to take Xin Long's patches and drop them on top of some known-good kernel and use that as a base for your development. Earlier when I was looking at them I applied Xin Long's patches to the selinux/stable-5.16 branch via the selinux-pr-20211101 tag and they applied cleanly, I suspect you could do the same with the v5.15 tag from Linus tree. Focus on your patches, I'll help resolve any (re)base/merging issues they may arise once you have things sorted out. Unfortunately, I was greeted by an email notification this morning that Xin Long's patches have made their way into Linus tree. I thought these patches were going to stay in -next for the v5.16-rcX timeframe, I didn't realize the netdev folks were planning to submit these for v5.16. I wasn't happy with the patches in linux-next, I'm definitely not happy with them in Linus tree; this is something we will need to deal with, but to be clear, regardless of what happens in Linus' tree please keep working on the fixes. > > > Regardless of the answers, this thing has rabbithole'd too much and > > > I'm already past my free cycles to dedicate to this, so I think it > > > will take me (and Xin) some time to prepare the corrected and > > > re-documented patches. Moreover, I think I realized how to to deal > > > with the peer_secid-vs.-multiple-assocs-on-one-socket problem that Xin > > > mentions in patch 4/4, fixing which can't really be split out into a > > > separate patch and will need some test coverage, so I don't think I > > > can rush this up at this point... > > > > It's not clear to me from your comments above if this is something you > > are currently working on, planning to work on soon, or giving up on in > > the near term. Are we able to rely on you for a patchset to fix this > > or are you unable to see this through at this time? > > I'm still working on it, but I'll need to juggle it with other things > now and I might need to defer it completely at some point. I don't > think I'll have all the patches ready sooner than two weeks from now. > My point was to explain that I'm unable to provide a proper fix in the > short term ... Thanks for the clarification and your continued work on this. If something happens and you are not able to continue to make progress on this please let me know so I can pick this up. As you know, we need to get this fixed. > ... and don't want to block the netdev maintainers in case they > were waiting for this to get resolved before e.g. sending a PR to > Linus. It's a bit of a moot point now (see above), but I *definitely* want to block the patches that are in the netdev tree from hitting Linus tree. I can see a middle ground that you described for keeping patches 1/4 and 2/4, but I see the remaining two patches as problematic and not something we want in a released kernel from Linus. If we had guarantees that we could have this fixed before v5.16 is released I might be inclined to let this slide, but considering your estimate of a minimum of two weeks of additional development, the potential need for further respins depending on review, the holiday season, etc. it seems like we need to appeal to Linus to do at least a partial revert for now. I'll send some email on this to Linus and the relevant mailing lists, but I need to do the revert and do some testing on it to make sure it's sane.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 5e5215fe2e83..5d9da4662449 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -5502,8 +5502,7 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk if (!selinux_policycap_extsockclass()) return selinux_sk_clone_security(sk, newsk); - if (asoc->secid != SECSID_WILD) - newsksec->sid = asoc->secid; + newsksec->sid = asoc->secid; newsksec->peer_sid = asoc->peer_secid; newsksec->sclass = sksec->sclass; selinux_netlbl_sctp_sk_clone(sk, newsk); @@ -5566,7 +5565,7 @@ static void selinux_sctp_assoc_established(struct sctp_association *asoc, selinux_inet_conn_established(asoc->base.sk, skb); asoc->peer_secid = sksec->peer_sid; - asoc->secid = SECSID_WILD; + asoc->secid = sksec->sid; } static int selinux_secmark_relabel_packet(u32 sid)
The commit referenced in the "Fixes" tag mistakenly attempted to preserve the label of the peeloff socket that had been set in selinux_socket_post_create() in the case of a client socket. However, the peeloff socket should in fact just inherit the label from the parent socket. In practice these labels are usually the same, but they may differ when setsockcreatecon(3) or socket type transition rules are involved. The fact that selinux_socket_[post_]create() are called on the peeloff socket is actually not what should be happening (it is a side effect of sctp_do_peeloff() using socket_create() to create the socket, which calls the aforementioned LSM hooks). A patch to fix this is being worked on. In the meanwhile, at least fix sctp_assoc_established() to set asoc->secid to the socket's sid and selinux_sctp_sk_clone() to unconditionally get the peeloff socket's sid from asoc->secid, which will ensure that the peeloff socket gets the right label in case of both client and server SCTP socket. The label set by selinux_socket_post_create() will be simply overwritten in both cases, as was the case before the commit this patch is fixing. Passed both the selinux-testsuite (with client peeloff tests added) and the SCTP functional test suite from lksctp-tools. Fixes: e7310c94024c ("security: implement sctp_assoc_established hook in selinux") Based-on-patch-by: Xin Long <lucien.xin@gmail.com> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> --- As agreed with Xin Long, I'm posting this fix up instead of him. I am now fairly convinced that this is the right way to deal with the immediate problem of client peeloff socket labeling. I'll work on addressing the side problem regarding selinux_socket_post_create() being called on the peeloff sockets separately. Please don't merge this patch without an ack from Paul, as it seems we haven't reached an overall consensus yet. security/selinux/hooks.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)