Bug 978964 - [RFE] Running Redis server, which does not support IPv4, nc cannot connect to it using localhost
Summary: [RFE] Running Redis server, which does not support IPv4, nc cannot connect to...
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: nmap
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Pavel Zhukov
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2013-06-27 11:54 UTC by Vít Ondruch
Modified: 2017-07-25 14:56 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-07-25 14:56:41 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)
patch: ncat will try all resolved addresses instead of only the first (3.94 KB, patch)
2013-08-30 14:58 UTC, Jaromír Končický
no flags Details | Diff
slightly improved version of the patch, tests shall not fail now (4.31 KB, patch)
2013-09-02 09:03 UTC, Jaromír Končický
no flags Details | Diff
New patch (17.46 KB, patch)
2013-10-11 09:05 UTC, Jaromír Končický
no flags Details | Diff
New patch (18.11 KB, patch)
2013-10-29 16:04 UTC, Jaromír Končický
no flags Details | Diff

Description Vít Ondruch 2013-06-27 11:54:34 UTC
Description of problem:
During review of rubygem-redis [1], it was discovered, that one of its test cases [2] fails. The testcase executes "nc localhost 6381", which fails with message "Ncat: Connection refused.". This works just fine if localhost4, 0.0.0.0 or 127.0.0.0 is used instead of localhost. It seems that Ncat is trying to connect to IPv6 address, while Redis does not support IPv6 ATM [3]. In such case, Ncat should try also the IPv4 address IMO.

Version-Release number of selected component (if applicable):
$ rpm -qf `which nc`
nmap-ncat-6.25-2.fc19.x86_64

$ rpm -q redis
redis-2.6.13-2.fc20.x86_64


How reproducible:
Always


Steps to Reproduce:
1. redis-server
2. $ nc localhost 6379
Ncat: Connection refused.
3.

Actual results:
$ nc localhost 6379
Ncat: Connection refused.


Expected results:
$ nc localhost4 6379

Additional info:

$ sudo netstat -ntlp | grep redis
tcp        0      0 0.0.0.0:6379            0.0.0.0:*               NASLOUCHÁ  25062/redis-server 


[1] https://bugzilla.redhat.com/show_bug.cgi?id=978284
[2] https://github.com/redis/redis-rb/blob/master/test/publish_subscribe_test.rb#L135
[3] https://github.com/antirez/redis/pull/61

Comment 1 Michal Hlavinka 2013-06-27 12:14:47 UTC
Thanks for reporting this.
Reproducer without redis-server:

1)make sure /etc/hosts contains 127.0.0.1 localhost and ::1 localhost
2)
one terminal:
 ncat -l 127.0.0.1 1234
second terminal:
echo test | ncat localhost 1234

-> connection fails

ncat translates localhost, gets ipv6+ipv4 address. Tries ipv6, fails and exits with error.

RFC4038 encourages applications to try ipv4 when ipv6 failed

Comment 2 Jaromír Končický 2013-08-28 15:18:37 UTC
The problem is actually much bigger. 
Even when I try, for example "ncat google.com 80", ncat fails with message "Ncat: Network is unreachable." for me. But if I type "ncat -4 google.com 80", ncat connects.
When ncat resolves hostname with "getaddrinfo" function, it takes only the first resolved address, wchich is, in our cases, IPv6 address.
As Michal wrote above, the program concept must be totally changed: 
ncat must take all resolved addresses instead of only the first one and when first connect fails, don't exit immediately but return and try another address.
Shouldn't upstream do this remake?

Comment 3 Jaromír Končický 2013-08-30 12:17:31 UTC
Added post at dev: http://seclists.org/nmap-dev/2013/q3/476

Comment 4 Jaromír Končický 2013-08-30 14:58:03 UTC
Created attachment 792215 [details]
patch: ncat will try all resolved addresses instead of only the first

I created a patch that will add the mentioned functionality.
With the patch applied, the reproducer commands should work. You can try running with --verbose to see how it tries to connect.
Please note that this patch is very ugly and may break other ncat's functionalities. I will also wait for upstream feedback and eventually improve the patch to be safe.
The patch is based on nmap-6.40-2.fc19.

