sftp: fail rm and rmdir of a missing path, and mkdir of an existing one (#12168)

* sftp: fail rm and rmdir of a missing path, and mkdir of an existing one

* sftp: create directories exclusively

---------

Co-authored-by: user.email <chrislusf@users.noreply.github.com>

weed/sftpd/sftp_copy_existing_test.go

@@ -227,3 +227,27 @@ func TestRequireParentDir(t *testing.T) {
 		t.Fatalf("file as parent: got %v, want os.ErrNotExist", err)
 	}
 }
+
+// rm takes files and rmdir directories; a missing path is an error for both.
+func TestRequireEntry(t *testing.T) {
+	fs := newCopyTestServer(t, "", map[string]*filer_pb.Entry{
+		"/home/d":       {Name: "d", IsDirectory: true},
+		"/home/d/a.txt": {Name: "a.txt"},
+	})
+
+	if err := fs.requireEntry("/home/d/a.txt", false); err != nil {
+		t.Fatalf("rm of a file: %v", err)
+	}
+	if err := fs.requireEntry("/home/d", true); err != nil {
+		t.Fatalf("rmdir of a directory: %v", err)
+	}
+	if err := fs.requireEntry("/home/d/missing", false); err != os.ErrNotExist {
+		t.Fatalf("rm of a missing path: got %v, want os.ErrNotExist", err)
+	}
+	if err := fs.requireEntry("/home/d/a.txt", true); err != os.ErrNotExist {
+		t.Fatalf("rmdir of a file: got %v, want os.ErrNotExist", err)
+	}
+	if err := fs.requireEntry("/home/d", false); err == nil || err == os.ErrNotExist {
+		t.Fatalf("rm of a directory: got %v, want a failure", err)
+	}
+}

weed/sftpd/sftp_filer.go

@@ -256,6 +256,9 @@ func writeZeros(w io.Writer, size int64) error {
 }
 
 func (fs *SftpServer) removeEntry(absPath string) error {
+	if err := fs.requireEntry(absPath, false); err != nil {
+		return err
+	}
 	return fs.deleteEntry(absPath, false)
 }
 
@@ -523,7 +526,7 @@ func (fs *SftpServer) makeDir(absPath string) error {
 		return err
 	}
 	// default mode and ownership
