Re: [PATCH v3] smack: preserve low-integrity labels across file copy via whitelist

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] smack: broken exec handling for smk_preserve in smack_bprm_creds_for_exec()
- [High] smack: missing MAY_WRITE check in smack_inode_init_security() for smk_preserve
- [High] smack: smk_preserve corrupts inode labels across OverlayFS copy-up
- [Medium] smack: silent allocation failure in smk_preserve_set()
- [Medium] smack: smack_file_open() re-records smk_preserve on O_RDONLY | O_CREAT
- [Medium] smack: unimported label rejection and unbounded write in smk_write_preserve_wl()
--

commit d4423a161be1eb02cab93ddd7185fcd574c8aaf2
Author: Tang Pengke <tangpengke@yhkylin.cn>

smack: preserve low-integrity labels across file copy via whitelist

This commit introduces a configurable whitelist (/smack/preserve-whitelist) of
low-integrity Smack labels that are recorded in a per-task LSM blob when a
process opens a whitelisted regular file read-only, so that the next inode
created by that thread inherits the preserved label instead of the process
label.

> diff --git a/security/smack/smack_access.c b/security/smack/smack_access.c
> index fb85356266e53..4982127ce985f 100644
> --- a/security/smack/smack_access.c
> +++ b/security/smack/smack_access.c
> @@ -53,6 +53,83 @@ static u32 smack_next_secid = 10;

[ ... ]

> +void smk_preserve_set(struct smack_known *skp, bool on)
> +{
> +	struct smk_preserve_wl *e, *new = NULL;
> +
> +	if (on) {
> +		new = kzalloc_obj(*new, GFP_KERNEL);
> +		if (!new)
> +			return;
> +		new->skp = skp;
> +	}

[Severity: Medium]
When kzalloc_obj(*new, GFP_KERNEL) fails in smk_preserve_set(), this returns
early without an error code, and smk_write_preserve_wl() in
security/smack/smackfs.c then unconditionally returns count to userspace:

    smk_preserve_set(skp, on);
    kfree(data);
    return count;

Should smk_preserve_set() return an int so that smk_write_preserve_wl() can
propagate -ENOMEM to userspace when the whitelist allocation fails?

[ ... ]

> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 8e88ac65fd7fc..094d26c943b52 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -915,6 +915,11 @@ static int smack_bprm_creds_for_exec(struct linux_binprm *bprm)
>  	struct superblock_smack *sbsp;
>  	int rc;
>  
> +#ifdef CONFIG_SECURITY_SMACK_COPYWHITELIST
> +	/* a new program starts with a clean copy record */
> +	smack_task(current)->smk_preserve = NULL;
> +#endif

[Severity: High]
Can clearing smack_task(current)->smk_preserve in smack_bprm_creds_for_exec()
allow a task to drop its pending preserve record without actually executing a
new program, or leave smk_preserve armed across exec?

In fs/exec.c:bprm_execve(), security_bprm_creds_for_exec() runs before the
bprm->is_check early return and before exec_binprm():

    retval = security_bprm_creds_for_exec(bprm);
    if (retval || bprm->is_check)
        goto out;

    retval = exec_binprm(bprm);

If a task that has opened a whitelisted low-integrity file calls
execveat(..., AT_EXECVE_CHECK) or invokes execve() on a target that fails in
smack_bprm_creds_for_exec() or exec_binprm(), smk_preserve is cleared on
current even though no exec took place, allowing subsequent file creations by
that task to use its trusted process label.

In addition, fs/exec.c:do_open_execat() and open_exec() open the executable
and any interpreter with O_LARGEFILE | O_RDONLY | __FMODE_EXEC. Because
smack_file_open() checks (file->f_flags & O_ACCMODE) == O_RDONLY without
excluding __FMODE_EXEC (or file->f_mode & FMODE_EXEC):

1. If do_open_execat() opens a whitelisted file and then fails before
   bprm_execve() (for example, at path_noexec(&file->f_path) on a noexec
   mount), smack_file_open() has already set smk_preserve on the calling task.

2. If exec_binprm() opens a script interpreter via open_exec() after
   security_bprm_creds_for_exec() has already run, smack_file_open() will
   record smk_preserve for the newly executed program if the interpreter's
   label is on the whitelist.

Should smack_file_open() ignore FMODE_EXEC opens, and should smk_preserve be
cleared when the exec commits in a bprm_committing_creds or
bprm_committed_creds hook instead of smack_bprm_creds_for_exec()?

>  	isp = smack_inode(inode);
>  	if (isp->smk_task == NULL || isp->smk_task == bsp->smk_task)
>  		return 0;
> @@ -1075,6 +1080,26 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
>  		}
>  	}
>  
> +#ifdef CONFIG_SECURITY_SMACK_COPYWHITELIST
> +	/*
> +	 * If a whitelisted label was recorded on file open, inherit it so
> +	 * the new file keeps the source's (low-integrity) label across the
> +	 * copy.  Transmutation takes precedence: when it applies the record
> +	 * is dropped, otherwise it would leak into a later create in a
> +	 * non-transmutable directory.
> +	 */
> +	if (trans_cred || (trans_rule && smk_inode_transmutable(dir))) {
> +		smack_task(current)->smk_preserve = NULL;
> +	} else {
> +		struct smack_known *skp = smack_task(current)->smk_preserve;
> +
> +		if (skp != NULL) {
> +			issp->smk_inode = skp;
> +			smack_task(current)->smk_preserve = NULL;
> +		}
> +	}
> +#endif

