Page MenuHomeFreeBSD

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

Authored by nc on Mar 11 2020, 1:48 AM.
Referenced Files
F80106401: D24021.diff
Thu, Mar 28, 12:19 AM
Unknown Object (File)
Thu, Mar 14, 10:17 PM
Unknown Object (File)
Wed, Mar 13, 3:45 AM
Unknown Object (File)
Feb 22 2024, 2:39 PM
Unknown Object (File)
Feb 16 2024, 11:31 AM
Unknown Object (File)
Feb 10 2024, 12:00 AM
Unknown Object (File)
Feb 6 2024, 7:25 AM
Unknown Object (File)
Jan 23 2024, 8:10 AM

Details

Reviewers
melifaro
ae
donner
rgrimes
Group Reviewers
manpages
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 - subversion
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

donner added a subscriber: donner.

Good catch.

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.

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?

This comment was removed by driesm.

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

nc 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 ...

nc 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.

nc 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
nc edited the summary of this revision. (Show Details)
nc 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.

nc 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 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.

nc 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.

donner requested changes to this revision.May 12 2020, 9:07 PM
donner 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
nc 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 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

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.

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.

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

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 added inline comments.
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_*.