user/2393: Dangerous string operations in mg

[Vincent Labrecque <limitln@psyfreaks.ca>]

>Number:         2393
>Category:       user
>Synopsis:       mg does a few unsafe string operations (str{,n}{cpy,cat})
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    bugs
>State:          open
>Class:          change-request
>Submitter-Id:   net
>Arrival-Date:   Mon Feb 11 20:40:01 MST 2002
>Last-Modified:
>Originator:     Vincent Labrecque
>Organization:
None
>Release:        OpenBSD 3.0-current
>Environment:
        <machine, os, target, libraries (multiple lines)>
        System      : OpenBSD 3.0
        Architecture: OpenBSD.i386
        Machine     : i386
>Description:
        <precise description of the problem (multiple lines)>
	There isn't much to say.

	Mg does a few strcpy and strcat calls.  Just fix them.

	At the same time, I simplified some parts of the code (definekey()).


>How-To-Repeat:
        <code/input/activities to reproduce the problem (multiple lines)>
	read the mg code

>Fix:
	Look at and alpply this diff.

Common subdirectories: /sources/src/usr.bin/mg/CVS and /sources/lsrc/usr.bin/mg/CVS
diff -u /sources/src/usr.bin/mg/buffer.c /sources/lsrc/usr.bin/mg/buffer.c
--- /sources/src/usr.bin/mg/buffer.c	Sat Jan 26 16:02:53 2002
+++ /sources/lsrc/usr.bin/mg/buffer.c	Sun Feb 10 17:59:56 2002
@@ -453,7 +453,7 @@
 	} while (i++ < defb_nmodes);
 	bp->b_fname[0] = '\0';
 	bzero(&bp->b_fi, sizeof(bp->b_fi));
-	(void) strcpy(bp->b_bname, bname);
+	(void) strlcpy(bp->b_bname, bname, sizeof bp->b_bname);
 	lp->l_fp = lp;
 	lp->l_bp = lp;
 	bp->b_bufp = bheadp;
diff -u /sources/src/usr.bin/mg/def.h /sources/lsrc/usr.bin/mg/def.h
--- /sources/src/usr.bin/mg/def.h	Sat Jan 26 16:02:53 2002
+++ /sources/lsrc/usr.bin/mg/def.h	Sun Feb 10 17:58:04 2002
@@ -573,7 +573,7 @@
 extern int	 defb_flag;
 extern const char cinfo[];
 extern char	*keystrings[];
-extern char	 pat[];
+extern char	 pat[NPAT];
 #ifndef NO_DPROMPT
 extern char	 prompt[];
 #endif	/* !NO_DPROMPT */
diff -u /sources/src/usr.bin/mg/dir.c /sources/lsrc/usr.bin/mg/dir.c
--- /sources/src/usr.bin/mg/dir.c	Wed May 23 22:05:21 2001
+++ /sources/lsrc/usr.bin/mg/dir.c	Sun Feb 10 17:37:14 2002
@@ -38,7 +38,7 @@
 	if ((s = ereply("Change default directory: ", bufc, NPAT)) != TRUE)
 		return (s);
 	if (bufc[0] == '\0')
