Re: Bringing CPE to NSE

[David Fifield <david () bamsoftware com>]

On Tue, Jan 03, 2012 at 09:39:05PM +0100, Henri Doreau wrote:
> 2011/11/7 Henri Doreau <henri.doreau () greenbone net>:
> > The patch attached adds a "cpe" table to port.version, and OS CPEs are
> > available as additional entries in host.os.
> >
> > set_port_version() handles modifications of the CPE list, so that NSE
> > scripts can add new entries or modify the existing ones (for service
> > detection only).
> >
> > Feedback on this is attempt to design a NSE interface is, as always, welcome.
> >
> > Regards.
> Did someone had an opportunity to look at it?
> I've re-generated the patch against latest revision. Sample script
> attached as well for convenience.

I have tried the patch and the sample script, and I get memory leaks. I
run:

valgrind --leak-check=full ./nmap -sV localhost --script cpe-sample -d

A sample of the leaks is

==12793== 28 bytes in 5 blocks are definitely lost in loss record 479 of 589
==12793==    at 0x4C2779D: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==12793==    by 0x62E4421: strdup (strdup.c:43)
==12793==    by 0x485D47: PortList::setServiceProbeResults(unsigned short, int, serviceprobestate, char const*, 
service_tunnel_type, char const*, char const*, char const*, char const*, char const*, char const*, std::vector<char 
const*, std::allocator<char const*> > const*, char const*) (portlist.cc:374)
==12793==    by 0x4928A6: service_scan(std::vector<Target*, std::allocator<Target*> >&) (service_scan.cc:2559)
==12793==    by 0x438574: nmap_main(int, char**) (nmap.cc:1939)
==12793==    by 0x4304BD: main (main.cc:195)
==12793==
==12793== 46 bytes in 5 blocks are definitely lost in loss record 500 of 589
==12793==    at 0x4C2779D: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==12793==    by 0x4ACB6C: safe_malloc (nbase_memalloc.c:117)
==12793==    by 0x48395F: cstringSanityCheck(char const*, int) (portlist.cc:328)
==12793==    by 0x485D91: PortList::setServiceProbeResults(unsigned short, int, serviceprobestate, char const*, 
service_tunnel_type, char const*, char const*, char const*, char const*, char const*, char const*, std::vector<char 
const*, std::allocator<char const*> > const*, char const*) (portlist.cc:383)
==12793==    by 0x4928A6: service_scan(std::vector<Target*, std::allocator<Target*> >&) (service_scan.cc:2559)
==12793==    by 0x438574: nmap_main(int, char**) (nmap.cc:1939)
==12793==    by 0x4304BD: main (main.cc:195)

I think I know why this is happening, so let me explain. The two places
in Nmap where CPEs are stored use a flat list of strings, the same
structure I suggested for the NSE interface:

struct OS_Classification {
  ...
  std::vector<const char *> cpe;
};

struct serviceDeductions {
  ...
  std::vector<char *> cpe;
  ...
};

The one exception is in the structure representing a match line, which
allows only one "a", one "h", and one "o" CPE. This is for annoying
memory management reasons, to change which would require rewriting
ServiceProbeMatch::testMatch, I believe.

struct MatchDetails {
  ...
  // CPE identifiers for application, OS, and hardware type.
  const char *cpe_a;
  const char *cpe_o;
  const char *cpe_h;
};

Your patch changes MatchDetails to also use a vector, and I think that's
where the leaks are coming from.

The good news is that I don't think you actually have to change
MatchDetails. The restriction to only three CPEs only exists at a very
low level of the -sV phase. After that it becomes a serviceDeductions
and you can put in as many CPEs as you want. Other than that, what
you've done with having PortList::setServiceProbeResults take a vector
and free what was previously stored looks correct.

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