On Thu, Jul 06, 2006 at 02:49:40PM -0700, Erik Hovland wrote:
> I have finished an audit of dropbear and decided to reply to my own post
> because my current draft of patches expands on the same file (and a few
> others.
>
> The patches should be consistent with current mtn repo and should have
> annotations in each file. I am willing to rework any patch that doesn't
> meet the statisfaction of the devs. So please send feedback.
Thanks for the eyeballing and patches. I've applied most of
them with only minor changes.
With the svr-chansession.c exit patch I think the current
code is correct, as the exit value will only be unset when
i == svr_ses.childpidsize. I've modified the code to be a
bit clearer anyway.
For the ssh-pty.c patch, I don't think this improves the
security/correctness much. tty_name is always a /dev/ttyXXX
device, and if an attacker can manipulate paths in /dev/, then
there are larger problems. Does that analysis sound
reasonable?
Cheers,
Matt
(PS, if you're using the monotone head, beware that there's
a known issue that can cause it to wait for input when
closing on Linux.)
> BUG: Potentially dangeruous use of stat/chown/chmod. Since we use the
> string tty_name for all calls on the tty device it is possible for
> someone to be underhanded and use these calls and that string
> to elevate priviledges.
>
> FIX: Switch to using fstat/fchown/fchmod and obtain a file
> descriptor instead of using a string.
>
> NOTE: I doubt this can be exploited. But why leave it hanging around.
>
--- sshpty.c old
+++ sshpty.c new
@@ -356,6 +356,7 @@ void
pty_setowner(struct passwd *pw, const char *tty_name)
{
struct group *grp;
+ FILE* ttyfd;
gid_t gid;
mode_t mode;
struct stat st;
@@ -375,21 +376,25 @@ pty_setowner(struct passwd *pw, const ch
* Warn but continue if filesystem is read-only and the uids match/
* tty is owned by root.
*/
- if (stat(tty_name, &st)) {
- dropbear_exit("pty_setowner: stat(%.101s) failed: %.100s",
+ if (!(ttyfd = fopen(tty_name, "w+"))) {
+ dropbear_exit("pty_setowner: fopen(%.101s) failed: %.100s",
+ tty_name, strerror(errno));
+ }
+ if (fstat(ttyfd, &st)) {
+ dropbear_exit("pty_setowner: fstat(%.101s) failed: %.100s",
tty_name, strerror(errno));
}
if (st.st_uid != pw->pw_uid || st.st_gid != gid) {
- if (chown(tty_name, pw->pw_uid, gid) < 0) {
+ if (fchown(ttyfd, pw->pw_uid, gid) < 0) {
if (errno == EROFS &&
(st.st_uid == pw->pw_uid || st.st_uid == 0)) {
dropbear_log(LOG_ERR,
- "chown(%.100s, %u, %u) failed: %.100s",
+ "fchown(%.100s, %u, %u) failed: %.100s",
tty_name, (unsigned
int)pw->pw_uid, (unsigned int)gid,
strerror(errno));
} else {
- dropbear_exit("chown(%.100s, %u, %u) failed:
%.100s",
+ dropbear_exit("fchown(%.100s, %u, %u) failed:
%.100s",
tty_name, (unsigned int)pw->pw_uid,
(unsigned int)gid,
strerror(errno));
}
@@ -397,16 +402,18 @@ pty_setowner(struct passwd *pw, const ch
}
if ((st.st_mode & (S_IRWXU|S_IRWXG|S_IRWXO)) != mode) {
- if (chmod(tty_name, mode) < 0) {
+ if (fchmod(ttyfd, mode) < 0) {
if (errno == EROFS &&
(st.st_mode & (S_IRGRP | S_IROTH)) == 0) {
dropbear_log(LOG_ERR,
- "chmod(%.100s, 0%o) failed: %.100s",
+ "fchmod(%.100s, 0%o) failed: %.100s",
tty_name, mode, strerror(errno));
} else {
- dropbear_exit("chmod(%.100s, 0%o) failed:
%.100s",
+ dropbear_exit("fchmod(%.100s, 0%o) failed:
%.100s",
tty_name, mode, strerror(errno));
}
}
}
+ if (ttyfd)
+ fclose(ttyfd);
}
