Bug 226429 - ipfw table add & hostname parsing
Summary: ipfw table add & hostname parsing
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 11.3-RELEASE
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-ipfw mailing list
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2018-03-07 18:55 UTC by bugs.freebsd.org
Modified: 2020-04-02 12:14 UTC (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description bugs.freebsd.org 2018-03-07 18:55:03 UTC
Example:
ipfw table 6 create type addr valtype ipv4

ipfw table 6 add localhost localhost
added: 127.0.0.1/32 1.0.0.127

ipfw table 6 list
--- table(6), set(0) ---
127.0.0.1/32 1.0.0.127

What is 1.0.0.127, why not 127.0.0.1?
Comment 1 Conrad Meyer freebsd_committer 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 freebsd_committer 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?
Comment 3 bugs.freebsd.org 2018-03-08 06:55:03 UTC
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 freebsd_committer 2018-03-08 17:23:37 UTC
Thanks for the report!
Comment 5 commit-hook freebsd_committer 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
Comment 6 bugs.freebsd.org 2020-04-02 12:14:50 UTC
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.