Re: Fix off-by-one error in stun.lua

[Gordon Fyodor Lyon <fyodor () nmap org>]

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/