Comment 5 Tomáš Hozza 2013-09-02 07:27:08 UTC
(In reply to jkoncick from comment #4)
> Created attachment 792215 [details]
> patch: ncat will try all resolved addresses instead of only the first
> 
> I created a patch that will add the mentioned functionality.
> With the patch applied, the reproducer commands should work. You can try
> running with --verbose to see how it tries to connect.
> Please note that this patch is very ugly and may break other ncat's
> functionalities. I will also wait for upstream feedback and eventually
> improve the patch to be safe.
> The patch is based on nmap-6.40-2.fc19.

Thanks for the patch. There some failures in ncat test suite with your patch
applied. Please run the ncat/test/ncat-test.pl to test if your patch doesn't
break anything.

Please also rebase your patch on the latest upstream SVN repository
http://nmap.org/book/install.html#inst-svn


Here is diff of the ncat-test.pl with and without the patch:

$ diff test_without_patch test_with_patch
11,12c11,14
< PASS Server UNIX socket listen on ncat.unixsock (STREAM)
< PASS Server UNIX socket listen on ncat.unixsock --udp (DGRAM)
---
> FAIL Server UNIX socket listen on ncat.unixsock (STREAM)
>      Server got "", not "abc\n" from client at ./ncat-test.pl line 606.
> FAIL Server UNIX socket listen on ncat.unixsock --udp (DGRAM)
>      Server got "", not "abc\n" from client at ./ncat-test.pl line 623.
14c16,17
< PASS Connect connection refused exit code
---
> FAIL Connect connection refused exit code
>      Exit code was 2, not 1 at ./ncat-test.pl line 658.
79c82,83
< PASS --exec through proxy
---
> FAIL --exec through proxy
>      Server received "CONNECT 127.0.0.1:40000 HTTP/1.0\r\n\r\n", not "abc\n" at ./ncat-test.pl line 1139.
95c99,100
< PASS --exec through proxy, environment variables
---
> FAIL --exec through proxy, environment variables
>      Client received "CONNECT 127.0.0.1:40000 HTTP/1.0\r\n\r\n". at ./ncat-test.pl line 1168.
152,154c157,162
< PASS Connect through HTTP proxy with -p
< PASS Connect through SOCKS4 proxy with -p
< PASS Connect to UNIX datagram socket with -s
---
> FAIL Connect through HTTP proxy with -p
>      Died at ./ncat-test.pl line 1534.
> FAIL Connect through SOCKS4 proxy with -p
>      Died at ./ncat-test.pl line 1555.
> FAIL Connect to UNIX datagram socket with -s
>      Died at ./ncat-test.pl line 1580.
171c179,182
< PASS HTTP CONNECT proxy relays
---
> FAIL HTTP CONNECT proxy relays
>      Proxy relayed "CONNECT 127.0.0.1:40000 HTTP/1.0
> 
> ", not "abc\n" at ./ncat-test.pl line 1880.
355c366
< 4 unexpected failures.
---
> 13 unexpected failures.

Comment 6 Jaromír Končický 2013-09-02 09:03:40 UTC
Created attachment 792769 [details]
slightly improved version of the patch, tests shall not fail now

Comment 7 Tomáš Hozza 2013-09-04 12:34:27 UTC
(In reply to jkoncick from comment #6)
> Created attachment 792769 [details]
> slightly improved version of the patch, tests shall not fail now

Hi.

I had a look at your new patch and have a couple of suggestions:

1. in ncat_core.c:resolve_internal() - I don't think it is really a good idea
   to secretly save a pointer to dynamically allocated memory to some global
   variable and then free it only in one single particular part of the
   program execution path. The dynamically allocated memory should be either
   returned as a pointer from the function, or assigned to some of the function
   arguments.

   The resolve() function that calls resolve_internal() function is used on
   multiple places in the main(). Therefore saving the pointer to a global
   variable leaks memory:

$ valgrind ncat -4 -g foo.com -g bar.com example.com
==21425== Memcheck, a memory error detector
==21425== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==21425== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==21425== Command: ncat -4 -g foo.com -g bar.com example.com
==21425== 
Ncat: Connection timed out.
==21425== 
==21425== HEAP SUMMARY:
==21425==     in use at exit: 90,627 bytes in 2,668 blocks
==21425==   total heap usage: 2,837 allocs, 169 frees, 140,671 bytes allocated
==21425== 
==21425== LEAK SUMMARY:
==21425==    definitely lost: 0 bytes in 0 blocks
==21425==    indirectly lost: 0 bytes in 0 blocks
==21425==      possibly lost: 0 bytes in 0 blocks
==21425==    still reachable: 90,627 bytes in 2,668 blocks
==21425==         suppressed: 0 bytes in 0 blocks
==21425== Rerun with --leak-check=full to see details of leaked memory
==21425== 
==21425== For counts of detected and suppressed errors, rerun with: -v
==21425== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 2 from 2)

Ncat with your latest patch:
$ valgrind ./ncat -4 -g foo.com -g bar.com example.com
==21426== Memcheck, a memory error detector
==21426== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==21426== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==21426== Command: ./ncat -4 -g foo.com -g bar.com example.com
==21426== 
Ncat: Could not connect to to "example.com". Run with --verbose for details.
==21426== 
==21426== HEAP SUMMARY:
==21426==     in use at exit: 86,563 bytes in 2,664 blocks
==21426==   total heap usage: 2,829 allocs, 165 frees, 136,408 bytes allocated
==21426== 
==21426== LEAK SUMMARY:
==21426==    definitely lost: 128 bytes in 2 blocks
==21426==    indirectly lost: 64 bytes in 1 blocks
==21426==      possibly lost: 288 bytes in 1 blocks
==21426==    still reachable: 86,083 bytes in 2,660 blocks
==21426==         suppressed: 0 bytes in 0 blocks
==21426== Rerun with --leak-check=full to see details of leaked memory
==21426== 
==21426== For counts of detected and suppressed errors, rerun with: -v
==21426== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 2 from 2)


