SCTP: Validate initiate tag and chunk type if verification

PATCH In review Wei Yongjun 3 participants 6 messages Jun 6, 2008 – Jun 10, 2008

Subsystems (2)

Networking SCTP protocol

lore

Share & tools

lore

Wei Yongjun

author

2008-06-06 07:23 UTC #0 lore raw

PATCH 1/1 SCTP: Validate initiate tag and chunk type if verification

This patch add to validate initiate tag and chunk type if verification tag is 0 when handling ICMP message.

RFC 4960, Appendix C. ICMP Handling

ICMP6) An implementation MUST validate that the Verification Tag contained in the ICMP message matches the Verification Tag of the peer. If the Verification Tag is not 0 and does NOT match, discard the ICMP message. If it is 0 and the ICMP message contains enough bytes to verify that the chunk type is an INIT chunk and that the Initiate Tag matches the tag of the peer, continue with ICMP7. If the ICMP message is too short or the chunk type or the Initiate Tag does not match, silently discard the packet.

Signed-off-by Wei Yongjun <[email protected]>

net/sctp/input.c +26 -1

@@ -430,6 +430,9 @@ struct sock *sctp_err_lookup(int family,
 	struct sock *sk = NULL;
 	struct sctp_association *asoc;
 	struct sctp_transport *transport = NULL;
+	struct sctp_init_chunk *chunkhdr;
+	__u32 vtag = ntohl(sctphdr->vtag);
+	int len = skb->len - ((void *)sctphdr - (void *)skb->data);
 
 	*app = NULL; *tpp = NULL;

@@ -451,7 +454,29 @@ struct sock *sctp_err_lookup(int family,
 
 	sk = asoc->base.sk;
 
-	if (ntohl(sctphdr->vtag) != asoc->c.peer_vtag) {
+	/* RFC 4960, Appendix C. ICMP Handling
+	 *
+	 * ICMP6) An implementation MUST validate that the Verification Tag
+	 * contained in the ICMP message matches the Verification Tag of
+	 * the peer.  If the Verification Tag is not 0 and does NOT
+	 * match, discard the ICMP message.  If it is 0 and the ICMP
+	 * message contains enough bytes to verify that the chunk type is
+	 * an INIT chunk and that the Initiate Tag matches the tag of the
+	 * peer, continue with ICMP7.  If the ICMP message is too short
+	 * or the chunk type or the Initiate Tag does not match, silently
+	 * discard the packet.
+	 */
+	if (vtag = 0) {
+		chunkhdr = (struct sctp_init_chunk *)((void *)sctphdr
+				+ sizeof(struct sctphdr));
+		if (len < sizeof(struct sctphdr) + sizeof(sctp_chunkhdr_t)
+			  + sizeof(__be32)
+		    || chunkhdr->chunk_hdr.type != SCTP_CID_INIT
+		    || ntohl(chunkhdr->init_hdr.init_tag) != asoc->c.my_vtag) {
+			ICMP_INC_STATS_BH(ICMP_MIB_INERRORS);
+			goto out;
+		}
+	} else if (vtag != asoc->c.peer_vtag) {
 		ICMP_INC_STATS_BH(ICMP_MIB_INERRORS);
 		goto out;
 	}

Neil Horman

2008-06-06 14:22 UTC #1 lore raw

Re: [PATCH] SCTP: Validate initiate tag and chunk type if

Quoted 62 lines · Wei Yongjun

This patch add to validate initiate tag and chunk type if verification tag is 0 when handling ICMP message.

RFC 4960, Appendix C. ICMP Handling

Signed-off-by Wei Yongjun <[email protected]>

net/sctp/input.c +26 -2

@@ -430,6 +430,9 @@ struct sock *sctp_err_lookup(int family,
 	struct sock *sk = NULL;
 	struct sctp_association *asoc;
 	struct sctp_transport *transport = NULL;
+	struct sctp_init_chunk *chunkhdr;
+	__u32 vtag = ntohl(sctphdr->vtag);
+	int len = skb->len - ((void *)sctphdr - (void *)skb->data);
 
 	*app = NULL; *tpp = NULL;

@@ -451,7 +454,29 @@ struct sock *sctp_err_lookup(int family,
 
 	sk = asoc->base.sk;
 
-	if (ntohl(sctphdr->vtag) != asoc->c.peer_vtag) {
+	/* RFC 4960, Appendix C. ICMP Handling
+	 *
+	 * ICMP6) An implementation MUST validate that the Verification Tag
+	 * contained in the ICMP message matches the Verification Tag of
+	 * the peer.  If the Verification Tag is not 0 and does NOT
+	 * match, discard the ICMP message.  If it is 0 and the ICMP
+	 * message contains enough bytes to verify that the chunk type is
+	 * an INIT chunk and that the Initiate Tag matches the tag of the
+	 * peer, continue with ICMP7.  If the ICMP message is too short
+	 * or the chunk type or the Initiate Tag does not match, silently
+	 * discard the packet.
+	 */
+	if (vtag = 0) {
+		chunkhdr = (struct sctp_init_chunk *)((void *)sctphdr
+				+ sizeof(struct sctphdr));
+		if (len < sizeof(struct sctphdr) + sizeof(sctp_chunkhdr_t)
+			  + sizeof(__be32)
+		    || chunkhdr->chunk_hdr.type != SCTP_CID_INIT
+		    || ntohl(chunkhdr->init_hdr.init_tag) != asoc->c.my_vtag) {
+			ICMP_INC_STATS_BH(ICMP_MIB_INERRORS);
+			goto out;
+		}
+	} else if (vtag != asoc->c.peer_vtag) {
 		ICMP_INC_STATS_BH(ICMP_MIB_INERRORS);
 		goto out;
 	}
 
 
 
--

Quoted 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

I see that the the above comments relate specifically to ICMP6, but sctp_err_lookup is common between ipv4 and ipv6 sctp. I assume the same verification requirement is common to v4 and v6 as well (despite the comment). I don't see how it can hurt to check in both cases, but I wanted to be certain.

Regards
Neil

Signature · 4 lines

/****************************************************
 * Neil Horman <[email protected]>
 * Software Engineer, Red Hat
 ****************************************************/

Vlad Yasevich

2008-06-06 14:31 UTC #2 lore raw

In reply to Wei Yongjun

Quoted 4 lines · Neil Horman

More majordomo info at http://vger.kernel.org/majordomo-info.html
I see that the the above comments relate specifically to ICMP6, but sctp_err_lookup is common between ipv4 and ipv6 sctp. I assume the same

Neil, ICMP6 is the just bullet 6 from the ICMP processing, not a reference to ICMPv6 protocol.

-vlad

Quoted 6 lines

verification requirement is common to v4 and v6 as well (despite the comment). I don't see how it can hurt to check in both cases, but I wanted to be certain.
Regards
Neil

Neil Horman

Acked-by on v1 1/1

2008-06-06 14:36 UTC #3 lore raw

Re: [PATCH] SCTP: Validate initiate tag and chunk type if

In reply to Wei Yongjun

Quoted 11 lines · Vlad Yasevich

Quoted 4 lines · Neil Horman
More majordomo info at http://vger.kernel.org/majordomo-info.html
I see that the the above comments relate specifically to ICMP6, but sctp_err_lookup is common between ipv4 and ipv6 sctp. I assume the same
Neil, ICMP6 is the just bullet 6 from the ICMP processing, not a reference to ICMPv6 protocol.
-vlad

Ahh, my fault, that makes much more sense. Thanks!

Acked-by Neil Horman <[email protected]>

Quoted 11 lines

Quoted 6 lines
verification requirement is common to v4 and v6 as well (despite the comment). I don't see how it can hurt to check in both cases, but I wanted to be certain.
Regards
Neil
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 4 lines

/****************************************************
 * Neil Horman <[email protected]>
 * Software Engineer, Red Hat
 ****************************************************/

Vlad Yasevich

2008-06-06 14:52 UTC #4 lore raw

In reply to Wei Yongjun

Quoted 42 lines · Wei Yongjun

This patch add to validate initiate tag and chunk type if verification tag is 0 when handling ICMP message.
RFC 4960, Appendix C. ICMP Handling
ICMP6) An implementation MUST validate that the Verification Tag contained in the ICMP message matches the Verification Tag of the peer. If the Verification Tag is not 0 and does NOT match, discard the ICMP message. If it is 0 and the ICMP message contains enough bytes to verify that the chunk type is an INIT chunk and that the Initiate Tag matches the tag of the peer, continue with ICMP7. If the ICMP message is too short or the chunk type or the Initiate Tag does not match, silently discard the packet.
Signed-off-by Wei Yongjun <[email protected]>
net/sctp/input.c 2008-06-01 04:23:27.000000000 -0400 renamed from net/sctp/input.c 2008-05-31 23:49:24.000000000 -0400 +12 -1
@@ -430,6 +430,9 @@ struct sock *sctp_err_lookup(int family,
    struct sock *sk = NULL;
    struct sctp_association *asoc;
    struct sctp_transport *transport = NULL;
+    struct sctp_init_chunk *chunkhdr;
+    __u32 vtag = ntohl(sctphdr->vtag);
+    int len = skb->len - ((void *)sctphdr - (void *)skb->data);
 
    *app = NULL; *tpp = NULL;
 
@@ -451,7 +454,29 @@ struct sock *sctp_err_lookup(int family,
 
    sk = asoc->base.sk;
 
-    if (ntohl(sctphdr->vtag) != asoc->c.peer_vtag) {
+    /* RFC 4960, Appendix C. ICMP Handling
+     *
+     * ICMP6) An implementation MUST validate that the Verification Tag
+     * contained in the ICMP message matches the Verification Tag of
+     * the peer.  If the Verification Tag is not 0 and does NOT
+     * match, discard the ICMP message.  If it is 0 and the ICMP
+     * message contains enough bytes to verify that the chunk type is
+     * an INIT chunk and that the Initiate Tag matches the tag of the
+     * peer, continue with ICMP7.  If the ICMP message is too short

net/sctp/input.c 2008-06-01 04:23:27.000000000 -0400 struct sock *sctp_err_lookup(int family, · line 475

Quoted 10 lines

net/sctp/input.c 2008-06-01 04:23:27.000000000 -0400 renamed from net/sctp/input.c 2008-05-31 23:49:24.000000000 -0400 +10 -0

+     * or the chunk type or the Initiate Tag does not match, silently
+     * discard the packet.
+     */
+    if (vtag = 0) {
+        chunkhdr = (struct sctp_init_chunk *)((void *)sctphdr
+                + sizeof(struct sctphdr));
+        if (len < sizeof(struct sctphdr) + sizeof(sctp_chunkhdr_t)
+              + sizeof(__be32)
+            || chunkhdr->chunk_hdr.type != SCTP_CID_INIT
+            || ntohl(chunkhdr->init_hdr.init_tag) != asoc->c.my_vtag) {

The logical operands usually go on the line above like this:

	if ( foo ||
              bar)

net/sctp/input.c 2008-06-01 04:23:27.000000000 -0400 struct sock *sctp_err_lookup(int family, · line 476

Quoted 1 line

net/sctp/input.c 2008-06-01 04:23:27.000000000 -0400 renamed from net/sctp/input.c 2008-05-31 23:49:24.000000000 -0400 +1 -0
+            ICMP_INC_STATS_BH(ICMP_MIB_INERRORS);

This ICMP_INC_STAT_BH causes a double count, since when we return NULL out of this function the parent sctp_v4_err() or sctp_v6_err() will count this stat.

net/sctp/input.c 2008-06-01 04:23:27.000000000 -0400 struct sock *sctp_err_lookup(int family, · line 480

Quoted 4 lines

net/sctp/input.c 2008-06-01 04:23:27.000000000 -0400 renamed from net/sctp/input.c 2008-05-31 23:49:24.000000000 -0400 +3 -0
+            goto out;
+        }
+    } else if (vtag != asoc->c.peer_vtag) {
        ICMP_INC_STATS_BH(ICMP_MIB_INERRORS);

Same here. Latent bug.

-vlad

Quoted 10 lines

net/sctp/input.c 2008-06-01 04:23:27.000000000 -0400 renamed from net/sctp/input.c 2008-05-31 23:49:24.000000000 -0400 +0 -0
        goto out;
    }
Signature · 4 lines
-- 
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Wei Yongjun

author

2008-06-10 01:12 UTC #5 lore raw

In reply to Wei Yongjun

Quoted 23 lines · Vlad Yasevich

Review
Quoted 8 lines · Wei Yongjun
(quoted diff) +7 -0
+    if (vtag = 0) {
+        chunkhdr = (struct sctp_init_chunk *)((void *)sctphdr
+                + sizeof(struct sctphdr));
+        if (len < sizeof(struct sctphdr) + sizeof(sctp_chunkhdr_t)
+              + sizeof(__be32)
+            || chunkhdr->chunk_hdr.type != SCTP_CID_INIT
+            || ntohl(chunkhdr->init_hdr.init_tag) != asoc->c.my_vtag) {
The logical operands usually go on the line above like this:
    if ( foo ||
             bar)
Quoted 1 line
+ ICMP_INC_STATS_BH(ICMP_MIB_INERRORS);
This ICMP_INC_STAT_BH causes a double count, since when we return NULL out
of this function the parent sctp_v4_err() or sctp_v6_err() will count this
stat.

Quoted 12 lines

Review

Quoted 4 lines

(quoted diff) +3 -0

+            goto out;
+        }
+    } else if (vtag != asoc->c.peer_vtag) {
        ICMP_INC_STATS_BH(ICMP_MIB_INERRORS);

Same here. Latent bug.

-vlad

Quoted 3 lines

        goto out;
    }

This patch add to validate initiate tag and chunk type if verification tag is 0 when handling ICMP message.

RFC 4960, Appendix C. ICMP Handling

Signed-off-by Wei Yongjun <[email protected]>

net/sctp/input.c +25 -2

@@ -430,6 +430,9 @@ struct sock *sctp_err_lookup(int family,
 	struct sock *sk = NULL;
 	struct sctp_association *asoc;
 	struct sctp_transport *transport = NULL;
+	struct sctp_init_chunk *chunkhdr;
+	__u32 vtag = ntohl(sctphdr->vtag);
+	int len = skb->len - ((void *)sctphdr - (void *)skb->data);
 
 	*app = NULL; *tpp = NULL;

@@ -451,8 +454,28 @@ struct sock *sctp_err_lookup(int family,
 
 	sk = asoc->base.sk;
 
-	if (ntohl(sctphdr->vtag) != asoc->c.peer_vtag) {
-		ICMP_INC_STATS_BH(ICMP_MIB_INERRORS);
+	/* RFC 4960, Appendix C. ICMP Handling
+	 *
+	 * ICMP6) An implementation MUST validate that the Verification Tag
+	 * contained in the ICMP message matches the Verification Tag of
+	 * the peer.  If the Verification Tag is not 0 and does NOT
+	 * match, discard the ICMP message.  If it is 0 and the ICMP
+	 * message contains enough bytes to verify that the chunk type is
+	 * an INIT chunk and that the Initiate Tag matches the tag of the
+	 * peer, continue with ICMP7.  If the ICMP message is too short
+	 * or the chunk type or the Initiate Tag does not match, silently
+	 * discard the packet.
+	 */
+	if (vtag = 0) {
+		chunkhdr = (struct sctp_init_chunk *)((void *)sctphdr
+				+ sizeof(struct sctphdr));
+		if (len < sizeof(struct sctphdr) + sizeof(sctp_chunkhdr_t)
+			  + sizeof(__be32) ||
+		    chunkhdr->chunk_hdr.type != SCTP_CID_INIT ||
+		    ntohl(chunkhdr->init_hdr.init_tag) != asoc->c.my_vtag) {
+			goto out;
+		}
+	} else if (vtag != asoc->c.peer_vtag) {
 		goto out;
 	}