From 9f64e64a6ee1fe44a05ed57893785fa9064125e1 Mon Sep 17 00:00:00 2001
From: Michael Pratt <mpratt@google.com>
Date: Wed, 5 Dec 2018 14:31:07 -0800
Subject: [PATCH] Enforce directory accessibility before delete Walk

By Walking before checking that the directory is writable and
executable, MayDelete may return the Walk error (e.g., ENOENT) which
would normally be masked by a permission error (EACCES).

PiperOrigin-RevId: 224222453
Change-Id: I108a7f730e6bdaa7f277eaddb776267c00805475
---
 pkg/sentry/fs/dirent.go | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/pkg/sentry/fs/dirent.go b/pkg/sentry/fs/dirent.go
index 4c0482036..c4918a11b 100644
--- a/pkg/sentry/fs/dirent.go
+++ b/pkg/sentry/fs/dirent.go
@@ -1461,6 +1461,10 @@ func checkSticky(ctx context.Context, dir *Dirent, victim *Dirent) error {
 //
 // Compare Linux kernel fs/namei.c:may_delete.
 func MayDelete(ctx context.Context, root, dir *Dirent, name string) error {
+	if err := dir.Inode.CheckPermission(ctx, PermMask{Write: true, Execute: true}); err != nil {
+		return err
+	}
+
 	victim, err := dir.Walk(ctx, root, name)
 	if err != nil {
 		return err
@@ -1470,11 +1474,11 @@ func MayDelete(ctx context.Context, root, dir *Dirent, name string) error {
 	return mayDelete(ctx, dir, victim)
 }
 
+// mayDelete determines whether `victim`, a child of `dir`, can be deleted or
+// renamed by `ctx`.
+//
+// Preconditions: `dir` is writable and executable by `ctx`.
 func mayDelete(ctx context.Context, dir, victim *Dirent) error {
-	if err := dir.Inode.CheckPermission(ctx, PermMask{Write: true, Execute: true}); err != nil {
-		return err
-	}
-
 	if err := checkSticky(ctx, dir, victim); err != nil {
 		return err
 	}
@@ -1512,6 +1516,15 @@ func Rename(ctx context.Context, root *Dirent, oldParent *Dirent, oldName string
 		return syscall.ENOENT
 	}
 
+	// Do we have general permission to remove from oldParent and
+	// create/replace in newParent?
+	if err := oldParent.Inode.CheckPermission(ctx, PermMask{Write: true, Execute: true}); err != nil {
+		return err
+	}
+	if err := newParent.Inode.CheckPermission(ctx, PermMask{Write: true, Execute: true}); err != nil {
+		return err
+	}
+
 	// renamed is the dirent that will be renamed to something else.
 	renamed, err := oldParent.walk(ctx, root, oldName, false /* may unlock */)
 	if err != nil {
@@ -1549,10 +1562,7 @@ func Rename(ctx context.Context, root *Dirent, oldParent *Dirent, oldName string
 			return err
 		}
 
-		// Make sure we can create a new child in the new parent.
-		if err := newParent.Inode.CheckPermission(ctx, PermMask{Write: true, Execute: true}); err != nil {
-			return err
-		}
+		// newName doesn't exist; simply create it below.
 	} else {
 		// Check constraints on the dirent being replaced.
 
@@ -1560,7 +1570,7 @@ func Rename(ctx context.Context, root *Dirent, oldParent *Dirent, oldName string
 		// across the Rename, so must call DecRef manually (no defer).
 
 		// Check that we can delete replaced.
-		if err := mayDelete(ctx, oldParent, renamed); err != nil {
+		if err := mayDelete(ctx, newParent, replaced); err != nil {
 			replaced.DecRef()
 			return err
 		}