Re: Fix off-by-one error in stun.lua
Gordon Fyodor Lyon <[email protected]> Thu, 12 Dec 2019 10:55:50 -0800
| Newsgroups | gmane.comp.security.nmap.devel |
|---|---|
| Message-ID | <CAJjO9MkP3HDgfcKEeSbi9NVYTcm=nVjeWz00twyaxHmARd9NKQ@mail.gmail.com> |
--===============4634898112748404619== Content-Type: multipart/alternative; boundary="0000000000006165e00599864d07" --0000000000006165e00599864d07 Content-Type: text/plain; charset="UTF-8" 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 <[email protected]> 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 <[email protected]> > 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/ > --0000000000006165e00599864d07 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div dir=3D"ltr">Thanks David.=C2=A0 Good catch!=C2=A0 Please check this fi= x in.=C2=A0 I have also created an issue for checking and fixing the possib= le issues you referenced in the redis and rsync libraries:=C2=A0<a href=3D"= https://github.com/nmap/nmap/issues/1855">https://github.com/nmap/nmap/issu= es/1855</a><div><br></div><div>-Fyodor</div><div><br></div></div><br><div c= lass=3D"gmail_quote"><div dir=3D"ltr" class=3D"gmail_attr">On Fri, Dec 6, 2= 019 at 3:16 PM David Fifield <<a href=3D"mailto:[email protected]">d= [email protected]</a>> wrote:<br></div><blockquote class=3D"gmail_quo= te" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204= );padding-left:1ex">The stun-info script currently (r37773) crashes with a = "data string too<br> short" error.<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 $ nmap -n -Pn -sU -p 3478 <a href=3D"http://stu= n.ekiga.net" rel=3D"noreferrer" target=3D"_blank">stun.ekiga.net</a> --scri= pt=3Dstun-info -d<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 ...<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 NSE: stun-info against <a href=3D"http://stun.e= kiga.net" rel=3D"noreferrer" target=3D"_blank">stun.ekiga.net</a> (<a href= =3D"http://216.93.246.18:3478" rel=3D"noreferrer" target=3D"_blank">216.93.= 246.18:3478</a>) threw an error!<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 nselib/stun.lua:52: bad argument #2 to 'unp= ack' (data string too short)<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 stack traceback:<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 [C]: in function &#= 39;string.unpack'<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 nselib/stun.lua:52:= in field 'parse'<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 nselib/stun.lua:256= : in function <nselib/stun.lua:250><br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (...tail calls...)<= br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 nselib/stun.lua:338= : in method 'getExternalAddress'<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 scripts/stun-info.n= se:37: in function <scripts/stun-info.nse:30><br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (...tail calls...)<= br> <br> I tracked the error down to Comm.recv in stun.lua. The code intends to<br> receive exactly 20 bytes (Header.size), then pass those 20 bytes to<br> Header.parse. But self.socket:receive_buf is returning only 19 bytes,<br> which causes Header.parse to fail.<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 local status, hdr_data =3D self.socket:receive_= buf(match.numbytes(Header.size), false)<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 if ( not(status) ) then<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return false, "Failed to receive re= sponse from server"<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 end<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 local header =3D Header.parse(hdr_data)<br> <br> The problem is the second argument, `false`, to receive_buf, which<br> indicates whether to keep the delimiter attached to the returned string.<br= > <a href=3D"https://nmap.org/nsedoc/lib/nmap.html#receive_buf" rel=3D"norefe= rrer" target=3D"_blank">https://nmap.org/nsedoc/lib/nmap.html#receive_buf</= a><br> match.numbytes(20) works by returning a string of length 19 and a<br> pseudo-delimiter of length 1. Discarding the delimiter with `false`<br> results in a string that is too short.<br> <br> It turns out that this code used to work, despite the off-by-one bug. It<br= > stopped working with the upgrade to Lua 5.3 (r35944 works, r35945<br> doesn't). That was when bin.unpack was replaced by string.unpack.<br> bin.unpack tolerated unpacking from a too-short string, but<br> string.unpack does not. The missing 20th byte didn't affect the output<= br> of stun-info.nse because it was only a part of a transaction ID. Another<br= > part of the code inadvertently compensated by beginning parsing starting<br= > at the 19th byte, not the 20th.<br> <br> I found three other places that use the problematic pattern<br> `socket:receive_buf(match.numbytes(X), false)`. I didn't check whether<= br> the ones in redis.lua or rsync.lua cause a problem.<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 $ git grep 'receive_buf.*match\.numbytes.*,= false)'<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 nselib/redis.lua:=C2=A0 =C2=A0 =C2=A0 status, d= ata =3D self.socket:receive_buf(match.numbytes(len), false)<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 nselib/rsync.lua:=C2=A0 =C2=A0 status, data =3D= self.socket:receive_buf(match.numbytes(4), false)<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 nselib/rsync.lua:=C2=A0 =C2=A0 status, data =3D= self.socket:receive_buf(match.numbytes(len), false)<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 nselib/stun.lua:=C2=A0 =C2=A0 local status, hdr= _data =3D self.socket:receive_buf(match.numbytes(Header.size), false)<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 nselib/stun.lua:=C2=A0 =C2=A0 local status, dat= a =3D self.socket:receive_buf(match.numbytes(header.length), false)<br> <br> <br> From 9a45ee73ccf7163ad83d7569f79f44bdb5ffa934 Mon Sep 17 00:00:00 2001<br> From: David Fifield <<a href=3D"mailto:[email protected]" target=3D"= _blank">[email protected]</a>><br> Date: Fri, 6 Dec 2019 14:15:43 -0700<br> Subject: [PATCH] Fix an off-by-one error in stun.lua.<br> <br> ---<br> =C2=A0CHANGELOG=C2=A0 =C2=A0 =C2=A0 =C2=A0| 3 +++<br> =C2=A0nselib/stun.lua | 6 +++---<br> =C2=A02 files changed, 6 insertions(+), 3 deletions(-)<br> <br> diff --git a/CHANGELOG b/CHANGELOG<br> index 45b8b680e..fe107d984 100644<br> --- a/CHANGELOG<br> +++ b/CHANGELOG<br> @@ -57,6 +57,9 @@ o [NSE][GH#1665] The HTTP library no longer crashes when = code requests digest<br> =C2=A0o [NSE] Fixed a bug in http-wordpress-users.nse that could cause<br> =C2=A0 =C2=A0extraneous output to be captured as part of a username. [Duart= e Silva]<br> <br> +o [NSE] Fixed an off-by-one bug in the stun.lua library that prevented<br> +=C2=A0 parsing a server response.<br> +<br> =C2=A0Nmap 7.80 [2019-08-10]<br> <br> =C2=A0o [Windows] The Npcap Windows packet capturing library (<a href=3D"ht= tps://npcap.org/" rel=3D"noreferrer" target=3D"_blank">https://npcap.org/</= a>)<br> diff --git a/nselib/stun.lua b/nselib/stun.lua<br> index 1b988c2f0..438cdbcc1 100644<br> --- a/nselib/stun.lua<br> +++ b/nselib/stun.lua<br> @@ -188,7 +188,7 @@ Response =3D {<br> =C2=A0 =C2=A0 =C2=A0-- @name Response.Bind.parse<br> =C2=A0 =C2=A0 =C2=A0parse =3D function(data)<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0local resp =3D Response.Bind:new()<br> -=C2=A0 =C2=A0 =C2=A0 local pos =3D Header.size<br> +=C2=A0 =C2=A0 =C2=A0 local pos =3D Header.size + 1<br> <br> =C2=A0 =C2=A0 =C2=A0 =C2=A0resp.header =3D Header.parse(data)<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0resp.attributes =3D {}<br> @@ -248,7 +248,7 @@ Comm =3D {<br> =C2=A0 =C2=A0--=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0err string containing an e= rror message, if status is false<br> =C2=A0 =C2=A0-- @name Comm.recv<br> =C2=A0 =C2=A0recv =3D function(self)<br> -=C2=A0 =C2=A0 local status, hdr_data =3D self.socket:receive_buf(match.num= bytes(Header.size), false)<br> +=C2=A0 =C2=A0 local status, hdr_data =3D self.socket:receive_buf(match.num= bytes(Header.size), true)<br> =C2=A0 =C2=A0 =C2=A0if ( not(status) ) then<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0return false, "Failed to receive response f= rom server"<br> =C2=A0 =C2=A0 =C2=A0end<br> @@ -258,7 +258,7 @@ Comm =3D {<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0return false, "Failed to parse response hea= der"<br> =C2=A0 =C2=A0 =C2=A0end<br> <br> -=C2=A0 =C2=A0 local status, data =3D self.socket:receive_buf(match.numbyte= s(header.length), false)<br> +=C2=A0 =C2=A0 local status, data =3D self.socket:receive_buf(match.numbyte= s(header.length), true)<br> =C2=A0 =C2=A0 =C2=A0if ( header.type =3D=3D MessageType.BINDING_RESPONSE ) = then<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0local resp =3D Response.Bind.parse(hdr_data .. d= ata)<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0return true, resp<br> -- <br> 2.20.1<br> <br> _______________________________________________<br> Sent through the dev mailing list<br> <a href=3D"https://nmap.org/mailman/listinfo/dev" rel=3D"noreferrer" target= =3D"_blank">https://nmap.org/mailman/listinfo/dev</a><br> Archived at <a href=3D"http://seclists.org/nmap-dev/" rel=3D"noreferrer" ta= rget=3D"_blank">http://seclists.org/nmap-dev/</a><br> </blockquote></div> --0000000000006165e00599864d07-- --===============4634898112748404619== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Sent through the dev mailing list https://nmap.org/mailman/listinfo/dev Archived at http://seclists.org/nmap-dev/ --===============4634898112748404619==--