From owner-svn-src-all@freebsd.org Sun Aug 11 23:54:50 2019 Return-Path: Delivered-To: svn-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 1024EC903F; Sun, 11 Aug 2019 23:54:50 +0000 (UTC) (envelope-from cy@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 466G6x6hJdz4bMh; Sun, 11 Aug 2019 23:54:49 +0000 (UTC) (envelope-from cy@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id C3C2E1E076; Sun, 11 Aug 2019 23:54:49 +0000 (UTC) (envelope-from cy@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id x7BNsnJ2081108; Sun, 11 Aug 2019 23:54:49 GMT (envelope-from cy@FreeBSD.org) Received: (from cy@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id x7BNsnWv081107; Sun, 11 Aug 2019 23:54:49 GMT (envelope-from cy@FreeBSD.org) Message-Id: <201908112354.x7BNsnWv081107@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: cy set sender to cy@FreeBSD.org using -f From: Cy Schubert Date: Sun, 11 Aug 2019 23:54:49 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r350880 - head/sys/contrib/ipfilter/netinet X-SVN-Group: head X-SVN-Commit-Author: cy X-SVN-Commit-Paths: head/sys/contrib/ipfilter/netinet X-SVN-Commit-Revision: 350880 X-SVN-Commit-Repository: base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 11 Aug 2019 23:54:50 -0000 Author: cy Date: Sun Aug 11 23:54:49 2019 New Revision: 350880 URL: https://svnweb.freebsd.org/changeset/base/350880 Log: r272552 applied the patch from ipfilter upstream fil.c r1.129 to fix broken ipfilter rule matches (upstream bug #554). The upstream patch was incomplete, it resolved all but one rule compare issue. The issue fixed here is when "{to, reply-to, dup-to} interface" are used in conjuncion with "on interface". The match was only made if the on keyword was specified in the same order in each case referencing the same rule. This commit fixes this. The reason for this is that interface name strings and comment keyword comments are stored in a a variable length field starting at fr_names in the frentry struct. These strings are placed into this variable length in the order they are encountered by ipf_y.y and indexed through index pointers in fr_ifnames, fr_comment or one of the frdest struct fd_name fields. (Three frdest structs are within frentry.) Order matters and this patch takes this into account. While in here it was discovered that though ipfilter is designed to support multiple interface specifiations per rule (up to four), this undocumented (the man page makes no mention of it) feature does not work. A todo is to fix the multiple interfaces feature at a later date. To understand the design decision as to why only four were intended, it is suspected that the decision was made because Sun workstations and PCs rarely if ever exceeded four NICs at the time, this is not true in 2019. PR: 238796 Reported by: WHR MFC after: 2 weeks Modified: head/sys/contrib/ipfilter/netinet/fil.c head/sys/contrib/ipfilter/netinet/ip_fil.h Modified: head/sys/contrib/ipfilter/netinet/fil.c ============================================================================== --- head/sys/contrib/ipfilter/netinet/fil.c Sun Aug 11 23:19:56 2019 (r350879) +++ head/sys/contrib/ipfilter/netinet/fil.c Sun Aug 11 23:54:49 2019 (r350880) @@ -4418,6 +4418,28 @@ ipf_matchicmpqueryreply(v, ic, icmp, rev) } +/* + * IFNAMES are located in the variable length field starting at + * frentry.fr_names. As pointers within the struct cannot be passed + * to the kernel from ipf(8), an offset is used. An offset of -1 means it + * is unused (invalid). If it is used (valid) it is an offset to the + * character string of an interface name or a comment. The following + * macros will assist those who follow to understand the code. + */ +#define IPF_IFNAME_VALID(_a) (_a != -1) +#define IPF_IFNAME_INVALID(_a) (_a == -1) +#define IPF_IFNAMES_DIFFERENT(_a) \ + !((IPF_IFNAME_INVALID(fr1->_a) && \ + IPF_IFNAME_INVALID(fr2->_a)) || \ + (IPF_IFNAME_VALID(fr1->_a) && \ + IPF_IFNAME_VALID(fr2->_a) && \ + !strcmp(FR_NAME(fr1, _a), FR_NAME(fr2, _a)))) +#define IPF_FRDEST_DIFFERENT(_a) \ + (memcmp(&fr1->_a.fd_addr, &fr2->_a.fd_addr, \ + offsetof(frdest_t, fd_name) - offsetof(frdest_t, fd_addr)) || \ + IPF_IFNAMES_DIFFERENT(_a.fd_name)) + + /* ------------------------------------------------------------------------ */ /* Function: ipf_rule_compare */ /* Parameters: fr1(I) - first rule structure to compare */ @@ -4430,22 +4452,50 @@ ipf_matchicmpqueryreply(v, ic, icmp, rev) static int ipf_rule_compare(frentry_t *fr1, frentry_t *fr2) { + int i; + if (fr1->fr_cksum != fr2->fr_cksum) return (1); if (fr1->fr_size != fr2->fr_size) return (2); if (fr1->fr_dsize != fr2->fr_dsize) return (3); - if (bcmp((char *)&fr1->fr_func, (char *)&fr2->fr_func, FR_CMPSIZ(fr1)) + if (bcmp((char *)&fr1->fr_func, (char *)&fr2->fr_func, FR_CMPSIZ) != 0) return (4); + /* + * XXX: There is still a bug here as different rules with the + * the same interfaces but in a different order will compare + * differently. But since multiple interfaces in a rule doesn't + * work anyway a simple straightforward compare is performed + * here. Ultimately frentry_t creation will need to be + * revisited in ipf_y.y. While the other issue, recognition + * of only the first interface in a list of interfaces will + * need to be separately addressed along with why only four. + */ + for (i = 0; i < FR_NUM(fr1->fr_ifnames); i++) { + /* + * XXX: It's either the same index or uninitialized. + * We assume this because multiple interfaces + * referenced by the same rule doesn't work anyway. + */ + if (IPF_IFNAMES_DIFFERENT(fr_ifnames[i])) + return(5); + } + + if (IPF_FRDEST_DIFFERENT(fr_tif)) + return (6); + if (IPF_FRDEST_DIFFERENT(fr_rif)) + return (7); + if (IPF_FRDEST_DIFFERENT(fr_dif)) + return (8); if (!fr1->fr_data && !fr2->fr_data) return (0); /* move along, nothing to see here */ if (fr1->fr_data && fr2->fr_data) { if (bcmp(fr1->fr_caddr, fr2->fr_caddr, fr1->fr_dsize) == 0) return (0); /* same */ } - return (5); + return (9); } Modified: head/sys/contrib/ipfilter/netinet/ip_fil.h ============================================================================== --- head/sys/contrib/ipfilter/netinet/ip_fil.h Sun Aug 11 23:19:56 2019 (r350879) +++ head/sys/contrib/ipfilter/netinet/ip_fil.h Sun Aug 11 23:54:49 2019 (r350880) @@ -735,12 +735,9 @@ typedef struct frentry { u_char fr_icode; /* return ICMP code */ int fr_group; /* group to which this rule belongs */ int fr_grhead; /* group # which this rule starts */ - int fr_ifnames[4]; int fr_isctag; int fr_rpc; /* XID Filtering */ ipftag_t fr_nattag; - frdest_t fr_tifs[2]; /* "to"/"reply-to" interface */ - frdest_t fr_dif; /* duplicate packet interface */ /* * These are all options related to stateful filtering */ @@ -750,6 +747,12 @@ typedef struct frentry { int fr_icmphead; /* ICMP group for state options */ u_int fr_age[2]; /* non-TCP state timeouts */ /* + * These are compared separately. + */ + int fr_ifnames[4]; + frdest_t fr_tifs[2]; /* "to"/"reply-to" interface */ + frdest_t fr_dif; /* duplicate packet interface */ + /* * How big is the name buffer at the end? */ int fr_namelen; @@ -827,9 +830,10 @@ typedef struct frentry { #define FR_NOLOGTAG 0 -#define FR_CMPSIZ(_f) ((_f)->fr_size - \ - offsetof(struct frentry, fr_func)) +#define FR_CMPSIZ (offsetof(struct frentry, fr_ifnames) - \ + offsetof(struct frentry, fr_func)) #define FR_NAME(_f, _n) (_f)->fr_names + (_f)->_n +#define FR_NUM(_a) (sizeof(_a) / sizeof(*_a)) /*