[sestarnix] Initial file_permission() hook and integration

Implement the LSM file_permission(), hook, which validates that
the current task still has the specified set of
read/write/execute permissions to an open FileObject.

Bug: 364569337, 385121365
Change-Id: Idae66deada0a59b5023236264eb9aa08d63d958f
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/1111716
Reviewed-by: Nelly Vouzoukidou <[email protected]>
Fuchsia-Auto-Submit: Wez <[email protected]>
Commit-Queue: Auto-Submit <[email protected]>

Author: drwez

src/starnix/kernel/security/hooks.rs

@@ -178,6 +178,18 @@ pub struct FsNodeSecurityXattr {
     pub value: FsString,
 }
 
+/// Checks whether the `current_task` has the specified `permission_flags` to the `file`.
+/// Corresponds to the `file_permission()` LSM hook.
+pub fn file_permission(
+    current_task: &CurrentTask,
+    file: &FileObject,
+    permission_flags: PermissionFlags,
+) -> Result<(), Errno> {
+    if_selinux_else_default_ok(current_task, |security_server| {
+        selinux_hooks::file_permission(security_server, current_task, file, permission_flags)
+    })
+}
+
 /// Called by the VFS to initialize the security state for an `FsNode` that is being linked at
 /// `dir_entry`.
 /// If the `FsNode` security state had already been initialized, or no policy is yet loaded, then

src/starnix/kernel/security/selinux_hooks/mod.rs

@@ -14,6 +14,7 @@ use crate::vfs::{
     DirEntry, DirEntryHandle, FileHandle, FileObject, FileSystem, FileSystemHandle, FsNode, FsStr,
     FsString, PathBuilder, UnlinkKind, ValueOrSize, XattrOp,
 };
+use crate::TODO_DENY;
 use audit::{audit_log, AuditContext};
 use bstr::BStr;
 use linux_uapi::XATTR_NAME_SELINUX;
@@ -31,6 +32,7 @@ use starnix_uapi::arc_key::WeakKey;
 use starnix_uapi::device_type::DeviceType;
 use starnix_uapi::errors::{Errno, ENODATA};
 use starnix_uapi::file_mode::FileMode;