2. Because of the point 1. I think it would be better to either modify the
   resolve_internal() function and introduce a new resolve_multi() as a opposite
   to existing resolve() function that resolves and returns only the first
   address OR modify the resolve() function itself and also every part of the
   ncat source where it is used.

3. In the ncat_connect() you use GOTO statement. The label that goto jumps it in the
   wrong place, because every time it jumps, initialization of some variables and
   contexts is done again and again and it may leak more memory.

   Anyway ncat uses event loop to process tasks (connect, timeout, I/O, ...). Once
   the event loop is started it makes more sense to create a new connect event for
   next address if you find out in connect_handler() that the connecting failed or
   timeouted.

4. Currently your solution works only if ncat connects only directly to some server.
   It would not work if it is connecting for example through HTTP proxy that is
   listening only on IPv4 address, but runs on server that has also IPv6 address.

Comment 8 Jaromír Končický 2013-09-05 07:29:37 UTC
I agree with you at point 1. I am fully aware of the memory leak. I needed somehow to transfer all addresses from resolve_internal function, and because it had means to transfer only one, I decided to do this through a global variable. I would need to add freeaddrinfo() to more places for the other cases, I was not sure where exactly however. The point 2. might be a good solution.
About 3., yes, GOTO is not nice construct at all. I an not yet familiar enough with the code. I will need to study it more so that I know better where and how to put the loop so it is clear. I wanted to make a fast patch to show upstream asap that we are doing something with this issue and to ask for their suggestions about the solution, before I make deeper changes in the code.

Comment 9 Jaromír Končický 2013-10-11 09:05:39 UTC
Created attachment 810919 [details]
New patch

I made a different patch to solve this problem. I was following the comments of Tomas Hozza and David Fifield on nmap upstream and tried to make the changes according to them. So no GOTO statement, no hidden global variable to store resolve results and next connection is added in connect_handler() if previous connection fails.
I hope it is more acceptable now, but I think it still does not solve the problem 4) in Tomas's comment, but that would be more complicated.
I'm proposing this also upstream.

