Re: More problems with QPOPPER - <sigh>

[agifford () INFOWEST COM (Aaron D. Gifford)]

John Fraizer wrote:
<<snipped examples of misc. testing for buffer overruns>>
> It segfaulted and dumped core.
>
> Damnit, Jim, I'm a Doctor not a C programmer!  I have managed to locate
> the portion of the code that is bypassing the " -ERR Command
> "xxxxxxxxxxxxxxxxx" (truncated) exceedes maximum permitted size. " code
> from the installed patches:
>
> In pop_parse.c, we find:
>
>           /*
>            * This is kinda gross.  Passwords have to be parsed diffrently
>            * as they may contain spaces.  If you think of a cleaner way,
>            * do it.  The "p->pop_command[0] == 'p'" is so save a call to
>            * strcmp() on ever call to pop_parse();  This parsing keeps
>            * leading and trailing speces behind for the password command.
>            */
>           if(p->pop_command[0] == 'p' && strcmp(p->pop_command,"pass") ==
> 0) {
>             if (*mp != 0) {
>               p->pop_parm[1] = mp;
>               if (strlen(mp) > 0) {
>                 mp = mp + strlen(mp) - 1;
>                 while (*mp == 0xa || *mp == 0xd) *mp-- = 0;
>               }
>
> Looks like basically that if the parser sees that the command was actually
> a password argument, it doesn't send it through the truncate code.

Looks like qpopper after the "if(p->pop_command..." bit assumes everything
else in the buffer is the password except any trailing CR/LF characters, which
it removes.  I cannot understand the "if (strlen(mp) > 0) {" test, because the
previous "if (*mp != 0) {" test should guarantee that strlen() will always at
least return 1.

For those who want to be consistent about limiting argument length using
MAXPARMLEN (which is defined in popper.h), you can try this snippit instead
of the existing snippit in pop_parse.c as quoted above:

 if(p->pop_command[0] == 'p' && strcmp(p->pop_command,"pass") == 0) {
   if (*mp != 0) {
       if (strlen(mp) > MAXPARMLEN) {
         mp[MAXPARMLEN] = '\0';
         pop_msg(p,POP_FAILURE,
                 "Argument %d \"%s\" (truncated) exceeds maximum permitted size.",
                 i+1, mp);
         return(-1);
       }
     p->pop_parm[1] = mp;
     mp = mp + strlen(mp) - 1;
     while (*mp == 0xa || *mp == 0xd) *mp-- = 0;
     return(1);
   } else
     return (-1);
 }


PLEASE be aware that you need a large enough MAXPARMLEN defined in popper.h to
handle large passwords or APOP depending on your individual needs.  I've been
using 32 on my system, which should permit APOP to work.


Another fun qpopper trivia fact for the security conscious: It is possible to
glean valid user names from sites using certain configurations of qpopper with
APOP support.  For example:

  localhost# telnet localhost 110
  +OK QPOP (version 2.41beta1) at localhost starting. <115.899106609@localhost>
  APOP bogus-user 1638de71888f8c3ff023ac5c38621211
  -ERR Password supplied for "bogus" is incorrect.
  +OK Pop server at localhost signing off.
  Connection closed by foreign host.
  localhost# telnet localhost 110
  +OK QPOP (version 2.41beta1) at localhost starting. <119.899106628@localhost>
  APOP real-user 8463af56e9a5d72cc84012ad7748f92c
  -ERR not authorized
  +OK Pop server at localhost signing off.
  Connection closed by foreign host.
  localhost#

Nice.  In some cases where APOP support is compiled in but the APOP database
does not exist, the error message on a valid user might be "-ERR POP
authorization DB not available (real-user)" instead of the "-ERR not
authorized" message.  I don't know if this would work for sites with properly
configured APOP or not.  It worked on my own machine which does NOT use APOP
but had APOP compiled in by default.

Aaron out.