Re: [Ncat] hang on ongoing ssl negotation in brokering mode

[David Fifield <david () bamsoftware com>]

On Thu, Jun 16, 2011 at 11:51:52PM +0300, Shinnok wrote:
> Hello David,
>
> On 06/13/2011 11:09 PM, David Fifield wrote:
> > Please find a way to factor this code into its own function. It's too
> > much to have it repeated in each read loop.
> >
> >             if(o.ssl && info->ssl && !info->ssl_accept_done){
> >                 int ret = SSL_accept(info->ssl);
> >                 int sslerr = 0;
> ...
> >                     goto loop_end;
> >                 }
> >             }
>
> Attached is a patch that tries to resolve this duplication issue,
> however, I am not pleased with the outcome that much, since there still
> is code duplication as well as unwarranted obfuscation as I guessed
> previously.
>
> However, maybe I am thinking it wrong, and I should just grab the entire
> ssl negotiation logic code as is and paste it to some function that
> takes as an argument not only an fdinfo structure(which is the clean
> way), but the fd_list_t pointers so that I can take the necessary action
> upon them as needed(rm_fd,etc..) directly from the function. However, I
> would refrain from doing that, since it breaks modularity if I put that
> kind of ncat_listen/ncat_broker specific logic in ncat_ssl.c.
> There's also that fact that I would need to pass fds_ready too, and then
> there is the question of how to skip the infinite loops(the continue's).
> Oh, and the goto..

Having the same code copy-pasted in four places is not practically
maintainable. We need to find a better solution. The patch you attached
is better, even though it requires a duplicated switch block, in that
someone looking at that code will be led to the ssl_handshake function,
where they can make a change once.

Please commit this patch.

> Maybe we are worrying too much about this code duplication issue, then
> we should. The code in ncat_broker.c and ncat_listen.c is already
> similar enough, especially in ncat_listen_stream vs. ncat_broker and
> handle_connection in both. Passing stuff around and calling functions
> adds to performance slowdown too, besides obfuscation, for no real gain.

You can't be serious about being concerned with function call overhead
here. That is vastly outweighed by the ongoing costs of code
maintenance, not to mention the runtime of the crypto operations
underneath.

> The only other way, is to refactor the entire logic code in the
> functions in question in order to make de-duplication possible, which is
> probably not warranted at this time.

That is completely fine. That's what our test program is for. If you can
find a way to express the program flow better, please do it.

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