Bug 81287 - [patch] fingerd(8) might send a line not ending in CRLF
Summary: [patch] fingerd(8) might send a line not ending in CRLF
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: standards (show other bugs)
Version: 5.4-RELEASE
Hardware: Any Any
: Normal Affects Only Me
Assignee: freebsd-standards (Nobody)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-05-20 03:50 UTC by Rostislav Krasny
Modified: 2018-01-03 05:14 UTC (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Rostislav Krasny 2005-05-20 03:50:07 UTC
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.

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.



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--cHmXRZJaG9K6Cs5hbzMsEqxWqZagpG2owHHByxVWxRZqF94w
Content-Type: text/plain; name="file.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment; filename="file.diff"

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 */
 }
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.
Comment 1 Eitan Adler freebsd_committer freebsd_triage 2017-12-31 08:01:35 UTC
For bugs matching the following criteria:

Status: In Progress Changed: (is less than) 2014-06-01

Reset to default assignee and clear in-progress tags.

Mail being skipped