Re: [PATCH] Extended SSL support in Nmap, review

[David Fifield <david () bamsoftware com>]

On Sat, Feb 21, 2009 at 08:01:33PM +0100, Kristof Boeynaems wrote:
> I wrote a patch for the issue described at length in  
> http://seclists.org/nmap-dev/2009/q1/0330.html.

Thanks very much for the patch. This is not a trivial issue and it is
noteworthy that you stepped up and wrote an implementation. The code in
general looks good. The change works great for Nmap version detection,
but I would like to see another design that is also suitable for Ncat.

Let me restate the problem, so I can see if I understand it and for the
benefit of anyone following the conversation. Nsock's SSL connection
function (used by Nmap service detection and by Ncat) makes its
connection in a way that is not understood by a small fraction of SSL
servers. Therefore neither version detection nor Ncat can connect to
these server. Furthermore, the binary probe that version detection uses
to detect an SSL service (SSLSessionReq) does not work against
SSLv2-only servers, so Nmap does not know to try to reconnect with SSL.

Now I will summarize your solution as I understand it. There are new
functions nsock_connect_ssl3 and nsock_connect_tls1 that parallel the
old nsock_connect_ssl. There's a new version detection probe,
SSLv23SessionReq, that works against SSLv2-only servers. More
importantly, there are new match lines that return a service of "ssl",
"tls1", or "ssl3". These service names are used internally by Nmap to
select which nsock_connect_* function to use, however these are always
changed to "ssl" in the final output.

I don't like the code duplication in the patch. The new functions
Nsock_SSL3_Init and Nsock_TLS1_Init are basically copies of
Nsock_SSL_Init. A better design is what you did with the connect
functions: Each of them just passes a special parameter to a generic
nsock_connect_ssl_all function.

The patch relies on Nmap's version detection to know which SSL connect
function to connect with. This won't be convenient for Ncat, which
doesn't have any version detection to rely on. The more I think about
it, the more I think that trying multiple methods on every connection is
the right way. First SSLv23, then TLSv1, then SSLv3. It will be slower
when the first connection fails, but the research offered so far
indicates that it succeeds more than 99% of the time. It will offer Ncat
a convenient API to connect to any SSL server.

I don't think implementing this solution will be easy. I briefly looked
into doing it myself but gave up when it wasn't as simple as I
anticipated. A sensible default would be to try all three methods on
every SSL connection. There could be a way to control which method is
tried first, or to disable the fallbacks, but it would have to be done
in a nice way. I don't think either of those features is worth adding an
extra parameter to nsock_connect_ssl, unless it's a structure that can
express a lot of options at once.

Please don't be discouraged. It means a lot that you wrote this code and
offered it for review, not to mention the research that led you to it.
All important patches of this sort get reviewed and modified before they
are incorporated.

> - I did some performance testing with 1000 random SSL servers with  
> various ports open (443,465,636,990,995,993). I compared three instances  
> of Nmap:
> 1. Nmap-4.85BETA3
> 2. Nmap-4.85BETA3 with the new nmap-services-probes file provided in the  
> patch. Note that for this test, I had to slightly change the  
> nmap-services-probes file, replacing all "ssl3" and "tls1" detections  
> with "ssl", as Nmap-4.85BETA3 does not know these other versions.
> 3. A patched version of Nmap-4.85BETA3
>
> I used the following command: nmap -T4 -v -n -PN -sV  
> -p443,465,636,990,995,993 -iL <list of 100 random SSL servers collected  
> earlier via an iR scan> -d -oA <filename>

I especially want to thank you for these test results. They are an
indication of intellectual honesty and rigor as a developer. The results
(with the typo correction in http://seclists.org/nmap-dev/2009/q1/0485.html),
show that this SSL fix, while not urgent, is worthwhile.

David Fifield

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