Date: Sun, 3 Aug 2025 09:53:04 GMT From: "Andrey V. Elsukov" <ae@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: 91ed876385d4 - main - ipfw: forbid adding keep-state rules that depend on tablearg Message-ID: <202508030953.5739r4Z0074967@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by ae: URL: https://cgit.FreeBSD.org/src/commit/?id=91ed876385d4531b6ab2f9176be969318e1aefc1 commit 91ed876385d4531b6ab2f9176be969318e1aefc1 Author: Andrey V. Elsukov <ae@FreeBSD.org> AuthorDate: 2025-07-22 08:02:17 +0000 Commit: Andrey V. Elsukov <ae@FreeBSD.org> CommitDate: 2025-08-03 09:46:51 +0000 ipfw: forbid adding keep-state rules that depend on tablearg tablearg value is determined after making table lookup. When we applying rule action that uses dynamic state, such lookup was not done and thus rule action can not determine what table and what value should be used as tablearg. To prevent this add check for such rules and return error when they are added. Obtained from: Yandex LLC Sponsored by: Yandex LLC Differential Revision: https://reviews.freebsd.org/D51458 --- sys/netpfil/ipfw/ip_fw_private.h | 1 + sys/netpfil/ipfw/ip_fw_sockopt.c | 31 +++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/sys/netpfil/ipfw/ip_fw_private.h b/sys/netpfil/ipfw/ip_fw_private.h index 79b3ed43f63b..c490d2849a7d 100644 --- a/sys/netpfil/ipfw/ip_fw_private.h +++ b/sys/netpfil/ipfw/ip_fw_private.h @@ -489,6 +489,7 @@ struct obj_idx { struct rule_check_info { uint16_t flags; /* rule-specific check flags */ +#define IPFW_RCIFLAG_HAS_STATE 0x0001 uint16_t object_opcodes; /* num of opcodes referencing objects */ uint16_t urule_numoff; /* offset of rulenum in bytes */ uint8_t version; /* rule version */ diff --git a/sys/netpfil/ipfw/ip_fw_sockopt.c b/sys/netpfil/ipfw/ip_fw_sockopt.c index 19f5fff2749a..5d57759ffb00 100644 --- a/sys/netpfil/ipfw/ip_fw_sockopt.c +++ b/sys/netpfil/ipfw/ip_fw_sockopt.c @@ -1311,6 +1311,9 @@ ipfw_check_rule(struct ip_fw_rule *rule, size_t size, return (check_ipfw_rule_body(rule->cmd, rule->cmd_len, ci)); } +#define CHECK_TARG(a, c) \ + ((a) == IP_FW_TARG && ((c)->flags & IPFW_RCIFLAG_HAS_STATE)) + enum ipfw_opcheck_result ipfw_check_opcode(ipfw_insn **pcmd, int *plen, struct rule_check_info *ci) { @@ -1326,6 +1329,7 @@ ipfw_check_opcode(ipfw_insn **pcmd, int *plen, struct rule_check_info *ci) if (cmdlen != F_INSN_SIZE(ipfw_insn_kidx)) return (BAD_SIZE); ci->object_opcodes++; + ci->flags |= IPFW_RCIFLAG_HAS_STATE; break; case O_PROTO: case O_IP_SRC_ME: @@ -1410,6 +1414,8 @@ ipfw_check_opcode(ipfw_insn **pcmd, int *plen, struct rule_check_info *ci) cmd->arg1 & 0x7FFF); return (FAILED); } + if (CHECK_TARG(cmd->arg1, ci)) + goto bad_targ; return (CHECK_ACTION); case O_UID: @@ -1518,11 +1524,16 @@ ipfw_check_opcode(ipfw_insn **pcmd, int *plen, struct rule_check_info *ci) case O_QUEUE: if (cmdlen != F_INSN_SIZE(ipfw_insn)) return (BAD_SIZE); + if (CHECK_TARG(cmd->arg1, ci)) + goto bad_targ; return (CHECK_ACTION); case O_FORWARD_IP: if (cmdlen != F_INSN_SIZE(ipfw_insn_sa)) return (BAD_SIZE); + if (insntoc(cmd, sa)->sa.sin_addr.s_addr == INADDR_ANY && + (ci->flags & IPFW_RCIFLAG_HAS_STATE)) + goto bad_targ; return (CHECK_ACTION); #ifdef INET6 case O_FORWARD_IP6: @@ -1537,6 +1548,8 @@ ipfw_check_opcode(ipfw_insn **pcmd, int *plen, struct rule_check_info *ci) return (FAILED); if (cmdlen != F_INSN_SIZE(ipfw_insn)) return (BAD_SIZE); + if (CHECK_TARG(cmd->arg1, ci)) + goto bad_targ; return (CHECK_ACTION); case O_NETGRAPH: case O_NGTEE: @@ -1544,12 +1557,16 @@ ipfw_check_opcode(ipfw_insn **pcmd, int *plen, struct rule_check_info *ci) return (FAILED); if (cmdlen != F_INSN_SIZE(ipfw_insn)) return (BAD_SIZE); + if (CHECK_TARG(cmd->arg1, ci)) + goto bad_targ; return (CHECK_ACTION); case O_NAT: if (!IPFW_NAT_LOADED) return (FAILED); if (cmdlen != F_INSN_SIZE(ipfw_insn_nat)) return (BAD_SIZE); + if (CHECK_TARG(cmd->arg1, ci)) + goto bad_targ; return (CHECK_ACTION); case O_SKIPTO: @@ -1557,6 +1574,11 @@ ipfw_check_opcode(ipfw_insn **pcmd, int *plen, struct rule_check_info *ci) case O_SETMARK: if (cmdlen != F_INSN_SIZE(ipfw_insn_u32)) return (BAD_SIZE); + /* O_CALLRETURN + F_NOT means 'return' opcode. */ + if (cmd->opcode != O_CALLRETURN || (cmd->len & F_NOT) == 0) { + if (CHECK_TARG(insntoc(cmd, u32)->d[0], ci)) + goto bad_targ; + } return (CHECK_ACTION); case O_CHECK_STATE: @@ -1577,6 +1599,8 @@ ipfw_check_opcode(ipfw_insn **pcmd, int *plen, struct rule_check_info *ci) case O_REASS: if (cmdlen != F_INSN_SIZE(ipfw_insn)) return (BAD_SIZE); + if (cmd->opcode == O_SETDSCP && CHECK_TARG(cmd->arg1, ci)) + goto bad_targ; return (CHECK_ACTION); #ifdef INET6 case O_IP6_SRC: @@ -1627,6 +1651,13 @@ ipfw_check_opcode(ipfw_insn **pcmd, int *plen, struct rule_check_info *ci) } } return (SUCCESS); +bad_targ: + /* + * For dynamic states we can not correctly initialize tablearg value, + * because we don't go through rule's opcodes except rule action. + */ + printf("ipfw: tablearg is not allowed with dynamic states\n"); + return (FAILED); } static __noinline int
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202508030953.5739r4Z0074967>