Summary
https://github.com/uber/pam-ussh was open-sourced today (kudos!) and is presumably used within Uber's infrastructure. This is a PAM module written a Go that "will authenticate a user based on them having an ssh certificate in their ssh-agent signed by a specified ssh CA." A cursory look at the code and the documentation reveals that the module trusts the SSH_AUTH_SOCK environment variable to determine the Unix domain socket it will use to talk to ssh-agent:
https://github.com/uber/pam-ussh/blob/a50585bb7a0f16cd4e813509ad1393731cbb9a14/pam_ussh.go#L70
The documentation suggests configuring the module for sudo's PAM stack. Since sudo is normally installed SUID root and invokes its PAM stacks with root privileges, it may happily talk to another user's ssh-agent via that other user's and agent's Unix domain socket, despite of this socket not being accessible to the invoking user directly (since it's protected with Unix file permissions). Being an environment variable, SSH_AUTH_SOCK is under control of the user/attacker invoking sudo. In other words, the code and suggested setup effectively bypass Unix file permissions on ssh-agent sockets.
This appears to be a design error of pam-ussh.
Security Impact
A local user may trick pam-ussh and thus sudo to authenticate itself using another local user's certificate. This other user has to be currently logged in, with ssh-agent active. (The attack script may wait for this condition to be met.) The attacking user doesn't have to possess any certificate at all, nor use ssh-agent themselves. In fact, the attack may likely be mounted even by a compromised system service pseudo-user, provided that sudo is within its reach.
Reproduction Steps
Not having access to a setup that Uber actually uses within its infrastructure, I can only be moderately confident that this security issue applies and these steps will work. That said, the reproduction steps may be roughly as follows:
-
Login (over SSH or otherwise) as a non-privileged user to a shell account on a system with pam-ussh deployed for sudo.
-
Check the process list and directories matching /tmp/ssh-* for instances of ssh-agent and their sockets corresponding to other logged in users. If none, then wait until this changes.
-
Infer the target user's ssh-agent socket pathname by listing /tmp/ssh-* and obtaining the PID from the process list. The pathname may be of the form /tmp/ssh-RND/agent.PID, where the "RND" and "PID" portions are determined from the directory listing and the process list, respectively.
-
Invoke a command like "SSH_AUTH_SOCK=/tmp/ssh-RND/agent.PID sudo bash" with the "RND" and "PID" substituted with substrings identified above.
Specifics
- If applicable, what account were you using to test?
- N/A
- If applicable, what domain(s) does this vulnerability affect?
- Unclear - it's internal infrastructure that might be in use behind a variety of Uber services
- Does this only affect specific versions or vendors?
- pam-ussh at least as published on GitHub as of today (Feb 8, 2017)
This might or might not be within scope of Uber's bug bounty program, and I have not actually reproduced the issue - only having identified its likelihood through the source code and documentation. Yet I figured I'd give this communication channel a try, and report this. I'd appreciate a review of these findings and any feedback you might provide.
A related concern is that Go's runtime might not be suitable for use in SUID/SGID programs yet - e.g., it might also have environment variables it trusts too much, as well as many other issues that libc's have been hardened against over the years (and decades). I have not looked into that yet, but you might want to.
Hey @solardiz thank you for this report, our first one for the newly-open sourced pam-ussh module! This is not my area of expertise so I have forwarded your concern over to the internal team. I will let you know as soon as I hear back from them.
Best of luck on the rest of the bug bounty program!
Hey @solardiz the author confirms the issue and is working on a fix.
Hi @dnathe4th! Thank you for the timely & clear communication on this issue.
If the author likes and doesn't mind sharing, I'd be happy to comment on possible approaches to fixing the issue that they might be considering. One approach could be to have the PAM module temporarily switch the euid to the user's. Another could be to verify the Unix domain socket ownership (preferably in a race-free manner, although in this case there are mitigating factors already). Or both. There are other approaches as well, but those feel more invasive (would be harder to deploy).
Hey @solardiz ! We appreciate the recommendations and I have forwarded them on to the author. I believe he already has a fix in progress but additional suggestions are always welcome. I will reach back out next update I get from him.
Hey @solardiz ! The author and team are working through a proposed solution to your report. They are still discussing how to avoid the race condition currently present in the attached patch, but asked me to send it your way in case you had additional feedback for this in-progress work.
Thank you for taking the time to work through this with us!
Thanks for sharing the patch, @dnathe4th.
Meanwhile, I've been thinking of how to fix this issue best, and I came to the conclusion that the straightforward approach, as this patch takes, results in races that appear unavoidable within traditional Unix syscall semantics. Traditionally, code trying to access untrusted pathnames yet running with excessive privileges uses pre- and post-checks to make sure the file opened is actually the file that was meant to be opened. This would also work for a FIFO, but it doesn't work for Unix domain sockets. Besides, with that approach other (relatively minor) problems traditionally remain: potential side-effects on open(), or in this case on connect() - e.g. another/unintended service receiving a connection attempt, and these attempts/checks serving as an oracle to probe for existence of filesystem entries (not necessarily sockets, and anywhere among mounted filesystems) in directories that the invoking user couldn't have accessed directly.
In other words, when fixing the main issue (the one with authentication) we should also have in mind that this is privileged code that might have side-effects and/or infoleaks unrelated to its purpose, and the straightforward approach in fact keeps the code problematic in those ways - as well as, and more importantly, in not fully fixing the original issue because of the races, which probably allow for the wrong socket to be accessed again, through the attacker replacing their dummy socket or its holding directory with a (sym)link to the victim's directory/socket. Even if the code post-checked the pathname again, that would be no guarantee the attacker had not replaced the directory/socket back. The attacker can probably manage to obtain the same st_ino, too. (At least with sequential inode number allocation.)
So I recommend that you temporarily set euid to the target user's id, which in case of running from sudo should match the current ruid, and then after connecting to the socket (or failing) set euid back to whatever it was. The latter operation should work due to saved IDs. For example, see our non-fsuid implementaiton of ch_uid() here:
http://cvsweb.openwall.com/cgi/cvsweb.cgi/Owl/packages/tcb/tcb/libs/libtcb.c?rev=1.9
(If you were working with files, I'd recommend only changing fsuid, but it's unclear whether it's supposed to apply to Unix domain sockets as well or not - so changing the euid seems a safer bet in this case.)
Keeping the stat() checks as well is fine, but only as an extra.
Oh, and passing the arbitrary attacker-chosen pathname as part of pamLog() message might not be a good idea - e.g., can make this pathname itself be the string "file /whatever (owner whoevertoblame, uid 1000)" followed by enough whitespace that the rest of the string doesn't fit into a buffer and is truncated. Similarly, the username is only OK to log if it's been validated, whereas in the code we should have !ok for invalid usernames as well. I suggest only logging the uid's.
I hope this helps.
It sounds like they independently came to a similar conclusion and went with the euid swap. I attached their current proposed patch if you are interested.
Thank you for the continued iterations on this update!
Thanks. This is now good in concept, but problematic in implementation. I'll post a more detailed comment a bit later - am busy at the moment.
Issues with that patch:
-
The call to setuid(0) is wrong in that it will do more than is needed. We have previously only changed the euid - now with setuid(0) we're also setting the real uid to 0 even if it wasn't 0 before we started. It should be replaced with seteuid(origEUID). The change_uid() and setuid() functions may then be dropped.
-
The check "os.Getuid() != origEUID" may be enhanced to also take care of the case where the invoking program has already fully switched to root, but we're authenticating against a non-root user's ssh-agent. Thus, "os.Getuid() != origEUID || origEUID == 0".
-
The fileUID() check should be after the seteuid(uid) call. Otherwise we have that oracle/infoleak that I mentioned in a previous comment, allowing for probing of arbitrary inaccessible pathnames.
-
Also as I mentioned in that other comment, I suggest that you avoid including the attacker-controlled socket pathname in the log message.
Further comments on the patch ("non-issues"):
-
On Linux, whether seteuid() and setuid() are per-thread or not depends on whether you're invoking the direct syscalls or the glibc wrappers of the same name. You possibly already found the discussion at https://github.com/golang/go/issues/1435 where it's said that in Go they happened to be per-thread, yet in there test.go calls syscall.Setuid(), whereas you use an import "C" block #include'ing glibc headers and calling those functions. Thus, while I am not familiar with Go, it appears to me that in your case you'd likely be invoking the function versions that are trying to apply the change across all threads (whether they succeed is another matter - it's a can of worms). This means that your source code comments are probably wrong.
-
"Luckily", PAM modules are commonly or even typically not thread-safe anyway, and certainly e.g. sudo shouldn't expect thread-safety there. (It's yet another reason why it's pretty crazy to have a PAM module in Go, in my opinion. Why take the extra risks? But I digress.)
-
Similarly, you don't really have to use the *_r() function versions, but if you do I recommend you don't rely solely on the semantics where they NULL out the provided pointer on error. I recommend that you also check the return value.
-
Good thinking about ptrace. On Linux, the dumpable flag is reset automagically on SUID exec as well as when the program changes uid's. This has been so for 20+ years. I don't know re: Mac OS X; I hope they also started doing it right many years ago, but I wouldn't vouch for that. So while your code is likely redundant in that respect, it's indeed OK to keep those hardening changes.
-
I would recommend s/8096/8192/ for clarity (I guess you upgraded this from 4096, but forgot to edit the 96) and, more importantly, I recommend to use sizeof(buf) instead of the constant in the function calls.
-
Strictly speaking, and for some use cases this could conceivably be a real issue, you could also want to switch egid and groups. This could matter in case the module is invoked from a service (so has not only uid 0, but also e.g. gid 0), yet somehow the env var is defined. However, this complicates the logic (of when to do it, how to recover from failures), and I am tired - don't want to increase the number of iterations of code review. ;-) So maybe document this somewhere?
I see that most of what I pointed out has been addressed, and the fix made public. That's great. Thank you!
Out of those I pointed out before, the only fix missed, and one still needed, is this:
"The fileUID() check should be after the seteuid(uid) call. Otherwise we have that oracle/infoleak that I mentioned in a previous comment, allowing for probing of arbitrary inaccessible pathnames."
Specifically, the line "ownerUID := fileUID(authSock)" (and its preceding comment) should be moved to below the "if { ... }" block that follows (the one that switches euid). I guess maybe my earlier comment about the oracle/infoleak wasn't communicated to the author, so this concern wasn't understood?
Meanwhile, I updated my understanding of PAM thread-safety. Things have improved a bit since last time I had checked (years ago). It appears that now Linux-PAM's pam_unix does use getpwnam_r() and friends, yet it still switches the process global uids:
https://git.fedorahosted.org/cgit/linux-pam.git/tree/modules/pam_unix/passverify.c#n154
https://git.fedorahosted.org/cgit/linux-pam.git/tree/libpam/pam_modutil_getpwnam.c
This combination appears to be similar to what pam-ussh does now. So this detail on PAM is JFYI, no action needed on it.
In your code, if you wanted to be thread-safer than pam_unix, you could probably switch fsuid instead of euid, which should do the trick in this case due to parent directory permissions - and in fact the uncertainty regarding whether/how permissions on Unix sockets themselves are applied (across different Unix-likes) is probably the reason why OpenSSH does this mkdtemp() in the first place. Note that there's no getfsuid() - instead, setfsuid()'s return value is the previous fsuid. fsuid should per-thread even with glibc (not only direct syscall). Again, this is just an option, and no change is necessary.
Thanks again for the prompt fix.
Hi,
this is peter, the pam-ussh author. first of all, thank you so much for posting this initial bug report, and for following up with the reviews and comments. i'm a little embarrassed by how trivial this bug was, and i'm trying to blame my co-workers for missing it in review, but they're having none of it (this bug is all on me).
anyway, I tried posting a reply earlier but I didn't have appropriate h1 perms to it ended up being an internal only comment.
regarding the fileUID check; I agree that it can be used force the pam-ussh module to reveal more info than the caller would get from running, eg. ls(1), but it seems like if a user is on the system, they should be able to figure out everything that fileUID returns via 'ls /tmp' and 'ps auxgw | grep ssh-agent', right? If the auth sock's are just /tmp/ssh-<random>/agent.<pid>, it's not much work to figure out <pid> and <random>. But from the admin's perspective, a log line of
pam-ussh[54762]: error opening auth sock (sock owner: 4015/alice) by (caller: 4027/mallory)
is more useful than
pam-ussh[54762]: error opening auth sock by (caller: 4027/mallory)
or am I missing something?
Hi Peter! Nice to talk to you directly. (I was already wondering if I should possibly move further discussion to GitHub or e-mail.)
Thank you for your prompt handling of this issue and its related sub-issues!
I think you're missing my point regarding the fileUID check. I'll try to explain:
The problem with doing this check prior to the euid switch isn't limited to accessing another user's ssh-agent socket. It is about probing for existence of any pathname anywhere on the mounted filesystems, not limited to /tmp, nor to sockets. Side-channels allowing for such probing may include timing, log file size growth, log file contents (if readable), and probably/eventually more (e.g., some procfs or sysfs file showing stats on the user's own sudo's use of system resources - protection of such files for user's own invocations of SUID/SGID programs is traditionally inconsistent).
Fixing this doesn't strictly require omitting the "(sock owner: 4015/alice)" detail from the log message. However, I think I see what you mean: since the other user socket's parent directory is not readable after you've switched the euid, you wouldn't be able to check permissions directly on the socket itself. If you like, you could then fall back to recording permissions of the directory, just for the log message (and have it say those are for the directory). I am not sure if this complexity is justified.
One more issue I haven't yet mentioned: much of our logic so far is specific to sudo's use of the module. In case the module is stacked for su, the logic falls apart. The target user would be root, so that's the username you'd obtain from PAM and you would be accessing the socket with root privileges. However, the env var remains under control of an unprivileged user (and moreover even if it is not altered then it's set to point to the user's socket). Your "pam-ussh: check cert against the pam username" commit hopefully defeats the obvious attack on the authentication (although this depends on some external/setup detail), but it doesn't deal with the pathname probing issue above. Maybe you should re-word the README some further, stating that the module is only meant for use with sudo, and any other use may be unsafe?
Ah, of course. I'll stat the owning directory.
can you open an issue on github for the stacking issue? I think you're correct in your analysis that check the username against the cert should prevent the obvious bug (another embarrassing omission from the original commit :-/) but I want to be sure and that discussion is probably outside of the scope of h1.
Thanks again. This has been incredibly helpful.
OK, created https://github.com/uber/pam-ussh/issues/4
Hey @solardiz , thank you one final time for your work on this. It looks like everything is wrapped up and the version bumped, so I am closing this as resolved.
Our team meets weekly to evaluate resolved reports for bounties, so expect to hear back from us soon. Have a great day!
Hi @solardiz, thanks for this report, and for your participation in our bug bounty program. This was the first submission for an open-source project for our program, and we appreciate your time working with us through the fix. We hope to see more submissions from you in the future!
Thank you! How about making this issue public (I mean the HackerOne discussion above, as the actual bug and fix are already public via GitHub commits)? You could use this to set a precedent about your open source projects being in scope and eligible to bounty payouts. Maybe that would encourage others to report bugs in those as well.