New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support of ipv6 on client and server (asked by issue #8) #53

Open
wants to merge 12 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@svalat

svalat commented Jan 5, 2018

Hi,
I was looking for ipv6 support for one of my server so I implemented it. It should close #8 .

The patch contains changes to knock client and to knockd server to support v4/v6.

Client

About the client, there is a choice to be done for the default mode. I currently use the default IP returned by DNS resolving, which could be ipv6. Then you can choose to force v4/v6 by using option -4 / --ipv4 or -6 / --ipv6.

The user can give a host name or an ipv6 like with v4 mode.

To be seen if it is not better to keep v4 only by default, can be patched by changing the default value of o_ip line 55 in knock.c.

If we keep the default ip returned by the DNS maybe we should add a message to tell the user which ip we selected, otherwise someone might get stuck not knowing that he knock on ipv6 instead of v4.

Server

The server by default extract all the IPs from the selected interface (v6 and v4). Then it builds the pcap rules for all those ips. We can disable v6 support by using options -4 / --only-ip-v4.

Again to be seen if we do not make the invert and require enabling of ipv6 instead of enabled by default.

There is a trick for the pcap rules: tcp flags (SYN, FIN...) are not supported for ipv6 so I had to hardcode the extraction. Eg for FIN :

if (ipv6)
		bufsize = realloc_strcat(&buffer, " and ip6[13+40] & tcp-fin ", bufsize);//using directly mask as pcap didn't yet support flags for IPv6
else
		bufsize = realloc_strcat(&buffer, " and tcp[tcpflags] & tcp-fin ", bufsize);

Local address

IPv6 link often setup a local IP on every link, in the current patch I also handle the knocking on this local IP, if you want to disable this feature import my branch no-local there is a commit to get same behavior than the debian proposal from issue #8.

One thing on this, don't know why when requesting the list of IP of the selected interface the local link is returnes with an extra string : a0a6:sds4...:44%enp0s3. So to proceed I remove what is after %.

About IPv6 packet encapsulation

IPv6 can handle extended header for some usage. This is not handled by this patch, I considered the TCP/UDP header directly after the UDP packet. The other cases are detected and ignored.

Server rules

About the rules, we might want to call iptables to open access but ipv6 is handled by ip6tables instead of iptables. In order to proceed we provide two separate commands, one for ipv4 and one for ipv6 in the config file :

[options]
        logfile = /var/log/knockd.log

[openSSH]
        sequence    = 7000,8000,9000
        seq_timeout = 5
        command     = /bin/echo OPEN %IP% V4
        command_6   = /bin/echo OPEN %IP% V6
        tcpflags    = syn

[closeSSH]
        sequence    = 9000,8000,7000
        seq_timeout = 5
        command     = /bin/echo CLOSE %IP% V4
        command_6   = /bin/echo CLOSE %IP% V6
        tcpflags    = syn

Same for start_command_6 and stop_command_6.

New dependency

The code now depend on :

#include <netinet/ip6.h>

Doc

I also updated the documentation (--help and manpages) accordingly and add an ipv6 config file example for the server.

Bugfix

There was a double free corruption in knockd cleanup() function when the interface has multiple IPs.

Validation

I validated the full chain by makeing port knocking on ipv4 and v6 on one of my server using tcp-syn.

@svalat svalat force-pushed the svalat:master branch from e8d6954 to fdfb0d2 Jan 5, 2018

@svalat svalat force-pushed the svalat:master branch from fdfb0d2 to a908677 Jan 5, 2018

@svalat svalat changed the title from Add support of ipv6 on client and server (asked by #8) to Add support of ipv6 on client and server (asked by issue #8) Jan 8, 2018

@svalat svalat force-pushed the svalat:master branch from 6ab4ca0 to a908677 Jan 11, 2018

@Coksnuss Coksnuss referenced this pull request Aug 8, 2018

Open

Consider ipv6 #8

Coksnuss added a commit to Coksnuss/knock that referenced this pull request Aug 8, 2018

TDFKAOlli added a commit to TDFKAOlli/openwrt_knockd that referenced this pull request Sep 29, 2018

Included ipv6 patch (pull #53 - jvinet/knock#53) fetched from github
Included Null-pointer reference fix (pull #52 - jvinet/knock#52) fetched from github
Moved patch for unititialized tcp flags parameter to third patch position
Fixed patch files 002 and 003 to fit after ipv6 patch

Tested on OpenWrt 18.06.1 and found working for ipv4 and ipv6
@TDFKAOlli

This comment has been minimized.

TDFKAOlli commented Sep 29, 2018

Hi,
I would like to propose a small additional change. I'm using the same command for IPv4 and IPv6, so I don't like to double the "command". So when command6 is not set I would like to default to command.
Patch for this attached. (Note: Done on #53, #52 and #55, so line numbers might not match).
--- a/src/knockd.c 2018-09-29 23:55:24.330999447 +0200
+++ b/src/knockd.c 2018-09-30 00:04:38.333019487 +0200
@@ -1469,6 +1469,11 @@
if (attempt->fromIpV6)
{
start_command = attempt->door->start_command6;
+ // Default to "command" if "command6" is not set
+ if(NULL == start_command)
+ {
+ start_command = attempt->door->start_command;
+ }
stop_command = attempt->door->stop_command6;
} else {
start_command = attempt->door->start_command;

@svalat

This comment has been minimized.

svalat commented Sep 30, 2018

I made it for start & stop. Can you please test on your setup ? I do not currently have a IPV6 server under my hand.

As in your proposal I use NULL to detect not defined, please check it is enougth. It might keep a definition with empty value to be setup as "no command".

I also updated the manpage.

@TDFKAOlli

This comment has been minimized.

TDFKAOlli commented Sep 30, 2018

Thanks a lot. I'll update to use your change. I have tested it with my proposal and that was working fine on my router. Before nothing was executed on IPv6 knock, with change the "command" line was exectued using same config. As such I'm very confident it is working.
And I thought the same, just checking for NULL is best here, as it offers to disable the command by providing an empty string.
I'll let you know when I tested it.

@TDFKAOlli

This comment has been minimized.

TDFKAOlli commented Oct 1, 2018

Just noticed when compiling with new ipv6 .patch file from this pull request:

src/knockd.c: In function 'sniff':
src/knockd.c:1654:12: warning: 'ip6' may be used uninitialized in this function [-Wmaybe-uninitialized]
dport = ntohs(tcp->th_dport);
^~~~~~~~~~~~~~~~~~~~
I think compiler is right in that sense, that ip6 is not intiated in the alternative else-cases of "if(lltype == DLT_EN10MB)". So e.g. if lltype == DLT_LINUX_SLL or lltype == DLT_RAW, ip6 would be uninitialized and later in "else if (ip->ip_v == 6) {" we would try to use and dereference ip6...

Probably works in OpenWRT because all packets are of type DLT_EN10MB.

EDIT: Checked on OpenWRT and it is working. I still have problems to overwrite the default by an empty string, because of OpenWRT UCI config interpretation. But I did one manual change to check it at it is working.

EDIT2: Sketched the fix for the compiler warning:
@@ -1598,9 +1598,11 @@
#ifdef __linux__
} else if(lltype == DLT_LINUX_SLL) {
ip = (struct ip*)((u_char*)packet + 16);
+ ip6 = (struct ip6_hdr*)((u_char*)packet + 16);
#endif
} else if(lltype == DLT_RAW) {
ip = (struct ip*)((u_char*)packet);
+ ip6 = (struct ip6_hdr*)((u_char*)packet);
} else {
dprint("link layer header type of packet not recognized, ignoring...\n");
return;

TDFKAOlli added a commit to TDFKAOlli/openwrt_knockd that referenced this pull request Oct 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment