netdev - Re: [PATCH] net: socket: Always initialize family field at move_addr_to

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 12 Apr 2019 09:24:10 +0900
From:   Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
To:     Paul Moore <paul@...l-moore.com>
Cc:     linux-security-module <linux-security-module@...r.kernel.org>,
        David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
        syzbot+0049bebbf3042dbd2e8f@...kaller.appspotmail.com,
        syzbot+915c9f99f3dbc4bd6cd1@...kaller.appspotmail.com
Subject: Re: [PATCH] net: socket: Always initialize family field at
 move_addr_to_kernel().

Paul Moore wrote:
> > +       /*
> > +        * Nothing more to do if valid length is too short to check
> > +        * address->sa_family.
> > +        */
> > +       if (addrlen < offsetofend(struct sockaddr, sa_family))
> > +               goto out;
> 
> SELinux already checks the address length further down in
> selinux_socket_bind() for the address families it care about, although
> it does read sa_family before the address length check so no
> unfortunately it's of no help.

Exactly.

Since we will anyway have to check valid length after sa_family is checked,
reading a stale sa_family is harmless except KMSAN complains uninit-value.
Therefore,

--- a/net/socket.c
+++ b/net/socket.c
@@ -181,6 +181,7 @@ static ssize_t sock_splice_read(struct file *file, loff_t *ppos,
 
 int move_addr_to_kernel(void __user *uaddr, int ulen, struct sockaddr_storage *kaddr)
 {
+	kaddr->ss_family = 0;
 	if (ulen < 0 || ulen > sizeof(struct sockaddr_storage))
 		return -EINVAL;
 	if (ulen == 0)

or

--- a/net/socket.c
+++ b/net/socket.c
@@ -181,6 +181,7 @@ static ssize_t sock_splice_read(struct file *file, loff_t *ppos,
 
 int move_addr_to_kernel(void __user *uaddr, int ulen, struct sockaddr_storage *kaddr)
 {
+	memset(kaddr, 0, sizeof(struct sockaddr_storage));
 	if (ulen < 0 || ulen > sizeof(struct sockaddr_storage))
 		return -EINVAL;
 	if (ulen == 0)

will avoid adding this "addrlen < offsetofend(struct sockaddr, sa_family)" check to every
LSM module. It is a bit pity that while it is guaranteed that sizeof(struct sockaddr_storage)
bytes are accessible, it is not guaranteed that reading "struct sockaddr_storage"->family is
safe. (Thus, I wanted to hear comments from LSM people.)

> 
> I imagine you could move this new length check inside the
> PF_INET/PF_INET6 if-then code block to minimize the impact to other
> address families, but I suppose there is some value in keeping it
> where it is too.

I guess that since PF_INET/PF_INET6/PF_UNIX will be most frequently used protocols,
the impact to non-PF_INET/PF_INET6 protocols is negligible.