Date: Fri, 20 May 2005 02:45:49 GMT From: Rostislav Krasny <rosti.bsd@gmail.com> To: freebsd-gnats-submit@FreeBSD.org Subject: standards/81287: [PATCH]: fingerd(8) might send a line not ending in CRLF Message-ID: <200505200245.j4K2jnql058202@www.freebsd.org> Resent-Message-ID: <200505200250.j4K2o7KC018747@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
>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@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:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200505200245.j4K2jnql058202>