DragonFlyBSD Kernel Audit
DF-2996 / fix.diff
← back to finding ↓ download raw
--- a/sys/vfs/nfs/nfs_vnops.c
+++ b/sys/vfs/nfs/nfs_vnops.c
@@ -2991,9 +2991,17 @@
 {
 	struct sillyrename *sp;
 	struct nfsnode *np;
+	struct nfsnode *np2;
 	int error;
 
 	/*
+	 * np must be NULL on entry: nfs_lookitup() leaves *npp untouched
+	 * when it fails, and a garbage non-NULL *npp makes it take the
+	 * "update the file handle of this node" path on a wild pointer.
+	 */
+	np = NULL;
+
+	/*
 	 * Force finalization so the VOP_INACTIVE() call is not delayed.
 	 * This prevents cred structures from building up in nfsnodes
 	 * for deleted files.
@@ -3035,6 +3043,41 @@
 		goto bad;
 	error = nfs_lookitup(dvp, sp->s_name, sp->s_namlen, sp->s_cred,
 		cnp->cn_td, &np);
+
+	/*
+	 * The looked-up object MUST be the renamed file itself.  A
+	 * malicious (or buggy) server can return any file handle here -
+	 * including a directory's - and nfs_lookitup() then hands us
+	 * whatever nfsnode that handle maps to.  Storing sp through the
+	 * wrong np is a wild pointer write when the lookup failed, and a
+	 * type confusion against the n_cookies LIST_HEAD when the node
+	 * happens to be a directory (nfs_reclaim() would walk and free
+	 * sp as an nfsdmap).
+	 */
+	if (error == 0) {
+		np2 = VTONFS(vp);
+		/*
+		 * Wrong object, or the server's attributes retyped the
+		 * node (e.g. VREG -> VDIR) while nfs_lookitup() applied
+		 * them: either way n_sillyrename would alias n_cookies.
+		 */
+		if ((np != np2 && NFS_CMPFH(np, np2->n_fhp, np2->n_fhsize) == 0)
+		    || NFSTOV(np)->v_type != VREG) {
+			/* wrong object: release what lookitup acquired */
+			if (np == VTONFS(dvp)) {
+				vrele(dvp);
+			} else {
+				vput(NFSTOV(np));
+			}
+			error = EBADRPC;
+		}
+	}
+	if (error) {
+		vrele(sp->s_dvp);
+		crfree(sp->s_cred);
+		kfree((caddr_t)sp, M_NFSREQ);
+		return (error);
+	}
 	np->n_sillyrename = sp;
 	return (0);
 bad:
@@ -3085,6 +3128,7 @@
 		    np->n_fhsize = fhlen;
 		    newvp = NFSTOV(np);
 		} else if (NFS_CMPFH(dnp, nfhp, fhlen)) {
+		    np = dnp;	/* *npp must never stay uninitialized */
 		    vref(dvp);
 		    newvp = dvp;
 		} else {