>Number:         21741
>Category:       bin
>Synopsis:       Buffer overrun and wrong usage of  islower() in less 381
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    bin-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat May 31 20:52:00 UTC 2003
>Closed-Date:
>Last-Modified:
>Originator:     Christian Biere
>Release:        NetBSD 1.6R
>Organization:
>Environment:

>Description:
There's a possible buffer overrun in tags.c in findgtag(). If
the environment variable LESSGLOBALTAGS is large enough, sprintf() will
cause a buffer overrun of the local variable `command' which can hold
512 chars.
The functions islower() and other ctype functions (resp. macros) are not
used correctly, thus causing undefined behaviour on machines with char
being signed char.
The bugs above can be fixed by the attached patch.


However, there's a further problem:
If defined the environment variable LESSBINFMT is used as second
parameter (the format string) for sprintf(). The used buffer can hold
only 8 chars. This limit is not mentioned anywhere in the man page
and LESSBINFMT can cause all kinds of havoc sprintf() can cause if used
"incorrectly".

>How-To-Repeat:


>Fix:


--Multipart_Sat__31_May_2003_22:51:41_+0200_081bd000
Content-Type: text/plain;
 name="less.udif"
Content-Disposition: attachment;
 filename="less.udif"
Content-Transfer-Encoding: 7bit

--- command.c	2003/05/31 17:02:06	1.1
+++ command.c	2003/05/31 19:09:21
@@ -203,7 +203,7 @@
 			every_first_cmd = save(cbuf);
 		break;
 	case A_OPT_TOGGLE:
-		toggle_option(optchar, cbuf, optflag);
+		toggle_option((unsigned char) optchar, cbuf, optflag);
 		optchar = '\0';
 		break;
 	case A_F_BRACKET:
@@ -403,7 +403,7 @@
 				if (cmd_char(c) == CC_QUIT)
 					return (MCA_DONE);
 				p = get_cmdbuf();
-				lc = islower(p[0]);
+				lc = islower((unsigned char) p[0]);
 				o = findopt_name(&p, &oname, NULL);
 				if (o != NULL)
 				{
@@ -413,15 +413,15 @@
 					 * display the full option name.
 					 */
 					optchar = o->oletter;
-					if (!lc && islower(optchar))
-						optchar = toupper(optchar);
+					if (!lc && islower((unsigned char) optchar))
+						optchar = toupper((unsigned char) optchar);
 					cmd_reset();
 					mca_opt_toggle();
 					for (p = oname;  *p != '\0';  p++)
 					{
 						c = *p;
-						if (!lc && islower(c))
-							c = toupper(c);
+						if (!lc && islower((unsigned char) c))
+							c = toupper((unsigned char) c);
 						if (cmd_char(c) != CC_OK)
 							return (MCA_DONE);
 					}
--- opttbl.c	2003/05/31 19:12:34	1.1
+++ opttbl.c	2003/05/31 19:12:52
@@ -454,7 +454,7 @@
 	{
 		if (o->oletter == c)
 			return (o);
-		if ((o->otype & TRIPLE) && toupper(o->oletter) == c)
+		if ((o->otype & TRIPLE) && toupper((unsigned char) o->oletter) == c)
 			return (o);
 	}
 	return (NULL);
--- tags.c	2003/05/31 17:06:02	1.1
+++ tags.c	2003/05/31 20:12:38
@@ -498,7 +498,7 @@
 #if !HAVE_POPEN
 		return TAG_NOFILE;
 #else
-		char command[512];
+		char *command;
 		char *flag;
 		char *qtag;
 		char *cmd = lgetenv("LESSGLOBALTAGS");
@@ -528,10 +528,12 @@
 		qtag = shell_quote(tag);
 		if (qtag == NULL)
 			qtag = tag;
+		command = (char *) ecalloc(strlen(cmd)+strlen(flag)+strlen(qtag)+5, sizeof(char));
 		sprintf(command, "%s -x%s %s", cmd, flag, qtag);
 		if (qtag != tag)
 			free(qtag);
 		fp = popen(command, "r");
+		free(command);
 #endif
 	}
 	if (fp != NULL)
@@ -539,6 +541,7 @@
 		while (fgets(buf, sizeof(buf), fp))
 		{
 			char *name, *file, *line;
+			int len;
 
 			if (sigs)
 			{
@@ -548,8 +551,8 @@
 #endif
 				return TAG_INTR;
 			}
-			if (buf[strlen(buf) - 1] == '\n')
-				buf[strlen(buf) - 1] = 0;
+			if ((len = strlen(buf)) && buf[len - 1] == '\n')
+				buf[len - 1] = 0;
 			else
 			{
 				int c;
@@ -712,12 +715,12 @@
 {
 	char *p = buf;
 
-	for (*tag = p;  *p && !isspace(*p);  p++)	/* tag name */
+	for (*tag = p;  *p && !isspace((unsigned char) *p);  p++)	/* tag name */
 		;
 	if (*p == 0)
 		return (-1);
 	*p++ = 0;
-	for ( ;  *p && isspace(*p);  p++)		/* (skip blanks) */
+	for ( ;  *p && isspace((unsigned char) *p);  p++)		/* (skip blanks) */
 		;
 	if (*p == 0)
 		return (-1);
@@ -725,27 +728,27 @@
 	 * If the second part begin with other than digit,
 	 * it is assumed tag type. Skip it.
 	 */
-	if (!isdigit(*p))
+	if (!isdigit((unsigned char) *p))
 	{
-		for ( ;  *p && !isspace(*p);  p++)	/* (skip tag type) */
+		for ( ;  *p && !isspace((unsigned char) *p);  p++)	/* (skip tag type) */
 			;
-		for (;  *p && isspace(*p);  p++)	/* (skip blanks) */
+		for (;  *p && isspace((unsigned char) *p);  p++)	/* (skip blanks) */
 			;
 	}
-	if (!isdigit(*p))
+	if (!isdigit((unsigned char) *p))
 		return (-1);
 	*line = p;					/* line number */
-	for (*line = p;  *p && !isspace(*p);  p++)
+	for (*line = p;  *p && !isspace((unsigned char) *p);  p++)
 		;
 	if (*p == 0)
 		return (-1);
 	*p++ = 0;
-	for ( ; *p && isspace(*p);  p++)		/* (skip blanks) */
+	for ( ; *p && isspace((unsigned char) *p);  p++)		/* (skip blanks) */
 		;
 	if (*p == 0)
 		return (-1);
 	*file = p;					/* file name */
-	for (*file = p;  *p && !isspace(*p);  p++)
+	for (*file = p;  *p && !isspace((unsigned char) *p);  p++)
 		;
 	if (*p == 0)
 		return (-1);

--Multipart_Sat__31_May_2003_22:51:41_+0200_081bd000--
>Release-Note:
>Audit-Trail:
>Unformatted:
 This is a multi-part message in MIME format.
 
 --Multipart_Sat__31_May_2003_22:51:41_+0200_081bd000
 Content-Type: text/plain; charset=US-ASCII
 Content-Transfer-Encoding: 7bit