Date: Sun, 28 Jun 1998 13:17:34 -0400 (EDT) From: spork <spork@super-g.com> To: "Aaron D. Gifford" <agifford@infowest.com> Cc: security@FreeBSD.ORG Subject: Re: popper popper and more popper (Included is a FIX to the not-working popper) Message-ID: <Pine.BSF.3.96.980628131556.14107A-100000@super-g.inch.com> In-Reply-To: <3595D4F7.DDCF4E0E@infowest.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <CR><LF> */
> 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 <CR><LF> */
> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.BSF.3.96.980628131556.14107A-100000>