Skip to content

Commit

Permalink
pf: fix icmp-in-icmp state lookup
Browse files Browse the repository at this point in the history 
In 534ee17e6 pf state checking for ICMP(v6) was made stricter. This change
failed to correctly set the pf_pdesc for ICMP-in-ICMP lookups, resulting in ICMP
error packets potentially being dropped incorrectly.
Specially, it copied the ICMP header into a separate variable, not into the
pf_pdesc.

Populate the required pf_pdesc fields for the embedded ICMP packet's state lookup.

PR:		280701
MFC after:	1 week
Sponsored by:	Rubicon Communications, LLC ("Netgate")
  • Loading branch information
@kprovost @fichtner
kprovost authored and fichtner committed Aug 13, 2024
1 parent d10a84e commit 5c2b2da
Showing 1 changed file with 17 additions and 14 deletions.
31 changes: 17 additions & 14 deletions sys/netpfil/pf/pf.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7105,22 +7105,23 @@ pf_test_state_icmp(struct pf_kstate **state, struct pfi_kkif *kif,
}
#ifdef INET
case IPPROTO_ICMP: {
struct icmp iih;
struct icmp *iih = &pd2.hdr.icmp;

if (!pf_pull_hdr(m, off2, &iih, ICMP_MINLEN,
if (!pf_pull_hdr(m, off2, iih, ICMP_MINLEN,
NULL, reason, pd2.af)) {
DPFPRINTF(PF_DEBUG_MISC,
("pf: ICMP error message too short i"
"(icmp)\n"));
return (PF_DROP);
}

icmpid = iih.icmp_id;
pf_icmp_mapping(&pd2, iih.icmp_type,
icmpid = iih->icmp_id;
pf_icmp_mapping(&pd2, iih->icmp_type,
&icmp_dir, &multi, &virtual_id, &virtual_type);

pd2.dir = icmp_dir;
ret = pf_icmp_state_lookup(&key, &pd2, state, m,
pd->dir, kif, virtual_id, virtual_type,
pd2.dir, kif, virtual_id, virtual_type,
icmp_dir, &iidx, PF_ICMP_MULTI_NONE);
if (ret >= 0)
return (ret);
Expand All @@ -7134,10 +7135,10 @@ pf_test_state_icmp(struct pf_kstate **state, struct pfi_kkif *kif,
if (PF_ANEQ(pd2.src,
&nk->addr[pd2.sidx], pd2.af) ||
(virtual_type == htons(ICMP_ECHO) &&
nk->port[iidx] != iih.icmp_id))
nk->port[iidx] != iih->icmp_id))
pf_change_icmp(pd2.src,
(virtual_type == htons(ICMP_ECHO)) ?
&iih.icmp_id : NULL,
&iih->icmp_id : NULL,
daddr, &nk->addr[pd2.sidx],
(virtual_type == htons(ICMP_ECHO)) ?
nk->port[iidx] : 0, NULL,
Expand All @@ -7153,26 +7154,28 @@ pf_test_state_icmp(struct pf_kstate **state, struct pfi_kkif *kif,

m_copyback(m, off, ICMP_MINLEN, (caddr_t)&pd->hdr.icmp);
m_copyback(m, ipoff2, sizeof(h2), (caddr_t)&h2);
m_copyback(m, off2, ICMP_MINLEN, (caddr_t)&iih);
m_copyback(m, off2, ICMP_MINLEN, (caddr_t)iih);
}
return (PF_PASS);
break;
}
#endif /* INET */
#ifdef INET6
case IPPROTO_ICMPV6: {
struct icmp6_hdr iih;
struct icmp6_hdr *iih = &pd2.hdr.icmp6;

if (!pf_pull_hdr(m, off2, &iih,
if (!pf_pull_hdr(m, off2, iih,
sizeof(struct icmp6_hdr), NULL, reason, pd2.af)) {
DPFPRINTF(PF_DEBUG_MISC,
("pf: ICMP error message too short "
"(icmp6)\n"));
return (PF_DROP);
}

pf_icmp_mapping(&pd2, iih.icmp6_type,
pf_icmp_mapping(&pd2, iih->icmp6_type,
&icmp_dir, &multi, &virtual_id, &virtual_type);

pd2.dir = icmp_dir;
ret = pf_icmp_state_lookup(&key, &pd2, state, m,
pd->dir, kif, virtual_id, virtual_type,
icmp_dir, &iidx, PF_ICMP_MULTI_NONE);
Expand Down Expand Up @@ -7200,10 +7203,10 @@ pf_test_state_icmp(struct pf_kstate **state, struct pfi_kkif *kif,
if (PF_ANEQ(pd2.src,
&nk->addr[pd2.sidx], pd2.af) ||
((virtual_type == htons(ICMP6_ECHO_REQUEST)) &&
nk->port[pd2.sidx] != iih.icmp6_id))
nk->port[pd2.sidx] != iih->icmp6_id))
pf_change_icmp(pd2.src,
(virtual_type == htons(ICMP6_ECHO_REQUEST))
? &iih.icmp6_id : NULL,
? &iih->icmp6_id : NULL,
daddr, &nk->addr[pd2.sidx],
(virtual_type == htons(ICMP6_ECHO_REQUEST))
? nk->port[iidx] : 0, NULL,
Expand All @@ -7221,7 +7224,7 @@ pf_test_state_icmp(struct pf_kstate **state, struct pfi_kkif *kif,
(caddr_t)&pd->hdr.icmp6);
m_copyback(m, ipoff2, sizeof(h2_6), (caddr_t)&h2_6);
m_copyback(m, off2, sizeof(struct icmp6_hdr),
(caddr_t)&iih);
(caddr_t)iih);
}
return (PF_PASS);
break;
Expand Down

0 comments on commit 5c2b2da

Please sign in to comment.