Nmap Development mailing list archives
Re: ncat should try connecting to all resolved addresses, not only the first one
From: Jaromir Koncicky <jkoncick () redhat com>
Date: Mon, 27 Jan 2014 08:08:40 -0500 (EST)
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/
Current thread:
- Re: ncat should try connecting to all resolved addresses, not only the first one Jaromir Koncicky (Jan 27)