Bug 209545 - Heap overflows in an driver
Summary: Heap overflows in an driver
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-wireless
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2016-05-16 09:10 UTC by CTurt
Modified: 2016-10-07 15:02 UTC (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description CTurt 2016-05-16 09:10:58 UTC
The `an` driver doesn't perform bound checks on the lengths used by the `AIROFLSHGCHR` and `AIROFLSHPCHR` commands implemented through `SIOCGPRIVATE_0`, which leads to heap overflows from the `copyin` calls, accessible by a privileged user only.

sys/dev/an/if_an.c:

static int
an_ioctl(struct ifnet *ifp, u_long command, caddr_t data)
{
	...

	switch (command) {
	...
	case SIOCGPRIVATE_0:		/* used by Cisco client utility */
		if ((error = priv_check(td, PRIV_DRIVER)))
			goto out;
		error = copyin(ifr->ifr_data, &l_ioctl, sizeof(l_ioctl));
		if (error)
			goto out;
		mode = l_ioctl.command;

		AN_LOCK(sc);
		if (mode >= AIROGCAP && mode <= AIROGSTATSD32) {
			error = readrids(ifp, &l_ioctl);
		} else if (mode >= AIROPCAP && mode <= AIROPLEAPUSR) {
			error = writerids(ifp, &l_ioctl);
		} else if (mode >= AIROFLSHRST && mode <= AIRORESTART) {
			error = flashcard(ifp, &l_ioctl);
		} else {
			error =-1;
		}
		AN_UNLOCK(sc);
		if (!error) {
			/* copy out the updated command info */
			error = copyout(&l_ioctl, ifr->ifr_data, sizeof(l_ioctl));
		}
		break;
	...
out:

	return(error != 0);
}	

static int
flashcard(struct ifnet *ifp, struct aironet_ioctl *l_ioctl)
{
	int		z = 0, status;
	struct an_softc	*sc;

	sc = ifp->if_softc;
	if (sc->mpi350) {
		if_printf(ifp, "flashing not supported on MPI 350 yet\n");
		return(-1);
	}
	status = l_ioctl->command;

	switch (l_ioctl->command) {
	...
	case AIROFLSHGCHR:	/* Get char from aux */
		AN_UNLOCK(sc);
		status = copyin(l_ioctl->data, &sc->areq, l_ioctl->len);
		AN_LOCK(sc);
		if (status)
			return status;
		z = *(int *)&sc->areq;
		if ((status = flashgchar(ifp, z, 8000)) == 1)
			return 0;
		else
			return -1;
	case AIROFLSHPCHR:	/* Send char to card. */
		AN_UNLOCK(sc);
		status = copyin(l_ioctl->data, &sc->areq, l_ioctl->len);
		AN_LOCK(sc);
		if (status)
			return status;
		z = *(int *)&sc->areq;
		if ((status = flashpchar(ifp, z, 8000)) == -1)
			return -EIO;
		else
			return 0;
		break;
	...

	return -EINVAL;
}

I propose that the following bound checks are added to this command:

	case AIROFLSHGCHR:	/* Get char from aux */
+		if (l_ioctl->len > sizeof(sc->areq)) {
+			return -EINVAL;
+		}
...
	case AIROFLSHPCHR:	/* Send char to card. */
+		if (l_ioctl->len > sizeof(sc->areq)) {
+			return -EINVAL;
+		}
Comment 1 Sean Bruno freebsd_committer 2016-05-24 13:53:06 UTC
Test build running, will commit in a bit.
Comment 2 commit-hook freebsd_committer 2016-05-24 13:58:07 UTC
A commit references this bug:

Author: sbruno
Date: Tue May 24 13:57:24 UTC 2016
New revision: 300612
URL: https://svnweb.freebsd.org/changeset/base/300612

Log:
  Reject ioctl commands for FLSHGCHR and FLSHPCHR if the size is greater
  than sc->areq.  This is a bounds check to ensure we're not just cramming
  arbitrarily sized nonsense into the driver and overflowing the heap.

  PR:		209545
  Submitted by:	cturt@hardenedbsd.org
  MFC after:	2 weeks

Changes:
  head/sys/dev/an/if_an.c
Comment 3 commit-hook freebsd_committer 2016-07-22 03:26:07 UTC
A commit references this bug:

Author: sbruno
Date: Fri Jul 22 03:26:02 UTC 2016
New revision: 303177
URL: https://svnweb.freebsd.org/changeset/base/303177

Log:
  MFC r300612
  Reject ioctl commands for FLSHGCHR and FLSHPCHR if the size is greater
  than sc->areq.  This is a bounds check to ensure we're not just cramming
  arbitrarily sized nonsense into the driver and overflowing the heap.

  PR:		209545

Changes:
  stable/10/sys/dev/an/if_an.c