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

[Kristof Boeynaems <kristof.boeynaems () gmail com>]

On Tue, Mar 3, 2009 at 6:34 AM, David Fifield <david () bamsoftware com> wrote:
> 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.

That is very well summarized.

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

Again a very good summary.

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

Yes, I don't like that code duplication either, that should indeed be
refactored. At the time I was eager to get a patch out for this (for
"demonstration purpose") and figured this was the most straightforward
way to do it, and less likely to break existing SSL support.
All suggestions for a better design are very welcome.

> 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 agree. The other alternative would be to specify TLSv1 and SSLv3
support at the command line, and that kind of implementation would be
very well supported by the current Nsock patch; but I agree that
"automatic guessing" would be far better suited for Ncat.
To make that reasonably performant, a different design should be implemented.

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

It sounds indeed rather straightforward. In fact, you made a short
test program doing something like that earlier, see
http://seclists.org/nmap-dev/2009/q1/0430.html.
What difficulties did you encounter this time?

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

Thanks ;)

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

Thanks again. By the way, I still owe the list the results of my
(little) more extensive scanning, I'll post them later on, when I have
again access to the data.

In the mean time, I encourage others to perform some testing as well!

Cheers,

Kristof

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