Date: Thu, 26 May 2016 23:18:55 +0000 (UTC) From: Bryan Drewery <bdrewery@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r300792 - in head/sys: kern sys Message-ID: <201605262318.u4QNIteu069688@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: bdrewery Date: Thu May 26 23:18:54 2016 New Revision: 300792 URL: https://svnweb.freebsd.org/changeset/base/300792 Log: exec: Add credential change information into imgp for process_exec hook. This allows an EVENTHANDLER(process_exec) hook to see if the new image will cause credentials to change whether due to setgid/setuid or because of POSIX saved-id semantics. This adds 3 new fields into image_params: struct ucred *newcred Non-null if the credentials will change. bool credential_setid True if the new image is setuid or setgid. This will pre-determine the new credentials before invoking the image activators, where the process_exec hook is called. The new credentials will be installed into the process in the same place as before, after image activators are done handling the image. MFC after: 2 weeks Reviewed by: kib Sponsored by: EMC / Isilon Storage Division Differential Revision: https://reviews.freebsd.org/D6544 Modified: head/sys/kern/kern_exec.c head/sys/sys/imgact.h Modified: head/sys/kern/kern_exec.c ============================================================================== --- head/sys/kern/kern_exec.c Thu May 26 23:08:57 2016 (r300791) +++ head/sys/kern/kern_exec.c Thu May 26 23:18:54 2016 (r300792) @@ -356,7 +356,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; @@ -404,6 +404,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); @@ -485,6 +486,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 @@ -542,6 +624,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); @@ -639,7 +726,6 @@ interpret: PROC_LOCK(p); if (oldsigacts) p->p_sigacts = newsigacts; - oldcred = p->p_ucred; /* Stop profiling */ stopprofclock(p); @@ -671,38 +757,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 @@ -728,61 +785,24 @@ interpret: error = fdcheckstd(td); if (error != 0) goto done1; - newcred = crdup(oldcred); - euip = uifind(attr.va_uid); vn_lock(imgp->vp, LK_SHARED | LK_RETRY); 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); /* * Store the vp for use in procfs. This vnode was referenced by namei @@ -856,14 +876,6 @@ interpret: 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) @@ -881,10 +893,12 @@ done1: sigacts_free(oldsigacts); exec_fail_dealloc: - /* * free various allocated resources */ + if (euip != NULL) + uifree(euip); + if (imgp->firstpage != NULL) exec_unmap_first_page(imgp); @@ -926,6 +940,8 @@ exec_fail: SDT_PROBE1(proc, , , exec__failure, error); done2: + if (imgp->newcred != NULL) + crfree(oldcred); #ifdef MAC mac_execve_exit(imgp); mac_execve_interpreter_exit(interpvplabel); Modified: head/sys/sys/imgact.h ============================================================================== --- head/sys/sys/imgact.h Thu May 26 23:08:57 2016 (r300791) +++ head/sys/sys/imgact.h Thu May 26 23:18:54 2016 (r300792) @@ -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 */ @@ -82,6 +84,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?201605262318.u4QNIteu069688>