Page MenuHomeFreeBSD

ipfw: Support [w:x:y::z]:port (bracketed) IPv6 addresses in the fwd command
ClosedPublic

Authored by nc on Mar 10 2020, 3:57 AM.
Referenced Files
Unknown Object (File)
Fri, Mar 22, 9:51 PM
Unknown Object (File)
Fri, Mar 22, 9:51 PM
Unknown Object (File)
Fri, Mar 22, 9:51 PM
Unknown Object (File)
Fri, Mar 22, 9:51 PM
Unknown Object (File)
Fri, Mar 22, 9:51 PM
Unknown Object (File)
Fri, Mar 22, 9:51 PM
Unknown Object (File)
Sat, Mar 9, 2:29 PM
Unknown Object (File)
Jan 5 2024, 11:34 PM

Details

Summary

ipfw: Support {w:x:y::z}:port (bracketed) IPv6 addresses in the fwd command.

While we're doing this, optimize by not attempting to parse addresses if we're using tableargs.

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

Test Plan

Try compiling HEAD with this patch (or at least /usr/src/sbin/ipfw) and then run the following command:

ipfw add fwd "[::1]:3128" tcp from any to any

It should work

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

How about detecting the port separator first? (i.e. repeatly call strpbrk)
Then you can easily distinguish between the cases

  • starts with '[' -> numeric IPb6
  • contains ':' -> numeric IPv6
  • contains no letters -> numeric IPv4
  • use gai()
In D24011#527998, @lutz_donnerhacke.de wrote:

How about detecting the port separator first? (i.e. repeatly call strpbrk)

getaddrinfo let you simplify this even more:

  • strchr(',') and if found, you have the addr/port separated.
  • otherwise strrchr(':') and if found, separation done
  • if first char '[', check and remove last char ']', set numeric flag
  • call getaddrinfo with both pieces

Do not try to parse things, which are already handled by gai.

Your IPv4 suggestion is wrong. There can be IPv6 addresses without letters, like 2001:470:20::2, or 2600::1.

Also, your suggestions would require rewriting of the IP parsing code I modify, so I'd prefer not to do this unless necessary.

However, it's possible with my code that you can do IPv4 in brackets, like [1.2.3.4]:56789. I don't believe this causes any harm, but if it does I'm open to modifications or even the "rewrite" I mentioned earlier.

That would be my approach https://reviews.freebsd.org/differential/diff/69565/
I'd further eliminate the temporary storage "struct sockaddr_storage result", and copy directly from the gai result into the action (with memcpy).

Your approach looks good, reducing memory consumption.

Adding bz, as it is his XXX-BZ comment being removed about implementing this.

sbin/ipfw/ipfw2.c
4042

So technically I can not have a hostnamed tablearg?

4046

I would think all of the above attempt at parsing an IP address belongs inside this else block as none of it is needed in the first half of this if.

4051

Magic pointer arithmetic? v6brac is 0 or 1 when we get here, so probably some place an *av++ should of happened to indicate we have swallowed a token?

nc retitled this revision from ipfw: Support {w:x:y::z}:port (bracketed) IPv6 addresses in the fwd command to ipfw: Support [w:x:y::z]:port (bracketed) IPv6 addresses in the fwd command.
nc edited the summary of this revision. (Show Details)

Here, I resolve two of three of rgrimes' suggestions.

About the hostnamed tablearg, I believe it should stay the same (CLARIFICATION: as it is today) but did not test this.

That reads much better, cleaned it up a fair bit more than I thought it would.

This revision is now accepted and ready to land.Apr 28 2020, 4:23 AM

I'm not a committer (I'm also not neel@), could someone please commit this?

ipfw2.c
3992 ↗(On Diff #71094)

Hmm, is this standard syntax for IPv4 addresses? I have never seen it.

4040 ↗(On Diff #71094)

Per style(9), casts are not followed by a space.

This revision now requires review to proceed.Jun 24 2020, 5:54 PM
nc marked 3 inline comments as done.Jun 24 2020, 5:54 PM
ipfw2.c
3992 ↗(On Diff #71094)

No, but a canonical extension.

This looks ok to me modulo the nits I pointed out.

sbin/ipfw/ipfw2.c
4006

style(9) discourages combined declarations and initializations.

4058

Similarly here. ptr also doesn't seem like a great name to me. I think you could simply do without this variable:

s = strchr(*av, ']');
if (s != NULL) {
    *(s++) = '\0';
} else {
    s = *av;
}
s = strchr(s, ':');
...
This revision is now accepted and ready to land.Jun 25 2020, 3:48 PM
nc marked 2 inline comments as done.

Good catch on the tidbits, here's an revised patch.

This revision now requires review to proceed.Jun 25 2020, 4:11 PM
This revision is now accepted and ready to land.Jun 25 2020, 6:02 PM