[Severity: High]
What happens here when the creating task has MAY_READ access to the
whitelisted label skp, but does not have MAY_WRITE access to skp?

In smack_file_open(), smk_preserve is recorded after checking only MAY_READ on
the source inode. When the task then creates a new file via open(dst,
O_CREAT | O_WRONLY, mode) in a writable non-transmuting directory,
may_o_create() checks MAY_WRITE | MAY_EXEC on the parent directory, and
smack_inode_init_security() assigns issp->smk_inode = skp without checking
whether the task has MAY_WRITE access to skp.

Because FMODE_CREATED is set on the struct file, fs/namei.c:do_open() sets
acc_mode = 0 before may_open():

    if (file->f_mode & FMODE_CREATED) {
        /* Don't check for write permission, don't truncate */
        open_flag &= ~O_TRUNC;
        acc_mode = 0;
    }

may_open() then passes only MAY_OPEN to inode_permission(), which
smack_inode_permission() masks out, and smack_file_open() checks only
MAY_READ. Since Smack does not hook file_permission on write(), the task
receives a writable file descriptor and can write arbitrary content to an
inode labeled skp despite policy denying it write access to skp. Meanwhile,
follow-up operations that copy utilities perform on the newly created file
(such as fchmod(), fchown(), futimens(), setxattr(), rename(), or unlink())
check MAY_WRITE against issp->smk_inode and fail with -EACCES.

[Severity: High]
Also in smack_inode_init_security(), how does smk_preserve interact with
OverlayFS copy-up and credential overrides?

When a task with a pending smk_preserve creates a file in an OverlayFS
directory whose parent is still on a lower layer,
ovl_create_object() -> ovl_copy_up() -> ovl_copy_up_workdir() calls
security_inode_copy_up() -> smack_inode_copy_up() and installs copy_up_cred
via override_creds():

    isp = smack_inode(d_inode(dentry));
    skp = isp->smk_inode;
    tsp->smk_task = skp;
    *new = new_creds;

smack_inode_copy_up() leaves tsp->smk_transmuted as NULL, so when
ovl_create_temp() creates the upper directory, smack_inode_init_security()
sees trans_cred == false, overwrites the copied-up upper directory's label
with smack_task(current)->smk_preserve, and clears smk_preserve = NULL.
Because smack_inode_copy_up_xattr() returns -ECANCELED for XATTR_NAME_SMACK,
the upper directory is permanently relabeled to the low-integrity label, and
when ovl_create_object() then creates the actual target file, smk_preserve has
already been consumed so the copied file receives the task's trusted label.

