[Acked] [Lucid][CVE-2014-3687] net: sctp: fix panic on duplicate ASCONF chunks

[Andy Whitcroft]

On Tue, Nov 11, 2014 at 05:49:24PM +0000, Luis Henriques wrote:
> From: Daniel Borkmann <dborkman at redhat.com>
> 
> When receiving a e.g. semi-good formed connection scan in the
> form of ...
> 
>   -------------- INIT[ASCONF; ASCONF_ACK] ------------->
>   <----------- INIT-ACK[ASCONF; ASCONF_ACK] ------------
>   -------------------- COOKIE-ECHO -------------------->
>   <-------------------- COOKIE-ACK ---------------------
>   ---------------- ASCONF_a; ASCONF_b ----------------->
> 
> ... where ASCONF_a equals ASCONF_b chunk (at least both serials
> need to be equal), we panic an SCTP server!
> 
> The problem is that good-formed ASCONF chunks that we reply with
> ASCONF_ACK chunks are cached per serial. Thus, when we receive a
> same ASCONF chunk twice (e.g. through a lost ASCONF_ACK), we do
> not need to process them again on the server side (that was the
> idea, also proposed in the RFC). Instead, we know it was cached
> and we just resend the cached chunk instead. So far, so good.
> 
> Where things get nasty is in SCTP's side effect interpreter, that
> is, sctp_cmd_interpreter():
> 
> While incoming ASCONF_a (chunk = event_arg) is being marked
> !end_of_packet and !singleton, and we have an association context,
> we do not flush the outqueue the first time after processing the
> ASCONF_ACK singleton chunk via SCTP_CMD_REPLY. Instead, we keep it
> queued up, although we set local_cork to 1. Commit 2e3216cd54b1
> changed the precedence, so that as long as we get bundled, incoming
> chunks we try possible bundling on outgoing queue as well. Before
> this commit, we would just flush the output queue.
> 
> Now, while ASCONF_a's ASCONF_ACK sits in the corked outq, we
> continue to process the same ASCONF_b chunk from the packet. As
> we have cached the previous ASCONF_ACK, we find it, grab it and
> do another SCTP_CMD_REPLY command on it. So, effectively, we rip
> the chunk->list pointers and requeue the same ASCONF_ACK chunk
> another time. Since we process ASCONF_b, it's correctly marked
> with end_of_packet and we enforce an uncork, and thus flush, thus
> crashing the kernel.
> 
> Fix it by testing if the ASCONF_ACK is currently pending and if
> that is the case, do not requeue it. When flushing the output
> queue we may relink the chunk for preparing an outgoing packet,
> but eventually unlink it when it's copied into the skb right
> before transmission.
> 
> Joint work with Vlad Yasevich.
> 
> Fixes: 2e3216cd54b1 ("sctp: Follow security requirement of responding with 1 packet")
> Signed-off-by: Daniel Borkmann <dborkman at redhat.com>
> Signed-off-by: Vlad Yasevich <vyasevich at gmail.com>
> Signed-off-by: David S. Miller <davem at davemloft.net>
> (cherry picked from commit b69040d8e39f20d5215a03502a8e8b4c6ab78395)
> CVE-2014-3687
> BugLink: http://bugs.launchpad.net/bugs/1386392
> Signed-off-by: Luis Henriques <luis.henriques at canonical.com>
> ---
>  include/net/sctp/sctp.h | 5 +++++
>  net/sctp/associola.c    | 2 ++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> index 8a6d5297de16..ad5989a418b2 100644
> --- a/include/net/sctp/sctp.h
> +++ b/include/net/sctp/sctp.h
> @@ -509,6 +509,11 @@ static inline void sctp_assoc_pending_pmtu(struct sctp_association *asoc)
>  	asoc->pmtu_pending = 0;
>  }
>  
> +static inline bool sctp_chunk_pending(const struct sctp_chunk *chunk)
> +{
> +	return !list_empty(&chunk->list);
> +}
> +
>  /* Walk through a list of TLV parameters.  Don't trust the
>   * individual parameter lengths and instead depend on
>   * the chunk length to indicate when to stop.  Make sure
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index 506236df746b..3b3b2e0ca54d 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -1601,6 +1601,8 @@ struct sctp_chunk *sctp_assoc_lookup_asconf_ack(
>  	 * ack chunk whose serial number matches that of the request.
>  	 */
>  	list_for_each_entry(ack, &asoc->asconf_ack_list, transmitted_list) {
> +		if (sctp_chunk_pending(ack))
> +			continue;
>  		if (ack->subh.addip_hdr->serial == serial) {
>  			sctp_chunk_hold(ack);
>  			return ack;
> -- 

Is a cherry-pick, looks good.

Acked-by: Andy Whitcroft <apw at canonical.com>

-apw