[PATCH] RAW6: Do not allow set IPV6_CHECKSUM for ICMPv6 socket

[PATCH] RAW6: Do not allow set IPV6_CHECKSUM for ICMPv6 socket

[Wang Chen]

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

Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
---
 net/ipv6/raw.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c
index 0a6fbc1..c4cc1f9 100644
--- a/net/ipv6/raw.c
+++ b/net/ipv6/raw.c
@@ -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

Re: [PATCH] RAW6: Do not allow set IPV6_CHECKSUM for ICMPv6 socket

[David Miller]

From: Wang Chen <wangchen@cn.fujitsu.com>
Date: Fri, 18 Apr 2008 18:32:46 +0800

> As RFC3542 mentions: An attempt to set IPV6_CHECKSUM for an
> ICMPv6 socket will fail.
> Add a check for that in do_rawv6_setsockopt().
> 
> Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>

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.

Re: [PATCH] RAW6: Do not allow set IPV6_CHECKSUM for ICMPv6 socket

[Wang Chen]

David Miller said the following on 2008-4-18 19:09:
> From: Wang Chen <wangchen@cn.fujitsu.com>
> Date: Fri, 18 Apr 2008 18:32:46 +0800
> 
>> As RFC3542 mentions: An attempt to set IPV6_CHECKSUM for an
>> ICMPv6 socket will fail.
>> Add a check for that in do_rawv6_setsockopt().
>>
>> Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
> 
> 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.
> 

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);
	}
---

Re: [PATCH] RAW6: Do not allow set IPV6_CHECKSUM for ICMPv6 socket

[David Miller]

From: Wang Chen <wangchen@cn.fujitsu.com>
Date: Sun, 20 Apr 2008 15:18:52 +0800

> Why not remove the RFC-breaking code from applications?

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.

Re: [PATCH] RAW6: Do not allow set IPV6_CHECKSUM for ICMPv6 socket

[Wang Chen]

David Miller said the following on 2008-4-20 15:33:
> From: Wang Chen <wangchen@cn.fujitsu.com>
> Date: Sun, 20 Apr 2008 15:18:52 +0800
> 
>> Why not remove the RFC-breaking code from applications?
> 
> 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.
> 

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().

Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
---
 net/ipv6/raw.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c
index 0a6fbc1..0be4eb3 100644
--- a/net/ipv6/raw.c
+++ b/net/ipv6/raw.c
@@ -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

Re: [PATCH] RAW6: Do not allow set IPV6_CHECKSUM for ICMPv6 socket

[YOSHIFUJI Hideaki / 吉藤英明]

In article <480B0A59.7000408@cn.fujitsu.com> (at Sun, 20 Apr 2008 17:18:17 +0800), Wang Chen <wangchen@cn.fujitsu.com> says:

> David Miller said the following on 2008-4-20 15:33:
> > From: Wang Chen <wangchen@cn.fujitsu.com>
> > Date: Sun, 20 Apr 2008 15:18:52 +0800
> > 
> >> Why not remove the RFC-breaking code from applications?
> > 
> > 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.
> > 
> 
> 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().
> 
> Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>

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 28024cad925091176166a3f4468ad622d18ef41c Mon Sep 17 00:00:00 2001
From: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
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.

Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
---
 net/ipv6/raw.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c
index 0a6fbc1..9ad9aca 100644
--- a/net/ipv6/raw.c
+++ b/net/ipv6/raw.c
@@ -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

Re: [PATCH] RAW6: Do not allow set IPV6_CHECKSUM for ICMPv6 socket

[David Miller]

From: YOSHIFUJI Hideaki / 吉藤英明 <yoshfuji@linux-ipv6.org>
Date: Sun, 20 Apr 2008 18:38:35 +0900 (JST)

> 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.

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

Re: [PATCH] RAW6: Do not allow set IPV6_CHECKSUM for ICMPv6 socket

[David Miller]

From: "chen wang" <ellre923@gmail.com>
Date: Sun, 20 Apr 2008 18:42:14 +0800

> 2008/4/20, YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>:
> > RFC3542 tells that IPV6_CHECKSUM socket option in the IPPROTO_IPV6
> 
> 
> 
>              ^^^^^^^^^^^^^^^^^^^
> Where in RFC3542 says that "only IPPROTO_IPV6 level" is not allowed to be
> set
> IPV6_CHECKSUM option on ICMPv6 socket?

Yoshifuji-san, please address this question.

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

Re: [PATCH] RAW6: Do not allow set IPV6_CHECKSUM for ICMPv6 socket

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20080424.034845.137470754.davem@davemloft.net> (at Thu, 24 Apr 2008 03:48:45 -0700 (PDT)), David Miller <davem@davemloft.net> says:

> From: "chen wang" <ellre923@gmail.com>
> Date: Sun, 20 Apr 2008 18:42:14 +0800
> 
> > 2008/4/20, YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>:
> > > RFC3542 tells that IPV6_CHECKSUM socket option in the IPPROTO_IPV6
> > 
> > 
> > 
> >              ^^^^^^^^^^^^^^^^^^^
> > Where in RFC3542 says that "only IPPROTO_IPV6 level" is not allowed to be
> > set
> > IPV6_CHECKSUM option on ICMPv6 socket?
> 
> Yoshifuji-san, please address this question.
> 
> I haven't applied this patch, and I won't until this is resolved
> and discussed.

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

Re: [PATCH] RAW6: Do not allow set IPV6_CHECKSUM for ICMPv6 socket

[Rémi Denis-Courmont]

Le Thursday 24 April 2008 15:19:53 YOSHIFUJI Hideaki / 吉藤英明, vous avez écrit :
> 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.

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/

Re: [PATCH] RAW6: Do not allow set IPV6_CHECKSUM for ICMPv6 socket

[Wang Chen]

Rémi Denis-Courmont said the following on 2008-4-24 23:26:
> 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 
  ^^^^^^^^^^^^^^^
Remi, should be setsockopt(SOL_IPV6, IPV6_CHECKSUM)?

> really care if this is changed.
> 

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

Re: [PATCH] RAW6: Do not allow set IPV6_CHECKSUM for ICMPv6 socket

[David Miller]

From: Wang Chen <wangchen@cn.fujitsu.com>
Date: Fri, 25 Apr 2008 09:03:59 +0800

> 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

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.

Re: [PATCH] RAW6: Do not allow set IPV6_CHECKSUM for ICMPv6 socket

[David Miller]

From: YOSHIFUJI Hideaki / 吉藤英明 <yoshfuji@linux-ipv6.org>
Date: Thu, 24 Apr 2008 21:19:53 +0900 (JST)

> In article <20080424.034845.137470754.davem@davemloft.net> (at Thu, 24 Apr 2008 03:48:45 -0700 (PDT)), David Miller <davem@davemloft.net> says:
> 
> > From: "chen wang" <ellre923@gmail.com>
> > Date: Sun, 20 Apr 2008 18:42:14 +0800
> > 
> > > 2008/4/20, YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>:
> > > > RFC3542 tells that IPV6_CHECKSUM socket option in the IPPROTO_IPV6
> > > 
> > > 
> > > 
> > >              ^^^^^^^^^^^^^^^^^^^
> > > Where in RFC3542 says that "only IPPROTO_IPV6 level" is not allowed to be
> > > set
> > > IPV6_CHECKSUM option on ICMPv6 socket?
> > 
> > Yoshifuji-san, please address this question.
> > 
> > I haven't applied this patch, and I won't until this is resolved
> > and discussed.
> 
> 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.

Thank you for this clarification.

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

Thanks again.