bin/77462: [PATCH] Use of uninitialized variables in lpc(8) (SIGSEGV)

[Wojciech A. Koszek]

>Number:         77462
>Category:       bin
>Synopsis:       [PATCH] Use of uninitialized variables in lpc(8) (SIGSEGV)
>Confidential:   no
>Severity:       serious
>Priority:       high
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Sun Feb 13 19:50:17 GMT 2005
>Closed-Date:
>Last-Modified:
>Originator:     Wojciech A. Koszek
>Release:        FreeBSD 5.3-STABLE i386
>Organization:
>Environment:
System: FreeBSD dunstan.freebsd.czest.pl 5.3-STABLE FreeBSD 5.3-STABLE #0: Sat Feb 12 11:15:23 CET 2005 root at dunstan.freebsd.czest.pl:/usr/obj/usr/src/sys/HOME6 i386


Tests made on -STABLE and -CURRENT.

>Description:
lpc(8) uses editline(3) library to handle user input. If data comes from
terminal, it uses el_gets(3) function. Overwise, fgets(3) is used. Structures
for el_* functions have to be initialized before making use of them.
	User may send malicious data throught fgets(3), skipping variables
initialization, and the same, causing lpc to get SIGSEGV.
My analisis has shown it *might* be expoited in theory. lpc(8) is SGID with
EGID == daemon.

>How-To-Repeat:
Repeating is trivial:

$ echo "..:" | lpc

or

$ cat /dev/random | lpc

>Fix:
Solution is very simple. Structures are used for processing data either from
el_gets() or fgets(), so initialization has to be done earlier. Attached
patch [lpc.0.patch] should correct this problem. 


--- lpc.0.patch begins here ---
Index: src/usr.sbin/lpr/lpc/lpc.c
===================================================================
RCS file: /home/ncvs/src/usr.sbin/lpr/lpc/lpc.c,v
retrieving revision 1.28
diff -u -r1.28 lpc.c
--- src/usr.sbin/lpr/lpc/lpc.c	13 Oct 2003 07:24:22 -0000	1.28
+++ src/usr.sbin/lpr/lpc/lpc.c	18 Nov 2004 14:23:21 -0000
@@ -162,27 +162,27 @@
 	bp = NULL;
 	el = NULL;
 	hist = NULL;
+
+	el = el_init("lpc", stdin, stdout, stderr);
+	hist = history_init();
+	history(hist, &he, H_EVENT, 100);
+	el_set(el, EL_HIST, history, hist);
+	el_set(el, EL_EDITOR, "emacs");
+	el_set(el, EL_PROMPT, lpc_prompt);
+	el_set(el, EL_SIGNAL, 1);
+	el_source(el, NULL);
+
 	for (;;) {
 		if (fromatty) {
-			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);
-				el_set(el, EL_EDITOR, "emacs");
-				el_set(el, EL_PROMPT, lpc_prompt);
-				el_set(el, EL_SIGNAL, 1);
-				el_source(el, NULL);
-				/*
-				 * EditLine init may call 'cgetset()' to set a
-				 * capability-db meant for termcap (eg: to set
-				 * terminal type 'xterm').  Reset that now, or
-				 * that same db-information will be used for
-				 * printcap (giving us an "xterm" printer, with
-				 * all kinds of invalid capabilities...).
-				 */
-				cgetset(NULL);
-			}
+			/*
+			 * EditLine init may call 'cgetset()' to set a
+			 * capability-db meant for termcap (eg: to set
+			 * terminal type 'xterm').  Reset that now, or
+			 * that same db-information will be used for
+			 * printcap (giving us an "xterm" printer, with
+			 * all kinds of invalid capabilities...).
+			 */
+			cgetset(NULL);
 			if ((bp = el_gets(el, &num)) == NULL || num == 0)
 				quit(0, NULL);
 
--- lpc.0.patch ends here ---

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