From owner-freebsd-security Sat Jun 27 22:31:17 1998 Return-Path: Received: (from majordom@localhost) by hub.freebsd.org (8.8.8/8.8.8) id WAA00255 for freebsd-security-outgoing; Sat, 27 Jun 1998 22:31:17 -0700 (PDT) (envelope-from owner-freebsd-security@FreeBSD.ORG) Received: from infowest.com (infowest.com [204.17.177.10]) by hub.freebsd.org (8.8.8/8.8.8) with ESMTP id WAA00244 for ; Sat, 27 Jun 1998 22:31:15 -0700 (PDT) (envelope-from agifford@infowest.com) Received: from infowest.com (liberty.infowest.com [207.49.60.254]) by infowest.com (8.8.8/8.8.8) with ESMTP id XAA27161 for ; Sat, 27 Jun 1998 23:30:43 -0600 (MDT) Message-ID: <3595D4F7.DDCF4E0E@infowest.com> Date: Sat, 27 Jun 1998 23:30:31 -0600 From: "Aaron D. Gifford" X-Mailer: Mozilla 4.05 [en] (X11; U; FreeBSD 2.2.6-STABLE i386) MIME-Version: 1.0 To: security@FreeBSD.ORG Subject: popper popper and more popper (Included is a FIX to the not-working popper) Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Sender: owner-freebsd-security@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.org Hello, I don't know this message really should go, but I have an additional bug fix for qpopper (at the bottom of this message), a suggested cosmetic change (the first part of this message), and an optional patch (middle of this message) for qpopper. ===== FIRST ===== The purely cosmetic change first... In the file pop_auth.c the line: return (pop_msg(p,POP_FAILURE,"This command is not supported yet")); functions perfectly, but my log files keep getting messages like: Jun 27 21:52:52 blah popper[22348]: @dialport05.xyzisp.org: -ERR This command is not supported yet Before I groked the popper source code, I had NO CLUE what this meant. After changing the above line of code thus: return (pop_msg(p,POP_FAILURE,"The auth command is not supported yet")); my log files are completely comprehensible without having to look at the popper source code. ===== SECOND ===== Now for the second change, the optional patch. Take it with a grain of salt. I personally like it and think it improves the security and handling of untrusted data from the POP client. It MIGHT violate the POP3 RFC even though it does not break any of the POP clients I've tested (Eudora, Netscape mail, and MS Internet Mail). In looking at the recent buffer overflows, I noticed that popper.h had an interesting define: #define MAXPARMLEN 10 Yet if you grep for MAXPARMLEN, it ONLY shows up in the header file. I highly suspect that the qpopper author(s) intended to limit POP commands and parameters to this length but never implemented it. Here's a quick patch that implements this feature. Had it been implemented in the first place, the recent buffer exploits would have been more difficult or perhaps even impossible. It may be that such an implementation may violate an RFC (I haven't read the POP3 definition). I don't know. Perhaps you might only include the patch as an additonal optional patch with a brief note in the README for those who want to add this functionality. I have been running the patch below on a moderate volume 6,000 user system without any trouble. Here it is: diff -p popper.h popper.new.h *** popper.h Sat Jun 27 22:46:59 1998 --- popper.new.h Sat Jun 27 22:47:09 1998 *************** *** 59,65 **** #define MAXMSGLINELEN MAXLINELEN #define MAXCMDLEN 4 #define MAXPARMCOUNT 5 ! #define MAXPARMLEN 10 #define ALLOC_MSGS 20 #ifndef OSF1 --- 59,65 ---- #define MAXMSGLINELEN MAXLINELEN #define MAXCMDLEN 4 #define MAXPARMCOUNT 5 ! #define MAXPARMLEN 16 #define ALLOC_MSGS 20 #ifndef OSF1 diff -p pop_parse.c pop_parse.new.c *** pop_parse.c Wed Nov 19 14:20:38 1997 --- pop_parse.new.c Sat Jun 27 22:58:17 1998 *************** char * buf; /* Pointer *** 26,31 **** --- 26,32 ---- { char * mp; register int i; + register int parmlen; /* Loop through the POP command array */ for (mp = buf, i = 0; ; i++) { *************** char * buf; /* Pointer *** 45,52 **** /* Point to the start of the token */ p->pop_parm[i] = mp; /* Search for the first space character (end of the token) */ ! while (!isspace(*mp) && *mp) mp++; /* Delimit the token with a null */ if (*mp) *mp++ = 0; --- 46,75 ---- /* Point to the start of the token */ p->pop_parm[i] = mp; + /* Start counting the length of this token */ + parmlen = 0; + /* Search for the first space character (end of the token) */ ! while (!isspace(*mp) && *mp) { ! mp++; ! parmlen++; ! if (parmlen > MAXPARMLEN) { ! /* Truncate parameter to the max. allowable size */ ! *mp = '\0'; ! ! /* Fail with an appropriate message */ ! if (i == 0) { ! pop_msg(p,POP_FAILURE, ! "Command \"%s\" (truncated) exceedes maximum permitted size.", ! p->pop_command); ! } else { ! pop_msg(p,POP_FAILURE, ! "Argument %d \"%s\" (truncated) exceeds maximum permitted size.", ! i, p->pop_parm[i]); ! } ! return(-1); ! } ! } /* Delimit the token with a null */ if (*mp) *mp++ = 0; ===== LAST the BUG FIX (Two parts) ===== Last of all, I have a few problems with patch-ag. First, in a patched pop_msg.c, beginning at line 92: /* Append the */ len -= strlen(message); (void)strncat(message, len, "\r\n"); Before the above assignment: len == sizeof(message) - strlen(stat == POP_SUCCESS ? POP_OK : POP_ERR) After the assignment: len == sizeof(message) - strlen(stat == POP_SUCCESS ? POP_OK : POP_ERR) - strlen(message) That means that if: stat == POP_SUCCESS strlen(POP_OK) == 5 sizeof(message) == 1024 assume that vsnprintf(mp,len,format,ap) appends a VERY LARGE string with a strlen of 1018 to message Then the before and after would be: BEFORE: len == 1019 (or 1024 - 5) AFTER: len == -4 (or 1019 - 1023) The strlen(stat == POP_SUCCESS ? POP_OK : POP_ERR) essentially gets subtracted twice by the code, once above the v/snprintf()'s and again afterward. I believe the code should instead read beginning at line 92: /* Append the */ len -= strlen(mp); (void)strncat(message, "\r\n", len); There is also the possibility that the strncat() will fail to append the "\r\n" in extremem cases because there's not enough buffer length left. I believe this should not be allowed to happen. **** BIG NOTE **** The problems reported today about popper not working after Jordan's patches occur because the new call to strncat() mistakenly transposes the "\r\n" and len parameters. The correct parameter order is as I show in my above code. This fixes this problem and lets popper work normally. **** END NOTE **** The pop_msg.c code at line 62 as currently patched reads: /* Point past the POP status indicator in the message message */ l = strlen(mp); len -= l, mp += l; I would instead do: /* Point past the POP status indicator in the message message */ l = strlen(mp); mp += l; /* * Subtract an additional 2 from the remaining buffer length * so that after the vsnprintf()/snprintf() calls there will * still be enough buffer space to append a "\r\n" even in a * worst-case scenario. */ len -= l - 2; Why? By pre-removing 2 chars from the buffer maximum limit, there should always be room left for the "\r\n" appended later on. I believe this would be the "right" thing to do. It guarantees that the POP client will always be sent the expected "\r\n" sequence even in abnormal cases. On a mostly unrelated note, many many kudos and thanks to the entire FreeBSD core team and to all contributors! I use FreeBSD as the core OS for InfoWest, a local ISP I work for and it is ROCK SOLID! I also run it at home and now RARELY ever boot to Windows. Sincerely, Aaron Gifford To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe security" in the body of the message