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.

Sign up for GitHub

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

Jump to bottom

Add an open-coded check for selinux to avoid linkage to libselinux/libpcre/libdl #6464

Closed
wants to merge 2 commits into from
Closed

Add an open-coded check for selinux to avoid linkage to libselinux/libpcre/libdl #6464

wants to merge 2 commits into from

Conversation

keszybz
Copy link
Member

@keszybz keszybz commented Jul 27, 2017

I'm not entirely sure why this works, but it has a pretty nice effect. Loading additional libraries into every process which uses the glibc resolver interface is not nice: it slows things down, and has potential security effects, by making it easier to exploit vulnerabilities. I would like this, or something to the same effect, to go in.

@keszybz
selinux-util: move mac_selinux_use to new file
9fcc96e
@keszybz
Add work-around to avoid libselinux/libpcre/libdl linkage in nss modules
8127cff 
With -Dselinux-workaround=true:
$ ldd build/libnss_mymachines.so.2
	linux-vdso.so.1 (0x00007fffec33e000)
	libpthread.so.0 => /lib64/libpthread.so.0 (0x00007f18404e4000)
	libc.so.6 => /lib64/libc.so.6 (0x00007f1840113000)
	librt.so.1 => /lib64/librt.so.1 (0x00007f183ff0b000)
	libcap.so.2 => /lib64/libcap.so.2 (0x00007f183fd06000)
	/lib64/ld-linux-x86-64.so.2 (0x00005612fe411000)
With -Dselinux-workaround=false:
	linux-vdso.so.1 (0x00007ffe5ad87000)
	libpthread.so.0 => /lib64/libpthread.so.0 (0x00007f107f893000)
	libc.so.6 => /lib64/libc.so.6 (0x00007f107f4c2000)
	librt.so.1 => /lib64/librt.so.1 (0x00007f107f2ba000)
	libcap.so.2 => /lib64/libcap.so.2 (0x00007f107f0b5000)
	libselinux.so.1 => /lib64/libselinux.so.1 (0x00007f107ee8d000)
	/lib64/ld-linux-x86-64.so.2 (0x000055ec087a0000)
	libpcre.so.1 => /lib64/libpcre.so.1 (0x00007f107ec19000)
	libdl.so.2 => /lib64/libdl.so.2 (0x00007f107ea15000)

The result is the same for nss-resolve and nss-systemd.

This is with gcc-7.1.1-3.fc26.x86_64, binutils-2.27-24.fc26.x86_64,
meson-0.41.1-1.fc26.noarch, libselinux-2.6-6.fc26.x86_64.

The sequence of calls from the nss modules to selinux is:

sd_bus_open_system → sd_bus_start → bus_start_address
→ bus_container_connect_socket → bus_socket_start_auth
→ bus_get_peercred → mac_selinux_use → is_selinux_enabled

It seems that eliminating is_selinux_enabled() is enough to allow gcc
to deduce that the other selinux functions are not necessary and drop the
selinux linkage. For some reason this only works when mac_selinux_use()
is defined in a seperate compilation unit.

The reason to cut down the library list is that those modules are loaded
everywhere, also for very short-lived programs, so every library adds a small
cost. Additionally, presence of additional libraries in memory — particularly
libdl — can be argued to make it easier to find code to reuse from exploits.
Such reduction should make the installation of our nss modules by default more
palatable to some folks.

This is a bit ugly, but might be worth the trouble.

Previously, in 8c78165 (and two earlier
commits) I replaced linking to libshared.la with libsystemd-internal.la
(which is now libsystemd_internal or libsystemd.a under meson), to remove
linkage to liblzma, liblz4, libgcrypt, libgpg-error, libacl, libidn, and
libseccomp. At the time Fedora builds were using libtool which misplaced the
--as-needed argument. Under meson it is placed correctly (before any libraries).
@martinpitt martinpitt mentioned this pull request Jul 28, 2017
Closed
@poettering
Copy link
Member

maybe we should go the other way, and try to remove that call chain into libselinux. For example, I think we could just invoke getpeersec() unconditionally, there's not really much of a cost involved with that, and we could even cache if it returns EOPNOTSUPP if there's value in suppressing this syscall on each connection. AFAICS it's the only place we call into the selinux code.

Or to say this differently: if the kernel allows us to query that data we should also be Ok with passing it on to our own caller, regardless if selinux is technically on or off...

