On Sat, May 5, 2018 at 6:33 AM, Marcelo Ricardo Leitner
<marcelo.leitner@xxxxxxxxx> wrote:
> On Fri, May 04, 2018 at 05:05:10PM +0800, Xin Long wrote:
>> Now sctp only delays the authentication for the normal cookie-echo
>> chunk by setting chunk->auth_chunk in sctp_endpoint_bh_rcv(). But
>> for the duplicated one with auth, in sctp_assoc_bh_rcv(), it does
>> authentication first based on the old asoc, which will definitely
>> fail due to the different auth info in the old asoc.
>>
>> The duplicated cookie-echo chunk will create a new asoc with the
>> auth info from this chunk, and the authentication should also be
>> done with the new asoc's auth info for all of the collision 'A',
>> 'B' and 'D'. Otherwise, the duplicated cookie-echo chunk with auth
>> will never pass the authentication and create the new connection.
>>
>> This issue exists since very beginning, and this fix is to make
>> sctp_assoc_bh_rcv() follow the way sctp_assoc_bh_rcv() does for
> I guess you meant sctp_endpoint_bh_rcv here --^ right?
have posted v2, thanks.
>
> Other than this LGTM
>
>> the normal cookie-echo chunk to delay the authentication.
>>
>> While at it, remove the unused params from sctp_sf_authenticate()
>> and define sctp_auth_chunk_verify() used for all the places that
>> do the delayed authentication.
>>
>> Signed-off-by: Xin Long <lucien.xin@xxxxxxxxx>
>> ---
>> net/sctp/associola.c | 30 ++++++++++++++++-
>> net/sctp/sm_statefuns.c | 86 ++++++++++++++++++++++++++-----------------------
>> 2 files changed, 75 insertions(+), 41 deletions(-)
>>
>> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
>> index 837806d..a47179d 100644
>> --- a/net/sctp/associola.c
>> +++ b/net/sctp/associola.c
>> @@ -1024,8 +1024,9 @@ static void sctp_assoc_bh_rcv(struct work_struct *work)
>> struct sctp_endpoint *ep;
>> struct sctp_chunk *chunk;
>> struct sctp_inq *inqueue;
>> - int state;
>> + int first_time = 1; /* is this the first time through the loop */
>> int error = 0;
>> + int state;
>>
>> /* The association should be held so we should be safe. */
>> ep = asoc->ep;
>> @@ -1036,6 +1037,30 @@ static void sctp_assoc_bh_rcv(struct work_struct *work)
>> state = asoc->state;
>> subtype = SCTP_ST_CHUNK(chunk->chunk_hdr->type);
>>
>> + /* If the first chunk in the packet is AUTH, do special
>> + * processing specified in Section 6.3 of SCTP-AUTH spec
>> + */
>> + if (first_time && subtype.chunk == SCTP_CID_AUTH) {
>> + struct sctp_chunkhdr *next_hdr;
>> +
>> + next_hdr = sctp_inq_peek(inqueue);
>> + if (!next_hdr)
>> + goto normal;
>> +
>> + /* If the next chunk is COOKIE-ECHO, skip the AUTH
>> + * chunk while saving a pointer to it so we can do
>> + * Authentication later (during cookie-echo
>> + * processing).
>> + */
>> + if (next_hdr->type == SCTP_CID_COOKIE_ECHO) {
>> + chunk->auth_chunk = skb_clone(chunk->skb,
>> + GFP_ATOMIC);
>> + chunk->auth = 1;
>> + continue;
>> + }
>> + }
>> +
>> +normal:
>> /* SCTP-AUTH, Section 6.3:
>> * The receiver has a list of chunk types which it expects
>> * to be received only after an AUTH-chunk. This list has
>> @@ -1074,6 +1099,9 @@ static void sctp_assoc_bh_rcv(struct work_struct *work)
>> /* If there is an error on chunk, discard this packet. */
>> if (error && chunk)
>> chunk->pdiscard = 1;
>> +
>> + if (first_time)
>> + first_time = 0;
>> }
>> sctp_association_put(asoc);
>> }
>> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
>> index 28c070e..c9ae340 100644
>> --- a/net/sctp/sm_statefuns.c
>> +++ b/net/sctp/sm_statefuns.c
>> @@ -153,10 +153,7 @@ static enum sctp_disposition sctp_sf_violation_chunk(
>> struct sctp_cmd_seq *commands);
>>
>> static enum sctp_ierror sctp_sf_authenticate(
>> - struct net *net,
>> - const struct sctp_endpoint *ep,
>> const struct sctp_association *asoc,
>> - const union sctp_subtype type,
>> struct sctp_chunk *chunk);
>>
>> static enum sctp_disposition __sctp_sf_do_9_1_abort(
>> @@ -626,6 +623,38 @@ enum sctp_disposition sctp_sf_do_5_1C_ack(struct net *net,
>> return SCTP_DISPOSITION_CONSUME;
>> }
>>
>> +static bool sctp_auth_chunk_verify(struct net *net, struct sctp_chunk *chunk,
>> + const struct sctp_association *asoc)
>> +{
>> + struct sctp_chunk auth;
>> +
>> + if (!chunk->auth_chunk)
>> + return true;
>> +
>> + /* SCTP-AUTH: auth_chunk pointer is only set when the cookie-echo
>> + * is supposed to be authenticated and we have to do delayed
>> + * authentication. We've just recreated the association using
>> + * the information in the cookie and now it's much easier to
>> + * do the authentication.
>> + */
>> +
>> + /* Make sure that we and the peer are AUTH capable */
>> + if (!net->sctp.auth_enable || !asoc->peer.auth_capable)
>> + return false;
>> +
>> + /* set-up our fake chunk so that we can process it */
>> + auth.skb = chunk->auth_chunk;
>> + auth.asoc = chunk->asoc;
>> + auth.sctp_hdr = chunk->sctp_hdr;
>> + auth.chunk_hdr = (struct sctp_chunkhdr *)
>> + skb_push(chunk->auth_chunk,
>> + sizeof(struct sctp_chunkhdr));
>> + skb_pull(chunk->auth_chunk, sizeof(struct sctp_chunkhdr));
>> + auth.transport = chunk->transport;
>> +
>> + return sctp_sf_authenticate(asoc, &auth) == SCTP_IERROR_NO_ERROR;
>> +}
>> +
>> /*
>> * Respond to a normal COOKIE ECHO chunk.
>> * We are the side that is being asked for an association.
>> @@ -763,37 +792,9 @@ enum sctp_disposition sctp_sf_do_5_1D_ce(struct net *net,
>> if (error)
>> goto nomem_init;
>>
>> - /* SCTP-AUTH: auth_chunk pointer is only set when the cookie-echo
>> - * is supposed to be authenticated and we have to do delayed
>> - * authentication. We've just recreated the association using
>> - * the information in the cookie and now it's much easier to
>> - * do the authentication.
>> - */
>> - if (chunk->auth_chunk) {
>> - struct sctp_chunk auth;
>> - enum sctp_ierror ret;
>> -
>> - /* Make sure that we and the peer are AUTH capable */
>> - if (!net->sctp.auth_enable || !new_asoc->peer.auth_capable) {
>> - sctp_association_free(new_asoc);
>> - return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands);
>> - }
>> -
>> - /* set-up our fake chunk so that we can process it */
>> - auth.skb = chunk->auth_chunk;
>> - auth.asoc = chunk->asoc;
>> - auth.sctp_hdr = chunk->sctp_hdr;
>> - auth.chunk_hdr = (struct sctp_chunkhdr *)
>> - skb_push(chunk->auth_chunk,
>> - sizeof(struct sctp_chunkhdr));
>> - skb_pull(chunk->auth_chunk, sizeof(struct sctp_chunkhdr));
>> - auth.transport = chunk->transport;
>> -
>> - ret = sctp_sf_authenticate(net, ep, new_asoc, type, &auth);
>> - if (ret != SCTP_IERROR_NO_ERROR) {
>> - sctp_association_free(new_asoc);
>> - return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands);
>> - }
>> + if (!sctp_auth_chunk_verify(net, chunk, new_asoc)) {
>> + sctp_association_free(new_asoc);
>> + return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands);
>> }
>>
>> repl = sctp_make_cookie_ack(new_asoc, chunk);
>> @@ -1797,13 +1798,15 @@ static enum sctp_disposition sctp_sf_do_dupcook_a(
>> if (sctp_auth_asoc_init_active_key(new_asoc, GFP_ATOMIC))
>> goto nomem;
>>
>> + if (!sctp_auth_chunk_verify(net, chunk, new_asoc))
>> + return SCTP_DISPOSITION_DISCARD;
>> +
>> /* Make sure no new addresses are being added during the
>> * restart. Though this is a pretty complicated attack
>> * since you'd have to get inside the cookie.
>> */
>> - if (!sctp_sf_check_restart_addrs(new_asoc, asoc, chunk, commands)) {
>> + if (!sctp_sf_check_restart_addrs(new_asoc, asoc, chunk, commands))
>> return SCTP_DISPOSITION_CONSUME;
>> - }
>>
>> /* If the endpoint is in the SHUTDOWN-ACK-SENT state and recognizes
>> * the peer has restarted (Action A), it MUST NOT setup a new
>> @@ -1912,6 +1915,9 @@ static enum sctp_disposition sctp_sf_do_dupcook_b(
>> if (sctp_auth_asoc_init_active_key(new_asoc, GFP_ATOMIC))
>> goto nomem;
>>
>> + if (!sctp_auth_chunk_verify(net, chunk, new_asoc))
>> + return SCTP_DISPOSITION_DISCARD;
>> +
>> /* Update the content of current association. */
>> sctp_add_cmd_sf(commands, SCTP_CMD_UPDATE_ASSOC, SCTP_ASOC(new_asoc));
>> sctp_add_cmd_sf(commands, SCTP_CMD_NEW_STATE,
>> @@ -2009,6 +2015,9 @@ static enum sctp_disposition sctp_sf_do_dupcook_d(
>> * a COOKIE ACK.
>> */
>>
>> + if (!sctp_auth_chunk_verify(net, chunk, asoc))
>> + return SCTP_DISPOSITION_DISCARD;
>> +
>> /* Don't accidentally move back into established state. */
>> if (asoc->state < SCTP_STATE_ESTABLISHED) {
>> sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_STOP,
>> @@ -4171,10 +4180,7 @@ enum sctp_disposition sctp_sf_eat_fwd_tsn_fast(
>> * The return value is the disposition of the chunk.
>> */
>> static enum sctp_ierror sctp_sf_authenticate(
>> - struct net *net,
>> - const struct sctp_endpoint *ep,
>> const struct sctp_association *asoc,
>> - const union sctp_subtype type,
>> struct sctp_chunk *chunk)
>> {
>> struct sctp_shared_key *sh_key = NULL;
>> @@ -4275,7 +4281,7 @@ enum sctp_disposition sctp_sf_eat_auth(struct net *net,
>> commands);
>>
>> auth_hdr = (struct sctp_authhdr *)chunk->skb->data;
>> - error = sctp_sf_authenticate(net, ep, asoc, type, chunk);
>> + error = sctp_sf_authenticate(asoc, chunk);
>> switch (error) {
>> case SCTP_IERROR_AUTH_BAD_HMAC:
>> /* Generate the ERROR chunk and discard the rest
>> --
>> 2.1.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
