[NSE] socket:receive_buf(...) broken

[Patrick Donnelly <batrick () batbytes com>]

Hi all,

I'm in process of a major maintenance fix of the NSE nsock binding. In
the process of doing so I have found numerous bugs and problems but
one has been extra glaring that warrants a post here, now.

The socket:receive_buf [1] function has numerous problems:

When *any* socket method is called, excluding receive_buf, the buffer
is cleared (especially send!) [1]. This is "intentional behavior" but
has resulted in buggy usage of the function. For example, the ssh2.lua
library throws away the last 4 bytes of a payload because it does not
offset the initial integer giving the length. These extra four bytes
are kept in the buffer but discarded on a later send. These are good
bytes that were part of the payload! I assume these bytes were some
form of padding and that is the only reason any ssh operation
succeeds. I'll try to show what I mean in the code snippet (from
ssh2.lua) below:


--- Retrieve the size of the packet that is being received
--  and checks if it is fully received
--
--  This function is very similar to the function generated
--  with match.numbytes(num) function, except that this one
--  will check for the number of bytes on-the-fly, based on
--  the written on the SSH packet.
--
--  @param buffer The receive buffer
--  @return packet_length, packet_length or nil
--  the return is similar to the lua function string:find()
check_packet_length = function( buffer )
 local packet_length, offset
 offset, packet_length = bin.unpack( ">I", buffer )
 assert(packet_length)
 if packet_length + 4 > buffer:len() then return nil end
 return packet_length, packet_length
end

--- Receives a complete SSH packet, even if fragmented
--  this function is an abstraction layer to deal with
--  checking the packet size to know if there is any more
--  data to receive.
--
--  @param socket The socket used to receive the data
--  @return status True or false
--  @return packet The packet received
transport.receive_packet = function( socket )
 local status, packet = socket:receive_buf(check_packet_length)
 return status, packet
end


The transport.receive_packet function attempts to read from the socket
a binary integer (4 bytes) that describes the length of the payload.
The check_packet_length function will keep reading from the socket
until the length reaches packet_length+4 (the extra 4 accounting for
the binary integer). You can see a bug here in that the
check_packet_length function returns packet_length which will include
the binary integer but exclude the last 4 bytes of the payload! The
function should return packet_length+4, packet_length+4. The only
reason ssh2.lua gets away with this (on accident of course) is because
later sends clear the buffer.

Another error in the above code is the assumption that the buffer is
at least of size 4. If the initial call to the function is passed an
empty buffer string, the call will fail the assertion.

Another unspecified implementation detail that has saved us is that
receive_buf will automatically keep the pattern if the two integers
returned by check_packet_length (or any such function) are equal. This
means that the last byte is not discarded when it should be. I argue
this is also a bug. The correct way to capture the entire string is to
specify keeppattern as true when calling receive_buf. (It just so
happens that receive_packet would have properly failed, in a later
assertion not shown, if the last byte had been discarded as it should
be.)


These are the design problems I'm so far aware of with this function.
I'm fixing all of the above in the new implementation. To rehash the
changes to receive_buf:

o The buffer will no longer be cleared on a call to other socket
methods. Only receive_buf reads the buffer. Other read methods will
ignore the contents of the buffer. Do not interleave calls without
finishing the buffer.
o A "priming" call to the pattern function may pass a buffer of length
0. The function/pattern should handle this appropriately. (This needs
to be the case anyway; consider in the above example if for some
reason we only had 2 bytes in the buffer. The read of a 4 byte integer
would still fail!)
o receive_buf does not automatically set keep to true if the two
integers returned by the pattern function are equal.

[1] http://nmap.org/nsedoc/lib/nmap.html#receive_buf


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