Attachment 22323 Details for Bug 58081

[patch] lha-security-code-review.diff

lha-security-code-review.diff (text/plain), 10.10 KB, created by Thomas Biege on 2004-07-21 20:18:22 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Thomas Biege

Created: 2004-07-21 20:18:22 UTC

Size: 10.10 KB

patch

obsolete

>--- header.c.my-orig	2004-07-21 11:25:04.000000000 +0200
>+++ header.c	2004-07-21 13:34:12.000000000 +0200
>@@ -656,8 +656,10 @@
> 	}
> 
> 	if (dir_length) {
>-		strcat(dirname, hdr->name);
>-		strcpy(hdr->name, dirname);
>+		strncat(dirname, hdr->name, sizeof(dirname)-strlen(dirname));
>+		dirname[sizeof(dirname)-1] = 0;
>+		strncpy(hdr->name, dirname, sizeof(hdr->name));
>+		hdr->name[sizeof(hdr->name)-1] = 0;
> 		name_length += dir_length;
> 	}
> 
>--- lhadd.c.my-orig	2004-07-21 11:25:03.000000000 +0200
>+++ lhadd.c	2004-07-21 14:10:28.000000000 +0200
>@@ -10,6 +10,8 @@
> #include "lha.h"
> /* ------------------------------------------------------------------------ */
> static void     remove_files();
>+int 		strcmp_filename(char *, char *);
>+void		build_backup_name(char *, char *);
> 
> static char     new_archive_name_buffer[FILENAME_LENGTH];
> static char    *new_archive_name;
>@@ -21,7 +23,6 @@
> {
> 	long            header_pos, next_pos, org_pos, data_pos;
> 	long            v_original_size, v_packed_size;
>-	int             mode;
> 
> 	reading_filename = hdr->name;
> 	writting_filename = temporary_name;
>@@ -35,6 +36,8 @@
> 	if ((hdr->unix_mode & UNIX_FILE_SYMLINK) == UNIX_FILE_SYMLINK) {
> 		char            buf[256], *b1, *b2;
> 		if (!quiet) {
>+			/* make sure we use a zero-terminated buffer */
>+			hdr->name[255] = 0;
> 			strcpy(buf, hdr->name);
> 			b1 = strtok(buf, "|");
> 			b2 = strtok(NULL, "|");
>@@ -211,8 +214,11 @@
> 				add_sp(&sp, hdr.name, strlen(hdr.name) + 1);
> 		}
> 		else if ((hdr.unix_mode & UNIX_FILE_TYPEMASK) == UNIX_FILE_DIRECTORY) {
>+			/* make sure we use a zero-terminated buffer */
>+			hdr.name[sizeof(hdr.name)-1] = 0;
> 			strcpy(name, hdr.name);
> 			len = strlen(name);
>+			/* XXX thomas: what about multiple '/' or about ".." */
> 			if (len > 0 && name[len - 1] == '/')
> 				name[--len] = '\0';	/* strip tail '/' */
> 			if (stat(name, &stbuf) >= 0)	/* exist ? */
>@@ -237,16 +243,20 @@
> 
> 	old_header_pos = ftell(oafp);
> 	while (get_header(oafp, &ahdr)) {
>+		/* make sure we use a zero-terminated buffer */
>+		ahdr.name[sizeof(ahdr.name)-1] = 0;
> 		strcpy(lpath, ahdr.name);
> 		b1 = strtok(lpath, "|");
> 		b2 = strtok(NULL, "|");
> 		if (need_file(b1)) {	/* skip */
> 			fseek(oafp, ahdr.packed_size, SEEK_CUR);
> 			if (noexec || !quiet)
>+			{
> 				if (b2 != NULL)
> 					printf("delete %s -> %s\n", b1, b2);
> 				else
> 					printf("delete %s\n", b1);
>+			}
> 		}
> 		else {		/* copy */
> 			if (noexec) {
>--- lha.h.my-orig	2004-07-21 11:25:04.000000000 +0200
>+++ lha.h	2004-07-21 13:09:26.000000000 +0200
>@@ -11,6 +11,7 @@
> 		lharc.h		interface.h		slidehuf.h
> */
> #include <stdio.h>
>+#include <unistd.h>
> #include <errno.h>
> #include <ctype.h>
> #include <sys/types.h>
>--- lha_macro.h.my-orig	2004-07-21 11:25:04.000000000 +0200
>+++ lha_macro.h	2004-07-21 13:57:19.000000000 +0200
>@@ -53,7 +53,7 @@
> #define SEEK_SET		0
> #define SEEK_CUR		1
> #define SEEK_END		2
>-#endif	/* SEEK_SET
>+#endif	/* SEEK_SET */
> 
> 
> /* non-integral functions */
>--- lharc.c.my-orig	2004-07-21 11:25:03.000000000 +0200
>+++ lharc.c	2004-07-21 14:00:27.000000000 +0200
>@@ -831,9 +831,10 @@
> 	DIRENTRY       *dp;
> 	struct stat     tmp_stbuf, arc_stbuf, fil_stbuf;
> 
>-	strcpy(newname, name);
>+	strncpy(newname, name, sizeof(newname));
>+	newname[sizeof(newname)-1] = 0;
> 	len = strlen(name);
>-	if (len > 0 && newname[len - 1] != '/')
>+	if (len > 0 && newname[len - 1] != '/' && len < (sizeof(newname)-1))
> 		newname[len++] = '/';
> 
> 	dirp = opendir(name);
>@@ -847,6 +848,11 @@
> 
> 	for (dp = readdir(dirp); dp != NULL; dp = readdir(dirp)) {
> 		n = NAMLEN(dp);
>+			if(  len    >= (sizeof(newname)-1) ||
>+			    (len+n) >= (sizeof(newname)-1) ||
>+			         n  <= 0                   ||
>+			    (len+n) <= 0)
>+			break;
> 		strncpy(newname + len, dp->d_name, n);
> 		newname[len + n] = '\0';
> 		if (GETSTAT(newname, &fil_stbuf) < 0)
>@@ -904,7 +910,7 @@
> 		strcpy(temporary_name, TMP_FILENAME_TEMPLATE);
> 	}
> 	else {
>-		sprintf(temporary_name, "%s/lhXXXXXX", extract_directory);
>+		snprintf(temporary_name, sizeof(temporary_name), "%s/lhXXXXXX", extract_directory);
> 	}
> #ifdef MKSTEMP
> 	mkstemp(temporary_name);
>@@ -914,10 +920,16 @@
> #else
> 	char           *p, *s;
> 
>-	strcpy(temporary_name, archive_name);
>+	strncpy(temporary_name, archive_name, sizeof(temporary_name));
>+	temporary_name[sizeof(temporary_name)-1] = 0;
> 	for (p = temporary_name, s = (char *) 0; *p; p++)
> 		if (*p == '/')
> 			s = p;
>+
>+	if( sizeof(temporary_name) - ((size_t) (s-temporary_name)) - 1
>+	    <= strlen("lhXXXXXX"))
>+		    exit(-1);
>+    
> 	strcpy((s ? s + 1 : temporary_name), "lhXXXXXX");
> #ifdef MKSTEMP
> 	mkstemp(temporary_name);
>@@ -935,6 +947,7 @@
> {
> 	register char  *p, *dot;
> 
>+	/* XXX thomas: this can loop "forever", classical semi-decidable problem ;) */
> 	for (p = buffer, dot = (char *) 0; *p; p++) {
> 		if (*p == '.')
> 			dot = p;
>@@ -945,6 +958,7 @@
> 	if (dot)
> 		p = dot;
> 
>+	/* XXX thomas: this may overflow */
> 	strcpy(p, ext);
> }
> 
>@@ -955,6 +969,7 @@
> 	char           *buffer;
> 	char           *original;
> {
>+	/* XXX thomas: this may overflow */
> 	strcpy(buffer, original);
> 	modify_filename_extention(buffer, BACKUPNAME_EXTENTION);	/* ".bak" */
> }
>@@ -965,6 +980,7 @@
> 	char           *buffer;
> 	char           *orginal;
> {
>+	/* XXX thomas: this may overflow */
> 	strcpy(buffer, orginal);
> 	modify_filename_extention(buffer, ARCHIVENAME_EXTENTION);	/* ".lzh" */
> }
>@@ -993,7 +1009,7 @@
> xfopen(name, mode)
> 	char           *name, *mode;
> {
>-	FILE           *fp;
>+	FILE           *fp = NULL;
> 
> 	if (mode[0] == '!') {
> 		int	fd;
>@@ -1047,7 +1063,7 @@
> 		else
> 			return NULL;
> 	}
>-	if (p = (char *) rindex(archive_name, '.')) {
>+	if ( (p = (char *) rindex(archive_name, '.')) ) {
> 		if (strucmp(".LZH", p) == 0
>                     || strucmp(".LHA", p) == 0  /* Amiga */
> 		    || strucmp(".LZS", p) == 0
>@@ -1062,12 +1078,14 @@
> 
> 	if (open_old_archive_1(archive_name, &fp))
> 		return fp;
>-        sprintf(expanded_archive_name, "%s." ARCHIVEEXT_OLD, archive_name);
>+        snprintf(expanded_archive_name, sizeof(expanded_archive_name),
>+		"%s." ARCHIVEEXT_OLD, archive_name);
>         if (open_old_archive_1(expanded_archive_name, &fp)) {
>                 archive_name = expanded_archive_name;
>                 return fp;
>         }
>-	sprintf(expanded_archive_name, "%s.lzh", archive_name);
>+	snprintf(expanded_archive_name, sizeof(expanded_archive_name),
>+		"%s.lzh", archive_name);
> 	if (open_old_archive_1(expanded_archive_name, &fp)) {
> 		archive_name = expanded_archive_name;
> 		return fp;
>@@ -1076,7 +1094,8 @@
> 	 * if ( (errno&0xffff)!=E_PNNF ) { archive_name =
> 	 * expanded_archive_name; return NULL; }
> 	 */
>-	sprintf(expanded_archive_name, "%s.lzs", archive_name);
>+	snprintf(expanded_archive_name, sizeof(expanded_archive_name),
>+		"%s.lzs", archive_name);
> 	if (open_old_archive_1(expanded_archive_name, &fp)) {
> 		archive_name = expanded_archive_name;
> 		return fp;
>@@ -1106,6 +1125,7 @@
> 
> 		fgets(buffer, 1024, stdin);
> 
>+		/* XXX thomas: this may loop "forever" */
> 		for (p = selective; *p; p++)
> 			if (buffer[0] == *p)
> 				return p - selective;
>--- lhext.c.my-orig	2004-07-21 11:25:04.000000000 +0200
>+++ lhext.c	2004-07-21 12:51:44.000000000 +0200
>@@ -82,7 +82,8 @@
> 	register char  *p;
> 
> 	/* make parent directory name into PATH for recursive call */
>-	strcpy(path, name);
>+	memset(path, 0, sizeof(path));
>+	strncpy(path, name, sizeof(path)-1);
> 	for (p = path + strlen(path); p > path; p--)
> 		if (p[-1] == '/') {
> 			*--p = '\0';
>@@ -105,6 +106,7 @@
> 	if (verbose)
> 		printf("Making directory \"%s\".\n", path);
> 
>+	/* XXX thomas: should we keep this unsecure file mode? */
> 	if (mkdir(path, 0777) >= 0)	/* try */
> 		return TRUE;	/* successful done. */
> 	errno = 0;
>@@ -337,7 +339,8 @@
> 			if ((hdr->unix_mode & UNIX_FILE_TYPEMASK) == UNIX_FILE_SYMLINK) {
> 				char            buf[256], *bb1, *bb2;
> 				int             l_code;
>-				strcpy(buf, name);
>+				strncpy(buf, name, sizeof(buf));
>+				buf[sizeof(buf)-1] =  0;
> 				bb1 = strtok(buf, "|");
> 				bb2 = strtok(NULL, "|");
> 
>@@ -366,9 +369,11 @@
> 				if (quiet != TRUE) {
> 					printf("Symbolic Link %s -> %s\n", bb1, bb2);
> 				}
>-				strcpy(name, bb1);	/* Symbolic's name set */
>+				/* XXX thomas: this may overflow */
>+				strncpy(name, bb1, 255); /* Symbolic's name set */
>+				name[255] = 0;
> #else
>-				sprintf(buf, "%s -> %s", bb1, bb2);
>+				snprintf(buf, sizeof(buf), "%s -> %s", bb1, bb2);
> 				warning("Can't make Symbolic Link", buf);
> 				return;
> #endif
>--- lhlist.c.my-orig	2004-07-21 11:25:04.000000000 +0200
>+++ lhlist.c	2004-07-21 13:27:47.000000000 +0200
>@@ -251,7 +251,8 @@
> 			printf(" %s", hdr->name);
> 		else {
> 			char            buf[256], *b1, *b2;
>-			strcpy(buf, hdr->name);
>+			strncpy(buf, hdr->name, sizeof(buf));
>+			buf[sizeof(buf)-1] = 0;			
> 			b1 = strtok(buf, "|");
> 			b2 = strtok(NULL, "|");
> 			printf(" %s -> %s", b1, b2);
>--- util.c.my-orig	2004-07-21 11:25:04.000000000 +0200
>+++ util.c	2004-07-21 13:25:46.000000000 +0200
>@@ -13,7 +13,6 @@
>  * 1992 modified by Masaru Oki #ifndef USESTRCASECMP added. Mar 31 1992
>  * modified by Masaru Oki #ifdef NOMEMSET added.
>  */
>-#include <errno.h>
> 
> /* ------------------------------------------------------------------------ */
> extern unsigned short crc;
>@@ -276,21 +275,27 @@
> 	char           *path;
> {
> 	int             stat, rtn = 0;
>-	char           *cmdname;
>-	if ((cmdname = (char *) malloc(strlen(RMDIRPATH) + 1 + strlen(path) + 1))
>-	    == 0)
>+	pid_t		child;
>+
>+
>+	/* XXX thomas: shell meta chars in path could exec commands */
>+	/* therefore we should avoid using system() */
>+	if ((child = fork()) < 0)
>+		return (-1);	/* fork error */
>+	else if (child) {	/* parent process */
>+		while (child != wait(&stat))	/* ignore signals */
>+			continue;
>+	}
>+	else {			/* child process */
>+		execl(RMDIRPATH, "rmdir", path, (char *) 0);
>+		/* never come here except execl is error */
> 		return (-1);
>-	strcpy(cmdname, RMDIRPATH);
>-	*(cmdname + strlen(RMDIRPATH)) = ' ';
>-	strcpy(cmdname + strlen(RMDIRPATH) + 1, path);
>-	if ((stat = system(cmdname)) < 0)
>-		rtn = -1;	/* fork or exec error */
>-	else if (stat) {	/* RMDIR command error */
>-		errno = EIO;
>-		rtn = -1;
> 	}
>-	free(cmdname);
>-	return (rtn);
>+	if (stat != 0) {
>+		errno = EIO;	/* cannot get error num. */
>+		return (-1);
>+	}
>+	return (0);
> }
> 
> /* ------------------------------------------------------------------------ */

Actions: View | Diff

Attachments on bug 58081: 22305 | 22323 | 22324 | 22365 | 22366 | 22367