Re: [PATCH] nselib rpc.lua

[Patrik Karlsson <patrik () cqure net>]

Hi Djalal,

Once again, thanks for the patch, I appreciate it! I think it brings some great improvement to the error handling and I 
like the changes you've done in regard of how the Comm class is used.
I also think there are some good points in your TODO (specifically the NFSv4 support and authentication). If you want 
to help out looking in to these let me know.

Here are a few comments in regard to the patch:

* Make sure that all function names and parameters follow the same naming convention as used in the library.
  I know this is somewhat messy, looking at the NSE libraries and scripts as a whole. I have certainly introduced my 
share of the mess too changing conventions between scripts.
  That said, I think you should keep to the same naming convention as already used by a script or library to keep it 
consistent.

* The parameters "program" and "version" in the Comm:new method are never used.
   When creating a Comm instance like this Comm:new({program='mountd', version=mountd.version}) the program and version 
parameters are passed as a table in parameter o.
   While the function could take additional parameters, the current implementation does nothing with them.

* I'm not particularly happy with the set_rpcinfo function as it does not really match up with its description and also 
because it's never called outside of the constructor/new method. 
  I guess the reason for this code not being inside of the constructor is that it could then be returning nil on 
failure which isn't great.
  I therefore suggest you to move this code out to the connect method instead and catch any version mismatch there and 
return an error.

* The RPC_Registry table is somewhat superfluous and in my opinion the code becomes more difficult to read this way.

//Patrik


On 6 apr 2010, at 01.52, Djalal Harouni wrote:

> attached are two files: rpc.lua.diff and rpc.lua.new (new lib)
>
> new rpc.lua (code cleaned):
> - Comm Class/object will store/handle all rpc program infos and low
>  level network operations.
> - Comm object is passed to NFS, Mount, Portmap functions as a parameter
>  (they don't need to have a local copy).
> - added some debug output, nfs and mount versions mismatch check.
> - added NFS and Mount stat codes support with msg descriptions, perhaps
>  useful in the futur: nfs vulnerabilitu check (check nfs code, check
>  permissions issues, ressource limits ... etc)!!, i don't know if there
>  is a tool that let as check the common POSIX ACLs issues over NFS
>  (NFSv4 is a good condidate) or mount options: filesystems with block
>  special devices enabled ... etc, perhaps patrik's nfs-acls.nse script
>  is a good start.
>
> notes:
> - new protocols who may use the rpc.lua, must be added to the RPC_version table.
> - all the nse scripts wich uses the rpc.Helper class functions, must use
>  this code to handle errors:
>  ---
>  return stdnse.format_output(false, data)
>  ---
>  so we need probably simple patches for nfs-* and rpcinfo scripts.
>
> todo:
> - separate code: NFS code in nfs.lua ... etc, all these libraries will
>  require("rpc") to handle rpc communications, Portmap class and RpcInfo
>  functions must stay in rpc.lua
> - complete the NFS procedures.
> - add others filetypes support.
> - add NFSv4 support.
> - add more rpc stuff: error code, authentification ... etc
> - ...
>
> NB: patrik has already implemented this sutff, this is simply a
> re-design.
> thx patrik.
>
> feedbacks ?
>
>
> On 2010-04-02 20:03:40 +0200, Patrik Karlsson wrote:
> > I've commited this as r17146. I ended up patching only the ReadDir function as the StatFs function is only available 
> > for version less than 3 where the file handle length is fixed at 32 octets.
> > Thanks for the bug report Djalal!
> >
> > //Patrik
> >
> > On 2 apr 2010, at 17.49, Djalal Harouni wrote:
> >
> > > BTW patrik if you can commit these changes (nfsv3 fhandles correction) it will
> > > be cool so i can work on a clean svn copy.
> > >
> > > On 2010-04-02 16:41:47 +0100, Djalal Harouni wrote:
> > > > On 2010-04-02 17:32:08 +0200, Patrik Karlsson wrote:
> > > > > Hi Djalal,
> > > > >
> > > > > On 2 apr 2010, at 17.23, Djalal Harouni wrote:
> > > > >
> > > > > > On 2010-04-02 17:10:00 +0200, Patrik Karlsson wrote:
> > > > > > > I'm having some trouble applying this patch against the rpc.lua in svn.
> > > > > > > A portion of the patch gets rejected and my diff/patch skills are limited.
> > > > > > >
> > > > > > > Can anyone share some diff/patch magic or can you send me a patch against svn Djalal?
> > > > > > hi patrick i just send you the patch nfsv3_rpc.lua.diff against r17144
> > > > > > and it's attached to, for the second patch i will probably add some more
> > > > > > error handling and reporting as suggested by david:
> > > > > > http://seclists.org/nmap-dev/2010/q1/922
> > > > > >
> > > > > > thx
> > > > >
> > > > > This patch looks very small compared to the first you sent me which contained both the NFS3 file handle fixes, 
> > > > > comm class re-design and improved error handling.
> > > > > I had a quick look at the comm re-design in the previous patch you sent, and it looked quite good. Will it be 
> > > > > part of the second patch you mentioned?
> > > > yes, i must rewrite the error handling code.
> > > >
> > > > > Oh, and the self.version > 2 patch is incorrect as NFSv3 does not have the STATFS procedure, the procedure 17 has 
> > > > > been replaced with READDIRPLUS in v3.
> > > > ah sorry.
> > > >
> > > > > > -- 
> > > > > > Djalal
> > > > > > http://dzcore.wordpress.com
> > > > > > <nfsv3_rpc.lua.diff>_______________________________________________
> > > > > > Sent through the nmap-dev mailing list
> > > > > > http://cgi.insecure.org/mailman/listinfo/nmap-dev
> > > > > > Archived at http://seclists.org/nmap-dev/
> > > > >
> > > > >
> > > > > //Patrik
> > > > >
> > > > > --
> > > > > Patrik Karlsson
> > > > > http://www.cqure.net
> > > > > http://www.twitter.com/nevdull77
> > > >
> > > > -- 
> > > > Djalal
> > > > http://dzcore.wordpress.com
> > >
> > > -- 
> > > Djalal
> > > http://dzcore.wordpress.com
> > > _______________________________________________
> > > Sent through the nmap-dev mailing list
> > > http://cgi.insecure.org/mailman/listinfo/nmap-dev
> > > Archived at http://seclists.org/nmap-dev/
> >
> > --
> > Patrik Karlsson
> > http://www.cqure.net
> > http://www.twitter.com/nevdull77
>
> -- 
> Djalal
> http://dzcore.wordpress.com
> <rpc.lua.diff><rpc.lua.new>_______________________________________________
> Sent through the nmap-dev mailing list
> http://cgi.insecure.org/mailman/listinfo/nmap-dev
> Archived at http://seclists.org/nmap-dev/

--
Patrik Karlsson
http://www.cqure.net
http://www.twitter.com/nevdull77





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