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 SO_PEERSEC instead of /proc #207

Closed
DemiMarie opened this issue Jul 30, 2019 · 10 comments
Closed

Use SO_PEERSEC instead of /proc #207

DemiMarie opened this issue Jul 30, 2019 · 10 comments

Comments

@DemiMarie
Copy link

DemiMarie commented Jul 30, 2019

If dbus-broker does not have access to other user’s files in /proc, it fails to access SELinux contexts. Furthermore, using /proc to access SELinux contexts of other processes is inherently racy.

@teg
Copy link
Contributor

teg commented Aug 1, 2019

Hmm, that's what we use. Could you give some more details?

@DemiMarie
Copy link
Author

DemiMarie commented Aug 2, 2019

When running with hidpid=2 for procfs, dbus-broker logged USER_AVC audit messages that appeard to be SELinux denials, but had empty strings in place of either scontext or tcontext. This resulted in the system failing to boot properly. This means that dbus-broker is trying to access files in /proc that it should not need access to. It is okay if it accesses them, but it should gracefully handle failing to do so.

@teg
Copy link
Contributor

teg commented Aug 3, 2019

Right, we use /proc in one case. See

* XXX: We need the seclabel to run the broker for 2 reasons: First,

We would like to avoid this, but short of a hard requirement on a fairly recent kernel I'm not sure how to.

There should not be a problem with a race here as we are only ever accessing the parent pid.

@DemiMarie
Copy link
Author

DemiMarie commented Aug 6, 2019

@teg This is still a problem, as dbus-broker may not have permission to access /proc/<ppid>. This is the case, for example, if hidpid=2 is a procfs mount option.

@DemiMarie
Copy link
Author

DemiMarie commented Aug 6, 2019

@teg Another option is to use SO_PEERSEC if possible, and then fallback only if that fails.

@dvdhrm
Copy link
Member

dvdhrm commented Oct 26, 2019

The way SO_PEERSEC was implememted on socketpairs does not allow to detect "whether it fails". It never failed, but instead returned an LSM-specific fallback. In my opinion, this behavior makes no sense and breaks any application trying to use LSMs on socketpairs. Hence, we changed the kernel behavior (see the comment in the code that @teg mentioned).

IOW, without hard-requiring a new kernel, I dont see how to avoid using /proc. Do you have a suggestion?

@dvdhrm
Copy link
Member

dvdhrm commented Feb 6, 2020

I am closing this as changing to SO_PEERSEC requires a new kernels which we currently cannot support. If there is interest in this, please feel free to reopen.

Note that the code already contains comments about the SO_PEERSEC vs /proc situation and will adapt as soon as we bump the kernel requirement.

@dvdhrm dvdhrm closed this as completed Feb 6, 2020
@DemiMarie
Copy link
Author

DemiMarie commented Feb 15, 2020

@dvdhrm can we do a runtime check for the kernel version, or detect if we got the fallback value?

@dvdhrm
Copy link
Member

dvdhrm commented Feb 17, 2020

@DemiMarie The fallback value is a valid value. We cannot deduce any information from it. At least not with explicit acknowledgement from LSM maintainers. And the runtime kernel-version-detection is a no-go. The kernel people explicitly recommend against it, due to backports.

How about I make this a configure option for meson? We might just end up requiring a recent kernel in a few months, anyway. I think this feature is now almost 2 years old, so that should be ok as a dependency.

@dvdhrm
Copy link
Member

dvdhrm commented Feb 17, 2020

@DemiMarie I prepared a PR (#222) that allows a smooth transition. This is not a hard-requirement for v4.17 yet (it actually is less than 2 years old and I haven't checked whether we can rely on it in the Fedora versions we support). Can you make use of this? I will try to merge it today, if @teg reviews it, and then include it in the release I make this week.

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

No branches or pull requests

3 participants