sctp: fix to check the source address of COOKIE-ECHO
Subsystems (2)
PATCH 1/2 sctp: fix to check the source address of COOKIE-ECHO
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.
--- include/net/sctp/constants.h | 1 + net/sctp/sm_make_chunk.c | 28 ++++++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 0 deletions(-)
include/net/sctp/constants.h +1 -0
@@ -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,
net/sctp/sm_make_chunk.c +28 -0
@@ -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.
Signature · 1 line
1.6.5.2
Hi Wei
Quoted 7 lines · Wei Yongjun
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
Quoted 64 lines
Signed-off-by Wei Yongjun <[email protected]>--- include/net/sctp/constants.h | 1 + net/sctp/sm_make_chunk.c | 28 ++++++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 0 deletions(-)include/net/sctp/constants.h +1
-0@@ -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,net/sctp/sm_make_chunk.c +28
-0@@ -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.
Quoted 13 lines
Hi Wei
Quoted 8 lines · Wei Yongjun
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.
Quoted 6 lines
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.
Quoted 72 lines
With this patch, we will start rejecting associations with such implementations.
-vlad
Quoted 65 lines
Signed-off-by Wei Yongjun <[email protected]>--- include/net/sctp/constants.h | 1 + net/sctp/sm_make_chunk.c | 28 ++++++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 0 deletions(-)include/net/sctp/constants.h +1
-0@@ -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,net/sctp/sm_make_chunk.c +28
-0@@ -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.
Quoted 19 lines · Wei Yongjun
Quoted 12 lines
Hi Wei
Quoted 8 lines · Wei Yongjun
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.
Quoted 6 lines
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.
Quoted 8 lines
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
Quoted 73 lines
Quoted 72 lines
With this patch, we will start rejecting associations with such implementations.
-vlad
Quoted 65 lines
Signed-off-by Wei Yongjun <[email protected]>--- include/net/sctp/constants.h | 1 + net/sctp/sm_make_chunk.c | 28 ++++++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 0 deletions(-)include/net/sctp/constants.h +1
-0@@ -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,net/sctp/sm_make_chunk.c +28
-0@@ -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.
Quoted 39 lines
Quoted 21 lines · Wei Yongjun
Quoted 14 lines
Hi Wei
Quoted 9 lines · Wei Yongjun
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.
Quoted 7 lines
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.
Quoted 9 lines
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.
Quoted 7 lines
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.
Quoted 82 lines
-vlad
Quoted 76 lines
Quoted 75 lines
With this patch, we will start rejecting associations with such implementations.
-vlad
Quoted 66 lines
Signed-off-by Wei Yongjun <[email protected]>--- include/net/sctp/constants.h | 1 + net/sctp/sm_make_chunk.c | 28 ++++++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 0 deletions(-)include/net/sctp/constants.h +1
-0@@ -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,net/sctp/sm_make_chunk.c +28
-0@@ -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.