From e4dc99c7c21ba456fd72a70ada5d8d5f3850bcf5 Mon Sep 17 00:00:00 2001 From: Xin Long Date: Sat, 15 Apr 2017 22:00:27 +0800 Subject: [PATCH 1/3] sctp: process duplicated strreset out and addstrm out requests correctly Now sctp stream reconf will process a request again even if it's seqno is less than asoc->strreset_inseq. If one request has been done successfully and some data chunks have been accepted and then a duplicated strreset out request comes, the streamin's ssn will be cleared. It will cause that stream will never receive chunks any more because of unsynchronized ssn. It allows a replay attack. A similar issue also exists when processing addstrm out requests. It will cause more extra streams being added. This patch is to fix it by saving the last 2 results into asoc. When a duplicated strreset out or addstrm out request is received, reply it with bad seqno if it's seqno < asoc->strreset_inseq - 2, and reply it with the result saved in asoc if it's seqno >= asoc->strreset_inseq - 2. Note that it saves last 2 results instead of only last 1 result, because two requests can be sent together in one chunk. And note that when receiving a duplicated request, the receiver side will still reply it even if the peer has received the response. It's safe, As the response will be dropped by the peer. Signed-off-by: Xin Long Signed-off-by: David S. Miller --- include/net/sctp/structs.h | 1 + net/sctp/stream.c | 39 ++++++++++++++++++++++++++++---------- 2 files changed, 30 insertions(+), 10 deletions(-) diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h index b751399aa6b7..a8b38e123f97 100644 --- a/include/net/sctp/structs.h +++ b/include/net/sctp/structs.h @@ -1889,6 +1889,7 @@ struct sctp_association { __u32 strreset_outseq; /* Update after receiving response */ __u32 strreset_inseq; /* Update after receiving request */ + __u32 strreset_result[2]; /* save the results of last 2 responses */ struct sctp_chunk *strreset_chunk; /* save request chunk */ diff --git a/net/sctp/stream.c b/net/sctp/stream.c index 4ec3679a1cef..6cab7c3721a3 100644 --- a/net/sctp/stream.c +++ b/net/sctp/stream.c @@ -344,6 +344,13 @@ static sctp_paramhdr_t *sctp_chunk_lookup_strreset_param( return NULL; } +static void sctp_update_strreset_result(struct sctp_association *asoc, + __u32 result) +{ + asoc->strreset_result[1] = asoc->strreset_result[0]; + asoc->strreset_result[0] = result; +} + struct sctp_chunk *sctp_process_strreset_outreq( struct sctp_association *asoc, union sctp_params param, @@ -360,15 +367,19 @@ struct sctp_chunk *sctp_process_strreset_outreq( if (ntohl(outreq->send_reset_at_tsn) > sctp_tsnmap_get_ctsn(&asoc->peer.tsn_map)) { result = SCTP_STRRESET_IN_PROGRESS; - goto out; + goto err; } - if (request_seq > asoc->strreset_inseq) { + if (TSN_lt(asoc->strreset_inseq, request_seq) || + TSN_lt(request_seq, asoc->strreset_inseq - 2)) { result = SCTP_STRRESET_ERR_BAD_SEQNO; - goto out; - } else if (request_seq == asoc->strreset_inseq) { - asoc->strreset_inseq++; + goto err; + } else if (TSN_lt(request_seq, asoc->strreset_inseq)) { + i = asoc->strreset_inseq - request_seq - 1; + result = asoc->strreset_result[i]; + goto err; } + asoc->strreset_inseq++; /* Check strreset_enable after inseq inc, as sender cannot tell * the peer doesn't enable strreset after receiving response with @@ -427,6 +438,8 @@ struct sctp_chunk *sctp_process_strreset_outreq( GFP_ATOMIC); out: + sctp_update_strreset_result(asoc, result); +err: return sctp_make_strreset_resp(asoc, result, request_seq); } @@ -582,15 +595,19 @@ struct sctp_chunk *sctp_process_strreset_addstrm_out( __u32 result = SCTP_STRRESET_DENIED; struct sctp_stream_in *streamin; __u32 request_seq, incnt; - __u16 in; + __u16 in, i; request_seq = ntohl(addstrm->request_seq); - if (request_seq > asoc->strreset_inseq) { + if (TSN_lt(asoc->strreset_inseq, request_seq) || + TSN_lt(request_seq, asoc->strreset_inseq - 2)) { result = SCTP_STRRESET_ERR_BAD_SEQNO; - goto out; - } else if (request_seq == asoc->strreset_inseq) { - asoc->strreset_inseq++; + goto err; + } else if (TSN_lt(request_seq, asoc->strreset_inseq)) { + i = asoc->strreset_inseq - request_seq - 1; + result = asoc->strreset_result[i]; + goto err; } + asoc->strreset_inseq++; if (!(asoc->strreset_enable & SCTP_ENABLE_CHANGE_ASSOC_REQ)) goto out; @@ -638,6 +655,8 @@ struct sctp_chunk *sctp_process_strreset_addstrm_out( 0, ntohs(addstrm->number_of_streams), 0, GFP_ATOMIC); out: + sctp_update_strreset_result(asoc, result); +err: return sctp_make_strreset_resp(asoc, result, request_seq); } From d0f025e611581169c81e3c0fc44b5133d12748dd Mon Sep 17 00:00:00 2001 From: Xin Long Date: Sat, 15 Apr 2017 22:00:28 +0800 Subject: [PATCH 2/3] sctp: process duplicated strreset in and addstrm in requests correctly This patch is to fix the replay attack issue for strreset and addstrm in requests. When a duplicated strreset in or addstrm in request is received, reply it with bad seqno if it's seqno < asoc->strreset_inseq - 2, and reply it with the result saved in asoc if it's seqno >= asoc->strreset_inseq - 2. For strreset in or addstrm in request, if the receiver side processes it successfully, a strreset out or addstrm out request(as a response for that request) will be sent back to peer. reconf_time will retransmit the out request even if it's lost. So when receiving a duplicated strreset in or addstrm in request and it's result was performed, it shouldn't reply this request, but drop it instead. Signed-off-by: Xin Long Signed-off-by: David S. Miller --- net/sctp/stream.c | 38 +++++++++++++++++++++++++++++--------- 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/net/sctp/stream.c b/net/sctp/stream.c index 6cab7c3721a3..c91d97e5d590 100644 --- a/net/sctp/stream.c +++ b/net/sctp/stream.c @@ -456,12 +456,18 @@ struct sctp_chunk *sctp_process_strreset_inreq( __u32 request_seq; request_seq = ntohl(inreq->request_seq); - if (request_seq > asoc->strreset_inseq) { + if (TSN_lt(asoc->strreset_inseq, request_seq) || + TSN_lt(request_seq, asoc->strreset_inseq - 2)) { result = SCTP_STRRESET_ERR_BAD_SEQNO; - goto out; - } else if (request_seq == asoc->strreset_inseq) { - asoc->strreset_inseq++; + goto err; + } else if (TSN_lt(request_seq, asoc->strreset_inseq)) { + i = asoc->strreset_inseq - request_seq - 1; + result = asoc->strreset_result[i]; + if (result == SCTP_STRRESET_PERFORMED) + return NULL; + goto err; } + asoc->strreset_inseq++; if (!(asoc->strreset_enable & SCTP_ENABLE_RESET_STREAM_REQ)) goto out; @@ -496,10 +502,14 @@ struct sctp_chunk *sctp_process_strreset_inreq( asoc->strreset_outstanding = 1; sctp_chunk_hold(asoc->strreset_chunk); + result = SCTP_STRRESET_PERFORMED; + *evp = sctp_ulpevent_make_stream_reset_event(asoc, SCTP_STREAM_RESET_INCOMING_SSN, nums, str_p, GFP_ATOMIC); out: + sctp_update_strreset_result(asoc, result); +err: if (!chunk) chunk = sctp_make_strreset_resp(asoc, result, request_seq); @@ -671,15 +681,21 @@ struct sctp_chunk *sctp_process_strreset_addstrm_in( struct sctp_stream_out *streamout; struct sctp_chunk *chunk = NULL; __u32 request_seq, outcnt; - __u16 out; + __u16 out, i; request_seq = ntohl(addstrm->request_seq); - if (request_seq > asoc->strreset_inseq) { + if (TSN_lt(asoc->strreset_inseq, request_seq) || + TSN_lt(request_seq, asoc->strreset_inseq - 2)) { result = SCTP_STRRESET_ERR_BAD_SEQNO; - goto out; - } else if (request_seq == asoc->strreset_inseq) { - asoc->strreset_inseq++; + goto err; + } else if (TSN_lt(request_seq, asoc->strreset_inseq)) { + i = asoc->strreset_inseq - request_seq - 1; + result = asoc->strreset_result[i]; + if (result == SCTP_STRRESET_PERFORMED) + return NULL; + goto err; } + asoc->strreset_inseq++; if (!(asoc->strreset_enable & SCTP_ENABLE_CHANGE_ASSOC_REQ)) goto out; @@ -712,10 +728,14 @@ struct sctp_chunk *sctp_process_strreset_addstrm_in( stream->outcnt = outcnt; + result = SCTP_STRRESET_PERFORMED; + *evp = sctp_ulpevent_make_stream_change_event(asoc, 0, 0, ntohs(addstrm->number_of_streams), GFP_ATOMIC); out: + sctp_update_strreset_result(asoc, result); +err: if (!chunk) chunk = sctp_make_strreset_resp(asoc, result, request_seq); From 6c80138773efff75ee9598b4ebcd7aa0e3a5a2a3 Mon Sep 17 00:00:00 2001 From: Xin Long Date: Sat, 15 Apr 2017 22:00:29 +0800 Subject: [PATCH 3/3] sctp: process duplicated strreset asoc request correctly This patch is to fix the replay attack issue for strreset asoc requests. When a duplicated strreset asoc request is received, reply it with bad seqno if it's seqno < asoc->strreset_inseq - 2, and reply it with the result saved in asoc if it's seqno >= asoc->strreset_inseq - 2. But note that if the result saved in asoc is performed, the sender's next tsn and receiver's next tsn for the response chunk should be set. It's safe to get them from asoc. Because if it's changed, which means the peer has received the response already, the new response with wrong tsn won't be accepted by peer. Signed-off-by: Xin Long Signed-off-by: David S. Miller --- net/sctp/stream.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/net/sctp/stream.c b/net/sctp/stream.c index c91d97e5d590..dda53a293986 100644 --- a/net/sctp/stream.c +++ b/net/sctp/stream.c @@ -529,12 +529,21 @@ struct sctp_chunk *sctp_process_strreset_tsnreq( __u16 i; request_seq = ntohl(tsnreq->request_seq); - if (request_seq > asoc->strreset_inseq) { + if (TSN_lt(asoc->strreset_inseq, request_seq) || + TSN_lt(request_seq, asoc->strreset_inseq - 2)) { result = SCTP_STRRESET_ERR_BAD_SEQNO; - goto out; - } else if (request_seq == asoc->strreset_inseq) { - asoc->strreset_inseq++; + goto err; + } else if (TSN_lt(request_seq, asoc->strreset_inseq)) { + i = asoc->strreset_inseq - request_seq - 1; + result = asoc->strreset_result[i]; + if (result == SCTP_STRRESET_PERFORMED) { + next_tsn = asoc->next_tsn; + init_tsn = + sctp_tsnmap_get_ctsn(&asoc->peer.tsn_map) + 1; + } + goto err; } + asoc->strreset_inseq++; if (!(asoc->strreset_enable & SCTP_ENABLE_RESET_ASSOC_REQ)) goto out; @@ -591,6 +600,8 @@ struct sctp_chunk *sctp_process_strreset_tsnreq( next_tsn, GFP_ATOMIC); out: + sctp_update_strreset_result(asoc, result); +err: return sctp_make_strreset_tsnresp(asoc, result, request_seq, next_tsn, init_tsn); }