The kernel permissions check code was not checking deletability for
authorMatthew Dillon <dillon@apollo.backplane.com>
Mon, 4 May 2009 06:08:19 +0000 (23:08 -0700)
committerMatthew Dillon <dillon@apollo.backplane.com>
Mon, 4 May 2009 06:22:23 +0000 (23:22 -0700)
the rename source or the directory sticky bit for rename targets which
existed.

This only effected HAMMER which assumes the kernel is responsible for
permissions checks.

Reported-by: YONETANI Tomokazu <qhwt+dfly@les.ath.cx>
sys/kern/vfs_nlookup.c
sys/kern/vfs_syscalls.c
sys/sys/nlookup.h
sys/sys/vnode.h
sys/vfs/nfs/nfs_subs.c

index 886109f..3efa97f 100644 (file)
@@ -279,10 +279,17 @@ nlookup_simple(const char *str, enum uio_seg seg,
  * execute (search) permission.  nlookup does not examine the access 
  * permissions on the returned element.
  *
- * If NLC_CREATE or NLC_DELETE is set the last directory must allow node
- * creation (VCREATE/VDELETE), and an error code of 0 will be returned for
- * a non-existant target.  Otherwise a non-existant target will cause
- * ENOENT to be returned.
+ * If NLC_CREATE is set the last directory must allow node creation,
+ * and an error code of 0 will be returned for a non-existant
+ * target (not ENOENT).
+ *
+ * If NLC_RENAME is set the last directory mut allow node deletion,
+ * plus the sticky check is made, and an error code of 0 will be returned
+ * for a non-existant target (not ENOENT).  NLC_RENAME is set used for
+ * the rename target.
+ *
+ * If NLC_DELETE is set the last directory mut allow node deletion,
+ * plus the sticky check is made.
  *
  * If NLC_REFDVP is set nd->nl_dvp will be set to the directory vnode
  * of the returned entry.  The vnode will be referenced, but not locked,
@@ -387,9 +394,6 @@ nlookup(struct nlookupdata *nd)
         * The namecache topology is not allowed to be disconnected, so 
         * encountering a NULL parent will generate EINVAL.  This typically
         * occurs when a directory is removed out from under a process.
-        *
-        * If NLC_DELETE is set neither '.' or '..' can be the last component
-        * of a path.
         */
        if (nlc.nlc_namelen == 1 && nlc.nlc_nameptr[0] == '.') {
            cache_get(&nd->nl_nch, &nch);
@@ -447,24 +451,30 @@ nlookup(struct nlookupdata *nd)
 
        /*
         * Early completion.  ENOENT is not an error if this is the last
-        * component and NLC_CREATE was requested.  Note that ncp->nc_error
-        * is left as ENOENT in that case, which we check later on.
+        * component and NLC_CREATE or NLC_RENAME (rename target) was
+        * requested.  Note that ncp->nc_error is left as ENOENT in that
+        * case, which we check later on.
+        *
+        * NOTE: For NLC_RENAME in the ENOENT case we do a VCREATE test,
+        *       same as for NLC_CREATE.
         *
         * Also handle invalid '.' or '..' components terminating a path
-        * during removal.  The standard requires this and pax pretty
-        * stupidly depends on it.
+        * for a create/rename/delete.  The standard requires this and pax
+        * pretty stupidly depends on it.
         */
        for (xptr = ptr; *xptr == '/'; ++xptr)
                ;
        if (*xptr == 0) {
-           if (error == ENOENT && (nd->nl_flags & NLC_CREATE)) {
+           if (error == ENOENT && (nd->nl_flags & (NLC_CREATE | NLC_RENAME))) {
                if (nd->nl_flags & NLC_NFS_RDONLY)
                        error = EROFS;
                else
                        error = naccess(&nch, VCREATE, nd->nl_cred, NULL);
            }
-           if (error == 0 && wasdotordotdot && (nd->nl_flags & NLC_DELETE))
+           if (error == 0 && wasdotordotdot &&
+               (nd->nl_flags & (NLC_CREATE | NLC_RENAME | NLC_DELETE))) {
                error = EINVAL;
+           }
        }
 
        /*
@@ -589,10 +599,18 @@ nlookup(struct nlookupdata *nd)
        /*
         * Successful lookup of last element.
         *
-        * Check directory permissions if a deletion is specified.
+        * Check directory permissions if a deletion or rename (target)
+        * is specified.  This also handles the sticky test.
+        *
+        * We already checked permissions for creates in the early
+        * termination code above.
         */
-       if (*ptr == 0 && (nd->nl_flags & NLC_DELETE)) {
-           if ((error = naccess(&nch, VDELETE, nd->nl_cred, NULL)) != 0) {
+       if (*ptr == 0 && (nd->nl_flags & (NLC_DELETE | NLC_RENAME))) {
+           if (nd->nl_flags & NLC_DELETE)
+               error = naccess(&nch, VDELETE, nd->nl_cred, NULL);
+           else
+               error = naccess(&nch, VRENAME, nd->nl_cred, NULL);
+           if (error) {
                cache_put(&nch);
                break;
            }
@@ -713,21 +731,20 @@ fail:
  * Generally check the V* access bits from sys/vnode.h.  All specified bits
  * must pass for this function to return 0.
  *
- * If VCREATE is specified and the target ncp represents a non-existant
- * file or dir, or if VDELETE is specified and the target exists, the parent
- * directory is checked for VWRITE.  If VEXCL is specified and the target
- * ncp represents a positive hit, an error is returned.
+ * The file does not have to exist when checking VCREATE or VRENAME access.
+ *
+ * The file must not exist if VEXCL is specified.
  *
- * If VCREATE is not specified and the target does not exist (negative hit),
- * ENOENT is returned.  Note that nlookup() does not (and should not) return
- * ENOENT for non-existant leafs.
+ * Directory permissions in general are tested for VCREATE if the file
+ * does not exist, VDELETE if the file does exist, and VRENAME whether
+ * the file exists or not.
  *
- * If VDELETE is specified we also do sticky-bit tests on the parent
- * directory.
+ * The directory sticky bit is tested for VDELETE and VRENAME.  NOTE: For
+ * VRENAME we only care if the target exists.
  *
  * The passed ncp may or may not be locked.  The caller should use a
- * locked ncp on leaf lookups, especially for VCREATE, VDELETE, and VEXCL
- * checks.
+ * locked ncp on leaf lookups, especially for VCREATE, VRENAME, VDELETE,
+ * and VEXCL checks.
  */
 int
 naccess(struct nchandle *nch, int vmode, struct ucred *cred, int *stickyp)
@@ -746,9 +763,14 @@ naccess(struct nchandle *nch, int vmode, struct ucred *cred, int *stickyp)
     error = nch->ncp->nc_error;
     sticky = 0;
 
-    if (vmode & (VDELETE|VCREATE|VEXCL)) {
+    /*
+     * Directory permissions and VEXCL checks.  Do a precursor conditional
+     * to reduce overhead since most access checks are for read-only.
+     */
+    if (vmode & (VDELETE|VRENAME|VCREATE|VEXCL)) {
        if (((vmode & VCREATE) && nch->ncp->nc_vp == NULL) ||
-           ((vmode & VDELETE) && nch->ncp->nc_vp != NULL)
+           ((vmode & VDELETE) && nch->ncp->nc_vp != NULL) ||
+           (vmode & VRENAME)
        ) {
            if ((par.ncp = nch->ncp->nc_parent) == NULL) {
                if (error != EAGAIN)
@@ -757,7 +779,7 @@ naccess(struct nchandle *nch, int vmode, struct ucred *cred, int *stickyp)
                par.mount = nch->mount;
                cache_hold(&par);
                error = naccess(&par, VWRITE, cred, &sticky);
-               if ((vmode & VDELETE) && sticky)
+               if ((vmode & (VDELETE | VRENAME)) && sticky)
                    vmode |= VSVTX;
                cache_drop(&par);
            }
@@ -768,7 +790,7 @@ naccess(struct nchandle *nch, int vmode, struct ucred *cred, int *stickyp)
     if (error == 0) {
        error = cache_vget(nch, cred, LK_SHARED, &vp);
        if (error == ENOENT) {
-           if (vmode & VCREATE)
+           if (vmode & (VCREATE | VRENAME))
                error = 0;
        } else if (error == 0) {
            /* XXX cache the va in the namecache or in the vnode */
@@ -812,8 +834,10 @@ naccess_va(struct vattr *va, int vmode, struct ucred *cred)
 
     /*
      * Test the immutable bit for files, directories, and softlinks.
+     *
+     * NOTE: Only called for VRENAME if the target exists.
      */
-    if (vmode & (VWRITE|VDELETE)) {
+    if (vmode & (VWRITE|VDELETE|VRENAME)) {
        if (va->va_type == VDIR || va->va_type == VLNK || va->va_type == VREG) {
            if (va->va_flags & IMMUTABLE)
                return (EPERM);
@@ -842,14 +866,15 @@ naccess_va(struct vattr *va, int vmode, struct ucred *cred)
     }
 
     /*
-     * If VSVTX is set only the owner may access the file.  This bit is
-     * typically set for VDELETE checks on files in sticky directories
-     * when the user does not own the directory.  Note that other V bits
-     * are not typically set (for deletions we usually just care about
-     * the directory permissions, not the file permissions).
+     * If VSVTX is set only the owner may create or delete the file.
+     * This bit is typically set for VDELETE checks from unlink or
+     * the source file in a rename, and for VCREATE checks from the
+     * target file in a rename.
      *
-     * The effect on VDELETE checks is thus to not allow the deletion
-     * of the file, and otherwise allow it.
+     * Note that other V bits are not typically set in the VSVTX case.
+     * For creations and deletions we usually just care about directory
+     * permissions, not file permissions.  So if this test passes the
+     * return value winds up being 0.
      */
     if (vmode & VSVTX)
        return(EACCES);
index f9ef507..96b997f 100644 (file)
@@ -3137,7 +3137,7 @@ kern_rename(struct nlookupdata *fromnd, struct nlookupdata *tond)
        int error;
 
        bwillinode(1);
-       fromnd->nl_flags |= NLC_REFDVP;
+       fromnd->nl_flags |= NLC_REFDVP | NLC_DELETE;
        if ((error = nlookup(fromnd)) != 0)
                return (error);
        if ((fnchd.ncp = fromnd->nl_nch.ncp->nc_parent) == NULL)
@@ -3158,7 +3158,7 @@ kern_rename(struct nlookupdata *fromnd, struct nlookupdata *tond)
        fromnd->nl_flags &= ~NLC_NCPISLOCKED;
        cache_unlock(&fromnd->nl_nch);
 
-       tond->nl_flags |= NLC_CREATE | NLC_REFDVP;
+       tond->nl_flags |= NLC_RENAME | NLC_REFDVP;
        if ((error = nlookup(tond)) != 0) {
                cache_drop(&fnchd);
                return (error);
index 085d15c..3859bc9 100644 (file)
@@ -103,6 +103,7 @@ struct nlookupdata {
 #define NLC_LOCKVP             0x00000040      /* nl_open_vp from vn_open */
 #define NLC_CREATE             0x00000080
 #define NLC_DELETE             0x00000100
+#define NLC_RENAME             0x00000200      /* rename (target) */
 #define NLC_NFS_RDONLY         0x00010000      /* set by nfs_namei() only */
 #define NLC_NFS_NOSOFTLINKTRAV 0x00020000      /* do not traverse softlnks */
 #define NLC_REFDVP             0x00040000      /* set ref'd/unlocked nl_dvp */
index d12a042..a44bda0 100644 (file)
@@ -315,8 +315,10 @@ struct vnode {
  * Modes.  Note that these V-modes must match file S_I*USR, SUID, SGID,
  * and SVTX flag bits.
  *
- * VOWN, VCREATE, VDELETE, and VEXCL may only be used in naccess() calls.
+ * VOWN, VCREATE, VDELETE, VRENAME, and VEXCL may only be used in
+ * naccess() calls.
  */
+#define VRENAME        0200000         /* set with VCREATE or VDELETE if rename */
 #define VOWN   0100000         /* succeed if file owner or (other flags) */
 #define VDELETE        040000          /* delete if the file/dir exists */
 #define VCREATE        020000          /* create if the file/dir does not exist */
index 3e67bc3..11dc90f 100644 (file)
@@ -1580,8 +1580,10 @@ nfs_namei(struct nlookupdata *nd, struct ucred *cred, int nameiop,
        }
        if (rdonly)
                flags |= NLC_NFS_RDONLY;
-       if (nameiop == NAMEI_CREATE || nameiop == NAMEI_RENAME)
+       if (nameiop == NAMEI_CREATE)
                flags |= NLC_CREATE;
+       if (nameiop == NAMEI_RENAME)
+               flags |= NLC_RENAME;
 
        /*
         * We need a starting ncp from the directory vnode dp.  dp must not