[PATCH nf] netfilter: nf_conntrack_sip: fix use of uninitialized rtp_addr in process_sdp

[PATCH nf] netfilter: nf_conntrack_sip: fix use of uninitialized rtp_addr in process_sdp

[Weiming Shi]

process_sdp() declares union nf_inet_addr rtp_addr on the stack and
passes it to the nf_nat_sip sdp_session hook after walking the SDP
media descriptions. However rtp_addr is only initialized inside the
media loop when a recognized media type with a non-zero port is found.

If the SDP body contains no m= lines, only inactive media sections
(m=audio 0 ...) or only unrecognized media types, rtp_addr is never
assigned. Despite that, the function still calls hooks->sdp_session()
with &rtp_addr, causing nf_nat_sdp_session() to format the stale stack
value as an IP address and rewrite the SDP session owner and connection
lines with it.

With CONFIG_INIT_STACK_ALL_ZERO (default on most distributions) this
results in the session-level o= and c= addresses being rewritten to
0.0.0.0 for inactive SDP sessions. Without stack auto-init the
rewritten address is whatever happened to be on the stack.

Fix this by pre-initializing rtp_addr from the session-level connection
address (caddr) when available, and tracking via a have_rtp_addr flag
whether any valid address was established. Skip the sdp_session hook
entirely when no valid address exists.

Fixes: 4ab9e64e5e3c ("[NETFILTER]: nf_nat_sip: split up SDP mangling")
Reported-by: Xiang Mei <xmei5@asu.edu>
Signed-off-by: Weiming Shi <bestswngs@gmail.com>
---
 net/netfilter/nf_conntrack_sip.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
