Changeset View
Standalone View
sys/kern/vfs_lookup.c
Show First 20 Lines • Show All 176 Lines • ▼ Show 20 Lines | |||||
#ifdef CAPABILITY_MODE | #ifdef CAPABILITY_MODE | ||||
/* | /* | ||||
* In capability mode, lookups must be "strictly relative" (i.e. | * In capability mode, lookups must be "strictly relative" (i.e. | ||||
* not an absolute path, and not containing '..' components) to | * not an absolute path, and not containing '..' components) to | ||||
* a real file descriptor, not the pseudo-descriptor AT_FDCWD. | * a real file descriptor, not the pseudo-descriptor AT_FDCWD. | ||||
*/ | */ | ||||
if (error == 0 && IN_CAPABILITY_MODE(td) && | if (error == 0 && IN_CAPABILITY_MODE(td) && | ||||
(cnp->cn_flags & NOCAPCHECK) == 0) { | (cnp->cn_flags & NOCAPCHECK) == 0) { | ||||
ndp->ni_strictrelative = 1; | ndrequire_strict_relative_lookups(ndp, ENOTCAPABLE); | ||||
if (ndp->ni_dirfd == AT_FDCWD) { | if (ndp->ni_dirfd == AT_FDCWD) { | ||||
mjg: The caller is free to specify any error they want, yet this checks specifically for EPERM. I… | |||||
Done Inline ActionsWe actually want to honour EPERM above ENOTCAPABLE, since the user application code can always set the O_BENEATH flag and might expect EPERM when there is a failure. In all other cases, we preserve the existing ABI by returning ENOTCAPABLE. jonathan: We actually want to honour `EPERM` above `ENOTCAPABLE`, since the user application code can… | |||||
Done Inline ActionsBut proposed change would honour EPERM just fine. Further, when one day other caller will want to return EACCESS, it will just work, while with current patch whatever it sets will be converted to ENOTCAPABLE. That's only a nit though. mjg: But proposed change would honour EPERM just fine. Further, when one day other caller will want… | |||||
Not Done Inline ActionsThat's a fair point. Another development (not visible here on the issue tracker) is that we're considering just creating a brand new errno for this kind of failure. That would provide more information to the caller about the true nature of the error. It would also allow us to stay compatible between the FreeBSD and Linux implementations even though Linux doesn't have ENOTCAPABLE, and we could #define this value to ENOTCAPABLE on FreeBSD 10 to maintain ABI compatibility. jonathan: That's a fair point. Another development (not visible here on the issue tracker) is that we're… | |||||
#ifdef KTRACE | #ifdef KTRACE | ||||
if (KTRPOINT(td, KTR_CAPFAIL)) | if (KTRPOINT(td, KTR_CAPFAIL)) | ||||
ktrcapfail(CAPFAIL_LOOKUP, NULL, NULL); | ktrcapfail(CAPFAIL_LOOKUP, NULL, NULL); | ||||
#endif | #endif | ||||
error = ECAPMODE; | error = ECAPMODE; | ||||
} | } | ||||
} | } | ||||
#endif | #endif | ||||
▲ Show 20 Lines • Show All 48 Lines • ▼ Show 20 Lines | #ifdef CAPABILITIES | ||||
* all lookups relative to it must also be | * all lookups relative to it must also be | ||||
* strictly relative. | * strictly relative. | ||||
*/ | */ | ||||
CAP_ALL(&rights); | CAP_ALL(&rights); | ||||
if (!cap_rights_contains(&ndp->ni_filecaps.fc_rights, | if (!cap_rights_contains(&ndp->ni_filecaps.fc_rights, | ||||
&rights) || | &rights) || | ||||
ndp->ni_filecaps.fc_fcntls != CAP_FCNTL_ALL || | ndp->ni_filecaps.fc_fcntls != CAP_FCNTL_ALL || | ||||
ndp->ni_filecaps.fc_nioctls != -1) { | ndp->ni_filecaps.fc_nioctls != -1) { | ||||
ndp->ni_strictrelative = 1; | ndrequire_strict_relative_lookups(ndp, ENOTCAPABLE); | ||||
} | } | ||||
#endif | #endif | ||||
} | } | ||||
if (error != 0 || dp != NULL) { | if (error != 0 || dp != NULL) { | ||||
FILEDESC_SUNLOCK(fdp); | FILEDESC_SUNLOCK(fdp); | ||||
if (error == 0 && dp->v_type != VDIR) { | if (error == 0 && dp->v_type != VDIR) { | ||||
vrele(dp); | vrele(dp); | ||||
error = ENOTDIR; | error = ENOTDIR; | ||||
Show All 22 Lines | for (;;) { | ||||
if (*(cnp->cn_nameptr) == '/') { | if (*(cnp->cn_nameptr) == '/') { | ||||
vrele(dp); | vrele(dp); | ||||
if (ndp->ni_strictrelative != 0) { | if (ndp->ni_strictrelative != 0) { | ||||
#ifdef KTRACE | #ifdef KTRACE | ||||
if (KTRPOINT(curthread, KTR_CAPFAIL)) | if (KTRPOINT(curthread, KTR_CAPFAIL)) | ||||
ktrcapfail(CAPFAIL_LOOKUP, NULL, NULL); | ktrcapfail(CAPFAIL_LOOKUP, NULL, NULL); | ||||
#endif | #endif | ||||
namei_cleanup_cnp(cnp); | namei_cleanup_cnp(cnp); | ||||
return (ENOTCAPABLE); | return (ndp->ni_strictrelative); | ||||
} | } | ||||
while (*(cnp->cn_nameptr) == '/') { | while (*(cnp->cn_nameptr) == '/') { | ||||
cnp->cn_nameptr++; | cnp->cn_nameptr++; | ||||
ndp->ni_pathlen--; | ndp->ni_pathlen--; | ||||
} | } | ||||
dp = ndp->ni_rootdir; | dp = ndp->ni_rootdir; | ||||
VREF(dp); | VREF(dp); | ||||
} | } | ||||
▲ Show 20 Lines • Show All 321 Lines • ▼ Show 20 Lines | #endif | ||||
* .. in the other filesystem. | * .. in the other filesystem. | ||||
* 4. If the vnode is the top directory of | * 4. If the vnode is the top directory of | ||||
* the jail or chroot, don't let them out. | * the jail or chroot, don't let them out. | ||||
*/ | */ | ||||
if (cnp->cn_flags & ISDOTDOT) { | if (cnp->cn_flags & ISDOTDOT) { | ||||
if (ndp->ni_strictrelative != 0) { | if (ndp->ni_strictrelative != 0) { | ||||
#ifdef KTRACE | #ifdef KTRACE | ||||
if (KTRPOINT(curthread, KTR_CAPFAIL)) | if (KTRPOINT(curthread, KTR_CAPFAIL)) | ||||
ktrcapfail(CAPFAIL_LOOKUP, NULL, NULL); | ktrcapfail(CAPFAIL_LOOKUP, NULL, NULL); | ||||
Done Inline ActionsHmm. If we're limited by O_BENEATH, should we be using a different KTR trace function here? rwatson: Hmm. If we're limited by O_BENEATH, should we be using a different KTR trace function here? | |||||
Done Inline ActionsHow would you feel about using KTR_NAMEI instead? We could use KTR_CAPFAIL for Capsicum-caused failures and KTR_NAMEI for O_BENEATH-induced failures, but that's adding a lot of complexity and it's not entirely clear to me how we should handle the "both" case... jonathan: How would you feel about using `KTR_NAMEI` instead? We could use `KTR_CAPFAIL` for Capsicum… | |||||
Done Inline ActionsHurum. Agreed it would make things more complex, but it probably is the right thing. How about we simply make the decision based on whether it's ENOTCAPABLE or EPERM? rwatson: Hurum. Agreed it would make things more complex, but it probably is the right thing. How about… | |||||
Not Done Inline ActionsOk, I can try that and see how it looks. jonathan: Ok, I can try that and see how it looks. | |||||
#endif | #endif | ||||
Not Done Inline ActionsOk, @rwatson, how does this ktrace update look? jonathan: Ok, @rwatson, how does this ktrace update look? | |||||
error = ENOTCAPABLE; | error = ndp->ni_strictrelative; | ||||
goto bad; | goto bad; | ||||
} | } | ||||
if ((cnp->cn_flags & ISLASTCN) != 0 && | if ((cnp->cn_flags & ISLASTCN) != 0 && | ||||
(cnp->cn_nameiop == DELETE || cnp->cn_nameiop == RENAME)) { | (cnp->cn_nameiop == DELETE || cnp->cn_nameiop == RENAME)) { | ||||
error = EINVAL; | error = EINVAL; | ||||
goto bad; | goto bad; | ||||
} | } | ||||
for (;;) { | for (;;) { | ||||
▲ Show 20 Lines • Show All 476 Lines • ▼ Show 20 Lines | if (!(flags & NDF_NO_DVP_RELE) && | ||||
ndp->ni_dvp = NULL; | ndp->ni_dvp = NULL; | ||||
} | } | ||||
if (unlock_dvp) | if (unlock_dvp) | ||||
VOP_UNLOCK(ndp->ni_dvp, 0); | VOP_UNLOCK(ndp->ni_dvp, 0); | ||||
if (!(flags & NDF_NO_STARTDIR_RELE) && | if (!(flags & NDF_NO_STARTDIR_RELE) && | ||||
(ndp->ni_cnd.cn_flags & SAVESTART)) { | (ndp->ni_cnd.cn_flags & SAVESTART)) { | ||||
vrele(ndp->ni_startdir); | vrele(ndp->ni_startdir); | ||||
ndp->ni_startdir = NULL; | ndp->ni_startdir = NULL; | ||||
} | |||||
} | |||||
void | |||||
ndrequire_strict_relative_lookups(struct nameidata *ndp, int errnum) | |||||
{ | |||||
/* | |||||
* Monotonic ordering of possible field values: | |||||
* | |||||
* EPERM - user-level code has requested an O_BENEATH lookup | |||||
* ENOTCAPABLE - Capsicum demands strict relative lookups | |||||
* 0 - allow relative lookups | |||||
* | |||||
* We can always move up this list but never down. | |||||
*/ | |||||
rwatsonUnsubmitted Done Inline ActionsI might move this comment up to just above the function, and provide a little more explanation as to why this is needed, not just what it does. rwatson: I might move this comment up to just above the function, and provide a little more explanation… | |||||
jonathanAuthorUnsubmitted Done Inline ActionsI can certainly do this. I was wondering, however, if this function would be considered useful or overly heavyweight: the call could be replaced by a conditional assignment in most places (and an unconditional assignment in one). I like the clarity of an explicit function call, but it can't really be inlined (unless I move the implementation to a header file, which I also don't like). jonathan: I can certainly do this. I was wondering, however, if this function would be considered useful… | |||||
Done Inline ActionsI would be OK with moving to various in situ -- especially of the form "... CAPMODE ? ENOTCAPABLE : EPERM". rwatson: I would be OK with moving to various in situ -- especially of the form "... CAPMODE ? | |||||
KASSERT((errnum == EPERM) || (errnum == ENOTCAPABLE), | |||||
("invalid errno (not EPERM or ENOTCAPABLE)")); | |||||
Done Inline ActionsThis seems superfluous with the second KASSERT below drysdale_google.com: This seems superfluous with the second KASSERT below | |||||
Done Inline ActionsThis function will go away anyhow based the above discussion with Robert. jonathan: This function will go away anyhow based the above discussion with Robert. | |||||
switch (errnum) { | |||||
case EPERM: | |||||
ndp->ni_strictrelative = errnum; | |||||
break; | |||||
case ENOTCAPABLE: | |||||
if (ndp->ni_strictrelative != EPERM) { | |||||
ndp->ni_strictrelative = errnum; | |||||
} | |||||
break; | |||||
default: | |||||
KASSERT(false, ("invalid errno for strict relative lookups")); | |||||
rwatsonUnsubmitted Done Inline ActionsPossibly this should be a panic(), as should this occur at runtime, we want to fail closed, not open? Also, do include the proposed error number in the panic message. rwatson: Possibly this should be a panic(), as should this occur at runtime, we want to fail closed, not… | |||||
} | } | ||||
} | } | ||||
/* | /* | ||||
* Determine if there is a suitable alternate filename under the specified | * Determine if there is a suitable alternate filename under the specified | ||||
* prefix for the specified path. If the create flag is set, then the | * prefix for the specified path. If the create flag is set, then the | ||||
* alternate prefix will be used so long as the parent directory exists. | * alternate prefix will be used so long as the parent directory exists. | ||||
* This is used by the various compatiblity ABIs so that Linux binaries prefer | * This is used by the various compatiblity ABIs so that Linux binaries prefer | ||||
▲ Show 20 Lines • Show All 109 Lines • Show Last 20 Lines |
The caller is free to specify any error they want, yet this checks specifically for EPERM. I would say that's inconsequential for no good reason.
Instead, you can if (ndp->ni_nonrelativeerrno == 0) ndp->ni_nonrelativeerrno = ENOTCAPABLE;
while a no-op change with current consumers, may come in handy.