RAW6: Do not allow set IPV6_CHECKSUM for ICMPv6 socket

As RFC3542 mentions: An attempt to set IPV6_CHECKSUM for an ICMPv6 socket will fail.
Add a check for that in do_rawv6_setsockopt().

---
 net/ipv6/raw.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

index 0a6fbc1..c4cc1f9 100644
@@ -994,6 +994,11 @@ static int do_rawv6_setsockopt(struct sock *sk, int level, int optname,
 
 	switch (optname) {
 		case IPV6_CHECKSUM:
+			/* RFC3542: An attempt to set IPV6_CHECKSUM for an
+			 * ICMPv6 socket will fail.
+			 */
+			if (inet_sk(sk)->num == IPPROTO_ICMPV6)
+				return(-EINVAL);
 			/* You may get strange result with a positive odd offset;
 			   RFC2292bis agrees with me. */
 			if (val > 0 && (val&1))

1.5.4

From: Wang Chen <[email protected]>
Date: Fri, 18 Apr 2008 18:32:46 +0800

Enforcing these kinds of things we've allowed for so many years is rather pointless, especially if it breaks real applications. Which I believe it does in this case. The standards simply do not matter.

I seem to recall that traceroute6 in iputils does exactly this.

David Miller said the following on 2008-4-18 19:09:

Enforcing this can forbid disabling checksum for icmpv6 socket. The real applications, such as ping6 & traceroute6, are just doing what is default in kernel.
Why not remove the RFC-breaking code from applications?

See the source:
kernel->rawv6_init_sk()
---

	case IPPROTO_ICMPV6:
		rp->checksum = 1;
		rp->offset   = 2;
		break;

---

ping6->main()
---

	csum_offset = 2;
	sz_opt = sizeof(int);

	err = setsockopt(icmp_sock, SOL_RAW, IPV6_CHECKSUM, &csum_offset, sz_opt);
	if (err < 0) {
		perror("setsockopt(RAW_CHECKSUM)");
		exit(2);
	}

---

traceroute6->main()
---

	on = 2;
	if (setsockopt(icmp_sock, SOL_RAW, IPV6_CHECKSUM, &on, sizeof(on)) < 0) {
		perror("setsockopt(RAW_CHECKSUM)");
		exit(2);
	}

---

From: Wang Chen <[email protected]>
Date: Sun, 20 Apr 2008 15:18:52 +0800

Because once applications exist and are deployed we cannot break them with careless kernel changes. A user should not get a broken traceroute6 binary just because he upgrades his kernel, that's a bug.

The RFC is not a set of laws that must be followed under all circumstances. In this case it is worse to break applications on people's systems than be compliant to some standard.

You need to understand this basic premise before we can communicate at all about this topic.

David Miller said the following on 2008-4-20 15:33:

Yes. I agree with you that the RFC is not a law and we don't want to break applications by changing kernel.

So, how about the following approach which don't break iputils.

---
As RFC3542 mentions: An attempt to set IPV6_CHECKSUM for an ICMPv6 socket will fail. But there are some legacy applications which set the option to enable IPV6_CHECKSUM for ICMPv6 socket.
To forbid disabling checksum for ICMPv6 socket, add a check for that in do_rawv6_setsockopt().

---
 net/ipv6/raw.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

index 0a6fbc1..0be4eb3 100644
@@ -994,6 +994,12 @@ static int do_rawv6_setsockopt(struct sock *sk, int level, int optname,
 
 	switch (optname) {
 		case IPV6_CHECKSUM:
+			/* RFC3542: An attempt to set IPV6_CHECKSUM for an
+			 * ICMPv6 socket will fail. But for legacy application
+			 * compliance, allow offset=2 option value.
+			 */
+			if (inet_sk(sk)->num == IPPROTO_ICMPV6 && val != 2)
+				return(-EINVAL);
 			/* You may get strange result with a positive odd offset;
 			   RFC2292bis agrees with me. */
 			if (val > 0 && (val&1))

1.5.4

In article <[email protected]> (at Sun, 20 Apr 2008 17:18:17 +0800), Wang Chen <[email protected]> says:

NAK.

I have another approach.

RFC3542 only tells about IPPROTO_IPV6-level IPV6_CHECKSUM option only while ping6 and traceroute6 use IPPROTO_RAW-level IPV6_CHECKSUM option. Just disallow setting those on IPPROTO_IPV6 level.

----

From: YOSHIFUJI Hideaki <[email protected]>
Date: Sat, 19 Apr 2008 20:53:44 +0900
Subject: [PATCH] [IPV6] RAW: Disallow IPPROTO_IPV6-level IPV6_CHECKSUM socket option on ICMPv6 sockets.

RFC3542 tells that IPV6_CHECKSUM socket option in the IPPROTO_IPV6 level is not allowed on ICMPv6 sockets.
IPPROTO_RAW level IPV6_CHECKSUM socket option (a Linux extension) is still allowed.

---
 net/ipv6/raw.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

index 0a6fbc1..9ad9aca 100644
@@ -994,6 +994,19 @@ static int do_rawv6_setsockopt(struct sock *sk, int level, int optname,
 
 	switch (optname) {
 		case IPV6_CHECKSUM:
+			if (inet_sk(sk)->num == IPPROTO_ICMPV6 &&
+			    level == IPPROTO_IPV6) {
+				/*
+				 * RFC3542 tells that IPV6_CHECKSUM socket
+				 * option in the IPPROTO_IPV6 level is not
+				 * allowed on ICMPv6 sockets.
+				 * If you want to set it, use IPPROTO_RAW
+				 * level IPV6_CHECKSUM socket option
+				 * (Linux extension).
+				 */
+				return -EINVAL;
+			}
+
 			/* You may get strange result with a positive odd offset;
 			   RFC2292bis agrees with me. */
 			if (val > 0 && (val&1))

@@ -1069,6 +1082,11 @@ static int do_rawv6_getsockopt(struct sock *sk, int level, int optname,
 
 	switch (optname) {
 	case IPV6_CHECKSUM:
+		/*
+		 * We allow getsockopt() for IPPROTO_IPV6-level
+		 * IPV6_CHECKSUM socket option on ICMPv6 sockets
+		 * since RFC3542 is silent about it.
+		 */
 		if (rp->checksum == 0)
 			val = -1;
 		else

1.4.4.4

From: YOSHIFUJI Hideaki / 吉藤英明 <[email protected]>
Date: Sun, 20 Apr 2008 18:38:35 +0900 (JST)

Now that's a lot more reasonable, I think I'll apply this patch, thanks.

From: "chen wang" <[email protected]>
Date: Sun, 20 Apr 2008 18:42:14 +0800

Yoshifuji-san, please address this question.

I haven't applied this patch, and I won't until this is resolved and discussed.

In article <[email protected]> (at Thu, 24 Apr 2008 03:48:45 -0700 (PDT)), David Miller <[email protected]> says:

RFC3542 discusses about IPPROTO_IPV6 level IPV6_CHECKSUM socket option only. IPPROTO_RAW level IPV6_CHECKSUM socket option is undocumented Linux extension. So, we are free to choose allowing setting that option on ICMPv6 socket as we have been doing.

--yoshfuji

Le Thursday 24 April 2008 15:19:53 YOSHIFUJI Hideaki / 吉藤英明, vous avez écrit :

I wonder why RFC3542 forbids the contentious case. Is it a security consideration, that userland should not be allowed to create bogus ICMPv6 packets (IPV6_CHECKSUM can be set after dropping root after opening a raw socket, right?), or is it just some random IETF folklore ??

I'd note that my ndisc6 package does call setsockopt(SOL_IPV6, ICMPV6_CHECKSUM). Fortunately, it does not check for error values, so I don't really care if this is changed.

Rémi Denis-Courmont
http://www.remlab.net/

Rémi Denis-Courmont said the following on 2008-4-24 23:26:

^^^^^^^^^^^^^^^
Remi, should be setsockopt(SOL_IPV6, IPV6_CHECKSUM)?

David, should we make sure every common used userland package will not be influenced by this change?
No influence packages:
iputils
iproute2
ndisc6
net-tools

From: YOSHIFUJI Hideaki / 吉藤英明 <[email protected]>
Date: Thu, 24 Apr 2008 21:19:53 +0900 (JST)

Thank you for this clarification.

I've applied this patch and I'll queue it up for -stable as well.

Thanks again.

From: Wang Chen <[email protected]>
Date: Fri, 25 Apr 2008 09:03:59 +0800

I don't think it is necessary to maintain a list.

If problems crop up and get reported, we should just focus on fixing them.