index 4ab5ef71d96db..17af0ff4ea7ab 100644
--- a/net/netfilter/nf_conntrack_sip.c
+++ b/net/netfilter/nf_conntrack_sip.c
@@ -1040,6 +1040,7 @@ static int process_sdp(struct sk_buff *skb, unsigned int protoff,
 	unsigned int port;
 	const struct sdp_media_type *t;
 	int ret = NF_ACCEPT;
+	bool have_rtp_addr = false;
 
 	hooks = rcu_dereference(nf_nat_sip_hooks);
 
@@ -1056,8 +1057,11 @@ static int process_sdp(struct sk_buff *skb, unsigned int protoff,
 	caddr_len = 0;
 	if (ct_sip_parse_sdp_addr(ct, *dptr, sdpoff, *datalen,
 				  SDP_HDR_CONNECTION, SDP_HDR_MEDIA,
-				  &matchoff, &matchlen, &caddr) > 0)
+				  &matchoff, &matchlen, &caddr) > 0) {
 		caddr_len = matchlen;
+		memcpy(&rtp_addr, &caddr, sizeof(rtp_addr));
+		have_rtp_addr = true;
+	}
 
 	mediaoff = sdpoff;
 	for (i = 0; i < ARRAY_SIZE(sdp_media_types); ) {
@@ -1091,9 +1095,11 @@ static int process_sdp(struct sk_buff *skb, unsigned int protoff,
 					  &matchoff, &matchlen, &maddr) > 0) {
 			maddr_len = matchlen;
 			memcpy(&rtp_addr, &maddr, sizeof(rtp_addr));
-		} else if (caddr_len)
+			have_rtp_addr = true;
+		} else if (caddr_len) {
 			memcpy(&rtp_addr, &caddr, sizeof(rtp_addr));
-		else {
+			have_rtp_addr = true;
+		} else {
 			nf_ct_helper_log(skb, ct, "cannot parse SDP message");
 			return NF_DROP;
 		}
@@ -1125,7 +1131,7 @@ static int process_sdp(struct sk_buff *skb, unsigned int protoff,
 
 	/* Update session connection and owner addresses */
 	hooks = rcu_dereference(nf_nat_sip_hooks);
-	if (hooks && ct->status & IPS_NAT_MASK)
+	if (hooks && ct->status & IPS_NAT_MASK && have_rtp_addr)
 		ret = hooks->sdp_session(skb, protoff, dataoff,
 					 dptr, datalen, sdpoff,
 					 &rtp_addr);
-- 
2.43.0

Re: [PATCH nf] netfilter: nf_conntrack_sip: fix use of uninitialized rtp_addr in process_sdp

[Pablo Neira Ayuso]

On Mon, Mar 23, 2026 at 04:07:29PM +0800, Weiming Shi wrote:
> process_sdp() declares union nf_inet_addr rtp_addr on the stack and
> passes it to the nf_nat_sip sdp_session hook after walking the SDP
> media descriptions. However rtp_addr is only initialized inside the
> media loop when a recognized media type with a non-zero port is found.
> 
> If the SDP body contains no m= lines, only inactive media sections
> (m=audio 0 ...) or only unrecognized media types, rtp_addr is never
> assigned. Despite that, the function still calls hooks->sdp_session()
> with &rtp_addr, causing nf_nat_sdp_session() to format the stale stack
> value as an IP address and rewrite the SDP session owner and connection
> lines with it.
> 
> With CONFIG_INIT_STACK_ALL_ZERO (default on most distributions) this
> results in the session-level o= and c= addresses being rewritten to
> 0.0.0.0 for inactive SDP sessions. Without stack auto-init the
> rewritten address is whatever happened to be on the stack.
> 
> Fix this by pre-initializing rtp_addr from the session-level connection
> address (caddr) when available, and tracking via a have_rtp_addr flag
> whether any valid address was established. Skip the sdp_session hook
> entirely when no valid address exists.
> 
> Fixes: 4ab9e64e5e3c ("[NETFILTER]: nf_nat_sip: split up SDP mangling")
> Reported-by: Xiang Mei <xmei5@asu.edu>
> Signed-off-by: Weiming Shi <bestswngs@gmail.com>
> ---
>  net/netfilter/nf_conntrack_sip.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
> index 4ab5ef71d96db..17af0ff4ea7ab 100644
> --- a/net/netfilter/nf_conntrack_sip.c
> +++ b/net/netfilter/nf_conntrack_sip.c
> @@ -1040,6 +1040,7 @@ static int process_sdp(struct sk_buff *skb, unsigned int protoff,
>  	unsigned int port;
>  	const struct sdp_media_type *t;
>  	int ret = NF_ACCEPT;
> +	bool have_rtp_addr = false;
>  
>  	hooks = rcu_dereference(nf_nat_sip_hooks);
>  
> @@ -1056,8 +1057,11 @@ static int process_sdp(struct sk_buff *skb, unsigned int protoff,
>  	caddr_len = 0;
>  	if (ct_sip_parse_sdp_addr(ct, *dptr, sdpoff, *datalen,
>  				  SDP_HDR_CONNECTION, SDP_HDR_MEDIA,
> -				  &matchoff, &matchlen, &caddr) > 0)
> +				  &matchoff, &matchlen, &caddr) > 0) {
>  		caddr_len = matchlen;
> +		memcpy(&rtp_addr, &caddr, sizeof(rtp_addr));
> +		have_rtp_addr = true;
> +	}
>  
>  	mediaoff = sdpoff;
>  	for (i = 0; i < ARRAY_SIZE(sdp_media_types); ) {
> @@ -1091,9 +1095,11 @@ static int process_sdp(struct sk_buff *skb, unsigned int protoff,
>  					  &matchoff, &matchlen, &maddr) > 0) {
>  			maddr_len = matchlen;
>  			memcpy(&rtp_addr, &maddr, sizeof(rtp_addr));
> -		} else if (caddr_len)
> +			have_rtp_addr = true;
> +		} else if (caddr_len) {
>  			memcpy(&rtp_addr, &caddr, sizeof(rtp_addr));
> -		else {
> +			have_rtp_addr = true;

After this update, this loop sets over rtp_addr, but this was already
set by ct_sip_parse_sdp_addr() a bit above.

This new chunk results in:

                } else if (caddr_len) {
                       memcpy(&rtp_addr, &caddr, sizeof(rtp_addr));
                       have_rtp_addr = true;

which is not needed? Why does caddr need to be copied over and over
again to rtp_addr?

> +		} else {
>  			nf_ct_helper_log(skb, ct, "cannot parse SDP message");
>  			return NF_DROP;
>  		}

Re: [PATCH nf] netfilter: nf_conntrack_sip: fix use of uninitialized rtp_addr in process_sdp

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > @@ -1091,9 +1095,11 @@ static int process_sdp(struct sk_buff *skb, unsigned int protoff,
> >  					  &matchoff, &matchlen, &maddr) > 0) {
> >  			maddr_len = matchlen;
> >  			memcpy(&rtp_addr, &maddr, sizeof(rtp_addr));
> > -		} else if (caddr_len)
> > +			have_rtp_addr = true;
> > +		} else if (caddr_len) {
> >  			memcpy(&rtp_addr, &caddr, sizeof(rtp_addr));
> > -		else {
> > +			have_rtp_addr = true;
> 
> After this update, this loop sets over rtp_addr, but this was already
> set by ct_sip_parse_sdp_addr() a bit above.
> 
> This new chunk results in:
> 
>                 } else if (caddr_len) {
>                        memcpy(&rtp_addr, &caddr, sizeof(rtp_addr));
>                        have_rtp_addr = true;
> 
> which is not needed? Why does caddr need to be copied over and over
> again to rtp_addr?

Code before update was:
                if (ct_sip_parse_sdp_addr(ct, *dptr, mediaoff, *datalen,
                                          SDP_HDR_CONNECTION, SDP_HDR_MEDIA,
                                          &matchoff, &matchlen, &maddr) > 0) {
                        maddr_len = matchlen;
                        memcpy(&rtp_addr, &maddr, sizeof(rtp_addr));
                } else if (caddr_len)
                        memcpy(&rtp_addr, &caddr, sizeof(rtp_addr));
                else {


so we re-set rtp_addr to the session description in case it was
overwritten by earlier iteration of the loop.

1. Extract session description (caddr_len set)
2. enter loop, parse media description (overwrite rtp_addr with media
   address)
3. next loop fails ct_sip_parse_sdp_addr() call
   Restore the original session address instead of using
   the previous media description.

I think its correct this way.

Re: [PATCH nf] netfilter: nf_conntrack_sip: fix use of uninitialized rtp_addr in process_sdp

[Pablo Neira Ayuso]

On Thu, Mar 26, 2026 at 09:44:17PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > @@ -1091,9 +1095,11 @@ static int process_sdp(struct sk_buff *skb, unsigned int protoff,
> > >  					  &matchoff, &matchlen, &maddr) > 0) {
> > >  			maddr_len = matchlen;
> > >  			memcpy(&rtp_addr, &maddr, sizeof(rtp_addr));
> > > -		} else if (caddr_len)
> > > +			have_rtp_addr = true;
> > > +		} else if (caddr_len) {
> > >  			memcpy(&rtp_addr, &caddr, sizeof(rtp_addr));
> > > -		else {
> > > +			have_rtp_addr = true;
> > 
> > After this update, this loop sets over rtp_addr, but this was already
> > set by ct_sip_parse_sdp_addr() a bit above.
> > 
> > This new chunk results in:
> > 
> >                 } else if (caddr_len) {
> >                        memcpy(&rtp_addr, &caddr, sizeof(rtp_addr));
> >                        have_rtp_addr = true;
> > 
> > which is not needed? Why does caddr need to be copied over and over
> > again to rtp_addr?
> 
> Code before update was:
>                 if (ct_sip_parse_sdp_addr(ct, *dptr, mediaoff, *datalen,
>                                           SDP_HDR_CONNECTION, SDP_HDR_MEDIA,
>                                           &matchoff, &matchlen, &maddr) > 0) {
>                         maddr_len = matchlen;
>                         memcpy(&rtp_addr, &maddr, sizeof(rtp_addr));
>                 } else if (caddr_len)
>                         memcpy(&rtp_addr, &caddr, sizeof(rtp_addr));
>                 else {
> 
> 
> so we re-set rtp_addr to the session description in case it was
> overwritten by earlier iteration of the loop.
> 
> 1. Extract session description (caddr_len set)
> 2. enter loop, parse media description (overwrite rtp_addr with media
>    address)
> 3. next loop fails ct_sip_parse_sdp_addr() call
>    Restore the original session address instead of using
>    the previous media description.
> 
> I think its correct this way.

Thanks for explaining, this is leaving things as they were before this
patch.