@keszybz
Copy link
Member Author

keszybz commented Jul 31, 2017

I'll try that and see how the final linkage looks like. It's also possible that me might have to split libsystemd-bus into selinux and non-selinux parts. The whole things is a mess :(

keszybz added a commit to keszybz/systemd that referenced this pull request Dec 19, 2017
@keszybz
sd-bus: drop check for selinux before calling getsockopt(SO_PEERSEC)
db7d1dc 
Quoting Lennart Poettering in
systemd#6464 (comment):
> If the kernel allows us to query that data we should also be Ok with passing
> it on to our own caller, regardless if selinux is technically on or off...

The advantage is that this allows gcc to be smarter and reduce linkage:
(before)$ ldd build/libnss_systemd.so.2
	linux-vdso.so.1 (0x00007ffeb46ff000)
	librt.so.1 => /lib64/librt.so.1 (0x00007f2f60da6000)
	libcap.so.2 => /lib64/libcap.so.2 (0x00007f2f60ba1000)
	libselinux.so.1 => /lib64/libselinux.so.1 (0x00007f2f60978000)
	libpthread.so.0 => /lib64/libpthread.so.0 (0x00007f2f60759000)
	libc.so.6 => /lib64/libc.so.6 (0x00007f2f60374000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f2f61294000)
	libpcre2-8.so.0 => /lib64/libpcre2-8.so.0 (0x00007f2f600f0000)
	libdl.so.2 => /lib64/libdl.so.2 (0x00007f2f5feec000)
(after )$ ldd build/libnss_systemd.so.2
	linux-vdso.so.1 (0x00007ffe5f543000)
	librt.so.1 => /lib64/librt.so.1 (0x00007f427dcaa000)
	libcap.so.2 => /lib64/libcap.so.2 (0x00007f427daa5000)
	libpthread.so.0 => /lib64/libpthread.so.0 (0x00007f427d886000)
	libc.so.6 => /lib64/libc.so.6 (0x00007f427d4a1000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f427e196000)

Note that this only works in conjuction with the previous commit: either
of the two commits alone does not have the desired effect on linkage.

Replaces systemd#6464.
@keszybz keszybz closed this Dec 19, 2017
@keszybz keszybz deleted the selinux-workaround-1 branch December 19, 2017 14:27
Yamakuzure pushed a commit to elogind/elogind that referenced this pull request Jun 5, 2018
@keszybz @Yamakuzure
sd-bus: drop check for selinux before calling getsockopt(SO_PEERSEC)
b92a2af 
Quoting Lennart Poettering in
systemd/systemd#6464 (comment):
> If the kernel allows us to query that data we should also be Ok with passing
> it on to our own caller, regardless if selinux is technically on or off...

The advantage is that this allows gcc to be smarter and reduce linkage:
(before)$ ldd build/libnss_systemd.so.2
	linux-vdso.so.1 (0x00007ffeb46ff000)
	librt.so.1 => /lib64/librt.so.1 (0x00007f2f60da6000)
	libcap.so.2 => /lib64/libcap.so.2 (0x00007f2f60ba1000)
	libselinux.so.1 => /lib64/libselinux.so.1 (0x00007f2f60978000)
	libpthread.so.0 => /lib64/libpthread.so.0 (0x00007f2f60759000)
	libc.so.6 => /lib64/libc.so.6 (0x00007f2f60374000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f2f61294000)
	libpcre2-8.so.0 => /lib64/libpcre2-8.so.0 (0x00007f2f600f0000)
	libdl.so.2 => /lib64/libdl.so.2 (0x00007f2f5feec000)
(after )$ ldd build/libnss_systemd.so.2
	linux-vdso.so.1 (0x00007ffe5f543000)
	librt.so.1 => /lib64/librt.so.1 (0x00007f427dcaa000)
	libcap.so.2 => /lib64/libcap.so.2 (0x00007f427daa5000)
	libpthread.so.0 => /lib64/libpthread.so.0 (0x00007f427d886000)
	libc.so.6 => /lib64/libc.so.6 (0x00007f427d4a1000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f427e196000)

Note that this only works in conjuction with the previous commit: either
of the two commits alone does not have the desired effect on linkage.

Replaces #6464.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
needs-discussion 🤔 nss sd-bus selinux
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@keszybz @poettering