[patch] remove deadcode

[Matt Johnston]

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);
 }