← Commit list

4bdbb7cc0

Denys Vlasenko
Denys Vlasenko
2026-02-24
SUCCESS Duration: 146s

Subject

telnetd: if vfork in make_new_session() fails, do close socket.

Binary

No binary (build failed)

Architecture & Compiler Flags

Architecture: x86_64-linux-gnu Compiler: gcc (Ubuntu 11.4.0-1ubuntu1~22.04.3) 11.4.0 
gcc -Wp,-MD,applets/.usage.d -Wall -Wstrict-prototypes -O2 -fomit-frame-pointer      -Iinclude -Iinclude -o applets/usage applets/usage.c

Build Log

(log yok)

Commit Diff GitHub →

libbb/getpty.c +58 -24networking/telnetd.c +26 -22
libbb/getpty.c+58 -24
@@ -9,35 +9,69 @@
#define DEBUG 0
+// On modern systems (old ways were more kludgy with setuid "pt_chown" binary):
+// ptyfd = open("/dev/ptmx"):
+// Kernel creates slave /dev/pts/N with owner = real UID of opener,
+// group and mode as configured by /dev/pts mount options.
+// The mode is safe with standard mount options (gid=5,mode=620).
+//
+// grantpt(ptyfd) is now only a verification step. In glibc:
+// Calls ioctl(fd, TIOCGPTN, &minor) to confirm ptyfd is valid master pty.
+// If ioctl fails with ENOTTY, remaps to EINVAL (POSIX).
+// Otherwise returns 0. (No chown/chmod occurs.)
+//
+// unlockpt(ptyfd) is ioctl(TIOCSPTLCK, 0): clears optional protection flag
+// on slave. If flag was set, open(slave) fails with EIO for everyone (even root)
+// until unlocked. [Do modern kernels still set the lock in open("/dev/ptmx")?]
+// The lock historically prevented opening slave before grantpt() fixes perms
+// in misconfigured/old setups where kernel didn't set 0620 atomically.
+// Today it's mostly belt-and-suspenders + POSIX compatibility.
+//
+// The attack thwarted by the lock (assuming unprivileged adversary):
+// you open /dev/ptmx, kernel allocates /dev/pts/N with overly permissive mode.
+// Adversary manages to open /dev/pts/N before your grantpt() call
+// locks down permissions.
+// Typically, /dev/pts mount options are gid=5,mode=620, 5 = "tty" group,
+// system is set up with only trusted binaries and no users in "tty" group.
+// In this case, the locking mechanism is overkill (would be safe without it).
+// To fortify more, you can mount with gid=0,mode=600. Tools like "wall",
+// "mesg" would stop working.
+// Processes with my own UID can still race against me, but I probably
+// trust myself...
+//
+// ptsname(ptyfd), internally ioctl(TIOCGPTN): get index, build
+// "/dev/pts/NN" string which refers to the slave pty.
int FAST_FUNC xgetpty(char *line)
{
- int p;
-
#if ENABLE_FEATURE_DEVPTS
- p = open("/dev/ptmx", O_RDWR);
- if (p >= 0) {
- grantpt(p); /* chmod+chown corresponding slave pty */
- unlockpt(p); /* (what does this do?) */
+ int ptyfd;
+
+ ptyfd = open("/dev/ptmx", O_RDWR);
+ if (ptyfd < 0)
+ bb_simple_perror_msg_and_die("can't find free pty");
+ grantpt(ptyfd); /* chmod+chown corresponding slave pty */
+ unlockpt(ptyfd); /* allow open() on slave /dev node */
# ifndef HAVE_PTSNAME_R
- {
- const char *name;
- name = ptsname(p); /* find out the name of slave pty */
- if (!name) {
- bb_simple_perror_msg_and_die("ptsname error (is /dev/pts mounted?)");
- }
- safe_strncpy(line, name, GETPTY_BUFSIZE);
- }
-# else
- /* find out the name of slave pty */
- if (ptsname_r(p, line, GETPTY_BUFSIZE-1) != 0) {
+ {
+ const char *name;
+ name = ptsname(ptyfd); /* find out the name of slave pty */
+ if (!name) {
bb_simple_perror_msg_and_die("ptsname error (is /dev/pts mounted?)");
}
- line[GETPTY_BUFSIZE-1] = '\0';
-# endif
- return p;
+ safe_strncpy(line, name, GETPTY_BUFSIZE);
}
+# else
+ /* find out the name of slave pty */
+ if (ptsname_r(ptyfd, line, GETPTY_BUFSIZE-1) != 0) {
+ bb_simple_perror_msg_and_die("ptsname error (is /dev/pts mounted?)");
+ }
+ line[GETPTY_BUFSIZE-1] = '\0';
+# endif
+ return ptyfd;
+
#else
struct stat stb;
+ int ptyfd;
int i;
int j;
@@ -53,13 +87,13 @@ int FAST_FUNC xgetpty(char *line)
line[9] = j < 10 ? j + '0' : j - 10 + 'a';
if (DEBUG)
fprintf(stderr, "Trying to open device: %s\n", line);
- p = open(line, O_RDWR | O_NOCTTY);
- if (p >= 0) {
+ ptyfd = open(line, O_RDWR | O_NOCTTY);
+ if (ptyfd >= 0) {
line[5] = 't';
- return p;
+ return ptyfd;
}
}
}
-#endif /* FEATURE_DEVPTS */
bb_simple_error_msg_and_die("can't find free pty");
+#endif /* FEATURE_DEVPTS */
}
networking/telnetd.c+26 -22
@@ -375,9 +375,9 @@ static int net_to_pty__have_data_to_write(void *this)
if (buf[1] >= 240 && buf[1] <= 249) {
/* 2-byte commands (240..250 and 255):
- * IAC IAC (255) Literal 255. Supported.
+ * IAC IAC (255) Literal 255.
* IAC SE (240) End of subnegotiation. Treated as NOP.
- * IAC NOP (241) NOP. Supported.
+ * IAC NOP (241) NOP.
* IAC BRK (243) Break. Like serial line break. TODO via tcsendbreak()?
* IAC AYT (246) Are you there.
* These don't look useful:
@@ -385,7 +385,7 @@ static int net_to_pty__have_data_to_write(void *this)
* IAC IP (244) Suspend, interrupt or abort the process. (Ancient cousin of ^C).
* IAC AO (245) Abort output. "You can continue running, but do not send me the output".
* IAC EC (247) Erase character. The receiver should delete the last received char.
- * IAC EL (248) Erase line. The receiver should delete everything up tp last newline.
+ * IAC EL (248) Erase line. The receiver should delete everything up to last newline.
* IAC GA (249) Go ahead. For half-duplex lines: "now you talk".
*/
if (buf[1] == AYT && ts->sibling) /* notify other pipe that AYT was seen */
@@ -852,13 +852,13 @@ static void make_new_session(ioloop_state_t *io, int sockrd)
const int sockwr = sockrd != 0 ? sockrd : 1;
const char *login_argv[2];
struct termios termbuf;
- int fd, pid;
+ int ptyfd, pid;
char tty_name[GETPTY_BUFSIZE];
- /* Got a new connection, set up a tty */
- fd = xgetpty(tty_name);
- ndelay_on(fd);
- close_on_exec_on(fd);
+ /* Got a new connection, set up a pty */
+ ptyfd = xgetpty(tty_name);
+ ndelay_on(ptyfd);
+ close_on_exec_on(ptyfd);
/* SO_KEEPALIVE by popular demand */
setsockopt_keepalive(sockrd);
@@ -885,7 +885,7 @@ static void make_new_session(ioloop_state_t *io, int sockrd)
//Theoretically, our "WILL X" are requests and should only activate when client responds with "DO X".
//However, we do not wait/check for "DO"s. Why?
//There is nothing to "activate" on our side for TELOPT_ECHO.
-//For TELOPT_SGA, we don't even have code to support sending/understandig GAs,
+//For TELOPT_SGA, we don't even have code to support sending/understanding GAs,
//so SGA is "always activated".
/* Just stuff it into TCP stream! (no error check...) */
safe_write(sockwr, iacs_to_send, sizeof(iacs_to_send));
@@ -894,8 +894,8 @@ static void make_new_session(ioloop_state_t *io, int sockrd)
fflush_all();
pid = vfork(); /* NOMMU-friendly */
if (pid < 0) {
- close(fd);
- /* socket will be closed by caller */
+ close(ptyfd);
+ close(sockrd);
bb_simple_perror_msg("vfork");
return;
}
@@ -904,8 +904,8 @@ static void make_new_session(ioloop_state_t *io, int sockrd)
pty_to_net_t *to_net;
net_to_pty_t *to_pty;
- to_net = new_pty_to_net(fd, sockwr);
- to_pty = new_net_to_pty(sockrd, fd);
+ to_net = new_pty_to_net(ptyfd, sockwr);
+ to_pty = new_net_to_pty(sockrd, ptyfd);
dbg("to_net:%p to_pty:%p", to_net, to_pty);
to_pty->sibling = to_net;
to_net->sibling = to_pty;
@@ -926,7 +926,7 @@ static void make_new_session(ioloop_state_t *io, int sockrd)
//pid = getpid(); // redundant, setsid gives us our pid
pid = setsid();
- /* Open the child's side of the tty */
+ /* Open the child's side of the pty */
/* NB: setsid() disconnects from any previous ctty's. Therefore
* we must open child's side of the tty AFTER setsid! */
close(0);
@@ -943,7 +943,7 @@ static void make_new_session(ioloop_state_t *io, int sockrd)
free(lsa);
}
write_new_utmp(pid, LOGIN_PROCESS, tty_name, /*username:*/ "LOGIN", hostname);
- free(hostname);
+ IF_FEATURE_CLEAN_UP(free(hostname);)
}
/* The pseudo-terminal allocated to the client is configured to operate
@@ -969,8 +969,8 @@ static void make_new_session(ioloop_state_t *io, int sockrd)
login_argv[1] = NULL;
/* exec busybox applet (if PREFER_APPLETS=y), if that fails,
* exec external program.
- * NB: sock is either 0 or has CLOEXEC set on it.
- * fd has CLOEXEC set on it too. These two fds will be closed here.
+ * NB: sockrd is either 0 or has CLOEXEC set on it.
+ * ptyfd has CLOEXEC set on it too. These two fds will be closed here.
*/
BB_EXECVP(G.loginpath, (char **)login_argv);
/* _exit is safer with vfork, and we shouldn't send message
@@ -1138,9 +1138,9 @@ static int test_vhangup(void)
last_change_ns = now_ns;
}
- /* Exit if no change for 2 seconds */
+ /* Exit if no change for a few seconds */
if ((now_ns - last_change_ns) > 1000*1000*1000) {
- bb_simple_info_msg("no change for 1 second, stopping read() loop");
+ bb_simple_info_msg("no change for 1 second, loop stopped");
break;
}
@@ -1248,6 +1248,7 @@ static int test_net_to_pty_data_integrity(void)
if (input_data[i + 1] == SB) {
/* IAC SB ... skip subnegotiation */
if (input_data[i + 2] == TELOPT_NAWS) {
+//TODO: simulate unusual NAWS
/* IAC SB TELOPT_NAWS [4 bytes] */
i += 3; /* skip IAC SB TELOPT_NAWS */
/* Skip 4 bytes of window size */
@@ -1338,7 +1339,7 @@ static int test_net_to_pty_data_integrity(void)
close(ptyfd);
slave_fd = xopen(tty_name, O_RDWR);
- setsid(); /* this loses ctty (no tty signals ^C, ^Z, ^\; no SIGHUP on master close (probably?) */
+ setsid(); /* this loses ctty (no tty signals ^C, ^Z, ^\; no SIGHUP on master close (probably?)) */
tcgetattr(slave_fd, &termbuf);
cfmakeraw(&termbuf);
@@ -1372,7 +1373,7 @@ static int test_net_to_pty_data_integrity(void)
if (retry < 3) continue;
break; /* error (parent closed master) - expected */
}
- /* If happens, probably kenel bug! */
+ /* If happens, probably kernel bug! */
if (retry != 0) bb_error_msg_and_die("READ DATA AFTER EOF/ERROR RETRY!!!");
#if SAVE_NET2PTY_ACTUAL
xwrite(out_fd, read_buf, n);
@@ -1701,10 +1702,14 @@ int telnetd_main(int argc UNUSED_PARAM, char **argv)
#if ENABLE_FEATURE_TELNETD_STANDALONE
else {
int master_fd = 0;
+ /* For -w SEC, listening socket is 0. Otherwise: */
if (!(opt & OPT_WAIT)) {
unsigned portnbr = CONFIG_FEATURE_TELNETD_PORT_DEFAULT;
if (opt & OPT_PORT)
portnbr = xatou16(opt_portnbr);
+ /* If someone would run "telnetd 0>&-", we can get terribly confused. */
+ /* Nake sure master_fd, or accept() result fd, etc, cant become 0 or 1: */
+ bb_sanitize_stdio();
master_fd = create_and_bind_stream_or_die(opt_bindaddr, portnbr);
xlisten(master_fd, 1);
}
@@ -1712,7 +1717,6 @@ int telnetd_main(int argc UNUSED_PARAM, char **argv)
ioloop_insert_conn(&G.io, (void *)new_accept_conn(master_fd));
}
#endif
-
/* We don't want to die if just one session is broken */
signal(SIGPIPE, SIG_IGN);