If a string of longer than approximately 2048 characters is sent to the qpopper process, a denial of service condition will occur. Fix: I have made a quick patch for qpopper4.0.3. How-To-Repeat: Please see at the Description.
Responsible Changed From-To: freebsd-ports->lioux Over to maintainer
Hi, > >Description: > If a string of longer than approximately 2048 characters is sent to > the qpopper process, a denial of service condition will occur. Close inspection lead me to believe that the problem is inherent to the way qpopper was designed. It ALWAYS believe that str (string which is sent as a buffer to getline) returned from getline is true. However, str is only updated IF getline works. When it does not, getline returns str untouched, with the same value as before. Therefore, it is possible to make it infinitly loops under extreme circunstances. Exercise cases are up to the reader. :) Your patch addresses the correct problem source but it loses the buffer. > *** popper.c.dist Sat Jun 2 11:24:36 2001 > --- popper.c Tue Mar 26 16:24:30 2002 > *************** > *** 483,489 **** > --- 483,501 ---- > else > len = read ( pPOP->input_fd, junk, sizeof(junk) ); > if ( len <= 0 ) > + #if 0 > break; > + #else > + { > + /* > + * patch by Isao SEKI <iseki@gongon.com> > + * return 0 is meaningless after buffer overflow > + */ > + pop_log (pPOP, POP_NOTICE, HERE, > + "read 0 byte after buffer overflow\n"); > + return(NULL); > + } > + #endif > q = strchr ( junk, '\n' ); > if ( q == NULL ) { > disc += len; Could you try the following patch? Let me know how this turns out. If it is good, I'll commit it ASAP. I did not reproduce the 2048 bug so I am relying on you for feedback. --- popper/popper.c.orig Wed Mar 27 13:09:15 2002 +++ popper/popper.c Wed Mar 27 14:01:30 2002 @@ -483,7 +483,21 @@ else len = read ( pPOP->input_fd, junk, sizeof(junk) ); if ( len <= 0 ) + { + /* + * patch by Isao SEKI <iseki@gongon.com> and + * Mario Sergio Fujikawa Ferreira <lioux@FreeBSD.org> + * return 0 is meaningless after buffer overflow + */ + + str[0]='\0'; /* empty buffer before returning + since it seems that qpopper + believes in the contents + of str */ + pop_log (pPOP, POP_NOTICE, HERE, + "read 0 byte after buffer overflow\n"); break; + } q = strchr ( junk, '\n' ); if ( q == NULL ) { disc += len; -- Mario S F Ferreira - DF - Brazil - "I guess this is a signature." Computer Science Undergraduate | FreeBSD Committer | CS Developer flames to beloved devnull@someotherworldbeloworabove.org feature, n: a documented bug | bug, n: an undocumented feature
The comment of the function indicates that it should try and copy as much data as possible into 'str', ignoring the rest. I think the correct fix is to copy as much as is possible into str, even if it isn't terminated with a newline, and null-terminate it. Note this happens in any case if len > 0, so it shouldn't require a great alteration... On Wed, Mar 27, 2002 at 02:08:58PM -0300, Mario Sergio Fujikawa Ferreira wrote: > Hi, > > > >Description: > > If a string of longer than approximately 2048 characters is sent to > > the qpopper process, a denial of service condition will occur. > > Close inspection lead me to believe that the problem is > inherent to the way qpopper was designed. It ALWAYS believe that > str (string which is sent as a buffer to getline) returned from > getline is true. However, str is only updated IF getline works. > When it does not, getline returns str untouched, with the same value > as before. Therefore, it is possible to make it infinitly loops > under extreme circunstances. Exercise cases are up to the reader. :) > > Your patch addresses the correct problem source but it > loses the buffer. > > > *** popper.c.dist Sat Jun 2 11:24:36 2001 > > --- popper.c Tue Mar 26 16:24:30 2002 > > *************** > > *** 483,489 **** > > --- 483,501 ---- > > else > > len = read ( pPOP->input_fd, junk, sizeof(junk) ); > > if ( len <= 0 ) > > + #if 0 > > break; > > + #else > > + { > > + /* > > + * patch by Isao SEKI <iseki@gongon.com> > > + * return 0 is meaningless after buffer overflow > > + */ > > + pop_log (pPOP, POP_NOTICE, HERE, > > + "read 0 byte after buffer overflow\n"); > > + return(NULL); > > + } > > + #endif > > q = strchr ( junk, '\n' ); > > if ( q == NULL ) { > > disc += len; > > Could you try the following patch? Let me know how this turns out. > If it is good, I'll commit it ASAP. I did not reproduce the 2048 bug so > I am relying on you for feedback. > > --- popper/popper.c.orig Wed Mar 27 13:09:15 2002 > +++ popper/popper.c Wed Mar 27 14:01:30 2002 > @@ -483,7 +483,21 @@ > else > len = read ( pPOP->input_fd, junk, sizeof(junk) ); > if ( len <= 0 ) > + { > + /* > + * patch by Isao SEKI <iseki@gongon.com> and > + * Mario Sergio Fujikawa Ferreira <lioux@FreeBSD.org> > + * return 0 is meaningless after buffer overflow > + */ > + > + str[0]='\0'; /* empty buffer before returning > + since it seems that qpopper > + believes in the contents > + of str */ > + pop_log (pPOP, POP_NOTICE, HERE, > + "read 0 byte after buffer overflow\n"); > break; > + } > q = strchr ( junk, '\n' ); > if ( q == NULL ) { > disc += len; > > -- > Mario S F Ferreira - DF - Brazil - "I guess this is a signature." > Computer Science Undergraduate | FreeBSD Committer | CS Developer > flames to beloved devnull@someotherworldbeloworabove.org > feature, n: a documented bug | bug, n: an undocumented feature
On Wed, Mar 27, 2002 at 06:40:04PM +0100, David Rufino wrote: > > The comment of the function indicates that it should try and copy as much > data as possible into 'str', ignoring the rest. I think the correct fix > is to copy as much as is possible into str, even if it isn't terminated > with a newline, and null-terminate it. Note this happens in any case > if len > 0, so it shouldn't require a great alteration... FWIW, I agree. btw, on Bugtraq site there is a trivial one-line Perl script that sends > 2048 "A"s to Qpopper via Netcat and someone on qpopper list reports that his machine also starts eating memory, not just 100% CPU (infinite loop). On my 4.4-stable box I can't reproduce the memory eating. Further, on the qpopper list has appeard a tiny un-official patch that seems to address the problem, IMHO, in another way. Sorry currently I've not access to that patch to discuss it more appropriately :( Anyway, many thanks to you all for your help to fix this bug! :-) > > On Wed, Mar 27, 2002 at 02:08:58PM -0300, Mario Sergio Fujikawa Ferreira wrote: > > Hi, > > > > > >Description: > > > If a string of longer than approximately 2048 characters is sent to > > > the qpopper process, a denial of service condition will occur. > > > > Close inspection lead me to believe that the problem is > > inherent to the way qpopper was designed. It ALWAYS believe that > > str (string which is sent as a buffer to getline) returned from > > getline is true. However, str is only updated IF getline works. > > When it does not, getline returns str untouched, with the same value > > as before. Therefore, it is possible to make it infinitly loops > > under extreme circunstances. Exercise cases are up to the reader. :) > > > > Your patch addresses the correct problem source but it > > loses the buffer. > > > > > *** popper.c.dist Sat Jun 2 11:24:36 2001 > > > --- popper.c Tue Mar 26 16:24:30 2002 > > > *************** > > > *** 483,489 **** > > > --- 483,501 ---- > > > else > > > len = read ( pPOP->input_fd, junk, sizeof(junk) ); > > > if ( len <= 0 ) > > > + #if 0 > > > break; > > > + #else > > > + { > > > + /* > > > + * patch by Isao SEKI <iseki@gongon.com> > > > + * return 0 is meaningless after buffer overflow > > > + */ > > > + pop_log (pPOP, POP_NOTICE, HERE, > > > + "read 0 byte after buffer overflow\n"); > > > + return(NULL); > > > + } > > > + #endif > > > q = strchr ( junk, '\n' ); > > > if ( q == NULL ) { > > > disc += len; > > > > Could you try the following patch? Let me know how this turns out. > > If it is good, I'll commit it ASAP. I did not reproduce the 2048 bug so > > I am relying on you for feedback. > > > > --- popper/popper.c.orig Wed Mar 27 13:09:15 2002 > > +++ popper/popper.c Wed Mar 27 14:01:30 2002 > > @@ -483,7 +483,21 @@ > > else > > len = read ( pPOP->input_fd, junk, sizeof(junk) ); > > if ( len <= 0 ) > > + { > > + /* > > + * patch by Isao SEKI <iseki@gongon.com> and > > + * Mario Sergio Fujikawa Ferreira <lioux@FreeBSD.org> > > + * return 0 is meaningless after buffer overflow > > + */ > > + > > + str[0]='\0'; /* empty buffer before returning > > + since it seems that qpopper > > + believes in the contents > > + of str */ > > + pop_log (pPOP, POP_NOTICE, HERE, > > + "read 0 byte after buffer overflow\n"); > > break; > > + } > > q = strchr ( junk, '\n' ); > > if ( q == NULL ) { > > disc += len; > > > > -- > > Mario S F Ferreira - DF - Brazil - "I guess this is a signature." > > Computer Science Undergraduate | FreeBSD Committer | CS Developer > > flames to beloved devnull@someotherworldbeloworabove.org > > feature, n: a documented bug | bug, n: an undocumented feature -- bye! Ale ale@unixmania.net
Hi, Unfortunately, high cpu usage problem was reproduced by your new patch. I think that my patch (return null immediately or str=NULL) is not best way however it is better in limited time. Btw, I wrote a small program to sent big data. It is worked on Mac OS X machine at my testing. Maybe it will work on FreeBSD.
Well, this last patch seems to work. I sort of I do not believe in losing pointer references so I changed the way getline, tgetline work. Just so that they resemble read return codes. 1) if < 0 problem 2) if >= 0 user typed something (enter without anything else is something) Anyway, here is the patch. It works against the test program supplied by Isao Seki. Could you test it against live systems? If it works, I'll commit it ASAP. diff -ruN qpopper4.0.3.orig/popper/popper.c qpopper4.0.3/popper/popper.c --- qpopper4.0.3.orig/popper/popper.c Fri Jun 1 23:24:14 2001 +++ qpopper4.0.3/popper/popper.c Sat Mar 30 18:24:06 2002 @@ -125,8 +125,8 @@ state_table * s; char message [ MAXLINELEN ]; pop_result rslt = POP_FAILURE; - char * tgetline(); - char * getline(); + ssize_t tgetline(); + ssize_t getline(); /* * seed random with the current time to nearest second @@ -287,7 +287,7 @@ if ( hangup ) { pop_exit ( &p, HANGUP ); } - else if ( tgetline ( message, MAXLINELEN, &p, pop_timeout ) == NULL ) { + else if ( tgetline ( message, MAXLINELEN, &p, pop_timeout ) < 0 ) { pop_exit ( &p, (poptimeout) ? TIMEOUT : ABORT ); } else if ( StackSize ( &(p.InProcess) ) ) { @@ -400,8 +400,8 @@ * the input is discarded. */ -char -*getline ( char *str, int size, POP *pPOP ) +ssize_t +getline ( char *str, int size, POP *pPOP ) { char *p = NULL; char *pEnd = NULL; @@ -427,7 +427,7 @@ p++ ) if ( *p == '\n' ) break; - if ( p != pPOP->pcInEnd && *p == '\n' ) { + if ( p != pPOP->pcInEnd && *p == '\n' ) { /* * Got a line */ @@ -451,7 +451,7 @@ } _DEBUG_LOG3 ( pPOP, "getline() returning %d: '%.*s'", strlen(str), MIN(25, (int) strlen(str)), str ); - return ( str ); + return ( strlen(str) ); } /* got a line */ nRoom = pPOP->pcInBuf + nBufSz - pPOP->pcInEnd; @@ -483,7 +483,22 @@ else len = read ( pPOP->input_fd, junk, sizeof(junk) ); if ( len <= 0 ) - break; + { + /* + * patch by Isao SEKI <iseki@gongon.com> and + * Mario Sergio Fujikawa Ferreira <lioux@FreeBSD.org> + * return 0 is meaningless after buffer overflow + */ + + /* do not touch buffer before returning + * since it seems that qpopper + * believes in the contents of str + */ + + pop_log (pPOP, POP_NOTICE, HERE, + "read 0 byte. Possible buffer overflow\n"); + return (-1); + } q = strchr ( junk, '\n' ); if ( q == NULL ) { disc += len; @@ -522,7 +537,7 @@ } /* loop and discard until we see a '\n' */ _DEBUG_LOG1 ( pPOP, "getline() returning %d", strlen(str) ); - return ( str ); + return ( strlen(str) ); } /* nRoom == 0 */ if ( pPOP->tls_started ) @@ -544,7 +559,7 @@ } /* main loop */ _DEBUG_LOG0 ( pPOP, "getline() returning NULL" ); - return ( NULL ); + return ( -1 ); } @@ -552,12 +567,12 @@ /* * Get a line of input with a timeout. This part does the timeout */ -char * +ssize_t tgetline ( char *str, int size, POP *p, int timeout ) { int ring(); - - + ssize_t result; + (void) signal ( SIGALRM, VOIDSTAR ring ); alarm ( timeout ); if ( setjmp ( env ) ) { @@ -565,12 +580,13 @@ pop_log ( p, POP_NOTICE, HERE, "(v%s) Timeout (%d secs) during " "nw read from %s at %s (%s)", VERSION, timeout, p->user, p->client, p->ipaddr ); + result = 0; } else - str = getline ( str, size, p ); + result = getline ( str, size, p ); alarm ( 0 ); signal ( SIGALRM, SIG_DFL ); - return ( str ); + return ( result ); }
On Sat, Mar 30, 2002 at 06:52:56PM -0300, Mario Sergio Fujikawa Ferreira wrote: > Well, this last patch seems to work. I sort of I do not believe in > losing pointer references so I changed the way getline, tgetline > work. Just so that they resemble read return codes. > > 1) if < 0 problem > 2) if >= 0 user typed something (enter without anything else is > something) > > Anyway, here is the patch. It works against the test program > supplied by Isao Seki. Could you test it against live systems? > If it works, I'll commit it ASAP. Ok, however due to Easter holidays I can't test it until Tuesday April, 2nd... I'll report to you all my results. Thanks a lot! -- bye! Ale ale@unixmania.net
Mario, Great! I just have tested your last patch now and it is working fine. Isao At 18:52 03/30/2002 -0300, Mario Sergio Fujikawa Ferreira wrote: > Well, this last patch seems to work. I sort of I do not believe in > losing pointer references so I changed the way getline, tgetline > work. Just so that they resemble read return codes. > > 1) if < 0 problem > 2) if >= 0 user typed something (enter without anything else is > something) > > Anyway, here is the patch. It works against the test program > supplied by Isao Seki. Could you test it against live systems? > If it works, I'll commit it ASAP.
On Sun, Mar 31, 2002 at 01:57:11PM +0900, Isao SEKI wrote: > > Great! > I just have tested your last patch now and it is working fine. Real live system? Didn't I break anything else?
At 03:10 03/31/2002 -0300, Mario Sergio Fujikawa Ferreira wrote: > > Great! > > I just have tested your last patch now and it is working fine. > Real live system? Didn't I break anything else? I tested on our main server, "gongon.com". I did not find out any breaking. Isao
State Changed From-To: open->closed A consensual patch has been committed, thanks!