* Re: [PATCH 1/2] sctp: fix to check the source address of COOKIE-ECHO
2010-04-23 10:20 [PATCH 1/2] sctp: fix to check the source address of COOKIE-ECHO Wei Yongjun
@ 2010-04-23 14:12 ` Vlad Yasevich
2010-04-26 4:56 ` Wei Yongjun
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Vlad Yasevich @ 2010-04-23 14:12 UTC (permalink / raw)
To: linux-sctp
Hi Wei
Wei Yongjun wrote:
> SCTP does not check whether the source address of COOKIE-ECHO
> chunk is part of the any address parameters saved in COOKIE in
> CLOSED state. So even if the COOKIE-ECHO chunk other address
> with correct COOKIE, the COOKIE-ECHO chunk still be accepted.
> If the source does not match any address parameters saved in
> COOKIE, the COOKIE ECHO chunk should be silently discarded.
>
I don't think this is correct. An implementation is not required to specify
the source address in the address parameters list. The way most
of rfc4960 is written, the combination of source and address parameters
is used to construct the transports. There is no requirement that
the source should also be listed.
With this patch, we will start rejecting associations with such implementations.
-vlad
> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
> ---
> include/net/sctp/constants.h | 1 +
> net/sctp/sm_make_chunk.c | 28 ++++++++++++++++++++++++++++
> 2 files changed, 29 insertions(+), 0 deletions(-)
>
> diff --git a/include/net/sctp/constants.h b/include/net/sctp/constants.h
> index 6390884..288f2b7 100644
> --- a/include/net/sctp/constants.h
> +++ b/include/net/sctp/constants.h
> @@ -183,6 +183,7 @@ typedef enum {
> SCTP_IERROR_NO_DATA,
> SCTP_IERROR_BAD_STREAM,
> SCTP_IERROR_BAD_PORTS,
> + SCTP_IERROR_BAD_ADDR,
> SCTP_IERROR_AUTH_BAD_HMAC,
> SCTP_IERROR_AUTH_BAD_KEYID,
> SCTP_IERROR_PROTO_VIOLATION,
> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index 17cb400..bc7ac37 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -1591,6 +1591,7 @@ struct sctp_association *sctp_unpack_cookie(
> struct sk_buff *skb = chunk->skb;
> struct timeval tv;
> struct hash_desc desc;
> + union sctp_params param;
>
> /* Header size is static data prior to the actual cookie, including
> * any padding.
> @@ -1670,6 +1671,33 @@ no_hmac:
> goto fail;
> }
>
> + /* Check whether the source address of COOKIE ECHO chunk is part
> + * of the any address parameters. If the value does not match, the
> + * COOKIE ECHO chunk MUST be silently discarded.
> + */
> + if (asoc || sctp_cmp_addr_exact(sctp_source(chunk),
> + &bear_cookie->peer_addr))
> + goto addr_match;
> +
> + sctp_walk_params(param, &bear_cookie->peer_init[0], init_hdr.params) {
> + if (param.p->type = SCTP_PARAM_IPV4_ADDRESS ||
> + param.p->type = SCTP_PARAM_IPV6_ADDRESS) {
> + struct sctp_af *af;
> + union sctp_addr addr;
> +
> + af = sctp_get_af_specific(param_type2af(param.p->type));
> + af->from_addr_param(&addr, param.addr,
> + chunk->sctp_hdr->source, 0);
> +
> + if (sctp_cmp_addr_exact(sctp_source(chunk), &addr))
> + goto addr_match;
> + }
> + }
> +
> + *error = -SCTP_IERROR_BAD_ADDR;
> + goto fail;
> +
> +addr_match:
> /* Check to see if the cookie is stale. If there is already
> * an association, there is no need to check cookie's expiration
> * for init collision case of lost COOKIE ACK.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 1/2] sctp: fix to check the source address of COOKIE-ECHO
2010-04-23 10:20 [PATCH 1/2] sctp: fix to check the source address of COOKIE-ECHO Wei Yongjun
2010-04-23 14:12 ` Vlad Yasevich
@ 2010-04-26 4:56 ` Wei Yongjun
2010-04-26 14:42 ` Vlad Yasevich
2010-04-28 3:53 ` Wei Yongjun
3 siblings, 0 replies; 5+ messages in thread
From: Wei Yongjun @ 2010-04-26 4:56 UTC (permalink / raw)
To: linux-sctp
> Hi Wei
>
> Wei Yongjun wrote:
>
>> SCTP does not check whether the source address of COOKIE-ECHO
>> chunk is part of the any address parameters saved in COOKIE in
>> CLOSED state. So even if the COOKIE-ECHO chunk other address
>> with correct COOKIE, the COOKIE-ECHO chunk still be accepted.
>> If the source does not match any address parameters saved in
>> COOKIE, the COOKIE ECHO chunk should be silently discarded.
>>
>>
> I don't think this is correct. An implementation is not required to specify
> the source address in the address parameters list. The way most
> of rfc4960 is written, the combination of source and address parameters
> is used to construct the transports. There is no requirement that
> the source should also be listed.
>
Oh, my real mean is that the source should either be the source address
of the
INIT chunk or any addtess be list in the address parameters list. If It
is not any
of them, the asoc is started with new address? This is not a big
problem, but
endpoint can now accept COOKIE-ECHO with *any source address* plus correct
COOKIE in CLOSE state.
> With this patch, we will start rejecting associations with such implementations.
>
> -vlad
>
>
>> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
>> ---
>> include/net/sctp/constants.h | 1 +
>> net/sctp/sm_make_chunk.c | 28 ++++++++++++++++++++++++++++
>> 2 files changed, 29 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/net/sctp/constants.h b/include/net/sctp/constants.h
>> index 6390884..288f2b7 100644
>> --- a/include/net/sctp/constants.h
>> +++ b/include/net/sctp/constants.h
>> @@ -183,6 +183,7 @@ typedef enum {
>> SCTP_IERROR_NO_DATA,
>> SCTP_IERROR_BAD_STREAM,
>> SCTP_IERROR_BAD_PORTS,
>> + SCTP_IERROR_BAD_ADDR,
>> SCTP_IERROR_AUTH_BAD_HMAC,
>> SCTP_IERROR_AUTH_BAD_KEYID,
>> SCTP_IERROR_PROTO_VIOLATION,
>> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
>> index 17cb400..bc7ac37 100644
>> --- a/net/sctp/sm_make_chunk.c
>> +++ b/net/sctp/sm_make_chunk.c
>> @@ -1591,6 +1591,7 @@ struct sctp_association *sctp_unpack_cookie(
>> struct sk_buff *skb = chunk->skb;
>> struct timeval tv;
>> struct hash_desc desc;
>> + union sctp_params param;
>>
>> /* Header size is static data prior to the actual cookie, including
>> * any padding.
>> @@ -1670,6 +1671,33 @@ no_hmac:
>> goto fail;
>> }
>>
>> + /* Check whether the source address of COOKIE ECHO chunk is part
>> + * of the any address parameters. If the value does not match, the
>> + * COOKIE ECHO chunk MUST be silently discarded.
>> + */
>> + if (asoc || sctp_cmp_addr_exact(sctp_source(chunk),
>> + &bear_cookie->peer_addr))
>> + goto addr_match;
>> +
>> + sctp_walk_params(param, &bear_cookie->peer_init[0], init_hdr.params) {
>> + if (param.p->type = SCTP_PARAM_IPV4_ADDRESS ||
>> + param.p->type = SCTP_PARAM_IPV6_ADDRESS) {
>> + struct sctp_af *af;
>> + union sctp_addr addr;
>> +
>> + af = sctp_get_af_specific(param_type2af(param.p->type));
>> + af->from_addr_param(&addr, param.addr,
>> + chunk->sctp_hdr->source, 0);
>> +
>> + if (sctp_cmp_addr_exact(sctp_source(chunk), &addr))
>> + goto addr_match;
>> + }
>> + }
>> +
>> + *error = -SCTP_IERROR_BAD_ADDR;
>> + goto fail;
>> +
>> +addr_match:
>> /* Check to see if the cookie is stale. If there is already
>> * an association, there is no need to check cookie's expiration
>> * for init collision case of lost COOKIE ACK.
>>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 1/2] sctp: fix to check the source address of COOKIE-ECHO
2010-04-23 10:20 [PATCH 1/2] sctp: fix to check the source address of COOKIE-ECHO Wei Yongjun
2010-04-23 14:12 ` Vlad Yasevich
2010-04-26 4:56 ` Wei Yongjun
@ 2010-04-26 14:42 ` Vlad Yasevich
2010-04-28 3:53 ` Wei Yongjun
3 siblings, 0 replies; 5+ messages in thread
From: Vlad Yasevich @ 2010-04-26 14:42 UTC (permalink / raw)
To: linux-sctp
Wei Yongjun wrote:
>> Hi Wei
>>
>> Wei Yongjun wrote:
>>
>>> SCTP does not check whether the source address of COOKIE-ECHO
>>> chunk is part of the any address parameters saved in COOKIE in
>>> CLOSED state. So even if the COOKIE-ECHO chunk other address
>>> with correct COOKIE, the COOKIE-ECHO chunk still be accepted.
>>> If the source does not match any address parameters saved in
>>> COOKIE, the COOKIE ECHO chunk should be silently discarded.
>>>
>>>
>> I don't think this is correct. An implementation is not required to specify
>> the source address in the address parameters list. The way most
>> of rfc4960 is written, the combination of source and address parameters
>> is used to construct the transports. There is no requirement that
>> the source should also be listed.
>>
>
> Oh, my real mean is that the source should either be the source address
> of the
> INIT chunk or any addtess be list in the address parameters list. If It
> is not any
> of them, the asoc is started with new address? This is not a big
> problem, but
> endpoint can now accept COOKIE-ECHO with *any source address* plus correct
> COOKIE in CLOSE state.
I believe that's expected. We prevent adding new addresses on restart, but
if you start from closed, this would work.
This would be a very hard attack to pull off since the attacker would either
have to be on-path, or have extremely detailed information about association.
I just checked BSD sources and they appear to handle this case the same way.
However, having said that, I originally missed one of the conditions that I thought
would cause the failure. So, it looks like this would actually be a good check to add, but
I am wondering if we can do this in process_init. That way we don't have
to walk the parameter list yet another time.
-vlad
>
>> With this patch, we will start rejecting associations with such implementations.
>>
>> -vlad
>>
>>
>>> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
>>> ---
>>> include/net/sctp/constants.h | 1 +
>>> net/sctp/sm_make_chunk.c | 28 ++++++++++++++++++++++++++++
>>> 2 files changed, 29 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/include/net/sctp/constants.h b/include/net/sctp/constants.h
>>> index 6390884..288f2b7 100644
>>> --- a/include/net/sctp/constants.h
>>> +++ b/include/net/sctp/constants.h
>>> @@ -183,6 +183,7 @@ typedef enum {
>>> SCTP_IERROR_NO_DATA,
>>> SCTP_IERROR_BAD_STREAM,
>>> SCTP_IERROR_BAD_PORTS,
>>> + SCTP_IERROR_BAD_ADDR,
>>> SCTP_IERROR_AUTH_BAD_HMAC,
>>> SCTP_IERROR_AUTH_BAD_KEYID,
>>> SCTP_IERROR_PROTO_VIOLATION,
>>> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
>>> index 17cb400..bc7ac37 100644
>>> --- a/net/sctp/sm_make_chunk.c
>>> +++ b/net/sctp/sm_make_chunk.c
>>> @@ -1591,6 +1591,7 @@ struct sctp_association *sctp_unpack_cookie(
>>> struct sk_buff *skb = chunk->skb;
>>> struct timeval tv;
>>> struct hash_desc desc;
>>> + union sctp_params param;
>>>
>>> /* Header size is static data prior to the actual cookie, including
>>> * any padding.
>>> @@ -1670,6 +1671,33 @@ no_hmac:
>>> goto fail;
>>> }
>>>
>>> + /* Check whether the source address of COOKIE ECHO chunk is part
>>> + * of the any address parameters. If the value does not match, the
>>> + * COOKIE ECHO chunk MUST be silently discarded.
>>> + */
>>> + if (asoc || sctp_cmp_addr_exact(sctp_source(chunk),
>>> + &bear_cookie->peer_addr))
>>> + goto addr_match;
>>> +
>>> + sctp_walk_params(param, &bear_cookie->peer_init[0], init_hdr.params) {
>>> + if (param.p->type = SCTP_PARAM_IPV4_ADDRESS ||
>>> + param.p->type = SCTP_PARAM_IPV6_ADDRESS) {
>>> + struct sctp_af *af;
>>> + union sctp_addr addr;
>>> +
>>> + af = sctp_get_af_specific(param_type2af(param.p->type));
>>> + af->from_addr_param(&addr, param.addr,
>>> + chunk->sctp_hdr->source, 0);
>>> +
>>> + if (sctp_cmp_addr_exact(sctp_source(chunk), &addr))
>>> + goto addr_match;
>>> + }
>>> + }
>>> +
>>> + *error = -SCTP_IERROR_BAD_ADDR;
>>> + goto fail;
>>> +
>>> +addr_match:
>>> /* Check to see if the cookie is stale. If there is already
>>> * an association, there is no need to check cookie's expiration
>>> * for init collision case of lost COOKIE ACK.
>>>
>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 1/2] sctp: fix to check the source address of COOKIE-ECHO
2010-04-23 10:20 [PATCH 1/2] sctp: fix to check the source address of COOKIE-ECHO Wei Yongjun
` (2 preceding siblings ...)
2010-04-26 14:42 ` Vlad Yasevich
@ 2010-04-28 3:53 ` Wei Yongjun
3 siblings, 0 replies; 5+ messages in thread
From: Wei Yongjun @ 2010-04-28 3:53 UTC (permalink / raw)
To: linux-sctp
> Wei Yongjun wrote:
>
>>> Hi Wei
>>>
>>> Wei Yongjun wrote:
>>>
>>>
>>>> SCTP does not check whether the source address of COOKIE-ECHO
>>>> chunk is part of the any address parameters saved in COOKIE in
>>>> CLOSED state. So even if the COOKIE-ECHO chunk other address
>>>> with correct COOKIE, the COOKIE-ECHO chunk still be accepted.
>>>> If the source does not match any address parameters saved in
>>>> COOKIE, the COOKIE ECHO chunk should be silently discarded.
>>>>
>>>>
>>>>
>>> I don't think this is correct. An implementation is not required to specify
>>> the source address in the address parameters list. The way most
>>> of rfc4960 is written, the combination of source and address parameters
>>> is used to construct the transports. There is no requirement that
>>> the source should also be listed.
>>>
>>>
>> Oh, my real mean is that the source should either be the source address
>> of the
>> INIT chunk or any addtess be list in the address parameters list. If It
>> is not any
>> of them, the asoc is started with new address? This is not a big
>> problem, but
>> endpoint can now accept COOKIE-ECHO with *any source address* plus correct
>> COOKIE in CLOSE state.
>>
> I believe that's expected. We prevent adding new addresses on restart, but
> if you start from closed, this would work.
>
> This would be a very hard attack to pull off since the attacker would either
> have to be on-path, or have extremely detailed information about association.
>
> I just checked BSD sources and they appear to handle this case the same way.
>
> However, having said that, I originally missed one of the conditions that I thought
> would cause the failure. So, it looks like this would actually be a good check to add, but
> I am wondering if we can do this in process_init. That way we don't have
> to walk the parameter list yet another time.
>
current process_init() has lost some information we need for this
check, such as the source address of the cookie-echo chunk. If
do this in process_init(), extra parameters should be passed to
it, and the cost may be the same as this method.
> -vlad
>
>
>>
>>> With this patch, we will start rejecting associations with such implementations.
>>>
>>> -vlad
>>>
>>>
>>>
>>>> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
>>>> ---
>>>> include/net/sctp/constants.h | 1 +
>>>> net/sctp/sm_make_chunk.c | 28 ++++++++++++++++++++++++++++
>>>> 2 files changed, 29 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/include/net/sctp/constants.h b/include/net/sctp/constants.h
>>>> index 6390884..288f2b7 100644
>>>> --- a/include/net/sctp/constants.h
>>>> +++ b/include/net/sctp/constants.h
>>>> @@ -183,6 +183,7 @@ typedef enum {
>>>> SCTP_IERROR_NO_DATA,
>>>> SCTP_IERROR_BAD_STREAM,
>>>> SCTP_IERROR_BAD_PORTS,
>>>> + SCTP_IERROR_BAD_ADDR,
>>>> SCTP_IERROR_AUTH_BAD_HMAC,
>>>> SCTP_IERROR_AUTH_BAD_KEYID,
>>>> SCTP_IERROR_PROTO_VIOLATION,
>>>> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
>>>> index 17cb400..bc7ac37 100644
>>>> --- a/net/sctp/sm_make_chunk.c
>>>> +++ b/net/sctp/sm_make_chunk.c
>>>> @@ -1591,6 +1591,7 @@ struct sctp_association *sctp_unpack_cookie(
>>>> struct sk_buff *skb = chunk->skb;
>>>> struct timeval tv;
>>>> struct hash_desc desc;
>>>> + union sctp_params param;
>>>>
>>>> /* Header size is static data prior to the actual cookie, including
>>>> * any padding.
>>>> @@ -1670,6 +1671,33 @@ no_hmac:
>>>> goto fail;
>>>> }
>>>>
>>>> + /* Check whether the source address of COOKIE ECHO chunk is part
>>>> + * of the any address parameters. If the value does not match, the
>>>> + * COOKIE ECHO chunk MUST be silently discarded.
>>>> + */
>>>> + if (asoc || sctp_cmp_addr_exact(sctp_source(chunk),
>>>> + &bear_cookie->peer_addr))
>>>> + goto addr_match;
>>>> +
>>>> + sctp_walk_params(param, &bear_cookie->peer_init[0], init_hdr.params) {
>>>> + if (param.p->type = SCTP_PARAM_IPV4_ADDRESS ||
>>>> + param.p->type = SCTP_PARAM_IPV6_ADDRESS) {
>>>> + struct sctp_af *af;
>>>> + union sctp_addr addr;
>>>> +
>>>> + af = sctp_get_af_specific(param_type2af(param.p->type));
>>>> + af->from_addr_param(&addr, param.addr,
>>>> + chunk->sctp_hdr->source, 0);
>>>> +
>>>> + if (sctp_cmp_addr_exact(sctp_source(chunk), &addr))
>>>> + goto addr_match;
>>>> + }
>>>> + }
>>>> +
>>>> + *error = -SCTP_IERROR_BAD_ADDR;
>>>> + goto fail;
>>>> +
>>>> +addr_match:
>>>> /* Check to see if the cookie is stale. If there is already
>>>> * an association, there is no need to check cookie's expiration
>>>> * for init collision case of lost COOKIE ACK.
>>>>
>>>>
>>>
>>>
>>
>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread