Page MenuHomeFreeBSD

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

Authored by neel_neelc.org on Mar 11 2020, 1:48 AM.

Details

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>

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

neel_neelc.org created this revision.Mar 11 2020, 1:48 AM
This revision is now accepted and ready to land.Mar 11 2020, 1:28 PM

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.

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?

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.

Here, I add the test. It passes.

This revision now requires review to proceed.Mar 14 2020, 5:05 PM

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

tests/sys/netpfil/common/ipfw_me4.sh
5

2020 ? :-)

46

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.

47

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.

50

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

58

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

73

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

?

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"?

neel_neelc.org edited the summary of this revision. (Show Details)

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.

sbin/ipfw/ipfw.8
1354–1358

Matches any IPv4 or IPv6 address ...

neel_neelc.org marked 5 inline comments as done.Mar 16 2020, 12:34 AM

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

neel_neelc.org retitled this revision from ipfw: Add me4 as an alias to me in add_src() and add_dst(). to ipfw: Add me4 as to refer to an host's IPv4 address in add_src() and add_dst()..Mar 16 2020, 12:37 AM
neel_neelc.org edited the summary of this revision. (Show Details)
neel_neelc.org marked 2 inline comments as done.Apr 11 2020, 1:58 AM

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.

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

neel_neelc.org marked an inline comment as done.Apr 11 2020, 4:13 AM

The fill_ip6() code is now in D24403,

Accidentally included a file from another patch. Sorry.

bcr accepted this revision as: manpages.May 12 2020, 7:08 AM
bcr added a subscriber: bcr.

OK from manpages.

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

sbin/ipfw/ipfw2.c
3722–3723

Where does this match to "me"?

3751–3752

Dito

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

neel_neelc.org marked 4 inline comments as done.May 12 2020, 7:06 PM

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.

lutz_donnerhacke.de requested changes to this revision.May 12 2020, 9:07 PM
lutz_donnerhacke.de added inline comments.
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..

This revision now requires changes to proceed.May 12 2020, 9:07 PM
neel_neelc.org marked an inline comment as done.May 13 2020, 12:05 AM

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.

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

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

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.

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.

This revision is now accepted and ready to land.May 13 2020, 9:04 AM