Similarly, if a task with a pending smk_preserve modifies an existing lower
file or directory on OverlayFS (for example via chmod(), truncate(), or
open(O_WRONLY)), the copy-up creation in ovl_copy_up_tmpfile() or
ovl_copy_up_workdir() consumes smk_preserve and relabels the copied-up inode.
Furthermore, ovl_copy_up_file() opens the lower file internally via
ovl_path_open(..., O_LARGEFILE | O_RDONLY), which reaches smack_file_open()
and records smk_preserve on current even though the user only truncated or
opened the file for writing.

>  	if (rc == 0)
>  		if (xattr_dupval(xattrs, xattr_count,
>  			    XATTR_SMACK_SUFFIX,
> @@ -2077,6 +2102,39 @@ static int smack_file_open(struct file *file)

[ ... ]

> +	if (!list_empty(&smk_preserve_wl) &&
> +	    (file->f_flags & O_ACCMODE) == O_RDONLY &&
> +	    S_ISREG(inode->i_mode)) {
> +		struct smack_known *skp = smk_of_inode(inode);
> +
> +		if (smk_preserve_allowed(skp))
> +			smack_task(current)->smk_preserve = skp;
> +	}
> +#endif

[Severity: Medium]
If a task with a pending smk_preserve creates a new regular file using
open(path, O_RDONLY | O_CREAT, mode) (or open(path, O_CREAT | O_EXCL, mode),
since O_RDONLY is 0), lookup_open() sets FMODE_CREATED and invokes
smack_inode_init_security(), which labels the new inode with skp and clears
smack_task(current)->smk_preserve = NULL.

Later in that same open() syscall, do_open() -> vfs_open() -> do_dentry_open()
calls security_file_open() -> smack_file_open() on the newly created file.
Because (file->f_flags & O_ACCMODE) == O_RDONLY is true and FMODE_CREATED is
not excluded here, won't smack_file_open() see the newly inherited skp on
inode and immediately set smack_task(current)->smk_preserve = skp again,
defeating the one-shot consumption?

> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> index a72bc7fabea99..7a6dc3e2eb812 100644
> --- a/security/smack/smackfs.c
> +++ b/security/smack/smackfs.c

[ ... ]

> @@ -2777,6 +2780,97 @@ static const struct file_operations smk_ptrace_ops = {

[ ... ]

> +static ssize_t smk_write_preserve_wl(struct file *file, const char __user *buf,
> +				     size_t count, loff_t *ppos)
> +{
> +	struct smack_known *skp;
> +	char *data, *label;
> +	bool on = true;
> +
> +	if (!smack_privileged(CAP_MAC_ADMIN))
> +		return -EPERM;
> +
> +	data = memdup_user_nul(buf, count);
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
> +
> +	label = strim(data);
> +	if (!label[0]) {
> +		kfree(data);
> +		return -EINVAL;
> +	}
> +
> +	/* A leading '-' removes the label (same convention as revoke-subject). */
> +	if (label[0] == '-') {
> +		on = false;
> +		label = strim(label + 1);
> +		if (!label[0]) {
> +			kfree(data);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	skp = smk_find_entry(label);
> +	if (!skp) {
> +		kfree(data);
> +		return -ENOENT;
> +	}

[Severity: Medium]
When adding a label to /smack/preserve-whitelist (for example at boot before a
removable filesystem with smackfsdef= is mounted or before policy rules
referencing that label are loaded), smk_find_entry(label) returns NULL and
smk_write_preserve_wl() fails with -ENOENT if the label has not already been
imported into smack_known_hash.

Other smackfs label configuration writers such as smk_write_onlycap(),
smk_write_ambient(), smk_write_unconfined(), smk_write_syslog(), and
smk_write_relabel_self() check *ppos != 0, bound count (to SMK_LONGLABEL /
PAGE_SIZE), and call smk_import_entry(label, 0) when adding a label so valid
labels are imported on demand. Should smk_write_preserve_wl() also bound count
and use smk_import_entry() when on is true?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/684C81A1C8289709+20261028161521.925644-1-tangpengke@yhkylin.cn?part=1