Re: --excludefile causing reads in free()'d memory

[Brandon Enright <bmenrigh () ucsd edu>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Mon, 9 Mar 2009 10:33:22 -0600 or thereabouts David Fifield
<david () bamsoftware com> wrote:

> On Thu, Mar 05, 2009 at 08:19:52PM +0000, Brandon Enright wrote:
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> >
> > While troubleshooting some other issues, I noticed that Valgrind
> > complains with the following error when I use the --excludefile
> > option:
> >
> > ==12717== Invalid read of size 1
> > ==12717==    at 0x646547C: strtok (in /lib64/libc-2.6.1.so)
> > ==12717==    by 0x421E54: load_exclude(_IO_FILE*, char*)
> > (targets.cc:333) ==12717==    by 0x41E9AE: nmap_main(int, char**)
> > (nmap.cc:1576) ==12717==    by 0x419EA6: main (main.cc:224)
> > ==12717==  Address 0x76aa216 is 14 bytes inside a block of size 15
> > free'd ==12717==    at 0x4C210AA: free
> > (in /usr/lib64/valgrind/amd64-linux/vgpreload_memcheck.so)
> > ==12717==    by 0x456732: TargetGroup::parse_expr(char const*, int)
> > (TargetGroup.cc:318) ==12717==    by 0x421E7A:
> > load_exclude(_IO_FILE*, char*) (targets.cc:328) ==12717==    by
> > 0x41E9AE: nmap_main(int, char**) (nmap.cc:1576) ==12717==    by
> > 0x419EA6: main (main.cc:224)
> >
> >
> > The offending code appears to be targets.cc:328 and targets.cc:333
> >
> >       pc=strtok(acBuf, "\t\n ");
> >
> >       while ((char *)0 != pc) {
> >          if(excludelist[i].parse_expr(pc,o.af()) == 0) {
> >            if (o.debugging > 1)
> >              error("Loaded exclude target of: %s", pc);
> >            ++i;
> >          }
> >          pc=strtok(NULL, "\t\n ");
> >       }
> >     }
> >
> >
> > Valgrind seems to think that parse_expr(pc,o.af()) is causing some
> > memory to be freed that is being read by the subsequent call to
> > pc=strtok(NULL, "\t\n ");
>
> The bug is that TargetGroup::parse_expr calls strtok also and messes
> up the global strtok state. parse_expr makes strtok look at the
> strduped memory and load_exclude continues looking at it. This
> creates a bug, which is that only the first specification on each
> line of the file is honored.
>
> $ cat > excludes.txt <<EOF
> 1.1.1.1 2.2.2.2
> 3.3.3.3
> EOF
> $ nmap -d2 --excludefile excludes.txt
> Loaded exclude target of: 1.1.1.1
> Loaded exclude target of: 3.3.3.3
>
> This would not be hard to fix. Does anyone want to try? You just have
> to remove strtok from one of the functions. The easiest way would
> probably be to use strchr instead in TargetGroup::parse_expr.
>
> David Fifield

Thanks for tracking this down.

Attached is a patch that changes the use of strtok() to strchr().  We
really shouldn't be using strtok anywhere we can help it.  Besides
being non-reentrant it modifies the string passed to it.  Anyone who
doesn't realize that is in for a surprise when they try to understand
the code.

Brandon

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.9 (GNU/Linux)

iEYEARECAAYFAkm9x0MACgkQqaGPzAsl94KtkwCfaRF/JweqAe+b7jiZ3AGVI8HE
0EcAn0guHraQuM4pkNB7eC6bMoymaSRe
=xN9R
-----END PGP SIGNATURE-----

Attachment: parse_expr_strtok.diff
Description:

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