Re: Bug when loading command line exclude list

[David Fifield <david () bamsoftware com>]

On Sun, May 24, 2009 at 04:27:43PM -0600, David Fifield wrote:
> On Tue, May 19, 2009 at 02:10:39PM -0400, Will Cladek wrote:
> > I found a bug in the load_exclude function in targets.cc.  Here's the
> > affected code:
> >
> >  else {
> >    /* If we are parsing command line, load the exclude file from the string */
> >    p_acBuf=strdup(szExclude);
> >    pc=strtok(p_acBuf, ",");
> >     while (NULL != pc) {
> >      if(excludelist[i].parse_expr(pc,o.af()) == 0) {
> >        if (o.debugging >1)
> >          error("Loaded exclude target of: %s", pc);
> >        ++i;
> >      }
> >         /* This is a totally cheezy hack, but since I can't use strtok_r...
> >       * If you can think of a better way to do this, feel free to change.
> >       * As for now, we will reset strtok each time we leave parse_expr */
> >      {
> >        int hack_i;
> >        char *hack_c = strdup(szExclude); 
> >
> >        pc=strtok(hack_c, ",");
> >
> >        for (hack_i = 0; hack_i < i; hack_i++)
> >          pc=strtok(NULL, ",");
> >         free(hack_c);
> >      }
> >    }
> >  }
> >
> > The problem is in the "cheezy hack", hack_c is duplicated from the
> > string of excluded hosts and pc is made to point to the next host in
> > that string.  But then hack_c is freed before pc, which points to that
> > freed bit of memory, is used on the next iteration of the while loop,
> > occasionally causing fatal errors.
>
> Thanks for finding this. I was trying to figure out why this code is so
> complicated, then I remembered that TargetGroup::parse_expr used to use
> the strtok function too. As you know, strtok has secret internal state
> so it can't be used concurrently by two different functions. That's why
> this code has to "reset" it.
>
> But Brandon Enright rewrote parse_expr not to use strtok in r12613, in
> response to a different bug (also related to the use of strtok). Can you
> send another patch that either uses a straightforward strtok loop
> (should be much shorter), or (better) does the same thing without
> calling strtok?

I forgot to reply this request back to Will so he may not have seen it.
I changed the code to use a straightforward strtok loop.

The load_exclude function needs to be rewritten from scratch to be
simpler and clearer. As it is, it parses the input file or command line
argument twice, the first time to get the number of entries and allocate
a buffer, the second to fill in the buffer. This makes it hard to change
how it works and could cause a buffer overflow in the future if someone
updates one loop but not the other. A tested replacement is welcome.

David Fifield

_______________________________________________
Sent through the nmap-dev mailing list
http://cgi.insecure.org/mailman/listinfo/nmap-dev
Archived at http://SecLists.Org