Re: [NSE] rpc library

[Djalal Harouni <tixxdz () gmail com>]

On 2010-04-08 22:10:38 +0200, Patrik Karlsson wrote:
> 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.
yes I am interested in NFSv4, I will give it a try in the next days.

> 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.
ok.

> * 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 will use a more basic model. 

> * 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.
Yeh, the idea was: since the lib uses the OO paradigm, perhaps creating simple facilities functions to change the 
propreities of an rpc Comm object: SetProgram (changes the program name and the id), SetVersion etc. I will create a 
new function: ChkVersion() used in the Connect() and in the SetVersion() (I didn't forget that this is Lua :) ).

> * The RPC_Registry table is somewhat superfluous and in my opinion the code becomes more difficult to read this way.
the RPC_registry table is useful when adding new rpc services and to
avoid hard coding in functions which is not flexible.
perhaps a new name for the table ?

And what about this:
- patches for nfs-* and rpcinfo scripts, to use "return stdnse.format_output(false, data)" on errors
- 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


NB: I will send you the new version ASAP, so we can move to other
features, thx.

> //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

-- 
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/