Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use target process context to set socket context #24994

Merged
merged 1 commit into from Oct 18, 2022

Conversation

tedx
Copy link

@tedx tedx commented Oct 13, 2022

Use target process context to set socket context when using SELinuxContextFromNet not systemds context. Currently when using the SELimuxContextFromNet option for a socket activated services systemd calls getcon_raw which returns init_t and uses the resulting context to compute the context to be passed to the setsockcreatecon call. A socket of type init_t is created and listened on and this means that SELinux policy cannot be written to control which processes (SELinux types) can connect to the socket since the ref policy allows all 'types' to connect to sockets of the type init_t. When security accessors see that any process can connect to a socket this raises serious concerns. I have spoken with SELinux contributors in person and on the mailing list and the consensus is that the best solution is to use the target executables context when computing the sockets context in all cases.

I had originally submitted this change as #24702 but inadvertently closed it when I sync'd my repo, sorry. The original request was waiting for msekletar to review, not sure when that was going to happen. That said I thought I had satisfied all of the requirements as I had gotten Karl MacMillian, a SELinux contributor, to review the PR. Hopefully this PR can be approved and merged soon.

@keszybz
Copy link
Member

keszybz commented Oct 18, 2022

In the future, please always put the full description of the commit in the commit, not in the github pr description.

--

This removes the branch that was added in 16115b0.
16115b0 did two things: it had the branch here in socket_determine_selinux_label() and a code in exec_child() to call label_get_child_mls_label(socket_fd, command->path, &label).

Before this patch, the flow was:

mac_selinux_get_child_mls_label:
  peercon = getpeercon_raw(socket_fd);
  if (!exec_label)
     exec_label = getfilecon_raw(exe);
  
socket_open_fds:
  if (params->selinux_context_net)                    #  
     label = mac_selinux_get_our_label();          #  this part is removed
  else                                                                 #
     label = mac_selinux_get_create_label_from_exe(path);
  socket_address_listen_in_cgroup(s, &p->address, label);

exec_child():
   exec_context = mac_selinux_get_child_mls_label(fd, executable, context->selinux_context);
   setexeccon(exec_context);

There was no explanation in the original patch. But removing the branch seems reasonable: we fall back to lookup for the exe path anyway. Since this is selinux, this change might break things. But I think it's fine to try this, because it seems much more reasonable. The reviews in #24702 were positive about the general approach. The issue of respecting SELinuxContext= was raised; this current version of the patch does that by the virtue of calling socket_determine_selinux_label().

Use target process context to set socket context when using SELinuxContextFromNet
not systemd's context. Currently when using the SELinuxContextFromNet option for
a socket activated services, systemd calls getcon_raw which returns init_t and
uses the resulting context to compute the context to be passed to the
setsockcreatecon call. A socket of type init_t is created and listened on and
this means that SELinux policy cannot be written to control which processes
(SELinux types) can connect to the socket since the ref policy allows all
'types' to connect to sockets of the type init_t. When security accessors see
that any process can connect to a socket this raises serious concerns. I have
spoken with SELinux contributors in person and on the mailing list and the
consensus is that the best solution is to use the target executables context
when computing the sockets context in all cases.

[zjs review/comment:

This removes the branch that was added in 16115b0.
16115b0 did two things: it had the branch here
in 'socket_determine_selinux_label()' and a code in 'exec_child()' to call
'label_get_child_mls_label(socket_fd, command->path, &label)'.

Before this patch, the flow was:
'''
mac_selinux_get_child_mls_label:
  peercon = getpeercon_raw(socket_fd);
  if (!exec_label)
     exec_label = getfilecon_raw(exe);

socket_open_fds:
  if (params->selinux_context_net)                 #
     label = mac_selinux_get_our_label();          #  this part is removed
  else                                             #
     label = mac_selinux_get_create_label_from_exe(path);
  socket_address_listen_in_cgroup(s, &p->address, label);

exec_child():
   exec_context = mac_selinux_get_child_mls_label(fd, executable, context->selinux_context);
   setexeccon(exec_context);
'''
]
@keszybz
Copy link
Member

keszybz commented Oct 18, 2022

jammy-ppc64e failed with: TEST-29-PORTABLE: FAIL (1208 s), unrelated.

@keszybz
Copy link
Member

keszybz commented Oct 18, 2022

Updated with the commit message adjusted. There were no other changes, and the CI passed previously, so I'll merge this now.

@keszybz keszybz merged commit 29dbc62 into systemd:main Oct 18, 2022
30 of 44 checks passed
@msekletar
Copy link
Contributor

FWIW, I also did the review of the patch I think it makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants