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

sysctl: enable coredump for suid binaries #15327

Merged
merged 1 commit into from Apr 7, 2020

Conversation

keszybz
Copy link
Member

@keszybz keszybz commented Apr 3, 2020

Right now the kernel will not dump anything that went through setuid or
setgid. But it is routine for daemons to do that, and it makes things hard to
debug.

systemd-coredump saves the coredump readable by the users the process was
running as. This should be enough to avoid information leakage. So let's also
tell the kernel to do the coredump.

For https://bugzilla.redhat.com/show_bug.cgi?id=1790972.

Both patterns are stored in the same file, so they are enabled or disabled
together. (Though suid_dumpable=2 is supposed to be safe even when writing to
plain files.)

@anitazha
Copy link
Member

anitazha commented Apr 3, 2020

systemd-coredump saves the coredump readable by the users the process was running as.

I thought the idea of suid_dumpable = 2 is that it's readable by root only so it avoids the security implications of having potentially privileged info leaked. Wouldn't making it readable by the user defeat the purpose?

@poettering
Copy link
Member

Hmm, can we extend the comment in the file about this, highlighting the impact of this change from the defaults?

Also, could you add a NEWS text for this at the same time, highlighting the same?

(I have the suspicion we should make the PR_SET_DUMPABLE prctl exposed through a per-service option, and similar for /proc/$PID/coredump_filter; see #6685)

@keszybz
Copy link
Member Author

keszybz commented Apr 4, 2020

I thought the idea of suid_dumpable = 2 is that it's readable by root only

Note: the kernel uses a different logic to determine ownership. "The core dump is owned by the filesystem user ID of the dumping process and no security is applied." systemd-coredump uses the effective uid of the running processes as owner.

I think it is OK to allow access to euid. Effectively, the euid would have access to this information when the process is running, and now it also gets access to some information in the coredump. If there are scenarios where this is not enough, then we should modify systemd-coredump to e.g. save the core only readable by uid=0. But even if we do such a change, it would be independent of this patch, or in other words, we'd need to do it even if we don't apply this one.

@keszybz
Copy link
Member Author

keszybz commented Apr 4, 2020

Hmm, can we extend the comment in the file about this, highlighting the impact of this change from the defaults?

I extended it a bit. If you think it should be even more, suggestions are welcome.

Also, could you add a NEWS text for this at the same time, highlighting the same?

Same here.

(I have the suspicion we should make the PR_SET_DUMPABLE prctl exposed through a per-service option, and similar for /proc/$PID/coredump_filter; see #6685)

We should, yeah.

Right now the kernel will not dump anything that went through setuid or
setgid. But it is routine for daemons to do that, and it makes things hard to
debug.

systemd-coredump saves the coredump readable by the users the process was
running as. This should be enough to avoid information leakage. So let's also
tell the kernel to do the coredump.

For https://bugzilla.redhat.com/show_bug.cgi?id=1790972.

Both patterns are stored in the same file, so they are enabled or disabled
together. (Though suid_dumpable=2 is supposed to be safe even when writing to
plain files.)
@keszybz
Copy link
Member Author

keszybz commented Apr 4, 2020

Updated with the typo fixed. Also see #15332.

@poettering poettering merged commit 6635f57 into systemd:master Apr 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants