netfilter: nf_conntrack_sip: fix ct_sip_walk_headers
diff mbox series

Message ID 20190605093240.23212-1-iryzhov@nfware.com
State Needs Review / ACK
Delegated to: Pablo Neira
Headers show
Series
  • netfilter: nf_conntrack_sip: fix ct_sip_walk_headers
Related show

Commit Message

Igor Ryzhov June 5, 2019, 9:32 a.m. UTC
ct_sip_next_header and ct_sip_get_header return an absolute
value of matchoff, not a shift from current dataoff.
So dataoff should be assigned matchoff, not incremented by it.

Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
---
 net/netfilter/nf_conntrack_sip.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Pablo Neira Ayuso June 17, 2019, 10:25 p.m. UTC | #1
On Wed, Jun 05, 2019 at 12:32:40PM +0300, Igor Ryzhov wrote:
> ct_sip_next_header and ct_sip_get_header return an absolute
> value of matchoff, not a shift from current dataoff.
> So dataoff should be assigned matchoff, not incremented by it.

Could we get a more detailed description of this bug? A description of
the simplified scenario / situation that help you found it would help
here.

Thanks.

> Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
> ---
>  net/netfilter/nf_conntrack_sip.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
> index c30c883c370b..966c5948f926 100644
> --- a/net/netfilter/nf_conntrack_sip.c
> +++ b/net/netfilter/nf_conntrack_sip.c
> @@ -480,7 +480,7 @@ static int ct_sip_walk_headers(const struct nf_conn *ct, const char *dptr,
>  				return ret;
>  			if (ret == 0)
>  				break;
> -			dataoff += *matchoff;
> +			dataoff = *matchoff;
>  		}
>  		*in_header = 0;
>  	}
> @@ -492,7 +492,7 @@ static int ct_sip_walk_headers(const struct nf_conn *ct, const char *dptr,
>  			break;
>  		if (ret == 0)
>  			return ret;
> -		dataoff += *matchoff;
> +		dataoff = *matchoff;
>  	}
>  
>  	if (in_header)
> -- 
> 2.21.0
>
Igor Ryzhov June 18, 2019, 9:51 a.m. UTC | #2
Hi Pablo,

This issue can be seen in the scenario when there are multiple
Contact headers and the first one is using a hostname and other
headers use IP addresses. In this case, ct_sip_walk_headers will
work the following way:

The first ct_sip_get_header call to will find the first Contact header
but will return -1 as the header uses a hostname. But matchoff will
be changed to the offset of this header. After that, dataoff should be
set to matchoff, so that the next ct_sip_get_header call find the next
Contact header. But instead of assigning dataoff to matchoff, it is
incremented by it, which is not correct, as matchoff is an absolute
value of the offset. So on the next call to the ct_sip_get_header,
dataoff will be incorrect, and the next Contact header may not be
found at all.

Best regards,
Igor


On Tue, Jun 18, 2019 at 1:25 AM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> On Wed, Jun 05, 2019 at 12:32:40PM +0300, Igor Ryzhov wrote:
> > ct_sip_next_header and ct_sip_get_header return an absolute
> > value of matchoff, not a shift from current dataoff.
> > So dataoff should be assigned matchoff, not incremented by it.
>
> Could we get a more detailed description of this bug? A description of
> the simplified scenario / situation that help you found it would help
> here.
>
> Thanks.
>
> > Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
> > ---
> >  net/netfilter/nf_conntrack_sip.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
> > index c30c883c370b..966c5948f926 100644
> > --- a/net/netfilter/nf_conntrack_sip.c
> > +++ b/net/netfilter/nf_conntrack_sip.c
> > @@ -480,7 +480,7 @@ static int ct_sip_walk_headers(const struct nf_conn *ct, const char *dptr,
> >                               return ret;
> >                       if (ret == 0)
> >                               break;
> > -                     dataoff += *matchoff;
> > +                     dataoff = *matchoff;
> >               }
> >               *in_header = 0;
> >       }
> > @@ -492,7 +492,7 @@ static int ct_sip_walk_headers(const struct nf_conn *ct, const char *dptr,
> >                       break;
> >               if (ret == 0)
> >                       return ret;
> > -             dataoff += *matchoff;
> > +             dataoff = *matchoff;
> >       }
> >
> >       if (in_header)
> > --
> > 2.21.0
> >
Pablo Neira Ayuso June 18, 2019, 3:04 p.m. UTC | #3
On Tue, Jun 18, 2019 at 12:51:13PM +0300, Igor Ryzhov wrote:
> Hi Pablo,
> 
> This issue can be seen in the scenario when there are multiple
> Contact headers and the first one is using a hostname and other
> headers use IP addresses. In this case, ct_sip_walk_headers will
> work the following way:
> 
> The first ct_sip_get_header call to will find the first Contact header
> but will return -1 as the header uses a hostname. But matchoff will
> be changed to the offset of this header. After that, dataoff should be
> set to matchoff, so that the next ct_sip_get_header call find the next
> Contact header. But instead of assigning dataoff to matchoff, it is
> incremented by it, which is not correct, as matchoff is an absolute
> value of the offset. So on the next call to the ct_sip_get_header,
> dataoff will be incorrect, and the next Contact header may not be
> found at all.

Thanks for explaining. Would you resubmit a v2 including this
description in the patch?

Thanks.

Patch
diff mbox series

diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
index c30c883c370b..966c5948f926 100644
--- a/net/netfilter/nf_conntrack_sip.c
+++ b/net/netfilter/nf_conntrack_sip.c
@@ -480,7 +480,7 @@  static int ct_sip_walk_headers(const struct nf_conn *ct, const char *dptr,
 				return ret;
 			if (ret == 0)
 				break;
-			dataoff += *matchoff;
+			dataoff = *matchoff;
 		}
 		*in_header = 0;
 	}
@@ -492,7 +492,7 @@  static int ct_sip_walk_headers(const struct nf_conn *ct, const char *dptr,
 			break;
 		if (ret == 0)
 			return ret;
-		dataoff += *matchoff;
+		dataoff = *matchoff;
 	}
 
 	if (in_header)