Date: Sat, 27 Jun 1998 23:30:31 -0600 From: "Aaron D. Gifford" <agifford@infowest.com> To: security@FreeBSD.ORG Subject: popper popper and more popper (Included is a FIX to the not-working popper) Message-ID: <3595D4F7.DDCF4E0E@infowest.com>
next in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?3595D4F7.DDCF4E0E>