* Re: commit b9ef5513c99b breaks ping to ipv6 linklocal addresses on debian buster
2020-01-06 21:03 ` Tetsuo Handa
@ 2020-01-06 21:33 ` Casey Schaufler
2020-01-07 18:05 ` Casey Schaufler
2020-01-07 18:40 ` Casey Schaufler
2 siblings, 0 replies; 15+ messages in thread
From: Casey Schaufler @ 2020-01-06 21:33 UTC (permalink / raw)
To: Tetsuo Handa, David Ahern; +Cc: netdev
On 1/6/2020 1:03 PM, Tetsuo Handa wrote:
> On 2020/01/07 1:41, David Ahern wrote:
>>>> #ifdef SMACK_IPV6_SECMARK_LABELING
>>>> rsp = smack_ipv6host_label(sip);
>>>>
>>>>
>>>> ie., if the socket family is AF_INET6 the address length should be an
>>>> IPv6 address. The family in the sockaddr is not as important.
>>>>
>>> Commit b9ef5513c99b was wrong, but we need to also fix commit c673944347ed ?
>>>
>> not sure. I have not seen a problem related to it yet.
>>
> A sample program shown below is expected to return 0.
> Casey, what does smack wants to do for IPv4 address on IPv6 socket case?
I have to take a minute to look more closely at what's involved.
>
> ----------
> #include <sys/types.h>
> #include <sys/socket.h>
> #include <sys/un.h>
> #include <arpa/inet.h>
> #include <unistd.h>
>
> int main(int argc, char *argv[])
> {
> const int fd1 = socket(PF_INET6, SOCK_DGRAM, 0);
> const int fd2 = socket(PF_INET, SOCK_DGRAM, 0);
> struct sockaddr_in addr1 = {
> .sin_family = AF_INET,
> .sin_addr.s_addr = htonl(INADDR_LOOPBACK),
> .sin_port = htons(10000)
> };
> struct sockaddr_in addr2 = { };
> char c = 0;
> struct iovec iov1 = { "", 1 };
> struct iovec iov2 = { &c, 1 };
> const struct msghdr msg1 = {
> .msg_iov = &iov1,
> .msg_iovlen = 1,
> .msg_name = &addr1,
> .msg_namelen = sizeof(addr1)
> };
> struct msghdr msg2 = {
> .msg_iov = &iov2,
> .msg_iovlen = 1,
> .msg_name = &addr2,
> .msg_namelen = sizeof(addr2)
> };
> if (bind(fd2, (struct sockaddr *) &addr1, sizeof(addr1)))
> return 1;
> if (sendmsg(fd1, &msg1, 0) != 1 || recvmsg(fd2, &msg2, 0) != 1)
> return 1;
> if (connect(fd1, (struct sockaddr *) &addr1, sizeof(addr1)))
> return 1;
> if (send(fd1, "", 1, 0) != 1 || recv(fd2, &c, 1, 0) != 1)
> return 1;
> return 0;
> }
> ----------
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: commit b9ef5513c99b breaks ping to ipv6 linklocal addresses on debian buster
2020-01-06 21:03 ` Tetsuo Handa
2020-01-06 21:33 ` Casey Schaufler
@ 2020-01-07 18:05 ` Casey Schaufler
2020-01-07 18:40 ` Casey Schaufler
2 siblings, 0 replies; 15+ messages in thread
From: Casey Schaufler @ 2020-01-07 18:05 UTC (permalink / raw)
To: Tetsuo Handa, David Ahern; +Cc: netdev, Casey Schaufler
On 1/6/2020 1:03 PM, Tetsuo Handa wrote:
> On 2020/01/07 1:41, David Ahern wrote:
>>>> #ifdef SMACK_IPV6_SECMARK_LABELING
>>>> rsp = smack_ipv6host_label(sip);
>>>>
>>>>
>>>> ie., if the socket family is AF_INET6 the address length should be an
>>>> IPv6 address. The family in the sockaddr is not as important.
>>>>
>>> Commit b9ef5513c99b was wrong, but we need to also fix commit c673944347ed ?
>>>
>> not sure. I have not seen a problem related to it yet.
>>
> A sample program shown below is expected to return 0.
> Casey, what does smack wants to do for IPv4 address on IPv6 socket case?
Thank you, this program has been very helpful. The problematic
checks are supposed to be simply for data sanity, not security.
I think I've got the right set of checks figured out. I'll send
a patch for review once it tests out.
>
> ----------
> #include <sys/types.h>
> #include <sys/socket.h>
> #include <sys/un.h>
> #include <arpa/inet.h>
> #include <unistd.h>
>
> int main(int argc, char *argv[])
> {
> const int fd1 = socket(PF_INET6, SOCK_DGRAM, 0);
> const int fd2 = socket(PF_INET, SOCK_DGRAM, 0);
> struct sockaddr_in addr1 = {
> .sin_family = AF_INET,
> .sin_addr.s_addr = htonl(INADDR_LOOPBACK),
> .sin_port = htons(10000)
> };
> struct sockaddr_in addr2 = { };
> char c = 0;
> struct iovec iov1 = { "", 1 };
> struct iovec iov2 = { &c, 1 };
> const struct msghdr msg1 = {
> .msg_iov = &iov1,
> .msg_iovlen = 1,
> .msg_name = &addr1,
> .msg_namelen = sizeof(addr1)
> };
> struct msghdr msg2 = {
> .msg_iov = &iov2,
> .msg_iovlen = 1,
> .msg_name = &addr2,
> .msg_namelen = sizeof(addr2)
> };
> if (bind(fd2, (struct sockaddr *) &addr1, sizeof(addr1)))
> return 1;
> if (sendmsg(fd1, &msg1, 0) != 1 || recvmsg(fd2, &msg2, 0) != 1)
> return 1;
> if (connect(fd1, (struct sockaddr *) &addr1, sizeof(addr1)))
> return 1;
> if (send(fd1, "", 1, 0) != 1 || recv(fd2, &c, 1, 0) != 1)
> return 1;
> return 0;
> }
> ----------
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: commit b9ef5513c99b breaks ping to ipv6 linklocal addresses on debian buster
2020-01-06 21:03 ` Tetsuo Handa
2020-01-06 21:33 ` Casey Schaufler
2020-01-07 18:05 ` Casey Schaufler
@ 2020-01-07 18:40 ` Casey Schaufler
2020-01-07 18:44 ` David Ahern
2 siblings, 1 reply; 15+ messages in thread
From: Casey Schaufler @ 2020-01-07 18:40 UTC (permalink / raw)
To: Tetsuo Handa, David Ahern; +Cc: netdev, Casey Schaufler
Does this patch address the Debian issue? It works for the test program
and on my Fedora system.
security/smack/smack_lsm.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 50c536cad85b..b0bb36419aeb 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -2857,7 +2857,9 @@ static int smack_socket_connect(struct socket *sock, struct sockaddr *sap,
rc = smack_netlabel_send(sock->sk, (struct sockaddr_in *)sap);
break;
case PF_INET6:
- if (addrlen < SIN6_LEN_RFC2133 || sap->sa_family != AF_INET6)
+ if (addrlen < SIN6_LEN_RFC2133)
+ return 0;
+ if (sap->sa_family != AF_INET6)
return -EINVAL;
#ifdef SMACK_IPV6_SECMARK_LABELING
rsp = smack_ipv6host_label(sip);
@@ -3694,8 +3696,9 @@ static int smack_socket_sendmsg(struct socket *sock, struct msghdr *msg,
break;
#if IS_ENABLED(CONFIG_IPV6)
case AF_INET6:
- if (msg->msg_namelen < SIN6_LEN_RFC2133 ||
- sap->sin6_family != AF_INET6)
+ if (msg->msg_namelen < SIN6_LEN_RFC2133)
+ return 0;
+ if (sap->sin6_family != AF_INET6)
return -EINVAL;
#ifdef SMACK_IPV6_SECMARK_LABELING
rsp = smack_ipv6host_label(sap);
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: commit b9ef5513c99b breaks ping to ipv6 linklocal addresses on debian buster
2020-01-07 18:40 ` Casey Schaufler
@ 2020-01-07 18:44 ` David Ahern
2020-01-08 2:41 ` David Ahern
0 siblings, 1 reply; 15+ messages in thread
From: David Ahern @ 2020-01-07 18:44 UTC (permalink / raw)
To: Casey Schaufler, Tetsuo Handa; +Cc: netdev
On 1/7/20 11:40 AM, Casey Schaufler wrote:
> Does this patch address the Debian issue? It works for the test program
> and on my Fedora system.
>
>
> security/smack/smack_lsm.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 50c536cad85b..b0bb36419aeb 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -2857,7 +2857,9 @@ static int smack_socket_connect(struct socket *sock, struct sockaddr *sap,
> rc = smack_netlabel_send(sock->sk, (struct sockaddr_in *)sap);
> break;
> case PF_INET6:
> - if (addrlen < SIN6_LEN_RFC2133 || sap->sa_family != AF_INET6)
> + if (addrlen < SIN6_LEN_RFC2133)
> + return 0;
> + if (sap->sa_family != AF_INET6)
> return -EINVAL;
I doubt it; can't test it until tonight. strace for ping on buster is
showing this:
$ strace -e connect ping6 -c1 -w1 ff02::1%eth1
connect(4, {sa_family=AF_UNSPEC,
sa_data="\4\1\0\0\0\0\377\2\0\0\0\0\0\0\0\0\0\0\0\0\0\1\3\0\0\0"}, 28)
ie., the addrlen >= SIN6_LEN_RFC2133 but the family is AF_UNSPEC. That
suggests to me the first check passes and the second check fails.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: commit b9ef5513c99b breaks ping to ipv6 linklocal addresses on debian buster
2020-01-07 18:44 ` David Ahern
@ 2020-01-08 2:41 ` David Ahern
2020-01-08 23:06 ` Casey Schaufler
0 siblings, 1 reply; 15+ messages in thread
From: David Ahern @ 2020-01-08 2:41 UTC (permalink / raw)
To: Casey Schaufler, Tetsuo Handa; +Cc: netdev
On 1/7/20 11:44 AM, David Ahern wrote:
> On 1/7/20 11:40 AM, Casey Schaufler wrote:
>> Does this patch address the Debian issue? It works for the test program
>> and on my Fedora system.
>>
>>
>> security/smack/smack_lsm.c | 9 ++++++---
>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>> index 50c536cad85b..b0bb36419aeb 100644
>> --- a/security/smack/smack_lsm.c
>> +++ b/security/smack/smack_lsm.c
>> @@ -2857,7 +2857,9 @@ static int smack_socket_connect(struct socket *sock, struct sockaddr *sap,
>> rc = smack_netlabel_send(sock->sk, (struct sockaddr_in *)sap);
>> break;
>> case PF_INET6:
>> - if (addrlen < SIN6_LEN_RFC2133 || sap->sa_family != AF_INET6)
>> + if (addrlen < SIN6_LEN_RFC2133)
>> + return 0;
>> + if (sap->sa_family != AF_INET6)
>> return -EINVAL;
>
> I doubt it; can't test it until tonight. strace for ping on buster is
> showing this:
>
> $ strace -e connect ping6 -c1 -w1 ff02::1%eth1
> connect(4, {sa_family=AF_UNSPEC,
> sa_data="\4\1\0\0\0\0\377\2\0\0\0\0\0\0\0\0\0\0\0\0\0\1\3\0\0\0"}, 28)
>
> ie., the addrlen >= SIN6_LEN_RFC2133 but the family is AF_UNSPEC. That
> suggests to me the first check passes and the second check fails.
>
confirmed. connect still fails EINVAL.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: commit b9ef5513c99b breaks ping to ipv6 linklocal addresses on debian buster
2020-01-08 2:41 ` David Ahern
@ 2020-01-08 23:06 ` Casey Schaufler
[not found] ` <CAPA1RqAqBZNeN=T_FpApLo_-oZXeFctgGHcggY6xAXV+qHYcKg@mail.gmail.com>
2020-01-09 4:06 ` David Ahern
0 siblings, 2 replies; 15+ messages in thread
From: Casey Schaufler @ 2020-01-08 23:06 UTC (permalink / raw)
To: David Ahern, Tetsuo Handa; +Cc: netdev, Casey Schaufler
This version should work, I think. Please verify. Thank you.
----
security/smack/smack_lsm.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 50c536cad85b..75b3953212e2 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -2857,10 +2857,13 @@ static int smack_socket_connect(struct socket *sock, struct sockaddr *sap,
rc = smack_netlabel_send(sock->sk, (struct sockaddr_in *)sap);
break;
case PF_INET6:
- if (addrlen < SIN6_LEN_RFC2133 || sap->sa_family != AF_INET6)
- return -EINVAL;
+ if (addrlen < SIN6_LEN_RFC2133)
+ return 0;
#ifdef SMACK_IPV6_SECMARK_LABELING
- rsp = smack_ipv6host_label(sip);
+ if (sap->sa_family != AF_INET6)
+ rsp = NULL;
+ else
+ rsp = smack_ipv6host_label(sip);
if (rsp != NULL)
rc = smk_ipv6_check(ssp->smk_out, rsp, sip,
SMK_CONNECTING);
@@ -3694,11 +3697,13 @@ static int smack_socket_sendmsg(struct socket *sock, struct msghdr *msg,
break;
#if IS_ENABLED(CONFIG_IPV6)
case AF_INET6:
- if (msg->msg_namelen < SIN6_LEN_RFC2133 ||
- sap->sin6_family != AF_INET6)
- return -EINVAL;
+ if (msg->msg_namelen < SIN6_LEN_RFC2133)
+ return 0;
#ifdef SMACK_IPV6_SECMARK_LABELING
- rsp = smack_ipv6host_label(sap);
+ if (sap->sin6_family != AF_INET6)
+ rsp = NULL;
+ else
+ rsp = smack_ipv6host_label(sap);
if (rsp != NULL)
rc = smk_ipv6_check(ssp->smk_out, rsp, sap,
SMK_CONNECTING);
^ permalink raw reply related [flat|nested] 15+ messages in thread
[parent not found: <CAPA1RqAqBZNeN=T_FpApLo_-oZXeFctgGHcggY6xAXV+qHYcKg@mail.gmail.com>]
* Re: commit b9ef5513c99b breaks ping to ipv6 linklocal addresses on debian buster
[not found] ` <CAPA1RqAqBZNeN=T_FpApLo_-oZXeFctgGHcggY6xAXV+qHYcKg@mail.gmail.com>
@ 2020-01-09 3:39 ` David Ahern
0 siblings, 0 replies; 15+ messages in thread
From: David Ahern @ 2020-01-09 3:39 UTC (permalink / raw)
To: 吉藤英明, Casey Schaufler; +Cc: Tetsuo Handa, netdev
On 1/8/20 6:53 PM, 吉藤英明 wrote:
> Hi,
>
> 2020年1月9日(木) 8:06 Casey Schaufler <casey@schaufler-ca.com
> <mailto:casey@schaufler-ca.com>>:
>
> This version should work, I think. Please verify. Thank you.
>
> ----
> security/smack/smack_lsm.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 50c536cad85b..75b3953212e2 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -2857,10 +2857,13 @@ static int smack_socket_connect(struct
> socket *sock, struct sockaddr *sap,
> rc = smack_netlabel_send(sock->sk, (struct
> sockaddr_in *)sap);
> break;
> case PF_INET6:
> - if (addrlen < SIN6_LEN_RFC2133 || sap->sa_family !=
> AF_INET6)
> - return -EINVAL;
> + if (addrlen < SIN6_LEN_RFC2133)
> + return 0;
>
>
> Why don't we assume AF_UNSPEC as if it were AF_INET6 for AF_INET6
> sockets here?
>
it is not called out explicitly, but the rest of the proposed change:
#ifdef SMACK_IPV6_SECMARK_LABELING
- rsp = smack_ipv6host_label(sip);
+ if (sap->sa_family != AF_INET6)
+ rsp = NULL;
+ else
+ rsp = smack_ipv6host_label(sip);
if (rsp != NULL)
rc = smk_ipv6_check(ssp->smk_out, rsp, sip,
SMK_CONNECTING);
seems to handle AF_UNSPEC. Did you have something else in mind?
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: commit b9ef5513c99b breaks ping to ipv6 linklocal addresses on debian buster
2020-01-08 23:06 ` Casey Schaufler
[not found] ` <CAPA1RqAqBZNeN=T_FpApLo_-oZXeFctgGHcggY6xAXV+qHYcKg@mail.gmail.com>
@ 2020-01-09 4:06 ` David Ahern
2020-01-09 9:50 ` Tetsuo Handa
1 sibling, 1 reply; 15+ messages in thread
From: David Ahern @ 2020-01-09 4:06 UTC (permalink / raw)
To: Casey Schaufler, Tetsuo Handa; +Cc: netdev
On 1/8/20 4:06 PM, Casey Schaufler wrote:
> This version should work, I think. Please verify. Thank you.
>
It does.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: commit b9ef5513c99b breaks ping to ipv6 linklocal addresses on debian buster
2020-01-09 4:06 ` David Ahern
@ 2020-01-09 9:50 ` Tetsuo Handa
0 siblings, 0 replies; 15+ messages in thread
From: Tetsuo Handa @ 2020-01-09 9:50 UTC (permalink / raw)
To: David Ahern, Casey Schaufler; +Cc: netdev
On 2020/01/09 13:06, David Ahern wrote:
> On 1/8/20 4:06 PM, Casey Schaufler wrote:
>> This version should work, I think. Please verify. Thank you.
>>
>
> It does.
>
Don't we want to do that check inside smk_ipv6_check() because
smk_ipv6_port_check() might call smk_ipv6_check() ?
smack does not want to call smack_netlabel_send() (as it is IPv4 socket)
when IPv4 address was given on IPv6 socket, does it?
Also, please fold smack_socket_bind() change (make it no-op unless
valid IPv6 address is given) into your patch.
^ permalink raw reply [flat|nested] 15+ messages in thread