[Acked] [Lucid][CVE-2014-3688] net: sctp: fix remote memory pressure from excessive queueing

[Andy Whitcroft]

On Tue, Nov 11, 2014 at 05:49:30PM +0000, Luis Henriques wrote:
> From: Daniel Borkmann <dborkman at redhat.com>
> 
> This scenario is not limited to ASCONF, just taken as one
> example triggering the issue. When receiving ASCONF probes
> in the form of ...
> 
>   -------------- INIT[ASCONF; ASCONF_ACK] ------------->
>   <----------- INIT-ACK[ASCONF; ASCONF_ACK] ------------
>   -------------------- COOKIE-ECHO -------------------->
>   <-------------------- COOKIE-ACK ---------------------
>   ---- ASCONF_a; [ASCONF_b; ...; ASCONF_n;] JUNK ------>
>   [...]
>   ---- ASCONF_m; [ASCONF_o; ...; ASCONF_z;] JUNK ------>
> 
> ... where ASCONF_a, ASCONF_b, ..., ASCONF_z are good-formed
> ASCONFs and have increasing serial numbers, we process such
> ASCONF chunk(s) marked with !end_of_packet and !singleton,
> since we have not yet reached the SCTP packet end. SCTP does
> only do verification on a chunk by chunk basis, as an SCTP
> packet is nothing more than just a container of a stream of
> chunks which it eats up one by one.
> 
> We could run into the case that we receive a packet with a
> malformed tail, above marked as trailing JUNK. All previous
> chunks are here goodformed, so the stack will eat up all
> previous chunks up to this point. In case JUNK does not fit
> into a chunk header and there are no more other chunks in
> the input queue, or in case JUNK contains a garbage chunk
> header, but the encoded chunk length would exceed the skb
> tail, or we came here from an entirely different scenario
> and the chunk has pdiscard=1 mark (without having had a flush
> point), it will happen, that we will excessively queue up
> the association's output queue (a correct final chunk may
> then turn it into a response flood when flushing the
> queue ;)): I ran a simple script with incremental ASCONF
> serial numbers and could see the server side consuming
> excessive amount of RAM [before/after: up to 2GB and more].
> 
> The issue at heart is that the chunk train basically ends
> with !end_of_packet and !singleton markers and since commit
> 2e3216cd54b1 ("sctp: Follow security requirement of responding
> with 1 packet") therefore preventing an output queue flush
> point in sctp_do_sm() -> sctp_cmd_interpreter() on the input
> chunk (chunk = event_arg) even though local_cork is set,
> but its precedence has changed since then. In the normal
> case, the last chunk with end_of_packet=1 would trigger the
> queue flush to accommodate possible outgoing bundling.
> 
> In the input queue, sctp_inq_pop() seems to do the right thing
> in terms of discarding invalid chunks. So, above JUNK will
> not enter the state machine and instead be released and exit
> the sctp_assoc_bh_rcv() chunk processing loop. It's simply
> the flush point being missing at loop exit. Adding a try-flush
> approach on the output queue might not work as the underlying
> infrastructure might be long gone at this point due to the
> side-effect interpreter run.
> 
> One possibility, albeit a bit of a kludge, would be to defer
> invalid chunk freeing into the state machine in order to
> possibly trigger packet discards and thus indirectly a queue
> flush on error. It would surely be better to discard chunks
> as in the current, perhaps better controlled environment, but
> going back and forth, it's simply architecturally not possible.
> I tried various trailing JUNK attack cases and it seems to
> look good now.
> 
> 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 26b87c7881006311828bb0ab271a551a62dcceb4)
> CVE-2014-3688
> BugLink: http://bugs.launchpad.net/bugs/1386393
> Signed-off-by: Luis Henriques <luis.henriques at canonical.com>
> ---
>  net/sctp/inqueue.c      | 33 +++++++--------------------------
>  net/sctp/sm_statefuns.c |  3 +++
>  2 files changed, 10 insertions(+), 26 deletions(-)
> 
> diff --git a/net/sctp/inqueue.c b/net/sctp/inqueue.c
> index bbf5dd2a97c4..7f33bfabdc5b 100644
> --- a/net/sctp/inqueue.c
> +++ b/net/sctp/inqueue.c
> @@ -149,18 +149,9 @@ struct sctp_chunk *sctp_inq_pop(struct sctp_inq *queue)
>  		} else {
>  			/* Nothing to do. Next chunk in the packet, please. */
>  			ch = (sctp_chunkhdr_t *) chunk->chunk_end;
> -
>  			/* Force chunk->skb->data to chunk->chunk_end.  */
> -			skb_pull(chunk->skb,
> -				 chunk->chunk_end - chunk->skb->data);
> -
> -			/* Verify that we have at least chunk headers
> -			 * worth of buffer left.
> -			 */
> -			if (skb_headlen(chunk->skb) < sizeof(sctp_chunkhdr_t)) {
> -				sctp_chunk_free(chunk);
> -				chunk = queue->in_progress = NULL;
> -			}
> +			skb_pull(chunk->skb, chunk->chunk_end - chunk->skb->data);
> +			/* We are guaranteed to pull a SCTP header. */
>  		}
>  	}
>  
> @@ -196,24 +187,14 @@ struct sctp_chunk *sctp_inq_pop(struct sctp_inq *queue)
>  	skb_pull(chunk->skb, sizeof(sctp_chunkhdr_t));
>  	chunk->subh.v = NULL; /* Subheader is no longer valid.  */
>  
> -	if (chunk->chunk_end < skb_tail_pointer(chunk->skb)) {
> +	if (chunk->chunk_end + sizeof(sctp_chunkhdr_t) <
> +	    skb_tail_pointer(chunk->skb)) {
>  		/* This is not a singleton */
>  		chunk->singleton = 0;
>  	} else if (chunk->chunk_end > skb_tail_pointer(chunk->skb)) {
> -		/* RFC 2960, Section 6.10  Bundling
> -		 *
> -		 * Partial chunks MUST NOT be placed in an SCTP packet.
> -		 * If the receiver detects a partial chunk, it MUST drop
> -		 * the chunk.
> -		 *
> -		 * Since the end of the chunk is past the end of our buffer
> -		 * (which contains the whole packet, we can freely discard
> -		 * the whole packet.
> -		 */
> -		sctp_chunk_free(chunk);
> -		chunk = queue->in_progress = NULL;
> -
> -		return NULL;
> +		/* Discard inside state machine. */
> +		chunk->pdiscard = 1;
> +		chunk->chunk_end = skb_tail_pointer(chunk->skb);
>  	} else {
>  		/* We are at the end of the packet, so mark the chunk
>  		 * in case we need to send a SACK.
> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> index ac98a1e2527e..d40ff4aabbf4 100644
> --- a/net/sctp/sm_statefuns.c
> +++ b/net/sctp/sm_statefuns.c
> @@ -160,6 +160,9 @@ sctp_chunk_length_valid(struct sctp_chunk *chunk,
>  {
>  	__u16 chunk_length = ntohs(chunk->chunk_hdr->length);
>  
> +	/* Previously already marked? */
> +	if (unlikely(chunk->pdiscard))
> +		return 0;
>  	if (unlikely(chunk_length < required_length))
>  		return 0;
>  

Is a cherry-pick.  Looks good.

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

-apw