n_tty: Remove LINEMODE support

Commit 26df6d13406d1 ("tty: Add EXTPROC support for LINEMODE") added the undocumented EXTPROC input processing mode, which ignores the ICANON setting and forces pty slave input to be processed in non-canonical mode.

Although intended to provide a transparent mechanism for local line edit with telnetd (and other remote shell protocols), the transparency is limited.

Userspace usage is abandoned; telnetd does not even compile with LINEMODE support. readline/bash and sshd never supported this.

Cc Howard Chu <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
  drivers/tty/n_tty.c | 22 ++++++----------------
  drivers/tty/pty.c   | 24 +-----------------------
  2 files changed, 7 insertions(+), 39 deletions(-)

drivers/tty/n_tty.c +6 -16

index eb9f114..5120d2b 100644
@@ -1562,10 +1562,6 @@ n_tty_receive_buf_standard(struct tty_struct *tty, const unsigned char *cp,
  				c &= 0x7f;
  			if (I_IUCLC(tty) && L_IEXTEN(tty))
  				c = tolower(c);
-			if (L_EXTPROC(tty)) {
-				put_tty_queue(c, ldata);
-				continue;
-			}
  			if (!test_bit(c, ldata->char_map))
  				n_tty_receive_char_inline(tty, c);
  			else if (n_tty_receive_char_special(tty, c) && count) {

@@ -1613,9 +1609,9 @@ static void __receive_buf(struct tty_struct *tty, const unsigned char *cp,
 
  	if (ldata->real_raw)
  		n_tty_receive_buf_real_raw(tty, cp, fp, count);
-	else if (ldata->raw || (L_EXTPROC(tty) && !preops))
+	else if (ldata->raw)
  		n_tty_receive_buf_raw(tty, cp, fp, count);
-	else if (tty->closing && !L_EXTPROC(tty))
+	else if (tty->closing)
  		n_tty_receive_buf_closing(tty, cp, fp, count);
  	else {
  		if (ldata->lnext) {

@@ -1637,13 +1633,13 @@ static void __receive_buf(struct tty_struct *tty, const unsigned char *cp,
  			tty->ops->flush_chars(tty);
  	}
 
-	if (ldata->icanon && !L_EXTPROC(tty))
+	if (ldata->icanon)
  		return;
 
  	/* publish read_head to consumer */
  	smp_store_release(&ldata->commit_head, ldata->read_head);
 
-	if ((read_cnt(ldata) >= ldata->minimum_to_wake) || L_EXTPROC(tty)) {
+	if (read_cnt(ldata) >= ldata->minimum_to_wake) {
  		kill_fasync(&tty->fasync, SIGIO, POLL_IN);
  		if (waitqueue_active(&tty->read_wait))
  			wake_up_interruptible_poll(&tty->read_wait, POLLIN);

@@ -1939,7 +1935,7 @@ static inline int input_available_p(struct tty_struct *tty, int poll)
  	struct n_tty_data *ldata = tty->disc_data;
  	int amt = poll && !TIME_CHAR(tty) && MIN_CHAR(tty) ? MIN_CHAR(tty) : 1;
 
-	if (ldata->icanon && !L_EXTPROC(tty))
+	if (ldata->icanon)
  		return ldata->canon_head != ldata->read_tail;
  	else
  		return ldata->commit_head - ldata->read_tail >= amt;

@@ -1973,7 +1969,6 @@ static int copy_from_read_buf(struct tty_struct *tty,
  	struct n_tty_data *ldata = tty->disc_data;
  	int retval;
  	size_t n;
-	bool is_eof;
  	size_t head = smp_load_acquire(&ldata->commit_head);
  	size_t tail = ldata->read_tail & (N_TTY_BUF_SIZE - 1);
 

@@ -1983,14 +1978,9 @@ static int copy_from_read_buf(struct tty_struct *tty,
  	if (n) {
  		retval = copy_to_user(*b, read_buf_addr(ldata, tail), n);
  		n -= retval;
-		is_eof = n == 1 && read_buf(ldata, tail) == EOF_CHAR(tty);
  		tty_audit_add_data(tty, read_buf_addr(ldata, tail), n,
  				ldata->icanon);
  		smp_store_release(&ldata->read_tail, ldata->read_tail + n);
-		/* Turn single EOF into zero-length read */
-		if (L_EXTPROC(tty) && ldata->icanon && is_eof &&
-		    (head == ldata->read_tail))
-			n = 0;
  		*b += n;
  		*nr -= n;
  	}

@@ -2257,7 +2247,7 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
  			continue;
  		}
 
-		if (ldata->icanon && !L_EXTPROC(tty)) {
+		if (ldata->icanon) {
  			retval = canon_copy_from_read_buf(tty, &b, &nr);
  			if (retval == -EAGAIN) {
  				retval = 0;

drivers/tty/pty.c +1 -23

index ee06b77..01ac182 100644
@@ -192,20 +192,6 @@ static int pty_get_pktmode(struct tty_struct *tty, int __user *arg)
  	return put_user(pktmode, arg);
  }
 
-/* Send a signal to the slave */
-static int pty_signal(struct tty_struct *tty, int sig)
-{
-	struct pid *pgrp;
-
-	if (tty->link) {
-		pgrp = tty_get_pgrp(tty->link);
-		if (pgrp)
-			kill_pgrp(pgrp, sig, 1);
-		put_pid(pgrp);
-	}
-	return 0;
-}
-
  static void pty_flush_buffer(struct tty_struct *tty)
  {
  	struct tty_struct *to = tty->link;

@@ -254,15 +240,13 @@ static void pty_set_termios(struct tty_struct *tty,
  {
  	/* See if packet mode change of state. */
  	if (tty->link && tty->link->packet) {
-		int extproc = (old_termios->c_lflag & EXTPROC) |
-				(tty->termios.c_lflag & EXTPROC);
  		int old_flow = ((old_termios->c_iflag & IXON) &&
  				(old_termios->c_cc[VSTOP] == '\023') &&
  				(old_termios->c_cc[VSTART] == '\021'));
  		int new_flow = (I_IXON(tty) &&
  				STOP_CHAR(tty) == '\023' &&
  				START_CHAR(tty) == '\021');
-		if ((old_flow != new_flow) || extproc) {
+		if (old_flow != new_flow) {
  			spin_lock_irq(&tty->ctrl_lock);
  			if (old_flow != new_flow) {
  				tty->ctrl_status &= ~(TIOCPKT_DOSTOP | TIOCPKT_NOSTOP);

@@ -271,8 +255,6 @@ static void pty_set_termios(struct tty_struct *tty,
  				else
  					tty->ctrl_status |= TIOCPKT_NOSTOP;
  			}
-			if (extproc)
-				tty->ctrl_status |= TIOCPKT_IOCTL;
  			spin_unlock_irq(&tty->ctrl_lock);
  			wake_up_interruptible(&tty->link->read_wait);
  		}

@@ -482,8 +464,6 @@ static int pty_bsd_ioctl(struct tty_struct *tty,
  		return pty_set_pktmode(tty, (int __user *)arg);
  	case TIOCGPKT: /* Get PT packet mode */
  		return pty_get_pktmode(tty, (int __user *)arg);
-	case TIOCSIG:    /* Send signal to other side of pty */
-		return pty_signal(tty, (int) arg);
  	case TIOCGPTN: /* TTY returns ENOTTY, but glibc expects EINVAL here */
  		return -EINVAL;
  	}

@@ -607,8 +587,6 @@ static int pty_unix98_ioctl(struct tty_struct *tty,
  		return pty_get_pktmode(tty, (int __user *)arg);
  	case TIOCGPTN: /* Get PT Number */
  		return put_user(tty->index, (unsigned int __user *)arg);
-	case TIOCSIG:    /* Send signal to other side of pty */
-		return pty_signal(tty, (int) arg);
  	}
 
  	return -ENOIOCTLCMD;

Quoted 11 lines · Peter Hurley

Commit 26df6d13406d1 ("tty: Add EXTPROC support for LINEMODE") added the undocumented EXTPROC input processing mode, which ignores the ICANON setting and forces pty slave input to be processed in non-canonical mode.

Although intended to provide a transparent mechanism for local line edit with telnetd (and other remote shell protocols), the transparency is limited.

Userspace usage is abandoned; telnetd does not even compile with LINEMODE support. readline/bash and sshd never supported this.

I object to this. Code for all of the above exists and works. I use this code daily.

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=585527
http://lists.gnu.org/archive/html/bug-readline/2011-01/msg00004.html
https://github.com/hyc/OpenSSH-LINEMODE

The lack of LINEMODE support in upstream sshd can only be considered a security hole.

http://www.metzdowd.com/pipermail/cryptography/2015-January/024288.html

Hi Howard,

Quoted 19 lines · Howard Chu

Quoted 11 lines · Peter Hurley

Commit 26df6d13406d1 ("tty: Add EXTPROC support for LINEMODE") added the undocumented EXTPROC input processing mode, which ignores the ICANON setting and forces pty slave input to be processed in non-canonical mode.

Although intended to provide a transparent mechanism for local line edit with telnetd (and other remote shell protocols), the transparency is limited.

Userspace usage is abandoned; telnetd does not even compile with LINEMODE support. readline/bash and sshd never supported this.

I object to this. Code for all of the above exists and works. I use this code daily.

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=585527
http://lists.gnu.org/archive/html/bug-readline/2011-01/msg00004.html
https://github.com/hyc/OpenSSH-LINEMODE

Quoted 3 lines

The lack of LINEMODE support in upstream sshd can only be considered a security hole.

http://www.metzdowd.com/pipermail/cryptography/2015-January/024288.html

These are all bug reports about userspace _not_ supporting this extension.

Where is a working userspace consumer of this interface?

I seriously doubt this works reliably.
What happens when the pty slave reader is in canonical mode and gets unterminated input because only a portion of the input is available yet? The way this is coded does _not_ require line termination before returning data to userspace.

Also, ioctl(FIONREAD) doesn't match what read() returns, nor that poll()/select() indicated input was available.

Quoted 22 lines · Peter Hurley

Hi Howard,

Quoted 19 lines · Howard Chu

Quoted 11 lines · Peter Hurley

Commit 26df6d13406d1 ("tty: Add EXTPROC support for LINEMODE") added the undocumented EXTPROC input processing mode, which ignores the ICANON setting and forces pty slave input to be processed in non-canonical mode.

Although intended to provide a transparent mechanism for local line edit with telnetd (and other remote shell protocols), the transparency is limited.

Userspace usage is abandoned; telnetd does not even compile with LINEMODE support. readline/bash and sshd never supported this.

I object to this. Code for all of the above exists and works. I use this code daily.

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=585527
http://lists.gnu.org/archive/html/bug-readline/2011-01/msg00004.html
https://github.com/hyc/OpenSSH-LINEMODE

Quoted 5 lines

Quoted 3 lines

The lack of LINEMODE support in upstream sshd can only be considered a security hole.

http://www.metzdowd.com/pipermail/cryptography/2015-January/024288.html

These are all bug reports about userspace _not_ supporting this extension.

Bug reports *with working patches* attached. And the fact remains that not supporting this feature *is* a security liability.

Quoted 1 line

Where is a working userspace consumer of this interface?

The OpenSSH fork on github is a full working client and server using this interface.

Quoted 4 lines

I seriously doubt this works reliably.
What happens when the pty slave reader is in canonical mode and gets unterminated input because only a portion of the input is available yet? The way this is coded does _not_ require line termination before returning data to userspace.

Userspace already has to deal with incomplete lines if the input line is longer than the input buffer.

Quoted 2 lines

Also, ioctl(FIONREAD) doesn't match what read() returns, nor that poll()/select() indicated input was available.

Hm, I think you're mistaken about poll/select.

    if ((!ldata->icanon && (ldata->read_cnt >= tty->minimum_to_wake)) ||
        L_EXTPROC(tty)) {
        kill_fasync(&tty->fasync, SIGIO, POLL_IN);
        if (waitqueue_active(&tty->read_wait))
            wake_up_interruptible(&tty->read_wait);
    }

Line mode goes back to BSD 4.x days, and it's useful over high latency links --- i.e., the sort of thing that you get with stone age cellular data networks (i.e., the sort of thing that we still have the US), amateur packet radio links, etc. So it's certianly a nice to have, if someone is willing to support it and the maintenance burden on the rest of the tty stack isn't too onerous.

Quoted 9 lines · Howard Chu

Quoted 4 lines · Peter Hurley

Commit 26df6d13406d1 ("tty: Add EXTPROC support for LINEMODE") added the undocumented EXTPROC input processing mode, which ignores the ICANON setting and forces pty slave input to be processed in non-canonical mode.

What's the motivation to remove this code, rather than improve it if it needs fixing? It has been removed from the Linux kernel at least once before already, and that was a mistake back then too.

It is a significant maintenance burden, and I have concerns about the level of support it's receiving. Here's some outstanding issues:

1. No man page documentation. At a minimum, tty_ioctl(4) and termios(3) need the userspace visible definitions and behavior documented. Better would be a LINEMODE (7) description of how this implementation works wrt supporting RFC 1116.

2. read(), poll()/select() and ioctl() with and without EXTPROC need to have _identical_ userspace behavior.

3. Does the local edit guarantee canon lines <= 4096 chars? What happens

    if pty slave reader does this?

	char buffer[4096];
	char *p = buffer;

	n = read(tty, buffer, sizeof(buffer));
	if (n <= 0)
		goto done;
	while (*p++ != '\n')
		;

4. ioctl(TIOCSIG) can send _any_ signal to a different process without permission checks. That's not good.

5. This needs to work with readline(). Right now, I don't see how this won't have worst-case behavior, constantly sending termios changes, with scripted input where the reader switches back-and-forth between canonical and non-canonical mode (like readline() does). Database shells behave like this, but you can do a 20-line shell mockup with just readline().

6. EXTPROC still does some input processing on the server. For example, 7-bit mode (ISTRIP), tolower mode (IUCLC) and processing while closing; if input processing is being done on the local/client side, why the extra work here?

7. This needs a reference userspace implementation which for the moment could double as regression testing. A library with unit tests would be ideal.

ISTM the right implementation, if there is one, is for EXTPROC to process input exactly like raw mode except that line termination wakes up read_wait and there is no special casing in read/poll.

Does SLC_FORW1 & SLC_FORW2
map directly to termios.c_cc[] line termination values?

I'd like to do away with the signalling part; just turn off EXTPROC and send the appropriate signalling char from the pty master, like telnetd does now. Same for EOF.

Quoted 14 lines · Peter Hurley

Quoted 9 lines · Howard Chu

Quoted 4 lines · Peter Hurley

Commit 26df6d13406d1 ("tty: Add EXTPROC support for LINEMODE") added the undocumented EXTPROC input processing mode, which ignores the ICANON setting and forces pty slave input to be processed in non-canonical mode.

What's the motivation to remove this code, rather than improve it if it needs fixing? It has been removed from the Linux kernel at least once before already, and that was a mistake back then too.

It is a significant maintenance burden, and I have concerns about the level of support it's receiving. Here's some outstanding issues:

Quoted 4 lines

1. No man page documentation. At a minimum, tty_ioctl(4) and termios(3) need the userspace visible definitions and behavior documented. Better would be a LINEMODE (7) description of how this implementation works wrt supporting RFC 1116.

That can be added easily enough.

Historically EXTPROC has had very little documentation of its own. E.g. the FreeBSD manpage only shows that the flag exists.

https://www.freebsd.org/cgi/man.cgi?query=termios&sektion=4

Quoted 2 lines

2. read(), poll()/select() and ioctl() with and without EXTPROC need to have _identical_ userspace behavior.

OK, this can be fixed.

Quoted 6 lines

3. Does the local edit guarantee canon lines <= 4096 chars? What happens

    if pty slave reader does this?

    char buffer[4096];
    char *p = buffer;

Quoted 5 lines

    n = read(tty, buffer, sizeof(buffer));
    if (n <= 0)
        goto done;
    while (*p++ != '\n')
        ;

This reader is broken, since the tty driver supports EOL and EOL2 and this code doesn't account for it.

In practice your concern is misdirected - it's the job of whatever code is talking to the pty master to send valid data to the pty slave. There's no reason for the tty driver to second-guess the app here.

Quoted 2 lines

4. ioctl(TIOCSIG) can send _any_ signal to a different process without permission checks. That's not good.

It can only send to the pty slave. Permissions were already checked when the pty master was opened.

What further checks do you think are needed? You think it should be limited to tty-specific signals: INT, QUIT, CONT, TSTP, TTIN, TTOU, WINCH?

Quoted 6 lines

5. This needs to work with readline(). Right now, I don't see how this won't have worst-case behavior, constantly sending termios changes, with scripted input where the reader switches back-and-forth between canonical and non-canonical mode (like readline() does). Database shells behave like this, but you can do a 20-line shell mockup with just readline().

readline() patched accordingly https://groups.google.com/forum/#!topic/gnu.bash.bug/o0UA55AhADs will cooperate. Sending one or two termios changes per input line is still far better than one character-at-a-time packets back and forth.

Quoted 4 lines

6. EXTPROC still does some input processing on the server. For example, 7-bit mode (ISTRIP), tolower mode (IUCLC) and processing while closing; if input processing is being done on the local/client side, why the extra work here?

That's defensive, on the assumption that something else might break if e.g. the tty expected only 7-bit input but 8-bit characters were sent to it.

Quoted 3 lines

7. This needs a reference userspace implementation which for the moment could double as regression testing. A library with unit tests would be ideal.

telnet/telnetd can probably used as a starting point for this.

Quoted 3 lines

ISTM the right implementation, if there is one, is for EXTPROC to process input exactly like raw mode except that line termination wakes up read_wait and there is no special casing in read/poll.

I agree that this sounds simpler but I feel there's a reason (which I can't remember at the moment) that it doesn't work out that way.

Quoted 2 lines

Does SLC_FORW1 & SLC_FORW2
map directly to termios.c_cc[] line termination values?

Maps exactly to VEOL / VEOL2.

Quoted 3 lines

I'd like to do away with the signalling part; just turn off EXTPROC and send the appropriate signalling char from the pty master, like telnetd does now. Same for EOF.

That introduces additional mode changes, which you were just worrying about above, re: readline. It would make the traffic stream less efficient overall.

Thanks, Howard.

[ adding Ted too ]

Quoted 15 lines · Howard Chu

Quoted 14 lines · Peter Hurley

Quoted 9 lines · Howard Chu

Quoted 4 lines · Peter Hurley

Commit 26df6d13406d1 ("tty: Add EXTPROC support for LINEMODE") added the undocumented EXTPROC input processing mode, which ignores the ICANON setting and forces pty slave input to be processed in non-canonical mode.

What's the motivation to remove this code, rather than improve it if it needs fixing? It has been removed from the Linux kernel at least once before already, and that was a mistake back then too.

It is a significant maintenance burden, and I have concerns about the level of support it's receiving. Here's some outstanding issues:

Quoted 6 lines

Quoted 4 lines

1. No man page documentation. At a minimum, tty_ioctl(4) and termios(3) need the userspace visible definitions and behavior documented. Better would be a LINEMODE (7) description of how this implementation works wrt supporting RFC 1116.

That can be added easily enough.

Is this you signing up? :)

Which brings up another point: only a pty master should be able to set EXTPROC mode. Right now, any tty can be set to EXTPROC and the pty slave can even accidentally unset it. This argues for a new pty ioctl() to set EXTPROC in termios. pty_set_termios() can silently merge the bit.

A description of how the pty master uses EXTPROC to implement line mode would be very helpful, especially to people working in the tty code (eg., me, although I don't need it now).

Quoted 12 lines

Quoted 11 lines

3. Does the local edit guarantee canon lines <= 4096 chars? What happens

     if pty slave reader does this?

     char buffer[4096];
     char *p = buffer;

     n = read(tty, buffer, sizeof(buffer));
     if (n <= 0)
         goto done;
     while (*p++ != '\n')
         ;

Quoted 1 line

This reader is broken, since the tty driver supports EOL and EOL2 and this code doesn't account for it.

This reader set EOL2 to DISABLED_CHAR earlier, and left EOL unchanged. I have seen userspace code that expects a line to be no longer than 4096 chars.

Quoted 1 line

In practice your concern is misdirected - it's the job of whatever code is talking to the pty master to send valid data to the pty slave. There's no reason for the tty driver to second-guess the app here.

What about a malicious client?

Quoted 4 lines

Quoted 3 lines

4. ioctl(TIOCSIG) can send _any_ signal to a different process without permission checks. That's not good.

Quoted 1 line

It can only send to the pty slave. Permissions were already checked when the pty master was opened.

Still not ok.

Quoted 1 line

What further checks do you think are needed? You think it should be limited to tty-specific signals: INT, QUIT, CONT, TSTP, TTIN, TTOU, WINCH?

My first choice is to do away with TIOCSIG ioctl completely; see ending comments. My second would be masked to only those already used: INT, QUIT, TSTP.

Quoted 8 lines

Quoted 6 lines

5. This needs to work with readline(). Right now, I don't see how this won't have worst-case behavior, constantly sending termios changes, with scripted input where the reader switches back-and-forth between canonical and non-canonical mode (like readline() does). Database shells behave like this, but you can do a 20-line shell mockup with just readline().

readline() patched accordingly https://groups.google.com/forum/#!topic/gnu.bash.bug/o0UA55AhADs will cooperate. Sending one or two termios changes per input line is still far better than one character-at-a-time packets back and forth.

Ok, I misunderstood: the only incompatibility here is that readline() simply doesn't use line mode. That's fine from the kernel perspective.

Although it does seem odd that the command shell doesn't support the input mode :)

Quoted 6 lines

Quoted 4 lines

6. EXTPROC still does some input processing on the server. For example, 7-bit mode (ISTRIP), tolower mode (IUCLC) and processing while closing; if input processing is being done on the local/client side, why the extra work here?

That's defensive, on the assumption that something else might break if e.g. the tty expected only 7-bit input but 8-bit characters were sent to it.

Ok, is that because RFC 1116 doesn't specify ISTRIP and IUCLC handling so the server can't be sure the client did it? If so, that should be documented so that refactors don't remove that handling.

Regular ptys dump input while closing so this should too.

Quoted 5 lines

Quoted 3 lines

7. This needs a reference userspace implementation which for the moment could double as regression testing. A library with unit tests would be ideal.

telnet/telnetd can probably used as a starting point for this.

Except telnet is unmaintained except by each distro. Your patches don't apply to my distro telnet; they had been applied but are now ifdef'd out. I messed around with rebuilding it but never got telnetd to actually set EXTPROC.

My point is there is no straightforward way to test this, and that's a problem.

In fact, this is my main concern: no matter how useful this might be, it's pointless if the complementary userspace puzzle piece is unsupported. I know you have written patches but that's not the same as support; for example your OpenSSH fork is 4 years old, which is a non-starter because I'm sure there's known exploits which have been fixed in those 4 years.

Why doesn't Ubuntu/Debian telnet even compile for line mode support? Did the package maintainer shut it off because it was causing bug reports?

What more needs to be done to get line mode working 100% in bash?

Quoted 5 lines

Quoted 3 lines

I'd like to do away with the signalling part; just turn off EXTPROC and send the appropriate signalling char from the pty master, like telnetd does now. Same for EOF.

That introduces additional mode changes, which you were just worrying about above, re: readline. It would make the traffic stream less efficient overall.

Maybe you misunderstood. If local signalling is being performed by the client, the over-the-wire traffic is the same. The pty master would turn off EXTPROC to write the signalling char but would not reflect the termios to the client (because it contains no relevant changes).

And the signalling char is isolated and unique: it's not as if it's being stuffed within an existing stream of i/o, because the client should have already ceased input if it's handling signals locally.

Quoted 25 lines · Peter Hurley

Thanks, Howard.

[ adding Ted too ]

Quoted 20 lines · Howard Chu

Quoted 18 lines · Peter Hurley

Quoted 9 lines · Howard Chu

Quoted 4 lines · Peter Hurley

Commit 26df6d13406d1 ("tty: Add EXTPROC support for LINEMODE") added the undocumented EXTPROC input processing mode, which ignores the ICANON setting and forces pty slave input to be processed in non-canonical mode.

What's the motivation to remove this code, rather than improve it if it needs fixing? It has been removed from the Linux kernel at least once before already, and that was a mistake back then too.

It is a significant maintenance burden, and I have concerns about the level of support it's receiving. Here's some outstanding issues:

1. No man page documentation. At a minimum, tty_ioctl(4) and termios(3) need the userspace visible definitions and behavior documented. Better would be a LINEMODE (7) description of how this implementation works wrt supporting RFC 1116.

Quoted 3 lines

Quoted 1 line

That can be added easily enough.

Is this you signing up? :)

Sure. I suppose the obvious place to add it is near the TIOCPKT doc in tty_ioctl(4). But, about your further comment:

Quoted 4 lines

Which brings up another point: only a pty master should be able to set EXTPROC mode. Right now, any tty can be set to EXTPROC and the pty slave can even accidentally unset it. This argues for a new pty ioctl() to set EXTPROC in termios. pty_set_termios() can silently merge the bit.

Historically EXTPROC could also be set on regular ttys, for lines that were hooked up to special terminal interfaces. E.g. X.29 PADs that did their own input processing. I'm not sure how relevant this is now, but this is the reason EXTPROC is not a pty-specific ioctl.

Quoted 3 lines

A description of how the pty master uses EXTPROC to implement line mode would be very helpful, especially to people working in the tty code (eg., me, although I don't need it now).

The original patch submission carried this description
http://lkml.iu.edu/hypermail/linux/kernel/1006.2/02428.html

Quoted 14 lines

Quoted 13 lines

Quoted 11 lines

3. Does the local edit guarantee canon lines <= 4096 chars? What happens

     if pty slave reader does this?

     char buffer[4096];
     char *p = buffer;

     n = read(tty, buffer, sizeof(buffer));
     if (n <= 0)
         goto done;
     while (*p++ != '\n')
         ;

This reader is broken, since the tty driver supports EOL and EOL2 and this code doesn't account for it.

Quoted 6 lines

This reader set EOL2 to DISABLED_CHAR earlier, and left EOL unchanged. I have seen userspace code that expects a line to be no longer than 4096 chars.

Quoted 1 line

In practice your concern is misdirected - it's the job of whatever code is talking to the pty master to send valid data to the pty slave. There's no reason for the tty driver to second-guess the app here.

What about a malicious client?

This is still a broken reader. It's as bad as using strchr() without knowing if the string is NUL-terminated, if 4096 bytes are read the thing will go off into the weeds.

Quoted 5 lines

Quoted 4 lines

Quoted 2 lines

4. ioctl(TIOCSIG) can send _any_ signal to a different process without permission checks. That's not good.

It can only send to the pty slave. Permissions were already checked when the pty master was opened.

Quoted 6 lines

Still not ok.

Quoted 1 line

What further checks do you think are needed? You think it should be limited to tty-specific signals: INT, QUIT, CONT, TSTP, TTIN, TTOU, WINCH?

My first choice is to do away with TIOCSIG ioctl completely; see ending comments. My second would be masked to only those already used: INT, QUIT, TSTP.

Perhaps your way is better. This patch simply duplicated the BSD functionality, to maintain compatibility.

Quoted 25 lines

Quoted 8 lines

Quoted 6 lines

5. This needs to work with readline(). Right now, I don't see how this won't have worst-case behavior, constantly sending termios changes, with scripted input where the reader switches back-and-forth between canonical and non-canonical mode (like readline() does). Database shells behave like this, but you can do a 20-line shell mockup with just readline().

readline() patched accordingly https://groups.google.com/forum/#!topic/gnu.bash.bug/o0UA55AhADs will cooperate. Sending one or two termios changes per input line is still far better than one character-at-a-time packets back and forth.

Ok, I misunderstood: the only incompatibility here is that readline() simply doesn't use line mode. That's fine from the kernel perspective.

Although it does seem odd that the command shell doesn't support the input mode :)

Quoted 6 lines

Quoted 4 lines

6. EXTPROC still does some input processing on the server. For example, 7-bit mode (ISTRIP), tolower mode (IUCLC) and processing while closing; if input processing is being done on the local/client side, why the extra work here?

That's defensive, on the assumption that something else might break if e.g. the tty expected only 7-bit input but 8-bit characters were sent to it.

Ok, is that because RFC 1116 doesn't specify ISTRIP and IUCLC handling so the server can't be sure the client did it? If so, that should be documented so that refactors don't remove that handling.

Quoted 20 lines

Regular ptys dump input while closing so this should too.

Quoted 5 lines

Quoted 3 lines

7. This needs a reference userspace implementation which for the moment could double as regression testing. A library with unit tests would be ideal.

telnet/telnetd can probably used as a starting point for this.

Except telnet is unmaintained except by each distro. Your patches don't apply to my distro telnet; they had been applied but are now ifdef'd out. I messed around with rebuilding it but never got telnetd to actually set EXTPROC.

My point is there is no straightforward way to test this, and that's a problem.

In fact, this is my main concern: no matter how useful this might be, it's pointless if the complementary userspace puzzle piece is unsupported. I know you have written patches but that's not the same as support; for example your OpenSSH fork is 4 years old, which is a non-starter because I'm sure there's known exploits which have been fixed in those 4 years.

Quoted 2 lines

Why doesn't Ubuntu/Debian telnet even compile for line mode support? Did the package maintainer shut it off because it was causing bug reports?

I don't know. Looking at the package history it appears they have never enabled it. https://launchpad.net/ubuntu/+source/netkit-telnet/+changelog

I also don't know where the actual VCS repo is for this code.

Quoted 1 line

What more needs to be done to get line mode working 100% in bash?

TAB-completion was messy. Although editing and scrolling through command history can be done completely locally, TAB-completion required sending a partial line to the server and feeding the completion results back, and then possibly flushing the input buffer if the results weren't actually used.

With a few years of hindsight, this can probably be cleaned up better now.

Quoted 11 lines

Quoted 5 lines

Quoted 3 lines

I'd like to do away with the signalling part; just turn off EXTPROC and send the appropriate signalling char from the pty master, like telnetd does now. Same for EOF.

That introduces additional mode changes, which you were just worrying about above, re: readline. It would make the traffic stream less efficient overall.

Maybe you misunderstood. If local signalling is being performed by the client, the over-the-wire traffic is the same. The pty master would turn off EXTPROC to write the signalling char but would not reflect the termios to the client (because it contains no relevant changes).

Quoted 4 lines

And the signalling char is isolated and unique: it's not as if it's being stuffed within an existing stream of i/o, because the client should have already ceased input if it's handling signals locally.

Makes sense. Don't know why the original telnet patch didn't do this.

Quoted 26 lines · Howard Chu

Quoted 25 lines · Peter Hurley

Thanks, Howard.

[ adding Ted too ]

Quoted 20 lines · Howard Chu

Quoted 18 lines · Peter Hurley

Quoted 9 lines · Howard Chu

Quoted 4 lines · Peter Hurley

Commit 26df6d13406d1 ("tty: Add EXTPROC support for LINEMODE") added the undocumented EXTPROC input processing mode, which ignores the ICANON setting and forces pty slave input to be processed in non-canonical mode.

What's the motivation to remove this code, rather than improve it if it needs fixing? It has been removed from the Linux kernel at least once before already, and that was a mistake back then too.

It is a significant maintenance burden, and I have concerns about the level of support it's receiving. Here's some outstanding issues:

1. No man page documentation. At a minimum, tty_ioctl(4) and termios(3) need the userspace visible definitions and behavior documented. Better would be a LINEMODE (7) description of how this implementation works wrt supporting RFC 1116.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

That can be added easily enough.

Is this you signing up? :)

Sure. I suppose the obvious place to add it is near the TIOCPKT doc in tty_ioctl(4). But, about your further comment:

Thanks.

Quoted 5 lines

Quoted 4 lines

Which brings up another point: only a pty master should be able to set EXTPROC mode. Right now, any tty can be set to EXTPROC and the pty slave can even accidentally unset it. This argues for a new pty ioctl() to set EXTPROC in termios. pty_set_termios() can silently merge the bit.

Quoted 1 line

Historically EXTPROC could also be set on regular ttys, for lines that were hooked up to special terminal interfaces. E.g. X.29 PADs that did their own input processing. I'm not sure how relevant this is now, but this is the reason EXTPROC is not a pty-specific ioctl.

How does the remote end find out when non-canon mode is selected? And who does the EXTPROC setting, because if it's the program that opened the tty, then it can just select raw mode instead.

stty.diff:
This file contains the changes needed for the stty(1) program to report on the current status of the TS_EXTPROC bit. It also allows the user to turn on/off the TS_EXTPROC bit. This is useful because it allows the user to say "stty -extproc", and the LINEMODE option will be automatically disabled, and saying "stty extproc" will re-enable the LINEMODE option.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

6. EXTPROC still does some input processing on the server. For example, 7-bit mode (ISTRIP), tolower mode (IUCLC) and processing while closing; if input processing is being done on the local/client side, why the extra work here?

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

That's defensive, on the assumption that something else might break if e.g. the tty expected only 7-bit input but 8-bit characters were sent to it.

Ok, is that because RFC 1116 doesn't specify ISTRIP and IUCLC handling so the server can't be sure the client did it? If so, that should be documented so that refactors don't remove that handling.

Could you get back to me about this, as well?

Quoted 20 lines

Quoted 20 lines

Regular ptys dump input while closing so this should too.

Quoted 5 lines

Quoted 3 lines

7. This needs a reference userspace implementation which for the moment could double as regression testing. A library with unit tests would be ideal.

telnet/telnetd can probably used as a starting point for this.

Except telnet is unmaintained except by each distro. Your patches don't apply to my distro telnet; they had been applied but are now ifdef'd out. I messed around with rebuilding it but never got telnetd to actually set EXTPROC.

My point is there is no straightforward way to test this, and that's a problem.

In fact, this is my main concern: no matter how useful this might be, it's pointless if the complementary userspace puzzle piece is unsupported. I know you have written patches but that's not the same as support; for example your OpenSSH fork is 4 years old, which is a non-starter because I'm sure there's known exploits which have been fixed in those 4 years.

Quoted 6 lines

Quoted 2 lines

Why doesn't Ubuntu/Debian telnet even compile for line mode support? Did the package maintainer shut it off because it was causing bug reports?

I don't know. Looking at the package history it appears they have never enabled it. https://launchpad.net/ubuntu/+source/netkit-telnet/+changelog

I also don't know where the actual VCS repo is for this code.

There isn't one. The base code is from downloaded tarball.

I don't know what distro you use (or if you even do), but for Ubuntu/Debian the base package information is here https://packages.debian.org/wheezy/telnetd
including maintainer email, etc.

Typically, to get changes merged requires making a patch(es) against the package source and sending/uploading those.

Quoted 4 lines · Peter Hurley

Quoted 3 lines · Howard Chu

Quoted 2 lines · Peter Hurley

Quoted 1 line · Howard Chu

Peter Hurley wrote:

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Which brings up another point: only a pty master should be able to set EXTPROC mode. Right now, any tty can be set to EXTPROC and the pty slave can even accidentally unset it. This argues for a new pty ioctl() to set EXTPROC in termios. pty_set_termios() can silently merge the bit.

Quoted 5 lines

Quoted 1 line

Historically EXTPROC could also be set on regular ttys, for lines that were hooked up to special terminal interfaces. E.g. X.29 PADs that did their own input processing. I'm not sure how relevant this is now, but this is the reason EXTPROC is not a pty-specific ioctl.

How does the remote end find out when non-canon mode is selected? And who does the EXTPROC setting, because if it's the program that opened the tty, then it can just select raw mode instead.

There's a few significant differences in semantics between raw mode and EXTPROC mode.

The point of EXTPROC is to maintain the tty driver state as if cooked mode were still in effect. One obvious difference is that VMIN/VTIME are overlaid on VEOL/VEOF in raw mode

, so the two are not directly interchangeable.

The fact that EXTPROC can be manually unset is by design. Quoting from the original again:

Quoted 7 lines

stty.diff:
This file contains the changes needed for the stty(1) program to report on the current status of the TS_EXTPROC bit. It also allows the user to turn on/off the TS_EXTPROC bit. This is useful because it allows the user to say "stty -extproc", and the LINEMODE option will be automatically disabled, and saying "stty extproc" will re-enable the LINEMODE option.

Quoted 11 lines

Quoted 10 lines

Quoted 10 lines

Quoted 6 lines

Quoted 4 lines

6. EXTPROC still does some input processing on the server. For example, 7-bit mode (ISTRIP), tolower mode (IUCLC) and processing while closing; if input processing is being done on the local/client side, why the extra work here?

That's defensive, on the assumption that something else might break if e.g. the tty expected only 7-bit input but 8-bit characters were sent to it.

Ok, is that because RFC 1116 doesn't specify ISTRIP and IUCLC handling so the server can't be sure the client did it? If so, that should be documented so that refactors don't remove that handling.

Quoted 1 line

Could you get back to me about this, as well?

The telnet protocol (RFC854) defines a Network Virtual Terminal (NVT) as using 7-bit USASCII in an 8-bit field. As such, it expects the client to be able to generate both upper and lower case itself, so there's no analogue to IUCLC, and there would be no reason to use ISTRIP.

RFC5198 updates the protocols to use UTF8. So again, it assumes full octets are being transmitted.

Perhaps we can drop these special cases from the driver.

Quoted 9 lines · Howard Chu

The fact that EXTPROC can be manually unset is by design. Quoting from the original again:

Quoted 7 lines

stty.diff:
This file contains the changes needed for the stty(1) program to report on the current status of the TS_EXTPROC bit. It also allows the user to turn on/off the TS_EXTPROC bit. This is useful because it allows the user to say "stty -extproc", and the LINEMODE option will be automatically disabled, and saying "stty extproc" will re-enable the LINEMODE option.

This option is not supported by gnu coreutils.

So it's really back to the question of, does allowing EXTPROC for regular ttys have _value_?

Quoted 15 lines

Quoted 14 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

6. EXTPROC still does some input processing on the server. For example, 7-bit mode (ISTRIP), tolower mode (IUCLC) and processing while closing; if input processing is being done on the local/client side, why the extra work here?

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

That's defensive, on the assumption that something else might break if e.g. the tty expected only 7-bit input but 8-bit characters were sent to it.

Ok, is that because RFC 1116 doesn't specify ISTRIP and IUCLC handling so the server can't be sure the client did it? If so, that should be documented so that refactors don't remove that handling.

Could you get back to me about this, as well?

Quoted 5 lines

The telnet protocol (RFC854) defines a Network Virtual Terminal (NVT) as using 7-bit USASCII in an 8-bit field. As such, it expects the client to be able to generate both upper and lower case itself, so there's no analogue to IUCLC, and there would be no reason to use ISTRIP.

RFC5198 updates the protocols to use UTF8. So again, it assumes full octets are being transmitted.

Perhaps we can drop these special cases from the driver.

I don't mind leaving it in, but without comments it looks like a refactoring error.

Quoted 12 lines · Peter Hurley

Quoted 10 lines · Howard Chu

The fact that EXTPROC can be manually unset is by design. Quoting from the original again:

Quoted 7 lines

stty.diff:
This file contains the changes needed for the stty(1) program to report on the current status of the TS_EXTPROC bit. It also allows the user to turn on/off the TS_EXTPROC bit. This is useful because it allows the user to say "stty -extproc", and the LINEMODE option will be automatically disabled, and saying "stty extproc" will re-enable the LINEMODE option.

Quoted 1 line

This option is not supported by gnu coreutils.

OK. It's in *BSD and Minix. Looks like I never wrote a patch for coreutils for this last time around.

Quoted 1 line · Peter Hurley

On 01/19/2015 02:43 PM, Howard Chu wrote:

Quoted 2 lines

So it's really back to the question of, does allowing EXTPROC for regular ttys have _value_?

Does preventing it have value?

I like having the option of turning linemode on and off in a session, for debugging purposes if nothing else.

Quoted 2 lines · Peter Hurley

Quoted 1 line · Howard Chu

Peter Hurley wrote:

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Which brings up another point: only a pty master should be able to set EXTPROC mode. Right now, any tty can be set to EXTPROC and the pty slave can even accidentally unset it. This argues for a new pty ioctl() to set EXTPROC in termios. pty_set_termios() can silently merge the bit.

Ok.
I might see if I can get telnet + a revised kernel interface to work together; I'll let you know if this works out.

Quoted 3 lines · Peter Hurley

Ok.
I might see if I can get telnet + a revised kernel interface to work together; I'll let you know if this works out.

The patch here https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=585527#20 still applies cleanly to current Ubuntu netkit-telnet source and works.

You'll have to use a dumb shell to see any effect.

Quoted 5 lines · Howard Chu

Quoted 3 lines · Peter Hurley

Ok.
I might see if I can get telnet + a revised kernel interface to work together; I'll let you know if this works out.

Quoted 1 line

The patch here https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=585527#20 still applies cleanly to current Ubuntu netkit-telnet source and works.

Aw snap. I just quickly glanced at the patch and source, and assumed that the 4 year old patch had been applied.

Quoted 1 line

You'll have to use a dumb shell to see any effect.

I'm testing it now.

Peter Hurley wrote:

Quoted 3 lines

A description of how the pty master uses EXTPROC to implement line mode would be very helpful, especially to people working in the tty code (eg., me, although I don't need it now).

The original patch submission carried this description
http://lkml.iu.edu/hypermail/linux/kernel/1006.2/02428.html

Paraphrased from the 1989 BSD patch by David Borman @ cray.com:

These are the changes needed for the kernel to support LINEMODE in the server.

There is a new bit in the termios local flag word, EXTPROC. When this bit is set, several aspects of the terminal driver are disabled. Input line editing, character echo, and mapping of signals are all disabled. This allows the telnetd to turn off these functions when in linemode, but still keep track of what state the user wants the terminal to be in.

New ioctl:
TIOCSIG Generate a signal to processes in the
current process group of the pty.

There is a new mode for packet driver, the TIOCPKT_IOCTL bit. When packet mode is turned on in the pty, and the EXTPROC bit is set, then whenever the state of the pty is changed, the next read on the master side of the pty will have the TIOCPKT_IOCTL bit set. This allows the process on the server side of the pty to know when the state of the terminal has changed; it can then issue the appropriate ioctl to retrieve the new state.

This reader set EOL2 to DISABLED_CHAR earlier, and left EOL unchanged. I have seen userspace code that expects a line to be no longer than 4096 chars.

Quoted 4 lines

Quoted 2 lines

4. ioctl(TIOCSIG) can send _any_ signal to a different process without permission checks. That's not good.

It can only send to the pty slave. Permissions were already checked when the pty master was opened.

Still not ok.

Quoted 3 lines

What further checks do you think are needed? You think it should be limited to tty-specific signals: INT, QUIT, CONT, TSTP, TTIN, TTOU, WINCH?

Quoted 3 lines · Peter Hurley

This reader set EOL2 to DISABLED_CHAR earlier, and left EOL unchanged. I have seen userspace code that expects a line to be no longer than 4096 chars.

Userspace code that does this is going to be very fragile. Input from the tty really should be considered completely untrustworthy.

For example, what if, while the tty is in canonical (ICANON) mode, an attacker sets PARMRK and clears IGNPAR, IGNBRK, and BRKINT on the tty and then proceeds to send a large number of breaks or parity errors down the tty?

That will certainly cause a buffer overflow of said userspace process. So if there is *any* setuid program which isn't defensively checking for buffer overruns it's kind of screwed anyway.

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

4. ioctl(TIOCSIG) can send _any_ signal to a different process without permission checks. That's not good.

It can only send to the pty slave. Permissions were already checked when the pty master was opened.

Still not ok.

Interestingly, TIOCSIG is supported by FreeBSD *and* OpenBSD, and without any kind of checks. That being said, I can certainly think of potential security threats that could happen if a malicious program were to run a setuid program (say, /bin/passwd) under a pty, and then proceeded to send it a one or more of non-trappable signals -- for example, either SIGKILL or SIGSTOP -- to either widen the window for a race condition, or to kill a setuid program at a particularly inconvenient time.

It might be interesting to see if someone could figure out an attack that utilized this ioctl, and then demonstrated it on OpenBSD. :-)

Quoted 3 lines

Quoted 3 lines

What further checks do you think are needed? You think it should be limited to tty-specific signals: INT, QUIT, CONT, TSTP, TTIN, TTOU, WINCH?

Definitely, and we should do this sooner rather than later IMHO. Preferably before someone figures out whether or not it's possible to attack OpenBSD and FreeBSD using this ioctl, and requests a CVE. :-)