| Message ID | f156034b4975584477c319a0946eb7adfc8d1e36.1495119678.git.dcaratti@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
Hi Davide, On Thu, May 18, 2017 at 06:01:43PM +0200, Davide Caratti wrote: > sctp_compute_cksum() implementation assumes that at least the SCTP header > is in the linear part of skb: modify conntrack error callback to avoid > false CRC32c mismatch, if the transport header is partially/entirely paged. I guess you considered this, but I would like to know the reason for this approach. Why not fix this from sctp_compute_cksum()? I mean, I can see other spots in the kernel tree that may be affected by this? Or is it that you're only observing this from a path that is specific of conntrack? > Signed-off-by: Davide Caratti <dcaratti@redhat.com> > --- > net/netfilter/nf_conntrack_proto_sctp.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/net/netfilter/nf_conntrack_proto_sctp.c b/net/netfilter/nf_conntrack_proto_sctp.c > index 13875d5..1c5b14a 100644 > --- a/net/netfilter/nf_conntrack_proto_sctp.c > +++ b/net/netfilter/nf_conntrack_proto_sctp.c > @@ -512,16 +512,19 @@ static int sctp_error(struct net *net, struct nf_conn *tpl, struct sk_buff *skb, > u8 pf, unsigned int hooknum) > { > const struct sctphdr *sh; > - struct sctphdr _sctph; > const char *logmsg; > > - sh = skb_header_pointer(skb, dataoff, sizeof(_sctph), &_sctph); > - if (!sh) { > + if (skb->len < dataoff + sizeof(struct sctphdr)) { > logmsg = "nf_ct_sctp: short packet "; > goto out_invalid; > } > if (net->ct.sysctl_checksum && hooknum == NF_INET_PRE_ROUTING && > skb->ip_summed == CHECKSUM_NONE) { > + if (!skb_make_writable(skb, dataoff + sizeof(struct sctphdr))) { > + logmsg = "nf_ct_sctp: failed to read header "; > + goto out_invalid; > + } > + sh = (const struct sctphdr *)(skb->data + dataoff); > if (sh->checksum != sctp_compute_cksum(skb, dataoff)) { > logmsg = "nf_ct_sctp: bad CRC "; > goto out_invalid; > -- > 2.7.4 > -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
hello Pablo, thank you for looking at this! On Fri, 2017-05-19 at 10:41 +0200, Pablo Neira Ayuso wrote: > On Thu, May 18, 2017 at 06:01:43PM +0200, Davide Caratti wrote: > > sctp_compute_cksum() implementation assumes that at least the SCTP header > > is in the linear part of skb: modify conntrack error callback to avoid > > false CRC32c mismatch, if the transport header is partially/entirely paged. > > I guess you considered this, but I would like to know the reason for > this approach. Why not fix this from sctp_compute_cksum()? I think sctp_compute_cksum() is legitimately needing the transport header i n the linear data of skb, because it needs to set to zero 4 octects at CRC32c offset before computing the CRC32c (as per RFC3309 §2.1). Since these are the last 4 octects of the SCTP header, then we need to __pskb_pull_tail() on the whole header, if some/all of its members are paged. > I mean, I can see other spots in the kernel tree that may be affected by this? > Or is it that you're only observing this from a path that is specific > of conntrack? I did the check before posting, and the kernel code seemed to already ensure skb is writable until SCTP header + sizeof(SCTP header) offset, before calling sctp_compute_cksum(). Just to be sure, I re-did that check today: besides nf_conntrack sctp_error(), I'm only doubtful about IPVS sctp_csum_check() (but I don't have a test scenario yet). That's why I propose to fix only sctp_error() in conntrack. Regarding IPVS, 2 out of 3 calls to sctp_compute_cksum() are preceded by skb_make_writable(), which is correct. I can do a test for IPVS sctp_csum_check() and check if it also needs some change, and post it in a separate patch. Is that acceptable? thank you in advance, regards -- davide -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
hello Pablo, On Fri, 2017-05-19 at 13:39 +0200, Davide Caratti wrote: > On Fri, 2017-05-19 at 10:41 +0200, Pablo Neira Ayuso wrote: > > I mean, I can see other spots in the kernel tree that may be affected by this? > > Or is it that you're only observing this from a path that is specific > > of conntrack? > > I did the check before posting, and the kernel code seemed to already > ensure skb is writable until SCTP header + sizeof(SCTP header) offset, > before calling sctp_compute_cksum(). Just to be sure, I re-did that check > today: besides nf_conntrack sctp_error(), I'm only doubtful about IPVS > sctp_csum_check() (but I don't have a test scenario yet). looking at IPVS code: it seems to me that the only call to sctp_csum_check() is inside sctp_snat_handler(), after skb_make_writable() has returned successfully. So, apparently misuse of sctp_compute_cksum() affects only nf_conntrack module in sctp_error() callback. Maybe this patch needs 'Fixes: cf6e007eef83 ("netfilter: conntrack: validate SCTP crc32c in PREROUTING")' tag ? thanks! -- davide -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 23, 2017 at 03:51:05PM +0200, Davide Caratti wrote: > hello Pablo, > On Fri, 2017-05-19 at 13:39 +0200, Davide Caratti wrote: > > On Fri, 2017-05-19 at 10:41 +0200, Pablo Neira Ayuso wrote: > > > I mean, I can see other spots in the kernel tree that may be affected by this? > > > Or is it that you're only observing this from a path that is specific > > > of conntrack? > > > > I did the check before posting, and the kernel code seemed to already > > ensure skb is writable until SCTP header + sizeof(SCTP header) offset, > > before calling sctp_compute_cksum(). Just to be sure, I re-did that check > > today: besides nf_conntrack sctp_error(), I'm only doubtful about IPVS > > sctp_csum_check() (but I don't have a test scenario yet). > > looking at IPVS code: it seems to me that the only call to sctp_csum_check() > is inside sctp_snat_handler(), after skb_make_writable() has returned > successfully. So, apparently misuse of sctp_compute_cksum() affects only > nf_conntrack module in sctp_error() callback. > > Maybe this patch needs 'Fixes: cf6e007eef83 ("netfilter: conntrack: validate > SCTP crc32c in PREROUTING")' tag ? Thanks for explaining. I will append this "Fixes:" tag to this patch once I apply this. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 23, 2017 at 09:35:33PM +0200, Pablo Neira Ayuso wrote: > On Tue, May 23, 2017 at 03:51:05PM +0200, Davide Caratti wrote: > > hello Pablo, > > On Fri, 2017-05-19 at 13:39 +0200, Davide Caratti wrote: > > > On Fri, 2017-05-19 at 10:41 +0200, Pablo Neira Ayuso wrote: > > > > I mean, I can see other spots in the kernel tree that may be affected by this? > > > > Or is it that you're only observing this from a path that is specific > > > > of conntrack? > > > > > > I did the check before posting, and the kernel code seemed to already > > > ensure skb is writable until SCTP header + sizeof(SCTP header) offset, > > > before calling sctp_compute_cksum(). Just to be sure, I re-did that check > > > today: besides nf_conntrack sctp_error(), I'm only doubtful about IPVS > > > sctp_csum_check() (but I don't have a test scenario yet). > > > > looking at IPVS code: it seems to me that the only call to sctp_csum_check() > > is inside sctp_snat_handler(), after skb_make_writable() has returned > > successfully. So, apparently misuse of sctp_compute_cksum() affects only > > nf_conntrack module in sctp_error() callback. > > > > Maybe this patch needs 'Fixes: cf6e007eef83 ("netfilter: conntrack: validate > > SCTP crc32c in PREROUTING")' tag ? > > Thanks for explaining. > > I will append this "Fixes:" tag to this patch once I apply this. Applied, thanks. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/netfilter/nf_conntrack_proto_sctp.c b/net/netfilter/nf_conntrack_proto_sctp.c index 13875d5..1c5b14a 100644 --- a/net/netfilter/nf_conntrack_proto_sctp.c +++ b/net/netfilter/nf_conntrack_proto_sctp.c @@ -512,16 +512,19 @@ static int sctp_error(struct net *net, struct nf_conn *tpl, struct sk_buff *skb, u8 pf, unsigned int hooknum) { const struct sctphdr *sh; - struct sctphdr _sctph; const char *logmsg; - sh = skb_header_pointer(skb, dataoff, sizeof(_sctph), &_sctph); - if (!sh) { + if (skb->len < dataoff + sizeof(struct sctphdr)) { logmsg = "nf_ct_sctp: short packet "; goto out_invalid; } if (net->ct.sysctl_checksum && hooknum == NF_INET_PRE_ROUTING && skb->ip_summed == CHECKSUM_NONE) { + if (!skb_make_writable(skb, dataoff + sizeof(struct sctphdr))) { + logmsg = "nf_ct_sctp: failed to read header "; + goto out_invalid; + } + sh = (const struct sctphdr *)(skb->data + dataoff); if (sh->checksum != sctp_compute_cksum(skb, dataoff)) { logmsg = "nf_ct_sctp: bad CRC "; goto out_invalid;
sctp_compute_cksum() implementation assumes that at least the SCTP header is in the linear part of skb: modify conntrack error callback to avoid false CRC32c mismatch, if the transport header is partially/entirely paged. Signed-off-by: Davide Caratti <dcaratti@redhat.com> --- net/netfilter/nf_conntrack_proto_sctp.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)