Ipv6rebased #309

bastien-roucaries · 2020-04-04T15:19:25Z

Old ipv6 rebased over simplify address and upstream.

For reference

INET_ADDRSTRLEN is portable and allow to be robust against special crafted adress signed-off: Bastien Roucariès <rouca@debian.org>

getaddrinfo is the correct API for resolving IP and checking correctness Use it, instead of manual parsing

Less code and less copy/paste thus less errors

Introduce IPV6 constant and IPV46 constant for adress length

The remote code seems to be independent from the fwknop project though. Until it will be capable to return IPv6 addresses, in itself this will remain irrelevant for the purpose of adding IPv6 support to fwknop. On another hand, it does help us introduce definitions and update headers to actually support IPv6.

It is now called is_valid_ip_addr() and expects an additional parameter for the address family.

This greatly loosens the check for a valid IPv4/IPv6 there - but it is redundant anyway, since there is always a call to is_valid_ip_addr().

This alone should allow interacting with IPv4 firewalling rules over IPv6, for these two protocols.

This will allow porting the raw ICMP code to IPv6.

I believe it should be more portable this way, since AF_INET is required to be present in <sys/socket.h> in POSIX.

This should help with portability for the protocol family eventually.

This reverts commit d33c240.

The correct syntax is the comma (",") instead.

The problem looks real, fix looks good to me, but have not been able to reproduce the actual issue so far :(

This should address mrash#1.

Fixes mrash#1.

Seb35

I have no maintainer role in fwknop, but I reviewed and tested this PR.

It mostly work:

when I launch fwknopd -6 -U then ss -tulpn shows fwknopd listens on udp *:62201, the chains FWKNOP_INPUT are added in iptables and ip6tables, the jump from INPUT to FWKNOP_INPUT is added in iptables but not in ip6tables (functional issue 1) (maybe the rules in fwknopd.conf should be duplicated: e.g. IPT6_INPUT_ACCESS may be different than IPT_INPUT_ACCESS)
when submit a knock with an IPv6 address it is correctly authorised in ip6tables,
but when I submit an IPv4 the logs say "Added access rule to FWKNOP_INPUT for x.x.x.x -> ::/0 tcp/22, expires at 1705057386" but there is no authorisation for this IPv4 in iptables (functional issue 2)

I backported this PR to 2.6.10 and integrated it in Debian package, but there were almost no merge conflict, so I don’t think it is an issue in my process.

Also, there should be an entry in the changelog.

Also, I wonder if the default behaviour should not be with IPv4+IPv6 even without the -6 flag (and perhaps -4 (resp. -6) to restrict to IPv4 (resp. IPv6)). In this case it should be highlighted in the changelog to warn the sysadmins.

Seb35 · 2024-01-11T22:10:29Z

common/fko_util.c

-    if (*ndx == '.')
-        total_size--;
-


I aggree it is useless to modify the variable total_size here, but it seems to be to allow the final dot without counting it in the last label (63 characters without counting the delimiting dots). I guess the original intent was label_size--;.

Seb35 · 2024-01-11T22:30:15Z

client/http_resolve_host.c

+        log_msg(LOG_VERBOSITY_ERROR,
+		"[-] Could not resolve IP via: '%s%s'", extraerror1, extraerror2);
+        return(-1);
+    }


Very minor issue: wrong indentation from here to the end of the function.
Also, to stay aligned with the global style, it is 4 spaces and not 2 for this whole function.

Seb35 · 2024-01-11T23:24:35Z

server/config_init.c

@@ -1381,6 +1394,9 @@ config_init(fko_srv_options_t *opts, int argc, char **argv)
            case 'i':
                set_config_entry(opts, CONF_PCAP_INTF, optarg);
                break;
+	    case '6':
+		opts->family = AF_INET6;


With that, fwknopd -6 authorises only IPv6. There should be a mode where both IPv4 and IPv6 are handled.

Seb35 · 2024-01-11T23:40:16Z

server/access.c

