From e3e1b36896750e9c4d07ecb6eaf1a738b08cd876 Mon Sep 17 00:00:00 2001
From: Ayush Ranjan <ayushranjan@google.com>
Date: Mon, 17 Aug 2020 13:24:09 -0700
Subject: [PATCH] [vfs] Do O_DIRECTORY check after resolving symlinks.

Fixes python runtime test test_glob.
Updates #3515

We were checking is the to-be-opened dentry is a dir or not before resolving
symlinks. We should check that after resolving symlinks.
This was preventing us from opening a symlink which pointed to a directory
with O_DIRECTORY.

Also added this check in tmpfs and removed a duplicate check.

PiperOrigin-RevId: 327085895
---
 pkg/sentry/fsimpl/gofer/filesystem.go | 6 +++---
 pkg/sentry/fsimpl/tmpfs/filesystem.go | 5 ++---
 test/syscalls/linux/open.cc           | 8 ++++++++
 3 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/pkg/sentry/fsimpl/gofer/filesystem.go b/pkg/sentry/fsimpl/gofer/filesystem.go
index 610a7ed78..a3903db33 100644
--- a/pkg/sentry/fsimpl/gofer/filesystem.go
+++ b/pkg/sentry/fsimpl/gofer/filesystem.go
@@ -886,9 +886,6 @@ afterTrailingSymlink:
 	if mustCreate {
 		return nil, syserror.EEXIST
 	}
-	if !child.isDir() && rp.MustBeDir() {
-		return nil, syserror.ENOTDIR
-	}
 	// Open existing child or follow symlink.
 	if child.isSymlink() && rp.ShouldFollowSymlink() {
 		target, err := child.readlink(ctx, rp.Mount())
@@ -901,6 +898,9 @@ afterTrailingSymlink:
 		start = parent
 		goto afterTrailingSymlink
 	}
+	if rp.MustBeDir() && !child.isDir() {
+		return nil, syserror.ENOTDIR
+	}
 	return child.openLocked(ctx, rp, &opts)
 }
 
diff --git a/pkg/sentry/fsimpl/tmpfs/filesystem.go b/pkg/sentry/fsimpl/tmpfs/filesystem.go
index a4864df53..cb8b2d944 100644
--- a/pkg/sentry/fsimpl/tmpfs/filesystem.go
+++ b/pkg/sentry/fsimpl/tmpfs/filesystem.go
@@ -389,9 +389,8 @@ afterTrailingSymlink:
 		start = &parentDir.dentry
 		goto afterTrailingSymlink
 	}
-	// Open existing file.
-	if mustCreate {
-		return nil, syserror.EEXIST
+	if rp.MustBeDir() && !child.inode.isDir() {
+		return nil, syserror.ENOTDIR
 	}
 	return child.open(ctx, rp, &opts, false)
 }
diff --git a/test/syscalls/linux/open.cc b/test/syscalls/linux/open.cc
index 8f0c9cb49..77f390f3c 100644
--- a/test/syscalls/linux/open.cc
+++ b/test/syscalls/linux/open.cc
@@ -27,6 +27,7 @@
 #include "test/util/cleanup.h"
 #include "test/util/file_descriptor.h"
 #include "test/util/fs_util.h"
+#include "test/util/posix_error.h"
 #include "test/util/temp_path.h"
 #include "test/util/test_util.h"
 #include "test/util/thread_util.h"
@@ -408,6 +409,13 @@ TEST_F(OpenTest, FileNotDirectory) {
               SyscallFailsWithErrno(ENOTDIR));
 }
 
+TEST_F(OpenTest, SymlinkDirectory) {
+  auto dir = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
+  std::string link = NewTempAbsPath();
+  ASSERT_THAT(symlink(dir.path().c_str(), link.c_str()), SyscallSucceeds());
+  ASSERT_NO_ERRNO(Open(link, O_RDONLY | O_DIRECTORY));
+}
+
 TEST_F(OpenTest, Null) {
   char c = '\0';
   ASSERT_THAT(open(&c, O_RDONLY), SyscallFailsWithErrno(ENOENT));