sys/net/pf_ioctl.c: out of bounds read in pf_rule_copyin

Johann Höpfner <[email protected]>
Newsgroups gmane.os.openbsd.tech
Message-ID <akt9Yh4OH2rMKEpL@ritchie>
Hello tech@

In testing an experimental kernel-address-sanitizer in syzkaller 
the following issue was discovered. (and independently found by the
implementation running in syzbot this morning:
https://syzkaller.appspot.com/bug?extid=f028f6e8a734a5e8de11)

pf_rule_copyin executes strlcpy on non-null-terminated fields from 
userspace leading to a read beyond the allocation pointed to by "from", 
only if userspace sends a struct pf_rule without zero-terminators, e.g.
as follows. 

#include <fcntl.h>
#include <string.h>
#include <sys/ioctl.h>
#include <net/if.h>
#include <net/pfvar.h>
int main(){
        int fd = open("/dev/pf", O_RDWR);
        struct pfioc_rule pr;
        memset(&pr, 'A', sizeof(struct pfioc_rule));
        ioctl(fd, DIOCADDRULE, &pr);
}

Due to "from" being allocated in a 4096 byte bucket this is very unlikely
to cause a crash without kasan.

I suggest the following fix terminating the fields before strlcpy. A 
strscpy-like approach might be cleaner as we discard the return value.

Index: sys/net/pf_ioctl.c
===================================================================
--- sys/net/pf_ioctl.c
+++ sys/net/pf_ioctl.c
@@ -4040,6 +4040,15 @@
 
 	/* XXX union skip[] */
 
+	from->label[sizeof(from->label) - 1] = '\0';
+	from->ifname[sizeof(from->ifname) - 1] = '\0';
+	from->rcv_ifname[sizeof(from->rcv_ifname) - 1] = '\0';
+	from->qname[sizeof(from->qname) - 1] = '\0';
+	from->pqname[sizeof(from->pqname) - 1] = '\0';
+	from->tagname[sizeof(from->tagname) - 1] = '\0';
+	from->match_tagname[sizeof(from->match_tagname) - 1] = '\0';
+	from->overload_tblname[sizeof(from->overload_tblname) - 1] = '\0';
+
 	strlcpy(to->label, from->label, sizeof(to->label));
 	strlcpy(to->ifname, from->ifname, sizeof(to->ifname));
 	strlcpy(to->rcv_ifname, from->rcv_ifname, sizeof(to->rcv_ifname));
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.