On Wed, Jun 27, 2018 at 05:10:02PM +0200, Pablo Neira Ayuso wrote:
> Hi,
>
> On Sun, Jun 24, 2018 at 03:03:07PM +0200, Máté Eckl wrote:
> > This patch fixes a silent out-of-bound read possibility that was present
> > because of the misuse of this function.
>
> This is a bit confusing. Subject says this is a refactoring, but this
> seems not be a clean up, but actually fixing up something.
Yes, it is a fix indeed, sorry.
>
> > Mostly it was called with a struct udphdr *hp which had only the udphdr
> > part linearized by the skb_header_pointer, however
> > nf_tproxy_get_sock_v{4,6} uses it as a tcphdr pointer, so some reads for
> > tcp specific attributes may be invalid.
>
> I think we should rename title to something like:
>
> netfilter: nf_tproxy: possible non-linear access to transport header
>
> A "Fixes:" tag would be good? Is this a new bug or it has been
> introduced by your recent changes?
This code seems to date back to a583636a83ea in 2016. I'll add this to the
commit message.
> I think we should get this through nf.git, then you will have to wait
> a bit to see how this dependency propagates to nf-next.git.
>
> Another comestic comment below.
>
> > Signed-off-by: Máté Eckl <[email protected]>
> > ---
> > include/net/netfilter/nf_tproxy.h | 4 ++--
> > net/ipv4/netfilter/nf_tproxy_ipv4.c | 15 ++++++++++-----
> > net/ipv6/netfilter/nf_tproxy_ipv6.c | 15 ++++++++++-----
> > net/netfilter/xt_TPROXY.c | 8 ++++----
> > 4 files changed, 26 insertions(+), 16 deletions(-)
> >
> > diff --git a/include/net/netfilter/nf_tproxy.h
> > b/include/net/netfilter/nf_tproxy.h
> > index 9754a50ecde9..4cc64c8446eb 100644
> > --- a/include/net/netfilter/nf_tproxy.h
> > +++ b/include/net/netfilter/nf_tproxy.h
> > @@ -64,7 +64,7 @@ nf_tproxy_handle_time_wait4(struct net *net, struct
> > sk_buff *skb,
> > * belonging to established connections going through that one.
> > */
> > struct sock *
> > -nf_tproxy_get_sock_v4(struct net *net, struct sk_buff *skb, void *hp,
> > +nf_tproxy_get_sock_v4(struct net *net, struct sk_buff *skb,
> > const u8 protocol,
> > const __be32 saddr, const __be32 daddr,
> > const __be16 sport, const __be16 dport,
> > @@ -103,7 +103,7 @@ nf_tproxy_handle_time_wait6(struct sk_buff *skb, int
> > tproto, int thoff,
> > struct sock *sk);
> >
> > struct sock *
> > -nf_tproxy_get_sock_v6(struct net *net, struct sk_buff *skb, int thoff,
> > void *hp,
> > +nf_tproxy_get_sock_v6(struct net *net, struct sk_buff *skb, int thoff,
> > const u8 protocol,
> > const struct in6_addr *saddr, const struct in6_addr
> > *daddr,
> > const __be16 sport, const __be16 dport,
> > diff --git a/net/ipv4/netfilter/nf_tproxy_ipv4.c
> > b/net/ipv4/netfilter/nf_tproxy_ipv4.c
> > index 805e83ec3ad9..efbec3b2ad25 100644
> > --- a/net/ipv4/netfilter/nf_tproxy_ipv4.c
> > +++ b/net/ipv4/netfilter/nf_tproxy_ipv4.c
> > @@ -37,7 +37,7 @@ nf_tproxy_handle_time_wait4(struct net *net, struct
> > sk_buff *skb,
> > * to a listener socket if there's one */
> > struct sock *sk2;
> >
> > - sk2 = nf_tproxy_get_sock_v4(net, skb, hp, iph->protocol,
> > + sk2 = nf_tproxy_get_sock_v4(net, skb, iph->protocol,
> > iph->saddr, laddr ? laddr :
> > iph->daddr,
> > hp->source, lport ? lport :
> > hp->dest,
> > skb->dev,
> > NF_TPROXY_LOOKUP_LISTENER);
> > @@ -71,7 +71,7 @@ __be32 nf_tproxy_laddr4(struct sk_buff *skb, __be32
> > user_laddr, __be32 daddr)
> > EXPORT_SYMBOL_GPL(nf_tproxy_laddr4);
> >
> > struct sock *
> > -nf_tproxy_get_sock_v4(struct net *net, struct sk_buff *skb, void *hp,
> > +nf_tproxy_get_sock_v4(struct net *net, struct sk_buff *skb,
> > const u8 protocol,
> > const __be32 saddr, const __be32 daddr,
> > const __be16 sport, const __be16 dport,
> > @@ -79,16 +79,21 @@ nf_tproxy_get_sock_v4(struct net *net, struct sk_buff
> > *skb, void *hp,
> > const enum nf_tproxy_lookup_t lookup_type)
> > {
> > struct sock *sk;
> > - struct tcphdr *tcph;
> > + struct tcphdr _hdr, *hp;
>
> While you're updating this code, variable definitions in this form are
> prefered:
>
> struct tcphdr _hdr, *hp;
> struct sock *sk;
>
> Larger line if code first.
>
> Please, revamp and send v2.
>
> Thanks Máté.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