+    {
+        if(ip_list->family == AF_UNSPEC)
+            return 1;
+        if(ip_list->family != AF_INET6)


AF_INET here, isn’t it?

Seb35 · 2024-01-11T23:47:59Z

server/fw_util_iptables.c

 {
    int    exists = 0;

    if(have_ipt_chk_support == 1)
-        exists = jump_rule_exists_chk_support(opts, chain_num);
+        exists = jump_rule_exists_chk_support(opts, chain_num, ipv6);
    else
        exists = jump_rule_exists_no_chk_support(opts, chain_num);


The argument ipv6 is missing here, as well as the function definition a few lines above.

Adding the parameter ipv6 here + the ternary condition for fw_command6 in jump_rule_exists_no_chk_support solves my functional issue 1.
For some reason (I activated debug), I have "ipt_chk_support() -C not supported" for ipv4 and ipv6, although "iptables -C" does work but display the rule on stderr, perhaps it is interpreted as "does not work" by fwknop.

Seb35 · 2024-01-12T17:15:12Z

server/fw_util_iptables.c

+    /* XXX set adequately per SPA message */
+    int             ipv6 = (opts->family == AF_INET6) ? 1 : 0;
+


Something like

int ipv6 = 0; if(is_valid_ip_addr(spadat->use_src_ip, strlen(spadat->use_src_ip), AF_INET6)) { ipv6 = 1; }

works to distinguish IPv4 and IPv6 in the SPA message.
It was already checked in incoming_spa than the IP is valid with is_valid_ip_addr(…, AF_UNSPEC) so there is no security issue not to do the same check for IPv4.

With this change, both IPv4 and IPv6 are handled by fwknopd (which solves my functional issue 2).

Seb35 · 2024-01-15T10:13:05Z

The important changes are available here if you want to add them in this branch. With that, I was be able to make fwknop work both in IPv4 and IPv6 with two knock messages (1 per IP family); a further step will be to send the two IPs in a single message.

bam80 · 2024-01-15T10:31:17Z

@Seb35 thanks!
As you may have noticed the project does not payed proper attention from it's author nowadays.
Maybe you could take it over and maintain a fork, with unofficial builds there?

bastien-roucaries and others added 30 commits April 4, 2020 14:33

Use INET_ADDRSTRLEN instead of hardcoding it

6d65f1d

INET_ADDRSTRLEN is portable and allow to be robust against special crafted adress signed-off: Bastien Roucariès <rouca@debian.org>

Let IPs resolve using getaddrinfo

4c73258

getaddrinfo is the correct API for resolving IP and checking correctness Use it, instead of manual parsing

Factorize IP resolution between http and https

94bed23

Less code and less copy/paste thus less errors

Be more consistent when creating ~/.fwknoprc

03e1c20

Improve IPV4 min length by using strlen

de84936

Let IPs resolve to IPv6 addresses

2dc902b

Introduce IPV6 constant and IPV46 constant for adress length

Allow ipv6 for resolving url

5d8e6cf

Also catch the IPv6 version of INADDR_ANY

5a53b3b

Migrate is_valid_ipv4_addr() to a more generic function

8cc8908

It is now called is_valid_ip_addr() and expects an additional parameter for the address family.

Implement is_valid_ip_addr() with getaddrinfo()

2754326

Give have_allow_ip() a chance to allow IPv6

22aa677

This greatly loosens the check for a valid IPv4/IPv6 there - but it is redundant anyway, since there is always a call to is_valid_ip_addr().

Allow connecting to remote IPv6 hosts for TCP or UDP

d5ef047

This alone should allow interacting with IPv4 firewalling rules over IPv6, for these two protocols.

Use uppercase to log "IP"

a8aa02d

Also check for <netinet/ip6.h> and <netinet/icmp6.h>

cef18b7

This will allow porting the raw ICMP code to IPv6.

Rework the network listening routine

43dc125

Rework the network listening routine some more

508c9f3

Code cleanup

47f2309

Also include <netinet/ip6.h> where relevant

00a4914

Protect some more headers for inclusion

9b9af40

Add support for receiving SPA messages over IPv6

c69c44e

Interpret incoming addresses according to their family

73de567

Let access list stanzas be defined in IPv6

1dc74a1

Typo

b3ea4ab

Typo

43d1b4a

Constify

429ad67

Use a more appropriate type for sd_len

f2db271

Allow access control "ANY" with any protocol family

c489d83

Use AF_INET instead of PF_INET

65cbaf5

I believe it should be more portable this way, since AF_INET is required to be present in <sys/socket.h> in POSIX.

Use sizeof() instead of re-using hard-coded values

b846b35

This should help with portability for the protocol family eventually.

khorben and others added 25 commits April 4, 2020 17:14

Update a comment

5784487

Add preliminary support for IPv6 with iptables

7217e45

Code cleanup

ff4ea85

Fix a couple more uses of ctype(3)

6cbe8f3

Provide enough space to resolve IPv6 addresses

da6f1c9

Remove support for IPv6 for rules shared with IPv4

98b367a

Add support for rule expiration with IPv6

4369eea

Revert "Remove support for IPv6 for rules shared with IPv4"

c899fcb

This reverts commit d33c240.

Code cleanup

fc5d908

Default to IPv6 rules when listening on IPv6

a8c8a0d

Remove useless code

9ae09ed

Resolve hostnames to IPv6 addresses in IPv6 mode

2ff082c

Re-indent

1a020c6

Only support IPv4 on Windows

9633a26

Code cleanup

f1b8181

Use "::" for matching any address for IPv6 rules

d83e45a

No longer accept colon (":") as port separator

7929e3a

The correct syntax is the comma (",") instead.

Fix DNAT with IPv6

09b520b

Use "::/0" for IPT_ANY_IPV6

c3db6be

Port src_dst_check() to IPv6

0d28bae

The problem looks real, fix looks good to me, but have not been able to reproduce the actual issue so far :(

Also output the value obtained for ip6tables

a9ed5d6

This should address mrash#1.

Complete the check and search for ip6tables

26c1da6

Fixes mrash#1.

[test suite] start on IPv6 tests

040459d

[test suite] first working IPv6 complete cycle test

9c5069e

add IPv6 UDP server test, and IPv6 invalid destination test

2f98e06

bam80 mentioned this pull request Jun 6, 2023

Add IPv6 support to fwknop #285

Open

Seb35 suggested changes Jan 12, 2024

View reviewed changes

Seb35 reviewed Jan 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ipv6rebased #309

Ipv6rebased #309

bastien-roucaries commented Apr 4, 2020

Seb35 left a comment •

edited

Seb35 Jan 11, 2024

Seb35 Jan 11, 2024

Seb35 Jan 11, 2024

Seb35 Jan 11, 2024

Seb35 Jan 11, 2024

Seb35 Jan 12, 2024

Seb35 Jan 12, 2024

Seb35 commented Jan 15, 2024

bam80 commented Jan 15, 2024

		/* XXX set adequately per SPA message */
		int ipv6 = (opts->family == AF_INET6) ? 1 : 0;

Ipv6rebased #309

Are you sure you want to change the base?

Ipv6rebased #309

Conversation

bastien-roucaries commented Apr 4, 2020

Seb35 left a comment • edited

Choose a reason for hiding this comment

Seb35 Jan 11, 2024

Choose a reason for hiding this comment

Seb35 Jan 11, 2024

Choose a reason for hiding this comment

Seb35 Jan 11, 2024

Choose a reason for hiding this comment

Seb35 Jan 11, 2024

Choose a reason for hiding this comment

Seb35 Jan 11, 2024

Choose a reason for hiding this comment

Seb35 Jan 12, 2024

Choose a reason for hiding this comment

Seb35 Jan 12, 2024

Choose a reason for hiding this comment

Seb35 commented Jan 15, 2024

bam80 commented Jan 15, 2024

Seb35 left a comment •

edited