Replace libutempter with... Unix login. (setuid -> fail) #219

Closed
wants to merge 2 commits into from

4 participants

@saurik

So, the goal of this commit is to solve a bunch of the random little bugs that have been accumulated on various platforms and various corner cases by going into the good ol' drawer o' Unix and pulling out our old friend /bin/login to wield as a massive hammer, allowing us to smash all of them at once.

Keeping score, this fixes the following issues (or, in the couple cases, obsoletes the fix that was added for them, allowing that code to be deleted). (It additionally fixes an issue I haven't even reported yet, which is that the current mechanism ends up in a level-two shell with double-.bashrc.)

Fix #47: hostname is set in utmpx by login
Fix #61: login handles utmpx on Mac OS X
Obsolete #190: motd is now handled by login
Fix #193: login correctly sets cwd to home
Obsolete #199: shell lookup is done by login
Fix #201: login understands username maps
Fix #216: .hushlogin is handled by login

As hostnames can no longer be set dynamically, the hostname is now statically set to the PID of mosh-server without a hostname (which happens to fix the truncated PIDs reported on FreeBSD by zi: there isn't an issue filed for that either, but there probably should be, as it seems to be a legitimate concern... if the PID is important, which people seemed to believe it was on IRC, it should be done in a way that makes you actually able to see it ;P).

That all said, though: I seriously doubt that this commit will ever be merged to upstream, as it requires mosh-server to be setuid root (given previous discussion that seems to indicate a strong preference against making mosh-server itself even have temporary privileges, no matter how many bugs it fixes or how cleaner it makes the code; I am not certain I agree with that stance, but I will happily admit that it is justifiable, and maybe even "more correct" given the security concerns of this particular component).

Regardless, I have opened a pull request related to it, as I think it is an interesting discussion to have, given that the alternative seems to be to slowly reimplement login (and even then, probably fail to fully support OS X; even with this patch I'm still having an issue with the security session system: I seem to be getting disconnected from my bootstrap context... I added a sleep(2) which mostly mitigates the problem, but that is obviously not a reasonable solution).

(BTW: I'm sorry that all related bugs ended up with a double "related commits" attached to it... I wonder if GitHub will ever realize those other commits got replaced and clean them up... I didn't realize they had such horribly painfully stale caching all over the place, or I would not have bothered to change the commit message to add the notes that I did; sigh.)

@saurik saurik Replace libutempter with... Unix login.
Fix #47: hostname is set in utmpx by login
Fix #61: login handles utmpx on Mac OS X
Obsolete #190: motd is now handled by login
Fix #193: login correctly sets cwd to home
Obsolete #199: shell lookup is done by login
Fix #201: login understands username maps
Fix #216: .hushlogin is handled by login

As hostnames can no longer be set dynamically, the hostname is now statically set to the PID of mosh-server without a hostname (which happens to fix the truncated PIDs reported on FreeBSD by zi).

I seriously doubt that this commit will ever be merged to upstream, however, as it requires mosh-server to be setuid root (upstream seems against mosh-server even having temporary priviledges).
46b7052
@keithw
Mosh (mobile shell) member

By all means, let's have the discussion! As you surmise, it's going to take a lot more than these glitches for me to get comfortable shipping privileged code.

SSH has had a zillion security problems, most of them by trying to do things that mosh intentionally doesn't get its hands dirty with. We don't authenticate, we don't have privileged code, we don't negotiate cipher modes (over an insecure channel or otherwise!), all our datagrams represent idempotent operations (so we are oblivious to replays), etc.

To the extent we can simplify our design to make the security story crystal-clear, well, it's very attractive.

If you want to fix the utempter API to better support multiple entries in /etc/passwd for the same UID, I am all in favor and would be happy to see if we can get the Linux distributions on board with whatever improved API we come up with. (The FreeBSD folks using mosh may be able to participate as well.) mosh will be the first application to support utempter 3.

@saurik

So, to me, the counter argument is also pretty clear: the high-level goal should be to get all of the crazy weird code out of mosh (I would call this "simplifying the design"), so that what is left has the minimal possible surface area for attack. I realize that you think that making something setuid makes it infinitely more dangerous, but I will argue that if you get access to the user's personal account that's really all you need: all of the user's data is going to be stored in their account, and you will almost certainly be able to use that account to make outgoing connections, build a botnet, access the internal network, infect his account and steal his credentials to other systems, or whatever it is that you wanted to do as part of your attack.

In comparison, by not using the tried and trusted /bin/login to handle all of these crazy corner cases (and I think we've barely scratched the surface of those), mosh will either a) have to remain crippled by these bugs (which are sufficient to make mosh totally unusable for me; it might work for you, but to me a project that is billing itself as a "replacement for SSH" can't do half that job wrong and remain credible) or b) have to take on a lot of the operational complexity of OpenSSH, as it will pretty much have to re-build a cross-platform implementation of login: the latter of these two options drastically increases the attack surface, especially on the platforms that will be stretching the code the furthest to work with the least testing.

Therefore, I do not believe it is at all clear that the correct decision, from a security standpoint, is for mosh to avoid setu/gid: it certainly has a purity to it, and I'll happily admit that it is "justifiable" (you won't get fired for avoiding setu/gid), but in my eyes it is the "wrong call" in this specific case: it honestly isn't like libutempter is that great of a project to be foisting trust on given that we would effectively be rewriting it ourselves anyway. Having a setgid utmp binary that is called "utempter" is not intrinsically more secure than having a setgid utmp binary called "moshtempter", and it really should not be considered defacto more secure than having part of mosh be setuid root: it would be better to concentrate on the tradeoff, not summarily dismiss it.

Looking at libutempter: that library's implementation only supports Linux and FreeBSD (not even Mac OS X, which is often very similar to FreeBSD), is actually kind of scary to use (in that I believe that a balancing error while using it could lead to the utmp database simply being wrong: that is not simply not possible with /bin/login; I have yet to figure out how I can exploit this fact, but I think the stale entries can be resurrected), and has an API that makes it impossible to correctly implement the username semantics in #201. Really, if anyone spends the time to "improve libutempter" they are just going to be rewriting it from scratch and then going around trying to get people to let the new one use the name, as that will be all that is left.

Meanwhile, the project has not been touched in five years (last released in 2007), was only touched twice in the preceding four years (version 1.1.1, the first version from altlinux that supplanted the RedHat library) was released no later than early 2003), and is currently only used by two packages in Debian (screen is actually not one of them: it has some code that could use it, but it isn't actually used due to API limitations)... I am having a difficult time believing that anyone (whether it be the developers that have probably forgotten they were affiliated with this project, the Debian package maintainers who will have to make the hard call about forking upstream, or the people in charge of xterm and libkpty who may have a say in how libutempter works) will be happy about being dragged into a conversation to discuss adding yet another API layer to this library.

To top it all off: I actually believe libutempter to be a security /bug/ by its very design, as it allows untrusted code to spoof hostnames into utmp: when I run "who" on my system I want the hostnames being reported to be accurate and generated by code installed by root and trusted by the system administrator... having libutempter installed on your system allows any user to spawn a pseudoterminal, run whatever code they want in it, and claim to utmp/who/etc. that they are a remote login account accessing the system from, for example, /my/ IP address (which, if I saw it in who, I'd just go "oh, I must have another ssh session running somewhere", as opposed to "oh shit, there's a second login to my server that I don't recognize").

Instead, I think it would be much better to concentrate on how to use /bin/login to get as much as code as possible out of mosh, and to instead figure out how to best isolate it. I think it should be entirely safe to add a setuid root binary that is pretty much exactly exec login -f -h "local:pid=$PPID" "$(whoami)", where "whoami" is replaced by the standard "get the username with getlogin(), call getpwnam() on that username, if the uid isn't getuid() replace the username with getpwuid() of our actual uid; return the resulting username". The result is a setuid root program that is less than ten lines of code long and that handles all of these currently known corner cases as well as any future corner cases that may come up. (That is likely what I will be implementing next on this front, as I really have no personal interest in trying to figure out what to do about libutempter.)

@keithw
Mosh (mobile shell) member

I'm imagining how the conversation will go with our friends at Debian/Ubuntu/Fedora/FreeBSD/Homebrew and the security freakout community:

Us: "Hey guys, mosh 1.2 is out. By the way, it now needs something to be setuid root! Please make pkg, thanks."
Them: "!?!?!?!?!? Why did you destroy the 'no privileged code' selling point?"
Us: "Oh, you know, we want to be able to make utmp entries on Mac OS X mosh-servers! Crucial feature!"
Them: "?!?!?!!?!"
Us: "Ok, not just that. It's also too much of a pain to check .hushlogin to suppress the motd. I mean, seriously, these requirements were just piling up like crazy and the cross became too heavy to bear."
Them: "?!?!?!?!!"

@keithw
Mosh (mobile shell) member

Anyway, I certainly respect your position, although I don't share your appraisal of the current approach's difficulties. Checking .hushlogin and similar issues are not crucial features, aren't difficult, and frankly do not justify becoming setuid root to avoid their pain. (And I do like that our approach puts nice details in who, like the IP address of the user and whether the connection is active or not, but this is not crucial either.)

But I don't disagree with your suggested approach: if you want to "add a setuid root binary that is pretty much exactly exec login -f -h "local:pid=$PPID" "$(whoami)"", that's a fine plan and one I totally support. If you maintain it and get it part of MacPorts (/homebrew if that is possible), I'm happy to make mosh depend on it and use it when available.

(For the record, gnome-terminal and xfce4-terminal use gnome-pty-helper, which is basically the same approach if less widely available.)

@saurik

@keithw ... being able to add utmp entries on Mac OS X actually is a crucial feature of a "replacement for SSH": without it tons of things simply do not work, which is why there are already multiple bug reports of unrelated things that are broken by that feature not working by people who are not me (I don't even like OS X: I was forced to use it due to nearly all of my followers using it, not because I would have done so by choice). In order to make this work on Mac OS X something has to be setuid root, and while it could be "the ability to falsify the hostname field in the utmp database" (something I maintain is a security issue) as opposed to "login as yourself with the host of a process identifier" (which seems entirely benign), that seems sufficiently wrong that I actually intend to go file a bug report against the libutempter package asking for its removal from Debian and Ubuntu: projects that are using it should probably be modified to instead use this login wrapper that I am proposing, and the reason cited should be "system security".

@saurik

(edit: I spent some more time investigating this comment, and it turned out I misread the output of ls somewhere and was wrong. I've removed it from the discussion as I think it was horribly confusing by being wrong, but I don't want to delete it as I want to leave the marker that there was something here for a bit and it was wrong.)

@saurik

As we have determined in #218 that the race condition I ran into and "fixed" as part of this pull request had nothing to do with using login, I've moved that to a separate feature branch and will file it as a new issue.

@keithw
Mosh (mobile shell) member

Ok, it sounds like we have a path forward -- you will make a new wrapper (saurik-awesome-pty-wrapper?) and as soon as it is available (especially on OS X), I imagine we will support it. And if you want us to make the MacPort depend on it, we can probably do that too.

Closing this for now, but please reopen or file a new issue when there are next steps. Thanks, Keith.

@keithw keithw closed this Apr 19, 2012
@saurik

I actually wrote it this morning (with one change I had to make as on OS X I needed both real and effective root, not just effective), and I tested it on the various platforms I had access to (Ubuntu, FreeBSD, OS X 10.6).

https://gist.github.com/2417089

@EdSchouten

Though I find quite some things questionable about the programming style of your patchset, there are some small remarks I'd like to make.

First of all, on FreeBSD this patch doesn't require any elevated privileges on FreeBSD, as login is already setuid. Also, please add "--" to the command line as well. It's not like there's an exploit here, but bugs related to login-invocations are almost as old as UNIX itself.

Also, what's so bad about libutempter?

@EdSchouten

By the way, FreeBSD's libutempter can very easily be ported to Mac OS X. It may even work without any source code alterations. On FreeBSD it's actually called libulog internally. Source code:

http://svnweb.freebsd.org/base/head/lib/libulog
http://svnweb.freebsd.org/base/head/libexec/ulog-helper

@saurik

@EdSchouten A) I would love to know what you find "questionable" in the programming style (seriously: I always want to learn). I purposely went quite far out of my way to mimic the programming style of mosh (including usage of the weird my_argv array for memory "management", the mismatched intermingling of std::strings and character pointers, duplicating hardcoded size constants rather than using sizeof, and the usage of implicit comparison on pointers using !), so I feel I at least have to mention "I would never have coded something that looks like that in my own code". I guess the one thing I failed to do is I checked for == 0 on access, when mosh developers might have instead used !. Really, though, the only code that I even had much control over were lines 308-328, or 20 lines of code ;P.

B) I detail the problems with libutempter above, but in summary the issues are that it does not allow you to suggest a username (which makes it fail in situations where multiple usernames map to the same uid; other tools and libraries go out of their way to make certain that this situation works correctly) and by its very nature allows unprivileged users to arbitrarily alter the hostname field of their utmp entries (which undermines the credibility of utmp: only trusted processes should be able to log hostnames to that field), which the FreeBSD developers of the library seem to at least understand (they have a caveat in the source code "It does allow users to log arbitrary hostnames."), but do not seem to correctly appreciate.

@saurik

@EdSchouten While login is "already setuid", you cannot use it with -f (to force the login without credentials) unless you have root access (most platforms) or are already that same user (this is not allowed on Linux but is allowed on OS X; I do not know about FreeBSD as I am firewalled from my FreeBSD test box at my current location). On all platforms, however, usage of -h (which allows overriding the hostname arbitrarily) is only allowed by root, a security measure that is sadly undermined by having libutempter installed on your system. I vaguely concur on your request to add -- before the username passed to login (although AFAIK hyphens are never allowed before usernames anyway, and so the getpwnam uid verification should fail), but it is always a good idea to be anally safe (which I have attempted to do in my Gist).

@saurik

(Irritatingly, as this pull request is "Closed", it is not possible to add commits to it the way one normally would while discussing a pull request on GitHub. Again: GitHub pull requests track branches, not commits, and indicate an intent with an associated issue, not a specific request looking for an immediate conclusion. If you do not like this behavior I highly recommend using alternative systems to track code.)

@EdSchouten

Well, I guess the user/uid-aliasing is already inherent to give problems in many parts of UNIX-land. For example, even though utmp(x) and wtmp(x) correctly deal with user/uid-aliasing, lastlog does not. This is because in most implementations this file is simply an array of `struct lastlog's, indexed by uid. I think FreeBSD 9+ is the only operating system to ship with a lastlog that doesn't suffer from this problem.

Sorry for being so vague about the "questionable" programming style I mentioned in my previous message. It was in no way to be supposed to act as an insult. I guess the actual feedback I can give with respect to coding style is the following:

  • Inspecting the source code, I don't think there is any need to use strdup(). The strings are only passed to execvp() somewhere, so they don't need to be writable.
  • The hostname buffer doesn't really need to be 1024 bytes, right? It only stores "mosh:user.%u". A 32-bit integer represented as a decimal can only be up to 11 bytes (10 for the number, 1 extra for a minus). I guess something like char host[64] would be more than enough. Also, why not use sizeof(host) in the snprintf() call?
  • There has to be a cleaner way to find the login binary. Why not sanitize $PATH on startup and simply use "login"? The code already uses execvp().

Also I think your patchset should not remove support for libutempter. On FreeBSD the existing code already does a pretty good job, without requiring too many elevated privileges, so why abandon that model?

Thanks,
Ed

@saurik
  • I used strdup in order to get around the type conflict between char * and const char *. There also was an strdup in the original code that I had removed (although I may have myself added that strdup in a previous patch: I don't remember ;P). I actually find it disconcerting that the memory management of this code is so unclear, and had I designed it I would have probably kept an std::vector< std::string > until the last moment (and in fact have done this in various of my projects).

  • I used the size-1024 buffer because I had seen other size-1024 buffers in mosh's code when smaller buffers would have sufficed, but I see they do normally use size-64 buffers for the hostname. You will note that in my Gist (which is coded in a saurik-standard style) I used a size-64 buffer. I will not bother adding a commit for this to the branch as the 1024 buffer is fine, but will point out that that code will be replaced by the external relogin wrapper anyway.

  • I emphatically agree that sizeof(host) should be used: it seriously bothered me to no end as I typed that, as having that duplication is just asking for a mistake at a later time. However, if you check, you will notice that mosh never uses sizeof for this purpose. :( There are a few calls to snprintf that were removed by my patch you can examine.

  • On FreeBSD the current code causes the same problem with regards to usernames: this is not an OS X specific issue, and in fact the username problem I'm running into is happening on my Linux servers (I do not have any personal need to log in to an OS X system, and was only looking into those issues to help out others, as I spend my life swimming in random OS X-specific issues due to my work on Cydia). I will also again point out that libutempter avoids an elevated privilege, but itself exposes a privilege incorrectly: unprivileged users should not be able to modify the hostname field of utmp; in fact, the only reason /bin/login requires setuid privileges on OS X for our purposes is because it makes damned sure that you are root if you want to modify the hostname field: otherwise, you can run /bin/login -f myuser without root access and it will work correctly. The fact that libutempter allows this should be considered a security bug, and I intend to file it as such against Ubuntu/Debian in the near future.

@keithw
Mosh (mobile shell) member

@saurik Heya, I love your enthusiasm for mosh, but getting worked up about the semantics of github is not worth it! My understanding was that a pull request was literally a request for me to merge the attached commits to my master branch, and I try to deal with the inbox frequently so I'm not annoying people who want me to merge their commits. If you want to use them to put up straw men and use the pull request as a discussion forum, that's not how I had been interpreting them, but it's certainly not worth arguing about -- consider it reopened.

@keithw keithw reopened this Apr 19, 2012
@EdSchouten

But still, if it's just the username-aliasing, that can in fact even be fixed! I can simply modify FreeBSD's ulog-helper to call getlogin() and validate it against the current uid.

@saurik

@EdSchouten I thought of that patch, but it will not work: getlogin() is itself based on the utmp mechanism, and as so returns NULL in this context. Though, I have only verified that on Linux, and not on FreeBSD (and cannot right now as my FreeBSD testing box is firewalled from my access at this location).

That said, it had not yet occurred to me to attempt to mix/match the various solutions: if you use getenv("LOGNAME") as an additional check in that patch (as I do in the relogin.c wrapper I posted as a gist) then it may be sufficient, and the same patch may also work on the Linux versions of libutempter.

@EdSchouten

No, it isn't. On FreeBSD, getlogin() and setlogin() directly modify "struct session::s_login" inside the kernel.

At least on FreeBSD, it now works reliably. :-)

@saurik

@EdSchouten OK, I have talked to the system administrator of the network running the FreeBSD test box I have access to and got the firewall fixed, so I can now verify behaviors: in fact, getlogin() does seem to work on FreeBSD with my previous test case that failed on Linux. (edit: I did not see your message before I posted this reply.)

As I now see that you are the author of this tool, does it seriously not bother you that it allows users to arbitrarily edit the hostname field of utmp? If you do not believe that that is an issue, why does /bin/login go to lengths to disallow this?

It seems awkward that in one place this is considered a security issue of sufficient magnitude to require telnetd to run as root, and in another place it is considered "engh: doesn't matter". AFAIK this is the only reason telnetd is actually required to operate as root: otherwise, it could drop privileges all the way down to nobody as soon as it took over port 23.

$ login -h dsa
login: -h option: Operation not permitted
@saurik

@EdSchouten The specific code that handles this login flag (which the code itself documents as "for telnetd, etc." in a comment on line 52) is in usr.bin/login/login.c at around line 204:

    case 'h':
        if (uid != 0)
            errx(1, "-h option: %s", strerror(EPERM));
@EdSchouten

Yeah, it is a bit inconsistent. But there may be a historical reason why it's the way it is. The fact is that in FreeBSD 8-, login(1) would simply overwrite the existing utmp record associated with the terminal, that could in fact contain data that is assumed to be trusted. This is of course very bad.

With utmpx this is not the case anymore, as each invocation of `login -f ed' will simply add more entries to the database, using a random ut_id. So yes, we could revisit this decision. I'll add it to my todo-list.

@saurik

@EdSchouten Once you are willing to install ulog-helper (or utempter) on your system, however, there is no longer any reason to believe that the hostname field in utmp means anything at all; it does not matter whether the entries are being overwritten or accumulated, as any given line might be someone claiming an incorrect hostname: the value of "data that is assumed to be trusted" is entirely thrown out the window, and any of it may as well be overwritten.

You can't even claim that at least the original trusted entry will be visible, as you can simply run your own service daemon inside of the new falsified pseudo-terminal and drop your original connection (in fact, mosh does exactly this). The question really comes down to "what data is considered trusted", and the evidence seems to indicate that the hostname field has historically been trusted information, not just on FreeBSD but on every system that has ever supported utmp.

@kmcallister

I'm closing this because the discussion has run down, we all agree the code won't be merged, and it's not even clear what this thread is about anymore.

Please open tickets for any bugs that would have been fixed by this patch, if they don't already have tickets.

You can still comment on the closed issue. I understand that you can't attach new commits but I don't think that's a big deal. You can still link to a branch in your repo as an ordinary web link. That's more flexible and allows us to discuss multiple alternative approaches.

In the future I would like to reserve pull requests for code which is plausibly ready to be merged. That way the distinction between "issue" and "pull request" is useful to the people who actually do the pulling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment