misc/60697: [patch] pseudo-tty hack versus telnet race causes data loss.

[Peter Edwards]

>Number:         60697
>Category:       misc
>Synopsis:       [patch] pseudo-tty hack versus telnet race causes data loss.
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Mon Dec 29 10:00:29 PST 2003
>Closed-Date:
>Last-Modified:
>Originator:     Peter Edwards
>Release:        5.2 -CURRENT as of Dec 23, 2003
>Organization:
>Environment:
FreeBSD hippo 5.2-CURRENT FreeBSD 5.2-CURRENT #2: Tue Dec 23 12:46:05 GMT 2003     petere at hippo:/scratch/obj/scratch/src/sys/HIPPO  i386
>Description:
The pty driver appears to have a hack that prevents a deadlock at
the cost of discarding data: In the event that certain ioctls are
invoked on the master side of the pty, it'll drop any "unsent" data.
This can be seen at line 702 of kern/tty_pty.c

> 
>       switch (cmd) {
> #ifdef COMPAT_43
>               case TIOCSETP:
>               case TIOCSETN:
> #endif
>               case TIOCSETD:
>               case TIOCSETA:
>               case TIOCSETAW:
>               case TIOCSETAF:
>         /*
>          * IF CONTROLLER STTY THEN MUST FLUSH TO PREVENT A HANG.
>          * ttywflush(tp) will hang if there are characters in
>          * the outq.
>          */
>         ndflush(&tp->t_outq, tp->t_outq.c_cc);
>         break;
> 

I believe this is here to avoid having the master process waiting
until all the data written to the slave side is drained for some
operations, because its that process itself that needs to do the
draining.

However, this can cause problems with the telnet server. Here's an
example:

1: Telnetd answers an incoming call, grabs the master side of a
pseudo-terminal, and sends a bunch of telnet commands, one of which
is a "WILL ECHO" request.

2a: Just after this, it launches a new process to run "login": This
is given the slave side of the pseudo-terminal for its std(in|out|err)

2b: Meanwhile, the remote telnet client sends a response to
the WILL echo: In this case, a "DONT" ECHO.

2c: Meanwhile, the login process writes the phrase: "login: " to the
slave side of the terminal

3: If the telnetd process sees the network response before it
sees the pty data, the echo response causes a call to "tcsetattr(...,
TCSANOW, ...)".

This ends up as an ioctl(..., TIOCSETA, ...) which gets handled by
ptyioctl(), and exercises the code quoted above.

This, unfortunately, causes the "login:" written by the login process
to be discarded sometimes. For a human interacting with the telnet
server, this is merely an annoyance, but for automated tools, it
can be more of an issue, and just adds to the hackery that such
evil programs need to perform. I'm sure there are other occaisons
where the same data loss can rear its head beyond the initial flurry
of telnet option processing.

I'm sure a more sophisticated hack can work around this issue, but
the "TIOCSETA" operation that telnetd invokes does not actually
cause a blocking operation to occur: I think the check against
TIOCSETA is unneccessary. i.e., I propose the patch below.

>How-To-Repeat:
Write a dumb telnet client that responds negatively to any telnet
option requests. If you want to make it more reproducable, add a sleep
into the telnetd code in sys_term.c:startslave() for the parent (10ms
is plenty)
If someone's interested, I can spend some time ripping a minimal
example from my code.


>Fix:
Index: kern/tty_pty.c
===================================================================
RCS file: /usr/cvs/FreeBSD-CVS/src/sys/kern/tty_pty.c,v
retrieving revision 1.112
diff -u -r1.112 tty_pty.c
--- kern/tty_pty.c      9 Nov 2003 09:17:24 -0000       1.112
+++ kern/tty_pty.c      29 Dec 2003 17:17:42 -0000
@@ -705,7 +705,6 @@
                case TIOCSETN:
 #endif
                case TIOCSETD:
-               case TIOCSETA:
                case TIOCSETAW:
                case TIOCSETAF:
                        /*

>Release-Note:
>Audit-Trail:
>Unformatted: