Re: [NSE] ntp-monlist

[jah <jah () zadkiel plus com>]

On 03/06/2010 19:14, David Fifield wrote:
> Here's some things I noticed while reviewing ntp-monlist.
>
> This code in getPrivateMode is roundabout:
>   local imp = ('00%x'):format(impl or 0x03):sub(-2,-1)
>   local rc = ('00%x'):format(requestCode or 0x2a):sub(-2,-1)
>   pay = bin.pack('H', ('17 00 %s %s 00 00 00 00'):format(imp, rc))
> A better way is to use printf width specifiers.
>   pay = bin.pack('H', ('17 00 %02x %02x 00 00 00 00'):format(impl or 0x02, requestCode or 0x2a))
> I still don't think this is the clearest way to do what you're trying to
> do--converting a number to hex and then back to a number. I think I
> would use this:
>   pay = string.char(
>     0x17, 0x00, impl or 0x03, requestCode or 0x2a, 0x00, 0x00, 0x00, 0x00
>   )
>   
Yeah, printf width specifiers - not sure why I didn't use them... As you
say, string.char is much simpler.
> All your comments in getPrivateMode are appropriate. You need to do
> something similar in obj_from_response.
>
> I didn't understand the purpose of the obj_from_response function. I
> suppose it's so you can use the u8 and u16 methods on the packet. That's
> justifiable, but a little out of the ordinary. You at least need a
> comment saying that's what you're doing.
>   
Quite right; the packet methods are really useful and uncomplicated to
use and I didn't want to implement them again for this script.  It might
be useful to allow packet objects to be created without IP and TCP or
UDP headers, but in the absence of this ability I chose simply to add
dummy headers before creating the object. I've renamed obj_from_response
to make_udp_packet, simplified it in the same vein as getPrivateMode and
added comment to clarify its purpose.
> What does the 0xFFF mean here? Is it supposed to be 0xFFFF?
>   -- length checks
>   local icount = bit.band(pkt:u16(off+4), 0xFFF)
>   local isize  = bit.band(pkt:u16(off+6), 0xFFF)
>   
These values are 12 (least significant) bits wide.  I'll add a comment
here to make this clear.

> The comment here makes sense, but does that code really work?
>   -- flags may be wrong-endian. why? I really don't know. Some implementations not doing htonl?
>   if t.flags > 0xFFFFFF then
>     t.flags = t.flags - 0xFFFFFF
>   end
>   
You're right, this is bollocks isn't it!  Thanks for catching it and
thanks for reviewing. I've committed the changes in r17824.

Best,

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