lx_fs: improve safety when using dir handles

This patch consolidates the repetitive error handling across the RPC
functions, which take node handles or directory handles as arguments.

During this change, I noticed that directory handles - which are values
provided by the client - were not checked for their type before being
used. A misbehaving client may open a file, manually construct a
directory handle using the number of the file handle, and invoke a
directory operation at lx_fs, which would then wrongly access a file
node as directory node.

This patch solves this issue by introducing two distinct methods
_with_open_node and _with_open_dir_node, which perform the respective
safety checks.

Fixes #4608

repos/os/src/server/lx_fs/directory.h

@@ -112,6 +112,8 @@ class Lx_fs::Directory : public Node
 			closedir(_fd);
 		}
 
+		bool type_directory() const override { return true; }
+
 		void update_modification_time(Timestamp const time) override
 		{
 			struct timespec ts[2] = {

repos/os/src/server/lx_fs/main.cc

@@ -91,6 +91,7 @@ class Lx_fs::Session_component : private Session_resources,
 	private:
 
 		using Open_node      = File_system::Open_node<Node>;
+		using Dir_node       = File_system::Open_node<Directory>;
 		using Signal_handler = Genode::Signal_handler<Session_component>;
 
 		Genode::Env                 &_env;
@@ -238,6 +239,43 @@ class Lx_fs::Session_component : private Session_resources,
 			}
 		}
 
+		/**
+		 * Apply 'fn' to the open node referenced by 'node_handle'
+		 *
+		 * \throw Invalid_handle
+		 */
+		template <typename FN>
+		auto _with_open_node(Node_handle &node_handle, FN const &fn)
+		-> typename Trait::Functor<decltype(&FN::operator())>::Return_type
+		{
+			using Node = File_system::Node;
+			try {
+				return _open_node_registry.apply<Node>(node_handle, [&] (Node &node) {
+					return fn(static_cast<Open_node &>(node)); });
+			}
+			catch (Id_space<Node>::Unknown_id const &) {
+				throw Invalid_handle(); }
+		}
+
+		template <typename FN>
+		auto _with_open_dir_node(Dir_handle &dir_handle, FN const &fn)
+		-> typename Trait::Functor<decltype(&FN::operator())>::Return_type
+		{
+			using Node = File_system::Node;
+			try {
+				return _open_node_registry.apply<Node>(dir_handle, [&] (Node &node) {
+
+					Open_node &open_node = static_cast<Open_node &>(node);
+
+					if (!open_node.node().type_directory())
+						throw Invalid_handle();
+
+					return fn(static_cast<Dir_node &>(node)); });
+			}
+			catch (Id_space<Node>::Unknown_id const &) {
+				throw Invalid_handle(); }
+		}
+
 		/**
 		 * Watch_node::Response_handler interface
 		 */
@@ -315,6 +353,7 @@ class Lx_fs::Session_component : private Session_resources,
 		void upgrade(Genode::Ram_quota ram)  { _ram_guard.upgrade(ram); }
 		void upgrade(Genode::Cap_quota caps) { _cap_guard.upgrade(caps); }
 
+
 		/***************************
 		 ** File_system interface **
 		 ***************************/
@@ -324,9 +363,9 @@ class Lx_fs::Session_component : private Session_resources,
 			if (!valid_name(name.string()))
 				throw Invalid_name();
 
-			auto file_fn = [&] (Open_node &open_node) {
+			return _with_open_dir_node(dir_handle, [&] (Dir_node &dir_node) {
 
-				Node &dir = open_node.node();
+				Node &dir = dir_node.node();
 
 				if (!_writeable)
 					if (create || (mode != STAT_ONLY && mode != READ_ONLY))
@@ -337,16 +376,8 @@ class Lx_fs::Session_component : private Session_resources,
 				Open_node *open_file =
 					new (_alloc) Open_node(*file, _open_node_registry);
 
-				return open_file->id();
-			};
-
-			try {
-				return File_handle {
-					_open_node_registry.apply<Open_node>(dir_handle, file_fn).value
-				};
-			} catch (Id_space<File_system::Node>::Unknown_id const &) {
-				throw Invalid_handle();
-			}
+				return File_handle { open_file->id().value };
+			});
 		}
 
 		Symlink_handle symlink(Dir_handle, Name const &, bool /* create */) override
@@ -415,43 +446,23 @@ class Lx_fs::Session_component : private Session_resources,
 
 		void close(Node_handle handle) override
 		{
-			auto close_fn = [&] (Open_node &open_node) {
+			_with_open_node(handle, [&] (Open_node &open_node) {
 				Node &node = open_node.node();
 				destroy(_alloc, &open_node);
 				destroy(_alloc, &node);
-			};
-
-			try {
-				_open_node_registry.apply<Open_node>(handle, close_fn);
-			} catch (Id_space<File_system::Node>::Unknown_id const &) {
-				throw Invalid_handle();
-			}
+			});
 		}
 
