Nmap Development mailing list archives
Re: Fix off-by-one error in stun.lua
From: Gordon Fyodor Lyon <fyodor () nmap org>
Date: Thu, 12 Dec 2019 10:55:50 -0800
Thanks David. Good catch! Please check this fix in. I have also created an issue for checking and fixing the possible issues you referenced in the redis and rsync libraries: https://github.com/nmap/nmap/issues/1855 -Fyodor On Fri, Dec 6, 2019 at 3:16 PM David Fifield <david () bamsoftware com> wrote:
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/
_______________________________________________ Sent through the dev mailing list https://nmap.org/mailman/listinfo/dev Archived at http://seclists.org/nmap-dev/
Current thread:
- Fix off-by-one error in stun.lua David Fifield (Dec 06)
- Re: Fix off-by-one error in stun.lua Gordon Fyodor Lyon (Dec 12)
- Re: Fix off-by-one error in stun.lua David Fifield (Dec 13)
- Re: Fix off-by-one error in stun.lua Gordon Fyodor Lyon (Dec 12)