Fix off-by-one error in stun.lua

[David Fifield <david () bamsoftware com>]

The stun-info script currently (r37773) crashes with a "data string too
short" error.
        $ nmap -n -Pn -sU -p 3478 stun.ekiga.net --script=stun-info -d
        ...
        NSE: stun-info against stun.ekiga.net (216.93.246.18:3478) threw an error!
        nselib/stun.lua:52: bad argument #2 to 'unpack' (data string too short)
        stack traceback:
                [C]: in function 'string.unpack'
                nselib/stun.lua:52: in field 'parse'
                nselib/stun.lua:256: in function <nselib/stun.lua:250>
                (...tail calls...)
                nselib/stun.lua:338: in method 'getExternalAddress'
                scripts/stun-info.nse:37: in function <scripts/stun-info.nse:30>
                (...tail calls...)

I tracked the error down to Comm.recv in stun.lua. The code intends to
receive exactly 20 bytes (Header.size), then pass those 20 bytes to
Header.parse. But self.socket:receive_buf is returning only 19 bytes,
which causes Header.parse to fail.
        local status, hdr_data = self.socket:receive_buf(match.numbytes(Header.size), false)
        if ( not(status) ) then
          return false, "Failed to receive response from server"
        end
        local header = Header.parse(hdr_data)

The problem is the second argument, `false`, to receive_buf, which
indicates whether to keep the delimiter attached to the returned string.
https://nmap.org/nsedoc/lib/nmap.html#receive_buf
match.numbytes(20) works by returning a string of length 19 and a
pseudo-delimiter of length 1. Discarding the delimiter with `false`
results in a string that is too short.

It turns out that this code used to work, despite the off-by-one bug. It
stopped working with the upgrade to Lua 5.3 (r35944 works, r35945
doesn't). That was when bin.unpack was replaced by string.unpack.
bin.unpack tolerated unpacking from a too-short string, but
string.unpack does not. The missing 20th byte didn't affect the output
of stun-info.nse because it was only a part of a transaction ID. Another
part of the code inadvertently compensated by beginning parsing starting
at the 19th byte, not the 20th.

I found three other places that use the problematic pattern
`socket:receive_buf(match.numbytes(X), false)`. I didn't check whether
the ones in redis.lua or rsync.lua cause a problem.
        $ git grep 'receive_buf.*match\.numbytes.*, false)'
        nselib/redis.lua:      status, data = self.socket:receive_buf(match.numbytes(len), false)
        nselib/rsync.lua:    status, data = self.socket:receive_buf(match.numbytes(4), false)
        nselib/rsync.lua:    status, data = self.socket:receive_buf(match.numbytes(len), false)
        nselib/stun.lua:    local status, hdr_data = self.socket:receive_buf(match.numbytes(Header.size), false)
        nselib/stun.lua:    local status, data = self.socket:receive_buf(match.numbytes(header.length), false)


> From 9a45ee73ccf7163ad83d7569f79f44bdb5ffa934 Mon Sep 17 00:00:00 2001
From: David Fifield <david () bamsoftware com>
Date: Fri, 6 Dec 2019 14:15:43 -0700
Subject: [PATCH] Fix an off-by-one error in stun.lua.

---
 CHANGELOG       | 3 +++
 nselib/stun.lua | 6 +++---
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/CHANGELOG b/CHANGELOG
index 45b8b680e..fe107d984 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -57,6 +57,9 @@ o [NSE][GH#1665] The HTTP library no longer crashes when code requests digest
 o [NSE] Fixed a bug in http-wordpress-users.nse that could cause
   extraneous output to be captured as part of a username. [Duarte Silva]
 
+o [NSE] Fixed an off-by-one bug in the stun.lua library that prevented
+  parsing a server response.
+
 Nmap 7.80 [2019-08-10]
 
 o [Windows] The Npcap Windows packet capturing library (https://npcap.org/)
diff --git a/nselib/stun.lua b/nselib/stun.lua
index 1b988c2f0..438cdbcc1 100644
--- a/nselib/stun.lua
+++ b/nselib/stun.lua
@@ -188,7 +188,7 @@ Response = {
     -- @name Response.Bind.parse
     parse = function(data)
       local resp = Response.Bind:new()
-      local pos = Header.size
+      local pos = Header.size + 1
 
       resp.header = Header.parse(data)
       resp.attributes = {}
@@ -248,7 +248,7 @@ Comm = {
   --         err string containing an error message, if status is false
   -- @name Comm.recv
   recv = function(self)
-    local status, hdr_data = self.socket:receive_buf(match.numbytes(Header.size), false)
+    local status, hdr_data = self.socket:receive_buf(match.numbytes(Header.size), true)
     if ( not(status) ) then
       return false, "Failed to receive response from server"
     end
@@ -258,7 +258,7 @@ Comm = {
       return false, "Failed to parse response header"
     end
 
-    local status, data = self.socket:receive_buf(match.numbytes(header.length), false)
+    local status, data = self.socket:receive_buf(match.numbytes(header.length), true)
     if ( header.type == MessageType.BINDING_RESPONSE ) then
       local resp = Response.Bind.parse(hdr_data .. data)
       return true, resp
-- 
2.20.1

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