Re: interface group name validation
On Tue, Feb 09, 2021 at 11:08:09PM +0100, Alexander Bluhm wrote: > Hi, > > Next try to fix syzkaller crash > https://syzkaller.appspot.com/bug?id=54e16dc5bce6929e14b42e2f1379f1c18f62be43 > > Interface group names must fit into IFNAMSIZ and be unique. But > the kernel makes the unique check before trunkating with strlcpy(). > So there can be two interfaces groups with the same name. The kif > is created by a name lookup. The trunkated names are equal so there > is only one kif owned by both groups. When both groups are destroyed, > the single kif is removed twice from the RB tree. > > - Check length of group name before doing the unique check. > - The empty group name was allowed. That does not make much sense. > Does anyone use the empty interface group? > - Use the same check in kernel and ifconfig userland. > - ifconfig -group does not need name sanitation. The kernel will > just report that it does not exist. > > ok? > ok mvs@
Re: interface group name validation
On Tue, Feb 09, 2021 at 11:08:09PM +0100, Alexander Bluhm wrote: > Hi, > > Next try to fix syzkaller crash > https://syzkaller.appspot.com/bug?id=54e16dc5bce6929e14b42e2f1379f1c18f62be43 > > Interface group names must fit into IFNAMSIZ and be unique. But > the kernel makes the unique check before trunkating with strlcpy(). > So there can be two interfaces groups with the same name. The kif > is created by a name lookup. The trunkated names are equal so there > is only one kif owned by both groups. When both groups are destroyed, > the single kif is removed twice from the RB tree. > > - Check length of group name before doing the unique check. > - The empty group name was allowed. That does not make much sense. > Does anyone use the empty interface group? > - Use the same check in kernel and ifconfig userland. > - ifconfig -group does not need name sanitation. The kernel will > just report that it does not exist. > > ok? syzkaller was not able to trigger the panic using the syz reproducer with your diff applied: https://groups.google.com/g/syzkaller-openbsd-bugs/c/ZhqISaYBvVE/m/G-V3cB9OAgAJ ok anton@
Re: interface group name validation
Alexander Bluhm writes:
> Hi,
>
> Next try to fix syzkaller crash
> https://syzkaller.appspot.com/bug?id=54e16dc5bce6929e14b42e2f1379f1c18f62be43
>
> Interface group names must fit into IFNAMSIZ and be unique. But
> the kernel makes the unique check before trunkating with strlcpy().
> So there can be two interfaces groups with the same name. The kif
> is created by a name lookup. The trunkated names are equal so there
> is only one kif owned by both groups. When both groups are destroyed,
> the single kif is removed twice from the RB tree.
>
> - Check length of group name before doing the unique check.
> - The empty group name was allowed. That does not make much sense.
> Does anyone use the empty interface group?
> - Use the same check in kernel and ifconfig userland.
> - ifconfig -group does not need name sanitation. The kernel will
> just report that it does not exist.
>
> ok?
OK gnezdo@
>
> bluhm
>
> Index: sys/net/if.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/if.c,v
> retrieving revision 1.627
> diff -u -p -r1.627 if.c
> --- sys/net/if.c 8 Feb 2021 12:30:10 - 1.627
> +++ sys/net/if.c 9 Feb 2021 20:47:34 -
> @@ -2621,9 +2621,11 @@ if_addgroup(struct ifnet *ifp, const cha
> struct ifg_list *ifgl;
> struct ifg_group*ifg = NULL;
> struct ifg_member *ifgm;
> + size_t namelen;
>
> - if (groupname[0] && groupname[strlen(groupname) - 1] >= '0' &&
> - groupname[strlen(groupname) - 1] <= '9')
> + namelen = strlen(groupname);
> + if (namelen == 0 || namelen >= IFNAMSIZ ||
> + (groupname[namelen - 1] >= '0' && groupname[namelen - 1] <= '9'))
> return (EINVAL);
>
> TAILQ_FOREACH(ifgl, &ifp->if_groups, ifgl_next)
> Index: sbin/ifconfig/ifconfig.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sbin/ifconfig/ifconfig.c,v
> retrieving revision 1.432
> diff -u -p -r1.432 ifconfig.c
> --- sbin/ifconfig/ifconfig.c 16 Jan 2021 17:44:29 - 1.432
> +++ sbin/ifconfig/ifconfig.c 9 Feb 2021 21:02:50 -
> @@ -1634,16 +1634,20 @@ void
> setifgroup(const char *group_name, int dummy)
> {
> struct ifgroupreq ifgr;
> + size_t namelen;
>
> memset(&ifgr, 0, sizeof(ifgr));
> strlcpy(ifgr.ifgr_name, ifname, IFNAMSIZ);
>
> - if (group_name[0] &&
> - isdigit((unsigned char)group_name[strlen(group_name) - 1]))
> + namelen = strlen(group_name);
> + if (namelen == 0)
> + errx(1, "setifgroup: group name empty");
> + if (namelen >= IFNAMSIZ)
> + errx(1, "setifgroup: group name too long");
> + if (isdigit((unsigned char)group_name[namelen - 1]))
> errx(1, "setifgroup: group names may not end in a digit");
>
> - if (strlcpy(ifgr.ifgr_group, group_name, IFNAMSIZ) >= IFNAMSIZ)
> - errx(1, "setifgroup: group name too long");
> + strlcpy(ifgr.ifgr_group, group_name, IFNAMSIZ);
> if (ioctl(sock, SIOCAIFGROUP, (caddr_t)&ifgr) == -1) {
> if (errno != EEXIST)
> err(1," SIOCAIFGROUP");
> @@ -1658,10 +1662,6 @@ unsetifgroup(const char *group_name, int
>
> memset(&ifgr, 0, sizeof(ifgr));
> strlcpy(ifgr.ifgr_name, ifname, IFNAMSIZ);
> -
> - if (group_name[0] &&
> - isdigit((unsigned char)group_name[strlen(group_name) - 1]))
> - errx(1, "unsetifgroup: group names may not end in a digit");
>
> if (strlcpy(ifgr.ifgr_group, group_name, IFNAMSIZ) >= IFNAMSIZ)
> errx(1, "unsetifgroup: group name too long");
interface group name validation
Hi,
Next try to fix syzkaller crash
https://syzkaller.appspot.com/bug?id=54e16dc5bce6929e14b42e2f1379f1c18f62be43
Interface group names must fit into IFNAMSIZ and be unique. But
the kernel makes the unique check before trunkating with strlcpy().
So there can be two interfaces groups with the same name. The kif
is created by a name lookup. The trunkated names are equal so there
is only one kif owned by both groups. When both groups are destroyed,
the single kif is removed twice from the RB tree.
- Check length of group name before doing the unique check.
- The empty group name was allowed. That does not make much sense.
Does anyone use the empty interface group?
- Use the same check in kernel and ifconfig userland.
- ifconfig -group does not need name sanitation. The kernel will
just report that it does not exist.
ok?
bluhm
Index: sys/net/if.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/if.c,v
retrieving revision 1.627
diff -u -p -r1.627 if.c
--- sys/net/if.c8 Feb 2021 12:30:10 - 1.627
+++ sys/net/if.c9 Feb 2021 20:47:34 -
@@ -2621,9 +2621,11 @@ if_addgroup(struct ifnet *ifp, const cha
struct ifg_list *ifgl;
struct ifg_group*ifg = NULL;
struct ifg_member *ifgm;
+ size_t namelen;
- if (groupname[0] && groupname[strlen(groupname) - 1] >= '0' &&
- groupname[strlen(groupname) - 1] <= '9')
+ namelen = strlen(groupname);
+ if (namelen == 0 || namelen >= IFNAMSIZ ||
+ (groupname[namelen - 1] >= '0' && groupname[namelen - 1] <= '9'))
return (EINVAL);
TAILQ_FOREACH(ifgl, &ifp->if_groups, ifgl_next)
Index: sbin/ifconfig/ifconfig.c
===
RCS file: /data/mirror/openbsd/cvs/src/sbin/ifconfig/ifconfig.c,v
retrieving revision 1.432
diff -u -p -r1.432 ifconfig.c
--- sbin/ifconfig/ifconfig.c16 Jan 2021 17:44:29 - 1.432
+++ sbin/ifconfig/ifconfig.c9 Feb 2021 21:02:50 -
@@ -1634,16 +1634,20 @@ void
setifgroup(const char *group_name, int dummy)
{
struct ifgroupreq ifgr;
+ size_t namelen;
memset(&ifgr, 0, sizeof(ifgr));
strlcpy(ifgr.ifgr_name, ifname, IFNAMSIZ);
- if (group_name[0] &&
- isdigit((unsigned char)group_name[strlen(group_name) - 1]))
+ namelen = strlen(group_name);
+ if (namelen == 0)
+ errx(1, "setifgroup: group name empty");
+ if (namelen >= IFNAMSIZ)
+ errx(1, "setifgroup: group name too long");
+ if (isdigit((unsigned char)group_name[namelen - 1]))
errx(1, "setifgroup: group names may not end in a digit");
- if (strlcpy(ifgr.ifgr_group, group_name, IFNAMSIZ) >= IFNAMSIZ)
- errx(1, "setifgroup: group name too long");
+ strlcpy(ifgr.ifgr_group, group_name, IFNAMSIZ);
if (ioctl(sock, SIOCAIFGROUP, (caddr_t)&ifgr) == -1) {
if (errno != EEXIST)
err(1," SIOCAIFGROUP");
@@ -1658,10 +1662,6 @@ unsetifgroup(const char *group_name, int
memset(&ifgr, 0, sizeof(ifgr));
strlcpy(ifgr.ifgr_name, ifname, IFNAMSIZ);
-
- if (group_name[0] &&
- isdigit((unsigned char)group_name[strlen(group_name) - 1]))
- errx(1, "unsetifgroup: group names may not end in a digit");
if (strlcpy(ifgr.ifgr_group, group_name, IFNAMSIZ) >= IFNAMSIZ)
errx(1, "unsetifgroup: group name too long");
