Nmap Bug in payload.cc: corrupted UDP packets during -sU scan

[mzet via dev <dev () nmap org>]

Hi List,

 
It was observed that UDP packets that are sent during an UDP port scanning (-sU) are corrupted.

 
Background:

 
To make UDP scanning more effective, for many services (ports) Nmap takes content of UDP packets from nmap-payloads 
file. Logic for handling this is implemented in payload.cc source file.

 
Issue:

 
Due to subtle implementation issue (introduced in 
https://github.com/nmap/nmap/commit/9c83be38331adff3d23b154eadace9263edfab48),  most of the UDP packets that are 
actually sent by the Nmap during -sU scan are corrupted. Here: https://github.com/nmap/nmap/issues/2268  one can find 
walk thru and the reproduction of the issue for the SLP (Service Location Protocol), but this issue affects *most* of 
the protocols for which payload exists in nmap-payloads file (according to my experiments on Linux-based machine all 
protocols for which payloads are >= 16 bytes are affected, but that is most probably a coincidence caused by the way 
how C++ STL vector is implemented on this platform).

 
Buggy code:

 
315. const char *udp_port2payload(u16 dport, size_t *length, u8 tryno) {

316.   static const char *payload_null = "";
317.   std::map<struct proto_dport, std::vector<struct payload> >::iterator portPayloadIterator;
318.   std::vector<struct payload> portPayloadVector;                                                                   
  [1]
319.   std::vector<struct payload>::iterator portPayloadVectorIterator;
320.   proto_dport key(IPPROTO_UDP, dport);
321.   int portPayloadVectorSize;
322.
323.   portPayloadIterator = portPayloads.find(key);
324.
325.   if (portPayloadIterator != portPayloads.end()) {
326.     portPayloadVector = portPayloads.find(key)->second;                                                         [2]
327.     portPayloadVectorSize = portPayloadVector.size();
328.
329.     tryno %= portPayloadVectorSize;
330.
331.     if (portPayloadVectorSize > 0) {
332.       portPayloadVectorIterator = portPayloadVector.begin();                                                     
[3]
333.
334.       while (tryno > 0 && portPayloadVectorIterator != portPayloadVector.end()) {
335.         tryno--;
336.         portPayloadVectorIterator++;
337.       }
338.
339.       assert (tryno == 0);
340.       assert (portPayloadVectorIterator != portPayloadVector.end());
341.
342.       *length = portPayloadVectorIterator->data.size();
343.       return portPayloadVectorIterator->data.data();                                                               
     [4]
344.     } else {
345.       *length = 0;
346.       return payload_null;
347.     }
348.   } else {
349.     *length = 0;
350.     return payload_null;
351.   }
352. }

 
At [1] Empty STL 'portPayloadVector' vector is declared locally. At [2] 'portPayloadVector' is populated with the 
content of the vector holding actual payloads (from nmap-payloads file) for currently processed port. At [3] 
'portPayloadVectorIterator' is set to first element of our (local) vector 'portPayloadVector'. Finally at [4] pointer 
of the data allocated by the (local) vector is returned which is obviously wrong as after returning form the function 
the vector is destroyed and it's data is freed in vector's destructor.

 
Fix:

 
Proposed fix is available here: https://github.com/nmap/nmap/pull/2269

 
Instead of copying payload's vector locally, we operate on the reference to the original vector which is also faster as 
we get rid of copying vectors.

 
Best,

mzet

 
--

Z-Labs, Software Security Labs - https://z-labs.eu
PGP ID: 0xC0EE0613CA05BB39

_______________________________________________
Sent through the dev mailing list
https://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/