sctp: add some size validations
Subsystems (3)
Marcelo Ricardo Leitner
maintainercover sctp: add some size validations
Ilja Van Sprundel reported that some size validations on inbound SCTP packets were missing. After some code review, I noticed two others that are all fixed here.
Thanks Ilja for reporting this.
Marcelo Ricardo Leitner (4):
sctp: validate from_addr_param return
sctp: add size validation when walking chunks
sctp: validate chunk size in __rcv_asconf_lookup
sctp: add param size validation for SCTP_PARAM_SET_PRIMARY
include/net/sctp/structs.h | 2 +- net/sctp/bind_addr.c | 19 +++++++++-------- net/sctp/input.c | 11 +++++++--- net/sctp/ipv6.c | 7 ++++++- net/sctp/protocol.c | 7 ++++++- net/sctp/sm_make_chunk.c | 42 +++++++++++++++++++++++--------------- 6 files changed, 58 insertions(+), 30 deletions(-)
Signature · 1 line
2.31.1
Marcelo Ricardo Leitner
maintainerPATCH 1/4 sctp: validate from_addr_param return
Ilja reported that, simply putting it, nothing was validating that from_addr_param functions were operating on initialized memory. That is, the parameter itself was being validated by sctp_walk_params, but it doesn't check for types and their specific sizes and it could be a 0-length one, causing from_addr_param to potentially work over the next parameter or even uninitialized memory.
The fix here is to, in all calls to from_addr_param, check if enough space is there for the wanted IP address type.
--- include/net/sctp/structs.h | 2 +- net/sctp/bind_addr.c | 19 +++++++++++-------- net/sctp/input.c | 6 ++++-- net/sctp/ipv6.c | 7 ++++++- net/sctp/protocol.c | 7 ++++++- net/sctp/sm_make_chunk.c | 29 ++++++++++++++++------------- 6 files changed, 44 insertions(+), 26 deletions(-)
include/net/sctp/structs.h +1 -1
@@ -461,7 +461,7 @@ struct sctp_af { int saddr);void (*from_sk) (union sctp_addr *,struct sock *sk); -void (*from_addr_param) (union sctp_addr *, +bool (*from_addr_param) (union sctp_addr *,union sctp_addr_param *,__be16 port,int iif); int (*to_addr_param) (const union sctp_addr *,
net/sctp/bind_addr.c +11 -8
@@ -270,22 +270,19 @@ int sctp_raw_to_bind_addrs(struct sctp_bind_addr *bp, __u8 *raw_addr_list, rawaddr = (union sctp_addr_param *)raw_addr_list; af =sctp_get_af_specific (param_type2af (param->type)); -if (unlikely (!af)) { +if (unlikely (!af) || + !af->from_addr_param (&addr, rawaddr,htons (port),0 )) { retval = -EINVAL ; -sctp_bind_addr_clean (bp); -break ; + goto out_err; } - af->from_addr_param (&addr, rawaddr,htons (port),0 );if (sctp_bind_addr_state (bp, &addr) !=-1 ) goto next; retval =sctp_add_bind_addr (bp, &addr,sizeof (addr),SCTP_ADDR_SRC , gfp); -if (retval) { +if (retval)/* Can't finish building the list, clean up. */ -sctp_bind_addr_clean (bp); -break ; - } + goto out_err; next: len =ntohs (param->length);
@@ -294,6 +291,12 @@ int sctp_raw_to_bind_addrs(struct sctp_bind_addr *bp, __u8 *raw_addr_list, }return retval; + +out_err: +if (retval) +sctp_bind_addr_clean (bp); + +return retval; }/********************************************************************
net/sctp/input.c +4 -2
@@ -1131,7 +1131,8 @@ static struct sctp_association *__sctp_rcv_init_lookup(struct net *net,if (!af)continue ; - af->from_addr_param (paddr, params.addr, sh->source,0 ); +if (!af->from_addr_param (paddr, params.addr, sh->source,0 )) +continue ; asoc =__sctp_lookup_association (net, laddr, paddr, transportp);if (asoc)
@@ -1174,7 +1175,8 @@ static struct sctp_association *__sctp_rcv_asconf_lookup(if (unlikely (!af))return NULL ; - af->from_addr_param (&paddr, param, peer_port,0 ); +if (af->from_addr_param (&paddr, param, peer_port,0 )) +return NULL ;return __sctp_lookup_association (net, laddr, &paddr, transportp); }
net/sctp/ipv6.c +6 -1
@@ -551,15 +551,20 @@ static void sctp_v6_to_sk_daddr(union sctp_addr *addr, struct sock *sk) }/* Initialize a sctp_addr from an address parameter. */ -static void sctp_v6_from_addr_param (union sctp_addr *addr, +static bool sctp_v6_from_addr_param (union sctp_addr *addr,union sctp_addr_param *param,__be16 port,int iif) { +if (ntohs (param->v6.param_hdr.length) <sizeof (struct sctp_ipv6addr_param )) +return false; + addr->v6.sin6_family =AF_INET6 ; addr->v6.sin6_port = port; addr->v6.sin6_flowinfo =0 ;/* BUG */ addr->v6.sin6_addr = param->v6.addr; addr->v6.sin6_scope_id = iif; + +return true; }/* Initialize an address parameter from a sctp_addr and return the length
net/sctp/protocol.c +6 -1
@@ -254,14 +254,19 @@ static void sctp_v4_to_sk_daddr(union sctp_addr *addr, struct sock *sk) }/* Initialize a sctp_addr from an address parameter. */ -static void sctp_v4_from_addr_param (union sctp_addr *addr, +static bool sctp_v4_from_addr_param (union sctp_addr *addr,union sctp_addr_param *param,__be16 port,int iif) { +if (ntohs (param->v4.param_hdr.length) <sizeof (struct sctp_ipv4addr_param )) +return false; + addr->v4.sin_family =AF_INET ; addr->v4.sin_port = port; addr->v4.sin_addr.s_addr = param->v4.addr.s_addr;memset (addr->v4.sin_zero,0 ,sizeof (addr->v4.sin_zero)); + +return true; }/* Initialize an address parameter from a sctp_addr and return the length
net/sctp/sm_make_chunk.c +16 -13
@@ -2346,11 +2346,13 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk,/* Process the initialization parameters. */ sctp_walk_params(param, peer_init, init_hdr.params) { -if (!src_match && (param.p->type ==SCTP_PARAM_IPV4_ADDRESS || - param.p->type ==SCTP_PARAM_IPV6_ADDRESS )) { +if (!src_match && + (param.p->type ==SCTP_PARAM_IPV4_ADDRESS || + param.p->type ==SCTP_PARAM_IPV6_ADDRESS )) { af =sctp_get_af_specific (param_type2af (param.p->type)); - af->from_addr_param (&addr, param.addr, - chunk->sctp_hdr->source,0 ); +if (!af->from_addr_param (&addr, param.addr, + chunk->sctp_hdr->source,0 )) +continue ;if (sctp_cmp_addr_exact (sctp_source (chunk), &addr)) src_match =1 ; }
@@ -2531,7 +2533,8 @@ static int sctp_process_param(struct sctp_association *asoc,break ; do_addr_param: af =sctp_get_af_specific (param_type2af (param.p->type)); - af->from_addr_param (&addr, param.addr,htons (asoc->peer.port),0 ); +if (!af->from_addr_param (&addr, param.addr,htons (asoc->peer.port),0 )) +break ; scope =sctp_scope (peer_addr);if (sctp_in_scope (net, &addr, scope))if (!sctp_assoc_add_peer (asoc, &addr, gfp,SCTP_UNCONFIRMED ))
@@ -2632,15 +2635,13 @@ static int sctp_process_param(struct sctp_association *asoc, addr_param = param.v +sizeof (struct sctp_addip_param ); af =sctp_get_af_specific (param_type2af (addr_param->p.type)); -if (af ==NULL ) +if (!af)break ; - af->from_addr_param (&addr, addr_param, -htons (asoc->peer.port),0 ); +if (!af->from_addr_param (&addr, addr_param, +htons (asoc->peer.port),0 )) +break ; -/* if the address is invalid, we can't process it. -* XXX: see spec for what to do. -*/ if (!af->addr_valid (&addr,NULL ,NULL ))break ;
@@ -3054,7 +3055,8 @@ static __be16 sctp_process_asconf_param(struct sctp_association *asoc,if (unlikely (!af))return SCTP_ERROR_DNS_FAILED ; - af->from_addr_param (&addr, addr_param,htons (asoc->peer.port),0 ); +if (!af->from_addr_param (&addr, addr_param,htons (asoc->peer.port),0 )) +return SCTP_ERROR_DNS_FAILED ;/* ADDIP 4.2.1 This parameter MUST NOT contain a broadcast * or multicast address.
@@ -3331,7 +3333,8 @@ static void sctp_asconf_param_success(struct sctp_association *asoc,/* We have checked the packet before, so we do not check again. */ af =sctp_get_af_specific (param_type2af (addr_param->p.type)); - af->from_addr_param (&addr, addr_param,htons (bp->port),0 ); +if (!af->from_addr_param (&addr, addr_param,htons (bp->port),0 )) +return ;switch (asconf_param->param_hdr.type) {case SCTP_PARAM_ADD_IP :
Signature · 1 line
2.31.1
Re: [PATCH net 1/4] sctp: validate from_addr_param return
On Mon, Jun 28, 2021 at 04:13:41PM -0300, Marcelo Ricardo Leitner wrote:
[...]
net/sctp/input.c static struct sctp_association *__sctp_rcv_asconf_lookup( · line 1182
Quoted 10 lines
net/sctp/input.c +2
-1@@ -1174,7 +1175,8 @@ static struct sctp_association *__sctp_rcv_asconf_lookup(if (unlikely (!af))return NULL ; - af->from_addr_param (&paddr, param, peer_port,0 ); +if (af->from_addr_param (&paddr, param, peer_port,0 )) +return NULL ;return __sctp_lookup_association (net, laddr, &paddr, transportp); }
[...]
This condition needs to be inverted, doesn't it?
Ben.
Signature · 2 lines
Ben Hutchings friends: People who know you well, but like you anyway.
Marcelo Ricardo Leitner
maintainerRe: [PATCH net 1/4] sctp: validate from_addr_param return
Quoted 10 lines · Ben Hutchings
On Mon, Jun 28, 2021 at 04:13:41PM -0300, Marcelo Ricardo Leitner wrote:
[...]Quoted 8 lines
net/sctp/input.c +2
-1@@ -1174,7 +1175,8 @@ static struct sctp_association *__sctp_rcv_asconf_lookup(if (unlikely (!af))return NULL ; - af->from_addr_param (&paddr, param, peer_port,0 ); +if (af->from_addr_param (&paddr, param, peer_port,0 )) +return NULL ;
Quoted 5 lines
net/sctp/input.c static struct sctp_association *__sctp_rcv_asconf_lookup( · line 1182
Quoted 2 lines
net/sctp/input.c +0
-0return __sctp_lookup_association (net, laddr, &paddr, transportp); }[...]
This condition needs to be inverted, doesn't it?
Right you are. I'll send a patch today.
Thanks,
Marcelo
Marcelo Ricardo Leitner
maintainerPATCH 2/4 sctp: add size validation when walking chunks
In reply to Marcelo Ricardo LeitnerThe first chunk in a packet is ensured to be present at the beginning of sctp_rcv(), as a packet needs to have at least 1 chunk. But the second one, may not be completely available and ch->length can be over uninitialized memory.
Fix here is by only trying to walk on the next chunk if there is enough to hold at least the header, and then proceed with the ch->length validation that is already there.
--- net/sctp/input.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
net/sctp/input.c +1 -1
@@ -1247,7 +1247,7 @@ static struct sctp_association *__sctp_rcv_walk_lookup(struct net *net, ch = (struct sctp_chunkhdr *)ch_end; chunk_num++; - }while (ch_end <skb_tail_pointer (skb)); + }while (ch_end +sizeof (*ch) <skb_tail_pointer (skb));return asoc; }
Signature · 1 line
2.31.1
Marcelo Ricardo Leitner
maintainerPATCH 3/4 sctp: validate chunk size in __rcv_asconf_lookup
In reply to Marcelo Ricardo LeitnerIn one of the fallbacks that SCTP has for identifying an association for an incoming packet, it looks for AddIp chunk (from ASCONF) and take a peek. Thing is, at this stage nothing was validating that the chunk actually had enough content for that, allowing the peek to happen over uninitialized memory.
Similar check already exists in actual asconf handling in sctp_verify_asconf().
--- net/sctp/input.c | 3 +++ 1 file changed, 3 insertions(+)
net/sctp/input.c +3 -0
@@ -1168,6 +1168,9 @@ static struct sctp_association *__sctp_rcv_asconf_lookup(union sctp_addr_param *param;union sctp_addr paddr; +if (ntohs (ch->length) <sizeof (*asconf) +sizeof (struct sctp_paramhdr )) +return NULL ; +/* Skip over the ADDIP header and find the Address parameter */ param = (union sctp_addr_param *)(asconf +1 );
Signature · 1 line
2.31.1
Marcelo Ricardo Leitner
maintainerPATCH 4/4 sctp: add param size validation for SCTP_PARAM_SET_PRIMARY
In reply to Marcelo Ricardo LeitnerWhen SCTP handles an INIT chunk, it calls for example:sctp_sf_do_5_1B_init
sctp_verify_init
sctp_verify_param
sctp_process_init
sctp_process_param
handling of SCTP_PARAM_SET_PRIMARY
sctp_verify_init() wasn't doing proper size validation and neither the later handling, allowing it to work over the chunk itself, possibly being uninitialized memory.
--- net/sctp/sm_make_chunk.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
net/sctp/sm_make_chunk.c +10 -3
@@ -2166,9 +2166,16 @@ static enum sctp_ierror sctp_verify_param(struct net *net,break ;case SCTP_PARAM_SET_PRIMARY : -if (ep->asconf_enable) -break ; - goto unhandled; +if (!ep->asconf_enable) + goto unhandled; + +if (ntohs (param.p->length) <sizeof (struct sctp_addip_param ) + +sizeof (struct sctp_paramhdr )) { +sctp_process_inv_paramlength (asoc, param.p, + chunk, err_chunk); + retval =SCTP_IERROR_ABORT ; + } +break ;case SCTP_PARAM_HOST_NAME_ADDRESS :/* Tell the peer, we won't support this param. */
Signature · 1 line
2.31.1
This series was applied to netdev/net.git (refs/heads/master):
Hello:
This series was applied to netdev/net.git (refs/heads/master):
Quoted 6 lines · On Mon, 28 Jun 2021 16:13:40 -0300 you wrote:
Ilja Van Sprundel reported that some size validations on inbound SCTP packets were missing. After some code review, I noticed two others that are all fixed here.
Thanks Ilja for reporting this.
Quoted 7 lines
Marcelo Ricardo Leitner (4):
sctp: validatefrom_addr_paramreturn
sctp: add size validation when walking chunks
sctp: validate chunk size in__rcv_asconf_lookup
sctp: add param size validation forSCTP_PARAM_SET_PRIMARY[...]
Here is the summary with links:
- [net,1/4] sctp: validate from_addr_param return
https://git.kernel.org/netdev/net/c/0c5dc070ff3d - [net,2/4] sctp: add size validation when walking chunks https://git.kernel.org/netdev/net/c/50619dbf8db7 - [net,3/4] sctp: validate chunk size in __rcv_asconf_lookup https://git.kernel.org/netdev/net/c/b6ffe7671b24 - [net,4/4] sctp: add param size validation for SCTP_PARAM_SET_PRIMARY https://git.kernel.org/netdev/net/c/ef6c8d6ccf0c
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html