bin/80346: [PATCH] Misuse of el_init() can lead multiple programs to SIGSEGV

[Wojciech A. Koszek]

>Number:         80346
>Category:       bin
>Synopsis:       [PATCH] Misuse of el_init() can lead multiple programs to SIGSEGV
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Mon Apr 25 23:30:15 GMT 2005
>Closed-Date:
>Last-Modified:
>Originator:     Wojciech A. Koszek
>Release:        FreeBSD 5.3-RELEASE-p5 i386
>Organization:
>Environment:
System: FreeBSD dunstan.freebsd.czest.pl 5.3-RELEASE-p5 FreeBSD 5.3-RELEASE-p5 #4: Sun Mar 6 10:50:43 CET 2005 dunstan at ho:/usr/obj/usr/src/sys/HOME7 i386


>Description:
There is a problem in multiple programs which make use of editline library
which may lead software to crash.
Code placed in sys/kern/tty.c makes it possible to set TTY size to arbitrary
values. Number of rows and columns is kept in unsigned short variables.
Setting the biggest values for those variables is possible with stty(1):

	$ stty rows -1
	$ stty columns -1

After running applications with changed terminal size, further allocations
are done on given (very big) size. Allocation may not succeed and
applications which don't check return value of el_init() continue executing,
and thus, get SIGSEGV. This problem may have security implications --
programs linked with editline run with raised privileges (lpc). Known
problems:
src/usr.sbin/lpr/lpc/lpc.c
[..]
	if (!el) {
 		el = el_init("lpc", stdin, stdout, stderr);
		hist = history_init();
		history(hist, &he, H_EVENT, 100);
		el_set(el, EL_HIST, history, hist);					
[..]

src/usr.bin/tftp/main.c:
[..]
	vrbose = isatty(0);
	if (vrbose) {
       		el = el_init("tftp", stdin, stdout, stderr);
		hist = history_init();
	}
[..]

src/usr.sbin/cdcontrol/cdcontrol.c
[..]
	if (!el) {
		el = el_init("cdcontrol", stdin, stdout, stderr);
		hist = history_init();
		history(hist, &he, H_EVENT, 100);
[..]

src/usr.sbin/pppctl/pppctl.c
[..]
                history(td.hist, &hev, H_SETSIZE, size);
                td.edit = el_init("pppctl", stdin, stdout, stderr);
[..]

..and probably more.
>How-To-Repeat:
Preparation:

dunstan at dunstan:(~)$ stty rows -1
dunstan at dunstan:(~)$ stty columns -1
dunstan at dunstan:(~)$ stty -a | head -1
speed 38400 baud; 65535 rows; 65535 columns;

dunstan at dunstan:(~)$ lpc
zsh: segmentation fault  lpc

>Fix:
*Library* fix
el_init() should check result of term_init() and return NULL if term_init()
returned -1. Every application has to be corrected separately to properly
handle return value of el_init().

--- diff.0.el.c begins here ---
--- /usr/src/lib/libedit/el.c	Tue Apr 26 00:51:51 2005
+++ src/lib/libedit/el.c	Tue Apr 26 01:01:20 2005
@@ -74,14 +74,21 @@ el_init(const char *prog, FILE *fin, FIL
 	el->el_infd = fileno(fin);
 	el->el_outfile = fout;
 	el->el_errfile = ferr;
-	el->el_prog = strdup(prog);
+	if ((el->el_prog = strdup(prog)) == NULL) {
+		el_free(el);
+		return (NULL);
+	}
 
 	/*
          * Initialize all the modules. Order is important!!!
          */
 	el->el_flags = 0;
 
-	(void) term_init(el);
+	if (term_init(el) == -1) {
+		free(el->el_prog);
+		el_free(el);
+		return (NULL);
+	}
 	(void) key_init(el);
 	(void) map_init(el);
 	if (tty_init(el) == -1)
--- diff.0.el.c ends here ---

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