Comment 10 Tomáš Hozza 2013-10-24 13:00:37 UTC
(In reply to jkoncick from comment #9)
> Created attachment 810919 [details]
> New patch
> 
> I made a different patch to solve this problem. I was following the comments
> of Tomas Hozza and David Fifield on nmap upstream and tried to make the
> changes according to them. So no GOTO statement, no hidden global variable
> to store resolve results and next connection is added in connect_handler()
> if previous connection fails.
> I hope it is more acceptable now, but I think it still does not solve the
> problem 4) in Tomas's comment, but that would be more complicated.
> I'm proposing this also upstream.

Hi.

I had a look at your new patch. I must say you used much better approach this
time. However I still have some tips for things that can be improved:

You replaced "targetss" with fixed sized array with arbitrary size. From my
point of view it would be better not to expect that some size should be enough,
but rather use dynamically sized array, or to use linked list.

As a hint I would personally introduce new structure like
struct targetaddr {
    union sockaddr_u targetss;
    size_t targetsslen;
    struct targetaddr* next;
}
and use linked list of dynamically allocated items. The last one would have
"next" pointer set to NULL, so you know when to stop iterating.
The function resolve_multi() could construct the linked list and store it
in a pointer passed as function argument. This way the resolve_multi()
could be reused in the future also in other parts of ncat if needed.
The linked list can be stored in global pointer, and after nsock_loop()
returns, you would free the dynamically allocated linked list.

This approach would require changes in add_connection to take pointer to the
current item in the list as an argument, rather than index. Also the
connect_handler() would require minor changes.

Note that nsock events have void pointer to "user data" which is passed also
to connection handler. So you could use it for passing the linked list to
the connection_handler() or use one global pointer for list HEAD (so you can
free it in the end) and another pointer for the current item in the list.

Comment 11 Jaromír Končický 2013-10-29 16:04:05 UTC
Created attachment 817130 [details]
New patch

Hi. I followed your instructions and reworked the patch.
Personally, before making the first patch I was also thinking about making something to dynamically store all resolved addresses, like the linked list, but I rather wanted to avoid dynamically allocated memory and I also presumed that there wouldn't be cases with more than 10 addresses, and if yes, ncat would successfully connect before reaching the limit. But now I made dynamic allocation anyway.
I also needed to handle that one (the first) target address is used "more often" and without actually calling resolve_multi, using more that one address is actually rather a "special case". So I added a static first item of the list.
I woud like to show the patch to upstream (maybe both this and previous solution) and discuss again with them.
Thanks for helping.

Comment 12 Jaromír Končický 2013-12-16 16:01:16 UTC
It seems that nmap upstream is finally going to review and eventually accept the patch. Read more here: http://seclists.org/nmap-dev/2013/q4/281.
When it is accepted, I can also add the patch into our package until we update to newer upstream version including the patch.

Comment 13 Michal Hlavinka 2014-04-16 12:52:45 UTC
Jaromír, what's the status of this feature being included upstream? 
Last email I can see is from upstream (Dec 27), were those comments addressed? 
http://seclists.org/nmap-dev/2013/q4/326

Comment 14 Jaromír Končický 2014-04-16 13:14:58 UTC
I had some communication with upstream also in january and february (I also slightly improved my patch according to Davis's comments, but also added mine comments):
http://seclists.org/nmap-dev/2014/q1/100
http://seclists.org/nmap-dev/2014/q1/203
Jacek said he had nothing against including it and is just waiting for approval from David of Fyodor. Since then I did not get any other response, so maybe it's time for pinging them.

Comment 15 Vít Ondruch 2016-10-11 11:21:52 UTC
Ping? What is the state here? Is it resolved?

Comment 16 Daniel Miller 2017-03-17 02:34:17 UTC
Patch accepted by upstream: http://seclists.org/nmap-dev/2017/q1/215

Only took us 3.5 years. Thanks!

Comment 17 Fedora Admin XMLRPC Client 2017-05-31 12:25:38 UTC
This package has changed ownership in the Fedora Package Database.  Reassigning to the new owner of this component.

Comment 18 Pavel Zhukov 2017-07-25 14:56:41 UTC
Works fine with nmap-ncat-7.40-7 in Fedora 26.
Thank all contributors!


Note You need to log in before you can comment on or make changes to this bug.