| Message ID | 20210610142316.24354-1-phil@nwl.cc |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nf-next] netfilter: nft_exthdr: Search chunks in SCTP packets only | expand |
Hi Phil, On Thu, Jun 10, 2021 at 04:23:16PM +0200, Phil Sutter wrote: > Since user space does not generate a payload dependency, plain sctp > chunk matches cause searching in non-SCTP packets, too. Avoid this > potential mis-interpretation of packet data by checking pkt->tprot. > > Fixes: 133dc203d77df ("netfilter: nft_exthdr: Support SCTP chunks") > Signed-off-by: Phil Sutter <phil@nwl.cc> > --- > net/netfilter/nft_exthdr.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/net/netfilter/nft_exthdr.c b/net/netfilter/nft_exthdr.c > index 7f705b5c09de8..1093bb83f8aeb 100644 > --- a/net/netfilter/nft_exthdr.c > +++ b/net/netfilter/nft_exthdr.c > @@ -312,6 +312,9 @@ static void nft_exthdr_sctp_eval(const struct nft_expr *expr, > const struct sctp_chunkhdr *sch; > struct sctp_chunkhdr _sch; > > + if (!pkt->tprot_set || pkt->tprot != IPPROTO_SCTP) > + goto err; nft_set_pktinfo_unspec() already initializes pkt->tprot to zero. I think it's safe to simplify this to: if (pkt->tprot != IPPROTO_SCTP)
On Thu, Jun 10, 2021 at 07:43:34PM +0200, Pablo Neira Ayuso wrote: > On Thu, Jun 10, 2021 at 04:23:16PM +0200, Phil Sutter wrote: > > Since user space does not generate a payload dependency, plain sctp > > chunk matches cause searching in non-SCTP packets, too. Avoid this > > potential mis-interpretation of packet data by checking pkt->tprot. > > > > Fixes: 133dc203d77df ("netfilter: nft_exthdr: Support SCTP chunks") > > Signed-off-by: Phil Sutter <phil@nwl.cc> > > --- > > net/netfilter/nft_exthdr.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/net/netfilter/nft_exthdr.c b/net/netfilter/nft_exthdr.c > > index 7f705b5c09de8..1093bb83f8aeb 100644 > > --- a/net/netfilter/nft_exthdr.c > > +++ b/net/netfilter/nft_exthdr.c > > @@ -312,6 +312,9 @@ static void nft_exthdr_sctp_eval(const struct nft_expr *expr, > > const struct sctp_chunkhdr *sch; > > struct sctp_chunkhdr _sch; > > > > + if (!pkt->tprot_set || pkt->tprot != IPPROTO_SCTP) > > + goto err; > > nft_set_pktinfo_unspec() already initializes pkt->tprot to zero. > > I think it's safe to simplify this to: > > if (pkt->tprot != IPPROTO_SCTP) Are you sure? Checking the spots that (should) initialize tprot/tprot_set, in nft_do_chain_inet() it seems that if state->pf is neither NFPROTO_IPV4 nor NFPROTO_IPV6, nft_do_chain() is called without prior init. Maybe default case should call nft_set_pktinfo_unspec()? BTW: The final return call in nft_do_chain_inet_ingress() is dead code, right? Thanks, Phil
On Fri, Jun 11, 2021 at 12:26:24PM +0200, Phil Sutter wrote: > On Thu, Jun 10, 2021 at 07:43:34PM +0200, Pablo Neira Ayuso wrote: > > On Thu, Jun 10, 2021 at 04:23:16PM +0200, Phil Sutter wrote: > > > Since user space does not generate a payload dependency, plain sctp > > > chunk matches cause searching in non-SCTP packets, too. Avoid this > > > potential mis-interpretation of packet data by checking pkt->tprot. > > > > > > Fixes: 133dc203d77df ("netfilter: nft_exthdr: Support SCTP chunks") > > > Signed-off-by: Phil Sutter <phil@nwl.cc> > > > --- > > > net/netfilter/nft_exthdr.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/net/netfilter/nft_exthdr.c b/net/netfilter/nft_exthdr.c > > > index 7f705b5c09de8..1093bb83f8aeb 100644 > > > --- a/net/netfilter/nft_exthdr.c > > > +++ b/net/netfilter/nft_exthdr.c > > > @@ -312,6 +312,9 @@ static void nft_exthdr_sctp_eval(const struct nft_expr *expr, > > > const struct sctp_chunkhdr *sch; > > > struct sctp_chunkhdr _sch; > > > > > > + if (!pkt->tprot_set || pkt->tprot != IPPROTO_SCTP) > > > + goto err; > > > > nft_set_pktinfo_unspec() already initializes pkt->tprot to zero. > > > > I think it's safe to simplify this to: > > > > if (pkt->tprot != IPPROTO_SCTP) > > Are you sure? Checking the spots that (should) initialize > tprot/tprot_set, in nft_do_chain_inet() it seems that if state->pf is > neither NFPROTO_IPV4 nor NFPROTO_IPV6, nft_do_chain() is called without > prior init. Maybe default case should call nft_set_pktinfo_unspec()? state->pf in nft_do_chain_inet() can only be either NFPROTO_IPV4 or NFPROTO_IPV6. pkt->tprot_set is there to deal with a corner case: IPPROTO_IP (0). If pkt->tprot_set == true and pkt->tprot == 0, it means: "match on IPPROTO_IP". For other IPPROTO_*, checking pkt->tprot looks safe to me. > BTW: The final return call in nft_do_chain_inet_ingress() is dead code, > right? You mean the default case of nft_do_chain_inet_ingress()? inet/ingress is special, it allows you to filter IPv4 and IPv6 traffic only. Anything else from ingress is accepted (you should filter it via netdev family).
On Fri, Jun 11, 2021 at 01:56:54PM +0200, Pablo Neira Ayuso wrote: > On Fri, Jun 11, 2021 at 12:26:24PM +0200, Phil Sutter wrote: > > On Thu, Jun 10, 2021 at 07:43:34PM +0200, Pablo Neira Ayuso wrote: > > > On Thu, Jun 10, 2021 at 04:23:16PM +0200, Phil Sutter wrote: > > > > Since user space does not generate a payload dependency, plain sctp > > > > chunk matches cause searching in non-SCTP packets, too. Avoid this > > > > potential mis-interpretation of packet data by checking pkt->tprot. > > > > > > > > Fixes: 133dc203d77df ("netfilter: nft_exthdr: Support SCTP chunks") > > > > Signed-off-by: Phil Sutter <phil@nwl.cc> > > > > --- > > > > net/netfilter/nft_exthdr.c | 5 ++++- > > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/net/netfilter/nft_exthdr.c b/net/netfilter/nft_exthdr.c > > > > index 7f705b5c09de8..1093bb83f8aeb 100644 > > > > --- a/net/netfilter/nft_exthdr.c > > > > +++ b/net/netfilter/nft_exthdr.c > > > > @@ -312,6 +312,9 @@ static void nft_exthdr_sctp_eval(const struct nft_expr *expr, > > > > const struct sctp_chunkhdr *sch; > > > > struct sctp_chunkhdr _sch; > > > > > > > > + if (!pkt->tprot_set || pkt->tprot != IPPROTO_SCTP) > > > > + goto err; > > > > > > nft_set_pktinfo_unspec() already initializes pkt->tprot to zero. > > > > > > I think it's safe to simplify this to: > > > > > > if (pkt->tprot != IPPROTO_SCTP) > > > > Are you sure? Checking the spots that (should) initialize > > tprot/tprot_set, in nft_do_chain_inet() it seems that if state->pf is > > neither NFPROTO_IPV4 nor NFPROTO_IPV6, nft_do_chain() is called without > > prior init. Maybe default case should call nft_set_pktinfo_unspec()? > > state->pf in nft_do_chain_inet() can only be either NFPROTO_IPV4 or > NFPROTO_IPV6. Shouldn't there be a WARN_ON_ONCE or something in the default case then? Looking at nf_hook(), it seems entirely possible to me that state->pf might be NFPROTO_ARP, for instance. That's probably just me not getting it, but things we rely upon shouldn't be hidden that well, right? > pkt->tprot_set is there to deal with a corner case: IPPROTO_IP (0). > If pkt->tprot_set == true and pkt->tprot == 0, it means: "match on > IPPROTO_IP". For other IPPROTO_*, checking pkt->tprot looks safe to me. Ah, thanks for clarifying! So whenever I check a specific value that's non-zero, tprot_set doesn't matter. Should I send a patch for the same change in nft_tcp_header_pointer(), too? (That's where I copied the code from. ;) > > BTW: The final return call in nft_do_chain_inet_ingress() is dead code, > > right? > > You mean the default case of nft_do_chain_inet_ingress()? inet/ingress > is special, it allows you to filter IPv4 and IPv6 traffic only. > Anything else from ingress is accepted (you should filter it via > netdev family). Oh, sorry. Looks like I had tomatoes on the eyes[1]: I missed that the non-default cases just 'break' and therefore hit the function's last line. Thanks, Phil [1] Famous German idiom.
On Fri, Jun 11, 2021 at 02:52:48PM +0200, Phil Sutter wrote: > On Fri, Jun 11, 2021 at 01:56:54PM +0200, Pablo Neira Ayuso wrote: > > On Fri, Jun 11, 2021 at 12:26:24PM +0200, Phil Sutter wrote: > > > On Thu, Jun 10, 2021 at 07:43:34PM +0200, Pablo Neira Ayuso wrote: > > > > On Thu, Jun 10, 2021 at 04:23:16PM +0200, Phil Sutter wrote: > > > > > Since user space does not generate a payload dependency, plain sctp > > > > > chunk matches cause searching in non-SCTP packets, too. Avoid this > > > > > potential mis-interpretation of packet data by checking pkt->tprot. > > > > > > > > > > Fixes: 133dc203d77df ("netfilter: nft_exthdr: Support SCTP chunks") > > > > > Signed-off-by: Phil Sutter <phil@nwl.cc> > > > > > --- > > > > > net/netfilter/nft_exthdr.c | 5 ++++- > > > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/net/netfilter/nft_exthdr.c b/net/netfilter/nft_exthdr.c > > > > > index 7f705b5c09de8..1093bb83f8aeb 100644 > > > > > --- a/net/netfilter/nft_exthdr.c > > > > > +++ b/net/netfilter/nft_exthdr.c > > > > > @@ -312,6 +312,9 @@ static void nft_exthdr_sctp_eval(const struct nft_expr *expr, > > > > > const struct sctp_chunkhdr *sch; > > > > > struct sctp_chunkhdr _sch; > > > > > > > > > > + if (!pkt->tprot_set || pkt->tprot != IPPROTO_SCTP) > > > > > + goto err; > > > > > > > > nft_set_pktinfo_unspec() already initializes pkt->tprot to zero. > > > > > > > > I think it's safe to simplify this to: > > > > > > > > if (pkt->tprot != IPPROTO_SCTP) > > > > > > Are you sure? Checking the spots that (should) initialize > > > tprot/tprot_set, in nft_do_chain_inet() it seems that if state->pf is > > > neither NFPROTO_IPV4 nor NFPROTO_IPV6, nft_do_chain() is called without > > > prior init. Maybe default case should call nft_set_pktinfo_unspec()? > > > > state->pf in nft_do_chain_inet() can only be either NFPROTO_IPV4 or > > NFPROTO_IPV6. > > Shouldn't there be a WARN_ON_ONCE or something in the default case then? > Looking at nf_hook(), it seems entirely possible to me that state->pf > might be NFPROTO_ARP, for instance. That's probably just me not getting > it, but things we rely upon shouldn't be hidden that well, right? nft_do_chain_inet() is called from the NFPROTO_INET hook, which results in either NFPROTO_IPV4 or NFPROTO_IPV6. This is hot path, I would not add more code there. The default case is just there to avoid a warning from gcc. Probably a comment like /* Should not ever happen */ for the default case in nft_do_chain_inet() is fine with you? :) > > pkt->tprot_set is there to deal with a corner case: IPPROTO_IP (0). > > If pkt->tprot_set == true and pkt->tprot == 0, it means: "match on > > IPPROTO_IP". For other IPPROTO_*, checking pkt->tprot looks safe to me. > > Ah, thanks for clarifying! So whenever I check a specific value that's > non-zero, tprot_set doesn't matter. Should I send a patch for the same > change in nft_tcp_header_pointer(), too? (That's where I copied the code > from. ;) I think so, that's fine indeed.
diff --git a/net/netfilter/nft_exthdr.c b/net/netfilter/nft_exthdr.c index 7f705b5c09de8..1093bb83f8aeb 100644 --- a/net/netfilter/nft_exthdr.c +++ b/net/netfilter/nft_exthdr.c @@ -312,6 +312,9 @@ static void nft_exthdr_sctp_eval(const struct nft_expr *expr, const struct sctp_chunkhdr *sch; struct sctp_chunkhdr _sch; + if (!pkt->tprot_set || pkt->tprot != IPPROTO_SCTP) + goto err; + do { sch = skb_header_pointer(pkt->skb, offset, sizeof(_sch), &_sch); if (!sch || !sch->length) @@ -334,7 +337,7 @@ static void nft_exthdr_sctp_eval(const struct nft_expr *expr, } offset += SCTP_PAD4(ntohs(sch->length)); } while (offset < pkt->skb->len); - +err: if (priv->flags & NFT_EXTHDR_F_PRESENT) nft_reg_store8(dest, false); else
Since user space does not generate a payload dependency, plain sctp chunk matches cause searching in non-SCTP packets, too. Avoid this potential mis-interpretation of packet data by checking pkt->tprot. Fixes: 133dc203d77df ("netfilter: nft_exthdr: Support SCTP chunks") Signed-off-by: Phil Sutter <phil@nwl.cc> --- net/netfilter/nft_exthdr.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)