Home page logo
/

nmap-dev logo Nmap Development mailing list archives

Re: Bug when loading command line exclude list
From: David Fifield <david () bamsoftware com>
Date: Sun, 14 Jun 2009 17:04:56 -0600

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


  By Date           By Thread  

Current thread:
[ Nmap | Sec Tools | Mailing Lists | Site News | About/Contact | Advertising | Privacy ]