Refactor descriptor flags and security.

libc/include/fcntl.h

@@ -34,6 +34,14 @@
 
 __BEGIN_DECLS
 
+/* The kernel would like to simply deal with one bit for each base access mode,
+   but using the traditional names O_RDONLY, O_WRONLY and O_RDWR for this would
+   be weird, so it uses O_READ and O_WRITE bits instead. However, we provide the
+   traditional names here instead to remain compatible. */
+#define O_RDONLY O_READ
+#define O_WRONLY O_WRITE
+#define O_RDWR (O_READ | O_WRITE)
+
 /* TODO: F_* missing here */
 
 /* TODO: F_RDLCK, F_UNLCK, F_WRLCK missing here */

sortix/descriptor.cpp

@@ -43,6 +43,15 @@
 
 namespace Sortix {
 
+// Flags for the various base modes to open a file in.
+const int ACCESS_FLAGS = O_READ | O_WRITE | O_EXEC | O_SEARCH;
+
+// Flags that only make sense at open time.
+const int OPEN_FLAGS = O_CREAT | O_DIRECTORY | O_EXCL | O_TRUNC;
+
+// Flags that only make sense for descriptors.
+const int DESCRIPTOR_FLAGS = O_APPEND;
+
 bool LinkInodeInDir(ioctx_t* ctx, Ref<Descriptor> dir, const char* name,
                     Ref<Inode> inode)
 {
@@ -76,7 +85,7 @@ Ref<Descriptor> OpenDirContainingPath(ioctx_t* ctx, Ref<Descriptor> from,
 		*finalp = final;
 		return from;
 	}
-	Ref<Descriptor> ret = from->open(ctx, dirpath, O_RDONLY | O_DIRECTORY, 0);
+	Ref<Descriptor> ret = from->open(ctx, dirpath, O_READ | O_DIRECTORY, 0);
 	delete[] dirpath;
 	if ( !ret ) { delete[] final; return Ref<Descriptor>(); }
 	*finalp = final;
@@ -126,11 +135,16 @@ bool Descriptor::IsSeekable()
 
 int Descriptor::sync(ioctx_t* ctx)
 {
+	// TODO: Possible denial-of-service attack if someone opens the file without
+	//       that much rights and just syncs it a whole lot and slows down the
+	//       system as a whole.
 	return vnode->sync(ctx);
 }
 
 int Descriptor::stat(ioctx_t* ctx, struct stat* st)
 {
+	// TODO: Possible information leak if not O_READ | O_WRITE and the caller
+	//       is told about the file size.
 	return vnode->stat(ctx, st);
 }
 
@@ -148,11 +162,15 @@ int Descriptor::chown(ioctx_t* ctx, uid_t owner, gid_t group)
 int Descriptor::truncate(ioctx_t* ctx, off_t length)
 {
 	if ( length < 0 ) { errno = EINVAL; return -1; }
+	if ( !(dflags & O_WRITE) )
+		return errno = EPERM, -1;
 	return vnode->truncate(ctx, length);
 }
 
 off_t Descriptor::lseek(ioctx_t* ctx, off_t offset, int whence)
 {
+	// TODO: Possible information leak to let someone without O_READ | O_WRITE
+	//       seek the file and get information about data holes.
 	if ( !IsSeekable() )
 		return vnode->lseek(ctx, offset, whence);
 	ScopedLock lock(&curofflock);
@@ -179,6 +197,8 @@ off_t Descriptor::lseek(ioctx_t* ctx, off_t offset, int whence)
 
 ssize_t Descriptor::read(ioctx_t* ctx, uint8_t* buf, size_t count)
 {
+	if ( !(dflags & O_READ) )
+		return errno = EPERM, -1;
 	if ( !count ) { return 0; }
 	if ( (size_t) SSIZE_MAX < count ) { count = SSIZE_MAX; }
 	if ( !IsSeekable() )
@@ -193,6 +213,8 @@ ssize_t Descriptor::read(ioctx_t* ctx, uint8_t* buf, size_t count)
 
 ssize_t Descriptor::pread(ioctx_t* ctx, uint8_t* buf, size_t count, off_t off)
 {
+	if ( !(dflags & O_READ) )
+		return errno = EPERM, -1;
 	if ( off < 0 ) { errno = EINVAL; return -1; }
 	if ( !count ) { return 0; }
 	if ( SSIZE_MAX < count ) { count = SSIZE_MAX; }
@@ -201,6 +223,8 @@ ssize_t Descriptor::pread(ioctx_t* ctx, uint8_t* buf, size_t count, off_t off)
 
 ssize_t Descriptor::write(ioctx_t* ctx, const uint8_t* buf, size_t count)
 {
+	if ( !(dflags & O_WRITE) )
+		return errno = EPERM, -1;
 	if ( !count ) { return 0; }
 	if ( SSIZE_MAX < count ) { count = SSIZE_MAX; }
 	if ( !IsSeekable() )
@@ -219,6 +243,8 @@ ssize_t Descriptor::write(ioctx_t* ctx, const uint8_t* buf, size_t count)
 
 ssize_t Descriptor::pwrite(ioctx_t* ctx, const uint8_t* buf, size_t count, off_t off)
 {
+	if ( !(dflags & O_WRITE) )
+		return errno = EPERM, -1;
 	if ( off < 0 ) { errno = EINVAL; return -1; }
 	if ( !count ) { return 0; }
 	if ( SSIZE_MAX < count ) { count = SSIZE_MAX; }
@@ -238,6 +264,22 @@ int Descriptor::isatty(ioctx_t* ctx)
 ssize_t Descriptor::readdirents(ioctx_t* ctx, struct kernel_dirent* dirent,
                                 size_t size, size_t maxcount)
 {
+	// TODO: COMPATIBILITY HACK: Traditionally, you can open a directory with
+	//       O_RDONLY and pass it to fdopendir and then use it, which doesn't
+	//       set the needed O_SEARCH flag! I think some software even do it with
+	//       write permissions! Currently, we just let you search the directory
+	//       if you opened with any of the O_SEARCH, O_READ or O_WRITE flags.
+	//       A better solution would be to make fdopendir try to add the
+	//       O_SEARCH flag to the file descriptor. Or perhaps just recheck the
+	//       permissions to search (execute) the directory manually every time,
+	//       though that is less pure. Unfortunately, POSIX is pretty vague on
+	//       how O_SEARCH should be interpreted and most existing Unix systems
+	//       such as Linux doesn't even have that flag! And how about combining
+	//       it with the O_EXEC flag - POSIX allows that and it makes sense
+	//       because the execute bit on directories control search permission.
+	if ( !(dflags & (O_SEARCH | O_READ | O_WRITE)) )
+		return errno = EPERM, -1;
+
 	if ( !maxcount ) { return 0; }
 	if ( SSIZE_MAX < size ) { size = SSIZE_MAX; }
 	if ( size < sizeof(*dirent) ) { errno = EINVAL; return -1; }
@@ -268,20 +310,44 @@ ssize_t Descriptor::readdirents(ioctx_t* ctx, struct kernel_dirent* dirent,
 	return ret;
 }
 
+static bool IsSaneFlagModeCombination(int flags, mode_t /*mode*/)
+{
+	// It doesn't make sense to pass O_CREAT or O_TRUNC when attempting to open
+	// a directory. We also reject O_TRUNC | O_DIRECTORY early to prevent
+	// opening a directory, attempting to truncate it, and then aborting with an
+	// error because a directory was opened.
+	if ( (flags & (O_CREAT | O_TRUNC)) && (flags & (O_DIRECTORY)) )
+		return errno = EINVAL, false;
+
+	return true;
+}
+
+static bool IsLastPathElement(const char* elem)
+{
+	while ( !(*elem == '/' || *elem == '\0') )
+		elem++;
+	while ( *elem == '/' )
+		elem++;
+	return *elem == '\0';
+}
+
 Ref<Descriptor> Descriptor::open(ioctx_t* ctx, const char* filename, int flags,
                                  mode_t mode)
 {
+	// Unlike early Unix systems, the empty path does not mean the current
+	// directory on Sortix, so reject it.
 	if ( !filename[0] )
 		return errno = ENOENT, Ref<Descriptor>();
-	// O_DIRECTORY makes us only open directories. It is therefore a logical
-	// error to provide flags that only makes sense for non-directory. We also
-	// filter reject them early to prevent O_TRUNC | O_DIRECTORY from opening a
-	// file, truncating it, and then aborting the open with an error.
-	if ( (flags & (O_CREAT | O_TRUNC)) && (flags & (O_DIRECTORY)) )
+
+	// Reject some non-sensical flag combinations early.
+	if ( !IsSaneFlagModeCombination(flags, mode) )
 		return errno = EINVAL, Ref<Descriptor>();
+
 	Ref<Descriptor> desc(this);
 	while ( filename[0] )
 	{
+		// Reaching a slash in the path means that the caller intended what came
+		// before to be a directory, stop the open call if it isn't.
 		if ( filename[0] == '/' )
 		{
 			if ( !S_ISDIR(desc->type) )
@@ -289,25 +355,30 @@ Ref<Descriptor> Descriptor::open(ioctx_t* ctx, const char* filename, int flags,
 			filename++;
 			continue;
 		}
+
+		// Cut out the next path element from the input string.
 		size_t slashpos = strcspn(filename, "/");
-		bool lastelem = filename[slashpos] == '\0';
 		char* elem = String::Substring(filename, 0, slashpos);
 		if ( !elem )
 			return Ref<Descriptor>();
-		int open_flags = lastelem ? flags : O_RDONLY;
-		// Forward O_DIRECTORY so the operation can fail earlier. If it doesn't
-		// fail and we get a non-directory in return, we'll handle it on exit
-		// with no consequences (since opening an existing file is harmless).
-		open_flags &= ~(0 /*| O_DIRECTORY*/);
+
+		// Decide how to open the next element in the path.
+		bool lastelem = IsLastPathElement(filename);
+		int open_flags = lastelem ? flags : O_READ | O_DIRECTORY;
 		mode_t open_mode = lastelem ? mode : 0;
-		// TODO: O_NOFOLLOW.
+
+		// Open the next element in the path.
 		Ref<Descriptor> next = desc->open_elem(ctx, elem, open_flags, open_mode);
 		delete[] elem;
+
 		if ( !next )
 			return Ref<Descriptor>();
+
 		desc = next;
 		filename += slashpos;
 	}
+
+	// Abort the open if the user wanted a directory but this wasn't.
 	if ( flags & O_DIRECTORY && !S_ISDIR(desc->type) )
 		return errno = ENOTDIR, Ref<Descriptor>();
 
@@ -322,17 +393,30 @@ Ref<Descriptor> Descriptor::open_elem(ioctx_t* ctx, const char* filename,
                                       int flags, mode_t mode)
 {
 	assert(!strchr(filename, '/'));
-	int next_flags = flags & ~(O_APPEND | O_CLOEXEC);
-	Ref<Vnode> retvnode = vnode->open(ctx, filename, next_flags, mode);
+
+	// Verify that at least one of the base access modes are being used.
+	if ( !(flags & ACCESS_FLAGS) )
+		return errno = EINVAL, Ref<Descriptor>();
+
+	// Filter away flags that only make sense for descriptors.
+	int retvnode_flags = flags & ~DESCRIPTOR_FLAGS;
+	Ref<Vnode> retvnode = vnode->open(ctx, filename, retvnode_flags, mode);
 	if ( !retvnode )
 		return Ref<Descriptor>();
-	Ref<Descriptor> ret(new Descriptor(retvnode, flags & O_APPEND));
+
+	// Filter away flags that only made sense at during the open call.
+	int ret_flags = flags & ~OPEN_FLAGS;
+	Ref<Descriptor> ret(new Descriptor(retvnode, ret_flags));
 	if ( !ret )
 		return Ref<Descriptor>();
+
+	// Truncate the file if requested.
+	// TODO: This is a bit dodgy, should this be moved to the inode open method
+	//       or something? And how should error handling be done here?
 	if ( (flags & O_TRUNC) && S_ISREG(ret->type) )
-		if ( (flags & O_ACCMODE) == O_WRONLY ||
-		     (flags & O_ACCMODE) == O_RDWR )
+		if ( flags & O_WRITE )
 			ret->truncate(ctx, 0);
+
 	return ret;
 }
 
@@ -420,6 +504,8 @@ int Descriptor::rename_here(ioctx_t* ctx, Ref<Descriptor> from,
 
 ssize_t Descriptor::readlink(ioctx_t* ctx, char* buf, size_t bufsize)
 {
+	if ( !(dflags & O_READ) )
+		return errno = EPERM, -1;
 	if ( (size_t) SSIZE_MAX < bufsize )
 		bufsize = (size_t) SSIZE_MAX;
 	return vnode->readlink(ctx, buf, bufsize);
@@ -442,6 +528,8 @@ int Descriptor::gettermmode(ioctx_t* ctx, unsigned* mode)
 
 int Descriptor::poll(ioctx_t* ctx, PollNode* node)
 {
+	// TODO: Perhaps deny polling against some kind of events if this
+	//       descriptor's dflags would reject doing these operations?
 	return vnode->poll(ctx, node);
 }
 

sortix/fs/user.cpp

@@ -1207,7 +1207,7 @@ void Init(const char* devpath, Ref<Descriptor> slashdev)
 	// created, possibly with a O_MKDIR flag to open.
 	if ( slashdev->mkdir(&ctx, "fs", 0755) < 0 && errno != EEXIST )
 		PanicF("Could not create a %s/fs directory", devpath);
-	Ref<Descriptor> mpoint = slashdev->open(&ctx, "fs", O_RDWR, 0);
+	Ref<Descriptor> mpoint = slashdev->open(&ctx, "fs", O_READ | O_WRITE, 0);
 	if ( !mpoint )
 		PanicF("Could not open the %s/fs directory", devpath);
 	Ref<MountTable> mtable = CurrentProcess()->GetMTable();

sortix/include/sortix/fcntl.h

@@ -29,13 +29,11 @@
 
 __BEGIN_DECLS
 
-#define O_RDONLY 1
-#define O_WRONLY 2
-#define O_RDWR 3
-#define O_ACCMODE (O_RDONLY | O_WRONLY | O_RDWR)
-#define O_EXEC 4
-#define O_SEARCH 5
-#define O_LOWERFLAGS 0x7
+// Remember to update the flag classifications at the top of descriptor.cpp if
+// you add new flags here.
+#define O_READ (1<<0)
+#define O_WRITE (1<<1)
+#define O_EXEC (1<<2)
 #define O_APPEND (1<<3)
 #define O_CLOEXEC (1<<4)
 #define O_CREAT (1<<5)
@@ -43,6 +41,8 @@ __BEGIN_DECLS
 #define O_EXCL (1<<7)
 #define O_TRUNC (1<<8)
 #define O_CLOFORK (1<<9)
+#define O_SEARCH (1<<10)
+// TODO: O_NOFOLLOW.
 
 #define FD_CLOEXEC (1<<0)
 #define FD_CLOFORK (1<<1)

sortix/initrd.cpp

@@ -276,7 +276,7 @@ static bool ExtractDir(ioctx_t* ctx, uint32_t ino, Ref<Descriptor> dir)
 		{
 			if ( dir->mkdir(ctx, name, mode) && errno != EEXIST )
 				return false;
-			Ref<Descriptor> desc = dir->open(ctx, name, O_RDWR | O_DIRECTORY, 0);
+			Ref<Descriptor> desc = dir->open(ctx, name, O_SEARCH | O_DIRECTORY, 0);
 			if ( !desc )
 				return false;
 			if ( !ExtractNode(ctx, childino, desc) )
@@ -284,7 +284,7 @@ static bool ExtractDir(ioctx_t* ctx, uint32_t ino, Ref<Descriptor> dir)
 		}
 		if ( INITRD_S_ISREG(child->mode) )
 		{
-			Ref<Descriptor> desc = dir->open(ctx, name, O_WRONLY | O_CREAT, mode);
+			Ref<Descriptor> desc = dir->open(ctx, name, O_WRITE | O_CREAT, mode);
 			if ( !desc )
 				return false;
 			if ( !ExtractNode(ctx, childino, desc) )

sortix/io.cpp

@@ -172,7 +172,7 @@ static int sys_faccessat(int dirfd, const char* path, int /*mode*/, int flags)
 	const char* relpath = pathcopy;
 	Ref<Descriptor> from = PrepareLookup(&relpath, dirfd);
 	if ( !from ) { delete[] pathcopy; return -1; }
-	Ref<Descriptor> desc = from->open(&ctx, relpath, O_RDONLY);
+	Ref<Descriptor> desc = from->open(&ctx, relpath, O_READ);
 	delete[] pathcopy;
 	return desc ? 0 : -1;
 }
@@ -244,7 +244,7 @@ static int sys_truncateat(int dirfd, const char* path, off_t length)
 	const char* relpath = pathcopy;
 	Ref<Descriptor> from = PrepareLookup(&relpath, dirfd);
 	if ( !from ) { delete[] pathcopy; return -1; }
-	Ref<Descriptor> desc = from->open(&ctx, relpath, O_WRONLY);
+	Ref<Descriptor> desc = from->open(&ctx, relpath, O_WRITE);
 	delete[] pathcopy;
 	if ( !desc )
 		return -1;
@@ -275,7 +275,7 @@ static int sys_fstatat(int dirfd, const char* path, struct stat* st, int /*flags
 	const char* relpath = pathcopy;
 	Ref<Descriptor> from = PrepareLookup(&relpath, dirfd);
 	if ( !from ) { delete[] pathcopy; return -1; }
-	Ref<Descriptor> desc = from->open(&ctx, relpath, O_RDONLY);
+	Ref<Descriptor> desc = from->open(&ctx, relpath, O_READ);
 	delete[] pathcopy;
 	if ( !desc )
 		return -1;
@@ -353,7 +353,7 @@ static int sys_chdir(const char* path)
 	ioctx_t ctx; SetupUserIOCtx(&ctx);
 	const char* relpath = pathcopy;
 	Ref<Descriptor> from = PrepareLookup(&relpath);
-	Ref<Descriptor> desc = from->open(&ctx, relpath, O_RDONLY | O_DIRECTORY);
+	Ref<Descriptor> desc = from->open(&ctx, relpath, O_READ | O_DIRECTORY);
 	from.Reset();
 	delete[] pathcopy;
 	if ( !desc )
@@ -382,7 +382,7 @@ static int sys_fchownat(int dirfd, const char* path, uid_t owner, gid_t group, i
 	const char* relpath = pathcopy;
 	Ref<Descriptor> from = PrepareLookup(&relpath, dirfd);
 	if ( !from ) { delete[] pathcopy; return -1; }
-	Ref<Descriptor> desc = from->open(&ctx, relpath, O_WRONLY);
+	Ref<Descriptor> desc = from->open(&ctx, relpath, O_WRITE);
 	from.Reset();
 	delete[] pathcopy;
 	if ( !desc )
@@ -416,7 +416,7 @@ static int sys_fchmodat(int dirfd, const char* path, mode_t mode, int flags)
 	const char* relpath = pathcopy;
 	Ref<Descriptor> from = PrepareLookup(&relpath, dirfd);
 	if ( !from ) { delete[] pathcopy; return -1; }
-	Ref<Descriptor> desc = from->open(&ctx, relpath, O_WRONLY);
+	Ref<Descriptor> desc = from->open(&ctx, relpath, O_WRITE);
 	from.Reset();
 	delete[] pathcopy;
 	if ( !desc )
@@ -459,7 +459,7 @@ static int sys_linkat(int olddirfd, const char* oldpath,
 	Ref<Descriptor> oldfrom = PrepareLookup(&oldrelpath, olddirfd);
 	if ( !oldfrom ) { delete[] oldpathcopy; delete[] final_elem; return -1; }
 
-	Ref<Descriptor> file = oldfrom->open(&ctx, oldrelpath, O_RDONLY);
+	Ref<Descriptor> file = oldfrom->open(&ctx, oldrelpath, O_READ);
 	delete[] oldpathcopy;
 	if ( !file ) { delete[] final_elem; return -1; }
 
@@ -552,7 +552,7 @@ static ssize_t sys_readlinkat(int dirfd, const char* path, char* buf, size_t siz
 	Ref<Descriptor> from = PrepareLookup(&relpath, dirfd);
 	if ( !from ) { delete[] pathcopy; return -1; }
 	// TODO: Open the symbolic link, instead of what it points to!
-	Ref<Descriptor> desc = from->open(&ctx, relpath, O_RDONLY);
+	Ref<Descriptor> desc = from->open(&ctx, relpath, O_READ);
 	delete[] pathcopy;
 	if ( !desc )
 		return -1;

sortix/kernel.cpp

@@ -355,7 +355,7 @@ static void BootThread(void* /*user*/)
 	Ref<Vnode> vroot(new Vnode(iroot, Ref<Vnode>(NULL), 0, 0));
 	if ( !vroot )
 		Panic("Unable to allocate root vnode.");
-	Ref<Descriptor> droot(new Descriptor(vroot, 0));
+	Ref<Descriptor> droot(new Descriptor(vroot, O_SEARCH));
 	if ( !droot )
 		Panic("Unable to allocate root descriptor.");
 	CurrentProcess()->BootstrapDirectories(droot);
@@ -376,7 +376,7 @@ static void BootThread(void* /*user*/)
 	// Get a descriptor for the /dev directory so we can populate it.
 	if ( droot->mkdir(&ctx, "dev", 0775) != 0 && errno != EEXIST )
 		Panic("Unable to create RAM filesystem /dev directory.");
-	Ref<Descriptor> slashdev = droot->open(&ctx, "dev", O_RDONLY | O_DIRECTORY);
+	Ref<Descriptor> slashdev = droot->open(&ctx, "dev", O_READ | O_DIRECTORY);
 	if ( !slashdev )
 		Panic("Unable to create descriptor for RAM filesystem /dev directory.");
 
@@ -499,7 +499,7 @@ static void InitThread(void* /*user*/)
 
 	ioctx_t ctx; SetupKernelIOCtx(&ctx);
 	Ref<Descriptor> root = CurrentProcess()->GetRoot();
-	Ref<Descriptor> init = root->open(&ctx, initpath, O_EXEC);
+	Ref<Descriptor> init = root->open(&ctx, initpath, O_EXEC | O_READ);
 	if ( !init )
 		PanicF("Could not open %s in early kernel RAM filesystem:\n%s",
 		       initpath, strerror(errno));

sortix/pipe.cpp

@@ -26,6 +26,7 @@
 #include <errno.h>
 #include <string.h>
 
+#include <sortix/fcntl.h>
 #include <sortix/signal.h>
 #include <sortix/stat.h>
 #include <sortix/poll.h>
@@ -314,8 +315,8 @@ static int sys_pipe(int pipefd[2])
 	Ref<Vnode> send_vnode(new Vnode(send_inode, Ref<Vnode>(NULL), 0, 0));
 	if ( !recv_vnode || !send_vnode ) return -1;
 
-	Ref<Descriptor> recv_desc(new Descriptor(recv_vnode, 0));
-	Ref<Descriptor> send_desc(new Descriptor(send_vnode, 0));
+	Ref<Descriptor> recv_desc(new Descriptor(recv_vnode, O_READ));
+	Ref<Descriptor> send_desc(new Descriptor(send_vnode, O_WRITE));
 	if ( !recv_desc || !send_desc ) return -1;
 
 	Ref<DescriptorTable> dtable = process->GetDTable();

sortix/process.cpp

@@ -725,7 +725,7 @@ namespace Sortix
 		SetupKernelIOCtx(&ctx);
 
 		// TODO: Somehow mark the executable as busy and don't permit writes?
-		desc = process->Open(&ctx, filename, O_RDONLY, 0);
+		desc = process->Open(&ctx, filename, O_READ | O_WRITE, 0);
 		if ( !desc ) { goto cleanup_envp; }
 
 		if ( desc->stat(&ctx, &st) ) { goto cleanup_desc; }