Re: ncat should try connecting to all resolved addresses, not only the first one

[Jaromir Koncicky <jkoncick () redhat com>]

Well, I haven't touched ncat since I posted the last patch, but now I'm definitely going to finish this. However I'm 
quite unsure about some of the proposed changes.
I am not sure what other name to use instead of add_connection. From my point of view, it was the best choice, I didn't 
want to use "nsock_" prefix because it could lead to confusion that it is a part of nsock interface, which it is not. 
On the other hand, I should point out that it calls nsock_connect_*. So I came up with "add_nsock_connection".
I fully understand that resolve_multi is duplicating functionality and I don't like it either. David says that 
resolve_internal() should always return all addresses and then resolve() ignore all addresses except the first one if 
only first one is needed. That are the cases of caling resolve() with srcaddr, listenaddrs, httpconnect and 
socksconnect. This way I would allocate a list of all addresses in resolve_internal() and immediately destroy it, 
copying the contents from the first one. Maybe adding some boolean parameter telling whether I want all addresses or 
only one could be useful? You can see my solution in attached patch.
Also having one statically allocated target address seems rather useful to me. For example you don't need to 
dynamically allocate memory in case that only one address is resolved. But the main reason for using it was to preserve 
the behavior of original code where the first element was often accessed directly. In original code, targetaddr was 
initialized to AF_UNSPEC, meaning that targetaddr is "empty". If I dynamically allocated whole targetaddrs (including 
first element), the emptiness would be rather indicated by targetaddrs being NULL. So I would need to perform NULL 
check at every place where it is directly accessed and simulate the case that AF_UNSPEC is stored in first targetaddr.
As David said, it is possible that statically allocated first element could cause "hidden bugs", but aren't there more 
advantages of it?

----- Original Message -----
From: "David Fifield" <david () bamsoftware com>
To: "Jaromir Koncicky" <jkoncick () redhat com>
Cc: dev () nmap org
Sent: Friday, December 27, 2013 2:30:41 PM
Subject: Re: ncat should try connecting to all resolved addresses,      not     only the first one

On Wed, Dec 11, 2013 at 11:33:07AM -0500, Jaromir Koncicky wrote:
> That sounds really good that my patch is working. Also thanks for fixing the casts and malloc.
> The variable targetaddrs_allocated is here to indicate that targetaddrs was dynamically allocated and should be freed 
> in the end of the program.
> However, I got rid of this and now I am using up the first static member of targetaddrs and allocating memory only 
> for the second address and so on.
> I tried to make some tests for this, but the only testing cases I came up for this is to use 'localhost' as connect 
> address of client. In not-patched version, new tests with IPv4 should fail because ncat tries only IPv6 which is 
> resolved first. In patched ncat, all new tests should pass. Do you have more ideas for the tests?

This new patch is looking good. I am inclined to accept it.

add_connection is not a good name. It doesn't sound like it is actually
asking Nsock to make a connection, which it is. Can you think of a
better name for it, maybe reflective of the fact that all the functions
it calls are called ncat_connect_*?

There shouldn't be duplicated code in resolve_multi. I suggest that you
instead modify resolve_internal to return all addresses, and change
resolve to ignore all but the first result.

You should allocate the whole targetaddrs list. It's tricky if only the
first element is statically allocated. It means that some blocks of code
will be executed only rarely, which leaves more room for bugs to hide.

Thanks for writing tests; however they don't conceptually work in my
opinion. They rely on the name "localhost" resolving to more than one
address, which is not the case everywhere and anyway is out of the
control of the test program. I think we should leave these tests out
until we can think of better ones.

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

Attachment: diff.patch
Description:

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