Bug 226429

Comment 1 Conrad Meyer 2018-03-08 04:20:22 UTC

Clearly the endianness of the address is being reversed somewhere.

In ipfw/tables.c, the v->nh4 value is assumed to be host endian.  It is then converted to net endian and formatted.

However, tentry_fill_value()'s DNS resolution code is just broken for ipv4 values.  It bogusly casts the (uint32 *) &v->nh4 to (struct in_addr *) and passes it off to lookup_host().  lookup_host() uses gethostbyname()->h_addr_list[0] to assign to the passed in struct in_addr*.  h_addr_list is a network byte order value -- so at this point, it should have been converted to host order instead.

Comment 2 Conrad Meyer 2018-03-08 04:27:01 UTC

I suggest this untested patch to correct the issue (and a similar issue with legacy table values):

--- sbin/ipfw/tables.c    (revision 330627)
+++ sbin/ipfw/tables.c    (working copy)
@@ -1471,6 +1471,7 @@
        uint32_t i;
        int dval;
        char *comma, *e, *etype, *n, *p;
+       struct in_addr ipaddr;

        v = &tent->v.value;

@@ -1487,8 +1488,8 @@
                        return;
                }
                /* Try hostname */
-               if (lookup_host(arg, (struct in_addr *)&val) == 0) {
-                       set_legacy_value(val, v);
+               if (lookup_host(arg, &ipaddr) == 0) {
+                       set_legacy_value(ntohl(ipaddr.s_addr), v);
                        return;
                }
                errx(EX_OSERR, "Unable to parse value %s", arg);
@@ -1557,8 +1558,10 @@
                                v->nh4 = ntohl(a4);
                                break;
                        }
-                       if (lookup_host(n, (struct in_addr *)&v->nh4) == 0)
+                       if (lookup_host(n, &ipaddr) == 0) {
+                               v->nh4 = ntohl(ipaddr.s_addr);
                                break;
+                       }
                        etype = "ipv4";
                        break;
                case IPFW_VTYPE_DSCP:

Maybe you can try it out?

ipfw table 6 create type addr valtype ipv4
ipfw table 6 add localhost localhost
added: 127.0.0.1/32 127.0.0.1
ipfw table 6 list
--- table(6), set(0) ---
127.0.0.1/32 127.0.0.1

Now it works right.
Thanks you!

Comment 4 Conrad Meyer 2018-03-08 17:23:37 UTC

Thanks for the report!

Comment 5 commit-hook 2018-03-08 17:23:51 UTC

A commit references this bug:

Author: cem
Date: Thu Mar  8 17:23:19 UTC 2018
New revision: 330665
URL: https://svnweb.freebsd.org/changeset/base/330665

Log:
  ipfw(8): Fix endianness for Legacy and Ipv4 table hostname values

  The lookup_host() helper subroutine emits a struct in_addr value in network
  byte order via caller passed pointer.  However, the table value is expected
  to be stored in host byte order.  On little-endian machines, this produced a
  reversed endian table value for Legacy or IPv4 table types when the value
  was a hostname (instead of a plain IP address).

  Fix by using ntohl() on the output 32-bit address.

  While here, avoid some aliasing violations by storing the lookup_host()
  output in an intermediate object of the correct type.

  PR:		226429
  Reported by:	bugs.freebsd.org AT mx.zzux.com (also: Tested by)
  Security:	ipfw hostname table rules could potentially not act as admin intended
  Sponsored by:	Dell EMC Isilon

Changes:
  head/sbin/ipfw/tables.c

On 11.3 the same bug again.

Example from ipfw man page will not work as expected.

 Using the fwd action, the table entries may include hostnames and IP
     addresses.

           ipfw table T2 create type addr ftype ip
           ipfw table T2 add 192.168.2.0/24 10.23.2.1
>>>>>>>>>> ipfw table T21 add 192.168.0.0/27 router1.dmz
           ...
           ipfw add 100 fwd tablearg ip from any to table(1)


The patch above works on 11.3 also.

Comment 7 Piotr Pawel Stefaniak 2022-04-21 17:59:39 UTC

The original commit only made it to 12-release. It is now included in all supported branches.