Bug 208808 - Heap overflow in nlm system call
Summary: Heap overflow in nlm system call
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 bugs mailing list
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-14 21:28 UTC by CTurt
Modified: 2016-10-07 15:15 UTC (History)
5 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-04-14 21:28:06 UTC
There is a kernel heap overflow in the `nlm_register_services` function, reachable through the `nlm` system call. Due to the `priv_check` call in `sys_nlm_syscall`, this is triggerable as root only, so is non critical.

`nlm_register_services` is reachable with user controlled `addr_count`, where the following call to `malloc` is performed:

    xprts = malloc(addr_count * sizeof(SVCXPRT *), M_NLM, M_WAITOK|M_ZERO);

Firstly, since this allocation passes the `M_WAITOK` flag, by specifying large enough sizes, a user could cause DoS. However, more interestingly, by specifying an `addr_count` such that integer overflow occurs during the multiplication, an undersized buffer would be allocated, leading to heap overflow later on.

Full code path goes through `sys_nlm_syscall -> nlm_server_main -> nlm_register_services`, `(sys/nlm/nlm_prot_impl.c)`:

int
sys_nlm_syscall(struct thread *td, struct nlm_syscall_args *uap)
{
	int error;

#if __FreeBSD_version >= 700000
	error = priv_check(td, PRIV_NFS_LOCKD);
#else
	error = suser(td);
#endif
	if (error)
		return (error);

	nlm_debug_level = uap->debug_level;
	nlm_grace_threshold = time_uptime + uap->grace_period;
	nlm_next_idle_check = time_uptime + NLM_IDLE_PERIOD;

	return nlm_server_main(uap->addr_count, uap->addrs);
}

static int
nlm_server_main(int addr_count, char **addrs)
{
	struct thread *td = curthread;
	int error;
	SVCPOOL *pool = NULL;
	struct sockopt opt;
	int portlow;
#ifdef INET6
	struct sockaddr_in6 sin6;
#endif
	struct sockaddr_in sin;
	my_id id;
	sm_stat smstat;
	struct timeval timo;
	enum clnt_stat stat;
	struct nlm_host *host, *nhost;
	struct nlm_waiting_lock *nw;
	vop_advlock_t *old_nfs_advlock;
	vop_reclaim_t *old_nfs_reclaim;

	if (nlm_is_running != 0) {
		NLM_ERR("NLM: can't start server - "
		    "it appears to be running already\n");
		return (EPERM);
	}

	...

	error = nlm_register_services(pool, addr_count, addrs);

	...
}

static int
nlm_register_services(SVCPOOL *pool, int addr_count, char **addrs)
{
	static rpcvers_t versions[] = {
		NLM_SM, NLM_VERS, NLM_VERSX, NLM_VERS4
	};
	static void (*dispatchers[])(struct svc_req *, SVCXPRT *) = {
		nlm_prog_0, nlm_prog_1, nlm_prog_3, nlm_prog_4
	};
	static const int version_count = sizeof(versions) / sizeof(versions[0]);

	SVCXPRT **xprts;
	char netid[16];
	char uaddr[128];
	struct netconfig *nconf;
	int i, j, error;

	if (!addr_count) {
		NLM_ERR("NLM: no service addresses given - can't start server");
		return (EINVAL);
	}

	xprts = malloc(addr_count * sizeof(SVCXPRT *), M_NLM, M_WAITOK|M_ZERO);

	...
}

I propose that there should be a bound check on `addr_count` in `nlm_register_services`, such as the following:

	if (!addr_count) {
		NLM_ERR("NLM: no service addresses given - can't start server");
		return (EINVAL);
	}

+	if (addr_count < 0 || addr_count > 256) {
+		NLM_ERR("NLM: too many service addresses given - can't start server");
+		return (EINVAL);
+	}
+
	xprts = malloc(addr_count * sizeof(SVCXPRT *), M_NLM, M_WAITOK|M_ZERO);
Comment 1 Sean Bruno freebsd_committer 2016-04-19 18:56:03 UTC
Rick:

is 256 the right amount here?
Comment 2 Rick Macklem freebsd_committer 2016-04-19 22:42:34 UTC
No idea. I'm not familiar with the NLM implementation (rpc.lockd).
Comment 3 Sean Bruno freebsd_committer 2016-04-19 22:56:33 UTC
(In reply to Rick Macklem from comment #2)
Fair.  I'll see if emailing Doug does anything.  :-)
Comment 4 dfr 2016-04-20 08:44:04 UTC
(In reply to Sean Bruno from comment #3)

I'm not sure if 256 is the right number either but it seems unlikely that a real server would have that many addresses. The patch is probably fine as-is although it would be nice if the error message also mentioned the limit on number of addresses.
Comment 5 commit-hook freebsd_committer 2016-04-20 15:31:43 UTC
A commit references this bug:

Author: sbruno
Date: Wed Apr 20 15:31:03 UTC 2016
New revision: 298351
URL: https://svnweb.freebsd.org/changeset/base/298351

Log:
  Avoid a possible heap overflow in our nlm code by limiting the number
  of service to the arbitrary value of 256.  Log an appropriate message
  that indicates the hard limit.

  PR:		208808
  Submitted by:	cturt@hardenedbsd.org
  Reviewed by:	dfr
  Obtained from:	HardenedBSD
  MFC after:	2 weeks

Changes:
  head/sys/nlm/nlm_prot_impl.c
Comment 6 Sean Bruno freebsd_committer 2016-10-07 15:15:15 UTC
this was MFC'd to stable/10:

r303173 | sbruno | 2016-07-21 21:09:47 -0600 (Thu, 21 Jul 2016) | 6 lines

MFC r298351

Avoid a possible heap overflow in our nlm code by limiting the number
of service to the arbitrary value of 256.  Log an appropriate message
that indicates the hard limit.