>Number:         3576
>Category:       bin
>Synopsis:       getline() in ipf may overruns buffer and has some minor problems
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    bin-bug-people (Utility Bug People)
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon May  5 08:05:02 1997
>Last-Modified:
>Originator:     enami tsugutomo
>Organization:
	An individual
>Release:        NetBSD-current 1997 May 03
>Environment:
System: NetBSD pavlov.enami.ba2.so-net.or.jp 1.2D NetBSD 1.2D (PAVLOV) #246: Sun May 4 16:37:11 JST 1997 enami@pavlov.enami.ba2.so-net.or.jp:/b/netbsd/kernel/compile/PAVLOV i386


>Description:
	getline() in ipf may overruns buffer and has some minor problems.

	* getline() uses internally fgets() and iterates, but always
	use constant `size' for fgets().  It may cause buffer overrun.

	* getline() may acces buffer[-1] if input line is emplty (just
	\n).  In that case, \n is replaced with \0 and further
	p[strlen(p) - 1] may access p[-1].

	* getline() tests *str == '\n' as well as *str == '\0' but
	'\n' case never happens because \n is replaced by \0 in
	internal loop.

	* In procfile(), getline() is called with second arg
	sizeof(line) - 1, but I can't find any reason to reserve `-1'.
	Passing sizeof (line) is enough since terminating null is
	handled by getline() well.

	* getline() implicilty split too long line into two lines.
	parse() may cause an error in that case, but more direct error
	may help user.

	* procfile() tests if result of getline() has \n, but \n is
	replaced with NUL by getline(), so the test is not needed.

	* missing space after comma and one indent fix according to
	KNF.

>How-To-Repeat:
	please read the source.
>Fix:
	Here is my fix:

Index: ipf.c
===================================================================
RCS file: /a/cvsroot/NetBSD/src/usr.sbin/ipf/ipf/ipf.c,v
retrieving revision 1.1.1.1
diff -c -r1.1.1.1 ipf.c
*** ipf.c	1997/04/13 16:42:27	1.1.1.1
--- ipf.c	1997/05/02 01:07:11
***************
*** 188,199 ****
  		exit(1);
  	}
  
! 	while (getline(line, sizeof(line)-1, fp)) {
  		/*
! 		 * treat both CR and LF as EOL
  		 */
- 		if ((s = index(line, '\n')))
- 			*s = '\0';
  		if ((s = index(line, '\r')))
  			*s = '\0';
  		/*
--- 188,197 ----
  		exit(1);
  	}
  
! 	while (getline(line, sizeof(line), fp)) {
  		/*
! 		 * treat CR as EOL.  LF is converted to NUL by getline().
  		 */
  		if ((s = index(line, '\r')))
  			*s = '\0';
  		/*
***************
*** 206,212 ****
  			continue;
  
  		if (opts & OPT_VERBOSE)
! 			(void)fprintf(stderr, "[%s]\n",line);
  
  		fr = parse(line);
  		(void)fflush(stdout);
--- 204,210 ----
  			continue;
  
  		if (opts & OPT_VERBOSE)
! 			(void)fprintf(stderr, "[%s]\n", line);
  
  		fr = parse(line);
  		(void)fflush(stdout);
***************
*** 234,240 ****
  					perror("ioctl(SIOCZRLST)");
  				else {
  					printf("hits %ld bytes %ld ",
! 						fr->fr_hits, fr->fr_bytes);
  					printfr(fr);
  				}
  			} else if ((opts & OPT_REMOVE) &&
--- 232,238 ----
  					perror("ioctl(SIOCZRLST)");
  				else {
  					printf("hits %ld bytes %ld ",
! 					    fr->fr_hits, fr->fr_bytes);
  					printfr(fr);
  				}
  			} else if ((opts & OPT_REMOVE) &&
***************
*** 247,257 ****
  			}
  		}
  	}
  	(void)fclose(fp);
  }
  
  /*
!  * Similar to fgets(3) but can handle '\\'
   */
  static char *getline(str, size, file)
  register char	*str;
--- 245,261 ----
  			}
  		}
  	}
+ 	if (ferror(fp) || !feof(fp)) {
+ 		fprintf(stderr, "%s: %s: file error or line too long\n",
+ 		    name, file);
+ 		exit(1);
+ 	}
  	(void)fclose(fp);
  }
  
  /*
!  * Similar to fgets(3) but can handle '\\' and NL is converted to NUL.
!  * Returns NULL if error occured, EOF encounterd or input line is too long.
   */
  static char *getline(str, size, file)
  register char	*str;
***************
*** 259,275 ****
  FILE	*file;
  {
  	register char *p;
  
  	do {
! 		for (p = str;; p+= strlen(p) - 1) {
! 			if (!fgets(p, size, file))
! 				return(NULL);
! 			p[strlen(p) -1] = '\0';
! 			if (p[strlen(p) - 1] != '\\')
  				break;
  		}
! 	} while (*str == '\0' || *str == '\n');
! 	return(str);
  }
  
  
--- 263,295 ----
  FILE	*file;
  {
  	register char *p;
+ 	int s, len;
  
  	do {
! 		for (p = str, s = size;; p += len, s -= len) {
! 			/*
! 			 * if an error occured, EOF was encounterd, or there
! 			 * was no room to put NUL, return NULL.
! 			 */
! 			if (fgets(p, s, file) == NULL)
! 				return (NULL);
! 			len = strlen(p);
! 			/*
! 			 * if there was no room to put newline or there was
! 			 * no room to put entire line, return NULL.
! 			 */
! 			if (len == 0 || p[--len] != '\n')
! 				return (NULL);
! 			p[len] = '\0';
! 			/*
! 			 * if the line just read has only newline or does not
! 			 * end with '\\', we have read whole line.
! 			 */
! 			if (len == 0 || p[--len] != '\\')
  				break;
  		}
! 	} while (*str == '\0');
! 	return (str);
  }
  
  
>Audit-Trail:
>Unformatted: