From owner-freebsd-security Sun Jun 28 10:17:41 1998 Return-Path: Received: (from majordom@localhost) by hub.freebsd.org (8.8.8/8.8.8) id KAA14459 for freebsd-security-outgoing; Sun, 28 Jun 1998 10:17:41 -0700 (PDT) (envelope-from owner-freebsd-security@FreeBSD.ORG) Received: from super-g.inch.com (super-g.com [207.240.140.161]) by hub.freebsd.org (8.8.8/8.8.8) with ESMTP id KAA14449 for ; Sun, 28 Jun 1998 10:17:38 -0700 (PDT) (envelope-from spork@super-g.com) Received: from localhost (localhost [127.0.0.1]) by super-g.inch.com (8.8.8/8.8.5) with SMTP id NAA28545; Sun, 28 Jun 1998 13:17:34 -0400 (EDT) Date: Sun, 28 Jun 1998 13:17:34 -0400 (EDT) From: spork X-Sender: spork@super-g.inch.com To: "Aaron D. Gifford" cc: security@FreeBSD.ORG Subject: Re: popper popper and more popper (Included is a FIX to the not-working popper) In-Reply-To: <3595D4F7.DDCF4E0E@infowest.com> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-security@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.org Hi, I grabbed the MAXPARMLENGTH patch off of your post to Bugtraq yesterday, and had no problems with it. For whatever reason one patch didn't apply clean, but I think that's just a problem with your mailer wrapping lines. I've yet to see any ill effects from it... Thanks, Charles Charles Sprickman spork@super-g.com ---- On Sat, 27 Jun 1998, Aaron D. Gifford wrote: > 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 > To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe security" in the body of the message