[PATCH 1/3] selinux: Check address length before reading address family

[PATCH 1/3] selinux: Check address length before reading address family

[Tetsuo Handa]

KMSAN will complain if valid address length passed to bind()/connect() is
shorter than sizeof("struct sockaddr"->sa_family) bytes.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 security/selinux/hooks.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index d5fdcb0d26fe..c61787b15f27 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4512,7 +4512,7 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
 		struct lsm_network_audit net = {0,};
 		struct sockaddr_in *addr4 = NULL;
 		struct sockaddr_in6 *addr6 = NULL;
-		u16 family_sa = address->sa_family;
+		u16 family_sa;
 		unsigned short snum;
 		u32 sid, node_perm;
 
@@ -4522,6 +4522,9 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
 		 * need to check address->sa_family as it is possible to have
 		 * sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
 		 */
+		if (addrlen < offsetofend(struct sockaddr, sa_family))
+			return -EINVAL;
+		family_sa = address->sa_family;
 		switch (family_sa) {
 		case AF_UNSPEC:
 		case AF_INET:
@@ -4654,6 +4657,8 @@ static int selinux_socket_connect_helper(struct socket *sock,
 		 * need to check address->sa_family as it is possible to have
 		 * sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
 		 */
+		if (addrlen < offsetofend(struct sockaddr, sa_family))
+			return -EINVAL;
 		switch (address->sa_family) {
 		case AF_INET:
 			addr4 = (struct sockaddr_in *)address;
-- 
2.16.5

[PATCH 2/3] smack: Check address length before reading address family

[Tetsuo Handa]

KMSAN will complain if valid address length passed to bind()/connect()/
sendmsg() is shorter than sizeof("struct sockaddr"->sa_family) bytes.

Also, since smk_ipv6_port_label()/smack_netlabel_send()/
smack_ipv6host_label()/smk_ipv6_check()/smk_ipv6_port_check() are not
checking valid address length and/or address family, make sure we check
both. The minimal valid length in smack_socket_connect() is changed from
sizeof(struct sockaddr_in6) bytes to SIN6_LEN_RFC2133 bytes, for it seems
that Smack is not using "struct sockaddr_in6"->sin6_scope_id field.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 security/smack/smack_lsm.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 5c1613519d5a..4b59e4519e0c 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -2805,13 +2805,17 @@ static int smack_socket_socketpair(struct socket *socka,
  *
  * Records the label bound to a port.
  *
- * Returns 0
+ * Returns 0 on success, and error code otherwise
  */
 static int smack_socket_bind(struct socket *sock, struct sockaddr *address,
 				int addrlen)
 {
-	if (sock->sk != NULL && sock->sk->sk_family == PF_INET6)
+	if (sock->sk != NULL && sock->sk->sk_family == PF_INET6) {
+		if (addrlen < SIN6_LEN_RFC2133 ||
+		    address->sa_family != AF_INET6)
+			return -EINVAL;
 		smk_ipv6_port_label(sock, address);
+	}
 	return 0;
 }
 #endif /* SMACK_IPV6_PORT_LABELING */
@@ -2847,12 +2851,13 @@ static int smack_socket_connect(struct socket *sock, struct sockaddr *sap,
 
 	switch (sock->sk->sk_family) {
 	case PF_INET:
-		if (addrlen < sizeof(struct sockaddr_in))
+		if (addrlen < sizeof(struct sockaddr_in) ||
+		    sap->sa_family != AF_INET)
 			return -EINVAL;
 		rc = smack_netlabel_send(sock->sk, (struct sockaddr_in *)sap);
 		break;
 	case PF_INET6:
-		if (addrlen < sizeof(struct sockaddr_in6))
+		if (addrlen < SIN6_LEN_RFC2133 || sap->sa_family != AF_INET6)
 			return -EINVAL;
 #ifdef SMACK_IPV6_SECMARK_LABELING
 		rsp = smack_ipv6host_label(sip);
@@ -3682,9 +3687,15 @@ static int smack_socket_sendmsg(struct socket *sock, struct msghdr *msg,
 
 	switch (sock->sk->sk_family) {
 	case AF_INET:
+		if (msg->msg_namelen < sizeof(struct sockaddr_in) ||
+		    sip->sin_family != AF_INET)
+			return -EINVAL;
 		rc = smack_netlabel_send(sock->sk, sip);
 		break;
 	case AF_INET6:
+		if (msg->msg_namelen < SIN6_LEN_RFC2133 ||
+		    sap->sin6_family != AF_INET6)
+			return -EINVAL;
 #ifdef SMACK_IPV6_SECMARK_LABELING
 		rsp = smack_ipv6host_label(sap);
 		if (rsp != NULL)
-- 
2.16.5

Re: [PATCH 2/3] smack: Check address length before reading address family

[Casey Schaufler]

On 4/12/2019 3:59 AM, Tetsuo Handa wrote:
> KMSAN will complain if valid address length passed to bind()/connect()/
> sendmsg() is shorter than sizeof("struct sockaddr"->sa_family) bytes.
>
> Also, since smk_ipv6_port_label()/smack_netlabel_send()/
> smack_ipv6host_label()/smk_ipv6_check()/smk_ipv6_port_check() are not
> checking valid address length and/or address family, make sure we check
> both. The minimal valid length in smack_socket_connect() is changed from
> sizeof(struct sockaddr_in6) bytes to SIN6_LEN_RFC2133 bytes, for it seems
> that Smack is not using "struct sockaddr_in6"->sin6_scope_id field.
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

Acked-by: Casey Schaufler <casey@schaufler-ca.com>


> ---
>   security/smack/smack_lsm.c | 19 +++++++++++++++----
>   1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 5c1613519d5a..4b59e4519e0c 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -2805,13 +2805,17 @@ static int smack_socket_socketpair(struct socket *socka,
>    *
>    * Records the label bound to a port.
>    *
> - * Returns 0
> + * Returns 0 on success, and error code otherwise
>    */
>   static int smack_socket_bind(struct socket *sock, struct sockaddr *address,
>   				int addrlen)
>   {
> -	if (sock->sk != NULL && sock->sk->sk_family == PF_INET6)
> +	if (sock->sk != NULL && sock->sk->sk_family == PF_INET6) {
> +		if (addrlen < SIN6_LEN_RFC2133 ||
> +		    address->sa_family != AF_INET6)
> +			return -EINVAL;
>   		smk_ipv6_port_label(sock, address);
> +	}
>   	return 0;
>   }
>   #endif /* SMACK_IPV6_PORT_LABELING */
> @@ -2847,12 +2851,13 @@ static int smack_socket_connect(struct socket *sock, struct sockaddr *sap,
>   
>   	switch (sock->sk->sk_family) {
>   	case PF_INET:
> -		if (addrlen < sizeof(struct sockaddr_in))
> +		if (addrlen < sizeof(struct sockaddr_in) ||
> +		    sap->sa_family != AF_INET)
>   			return -EINVAL;
>   		rc = smack_netlabel_send(sock->sk, (struct sockaddr_in *)sap);
>   		break;
>   	case PF_INET6:
> -		if (addrlen < sizeof(struct sockaddr_in6))
> +		if (addrlen < SIN6_LEN_RFC2133 || sap->sa_family != AF_INET6)
>   			return -EINVAL;
>   #ifdef SMACK_IPV6_SECMARK_LABELING
>   		rsp = smack_ipv6host_label(sip);
> @@ -3682,9 +3687,15 @@ static int smack_socket_sendmsg(struct socket *sock, struct msghdr *msg,
>   
>   	switch (sock->sk->sk_family) {
>   	case AF_INET:
> +		if (msg->msg_namelen < sizeof(struct sockaddr_in) ||
> +		    sip->sin_family != AF_INET)
> +			return -EINVAL;
>   		rc = smack_netlabel_send(sock->sk, sip);
>   		break;
>   	case AF_INET6:
> +		if (msg->msg_namelen < SIN6_LEN_RFC2133 ||
> +		    sap->sin6_family != AF_INET6)
> +			return -EINVAL;
>   #ifdef SMACK_IPV6_SECMARK_LABELING
>   		rsp = smack_ipv6host_label(sap);
>   		if (rsp != NULL)

Re: [PATCH 2/3] smack: Check address length before reading address family

[James Morris]

On Fri, 12 Apr 2019, Casey Schaufler wrote:

> On 4/12/2019 3:59 AM, Tetsuo Handa wrote:
> > KMSAN will complain if valid address length passed to bind()/connect()/
> > sendmsg() is shorter than sizeof("struct sockaddr"->sa_family) bytes.
> > 
> > Also, since smk_ipv6_port_label()/smack_netlabel_send()/
> > smack_ipv6host_label()/smk_ipv6_check()/smk_ipv6_port_check() are not
> > checking valid address length and/or address family, make sure we check
> > both. The minimal valid length in smack_socket_connect() is changed from
> > sizeof(struct sockaddr_in6) bytes to SIN6_LEN_RFC2133 bytes, for it seems
> > that Smack is not using "struct sockaddr_in6"->sin6_scope_id field.
> > 
> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> 
> Acked-by: Casey Schaufler <casey@schaufler-ca.com>

Casey: will you be taking this via your tree?

-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 2/3] smack: Check address length before reading address family

[Casey Schaufler]

On 4/29/2019 12:58 PM, James Morris wrote:
> On Fri, 12 Apr 2019, Casey Schaufler wrote:
>
>> On 4/12/2019 3:59 AM, Tetsuo Handa wrote:
>>> KMSAN will complain if valid address length passed to bind()/connect()/
>>> sendmsg() is shorter than sizeof("struct sockaddr"->sa_family) bytes.
>>>
>>> Also, since smk_ipv6_port_label()/smack_netlabel_send()/
>>> smack_ipv6host_label()/smk_ipv6_check()/smk_ipv6_port_check() are not
>>> checking valid address length and/or address family, make sure we check
>>> both. The minimal valid length in smack_socket_connect() is changed from
>>> sizeof(struct sockaddr_in6) bytes to SIN6_LEN_RFC2133 bytes, for it seems
>>> that Smack is not using "struct sockaddr_in6"->sin6_scope_id field.
>>>
>>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>> Acked-by: Casey Schaufler <casey@schaufler-ca.com>
> Casey: will you be taking this via your tree?

Sure. I will add it today.

[PATCH 3/3] tomoyo: Check address length before reading address family

[Tetsuo Handa]

KMSAN will complain if valid address length passed to bind()/connect()/
sendmsg() is shorter than sizeof("struct sockaddr"->sa_family) bytes.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 security/tomoyo/network.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/security/tomoyo/network.c b/security/tomoyo/network.c
index 9094f4b3b367..f9ff121d7e1e 100644
--- a/security/tomoyo/network.c
+++ b/security/tomoyo/network.c
@@ -505,6 +505,8 @@ static int tomoyo_check_inet_address(const struct sockaddr *addr,
 {
 	struct tomoyo_inet_addr_info *i = &address->inet;
 
+	if (addr_len < offsetofend(struct sockaddr, sa_family))
+		return 0;
 	switch (addr->sa_family) {
 	case AF_INET6:
 		if (addr_len < SIN6_LEN_RFC2133)
@@ -594,6 +596,8 @@ static int tomoyo_check_unix_address(struct sockaddr *addr,
 {
 	struct tomoyo_unix_addr_info *u = &address->unix0;
 
+	if (addr_len < offsetofend(struct sockaddr, sa_family))
+		return 0;
 	if (addr->sa_family != AF_UNIX)
 		return 0;
 	u->addr = ((struct sockaddr_un *) addr)->sun_path;
-- 
2.16.5

Re: [PATCH 3/3] tomoyo: Check address length before reading address family

[James Morris]

On Fri, 12 Apr 2019, Tetsuo Handa wrote:

> KMSAN will complain if valid address length passed to bind()/connect()/
> sendmsg() is shorter than sizeof("struct sockaddr"->sa_family) bytes.
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next-tomoyo


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 1/3] selinux: Check address length before reading address family

[Paul Moore]

On Fri, Apr 12, 2019 at 7:00 AM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> KMSAN will complain if valid address length passed to bind()/connect() is
> shorter than sizeof("struct sockaddr"->sa_family) bytes.
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  security/selinux/hooks.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

A reminder to please CC the SELinux mailing list on patches such as these.

This looks good to me, and since it's specific to SELinux, I've gone
ahead and merged it into the selinux/next branch.  Thanks for your
help on this.

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index d5fdcb0d26fe..c61787b15f27 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4512,7 +4512,7 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
>                 struct lsm_network_audit net = {0,};
>                 struct sockaddr_in *addr4 = NULL;
>                 struct sockaddr_in6 *addr6 = NULL;
> -               u16 family_sa = address->sa_family;
> +               u16 family_sa;
>                 unsigned short snum;
>                 u32 sid, node_perm;
>
> @@ -4522,6 +4522,9 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
>                  * need to check address->sa_family as it is possible to have
>                  * sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
>                  */
> +               if (addrlen < offsetofend(struct sockaddr, sa_family))
> +                       return -EINVAL;
> +               family_sa = address->sa_family;
>                 switch (family_sa) {
>                 case AF_UNSPEC:
>                 case AF_INET:
> @@ -4654,6 +4657,8 @@ static int selinux_socket_connect_helper(struct socket *sock,
>                  * need to check address->sa_family as it is possible to have
>                  * sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
>                  */
> +               if (addrlen < offsetofend(struct sockaddr, sa_family))
> +                       return -EINVAL;
>                 switch (address->sa_family) {
>                 case AF_INET:
>                         addr4 = (struct sockaddr_in *)address;
> --
> 2.16.5

-- 
paul moore
www.paul-moore.com