-		(void) strcpy(bufc, wdir);
+		(void) strlcpy(bufc, wdir, sizeof bufc);
 	if (chdir(bufc) == -1) {
 		ewprintf("Can't change dir to %s", bufc);
 		return (FALSE);
diff -u /sources/src/usr.bin/mg/echo.c /sources/lsrc/usr.bin/mg/echo.c
--- /sources/src/usr.bin/mg/echo.c	Sat Jan 26 16:02:54 2002
+++ /sources/lsrc/usr.bin/mg/echo.c	Sun Feb 10 22:11:23 2002
@@ -429,113 +429,111 @@
 	if (bclear(bp) == FALSE)
 		return FALSE;
 
-	{	/* this {} present for historical reasons */
-
+	/*
+	 * first get the list of objects.  This list may contain only
+	 * the ones that complete what has been typed, or may be the
+	 * whole list of all objects of this type.  They are filtered
+	 * later in any case.  Set wholelist if the list has been
+	 * cons'ed up just for us, so we can free it later.  We have
+	 * to copy the buffer list for this function even though we
+	 * didn't for complt.  The sorting code does destructive
+	 * changes to the list, which we don't want to happen to the
+	 * main buffer list!
+	 */
+	if ((flags & EFBUF) != 0)
+		wholelist = lh = copy_list(&(bheadp->b_list));
+	else if ((flags & EFFUNC) != 0) {
+		buf[cpos] = '\0';
+		wholelist = lh = complete_function_list(buf, c);
+	} else if ((flags & EFFILE) != 0) {
+		buf[cpos] = '\0';
+		wholelist = lh = make_file_list(buf);
 		/*
-		 * first get the list of objects.  This list may contain only
-		 * the ones that complete what has been typed, or may be the
-		 * whole list of all objects of this type.  They are filtered
-		 * later in any case.  Set wholelist if the list has been
-		 * cons'ed up just for us, so we can free it later.  We have
-		 * to copy the buffer list for this function even though we
-		 * didn't for complt.  The sorting code does destructive
-		 * changes to the list, which we don't want to happen to the
-		 * main buffer list!
-		 */
-		if ((flags & EFBUF) != 0)
-			wholelist = lh = copy_list(&(bheadp->b_list));
-		else if ((flags & EFFUNC) != 0) {
-			buf[cpos] = '\0';
-			wholelist = lh = complete_function_list(buf, c);
-		} else if ((flags & EFFILE) != 0) {
-			buf[cpos] = '\0';
-			wholelist = lh = make_file_list(buf);
-			/*
 			 * We don't want to display stuff up to the / for file
 			 * names preflen is the list of a prefix of what the
 			 * user typed that should not be displayed.
 			 */
-			cp = strrchr(buf, '/');
-			if (cp)
-				preflen = cp - buf + 1;
-		} else
-			panic("broken complt call: flags");
-
+		cp = strrchr(buf, '/');
+		if (cp)
+			preflen = cp - buf + 1;
+	} else
+		panic("broken complt call: flags");
 
-		/*
-		 * Sort the list, since users expect to see it in alphabetic
-		 * order.
-		 */
-		lh2 = lh;
-		while (lh2) {
-			lh3 = lh2->l_next;
-			while (lh3) {
-				if (strcmp(lh2->l_name, lh3->l_name) > 0) {
-					cp = lh2->l_name;
-					lh2->l_name = lh3->l_name;
-					lh3->l_name = cp;
-				}
-				lh3 = lh3->l_next;
+	
+	/*
+	 * Sort the list, since users expect to see it in alphabetic
+	 * order.
+	 */
+	lh2 = lh;
+	while (lh2) {
+		lh3 = lh2->l_next;
+		while (lh3) {
+			if (strcmp(lh2->l_name, lh3->l_name) > 0) {
+				cp = lh2->l_name;
+				lh2->l_name = lh3->l_name;
+				lh3->l_name = cp;
 			}
-			lh2 = lh2->l_next;
+			lh3 = lh3->l_next;
 		}
+		lh2 = lh2->l_next;
+	}
 
-		/*
-		 * First find max width of object to be displayed, so we can
-		 * put several on a line.
-		 */
-		maxwidth = 0;
-		lh2 = lh;
-		while (lh2 != NULL) {
-			for (i = 0; i < cpos; ++i) {
-				if (buf[i] != lh2->l_name[i])
-					break;
-			}
-			if (i == cpos) {
-				width = strlen(lh2->l_name);
-				if (width > maxwidth)
-					maxwidth = width;
-			}
-			lh2 = lh2->l_next;
+	/*
+	 * First find max width of object to be displayed, so we can
+	 * put several on a line.
+	 */
+	maxwidth = 0;
+	lh2 = lh;
+	while (lh2 != NULL) {
+		for (i = 0; i < cpos; ++i) {
+			if (buf[i] != lh2->l_name[i])
+				break;
 		}
-		maxwidth += 1 - preflen;
+		if (i == cpos) {
+			width = strlen(lh2->l_name);
+			if (width > maxwidth)
+				maxwidth = width;
+		}
+		lh2 = lh2->l_next;
+	}
+	maxwidth += 1 - preflen;
 
-		/*
-		 * Now do the display.  objects are written into linebuf until
-		 * it fills, and then put into the help buffer.
-		 */
-		if ((linebuf = malloc((nrow + 1) * sizeof(char))) == NULL)
-			return FALSE;
-		cp = linebuf;
-		width = 0;
-		lh2 = lh;
-		while (lh2 != NULL) {
-			for (i = 0; i < cpos; ++i) {
-				if (buf[i] != lh2->l_name[i])
-					break;
-			}
-			if (i == cpos) {
-				if ((width + maxwidth) > ncol) {
-					*cp = 0;
-					addline(bp, linebuf);
-					cp = linebuf;
-					width = 0;
-				}
-				strcpy(cp, lh2->l_name + preflen);
-				i = strlen(lh2->l_name + preflen);
-				cp += i;
-				for (; i < maxwidth; i++)
-					*cp++ = ' ';
-				width += maxwidth;
-			}
-			lh2 = lh2->l_next;
+	/*
+	 * Now do the display.  objects are written into linebuf until
+	 * it fills, and then put into the help buffer.
+	 */
+	if ((linebuf = malloc((nrow + 1) * sizeof(char))) == NULL)
+		return FALSE;
+	cp = linebuf;
+	width = 0;
+	lh2 = lh;
+	while (lh2 != NULL) {
+		for (i = 0; i < cpos; ++i) {
+			if (buf[i] != lh2->l_name[i])
+				break;
 		}
-		if (width > 0) {
-			*cp = 0;
-			addline(bp, linebuf);
+		if (i == cpos) {
+			if ((width + maxwidth) > ncol) {
+				*cp = '\0';
+				addline(bp, linebuf);
+				cp = linebuf;
+				width = 0;
+			}
+			strcpy(cp, lh2->l_name + preflen);
+			i = strlen(lh2->l_name + preflen);
+			cp += i;
+			for (; i < maxwidth; i++)
+				*cp++ = ' ';
+			width += maxwidth;
 		}
-		free(linebuf);
+		lh2 = lh2->l_next;
+	}
+	if (width > 0) {
+		*cp = 0;
+		addline(bp, linebuf);
 	}
+	free(linebuf);
+	
 	/*
 	 * Note that we free lists only if they are put in wholelist lists
 	 * that were built just for us should be freed.  However when we use
diff -u /sources/src/usr.bin/mg/extend.c /sources/lsrc/usr.bin/mg/extend.c
--- /sources/src/usr.bin/mg/extend.c	Thu Jul  5 12:36:05 2001
+++ /sources/lsrc/usr.bin/mg/extend.c	Sun Feb 10 22:14:11 2002
@@ -495,20 +495,22 @@
  */
 /* ARGSUSED */
 int
-define_key(f, n)
-	int f, n;
+define_key(int f, int n)
 {
-	static char	 buf[48] = "Define key map: ";
-	KEYMAP		*mp;
+	static char buf[48];
+	char tmp[32];
+	KEYMAP *mp;
 
-	buf[16] = '\0';
-	if (eread(buf, &buf[16], 48 - 16, EFNEW) != TRUE)
+	strlcpy(buf, "Define key map: ", sizeof buf);
+	if (eread(buf, tmp, sizeof tmp, EFNEW) != TRUE)
 		return FALSE;
-	if ((mp = name_map(&buf[16])) == NULL) {
-		ewprintf("Unknown map %s", &buf[16]);
+	strlcat(buf, tmp, sizeof buf);
+	if ((mp = name_map(tmp)) == NULL) {
+		ewprintf("Unknown map %s", tmp);
 		return FALSE;
 	}
-	(void)strncat(&buf[16], " key: ", 48 - 16 - 1);
+	strlcat(buf, "key: ", sizeof buf);
+
 	return dobind(mp, buf, FALSE);
 }
 
diff -u /sources/src/usr.bin/mg/file.c /sources/lsrc/usr.bin/mg/file.c
--- /sources/src/usr.bin/mg/file.c	Wed May 23 22:05:22 2001
+++ /sources/lsrc/usr.bin/mg/file.c	Sun Feb 10 18:01:01 2002
@@ -95,7 +95,7 @@
 	char *fname;
 {
 	BUFFER		*bp;
-	char		 bname[NBUFN], *cp;
+	char		 bname[NBUFN];
 	unsigned int	 count = 1;
 
 	for (bp = bheadp; bp != NULL; bp = bp->b_bufp) {
@@ -103,10 +103,10 @@
 			return bp;
 	}
 	/* new buffer name */
-	strcpy(bname, basename(fname));
-	cp = bname + strlen(bname);
 	for (count = 1; bfind(bname, FALSE) != NULL; count++)
-		sprintf(cp, "<%d>", count);
+		;
+	snprintf(bname, sizeof bname, "%s%d", basename(fname), count);
+
 	return bfind(bname, TRUE);
 }
 
@@ -182,7 +182,7 @@
 	/* cheap */
 	bp = curbp;
 	if (newname != NULL)
-		(void)strcpy(bp->b_fname, newname);
+		(void)strlcpy(bp->b_fname, newname, sizeof bp->b_fname);
 
 	/* hard file open */
 	if ((s = ffropen(fname, needinfo ? bp : NULL)) == FIOERR)
@@ -340,7 +340,7 @@
 	/* old attributes are no longer current */
 	bzero(&curbp->b_fi, sizeof(curbp->b_fi));
 	if ((s = writeout(curbp, adjfname)) == TRUE) {
-		(void)strcpy(curbp->b_fname, adjfname);
+		(void)strlcpy(curbp->b_fname, adjfname, sizeof curbp->b_fname);
 #ifndef NO_BACKUP
 		curbp->b_flag &= ~(BFBAK | BFCHG);
 #else /* !NO_BACKUP */
diff -u /sources/src/usr.bin/mg/re_search.c /sources/lsrc/usr.bin/mg/re_search.c
--- /sources/src/usr.bin/mg/re_search.c	Wed May 23 22:05:25 2001
+++ /sources/lsrc/usr.bin/mg/re_search.c	Sun Feb 10 18:01:46 2002
@@ -430,7 +430,7 @@
 
 	if (s == TRUE) {
 		/* New pattern given */
-		(void)strcpy(re_pat, tpat);
+		(void)strlcpy(re_pat, tpat, sizeof re_pat);
 		if (casefoldsearch)
 			flags = REG_EXTENDED | REG_ICASE;
 		else
diff -u /sources/src/usr.bin/mg/region.c /sources/lsrc/usr.bin/mg/region.c
--- /sources/src/usr.bin/mg/region.c	Wed May 23 22:05:26 2001
+++ /sources/lsrc/usr.bin/mg/region.c	Sun Feb 10 18:01:59 2002
@@ -298,7 +298,7 @@
 		s = ereply("Prefix string (default %s): ",
 			   buf, sizeof buf, prefix_string);
 	if (s == TRUE)
-		(void)strcpy(prefix_string, buf);
+		(void)strlcpy(prefix_string, buf, sizeof prefix_string);
 	/* CR -- use old one */
 	if ((s == FALSE) && (prefix_string[0] != '\0'))
 		s = TRUE;
diff -u /sources/src/usr.bin/mg/search.c /sources/lsrc/usr.bin/mg/search.c
--- /sources/src/usr.bin/mg/search.c	Wed May 23 22:05:26 2001
+++ /sources/lsrc/usr.bin/mg/search.c	Sun Feb 10 17:57:51 2002
@@ -175,7 +175,7 @@
 	for (cip = 0; cip < NSRCH; cip++)
 		cmds[cip].s_code = SRCH_NOPR;
 
-	(void)strcpy(opat, pat);
+	(void)strlcpy(opat, pat, sizeof opat);
 	cip = 0;
 	pptr = -1;
 	clp = curwp->w_dotp;
@@ -216,7 +216,7 @@
 			curwp->w_flag |= WFMOVE;
 			srch_lastdir = dir;
 			(void)ctrlg(FFRAND, 0);
-			(void)strcpy(pat, opat);
+			(void)strlcpy(pat, opat, sizeof pat);
 			return ABORT;
 		case CCHR(']'):
 		case CCHR('S'):
@@ -689,7 +689,7 @@
 
 	/* specified */
 	if (s == TRUE)
-		(void) strcpy(pat, tpat);
+		(void) strlcpy(pat, tpat, sizeof pat);
 	/* CR, but old one */
 	else if (s == FALSE && pat[0] != 0)
 		s = TRUE;


>Audit-Trail:
>Unformatted: