Bug 219472

Summary: Out of bounds access in vlan
Product: Base System Reporter: CTurt <ecturt>
Component: kernAssignee: Mark Johnston <markj>
Status: Closed FIXED    
Severity: Affects Only Me CC: emaste, markj, net, op
Priority: ---    
Version: CURRENT   
Hardware: Any   
OS: Any   

Description CTurt 2017-05-23 12:36:18 UTC
This whole struct (`ifv_mib`) is user controllable through the `IFDATA_LINKSPECIFIC` sysctl command:

https://github.com/freebsd/freebsd/blob/release/11.0.1/sys/net/if_mib.c#L117

	case IFDATA_LINKSPECIFIC:
		error = SYSCTL_OUT(req, ifp->if_linkmib, ifp->if_linkmiblen);
		if (error || !req->newptr)
			goto out;

		error = SYSCTL_IN(req, ifp->if_linkmib, ifp->if_linkmiblen);
		if (error)
			goto out;
			break;

In the case of `struct ifvlan`, the contained `if_linkmib` is a `struct ifv_linkmib` containing a `uint16_t` called `ifvm_vid`:

https://github.com/freebsd/freebsd/blob/release/11.0.1/sys/net/if_vlan.c#L108

struct	ifvlan {
	struct	ifvlantrunk *ifv_trunk;
	struct	ifnet *ifv_ifp;
#define	TRUNK(ifv)	((ifv)->ifv_trunk)
#define	PARENT(ifv)	((ifv)->ifv_trunk->parent)
	void	*ifv_cookie;
	int	ifv_pflags;	/* special flags we have set on parent */
	struct	ifv_linkmib {
		int	ifvm_encaplen;	/* encapsulation length */
		int	ifvm_mtufudge;	/* MTU fudged by this much */
		int	ifvm_mintu;	/* min transmission unit */
		uint16_t ifvm_proto;	/* encapsulation ethertype */
		uint16_t ifvm_tag;	/* tag to apply on packets leaving if */
              	uint16_t ifvm_vid;	/* VLAN ID */
		uint8_t	ifvm_pcp;	/* Priority Code Point (PCP). */
	}	ifv_mib;
	SLIST_HEAD(, vlan_mc_entry) vlan_mc_listhead;
#ifndef VLAN_ARRAY
	LIST_ENTRY(ifvlan) ifv_list;
#endif
};
#define	ifv_proto	ifv_mib.ifvm_proto
#define	ifv_tag		ifv_mib.ifvm_tag
#define	ifv_vid 	ifv_mib.ifvm_vid
#define	ifv_pcp		ifv_mib.ifvm_pcp
#define	ifv_encaplen	ifv_mib.ifvm_encaplen
#define	ifv_mtufudge	ifv_mib.ifvm_mtufudge
#define	ifv_mintu	ifv_mib.ifvm_mintu

Thus, it follows that `ifv->ifv_vid` is a completely user controlled `uint16_t`, through the `IFDATA_LINKSPECIFIC` `sysctl` name.

This value is used as an index to perform reads and writes on the `vlans` array of size `0x1000` in multiple places.

https://github.com/freebsd/freebsd/blob/release/11.0.1/sys/net/if_vlan.c#L427

static __inline int
vlan_inshash(struct ifvlantrunk *trunk, struct ifvlan *ifv)
{

	if (trunk->vlans[ifv->ifv_vid] != NULL)
		return EEXIST;
	trunk->vlans[ifv->ifv_vid] = ifv;
	trunk->refcnt++;

	return (0);
}

static __inline int
vlan_remhash(struct ifvlantrunk *trunk, struct ifvlan *ifv)
{

	trunk->vlans[ifv->ifv_vid] = NULL;
	trunk->refcnt--;

	return (0);
}

...

However, this is a static array of size `VLAN_ARRAY_SIZE` (`0x1000`) elements:

https://github.com/freebsd/freebsd/blob/release/11.0.1/sys/net/ethernet.h#L86

#define EVL_VLID_MASK 0x0FFF

https://github.com/freebsd/freebsd/blob/release/11.0.1/sys/net/if_vlan.c#L89

struct ifvlantrunk {
	struct	ifnet   *parent;	/* parent interface of this trunk */
	struct	rmlock	lock;
#ifdef VLAN_ARRAY
#define	VLAN_ARRAY_SIZE	(EVL_VLID_MASK + 1)
	struct	ifvlan	*vlans[VLAN_ARRAY_SIZE]; /* static table */
#else
	struct	ifvlanhead *hash;	/* dynamic hash-list table */
	uint16_t	hmask;
	uint16_t	hwidth;
#endif
	int		refcnt;
};

So, out of bounds access is possible if `ifv_vid` is set to a value greater than `0xfff`.
Comment 1 Mark Johnston freebsd_committer 2019-01-08 01:04:52 UTC
vlan(4) doesn't even export the struct ifv_linkmib structure to userspace.  It never has, as far as I can see, and all other uses of if_linkmib use it to export standard MIBs to userspace.  I think it's a mistake that vlan(4) sets if_linkmib to begin with.

It's also worth nothing that no modern drivers set if_linkmib, so the IFDATA_LINKSPECIFIC interface is effectively obsolete.
Comment 2 Mark Johnston freebsd_committer 2019-01-08 01:14:18 UTC
https://reviews.freebsd.org/D18779
Comment 3 commit-hook freebsd_committer 2019-01-09 16:47:37 UTC
A commit references this bug:

Author: markj
Date: Wed Jan  9 16:47:16 UTC 2019
New revision: 342887
URL: https://svnweb.freebsd.org/changeset/base/342887

Log:
  Stop setting if_linkmib in vlan(4) ifnets.

  There are several reasons:
  - The structure being exported via IFDATA_LINKSPECIFIC doesn't appear
    to be a standard MIB.
  - The structure being exported is private to the kernel and always
    has been.
  - No other drivers in common use set the if_linkmib field.
  - Because IFDATA_LINKSPECIFIC can be used to overwrite the linkmib
    structure, a privileged user could use it to corrupt internal
    vlan(4) state. [1]

  PR:		219472
  Reported by:	CTurt <ecturt@gmail.com> [1]
  Reviewed by:	kp (previous version)
  MFC after:	1 week
  Sponsored by:	The FreeBSD Foundation
  Differential Revision:	https://reviews.freebsd.org/D18779

Changes:
  head/sys/net/if_vlan.c
Comment 4 commit-hook freebsd_committer 2019-01-16 03:08:37 UTC
A commit references this bug:

Author: markj
Date: Wed Jan 16 03:07:33 UTC 2019
New revision: 343076
URL: https://svnweb.freebsd.org/changeset/base/343076

Log:
  MFC r342887:
  Stop setting if_linkmib in vlan(4) ifnets.

  PR:	219472

Changes:
_U  stable/11/
  stable/11/sys/net/if_vlan.c
_U  stable/12/
  stable/12/sys/net/if_vlan.c