+use starnix_uapi::open_flags::OpenFlags;
 use starnix_uapi::{
     errno, error, FIGETBSZ, FIOASYNC, FIONBIO, FIONREAD, FS_IOC_GETFLAGS, FS_IOC_GETVERSION,
     FS_IOC_SETFLAGS, FS_IOC_SETVERSION,
@@ -42,6 +44,29 @@ use std::sync::{Arc, OnceLock};
 /// contexts in a filesystem node extended attributes.
 const SECURITY_SELINUX_XATTR_VALUE_MAX_SIZE: usize = 4096;
 
+/// Returns the set of `Permissions` on `class`, corresponding to the specified `flags`.
+fn permissions_from_flags(flags: PermissionFlags, class: FileClass) -> Vec<Permission> {
+    let mut result = Vec::new();
+    if flags.contains(PermissionFlags::READ) {
+        result.push(CommonFilePermission::Read.for_class(class));
+    }
+    // SELinux uses the `APPEND` bit to distinguish which of the "append" or the more general
+    // "write" permission to check for.
+    if flags.contains(PermissionFlags::APPEND) {
+        result.push(CommonFilePermission::Append.for_class(class));
+    } else if flags.contains(PermissionFlags::WRITE) {
+        result.push(CommonFilePermission::Write.for_class(class));
+    }
+    if flags.contains(PermissionFlags::EXEC) {
+        if class == FileClass::Dir {
+            result.push(DirPermission::Search.into());
+        } else {
+            result.push(CommonFilePermission::Execute.for_class(class));
+        }
+    }
+    result
+}
+
 /// Checks that `current_task` has permission to "use" the specified `file`, and the specified
 /// `permissions` to the underlying [`crate::vfs::FsNode`].
 fn has_file_permissions(
@@ -53,7 +78,8 @@ fn has_file_permissions(
     // Validate that the `subject` has the "fd { use }" permission to the `file`.
     // If the file and task security domains are identical then `fd { use }` is implicitly granted.
     let file_sid = file.security_state.state.sid;
-    if subject_sid != file_sid {
+    // TODO: https://fxbug.dev/385121365 - Should the kernel be implicitly allowed to "use" all FDs?
+    if subject_sid != SecurityId::initial(InitialSid::Kernel) && subject_sid != file_sid {
         check_permission(permission_check, subject_sid, file_sid, FdPermission::Use)?;
     }
 
@@ -108,6 +134,31 @@ fn todo_has_fs_node_permissions(
     Ok(())
 }
 
+/// Checks whether the `current_task`` has the permissions specified by `mask` to the `file`.
+pub fn file_permission(
+    security_server: &SecurityServer,
+    current_task: &CurrentTask,
+    file: &FileObject,
+    mut permission_flags: PermissionFlags,
+) -> Result<(), Errno> {
+    let current_sid = current_task.security_state.lock().current_sid;
+    let file_mode = file.name.entry.node.info().mode;
+    let file_class = file_class_from_file_mode(file_mode)?;
+
+    if file.flags().contains(OpenFlags::APPEND) {
+        permission_flags |= PermissionFlags::APPEND;
+    }
+
+    has_file_permissions(&security_server.as_permission_check(), current_sid, file, &[])?;
+    todo_has_fs_node_permissions(
+        TODO_DENY!("https://fxbug.dev/385121365", "Enforce file_permission() checks"),
+        &security_server.as_permission_check(),
+        current_sid,
+        file.node(),
+        &permissions_from_flags(permission_flags, file_class),
+    )
+}
+
 /// Returns the relative path from the root of the file system containing this `DirEntry`.
 fn get_fs_relative_path(dir_entry: &DirEntryHandle) -> FsString {
     let mut path_builder = PathBuilder::new();
@@ -300,7 +351,8 @@ fn make_fs_node_security_xattr(
 }
 
 fn file_class_from_file_mode(mode: FileMode) -> Result<FileClass, Errno> {
-    match mode.bits() & starnix_uapi::S_IFMT {
+    let file_type = mode.bits() & starnix_uapi::S_IFMT;
+    match file_type {
         starnix_uapi::S_IFLNK => Ok(FileClass::Link),
         starnix_uapi::S_IFREG => Ok(FileClass::File),
         starnix_uapi::S_IFDIR => Ok(FileClass::Dir),
@@ -779,70 +831,14 @@ pub fn fs_node_permission(
     permission_flags: PermissionFlags,
 ) -> Result<(), Errno> {
     let current_sid = current_task.security_state.lock().current_sid;
-    let FsNodeSidAndClass { sid: file_sid, class: file_class } =
-        fs_node_effective_sid_and_class(fs_node);
-    if permission_flags.contains(PermissionFlags::READ) {
-        todo_check_permission(
-            TODO_DENY!(
-                "https://fxbug.dev/380855359",
-                "Check read permission when calling fs_node_permission."
-            ),
-            &security_server.as_permission_check(),
-            current_sid,
-            file_sid,
-            CommonFilePermission::Read.for_class(file_class),
-        )?;
-    }
-
-    if permission_flags.contains(PermissionFlags::WRITE) {
-        todo_check_permission(
-            TODO_DENY!(
-                "https://fxbug.dev/380855359",
-                "Check write permission when calling fs_node_permission."
-            ),
-            &security_server.as_permission_check(),
-            current_sid,
-            file_sid,
-            CommonFilePermission::Write.for_class(file_class),
-        )?;
-    }
-
-    if permission_flags.contains(PermissionFlags::APPEND) {
-        check_permission(
-            &security_server.as_permission_check(),
-            current_sid,
-            file_sid,
-            CommonFilePermission::Append.for_class(file_class),
-        )?;
-    }
-
-    if permission_flags.contains(PermissionFlags::EXEC) {
-        if file_class == FileClass::Dir {
-            todo_check_permission(
-                TODO_DENY!(
-                    "https://fxbug.dev/380855359",
-                    "Check search permission when calling fs_node_permission."
-                ),
-                &security_server.as_permission_check(),
-                current_sid,
-                file_sid,
-                DirPermission::Search,
-            )?;
-        } else {
-            todo_check_permission(
-                TODO_DENY!(
-                    "https://fxbug.dev/380855359",
-                    "Check execute permission when calling fs_node_permission."
-                ),
-                &security_server.as_permission_check(),
-                current_sid,
-                file_sid,
-                CommonFilePermission::Execute.for_class(file_class),
-            )?;
-        }
-    }
-
-    Ok(())
+    let file_class = fs_node.security_state.lock().class;
+    todo_has_fs_node_permissions(
+        TODO_DENY!("https://fxbug.dev/380855359", "Enforce fs_node_permission checks."),
+        &security_server.as_permission_check(),
+        current_sid,
+        fs_node,
+        &permissions_from_flags(permission_flags, file_class),
+    )
 }
 
 pub(super) fn check_fs_node_getattr_access(

src/starnix/kernel/security/selinux_hooks/task.rs

@@ -4,11 +4,12 @@
 
 use crate::security::selinux_hooks::{
     check_permission, check_self_permission, fs_node_effective_sid_and_class, fs_node_ensure_class,
-    fs_node_set_label_with_task, has_file_permissions, todo_check_permission, FsNode,
-    PermissionCheck, ProcessPermission, TaskAttrs,
+    fs_node_set_label_with_task, has_file_permissions, todo_check_permission, PermissionCheck,
+    ProcessPermission, TaskAttrs,
 };
 use crate::security::{Arc, ProcAttr, ResolvedElfState, SecurityId, SecurityServer};
 use crate::task::{CurrentTask, Task};
+use crate::vfs::FsNode;
 use crate::TODO_DENY;
 use selinux::{FilePermission, NullessByteStr, ObjectClass};
 use starnix_types::ownership::TempRef;

src/starnix/kernel/vfs/file_object.rs

@@ -19,6 +19,7 @@ use crate::vfs::{
     FdNumber, FdTableId, FileReleaser, FileSystemHandle, FileWriteGuard, FileWriteGuardMode,
     FileWriteGuardRef, FsNodeHandle, NamespaceNode, RecordLockCommand, RecordLockOwner,
 };
+
 use fidl::HandleBased;
 use fidl_fuchsia_fxfs::CryptManagementMarker;
 use fuchsia_inspect_contrib::profile_duration;
@@ -201,6 +202,7 @@ pub trait FileOps: Send + Sync + AsAny + 'static {
         offset: usize,
         data: &mut dyn OutputBuffer,
     ) -> Result<usize, Errno>;
+
     /// Write to the file with an offset. If the file does not have persistent offsets (either
     /// directly, or because it is not seekable), offset will be 0 and can be ignored.
     /// Returns the number of bytes written.
@@ -1657,10 +1659,12 @@ impl FileObject {
     }
 
     /// Common implementation for `read` and `read_at`.
-    fn read_internal<R>(&self, read: R) -> Result<usize, Errno>
+    fn read_internal<R>(&self, current_task: &CurrentTask, read: R) -> Result<usize, Errno>
     where
         R: FnOnce() -> Result<usize, Errno>,
     {
+        security::file_permission(current_task, self, security::PermissionFlags::READ)?;
+
         if !self.can_read() {
             return error!(EBADF);
         }
@@ -1685,7 +1689,7 @@ impl FileObject {
     where
         L: LockEqualOrBefore<FileOpsCore>,
     {
-        self.read_internal(|| {
+        self.read_internal(current_task, || {
             let mut locked = locked.cast_locked::<FileOpsCore>();
             if !self.ops().has_persistent_offsets() {
                 if data.available() > MAX_LFS_FILESIZE {
@@ -1718,7 +1722,9 @@ impl FileObject {
         }
         checked_add_offset_and_length(offset, data.available())?;
         let mut locked = locked.cast_locked::<FileOpsCore>();
-        self.read_internal(|| self.ops.read(&mut locked, self, current_task, offset, data))
+        self.read_internal(current_task, || {
+            self.ops.read(&mut locked, self, current_task, offset, data)
+        })
     }
 
     /// Common checks before calling ops().write.
@@ -1732,6 +1738,8 @@ impl FileObject {
     where
         L: LockEqualOrBefore<FileOpsCore>,
     {
+        security::file_permission(current_task, self, security::PermissionFlags::WRITE)?;
+
         // We need to cap the size of `data` to prevent us from growing the file too large,
         // according to <https://man7.org/linux/man-pages/man2/write.2.html>:
         //

src/starnix/tests/selinux/expectations/selinux.json5

@@ -16,7 +16,7 @@
                 "*entrypoint*",
                 "*execshare*",
 
-                // TODO(http://b/364568874): `inherit` tests will pass once the `file_security_ops`
+                // TODO(http://b/385121365): `inherit` tests will pass once the `file_permission`
                 // hook correctly enforces the `write` permission.
                 "*inherit*",