-		Status status(Node_handle node_handle) override
+		Status status(Node_handle handle) override
 		{
-			auto status_fn = [&] (Open_node &open_node) {
-				return open_node.node().status();
-			};
-
-			try {
-				return _open_node_registry.apply<Open_node>(node_handle, status_fn);
-			} catch (Id_space<File_system::Node>::Unknown_id const &) {
-				throw Invalid_handle();
-			}
+			return _with_open_node(handle, [&] (Open_node &open_node) {
+				return open_node.node().status(); });
 		}
 
 		unsigned num_entries(Dir_handle dir_handle) override
 		{
-			auto fn = [&] (Open_node &open_node) {
-				return open_node.node().num_entries();
-			};
-
-			try {
-				return _open_node_registry.apply<Open_node>(dir_handle, fn);
-			} catch (Id_space<File_system::Node>::Unknown_id const &) {
-				throw Invalid_handle();
-			}
+			return _with_open_dir_node(dir_handle, [&] (Dir_node &dir_node) {
+				return dir_node.node().num_entries(); });
 		}
 
 		void control(Node_handle, Control) override
@@ -467,7 +478,7 @@ class Lx_fs::Session_component : private Session_resources,
 			if (!_writeable)
 				throw Permission_denied();
 
-			auto unlink_fn = [&] (Open_node &open_node) {
+			_with_open_node(dir_handle, [&] (Open_node &open_node) {
 
 				Absolute_path absolute_path("/");
 
@@ -502,13 +513,7 @@ class Lx_fs::Session_component : private Session_resources,
 					if (err == EACCES)
 						throw Permission_denied();
 				}
-			};
-
-			try {
-				_open_node_registry.apply<Open_node>(dir_handle, unlink_fn);
-			} catch (Id_space<File_system::Node>::Unknown_id const &) {
-				throw Invalid_handle();
-			}
+			});
 		}
 
 		void truncate(File_handle file_handle, file_size_t size) override
@@ -516,43 +521,20 @@ class Lx_fs::Session_component : private Session_resources,
 			if (!_writeable)
 				throw Permission_denied();
 
-			auto truncate_fn = [&] (Open_node &open_node) {
-				open_node.node().truncate(size);
-			};
-
-			try {
-				_open_node_registry.apply<Open_node>(file_handle, truncate_fn);
-			} catch (Id_space<File_system::Node>::Unknown_id const &) {
-				throw Invalid_handle();
-			}
+			_with_open_node(file_handle, [&] (Open_node &open_node) {
+				open_node.node().truncate(size); });
 		}
 
 		void move(Dir_handle dir_from, Name const & name_from,
 		          Dir_handle dir_to,   Name const & name_to) override
 		{
-			typedef File_system::Open_node<Directory> Dir_node;
-
 			Directory *to = 0;
 
-			auto to_fn = [&] (Dir_node &dir_node) {
-				to = &dir_node.node();
-			};
+			_with_open_dir_node(dir_to, [&] (Dir_node &dir_node) {
+				to = &dir_node.node(); });
 
-			try {
-				_open_node_registry.apply<Dir_node>(dir_to, to_fn);
-			} catch (Id_space<File_system::Node>::Unknown_id const &) {
-				throw Invalid_handle();
-			}
-
-			auto move_fn = [&] (Dir_node &dir_node) {
-				dir_node.node().rename(*to, name_from.string(), name_to.string());
-			};
-
-			try {
-				_open_node_registry.apply<Dir_node>(dir_from, move_fn);
-			} catch (Id_space<File_system::Node>::Unknown_id const &) {
-				throw Invalid_handle();
-			}
+			_with_open_dir_node(dir_from, [&] (Dir_node &dir_node) {
+				dir_node.node().rename(*to, name_from.string(), name_to.string()); });
 		}
 };
 

repos/os/src/server/lx_fs/node.h

@@ -60,6 +60,8 @@ class Lx_fs::Node : public File_system::Node_base
 		uint64_t inode()   const { return _inode; }
 		char const *name() const { return _name; }
 
+		virtual bool type_directory() const { return false; }
+
 		/**
 		 * Assign name
 		 */

repos/os/src/server/lx_fs/open_node.h

@@ -32,7 +32,8 @@ class File_system::Open_node : public File_system::Node
 
 		Genode::Id_space<File_system::Node>::Element _element;
 
-		NODE                                         &_node;
+		NODE &_node;
+
 		Genode::Constructible<File_system::Listener>  _listener { };
 
 		Listener::Version const _version_when_opened = _node.curr_version();