Bug 206626
| Summary: | Integer overflow in nfssvc system call | ||
|---|---|---|---|
| Product: | Base System | Reporter: | CTurt <ecturt> |
| Component: | kern | Assignee: | Ed Maste <emaste> |
| Status: | Closed FIXED | ||
| Severity: | Affects Many People | CC: | emaste, op, op, rmacklem, secteam, shawn.webb |
| Priority: | --- | Keywords: | security |
| Version: | CURRENT | Flags: | op:
mfc-stable10?
op: mfc-stable9? |
| Hardware: | Any | ||
| OS: | Any | ||
| URL: | https://reviews.freebsd.org/D6201 | ||
Any movement on this? 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); ... Any update? 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? (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? (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? 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. Would a jailed attacker be able to trigger this? Perchance with the hopes of breaking out of the jail? 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 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 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 |
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