Created attachment 2036696 [details] patch to replace atoi with strtol Description of problem: In the cockpit component pam-ssh-add uses a call to atoi() to convert the environment variable `SSH_AGENT_PID` to a integer. This assumes that this env variable has not been tampered with and it the env variable has been mangled this can lead to an overflow. The solution proposed is to replace it with a call to strtol() with error checking. Attached is a patch that does the same.
Luna told me on chat that this was discovered by Paolo Perego from SUSE's security team. Credit should go to him. This is set by pam_putenv() in start_agent(), in the same file. That part still runs as root. Sure, the user session process inherit this env variable and can change it, but I thought that these are copies, not the same memory as pam_putenv() uses. That would be rather wild, as these are different security domains. atoi() *by itself* does not crash on invalid inputs, it just doesn't return precise errors. If number conversion fails, then it returns 0, and then pam_sm_close_session() will not do anything. So *if* the user session can tamper with the pam handle's SSH_AGENT_PID, then the maximum possible exploit would be to kill arbitrary PIDs (i.e. local authenticated DoS). However, if that's possible, then the user session can also do heaven knows other things to corrupt the PAM handle.. Luna, Paolo: How can that be exploited? Do you have any details? Thanks!
so sorry for taking a while in theory one needs to set SSH_AGENT_PID in the file `$HOME/.pam_environment` during authentication for a user this is loaded and should replace any previously existing definition of the env var. for more information about .pam_environment. https://man7.org/linux/man-pages/man8/pam_env.8.html https://help.ubuntu.com/community/EnvironmentVariables#A.2BAH4-.2F.pam_environment
I've add Paolo to the issue
As a user, I created echo "SSH_AGENT_PID=pwned" > ~/.pam_environment The user_readenv option is deprecated, and not enabled in at least Fedora and RHEL 9. So I enabled it in Fedora 40 in /etc/pam.d/password-auth Curiously this seems to be so deprecated that it even segfaults in the PAM module: cockpit-session[1787]: pam_env(cockpit:session): deprecated reading of user environment enabled cockpit-session[1787]: segfault at 0 ip 00007f48a141901f sp 00007ffd73c0c7b0 error 4 in pam_env.so[7f48a1418000+3000] likely on CPU 0 (core 0, socket 0) This really isn't related to cockpit -- when I try to log in with ssh, it crashes in the same way. I'll report this, but that just means that the bug/exploit can't be tested in Fedora 40. user_readenv is enabled by default in Debian 12, and ssh'ing into the VM with the above ~/.pam_environment does give me $SSH_AGENT_PID=pwned in the session. I confirm that pam-ssh-add's pam_sm_close_session() does see that changed value. This is indeed a rather major surprise, and I figure that's the reason for that warning: Due to problematic security this functionality is deprecated since the 1.5.0 version and will be removed completely at some point in the future. So I can see the local privilege escalation/DoS: by creating a ~/.pam_environment file in some distributions, users can SIGTERM an arbitrary process in the system. However, I don't understand how this is related to an "integer overflow" -- the current code parses "pwned" (or any other non-number) as 0, which the code ignores. Moving to strtol() doesn't change anything wrt. to that other than producing an extra warning. But if the user specifies a valid positive value, then you could still kill an arbitrary process with that. To fix this in a better way, how about using pam_{set,get}_data() to store and retrieve the agent pid? That can't be tampered with by the user session.
This can be integer overflowed if you set the SSH_AGENT_PID to be extremely large. it can cause the overflow. Along with this yes we should move to pam_{set,get}_data() I'll add a patch later today to make this change as well. Should i submit a pr upstream as normal or do we want to handle this in a different manner?
> if you set the SSH_AGENT_PID to be extremely large. it can cause the overflow. Yes, but what does that *mean*? There's no crash or arbitrary code execution, a too high string value will just make atoi() return 0. I.e. representing this as an integer overflow is misleading IMHO -- it's a mild local privilege escalation (local authenticated DoS). I'm already working on a patch, currently testing it on Debian stable. Right now this bugzilla is private, so I won't send a (public) upstream PR) without your consent. As the impact is very low, I'm happy to handle this as a public issue without an embargo. What do you think?
I believe we should reach out to distributions who are vulnerable to this (at this point I believe it should only be debian) and confirm with them that they are okay with a public issue without an embargo. Otherwise I'm okay with making this issue public
Created attachment 2036918 [details] pam-ssh-add: Store agent pid in PAM data Tested patch. This is a bit involved as we now need to pick apart the KEY=VALUE format, but not too bad. With additional message() debugging I confirmed that the correct pid makes it all the way through. Paolo or RH Security team, can you please assign a CVE? Note that this is still private until Paolo/Luna tell us it's ok to publish. Thanks!
I haven't checked Ubuntu yet, and I'm not currently in a position to do so (in a train). Will check this afternoon. We should also drop the user_readenv option from tools/cockpit.debian.pam, as that's deprecated. But as of today, the option is still enabled in Debian testing.
Hi Martin, since you're upstream, I think RH Security Team can assign the CVE to this one.
I contacted the Debian and Ubuntu security teams if they want an embargo.
For SUSE I can say it would be nice to have the embargo
Debian responded and they don't need/want an embargo. Ubuntu hasn't responded. So Paolo, please define an embargo date. Please note that this makes validation of the patch hard: I can run a couple of tests locally, but we really need our full CI to fully validate it. In other words, we won't have an upstream release ready when the embargo lifts. I'll also defer the Fedora/RHEL/Debian releases until after it gets public and gets a new upstream release. But you can prepare an updated SUSE package already then. Security team, can we have a CVE here? I'm having a hard time finding the right bz user to ping, so adding BaseOS security for now.
Hi Martin, I found this responsible disclosure process I think we can follow: https://access.redhat.com/articles/red_hat_cna_vulnerability_disclosure_policy Since you're upstream, it's common it's you to define the coordinated release date of the vulnerability when the patch will be ready
As apparently the security team didn't actually see this bz, I asked secalert@ directly now. I also asked them whether they want an embargo. From my side I'd be ok with two weeks, so something like July 3? Does that work for you? (As I said, I don't really want/need one in the first place). I'll work on a more complete patch with an integration test case in the next days.
To me is absolute fine. @Luna is it ok to you July 3rd as disclosure date? Martin: are you (RH) issuing a CVE for this? Is it also possible to have the patch attached to this ticket so Luna can apply easily to the package he maintains at SUSE? Thank you so much Paolo
Paolo: Yes, I asked RH security for a CVE this morning. There is already a patch attached, but not yet with an integration test. I'll post both here ASAP.
This got assigned CVE-2024-6126.
To mop up this lose end: I reported the pam_env.so crash in Fedora as bug #2293045. The reproducer can still be tested on RHEL, Debian, Ubuntu.
Comment on attachment 2036696 [details] patch to replace atoi with strtol This doesn't address the root cause/vulnerability.
Created attachment 2037779 [details] pam-ssh-add: Store agent pid in PAM data [CVE-2024-6126] Updated patch. This fixes a crash, and adds an integration test. Locally tested against RHEL 9.4 and Debian testing (test doesn't work with Fedora as the pam option is broken, see above). I am reasonably confident in this now. However, I can't subject it to the full force of our CI until this goes public. Hence I will not actually land this anywhere until then. But you are welcome to apply the patch to OpenSUSE already.
Thank you so much. Are you (or RH security team) going to announce the CVE on oss-security mailing list, is it correct?
I'm not on that list. As the finder, can you do that?
RedHat Security team is on that list. Please can you ask them if they are going to publicly announce?
I think the patch is much more complicated than it needs to be: why not just store the pid in a global variable? pam_sm_close_session() runs in the same process, no?
Also: pants and suspenders: why not set the euid before killing the process? We run_as_user(), so why not kill_as_user()? Consider that the user could have killed the ssh-agent PID themselves and in the meantime this PID could have been reassigned to some other thing... perhaps the user even has control over that: they could create lots of processes and then cause the system to create their desired process with the target PID, possibly repeating the process many times to get it right, before finally logging out to take the selected process down...
I actually consider the PID reuse issue to be more serious since it doesn't depend on this deprecated/removed mode of another pam module with known security issues... This one is indisputably on us. I think I'd really like to see kill_as_user() here...
Hi, sorry I've taken a few days off for some university stuff. I'll be back around July 1st. July 3rd as a disclosure date works for me
Created attachment 2037812 [details] pam-ssh-add: Fix insecure killing of session ssh-agent [CVE-2024-6126] Thanks Allison for the review! Reworked accordingly and re-tested on RHEL 9.4 and Debian testing.
Some comments: - you store the gid, but don't use it - the setuid() might be easier if you fork() first and _exit() the child, but then you'd have to wait(). Matter of taste. - I was surprised to find the pw_uid saved, instead of just calling getuid(). Matter of taste. Otherwise this looks significantly better than the first round and I don't see any other obvious issues with it. On another topic: what's the rationale behind the embargo? It seems weird to announce this issue without being able to point to it having been fixed upstream.
(In reply to Allison Karlitskaya from comment #30) > Some comments: > - you store the gid, but don't use it Right, leftover. kill(2) says that it only looks at the uid, so not necessary to change the gid as well. Dropped. I'll attach an updated version. > - the setuid() might be easier if you fork() first and _exit() the child, > but then you'd have to wait(). Matter of taste. Too many moving parts for my taste. > - I was surprised to find the pw_uid saved, instead of just calling > getuid(). Matter of taste. Indeed setuid() is only called in setup_child(), so getuid() *should* work. auth_pwd is the target user, so conceptually it seems safer and cleaner to me to use that -- same as setup_child() does. > On another topic: what's the rationale behind the embargo? It seems weird > to announce this issue without being able to point to it having been fixed > upstream. Distributions often do that to coordinate patches and prepare updates so that they have fixed packages at the time of the announcement. I suppose that's what OpenSUSE wants to do. But that's a question for Paolo (who called for the embargo). Thanks!
Created attachment 2037822 [details] pam-ssh-add: Fix insecure killing of session ssh-agent [CVE-2024-6126]
Another few things after reading again: - the 'global' variables are actually file-scope and should be labelled 'static' - the error checking on `kill()` seems like it might be overdone, particularly considering that pid reuse is a normal thing that can happen. I'd drop the check and make this strictly "best" effort. - related to above: restoring root's uid really ought to be like a finally { } block after the kill() but if you hit the error case on kill() you return from the function with the uid still set as the user.
Created attachment 2037824 [details] pam-ssh-add: Fix insecure killing of session ssh-agent [CVE-2024-6126] Thanks Allison! Good points. Fixed.
Paolo: So that patch is now agreed upon. We'll apply it upstream on July 3 and make a new upstream release. In the meantime, feel free to prepare an OpenSUSE update with it.
Thank you Martin!
Subscribing Jelle, as he will review the upstream PR tomorrow. Jelle, this already went through around 5 rounds of reviews with the reporters and Lis. But please have a look at the patch and let's discuss questions/suggestions here. So far the plan is that I'll propose an upstream PR tomorrow with this patch (preferably around European morning), which we should land immediately, then cut a new release and do a blog post, and I'll update the Debian packages. Paolo, Luna, are you all set for a patched OpenSUSE release tomorrow? Do you want to do the publishing at a particular hour? (I don't think it's that important to time it that precisely, but as reporters you have the last word on that).
Release note draft, feedback appreciated! ## pam-ssh-add: Fix insecure killing of session ssh-agent [CVE-2024-6126] The [`pam_env` PAM module](https://man7.org/linux/man-pages/man8/pam_env.8.html) has a (deprecated) `user_readenv` option. Some distributions like Debian or Ubuntu enable this option by default, so Cockpit's PAM config in `/etc/pam.d/cockpit` enables it as well there. It's also possible to manually enable that option in other distributions such as Fedora or Red Hat Enterprise Linux. Cockpit's own `pam_ssh_add` module had a vulnerability when `user_readenv` is enabled. A local authenticated user could craft a `~/.pam_environment` file which would kill an arbitrary process on the system with root privileges when logging out of a Cockpit session. This could cause a Denial of Service. Version 320 fixes this problem. This is tracked as [CVE-2024-2947](https://www.cve.org/CVERecord?id=CVE-2024-2947). If you need to backport this to older cockpit versions, you can apply the [upstream patch](https://github.com/cockpit-project/cockpit/commit/XXXTODO). If you cannot install the new version or the patch, and you have potentially malicious local user on a system running Cockpit, you can mitigate this vulnerability by removing the `user_readenv=1` option from the `pam_env.so` line in `/etc/pam.d/cockpit`. This will disable reading any extra environment variables in user's `~/.pam_environment` files; the most common use of that is to set a local different from the system default. Many thanks to Paolo Perego <paolo.perego> for discovering, and Luna Dragon <luna.dragon> for reporting this issue!
(In reply to Martin Pitt from comment #37) > Subscribing Jelle, as he will review the upstream PR tomorrow. Jelle, this > already went through around 5 rounds of reviews with the reporters and Lis. > But please have a look at the patch and let's discuss questions/suggestions > here. > > So far the plan is that I'll propose an upstream PR tomorrow with this patch > (preferably around European morning), which we should land immediately, then > cut a new release and do a blog post, and I'll update the Debian packages. > Paolo, Luna, are you all set for a patched OpenSUSE release tomorrow? Do you > want to do the publishing at a particular hour? (I don't think it's that > important to time it that precisely, but as reporters you have the last word > on that). hi, can you give me a heads up just before you do this? I'd like to time pr's for older products where we don't have the freedom to update cockpit and then pull in the new release into everything else
Luna: Sure! Does 09:00 UTC / 11:00 CEST work for you? Also, I really don't mind if you make this public earlier than upstream, so just go ahead.
hi, 9:00 utc works for me. I'll push a fix to everything we can't update and I'll update the rest when the new release is made
9 utc works for me as well. Thank you for constant update and all the effort spent in coordinating and mitigation. Release note is ok to me. Kudos
Embargo is lifted. I sent https://github.com/cockpit-project/cockpit/pull/20698 with the upstream patch, and will see to land and release it quickly. I asked @ahanwate to make this bug public, as I can't. Thanks everyone, nice work together!
FEDORA-2024-9eb3674b7c (cockpit-320-1.fc39) has been submitted as an update to Fedora 39. https://bodhi.fedoraproject.org/updates/FEDORA-2024-9eb3674b7c
Also fixed in Fedora 40: https://bodhi.fedoraproject.org/updates/FEDORA-2024-d8bbe82ac1
FEDORA-2024-9eb3674b7c (cockpit-320-1.fc39) has been pushed to the Fedora 39 stable repository. If problem still persists, please make note of it in this bug report.