Bug 206626 - Integer overflow in nfssvc system call
Summary: Integer overflow in nfssvc system call
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Many People
Assignee: Ed Maste
URL: https://reviews.freebsd.org/D6201
Keywords: security
Depends on:
Blocks:
 
Reported: 2016-01-25 23:58 UTC by CTurt
Modified: 2016-10-07 14:49 UTC (History)
6 users (show)

See Also:
op: mfc-stable9?
op: mfc-stable10?


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description CTurt 2016-01-25 23:58:10 UTC
The system call `sys_nfssvc` calls `nfssvc_call`, which copies in some user data, and calls `nfssvc_idname`:

	if (uap->flag & NFSSVC_IDNAME) {
		if ((uap->flag & NFSSVC_NEWSTRUCT) != 0)
			error = copyin(uap->argp, &nid, sizeof(nid));
		else {
			error = copyin(uap->argp, &onid, sizeof(onid));
			if (error == 0) {
				nid.nid_flag = onid.nid_flag;
				nid.nid_uid = onid.nid_uid;
				nid.nid_gid = onid.nid_gid;
				nid.nid_usermax = onid.nid_usermax;
				nid.nid_usertimeout = onid.nid_usertimeout;
				nid.nid_name = onid.nid_name;
				nid.nid_namelen = onid.nid_namelen;
				nid.nid_ngroup = 0;
				nid.nid_grps = NULL;
			}
		}
		if (error)
			goto out;
		error = nfssvc_idname(&nid);
		goto out;

In `nfssvc_idname`, `nidp->nid_namelen` is user controllable, and is used without any bound checks:

/*
 * This function is called from the nfssvc(2) system call, to update the
 * kernel user/group name list(s) for the V4 owner and ownergroup attributes.
 */
APPLESTATIC int
nfssvc_idname(struct nfsd_idargs *nidp)
{
	...
	
	if (nidp->nid_flag & NFSID_INITIALIZE) {
		cp = malloc(nidp->nid_namelen + 1, M_NFSSTRING, M_WAITOK);
		error = copyin(CAST_USER_ADDR_T(nidp->nid_name), cp,
		    nidp->nid_namelen);
		if (error != 0) {
			free(cp, M_NFSSTRING);
			goto out;
		}
	
	...
}

Let's look at the disassembly of how `malloc` is called:

.text:FFFFFFFF807651AD                 mov     edi, [rdi+20h]
.text:FFFFFFFF807651B0                 mov     edx, 2
.text:FFFFFFFF807651B5                 mov     rsi, offset M_NEWNFSSTRING
.text:FFFFFFFF807651BC                 add     edi, 1
.text:FFFFFFFF807651BF                 movsxd  rdi, edi
.text:FFFFFFFF807651C2                 call    malloc

If a `nid_namelen` of `0xffffffff` is supplied, an allocation size of `0` bytes will be passed to `malloc`.

We then have `copyin` of `0xffffffff` bytes on this allocation of size `0`.

The bug is only triggerable as `root`, and I couldn't get any way to panic from writing to allocation of 0 bytes.

Code to play with if anyone is interested in exploring its potential further: https://gist.github.com/CTurt/957360482a4dc453f6a4
Comment 1 Shawn Webb 2016-03-01 20:58:10 UTC
Any movement on this?
Comment 2 CTurt 2016-04-13 19:29:18 UTC
It is possible to get panic from this bug by reducing the `nid_namelen` value to `0xfffffffe`. In this case, the system will panic due to a general protection fault in `vmem_alloc`. I've updated the PoC code to cause panic by this method (https://gist.github.com/CTurt/957360482a4dc453f6a4).

The patch for this bug is to add appropriate bound checks to `nfssvc_idname` in `sys/fs/nfs/nfs_commonsubs.c`:

APPLESTATIC int
nfssvc_idname(struct nfsd_idargs *nidp)
{
	struct nfsusrgrp *nusrp, *usrp, *newusrp;
	struct nfsuserhashhead *hp;
	int i;
	int error = 0;
	u_char *cp;

+	if (nidp->nid_namelen < 0 || nidp->nid_namelen > 128) {
+		error = EINVAL;
+		goto exit;
+	}
+
	...

Additionally, to better explain this bug, I've decided to go through the full code path to trigger it:

When supplying the `NFSSVC_IDNAME` flag to the `nfssvc` system call, after passing the privilege check, the `nfsd_call_nfscommon` function pointer will be called, which points to `nfssvc_nfscommon`:

https://github.com/freebsd/freebsd/blob/release/10.2.0/sys/nfs/nfs_nfssvc.c#L75

int
sys_nfssvc(struct thread *td, struct nfssvc_args *uap)
{
	int error;

	KASSERT(!mtx_owned(&Giant), ("nfssvc(): called with Giant"));

	AUDIT_ARG_CMD(uap->flag);

	/* Allow anyone to get the stats. */
	if ((uap->flag & ~NFSSVC_GETSTATS) != 0) {
		error = priv_check(td, PRIV_NFS_DAEMON);
		if (error != 0)
			return (error);
	}
	error = EINVAL;
	if ((uap->flag & (NFSSVC_ADDSOCK | NFSSVC_OLDNFSD | NFSSVC_NFSD)) &&
	    ...
	else if ((uap->flag & (NFSSVC_IDNAME | NFSSVC_GETSTATS |
	    NFSSVC_GSSDADDPORT | NFSSVC_GSSDADDFIRST | NFSSVC_GSSDDELETEALL |
	    NFSSVC_NFSUSERDPORT | NFSSVC_NFSUSERDDELPORT)) &&
	    nfsd_call_nfscommon != NULL)
		error = (*nfsd_call_nfscommon)(td, uap);
	...
	return (error);
}

`nfssvc_nfscommon` then calls `nfssvc_call`:

https://github.com/freebsd/freebsd/blob/release/10.2.0/sys/fs/nfs/nfs_commonport.c#L433

static int
nfssvc_nfscommon(struct thread *td, struct nfssvc_args *uap)
{
	int error;

	error = nfssvc_call(td, uap, td->td_ucred);
	NFSEXITCODE(error);
	return (error);
}

The `nfsd_idargs` struct will then be copied in from userland, and passed to `nfssvc_idname`:

static int
nfssvc_call(struct thread *p, struct nfssvc_args *uap, struct ucred *cred)
{
	int error = EINVAL;
	struct nfsd_idargs nid;

	if (uap->flag & NFSSVC_IDNAME) {
		error = copyin(uap->argp, (caddr_t)&nid, sizeof (nid));
		if (error)
			goto out;
		error = nfssvc_idname(&nid);
		goto out;
		...

In `nfssvc_idname` we have an allocation with `nidp->nid_namelen + 1`, and then `copyin` with `nidp->nid_namelen`. There were no bound checks on `nidp->nid_namelen`, so we have `malloc` with completely user controlled size:

https://github.com/freebsd/freebsd/blob/release/10.2.0/sys/fs/nfs/nfs_commonsubs.c#L3093

APPLESTATIC int
nfssvc_idname(struct nfsd_idargs *nidp)
{
	struct nfsusrgrp *nusrp, *usrp, *newusrp;
	struct nfsuserhashhead *hp;
	int i;
	int error = 0;
	u_char *cp;

	if (nidp->nid_flag & NFSID_INITIALIZE) {
	    cp = (u_char *)malloc(nidp->nid_namelen + 1,
		M_NFSSTRING, M_WAITOK);
	    error = copyin(CAST_USER_ADDR_T(nidp->nid_name), cp,
		nidp->nid_namelen);
		...
Comment 3 op 2016-05-03 20:35:25 UTC
Any update?
Comment 4 Rick Macklem freebsd_committer 2016-05-03 23:17:49 UTC
The nfssvc() syscall only allows these cases to be done
by root, so I don't see a problem?
(The syscall is only used by nfs daemons, but if some
 malicious program running as effective uid == 0 wants
 to crash the system it can.)

Are you really trying to make it impossible for a process
running as root to crash the system?
Comment 5 Shawn Webb 2016-05-03 23:23:49 UTC
(In reply to Rick Macklem from comment #4)

How about simply for code correctness sake? Incorrect code is okay because only root can trigger it?
Comment 6 Shawn Webb 2016-05-03 23:23:58 UTC
(In reply to Rick Macklem from comment #4)

How about simply for code correctness sake? Incorrect code is okay because only root can trigger it?
Comment 7 Rick Macklem freebsd_committer 2016-05-03 23:38:36 UTC
It is only broken if the application sends a bogus value for the
nid_namelen. Since the daemon is careful not to do that and it is
the only legitimate process that makes the call, I don't consider
that incorrect code.

If you want to commit a bounds check, go ahead.
Comment 8 Shawn Webb 2016-05-03 23:45:13 UTC
Would a jailed attacker be able to trigger this? Perchance with the hopes of breaking out of the jail?
Comment 9 Ed Maste freebsd_committer 2016-05-04 14:31:31 UTC
There's a minor error in the submitted patch (exit vs out label), and MAXHOSTNAMELEN seems like a more suitable bound to me rather than bare 128.

Up for review in https://reviews.freebsd.org/D6201
Comment 10 commit-hook freebsd_committer 2016-05-06 21:19:33 UTC
A commit references this bug:

Author: emaste
Date: Fri May  6 21:19:28 UTC 2016
New revision: 299199
URL: https://svnweb.freebsd.org/changeset/base/299199

Log:
  Add nid_namelen bounds check to nfssvc system call

  This is only allowed by root and only used by the nfs daemon, which
  should not provide an incorrect value. However, it's still good
  practice to validate data provided by userland.

  PR:		206626
  Reported by:	CTurt <cturt@hardenedbsd.org>
  Reviewed by:	rmacklem
  MFC after:	1 month
  Differential Revision:	https://reviews.freebsd.org/D6201

Changes:
  head/sys/fs/nfs/nfs_commonsubs.c
Comment 11 commit-hook freebsd_committer 2016-10-07 14:47:12 UTC
A commit references this bug:

Author: emaste
Date: Fri Oct  7 14:46:34 UTC 2016
New revision: 306809
URL: https://svnweb.freebsd.org/changeset/base/306809

Log:
  MFC r299199: Add nid_namelen bounds check to nfssvc system call

  This is only allowed by root and only used by the nfs daemon, which
  should not provide an incorrect value. However, it's still good
  practice to validate data provided by userland.

  PR:		206626

Changes:
_U  stable/10/
  stable/10/sys/fs/nfs/nfs_commonsubs.c