standards/81287: [PATCH]: fingerd(8) might send a line not ending in CRLF

Rostislav Krasny rosti.bsd at gmail.com
Thu May 19 19:50:08 PDT 2005


>Number:         81287
>Category:       standards
>Synopsis:       [PATCH]: fingerd(8) might send a line not ending in CRLF
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    freebsd-standards
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Fri May 20 02:50:07 GMT 2005
>Closed-Date:
>Last-Modified:
>Originator:     Rostislav Krasny
>Release:        FreeBSD 5.4-RELEASE
>Organization:
>Environment:
FreeBSD mercury 5.4-RELEASE FreeBSD 5.4-RELEASE #0: Sun May  8 00:55:11 IDT 2005     root at mercury:/usr/obj/usr/src/sys/MYKERNEL  i386
>Description:
According to RFC1288:

2.2.  Data format

   Any data transferred MUST be in ASCII format, with no parity, and
   with lines ending in CRLF (ASCII 13 followed by ASCII 10).  This
   excludes other character formats such as EBCDIC, etc.  This also
   means that any characters between ASCII 128 and ASCII 255 should
   truly be international data, not 7-bit ASCII with the parity bit set.

But fingerd(8) sends some specific lines not ending in CRLF. These specific lines in the code are:

"must provide username\r\n"
"forwarding service denied\r\n"

Although the strings are ending in CRLF the fingerd(8) adds an extra '\n' character to the end during a transmission.
>How-To-Repeat:
enable 'fingerd -s' or 'fingerd -s -l' in /etc/inetd.conf
start or restart the inetd(8) daemon
run 'nc -v 127.0.0.1 79' and press [Enter] when connected
run 'nc -v 127.0.0.1 79' and press [@] and then [Enter] when connected

In both cases you will get an extra '\n' character in the output.
>Fix:
The simple fix is just replacing all puts() functions to fputs() functions. But I made a much better patch that also fixes several other little problems and also simplifies the code. See my explanations after the patch.

diff -ur fingerd.orig/fingerd.c /usr/src/libexec/fingerd/fingerd.c
--- fingerd.orig/fingerd.c	Thu May 12 17:58:29 2005
+++ /usr/src/libexec/fingerd/fingerd.c	Fri May 20 04:14:04 2005
@@ -48,6 +48,7 @@
 #include <sys/types.h>
 #include <sys/param.h>
 #include <sys/socket.h>
+#include <sys/wait.h>
 #include <netinet/in.h>
 #include <netinet/tcp.h>
 #include <arpa/inet.h>
@@ -103,18 +104,15 @@
 	{
 		int one = 1;
 		if (setsockopt(STDOUT_FILENO, IPPROTO_TCP, TCP_NOPUSH, &one, 
-			       sizeof one) < 0) {
-			logerr("setsockopt(TCP_NOPUSH) failed: %m");
+			       sizeof one) == -1) {
+			logerr("setsockopt(TCP_NOPUSH): %m");
 		}
 	}
 
-	if (!fgets(line, sizeof(line), stdin))
-		exit(1);
-
 	if (logging || pflag) {
 		sval = sizeof(ss);
-		if (getpeername(0, (struct sockaddr *)&ss, &sval) < 0)
-			logerr("getpeername: %s", strerror(errno));
+		if (getpeername(0, (struct sockaddr *)&ss, &sval) == -1)
+			logerr("getpeername(): %m");
 		realhostname_sa(rhost, sizeof rhost - 1,
 				(struct sockaddr *)&ss, sval);
 		rhost[sizeof(rhost) - 1] = '\0';
@@ -122,40 +120,24 @@
 			setenv("FINGERD_REMOTE_HOST", rhost, 1);
 	}
 
-	if (logging) {
-		char *t;
-		char *end;
-
-		end = memchr(line, 0, sizeof(line));
-		if (end == NULL) {
-			if ((t = malloc(sizeof(line) + 1)) == NULL)
-				logerr("malloc: %s", strerror(errno));
-			memcpy(t, line, sizeof(line));
-			t[sizeof(line)] = 0;
-		} else {
-			if ((t = strdup(line)) == NULL)
-				logerr("strdup: %s", strerror(errno));
-		}
-		for (end = t; *end; end++)
-			if (*end == '\n' || *end == '\r')
-				*end = ' ';
-		syslog(LOG_NOTICE, "query from %s: `%s'", rhost, t);
+	if (!fgets(line, sizeof(line), stdin)) {
+		if (logging)
+			syslog(LOG_NOTICE, "no query from %s", rhost);
+		exit(0);
 	}
+	line[strcspn(line, "\r\n")] = '\0';
+
+	if (logging)
+		syslog(LOG_NOTICE, "query from %s: `%s'", rhost, line);
 
 	comp = &av[1];
 	av[2] = "--";
-	for (lp = line, ap = &av[3];;) {
-		*ap = strtok(lp, " \t\r\n");
-		if (!*ap) {
-			if (secure && ap == &av[3]) {
-				puts("must provide username\r\n");
-				exit(1);
-			}
-			break;
-		}
+	ap = &av[3];
+	*ap = strtok(line, " \t");
+	while (*ap != NULL) {
 		if (secure && strchr(*ap, '@')) {
-			puts("forwarding service denied\r\n");
-			exit(1);
+			fputs("forwarding service denied\r\n", stdout);
+			exit(0);
 		}
 
 		/* RFC742: "/[Ww]" == "-l" */
@@ -167,15 +149,20 @@
 			*ap = NULL;
 			break;
 		}
-		lp = NULL;
+		*ap = strtok(NULL, " \t");
+	}
+	if (secure && ap == &av[3]) {
+                fputs("must provide username\r\n", stdout);
+                exit(0);
 	}
 
 	if ((lp = strrchr(prog, '/')) != NULL)
 		*comp = ++lp;
 	else
 		*comp = prog;
-	if (pipe(p) < 0)
-		logerr("pipe: %s", strerror(errno));
+
+	if (pipe(p) == -1)
+		logerr("pipe(): %m");
 
 	switch(vfork()) {
 	case 0:
@@ -191,18 +178,21 @@
 #define MSG ": cannot execute\n"
 		write(STDERR_FILENO, MSG, strlen(MSG));
 #undef MSG
+		syslog(LOG_ERR, "execv(\"%s\"): %m", prog);
 		_exit(1);
 	case -1:
-		logerr("fork: %s", strerror(errno));
+		logerr("vfork(): %m");
 	}
 	(void)close(p[1]);
 	if (!(fp = fdopen(p[0], "r")))
-		logerr("fdopen: %s", strerror(errno));
+		logerr("fdopen(): %m");
 	while ((ch = getc(fp)) != EOF) {
 		if (ch == '\n')
 			putchar('\r');
 		putchar(ch);
 	}
+
+	wait(NULL);
 	exit(0);
 }
 
@@ -216,5 +206,5 @@
 	(void)vsyslog(LOG_ERR, fmt, ap);
 	va_end(ap);
 	exit(1);
-	/* NOTREACHED */
+	/* NOT REACHED */
 }


1. fgets() reads the input after we know the peer's address and
   even if no data was received we can log the address
2. fgets() always terminates the string with a null byte, so the
   memchr()/memcpy()/malloc() code is needless
3. reterminate the string, so it will not have any '\r' '\n' char
   and don't add spaces, so the query in the log will be real
4. use a %m for logging instead of %s and strerror(errno)
5. simplify the query parsing and other code
6. log a failure of execv() if occured
>Release-Note:
>Audit-Trail:
>Unformatted:


More information about the freebsd-standards mailing list