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

Fix systemd leak #213

Merged
merged 2 commits into from
Jul 28, 2020
Merged

Fix systemd leak #213

merged 2 commits into from
Jul 28, 2020

Conversation

nixbitcoin
Copy link
Member

Mitigates a security issue that allows unprivileged users to read other unprivileged user's processes' credentials from CGroup using systemctl status. Also adds tests to make sure this issue doesn't reoccur.

Mitigates a security issue that allows unprivileged users to read other
unprivileged user's processes' credentials from CGroup using `systemctl
status`.
Copy link
Member

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 6a8e29e

@jonasnick jonasnick merged commit 272b856 into fort-nix:master Jul 28, 2020
@erikarvstedt
Copy link
Collaborator

What exactly is the security issue here?
According to my understanding, all data returned by systemctl status is public information available to all users, e.g. via /proc.
Also, if this were an issue this should also be fixed upstream.

@jonasnick
Copy link
Member

systemctl status clightning would reveal the bitcoin-cli command including the -rpcpassword arguments in the cgroups process tree. I believe @nixbitcoin has a script to reproduce this.

@erikarvstedt
Copy link
Collaborator

The ARGV of all processes are public, e.g. via /proc/<PID>/cmdline.
If clightning leaks secrets through subprocess arguments, that's a clightning bug which can't be fixed by this PR.

@jonasnick
Copy link
Member

Doesn't work on my machine ("file not found"). It's prevented by security.hideProcessInformation = true;, which is part of the hardened kernel and set explicitly in this PR.

@jonasnick
Copy link
Member

If clightning leaks secrets through subprocess arguments

And yeah, that'd be bad and should be fixed upstream as well.

@erikarvstedt
Copy link
Collaborator

Ah, I missed that security.hideProcessInformation was enabled.
I also have to correct myself: Some process info like cmdline are only exposed by /proc and not by further means like syscalls.

This is indeed a systemd bug, great job catching this, @nixbitcoin!
Could you file a bug for systemd? GetUnitProcesses should only show info for the current user's unit when /proc access is restricted.

This PR still has some critical issues, which are fixed here.

@nixbitcoin
Copy link
Member Author

Could you file a bug for systemd? GetUnitProcesses should only show info for the current user's unit when /proc access is restricted.

Bug filed @ systemd/systemd#16825

@jonasnick
Copy link
Member

Bug filed @ ElementsProject/lightning#3984

@nixbitcoin nixbitcoin deleted the fix-systemd-leak branch March 3, 2021 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants