On Wed, 29 Sep 1999 17:39:52 -0400, Dan Astoorian wrote: > I have a couple of serious concerns about this patch. Well, at least they're (mostly) not items I hadn't thought of myself... > 1) It leaves behind a race condition; a symlink created between the > lstat() and the bind() will still get happily followed. The race > condition could be minimized by moving the lstat() and the bind() > closer together, but it can't be eliminated this way. This is why > it's important for the check to be made in the kernel, where it can > be done atomically. Agreed, but in the absence of source code for Digital Unix, there isn't much I can do about that. I admit it's far from ideal, but it's the best I came up with. > 2) Using popen() within a privileged process is somewhat reckless; it > potentially opens up the usual risks of shell-mischief, I'd certainly agree with you in the case of a setuid-root program, where the environment hasn't been sanitized, or the potential for user-supplied code finding its way into the popen() call exists, but in this case neither is true. The popen call is completely encapsulated in the code, and the process was started by root, running in a controlled environment from the start. Those who might still be wary of the popen() call could simply comment out the call to mail_report() lower down, and that simply wouldn't be a concern, (though they wouldn't get a report mailed to them immediately when someone tries to use the exploit). > although I haven't gone digging for any specific holes you've > opened up. What's wrong with using syslog? Nothing, and you'll see that right before calling mail_report(), I use the log_msg() routine which does just that. The point of the mail_report() routine with the popen() call was to mail a report to the sysadmin at the exact moment an exploit attempt was detected. Consider it optional. > 3) This isn't a vulnerability, but as a matter of principle, I don't > trust any code that could wind up containing this line: > > + if (dirname[strlen(dirname)] == '/') dirname[strlen(dirname)] = 0; > > (Trust me: dirname[strlen(dirname)] != '/'. Presumably a "- 1" was > intended someplace or two?) Yes. In each place, actually. <*blush*> :-( Thanks for noticing that. > The race condition is a hard problem; if bind() follows symlinks, it is > *impossible* to safely use it in a directory writable by anyone other > than geteuid(). Agreed, but I'm not sure that wouldn't break anything in ssh. > I haven't looked into what would be involved in creating a proper patch, > but appropriate ways to fix the problem *might* include: > > - changing the process's effective userid/groupid/credentials to match > the target user before doing the bind(), so that directories not > writable by the user are also not writable by the code doing the > bind(); or I certainly agree this might work, but I imagine sshd performs a number of other tasks after the bind, but before it can drop privileges. In fact I believe it forks and only the child drops privileges sometime before execing a shell. Don't quote me on that, though. > - using a different location for the Unix domain socket--one which > is verifiably manipulable only by root. This would probably change ssh so that it *must* be setuid root, which currently is only true if you intend to be able to fall-back to .rhosts for authentication. I prefer to not have the additional setuid root binary, (but yes, I realize I'm not much further ahead with a race condition in a different binary that runs as root. I don't have any better ideas, though). > [As long as I'm here: it's been pointed out to me that my test program > was missing a semicolon after "close(fd)". This was, of course, a > cut-and-paste error; my apologies.] That seems minor compared to a couple of zero-offset errors... Thanks for the comments... -- ---------------------------------------------------------------------- Sylvain Robitaille sylat_private Systems Manager Concordia University Instructional & Information Technology Montreal, Quebec, Canada ----------------------------------------------------------------------
This archive was generated by hypermail 2b30 : Fri Apr 13 2001 - 15:06:04 PDT