Date: Mon, 27 Jun 2016 22:12:11 +0000 (UTC) From: Bryan Drewery <bdrewery@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-10@freebsd.org Subject: svn commit: r302238 - in stable/10/sys: kern sys Message-ID: <201606272212.u5RMCBxJ038688@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: bdrewery Date: Mon Jun 27 22:12:11 2016 New Revision: 302238 URL: https://svnweb.freebsd.org/changeset/base/302238 Log: MFC r300792,r300851,r301580: r300792: exec: Add credential change information into imgp for process_exec hook. r300851: exec: get rid of one vnode lock/unlock pair in do_execve r301580: Old process credentials for setuid execve must not be dereferenced when the process credentials were not changed. This can happen if an error occured trying to activate the setuid binary. And on error, if new credentials were not yet assigned, they must be freed to not create the leak. Modified: stable/10/sys/kern/kern_exec.c stable/10/sys/sys/imgact.h Directory Properties: stable/10/ (props changed) Modified: stable/10/sys/kern/kern_exec.c ============================================================================== --- stable/10/sys/kern/kern_exec.c Mon Jun 27 22:10:07 2016 (r302237) +++ stable/10/sys/kern/kern_exec.c Mon Jun 27 22:12:11 2016 (r302238) @@ -359,7 +359,7 @@ do_execve(td, args, mac_p) { struct proc *p = td->td_proc; struct nameidata nd; - struct ucred *newcred = NULL, *oldcred; + struct ucred *oldcred; struct uidinfo *euip = NULL; register_t *stack_base; int error, i; @@ -367,7 +367,7 @@ do_execve(td, args, mac_p) struct vattr attr; int (*img_first)(struct image_params *); struct pargs *oldargs = NULL, *newargs = NULL; - struct sigacts *oldsigacts, *newsigacts; + struct sigacts *oldsigacts = NULL, *newsigacts = NULL; #ifdef KTRACE struct vnode *tracevp = NULL; struct ucred *tracecred = NULL; @@ -407,6 +407,7 @@ do_execve(td, args, mac_p) imgp->proc = p; imgp->attr = &attr; imgp->args = args; + oldcred = p->p_ucred; #ifdef MAC error = mac_execve_enter(imgp, mac_p); @@ -488,6 +489,87 @@ interpret: goto exec_fail_dealloc; imgp->proc->p_osrel = 0; + + /* + * Implement image setuid/setgid. + * + * Determine new credentials before attempting image activators + * so that it can be used by process_exec handlers to determine + * credential/setid changes. + * + * Don't honor setuid/setgid if the filesystem prohibits it or if + * the process is being traced. + * + * We disable setuid/setgid/etc in capability mode on the basis + * that most setugid applications are not written with that + * environment in mind, and will therefore almost certainly operate + * incorrectly. In principle there's no reason that setugid + * applications might not be useful in capability mode, so we may want + * to reconsider this conservative design choice in the future. + * + * XXXMAC: For the time being, use NOSUID to also prohibit + * transitions on the file system. + */ + credential_changing = 0; + credential_changing |= (attr.va_mode & S_ISUID) && + oldcred->cr_uid != attr.va_uid; + credential_changing |= (attr.va_mode & S_ISGID) && + oldcred->cr_gid != attr.va_gid; +#ifdef MAC + will_transition = mac_vnode_execve_will_transition(oldcred, imgp->vp, + interpvplabel, imgp); + credential_changing |= will_transition; +#endif + + if (credential_changing && +#ifdef CAPABILITY_MODE + ((oldcred->cr_flags & CRED_FLAG_CAPMODE) == 0) && +#endif + (imgp->vp->v_mount->mnt_flag & MNT_NOSUID) == 0 && + (p->p_flag & P_TRACED) == 0) { + imgp->credential_setid = true; + VOP_UNLOCK(imgp->vp, 0); + imgp->newcred = crdup(oldcred); + if (attr.va_mode & S_ISUID) { + euip = uifind(attr.va_uid); + change_euid(imgp->newcred, euip); + } + vn_lock(imgp->vp, LK_EXCLUSIVE | LK_RETRY); + if (attr.va_mode & S_ISGID) + change_egid(imgp->newcred, attr.va_gid); + /* + * Implement correct POSIX saved-id behavior. + * + * XXXMAC: Note that the current logic will save the + * uid and gid if a MAC domain transition occurs, even + * though maybe it shouldn't. + */ + change_svuid(imgp->newcred, imgp->newcred->cr_uid); + change_svgid(imgp->newcred, imgp->newcred->cr_gid); + } else { + /* + * Implement correct POSIX saved-id behavior. + * + * XXX: It's not clear that the existing behavior is + * POSIX-compliant. A number of sources indicate that the + * saved uid/gid should only be updated if the new ruid is + * not equal to the old ruid, or the new euid is not equal + * to the old euid and the new euid is not equal to the old + * ruid. The FreeBSD code always updates the saved uid/gid. + * Also, this code uses the new (replaced) euid and egid as + * the source, which may or may not be the right ones to use. + */ + if (oldcred->cr_svuid != oldcred->cr_uid || + oldcred->cr_svgid != oldcred->cr_gid) { + VOP_UNLOCK(imgp->vp, 0); + imgp->newcred = crdup(oldcred); + vn_lock(imgp->vp, LK_EXCLUSIVE | LK_RETRY); + change_svuid(imgp->newcred, imgp->newcred->cr_uid); + change_svgid(imgp->newcred, imgp->newcred->cr_gid); + } + } + /* The new credentials are installed into the process later. */ + /* * If the current process has a special image activator it * wants to try first, call it. For example, emulating shell @@ -545,6 +627,11 @@ interpret: vput(newtextvp); vm_object_deallocate(imgp->object); imgp->object = NULL; + imgp->credential_setid = false; + if (imgp->newcred != NULL) { + crfree(imgp->newcred); + imgp->newcred = NULL; + } /* set new name to that of the interpreter */ NDINIT(&nd, LOOKUP, LOCKLEAF | FOLLOW | SAVENAME, UIO_SYSSPACE, imgp->interpreter_name, td); @@ -611,8 +698,6 @@ interpret: bcopy(imgp->args->begin_argv, newargs->ar_args, i); } - vn_lock(imgp->vp, LK_SHARED | LK_RETRY); - /* * For security and other reasons, signal handlers cannot * be shared after an exec. The new process gets a copy of the old @@ -623,15 +708,13 @@ interpret: oldsigacts = p->p_sigacts; newsigacts = sigacts_alloc(); sigacts_copy(newsigacts, oldsigacts); - } else { - oldsigacts = NULL; - newsigacts = NULL; /* satisfy gcc */ } + vn_lock(imgp->vp, LK_SHARED | LK_RETRY); + PROC_LOCK(p); if (oldsigacts) p->p_sigacts = newsigacts; - oldcred = p->p_ucred; /* Stop profiling */ stopprofclock(p); @@ -663,38 +746,9 @@ interpret: } /* - * Implement image setuid/setgid. - * - * Don't honor setuid/setgid if the filesystem prohibits it or if - * the process is being traced. - * - * We disable setuid/setgid/etc in capability mode on the basis - * that most setugid applications are not written with that - * environment in mind, and will therefore almost certainly operate - * incorrectly. In principle there's no reason that setugid - * applications might not be useful in capability mode, so we may want - * to reconsider this conservative design choice in the future. - * - * XXXMAC: For the time being, use NOSUID to also prohibit - * transitions on the file system. + * Implement image setuid/setgid installation. */ - credential_changing = 0; - credential_changing |= (attr.va_mode & S_ISUID) && oldcred->cr_uid != - attr.va_uid; - credential_changing |= (attr.va_mode & S_ISGID) && oldcred->cr_gid != - attr.va_gid; -#ifdef MAC - will_transition = mac_vnode_execve_will_transition(oldcred, imgp->vp, - interpvplabel, imgp); - credential_changing |= will_transition; -#endif - - if (credential_changing && -#ifdef CAPABILITY_MODE - ((oldcred->cr_flags & CRED_FLAG_CAPMODE) == 0) && -#endif - (imgp->vp->v_mount->mnt_flag & MNT_NOSUID) == 0 && - (p->p_flag & P_TRACED) == 0) { + if (imgp->credential_setid) { /* * Turn off syscall tracing for set-id programs, except for * root. Record any set-id flags first to make sure that @@ -720,62 +774,28 @@ interpret: VOP_UNLOCK(imgp->vp, 0); setugidsafety(td); error = fdcheckstd(td); - if (error != 0) - goto done1; - newcred = crdup(oldcred); - euip = uifind(attr.va_uid); vn_lock(imgp->vp, LK_SHARED | LK_RETRY); + if (error != 0) + goto exec_fail_dealloc; PROC_LOCK(p); - /* - * Set the new credentials. - */ - if (attr.va_mode & S_ISUID) - change_euid(newcred, euip); - if (attr.va_mode & S_ISGID) - change_egid(newcred, attr.va_gid); #ifdef MAC if (will_transition) { - mac_vnode_execve_transition(oldcred, newcred, imgp->vp, - interpvplabel, imgp); + mac_vnode_execve_transition(oldcred, imgp->newcred, + imgp->vp, interpvplabel, imgp); } #endif - /* - * Implement correct POSIX saved-id behavior. - * - * XXXMAC: Note that the current logic will save the - * uid and gid if a MAC domain transition occurs, even - * though maybe it shouldn't. - */ - change_svuid(newcred, newcred->cr_uid); - change_svgid(newcred, newcred->cr_gid); - proc_set_cred(p, newcred); } else { if (oldcred->cr_uid == oldcred->cr_ruid && oldcred->cr_gid == oldcred->cr_rgid) p->p_flag &= ~P_SUGID; - /* - * Implement correct POSIX saved-id behavior. - * - * XXX: It's not clear that the existing behavior is - * POSIX-compliant. A number of sources indicate that the - * saved uid/gid should only be updated if the new ruid is - * not equal to the old ruid, or the new euid is not equal - * to the old euid and the new euid is not equal to the old - * ruid. The FreeBSD code always updates the saved uid/gid. - * Also, this code uses the new (replaced) euid and egid as - * the source, which may or may not be the right ones to use. - */ - if (oldcred->cr_svuid != oldcred->cr_uid || - oldcred->cr_svgid != oldcred->cr_gid) { - PROC_UNLOCK(p); - VOP_UNLOCK(imgp->vp, 0); - newcred = crdup(oldcred); - vn_lock(imgp->vp, LK_SHARED | LK_RETRY); - PROC_LOCK(p); - change_svuid(newcred, newcred->cr_uid); - change_svgid(newcred, newcred->cr_gid); - proc_set_cred(p, newcred); - } + } + /* + * Set the new credentials. + */ + if (imgp->newcred != NULL) { + proc_set_cred(p, imgp->newcred); + crfree(oldcred); + oldcred = NULL; } /* @@ -847,38 +867,7 @@ interpret: SDT_PROBE1(proc, , , exec__success, args->fname); - VOP_UNLOCK(imgp->vp, 0); -done1: - /* - * Free any resources malloc'd earlier that we didn't use. - */ - if (euip != NULL) - uifree(euip); - if (newcred != NULL) - crfree(oldcred); - - /* - * Handle deferred decrement of ref counts. - */ - if (oldtextvp != NULL) - vrele(oldtextvp); -#ifdef KTRACE - if (tracevp != NULL) - vrele(tracevp); - if (tracecred != NULL) - crfree(tracecred); -#endif - vn_lock(imgp->vp, LK_SHARED | LK_RETRY); - pargs_drop(oldargs); - pargs_drop(newargs); - if (oldsigacts != NULL) - sigacts_free(oldsigacts); - exec_fail_dealloc: - - /* - * free various allocated resources - */ if (imgp->firstpage != NULL) exec_unmap_first_page(imgp); @@ -908,24 +897,43 @@ exec_fail_dealloc: * the S_EXEC bit set. */ STOPEVENT(p, S_EXEC, 0); - goto done2; - } - + } else { exec_fail: - /* we're done here, clear P_INEXEC */ - PROC_LOCK(p); - p->p_flag &= ~P_INEXEC; - PROC_UNLOCK(p); + /* we're done here, clear P_INEXEC */ + PROC_LOCK(p); + p->p_flag &= ~P_INEXEC; + PROC_UNLOCK(p); + + SDT_PROBE1(proc, , , exec__failure, error); + } - SDT_PROBE1(proc, , , exec__failure, error); + if (imgp->newcred != NULL && oldcred != NULL) + crfree(imgp->newcred); -done2: #ifdef MAC mac_execve_exit(imgp); mac_execve_interpreter_exit(interpvplabel); #endif exec_free_args(args); + /* + * Handle deferred decrement of ref counts. + */ + if (oldtextvp != NULL) + vrele(oldtextvp); +#ifdef KTRACE + if (tracevp != NULL) + vrele(tracevp); + if (tracecred != NULL) + crfree(tracecred); +#endif + pargs_drop(oldargs); + pargs_drop(newargs); + if (oldsigacts != NULL) + sigacts_free(oldsigacts); + if (euip != NULL) + uifree(euip); + if (error && imgp->vmspace_destroyed) { /* sorry, no more process anymore. exit gracefully */ exit1(td, W_EXITCODE(0, SIGABRT)); Modified: stable/10/sys/sys/imgact.h ============================================================================== --- stable/10/sys/sys/imgact.h Mon Jun 27 22:10:07 2016 (r302237) +++ stable/10/sys/sys/imgact.h Mon Jun 27 22:12:11 2016 (r302238) @@ -38,6 +38,8 @@ #define MAXSHELLCMDLEN PAGE_SIZE +struct ucred; + struct image_args { char *buf; /* pointer to string buffer */ char *begin_argv; /* beginning of argv in buf */ @@ -81,6 +83,8 @@ struct image_params { int pagesizeslen; vm_prot_t stack_prot; u_long stack_sz; + struct ucred *newcred; /* new credentials if changing */ + bool credential_setid; /* true if becoming setid */ }; #ifdef _KERNEL
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201606272212.u5RMCBxJ038688>