-	err := filer_pb.Mkdir(context.Background(), fs, string(dir), name, func(entry *filer_pb.Entry) {
+	err := filer_pb.MkdirExclusive(context.Background(), fs, string(dir), name, func(entry *filer_pb.Entry) {
 		mode := uint32(0755 | os.ModeDir)
 		// Defensive check: all paths should be under HomeDir after toAbsolutePath translation
 		if absPath == fs.user.HomeDir || strings.HasPrefix(absPath, fs.user.HomeDir+"/") {
@@ -540,6 +543,9 @@ func (fs *SftpServer) makeDir(absPath string) error {
 		}
 		entry.Extended["creator"] = []byte(fs.user.Username)
 	})
+	if errors.Is(err, filer_pb.ErrEntryAlreadyExists) {
+		return os.ErrExist
+	}
 	return err
 }
 
@@ -562,9 +568,29 @@ func (fs *SftpServer) requireParentDir(absPath string) error {
 
 // removeDir deletes a directory.
 func (fs *SftpServer) removeDir(absPath string) error {
+	if err := fs.requireEntry(absPath, true); err != nil {
+		return err
+	}
 	return fs.deleteEntry(absPath, false)
 }
 
+// requireEntry fails unless absPath exists and is a directory exactly when
+// isDir is set (rm takes files, rmdir directories), with the errors OpenSSH's
+// sftp-server gives: no such file for a missing path or rmdir of a file.
+func (fs *SftpServer) requireEntry(absPath string, isDir bool) error {
+	entry, err := fs.getEntry(absPath)
+	if err != nil {
+		return err
+	}
+	if isDir && !entry.IsDirectory {
+		return os.ErrNotExist
+	}
+	if !isDir && entry.IsDirectory {
+		return fmt.Errorf("%s is a directory", absPath)
+	}
+	return nil
+}
+
 func (fs *SftpServer) putFile(filepath string, reader io.Reader, user *user.User) error {
 	dir, filename := util.FullPath(filepath).DirAndName()
 	// Escape the path so a "?" in the filename cannot inject filer query commands like cp.from/mv.from.

weed/sftpd/sftp_permissions_test.go

@@ -210,6 +210,51 @@ func TestRequireParentDirEnforced(t *testing.T) {
 	}
 }
 
+// mkdir of a taken name fails instead of replacing the entry, file or
+// directory alike: the create is exclusive, so a concurrent creator wins.
+func TestMakeDirExistingPath(t *testing.T) {
+	lis, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("listen: %v", err)
+	}
+	filer := &permissionTestFiler{entries: map[string]*filer_pb.Entry{
+		"/":               {IsDirectory: true, Attributes: &filer_pb.FuseAttributes{FileMode: 0755}},
+		"/home":           {IsDirectory: true, Attributes: &filer_pb.FuseAttributes{FileMode: 0755}},
+		"/home/alice":     {IsDirectory: true, Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0755}},
+		"/home/alice/d":   {IsDirectory: true, Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0755}},
+		"/home/alice/a.t": {Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0644}},
+	}}
+	grpcServer := grpc.NewServer()
+	filer_pb.RegisterSeaweedFilerServer(grpcServer, filer)
+	go func() { _ = grpcServer.Serve(lis) }()
+	t.Cleanup(grpcServer.Stop)
+
+	_, port, _ := net.SplitHostPort(lis.Addr().String())
+	fs := &SftpServer{
+		filerAddr:      pb.ServerAddress("127.0.0.1:0." + port),
+		grpcDialOption: grpc.WithTransportCredentials(insecure.NewCredentials()),
+		user: &user.User{
+			Username: "alice", Uid: 1000, Gid: 1000, HomeDir: "/home/alice",
+			Permissions: map[string][]string{
+				"/home/alice": {"*"},
+			},
+		},
+	}
+
+	if err := fs.makeDir("/home/alice/d"); err != os.ErrExist {
+		t.Fatalf("mkdir of an existing directory: got %v, want os.ErrExist", err)
+	}
+	if err := fs.makeDir("/home/alice/a.t"); err != os.ErrExist {
+		t.Fatalf("mkdir of an existing file: got %v, want os.ErrExist", err)
+	}
+	if got := filer.entries["/home/alice/d"].Attributes.FileMode; got != 0755 {
+		t.Errorf("existing directory was overwritten: mode %o", got)
+	}
+	if filer.entries["/home/alice/a.t"].IsDirectory {
+		t.Error("existing file was overwritten by a directory")
+	}
+}
+
 // renameEntry must honor a more specific ACL on an existing target: renaming
 // over a read-only file deletes it, so it requires delete permission on the
 // target file itself, not only write permission on the target directory.
@@ -363,7 +408,16 @@ func (f *permissionTestFiler) AtomicRenameEntry(_ context.Context, req *filer_pb
 }
 
 func (f *permissionTestFiler) CreateEntry(_ context.Context, req *filer_pb.CreateEntryRequest) (*filer_pb.CreateEntryResponse, error) {
-	f.entries[req.Directory+"/"+req.Entry.Name] = req.Entry
+	key := req.Directory + "/" + req.Entry.Name
+	if req.OExcl {
+		if _, ok := f.entries[key]; ok {
+			return &filer_pb.CreateEntryResponse{
+				Error:     "entry already exists",
+				ErrorCode: filer_pb.FilerError_ENTRY_ALREADY_EXISTS,
+			}, nil
+		}
+	}
+	f.entries[key] = req.Entry
 	return &filer_pb.CreateEntryResponse{}, nil
 }