Re: [NSE][PATCH] OpenSSL bindings for NSE

[David Fifield <david () bamsoftware com>]

On Wed, Oct 08, 2008 at 02:14:50PM +0200, Sven Klemm wrote:
> Fyodor wrote:
> > On Mon, Sep 22, 2008 at 05:47:21PM -0600, David Fifield wrote:
> > > On Fri, Sep 19, 2008 at 09:12:24AM +0200, Sven Klemm wrote:
> > > > Hi everyone,
> > > >
> > > > here is the latest OpenSSL bindings patch for nmap including support for 
> > > > multiprecision integer arithmetics, message digests, hmac, symmetric 
> > > > encryption, symmetric decryption.
> > > > Documentation for the new functions is also included.
> > > Hi Sven. This is looking great. The documentation is especially
> > > appreciated. This module will open a lot of doors for script developers
> > > and I'd like to see it integrated.
> >
> > I completely agree with David here and think Sven's OpenSSL module is
> > a winner!  We just need to figure out these last nagging issues (such
> > as how to degrade gracefully for people w/o OpenSSL).
>
> I've attached a new version which handles all mentioned issues.
> The action of the SSH-hostkey script is overwritten if no OpenSSL is
> available. The action produces a warning about missing OpenSSL when
> verbosity >= 3 the advantage of modifying the action is you only get
> the warning when the script would really trigger. But it could also be
>  modified to produce the warning at include time.
> The patch also removes references to nse_hash and modifies the pop3
> library to work with and without OpenSSL. The pop3 functions requiring
> OpenSSL will immediately return an error code indicating OpenSSL is
> missing if OpenSSL is missing.

This looks very good. I can't think of any remaining issues that would
be a barrier to integration. I tested it with and without OpenSSL on
GNU/Linux (and Windows, see below). Please commit it.

There are a few small things that should be changed or fixed, but they
can be done shortly after the merge takes place. Here's my list:

* The patch you attached didn't apply to mswin32/nmap.vcproj. Also the
  project file doesn't include the new nse_openssl.* files, leading to a
  compile error:
nse_init.cc(253) : error C2065: 'OPENSSLLIBNAME' : * undeclared identifier
nse_init.cc(253) : error C2065: 'luaopen_openssl' : undeclared identifier

* Delete the nse_hash.* files.

* The nmap_config.h file referred to in nse_openssl.* doesn't exist in
  the Windows build, leading to another compile error. The file
  nmap_winconfig.h should be used instead (#ifdef WIN32, see Target.cc).
  But in this case it appears to be used only for HAVE_OPENSSL, which
  the Unix build already checks for before including the files. Since
  OpenSSL is presumed to be available on Windows, that include can be
  removed and replaced with more specific ones.

* I would like to see the new fingerprint module be moved back into a
  part of ssh1 or ssh2. It could even be a part of both, with one module
  grabbing it from the other. "fingerprint" is too general a name for a
  module that only handles SSH fingerprints, especially when Nmap has
  things like OS and version fingerprints. And because it only does SSH,
  it should be part of an SSH module.

Great work! Like I said, go ahead and do the merge and then we can fix
these smaller issues together. The openssl module is already showing
fruit, especially with the SSH-hostkey script, a good one, and Ron's
forthcoming enhancements to the Windows networking scripts and
libraries.

David Fifield

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