ipfw: Add me4 as to refer to an host's IPv4 address in add_src() and add_dst().

[nc]

Summary

ipfw: Add "me4" to refer to the host's IPv4 address only in add_src() and add_dst().

Also, don't assume that !IPv6 is IPv4.

Submitted by: Neel Chauhan <neel AT neelc DOT org>

[donner]

Good catch.

[driesm.michiels_gmail.com]

Does this mean that for a current dual stack IPFW rule like:

allow tcp from any to me 443

It will only match for IPv4 packets, as "me" is only working with IPv4 addresses under the hood with the current behavior?
This is not the current behavior I'm observing since my web server answers IPv6 requests perfectly fine with my above rule.

I would really be a backer to fixing this properly so that "me" both matches "me4" and "me6" address families.

[donner]

In D24021#528343, @driesm.michiels_gmail.com wrote:

Does this mean that for a current dual stack IPFW rule like:

allow tcp from any to me 443

It will only match for IPv4 packets, as "me" is only working with IPv4 addresses under the hood with the current behavior?
This is not the current behavior I'm observing since my web server answers IPv6 requests perfectly fine with my above rule.

May you please provide the full ruleset, please?

[driesm.michiels_gmail.com]

This comment was removed by driesm.michiels_gmail.com.

[melifaro]

Adding me4 as an explicit ipv4 keyword is a good idea.
Could you please add automated tests to the patch, to ensure this functionality is always tested? It should be easily achieved with vnet jail & testing local exit codes of ping/other similar utilities.

[nc]

Here, I add the test. It passes.

[melifaro]

Thank you! Looks good, please see some comments inline.

tests/sys/netpfil/common/ipfw_me4.sh

4

2020 ? :-)

45

I'd suggest creating another jail for this end, to avoid using 192.0.2.0/24 in the host system and allow this tests running in parallel with other tests using this network.

46

Better to use something like ${jname} as jail identifier. Also: it would be better to have the jail name somewhat consistent with the test name, as in the future we will certainly have more ipfw(8) tests.

49

Maybe it's worth having simple setup with allow+deny rule and not having depends on rc.firewall functionality?

57

It would be better if you make the positive and negative cases into separate tests.

72

Could you also add the tests, verifying that:

• me4 does NOT match non-local IPv4 address (probably can rely on the fact that firewalling local outbound connection results in EPERM for the deny rule)
• me4 does NOT match IPv6 traffic

?

[driesm.michiels_gmail.com]

In D24021#529141, @melifaro wrote:

Adding me4 as an explicit ipv4 keyword is a good idea.

As long as it doesn't break existing rulesets :-). After this patch, will "me" still match both "me4" and "me6"?

[nc]

Here is the updated diff.

Here are the few changes:

• I have made the requested changes
• I have also combined the D21812 efforts into here.
• I made "me" be for IPv4 and IPv6 (as it is today), and "me4" be for IPv4-only
• I have added the additional unit tests for the "me4", and they pass

I am still using rc.firewall in the tests, primarily so I don't mess around with configuring the firewall myself.

[donner]

sbin/ipfw/ipfw.8

1354–1358

Matches any IPv4 or IPv6 address ...

[nc]

Clarify the "me" in the man page to say it's any IPv4 or IPv6 address.

[rgrimes]

I do not see how this makes any difference between the values me, me4 and me6. There still only appears to be one class of opcode, O_IP_{SRC,DST}_ME. Is that the intent?

sbin/ipfw/ipv6.c

351 ↗

(On Diff #69560)

Something seems wrong here, we are processing a me4 command inside an ipv6 only fill command?

Also why is this done as 3 if statements? Seems that:
if (strcmp(av, "me") == 0 || strcmp(av,"me6") ==0)
is the right way to do this.

[nc]

Makes complete sense. I just consolidated into one "if" statement, and removed the IPv4 code from ipv6.c.

[nc]

The fill_ip6() code is now in D24403,

[nc]

Accidentally included a file from another patch. Sorry.

[bcr]

OK from manpages.

[donner]

sbin/ipfw/ipfw2.c

3722–3731

Simplify the code to immediate return.

if (proto == IPPROTO_IPV6 || ...)

return add_srcip6(...);

if (proto == IPPROTO_IP || ...)

return add_srcip(...);

...

3751–3760

Dito

[donner]

sbin/ipfw/ipfw2.c

3722–3723

Where does this match to "me"?

3751–3752

Dito

[nc]

Thanks for the suggestions, Lutz. I also removed an unneeded variable.

[nc]

Note: I did some testing and the patch does not work as intended. "me4" also blocks IPv6, so I'm rewriting it.

sbin/ipfw/ipfw2.c

3722–3723

This happens in the next "if" statement, but also in fill_ip6() in ipv6.c.

[donner]

sbin/ipfw/ipfw2.c

3722–3723

So "me" does not match in IPv6 statements?

ipfw allow ip6 from any to me

We have to use "me6"?

ipfw allow ip6 from any to me6

This contradicts the description in the man page..

[nc]

What I meant is that "me4" also takes action on IPv6, it does not "block" it. The "block" means I tested with "deny".

I was in a rush to write a comment, sorry.

I have a patch which fixes this, but it uses new kernel opcodes. It will be posted soon.

[nc]

In this patch, "me4" is IPv4-only and "me" is dual-stack. It uses kernel opcodes, however.

[rgrimes]

In D24021#546333, @neel_neelc.org wrote:

In this patch, "me4" is IPv4-only and "me" is dual-stack. It uses kernel opcodes, however.

Please go read my comment #1. I'll review this if I get time, but this is what is needed as far as opcodes to make me4 and me6 different

[nc]

In D24021#546343, @rgrimes wrote:

In D24021#546333, @neel_neelc.org wrote:

In this patch, "me4" is IPv4-only and "me" is dual-stack. It uses kernel opcodes, however.

Please go read my comment #1. I'll review this if I get time, but this is what is needed as far as opcodes to make me4 and me6 different

O_IP_SRC_ME/O_IP_DST_ME is dual-stack.

In sys/netpfil/ipfw/ip_fw2.c, O_IP_SRC_ME/O_IP_DST_ME currently falls down to the IPv6 case. My new opcodes O_IP_SRC_ME4/O_IP_DST_ME4 prevent it from doing so for the new ones, while existing O_IP_SRC_ME/O_IP_DST_ME can function normally.

In the said file, there is a variable is_ipv4 which skips IPv6, but didn't know how to get there from ipfw(8) so that's the reason I added opcodes.

[donner]

In D24021#546333, @neel_neelc.org wrote:

In this patch, "me4" is IPv4-only and "me" is dual-stack. It uses kernel opcodes, however.

Ah, if you can leave a comment at those lines to guide the innocent reader, I'm fine.

[kp]

It appears that this needs to be rebased. The current patch doesn't apply on latest main.

tests/sys/netpfil/common/Makefile

14

Given that it's specific to ipfw it may make sense to introduce a tests/sys/netpfil/ipfw directory, similar to the pf one we already have, and reserve the common directory for tests that apply to at least two of our firewalls.

[jhb]

sys/netinet/ip_fw.h

172

You have to add these at the end of the enum to avoid breaking the ABI. I can't decide if O_IP4_SRC_ME would be better to match the v6-only name, though other IPv4-specific opcodes use OP_IP_*.