* Re: [PATCH v2] perf: Require CAP_KILL if sigtrap is requested
From: Marco Elver @ 2021-07-02 7:20 UTC (permalink / raw)
To: Eric W. Biederman
Cc: peterz, tglx, mingo, kasan-dev, linux-kernel, mingo, acme,
mark.rutland, alexander.shishkin, jolsa, namhyung,
linux-perf-users, omosnace, serge, linux-security-module, stable,
Dmitry Vyukov
In-Reply-To: <87h7hdn24k.fsf@disp2133>
On Thu, 1 Jul 2021 at 23:41, Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> Marco Elver <elver@google.com> writes:
>
> > If perf_event_open() is called with another task as target and
> > perf_event_attr::sigtrap is set, and the target task's user does not
> > match the calling user, also require the CAP_KILL capability.
> >
> > Otherwise, with the CAP_PERFMON capability alone it would be possible
> > for a user to send SIGTRAP signals via perf events to another user's
> > tasks. This could potentially result in those tasks being terminated if
> > they cannot handle SIGTRAP signals.
> >
> > Note: The check complements the existing capability check, but is not
> > supposed to supersede the ptrace_may_access() check. At a high level we
> > now have:
> >
> > capable of CAP_PERFMON and (CAP_KILL if sigtrap)
> > OR
> > ptrace_may_access() // also checks for same thread-group and uid
>
> Is there anyway we could have a comment that makes the required
> capability checks clear?
>
> Basically I see an inlined version of kill_ok_by_cred being implemented
> without the comments on why the various pieces make sense.
I'll add more comments. It probably also makes sense to factor the
code here into its own helper.
> Certainly ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS) should not
> be a check to allow writing/changing a task. It needs to be
> PTRACE_MODE_ATTACH_REALCREDS, like /proc/self/mem uses.
So if attr.sigtrap the checked ptrace mode needs to switch to
PTRACE_MODE_ATTACH_REALCREDS. Otherwise, it is possible to send a
signal if only read-ptrace permissions are granted.
Is my assumption here correct?
> Now in practice I think your patch probably has the proper checks in
> place for sending a signal but it is far from clear.
Thanks,
-- Marco
^ permalink raw reply
* Re: [PATCH v8 0/2] Add support for ECDSA-signed kernel modules
From: Jarkko Sakkinen @ 2021-07-02 6:50 UTC (permalink / raw)
To: Stefan Berger
Cc: jeyu, keyrings, dhowells, dwmw2, zohar, nayna, linux-integrity,
linux-security-module, linux-kernel, torvalds, Stefan Berger
In-Reply-To: <20210629213421.60320-1-stefanb@linux.vnet.ibm.com>
On Tue, Jun 29, 2021 at 05:34:19PM -0400, Stefan Berger wrote:
> From: Stefan Berger <stefanb@linux.ibm.com>
>
> This series adds support for ECDSA-signed kernel modules. It also
> attempts to address a kbuild issue where a developer created an ECDSA
> key for signing kernel modules and then builds an older version of the
> kernel, when bisecting the kernel for example, that does not support
> ECDSA keys.
>
> The first patch addresses the kbuild issue of needing to delete that
> ECDSA key if it is in certs/signing_key.pem and trigger the creation
> of an RSA key. However, for this to work this patch would have to be
> backported to previous versions of the kernel but would also only work
> for the developer if he/she used a stable version of the kernel to which
> this patch was applied. So whether this patch actually achieves the
> wanted effect is not always guaranteed.
>
> The 2nd patch adds the support for the ECSDA-signed kernel modules.
>
> Stefan
>
> v8:
> - Removed R-b tags and reordered Cc tags
>
> v7:
> - Changed Kconfig reference to kernel version from 5.11 to 5.13
> - Redirecting stderr of openssl to NULL device to address kernel robot
> detected issue
> - Replaced $(CONFIG_MODULE_SIG_KEY) with "certs/signing_key.pem" following
> Linus's suggestion
>
> v6:
> - Patch 2/4 is fixing V4's 1/2 and 4/4 is fixing V4's 2/2. Both fixup
> patches to be squashed.
>
> v5:
> - do not touch the key files if openssl is not installed; likely
> addresses an issue pointed out by kernel test robot
>
> v4:
> - extending 'depends on' with MODULES to (IMA_APPRAISE_MODSIG && MODULES)
>
> v3:
> - added missing OIDs for ECDSA signed hashes to pkcs7_sig_note_pkey_algo
> - added recommendation to use string hash to Kconfig help text
>
> v2:
> - Adjustment to ECDSA key detector string in 2/2
> - Rephrased cover letter and patch descriptions with Mimi
>
>
> Stefan Berger (2):
> certs: Trigger creation of RSA module signing key if it's not an RSA
> key
> certs: Add support for using elliptic curve keys for signing modules
>
> certs/Kconfig | 26 ++++++++++++++++++++++++++
> certs/Makefile | 21 +++++++++++++++++++++
> crypto/asymmetric_keys/pkcs7_parser.c | 8 ++++++++
> 3 files changed, 55 insertions(+)
>
> --
> 2.31.1
>
>
LGTM
For both:
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
/Jarkko
^ permalink raw reply
* Re: [PATCH v8 1/2] certs: Trigger creation of RSA module signing key if it's not an RSA key
From: Jarkko Sakkinen @ 2021-07-02 6:49 UTC (permalink / raw)
To: Linus Torvalds
Cc: Stefan Berger, Jessica Yu, keyrings, David Howells,
David Woodhouse, Mimi Zohar, Nayna Jain, linux-integrity,
LSM List, Linux Kernel Mailing List, Stefan Berger
In-Reply-To: <CAHk-=wgVZ6PUJ6Q=vqnhSkHnE2Rvr72xPFjoRU4=HHn-Rqxu4w@mail.gmail.com>
On Wed, Jun 30, 2021 at 12:17:38PM -0700, Linus Torvalds wrote:
> On Tue, Jun 29, 2021 at 2:34 PM Stefan Berger
> <stefanb@linux.vnet.ibm.com> wrote:
> >
> > Address a kbuild issue where a developer created an ECDSA key for signing
> > kernel modules and then builds an older version of the kernel, when bi-
> > secting the kernel for example, that does not support ECDSA keys.
>
> Thanks, these two don't confuse me any more.
>
> Linus
I'll (re-)test the changes, and make a PR after rc1 out.
/Jarkko
^ permalink raw reply
* Re: [PATCH v8 3/8] security/brute: Detect a brute force attack
From: Alexander Lobakin @ 2021-07-01 23:55 UTC (permalink / raw)
To: John Wood
Cc: Alexander Lobakin, Kees Cook, Jann Horn, Jonathan Corbet,
James Morris, Serge E. Hallyn, Shuah Khan, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, x86, H. Peter Anvin, Arnd Bergmann,
Andi Kleen, valdis.kletnieks, Greg Kroah-Hartman, Randy Dunlap,
Andrew Morton, linux-doc, linux-kernel, linux-security-module,
linux-kselftest, linux-arch, linux-hardening, kernel-hardening
Hi,
From: John Wood <john.wood@gmx.com>
Date: Sat, 5 Jun 2021 17:04:00 +0200
> For a correct management of a fork brute force attack it is necessary to
> track all the information related to the application crashes. To do so,
> use the extended attributes (xattr) of the executable files and define a
> statistical data structure to hold all the necessary information shared
> by all the fork hierarchy processes. This info is the number of crashes,
> the last crash timestamp and the crash period's moving average.
>
> The same can be achieved using a pointer to the fork hierarchy
> statistical data held by the task_struct structure. But this has an
> important drawback: a brute force attack that happens through the execve
> system call losts the faults info since these statistics are freed when
> the fork hierarchy disappears. Using this method makes not possible to
> manage this attack type that can be successfully treated using extended
> attributes.
>
> Also, to avoid false positives during the attack detection it is
> necessary to narrow the possible cases. So, only the following scenarios
> are taken into account:
>
> 1.- Launching (fork()/exec()) a setuid/setgid process repeatedly until a
> desirable memory layout is got (e.g. Stack Clash).
> 2.- Connecting to an exec()ing network daemon (e.g. xinetd) repeatedly
> until a desirable memory layout is got (e.g. what CTFs do for simple
> network service).
> 3.- Launching processes without exec() (e.g. Android Zygote) and
> exposing state to attack a sibling.
> 4.- Connecting to a fork()ing network daemon (e.g. apache) repeatedly
> until the previously shared memory layout of all the other children
> is exposed (e.g. kind of related to HeartBleed).
>
> In each case, a privilege boundary has been crossed:
>
> Case 1: setuid/setgid process
> Case 2: network to local
> Case 3: privilege changes
> Case 4: network to local
>
> To mark that a privilege boundary has been crossed it is only necessary
> to create a new stats for the executable file via the extended attribute
> and only if it has no previous statistical data. This is done using four
> different LSM hooks, one per privilege boundary:
>
> setuid/setgid process --> bprm_creds_from_file hook (based on secureexec
> flag).
> network to local -------> socket_accept hook (taking into account only
> external connections).
> privilege changes ------> task_fix_setuid and task_fix_setgid hooks.
>
> To detect a brute force attack it is necessary that the executable file
> statistics be updated in every fatal crash and the most important data
> to update is the application crash period. To do so, use the new
> "task_fatal_signal" LSM hook added in a previous step.
>
> The application crash period must be a value that is not prone to change
> due to spurious data and follows the real crash period. So, to compute
> it, the exponential moving average (EMA) is used.
>
> Based on the updated statistics two different attacks can be handled. A
> slow brute force attack that is detected if the maximum number of faults
> per fork hierarchy is reached and a fast brute force attack that is
> detected if the application crash period falls below a certain
> threshold.
>
> Moreover, only the signals delivered by the kernel are taken into
> account with the exception of the SIGABRT signal since the latter is
> used by glibc for stack canary, malloc, etc failures, which may indicate
> that a mitigation has been triggered.
>
> Signed-off-by: John Wood <john.wood@gmx.com>
>
> <snip>
>
> +static int brute_get_xattr_stats(struct dentry *dentry, struct inode *inode,
> + struct brute_stats *stats)
> +{
> + int rc;
> + struct brute_raw_stats raw_stats;
> +
> + rc = __vfs_getxattr(dentry, inode, XATTR_NAME_BRUTE, &raw_stats,
> + sizeof(raw_stats));
> + if (rc < 0)
> + return rc;
> +
> + stats->faults = le32_to_cpu(raw_stats.faults);
> + stats->nsecs = le64_to_cpu(raw_stats.nsecs);
> + stats->period = le64_to_cpu(raw_stats.period);
> + stats->flags = raw_stats.flags;
> + return 0;
> +}
>
> <snip>
>
> +static int brute_task_execve(struct linux_binprm *bprm, struct file *file)
> +{
> + struct dentry *dentry = file_dentry(bprm->file);
> + struct inode *inode = file_inode(bprm->file);
> + struct brute_stats stats;
> + int rc;
> +
> + inode_lock(inode);
> + rc = brute_get_xattr_stats(dentry, inode, &stats);
> + if (WARN_ON_ONCE(rc && rc != -ENODATA))
> + goto unlock;
I think I caught a problem here. Have you tested this with
initramfs?
According to init/do_mount.c's
init_rootfs()/rootfs_init_fs_context(), when `root=` cmdline
parameter is not empty, kernel creates rootfs of type ramfs
(tmpfs otherwise).
The thing about ramfs is that it doesn't support xattrs.
I'm running this v8 on a regular PC with initramfs and having
`root=` in cmdline, and Brute doesn't allow the kernel to run
any init processes (/init, /sbin/init, ...) with err == -95
(-EOPNOTSUPP) -- I'm getting a
WARNING: CPU: 0 PID: 173 at brute_task_execve+0x15d/0x200
<snip>
Failed to execute /init (error -95)
and so on (and a panic at the end).
If I omit `root=` from cmdline, then the kernel runs init process
just fine -- I guess because initramfs is then placed inside tmpfs
with xattr support.
As for me, this ramfs/tmpfs selection based on `root=` presence
is ridiculous and I don't see or know any reasons behind that.
But that's another story, and ramfs might be not the only one
system without xattr support.
I think Brute should have a fallback here, e.g. it could simply
ignore files from xattr-incapable filesystems instead of such
WARNING splats and stuff.
> +
> + if (rc == -ENODATA && bprm->secureexec) {
> + brute_reset_stats(&stats);
> + rc = brute_set_xattr_stats(dentry, inode, &stats);
> + if (WARN_ON_ONCE(rc))
> + goto unlock;
> + }
> +
> + rc = 0;
> +unlock:
> + inode_unlock(inode);
> + return rc;
> +}
> +
>
> <snip>
Thanks,
Al
^ permalink raw reply
* Re: [PATCH v2] perf: Require CAP_KILL if sigtrap is requested
From: Eric W. Biederman @ 2021-07-01 21:41 UTC (permalink / raw)
To: Marco Elver
Cc: peterz, tglx, mingo, kasan-dev, linux-kernel, mingo, acme,
mark.rutland, alexander.shishkin, jolsa, namhyung,
linux-perf-users, omosnace, serge, linux-security-module, stable,
Dmitry Vyukov
In-Reply-To: <20210701083842.580466-1-elver@google.com>
Marco Elver <elver@google.com> writes:
> If perf_event_open() is called with another task as target and
> perf_event_attr::sigtrap is set, and the target task's user does not
> match the calling user, also require the CAP_KILL capability.
>
> Otherwise, with the CAP_PERFMON capability alone it would be possible
> for a user to send SIGTRAP signals via perf events to another user's
> tasks. This could potentially result in those tasks being terminated if
> they cannot handle SIGTRAP signals.
>
> Note: The check complements the existing capability check, but is not
> supposed to supersede the ptrace_may_access() check. At a high level we
> now have:
>
> capable of CAP_PERFMON and (CAP_KILL if sigtrap)
> OR
> ptrace_may_access() // also checks for same thread-group and uid
Is there anyway we could have a comment that makes the required
capability checks clear?
Basically I see an inlined version of kill_ok_by_cred being implemented
without the comments on why the various pieces make sense.
Certainly ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS) should not
be a check to allow writing/changing a task. It needs to be
PTRACE_MODE_ATTACH_REALCREDS, like /proc/self/mem uses.
Now in practice I think your patch probably has the proper checks in
place for sending a signal but it is far from clear.
Eric
> Fixes: 97ba62b27867 ("perf: Add support for SIGTRAP on perf events")
> Cc: <stable@vger.kernel.org> # 5.13+
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Marco Elver <elver@google.com>
> ---
> v2:
> * Drop kill_capable() and just check CAP_KILL (reported by Ondrej Mosnacek).
> * Use ns_capable(__task_cred(task)->user_ns, CAP_KILL) to check for
> capability in target task's ns (reported by Ondrej Mosnacek).
> ---
> kernel/events/core.c | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index fe88d6eea3c2..43c99695dc3f 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -12152,10 +12152,23 @@ SYSCALL_DEFINE5(perf_event_open,
> }
>
> if (task) {
> + bool is_capable;
> +
> err = down_read_interruptible(&task->signal->exec_update_lock);
> if (err)
> goto err_file;
>
> + is_capable = perfmon_capable();
> + if (attr.sigtrap) {
> + /*
> + * perf_event_attr::sigtrap sends signals to the other
> + * task. Require the current task to have CAP_KILL.
> + */
> + rcu_read_lock();
> + is_capable &= ns_capable(__task_cred(task)->user_ns, CAP_KILL);
> + rcu_read_unlock();
> + }
> +
> /*
> * Preserve ptrace permission check for backwards compatibility.
> *
> @@ -12165,7 +12178,7 @@ SYSCALL_DEFINE5(perf_event_open,
> * perf_event_exit_task() that could imply).
> */
> err = -EACCES;
> - if (!perfmon_capable() && !ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS))
> + if (!is_capable && !ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS))
> goto err_cred;
> }
^ permalink raw reply
* Re: [PATCH v2 6/6] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
From: Richard Weinberger @ 2021-07-01 20:42 UTC (permalink / raw)
To: Ahmad Fatoum
Cc: Jonathan Corbet, David Howells, Jarkko Sakkinen, James Bottomley,
Mimi Zohar, kernel, James Morris, Serge E. Hallyn, horia geanta,
aymen sghaier, Herbert Xu, davem, Udit Agarwal, Eric Biggers,
Jan Luebbe, david, Franck Lenormand, Sumit Garg,
open list, ASYMMETRIC KEYS, Linux Crypto Mailing List,
Linux Doc Mailing List, linux-integrity, linux-kernel, LSM
In-Reply-To: <39e6d65ca5d2a0a35fb71d6c1f85add8ee489a19.1624364386.git-series.a.fatoum@pengutronix.de>
Ahmad,
----- Ursprüngliche Mail -----
> Von: "Ahmad Fatoum" <a.fatoum@pengutronix.de>
> +static struct caam_blob_priv *blobifier;
> +
> +#define KEYMOD "kernel:trusted"
I'm still think that hard coding the key modifier is not wise.
As I said[0], there are folks out there that want to provide their own modifier,
so it is not only about being binary compatible with other CAAM blob patches in the wild.
I'll happily implement that feature after your patches got merged but IMHO we should first agree on an interface.
How about allowing another optional parameter to Opt_new and Opt_load and having a key modifier
per struct trusted_key_payload instance?
Thanks,
//richard
[0]
https://patchwork.kernel.org/project/linux-crypto/patch/319e558e1bd19b80ad6447c167a2c3942bdafea2.1615914058.git-series.a.fatoum@pengutronix.de/#24085397
^ permalink raw reply
* Re: [PATCH v2 3/3] ima: Add digest and digest_len params to the functions to measure a buffer
From: Lakshmi Ramasubramanian @ 2021-07-01 17:27 UTC (permalink / raw)
To: Roberto Sassu, zohar, paul
Cc: stephen.smalley.work, prsriva02, tusharsu, linux-integrity,
linux-security-module, linux-kernel, selinux
In-Reply-To: <20210701125552.2958008-4-roberto.sassu@huawei.com>
On 7/1/2021 5:55 AM, Roberto Sassu wrote:
Hi Roberto,
> This patch adds the 'digest' and 'digest_len' parameters to
> ima_measure_critical_data() and process_buffer_measurement(), so that
> callers can get the digest of the passed buffer.
>
> These functions calculate the digest even if there is no suitable rule in
> the IMA policy and, in this case, they simply return 1 before generating a
> new measurement entry.
>
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 3386e7436440..b4b1dc25e4fb 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -838,17 +838,20 @@ int ima_post_load_data(char *buf, loff_t size,
> * @pcr: pcr to extend the measurement
> * @func_data: func specific data, may be NULL
> * @buf_hash: measure buffer data hash
> + * @digest: buffer digest will be written to
> + * @digest_len: buffer length
> *
> * Based on policy, either the buffer data or buffer data hash is measured
> *
> - * Return: 0 if the buffer has been successfully measured, a negative value
> - * otherwise.
> + * Return: 0 if the buffer has been successfully measured, 1 if the digest
> + * has been written to the passed location but not added to a measurement entry,
> + * a negative value otherwise.
> */
> int process_buffer_measurement(struct user_namespace *mnt_userns,
> struct inode *inode, const void *buf, int size,
> const char *eventname, enum ima_hooks func,
> int pcr, const char *func_data,
> - bool buf_hash)
> + bool buf_hash, u8 *digest, size_t digest_len)
> {
> int ret = 0;
> const char *audit_cause = "ENOMEM";
> @@ -869,7 +872,10 @@ int process_buffer_measurement(struct user_namespace *mnt_userns,
> int action = 0;
> u32 secid;
>
> - if (!ima_policy_flag)
> + if (digest && digest_len < digest_hash_len)
> + return -EINVAL;
> +
> + if (!ima_policy_flag && !digest)
> return -ENOENT;
Just wanted to check if you have verified this scenario:
If ima_policy_flag is 0, the in-memory ima policy data is not yet
initialized. In this case calling ima_get_action() will cause kernel
panic (NULL exception).
Please verify the above issue doesn't exist if the caller passes
non-NULL digest and ima_policy_flag is 0 (ima policy is not initialized).
thanks,
-lakshmi
>
> template = ima_template_desc_buf();
> @@ -891,7 +897,7 @@ int process_buffer_measurement(struct user_namespace *mnt_userns,
> action = ima_get_action(mnt_userns, inode, current_cred(),
> secid, 0, func, &pcr, &template,
> func_data);
> - if (!(action & IMA_MEASURE))
> + if (!(action & IMA_MEASURE) && !digest)
> return -ENOENT;
> }
>
> @@ -922,6 +928,12 @@ int process_buffer_measurement(struct user_namespace *mnt_userns,
> event_data.buf_len = digest_hash_len;
> }
>
> + if (digest)
> + memcpy(digest, iint.ima_hash->digest, digest_hash_len);
> +
> + if (!ima_policy_flag || (func && !(action & IMA_MEASURE)))
> + return 1;
> +
> ret = ima_alloc_init_template(&event_data, &entry, template);
> if (ret < 0) {
> audit_cause = "alloc_entry";
> @@ -966,7 +978,7 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
> ret = process_buffer_measurement(file_mnt_user_ns(f.file),
> file_inode(f.file), buf, size,
> "kexec-cmdline", KEXEC_CMDLINE, 0,
> - NULL, false);
> + NULL, false, NULL, 0);
> fdput(f);
> }
>
> @@ -977,26 +989,30 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
> * @buf: pointer to buffer data
> * @buf_len: length of buffer data (in bytes)
> * @hash: measure buffer data hash
> + * @digest: buffer digest will be written to
> + * @digest_len: buffer length
> *
> * Measure data critical to the integrity of the kernel into the IMA log
> * and extend the pcr. Examples of critical data could be various data
> * structures, policies, and states stored in kernel memory that can
> * impact the integrity of the system.
> *
> - * Return: 0 if the buffer has been successfully measured, a negative value
> - * otherwise.
> + * Return: 0 if the buffer has been successfully measured, 1 if the digest
> + * has been written to the passed location but not added to a measurement entry,
> + * a negative value otherwise.
> */
> int ima_measure_critical_data(const char *event_label,
> const char *event_name,
> const void *buf, size_t buf_len,
> - bool hash)
> + bool hash, u8 *digest, size_t digest_len)
> {
> if (!event_name || !event_label || !buf || !buf_len)
> return -ENOPARAM;
>
> return process_buffer_measurement(&init_user_ns, NULL, buf, buf_len,
> event_name, CRITICAL_DATA, 0,
> - event_label, hash);
> + event_label, hash, digest,
> + digest_len);
> }
>
> static int __init init_ima(void)
> diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c
> index e3047ce64f39..b02b061c5fac 100644
> --- a/security/integrity/ima/ima_queue_keys.c
> +++ b/security/integrity/ima/ima_queue_keys.c
> @@ -166,7 +166,7 @@ void ima_process_queued_keys(void)
> entry->keyring_name,
> KEY_CHECK, 0,
> entry->keyring_name,
> - false);
> + false, NULL, 0);
> list_del(&entry->list);
> ima_free_key_entry(entry);
> }
> diff --git a/security/selinux/ima.c b/security/selinux/ima.c
> index 4db9fa211638..d5d7b3ca9651 100644
> --- a/security/selinux/ima.c
> +++ b/security/selinux/ima.c
> @@ -88,7 +88,7 @@ void selinux_ima_measure_state_locked(struct selinux_state *state)
>
> measure_rc = ima_measure_critical_data("selinux", "selinux-state",
> state_str, strlen(state_str),
> - false);
> + false, NULL, 0);
>
> kfree(state_str);
>
> @@ -105,7 +105,8 @@ void selinux_ima_measure_state_locked(struct selinux_state *state)
> }
>
> measure_rc = ima_measure_critical_data("selinux", "selinux-policy-hash",
> - policy, policy_len, true);
> + policy, policy_len, true,
> + NULL, 0);
>
> vfree(policy);
> }
>
^ permalink raw reply
* Re: [RFC PATCH 0/1] xattr: Allow user.* xattr on symlink/special files if caller has CAP_SYS_RESOURCE
From: Casey Schaufler @ 2021-07-01 16:58 UTC (permalink / raw)
To: Vivek Goyal, Dr. David Alan Gilbert
Cc: Theodore Ts'o, Daniel Walsh, Schaufler, Casey,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
viro@zeniv.linux.org.uk, virtio-fs@redhat.com,
berrange@redhat.com, linux-security-module,
selinux@vger.kernel.org, Casey Schaufler
In-Reply-To: <20210701131030.GB159380@redhat.com>
On 7/1/2021 6:10 AM, Vivek Goyal wrote:
> On Thu, Jul 01, 2021 at 09:48:33AM +0100, Dr. David Alan Gilbert wrote:
>> * Theodore Ts'o (tytso@mit.edu) wrote:
>>> On Wed, Jun 30, 2021 at 04:01:42PM +0100, Dr. David Alan Gilbert wrote:
>>>> Even if you fix symlinks, I don't think it fixes device nodes or
>>>> anything else where the permissions bitmap isn't purely used as the
>>>> permissions on the inode.
>>> I think we're making a mountain out of a molehill. Again, very few
>>> people are using quota these days. And if you give someone write
>>> access to a 8TB disk, do you really care if they can "steal" 32k worth
>>> of space (which is the maximum size of an xattr, enforced by the VFS).
>>>
>>> OK, but what about character mode devices? First of all, most users
>>> don't have access to huge number of devices, but let's assume
>>> something absurd. Let's say that a user has write access to *1024*
>>> devices. (My /dev has 233 character mode devices, and I have write
>>> access to well under a dozen.)
>>>
>>> An 8TB disk costs about $200. So how much of the "stolen" quota space
>>> are we talking about, assuming the user has access to 1024 devices,
>>> and the file system actually supports a 32k xattr.
>>>
>>> 32k * 1024 * $200 / 8TB / (1024*1024*1024) = $0.000763 = 0.0763 cents
>>>
>>> A 2TB SSD is less around $180, so even if we calculate the prices
>>> based on SSD space, we're still talking about a quarter of a penny.
>>>
>>> Why are we worrying about this?
>> I'm not worrying about storage cost, but we would need to define what
>> the rules are on who can write and change a user.* xattr on a device
>> node. It doesn't feel sane to make it anyone who can write to the
>> device; then everyone can start leaving droppings on /dev/null.
>>
>> The other evilness I can imagine, is if there's a 32k limit on xattrs on
>> a node, an evil user could write almost 32k of junk to the node
>> and then break the next login that tries to add an acl or breaks the
>> next relabel.
> I guess 64k is per xattr VFS size limit.
>
> #define XATTR_SIZE_MAX 65536
>
> I just wrote a simple program to write "user.<N>" xattrs of size 1K
> each and could easily write 1M xattrs. So that 1G worth data right
> there. I did not try to push it further.
>
> So a user can write lot of data in the form of user.* xattrs on
> symlinks and device nodes if were to open it unconditionally. Hence
> permission semantics will probably will have to defined properly.
>
> I am wondering will it be alright if owner of the file (or CAP_FOWNER),
> is allowed to write user.* xattrs on symlinks and special files.
That would be sensible.
That's independent of your xattr mapping scheme.
>
> Vivek
>
^ permalink raw reply
* Re: [PATCH v2 2/3] ima: Return int in the functions to measure a buffer
From: Lakshmi Ramasubramanian @ 2021-07-01 16:15 UTC (permalink / raw)
To: Roberto Sassu, zohar, paul
Cc: stephen.smalley.work, prsriva02, tusharsu, linux-integrity,
linux-security-module, linux-kernel, selinux
In-Reply-To: <20210701125552.2958008-3-roberto.sassu@huawei.com>
On 7/1/2021 5:55 AM, Roberto Sassu wrote:
> ima_measure_critical_data() and process_buffer_measurement() currently
> don't return a result. A caller wouldn't be able to know whether those
> functions were executed successfully.
>
> This patch modifies the return type from void to int, and returns 0 if the
> buffer has been successfully measured, a negative value otherwise.
>
> Also, this patch does not modify the behavior of existing callers by
> processing the returned value. Instead, the value is stored in a variable
> marked as __maybe_unused.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
> include/linux/ima.h | 15 +++---
> security/integrity/ima/ima.h | 10 ++--
> security/integrity/ima/ima_appraise.c | 4 +-
> security/integrity/ima/ima_asymmetric_keys.c | 4 +-
> security/integrity/ima/ima_init.c | 6 ++-
> security/integrity/ima/ima_main.c | 48 ++++++++++++--------
> security/integrity/ima/ima_queue_keys.c | 15 +++---
> security/selinux/ima.c | 10 ++--
> 8 files changed, 66 insertions(+), 46 deletions(-)
>
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 81e830d01ced..60492263aa64 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -35,10 +35,10 @@ extern void ima_post_path_mknod(struct user_namespace *mnt_userns,
> extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
> extern int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size);
> extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
> -extern void ima_measure_critical_data(const char *event_label,
> - const char *event_name,
> - const void *buf, size_t buf_len,
> - bool hash);
> +extern int ima_measure_critical_data(const char *event_label,
> + const char *event_name,
> + const void *buf, size_t buf_len,
> + bool hash);
>
> #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
> extern void ima_appraise_parse_cmdline(void);
> @@ -144,10 +144,13 @@ static inline int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size
>
> static inline void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) {}
>
> -static inline void ima_measure_critical_data(const char *event_label,
> +static inline int ima_measure_critical_data(const char *event_label,
> const char *event_name,
> const void *buf, size_t buf_len,
> - bool hash) {}
> + bool hash)
> +{
> + return -ENOENT;
> +}
>
> #endif /* CONFIG_IMA */
>
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index f0e448ed1f9f..03db221324c3 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -264,11 +264,11 @@ void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
> struct evm_ima_xattr_data *xattr_value,
> int xattr_len, const struct modsig *modsig, int pcr,
> struct ima_template_desc *template_desc);
> -void process_buffer_measurement(struct user_namespace *mnt_userns,
> - struct inode *inode, const void *buf, int size,
> - const char *eventname, enum ima_hooks func,
> - int pcr, const char *func_data,
> - bool buf_hash);
> +int process_buffer_measurement(struct user_namespace *mnt_userns,
> + struct inode *inode, const void *buf, int size,
> + const char *eventname, enum ima_hooks func,
> + int pcr, const char *func_data,
> + bool buf_hash);
> void ima_audit_measurement(struct integrity_iint_cache *iint,
> const unsigned char *filename);
> int ima_alloc_init_template(struct ima_event_data *event_data,
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index ef9dcfce45d4..275a2377743f 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -345,6 +345,7 @@ int ima_check_blacklist(struct integrity_iint_cache *iint,
> enum hash_algo hash_algo;
> const u8 *digest = NULL;
> u32 digestsize = 0;
> + int process_rc __maybe_unused;
> int rc = 0;
>
> if (!(iint->flags & IMA_CHECK_BLACKLIST))
> @@ -355,7 +356,8 @@ int ima_check_blacklist(struct integrity_iint_cache *iint,
>
> rc = is_binary_blacklisted(digest, digestsize);
> if ((rc == -EPERM) && (iint->flags & IMA_MEASURE))
> - process_buffer_measurement(&init_user_ns, NULL, digest, digestsize,
> + process_rc = process_buffer_measurement(&init_user_ns,
I think there is no need to make this change now. If and when
ima_check_blacklist() needs to look at the return value of p_b_m(), this
change can be made.
> + NULL, digest, digestsize,
> "blacklisted-hash", NONE,
> pcr, NULL, false);
> }
> diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
> index c985418698a4..910367cdd920 100644
> --- a/security/integrity/ima/ima_asymmetric_keys.c
> +++ b/security/integrity/ima/ima_asymmetric_keys.c
> @@ -31,6 +31,7 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
> unsigned long flags, bool create)
> {
> bool queued = false;
> + int ret __maybe_unused;
>
> /* Only asymmetric keys are handled by this hook. */
> if (key->type != &key_type_asymmetric)
> @@ -60,7 +61,8 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
> * if the IMA policy is configured to measure a key linked
> * to the given keyring.
> */
> - process_buffer_measurement(&init_user_ns, NULL, payload, payload_len,
> + ret = process_buffer_measurement(&init_user_ns, NULL,
> + payload, payload_len,
Same comment as in ima_check_blacklist() - this change be made when needed.
> keyring->description, KEY_CHECK, 0,
> keyring->description, false);
> }
> diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
> index 5076a7d9d23e..6790eea88db8 100644
> --- a/security/integrity/ima/ima_init.c
> +++ b/security/integrity/ima/ima_init.c
> @@ -118,6 +118,7 @@ void __init ima_load_x509(void)
>
> int __init ima_init(void)
> {
> + int measure_rc __maybe_unused;
> int rc;
>
> ima_tpm_chip = tpm_default_chip();
> @@ -153,8 +154,9 @@ int __init ima_init(void)
>
> ima_init_key_queue();
>
> - ima_measure_critical_data("kernel_info", "kernel_version",
> - UTS_RELEASE, strlen(UTS_RELEASE), false);
> + measure_rc = ima_measure_critical_data("kernel_info", "kernel_version",
> + UTS_RELEASE, strlen(UTS_RELEASE),
> + false);
Same comment as in ima_check_blacklist() - this change be made when needed.
>
> return rc;
> }
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 8ef1fa357e0c..3386e7436440 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -827,7 +827,7 @@ int ima_post_load_data(char *buf, loff_t size,
> return 0;
> }
>
> -/*
> +/**
> * process_buffer_measurement - Measure the buffer or the buffer data hash
> * @mnt_userns: user namespace of the mount the inode was found from
> * @inode: inode associated with the object being measured (NULL for KEY_CHECK)
> @@ -840,12 +840,15 @@ int ima_post_load_data(char *buf, loff_t size,
> * @buf_hash: measure buffer data hash
> *
> * Based on policy, either the buffer data or buffer data hash is measured
> + *
> + * Return: 0 if the buffer has been successfully measured, a negative value
> + * otherwise.
> */
> -void process_buffer_measurement(struct user_namespace *mnt_userns,
> - struct inode *inode, const void *buf, int size,
> - const char *eventname, enum ima_hooks func,
> - int pcr, const char *func_data,
> - bool buf_hash)
> +int process_buffer_measurement(struct user_namespace *mnt_userns,
> + struct inode *inode, const void *buf, int size,
> + const char *eventname, enum ima_hooks func,
> + int pcr, const char *func_data,
> + bool buf_hash)
> {
> int ret = 0;
> const char *audit_cause = "ENOMEM";
> @@ -867,7 +870,7 @@ void process_buffer_measurement(struct user_namespace *mnt_userns,
> u32 secid;
>
> if (!ima_policy_flag)
> - return;
> + return -ENOENT;
>
> template = ima_template_desc_buf();
> if (!template) {
> @@ -889,7 +892,7 @@ void process_buffer_measurement(struct user_namespace *mnt_userns,
> secid, 0, func, &pcr, &template,
> func_data);
> if (!(action & IMA_MEASURE))
> - return;
> + return -ENOENT;
> }
>
> if (!pcr)
> @@ -937,7 +940,7 @@ void process_buffer_measurement(struct user_namespace *mnt_userns,
> func_measure_str(func),
> audit_cause, ret, 0, ret);
>
> - return;
> + return ret;
> }
>
> /**
> @@ -951,6 +954,7 @@ void process_buffer_measurement(struct user_namespace *mnt_userns,
> void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
> {
> struct fd f;
> + int ret __maybe_unused;
>
> if (!buf || !size)
> return;
> @@ -959,9 +963,10 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
> if (!f.file)
> return;
>
> - process_buffer_measurement(file_mnt_user_ns(f.file), file_inode(f.file),
> - buf, size, "kexec-cmdline", KEXEC_CMDLINE, 0,
> - NULL, false);
> + ret = process_buffer_measurement(file_mnt_user_ns(f.file),
> + file_inode(f.file), buf, size,
> + "kexec-cmdline", KEXEC_CMDLINE, 0,
> + NULL, false);
Since the return value of p_b_m() is not used here, this change can be
made when needed.
> fdput(f);
> }
>
> @@ -977,18 +982,21 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
> * and extend the pcr. Examples of critical data could be various data
> * structures, policies, and states stored in kernel memory that can
> * impact the integrity of the system.
> + *
> + * Return: 0 if the buffer has been successfully measured, a negative value
> + * otherwise.
> */
> -void ima_measure_critical_data(const char *event_label,
> - const char *event_name,
> - const void *buf, size_t buf_len,
> - bool hash)
> +int ima_measure_critical_data(const char *event_label,
> + const char *event_name,
> + const void *buf, size_t buf_len,
> + bool hash)
> {
> if (!event_name || !event_label || !buf || !buf_len)
> - return;
> + return -ENOPARAM;
>
> - process_buffer_measurement(&init_user_ns, NULL, buf, buf_len, event_name,
> - CRITICAL_DATA, 0, event_label,
> - hash);
> + return process_buffer_measurement(&init_user_ns, NULL, buf, buf_len,
> + event_name, CRITICAL_DATA, 0,
> + event_label, hash);
> }
>
> static int __init init_ima(void)
> diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c
> index 979ef6c71f3d..e3047ce64f39 100644
> --- a/security/integrity/ima/ima_queue_keys.c
> +++ b/security/integrity/ima/ima_queue_keys.c
> @@ -134,6 +134,7 @@ void ima_process_queued_keys(void)
> {
> struct ima_key_entry *entry, *tmp;
> bool process = false;
> + int ret __maybe_unused;
>
> if (ima_process_keys)
> return;
> @@ -159,13 +160,13 @@ void ima_process_queued_keys(void)
>
> list_for_each_entry_safe(entry, tmp, &ima_keys, list) {
> if (!timer_expired)
> - process_buffer_measurement(&init_user_ns, NULL,
> - entry->payload,
> - entry->payload_len,
> - entry->keyring_name,
> - KEY_CHECK, 0,
> - entry->keyring_name,
> - false);
> + ret = process_buffer_measurement(&init_user_ns, NULL,
> + entry->payload,
> + entry->payload_len,
> + entry->keyring_name,
> + KEY_CHECK, 0,
> + entry->keyring_name,
> + false);
Same comment as above.
> list_del(&entry->list);
> ima_free_key_entry(entry);
> }
> diff --git a/security/selinux/ima.c b/security/selinux/ima.c
> index 34d421861bfc..4db9fa211638 100644
> --- a/security/selinux/ima.c
> +++ b/security/selinux/ima.c
> @@ -75,6 +75,7 @@ void selinux_ima_measure_state_locked(struct selinux_state *state)
> char *state_str = NULL;
> void *policy = NULL;
> size_t policy_len;
> + int measure_rc __maybe_unused;
> int rc = 0;
>
> WARN_ON(!mutex_is_locked(&state->policy_mutex));
> @@ -85,8 +86,9 @@ void selinux_ima_measure_state_locked(struct selinux_state *state)
> return;
> }
>
> - ima_measure_critical_data("selinux", "selinux-state",
> - state_str, strlen(state_str), false);
> + measure_rc = ima_measure_critical_data("selinux", "selinux-state",
> + state_str, strlen(state_str),
> + false);
Since the return value of ima_measure_critical_data() is not used here,
this change can be made when needed.
>
> kfree(state_str);
>
> @@ -102,8 +104,8 @@ void selinux_ima_measure_state_locked(struct selinux_state *state)
> return;
> }
>
> - ima_measure_critical_data("selinux", "selinux-policy-hash",
> - policy, policy_len, true);
> + measure_rc = ima_measure_critical_data("selinux", "selinux-policy-hash",
> + policy, policy_len, true);
Same comment as above.
-lakshmi
>
> vfree(policy);
> }
>
^ permalink raw reply
* Re: [PATCH v2 1/3] ima: Introduce ima_get_current_hash_algo()
From: Lakshmi Ramasubramanian @ 2021-07-01 16:01 UTC (permalink / raw)
To: Roberto Sassu, zohar, paul
Cc: stephen.smalley.work, prsriva02, tusharsu, linux-integrity,
linux-security-module, linux-kernel, selinux
In-Reply-To: <20210701125552.2958008-2-roberto.sassu@huawei.com>
On 7/1/2021 5:55 AM, Roberto Sassu wrote:
> This patch introduces the new function ima_get_current_hash_algo(), that
> callers in the other kernel subsystems might use to obtain the hash
> algorithm selected by IMA.
>
> Its primary use will be to determine which algorithm has been used to
> calculate the digest written by ima_measure_critical_data() to the location
> passed as a new parameter (in a subsequent patch). >
> Since the hash algorithm does not change after the IMA setup phase, there
> is no risk of races (obtaining a digest calculated with a different
> algorithm than the one returned).
Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
-lakshmi
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
> include/linux/ima.h | 7 +++++++
> security/integrity/ima/ima_main.c | 5 +++++
> 2 files changed, 12 insertions(+)
>
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 61d5723ec303..81e830d01ced 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -11,9 +11,11 @@
> #include <linux/fs.h>
> #include <linux/security.h>
> #include <linux/kexec.h>
> +#include <crypto/hash_info.h>
> struct linux_binprm;
>
> #ifdef CONFIG_IMA
> +extern enum hash_algo ima_get_current_hash_algo(void);
> extern int ima_bprm_check(struct linux_binprm *bprm);
> extern int ima_file_check(struct file *file, int mask);
> extern void ima_post_create_tmpfile(struct user_namespace *mnt_userns,
> @@ -64,6 +66,11 @@ static inline const char * const *arch_get_ima_policy(void)
> #endif
>
> #else
> +static inline enum hash_algo ima_get_current_hash_algo(void)
> +{
> + return HASH_ALGO__LAST;
> +}
> +
> static inline int ima_bprm_check(struct linux_binprm *bprm)
> {
> return 0;
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 287b90509006..8ef1fa357e0c 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -76,6 +76,11 @@ static int __init hash_setup(char *str)
> }
> __setup("ima_hash=", hash_setup);
>
> +enum hash_algo ima_get_current_hash_algo(void)
> +{
> + return ima_hash_algo;
> +}
> +
> /* Prevent mmap'ing a file execute that is already mmap'ed write */
> static int mmap_violation_check(enum ima_hooks func, struct file *file,
> char **pathbuf, const char **pathname,
>
^ permalink raw reply
* Re: [RFC PATCH 0/1] xattr: Allow user.* xattr on symlink/special files if caller has CAP_SYS_RESOURCE
From: Vivek Goyal @ 2021-07-01 13:10 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Theodore Ts'o, Daniel Walsh, Casey Schaufler,
Schaufler, Casey, linux-fsdevel@vger.kernel.org,
linux-kernel@vger.kernel.org, viro@zeniv.linux.org.uk,
virtio-fs@redhat.com, berrange@redhat.com, linux-security-module,
selinux@vger.kernel.org
In-Reply-To: <YN2BYXv79PswrN2E@work-vm>
On Thu, Jul 01, 2021 at 09:48:33AM +0100, Dr. David Alan Gilbert wrote:
> * Theodore Ts'o (tytso@mit.edu) wrote:
> > On Wed, Jun 30, 2021 at 04:01:42PM +0100, Dr. David Alan Gilbert wrote:
> > >
> > > Even if you fix symlinks, I don't think it fixes device nodes or
> > > anything else where the permissions bitmap isn't purely used as the
> > > permissions on the inode.
> >
> > I think we're making a mountain out of a molehill. Again, very few
> > people are using quota these days. And if you give someone write
> > access to a 8TB disk, do you really care if they can "steal" 32k worth
> > of space (which is the maximum size of an xattr, enforced by the VFS).
> >
> > OK, but what about character mode devices? First of all, most users
> > don't have access to huge number of devices, but let's assume
> > something absurd. Let's say that a user has write access to *1024*
> > devices. (My /dev has 233 character mode devices, and I have write
> > access to well under a dozen.)
> >
> > An 8TB disk costs about $200. So how much of the "stolen" quota space
> > are we talking about, assuming the user has access to 1024 devices,
> > and the file system actually supports a 32k xattr.
> >
> > 32k * 1024 * $200 / 8TB / (1024*1024*1024) = $0.000763 = 0.0763 cents
> >
> > A 2TB SSD is less around $180, so even if we calculate the prices
> > based on SSD space, we're still talking about a quarter of a penny.
> >
> > Why are we worrying about this?
>
> I'm not worrying about storage cost, but we would need to define what
> the rules are on who can write and change a user.* xattr on a device
> node. It doesn't feel sane to make it anyone who can write to the
> device; then everyone can start leaving droppings on /dev/null.
>
> The other evilness I can imagine, is if there's a 32k limit on xattrs on
> a node, an evil user could write almost 32k of junk to the node
> and then break the next login that tries to add an acl or breaks the
> next relabel.
I guess 64k is per xattr VFS size limit.
#define XATTR_SIZE_MAX 65536
I just wrote a simple program to write "user.<N>" xattrs of size 1K
each and could easily write 1M xattrs. So that 1G worth data right
there. I did not try to push it further.
So a user can write lot of data in the form of user.* xattrs on
symlinks and device nodes if were to open it unconditionally. Hence
permission semantics will probably will have to defined properly.
I am wondering will it be alright if owner of the file (or CAP_FOWNER),
is allowed to write user.* xattrs on symlinks and special files.
Vivek
^ permalink raw reply
* [PATCH v2 3/3] ima: Add digest and digest_len params to the functions to measure a buffer
From: Roberto Sassu @ 2021-07-01 12:55 UTC (permalink / raw)
To: zohar, paul
Cc: stephen.smalley.work, prsriva02, tusharsu, nramas,
linux-integrity, linux-security-module, linux-kernel, selinux,
Roberto Sassu
In-Reply-To: <20210701125552.2958008-1-roberto.sassu@huawei.com>
This patch adds the 'digest' and 'digest_len' parameters to
ima_measure_critical_data() and process_buffer_measurement(), so that
callers can get the digest of the passed buffer.
These functions calculate the digest even if there is no suitable rule in
the IMA policy and, in this case, they simply return 1 before generating a
new measurement entry.
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
include/linux/ima.h | 5 +--
security/integrity/ima/ima.h | 2 +-
security/integrity/ima/ima_appraise.c | 2 +-
security/integrity/ima/ima_asymmetric_keys.c | 2 +-
security/integrity/ima/ima_init.c | 2 +-
security/integrity/ima/ima_main.c | 36 ++++++++++++++------
security/integrity/ima/ima_queue_keys.c | 2 +-
security/selinux/ima.c | 5 +--
8 files changed, 37 insertions(+), 19 deletions(-)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 60492263aa64..b6ab66a546ae 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -38,7 +38,7 @@ extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
extern int ima_measure_critical_data(const char *event_label,
const char *event_name,
const void *buf, size_t buf_len,
- bool hash);
+ bool hash, u8 *digest, size_t digest_len);
#ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
extern void ima_appraise_parse_cmdline(void);
@@ -147,7 +147,8 @@ static inline void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) {
static inline int ima_measure_critical_data(const char *event_label,
const char *event_name,
const void *buf, size_t buf_len,
- bool hash)
+ bool hash, u8 *digest,
+ size_t digest_len)
{
return -ENOENT;
}
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 03db221324c3..2f4c20b16ad7 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -268,7 +268,7 @@ int process_buffer_measurement(struct user_namespace *mnt_userns,
struct inode *inode, const void *buf, int size,
const char *eventname, enum ima_hooks func,
int pcr, const char *func_data,
- bool buf_hash);
+ bool buf_hash, u8 *digest, size_t digest_len);
void ima_audit_measurement(struct integrity_iint_cache *iint,
const unsigned char *filename);
int ima_alloc_init_template(struct ima_event_data *event_data,
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 275a2377743f..6ac8715f3563 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -359,7 +359,7 @@ int ima_check_blacklist(struct integrity_iint_cache *iint,
process_rc = process_buffer_measurement(&init_user_ns,
NULL, digest, digestsize,
"blacklisted-hash", NONE,
- pcr, NULL, false);
+ pcr, NULL, false, NULL, 0);
}
return rc;
diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
index 910367cdd920..0ad9995fab98 100644
--- a/security/integrity/ima/ima_asymmetric_keys.c
+++ b/security/integrity/ima/ima_asymmetric_keys.c
@@ -64,5 +64,5 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
ret = process_buffer_measurement(&init_user_ns, NULL,
payload, payload_len,
keyring->description, KEY_CHECK, 0,
- keyring->description, false);
+ keyring->description, false, NULL, 0);
}
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 6790eea88db8..4f5708d68cc7 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -156,7 +156,7 @@ int __init ima_init(void)
measure_rc = ima_measure_critical_data("kernel_info", "kernel_version",
UTS_RELEASE, strlen(UTS_RELEASE),
- false);
+ false, NULL, 0);
return rc;
}
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 3386e7436440..b4b1dc25e4fb 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -838,17 +838,20 @@ int ima_post_load_data(char *buf, loff_t size,
* @pcr: pcr to extend the measurement
* @func_data: func specific data, may be NULL
* @buf_hash: measure buffer data hash
+ * @digest: buffer digest will be written to
+ * @digest_len: buffer length
*
* Based on policy, either the buffer data or buffer data hash is measured
*
- * Return: 0 if the buffer has been successfully measured, a negative value
- * otherwise.
+ * Return: 0 if the buffer has been successfully measured, 1 if the digest
+ * has been written to the passed location but not added to a measurement entry,
+ * a negative value otherwise.
*/
int process_buffer_measurement(struct user_namespace *mnt_userns,
struct inode *inode, const void *buf, int size,
const char *eventname, enum ima_hooks func,
int pcr, const char *func_data,
- bool buf_hash)
+ bool buf_hash, u8 *digest, size_t digest_len)
{
int ret = 0;
const char *audit_cause = "ENOMEM";
@@ -869,7 +872,10 @@ int process_buffer_measurement(struct user_namespace *mnt_userns,
int action = 0;
u32 secid;
- if (!ima_policy_flag)
+ if (digest && digest_len < digest_hash_len)
+ return -EINVAL;
+
+ if (!ima_policy_flag && !digest)
return -ENOENT;
template = ima_template_desc_buf();
@@ -891,7 +897,7 @@ int process_buffer_measurement(struct user_namespace *mnt_userns,
action = ima_get_action(mnt_userns, inode, current_cred(),
secid, 0, func, &pcr, &template,
func_data);
- if (!(action & IMA_MEASURE))
+ if (!(action & IMA_MEASURE) && !digest)
return -ENOENT;
}
@@ -922,6 +928,12 @@ int process_buffer_measurement(struct user_namespace *mnt_userns,
event_data.buf_len = digest_hash_len;
}
+ if (digest)
+ memcpy(digest, iint.ima_hash->digest, digest_hash_len);
+
+ if (!ima_policy_flag || (func && !(action & IMA_MEASURE)))
+ return 1;
+
ret = ima_alloc_init_template(&event_data, &entry, template);
if (ret < 0) {
audit_cause = "alloc_entry";
@@ -966,7 +978,7 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
ret = process_buffer_measurement(file_mnt_user_ns(f.file),
file_inode(f.file), buf, size,
"kexec-cmdline", KEXEC_CMDLINE, 0,
- NULL, false);
+ NULL, false, NULL, 0);
fdput(f);
}
@@ -977,26 +989,30 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
* @buf: pointer to buffer data
* @buf_len: length of buffer data (in bytes)
* @hash: measure buffer data hash
+ * @digest: buffer digest will be written to
+ * @digest_len: buffer length
*
* Measure data critical to the integrity of the kernel into the IMA log
* and extend the pcr. Examples of critical data could be various data
* structures, policies, and states stored in kernel memory that can
* impact the integrity of the system.
*
- * Return: 0 if the buffer has been successfully measured, a negative value
- * otherwise.
+ * Return: 0 if the buffer has been successfully measured, 1 if the digest
+ * has been written to the passed location but not added to a measurement entry,
+ * a negative value otherwise.
*/
int ima_measure_critical_data(const char *event_label,
const char *event_name,
const void *buf, size_t buf_len,
- bool hash)
+ bool hash, u8 *digest, size_t digest_len)
{
if (!event_name || !event_label || !buf || !buf_len)
return -ENOPARAM;
return process_buffer_measurement(&init_user_ns, NULL, buf, buf_len,
event_name, CRITICAL_DATA, 0,
- event_label, hash);
+ event_label, hash, digest,
+ digest_len);
}
static int __init init_ima(void)
diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c
index e3047ce64f39..b02b061c5fac 100644
--- a/security/integrity/ima/ima_queue_keys.c
+++ b/security/integrity/ima/ima_queue_keys.c
@@ -166,7 +166,7 @@ void ima_process_queued_keys(void)
entry->keyring_name,
KEY_CHECK, 0,
entry->keyring_name,
- false);
+ false, NULL, 0);
list_del(&entry->list);
ima_free_key_entry(entry);
}
diff --git a/security/selinux/ima.c b/security/selinux/ima.c
index 4db9fa211638..d5d7b3ca9651 100644
--- a/security/selinux/ima.c
+++ b/security/selinux/ima.c
@@ -88,7 +88,7 @@ void selinux_ima_measure_state_locked(struct selinux_state *state)
measure_rc = ima_measure_critical_data("selinux", "selinux-state",
state_str, strlen(state_str),
- false);
+ false, NULL, 0);
kfree(state_str);
@@ -105,7 +105,8 @@ void selinux_ima_measure_state_locked(struct selinux_state *state)
}
measure_rc = ima_measure_critical_data("selinux", "selinux-policy-hash",
- policy, policy_len, true);
+ policy, policy_len, true,
+ NULL, 0);
vfree(policy);
}
--
2.25.1
^ permalink raw reply related
* [PATCH v2 2/3] ima: Return int in the functions to measure a buffer
From: Roberto Sassu @ 2021-07-01 12:55 UTC (permalink / raw)
To: zohar, paul
Cc: stephen.smalley.work, prsriva02, tusharsu, nramas,
linux-integrity, linux-security-module, linux-kernel, selinux,
Roberto Sassu
In-Reply-To: <20210701125552.2958008-1-roberto.sassu@huawei.com>
ima_measure_critical_data() and process_buffer_measurement() currently
don't return a result. A caller wouldn't be able to know whether those
functions were executed successfully.
This patch modifies the return type from void to int, and returns 0 if the
buffer has been successfully measured, a negative value otherwise.
Also, this patch does not modify the behavior of existing callers by
processing the returned value. Instead, the value is stored in a variable
marked as __maybe_unused.
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
include/linux/ima.h | 15 +++---
security/integrity/ima/ima.h | 10 ++--
security/integrity/ima/ima_appraise.c | 4 +-
security/integrity/ima/ima_asymmetric_keys.c | 4 +-
security/integrity/ima/ima_init.c | 6 ++-
security/integrity/ima/ima_main.c | 48 ++++++++++++--------
security/integrity/ima/ima_queue_keys.c | 15 +++---
security/selinux/ima.c | 10 ++--
8 files changed, 66 insertions(+), 46 deletions(-)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 81e830d01ced..60492263aa64 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -35,10 +35,10 @@ extern void ima_post_path_mknod(struct user_namespace *mnt_userns,
extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
extern int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size);
extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
-extern void ima_measure_critical_data(const char *event_label,
- const char *event_name,
- const void *buf, size_t buf_len,
- bool hash);
+extern int ima_measure_critical_data(const char *event_label,
+ const char *event_name,
+ const void *buf, size_t buf_len,
+ bool hash);
#ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
extern void ima_appraise_parse_cmdline(void);
@@ -144,10 +144,13 @@ static inline int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size
static inline void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) {}
-static inline void ima_measure_critical_data(const char *event_label,
+static inline int ima_measure_critical_data(const char *event_label,
const char *event_name,
const void *buf, size_t buf_len,
- bool hash) {}
+ bool hash)
+{
+ return -ENOENT;
+}
#endif /* CONFIG_IMA */
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index f0e448ed1f9f..03db221324c3 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -264,11 +264,11 @@ void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
struct evm_ima_xattr_data *xattr_value,
int xattr_len, const struct modsig *modsig, int pcr,
struct ima_template_desc *template_desc);
-void process_buffer_measurement(struct user_namespace *mnt_userns,
- struct inode *inode, const void *buf, int size,
- const char *eventname, enum ima_hooks func,
- int pcr, const char *func_data,
- bool buf_hash);
+int process_buffer_measurement(struct user_namespace *mnt_userns,
+ struct inode *inode, const void *buf, int size,
+ const char *eventname, enum ima_hooks func,
+ int pcr, const char *func_data,
+ bool buf_hash);
void ima_audit_measurement(struct integrity_iint_cache *iint,
const unsigned char *filename);
int ima_alloc_init_template(struct ima_event_data *event_data,
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index ef9dcfce45d4..275a2377743f 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -345,6 +345,7 @@ int ima_check_blacklist(struct integrity_iint_cache *iint,
enum hash_algo hash_algo;
const u8 *digest = NULL;
u32 digestsize = 0;
+ int process_rc __maybe_unused;
int rc = 0;
if (!(iint->flags & IMA_CHECK_BLACKLIST))
@@ -355,7 +356,8 @@ int ima_check_blacklist(struct integrity_iint_cache *iint,
rc = is_binary_blacklisted(digest, digestsize);
if ((rc == -EPERM) && (iint->flags & IMA_MEASURE))
- process_buffer_measurement(&init_user_ns, NULL, digest, digestsize,
+ process_rc = process_buffer_measurement(&init_user_ns,
+ NULL, digest, digestsize,
"blacklisted-hash", NONE,
pcr, NULL, false);
}
diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
index c985418698a4..910367cdd920 100644
--- a/security/integrity/ima/ima_asymmetric_keys.c
+++ b/security/integrity/ima/ima_asymmetric_keys.c
@@ -31,6 +31,7 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
unsigned long flags, bool create)
{
bool queued = false;
+ int ret __maybe_unused;
/* Only asymmetric keys are handled by this hook. */
if (key->type != &key_type_asymmetric)
@@ -60,7 +61,8 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
* if the IMA policy is configured to measure a key linked
* to the given keyring.
*/
- process_buffer_measurement(&init_user_ns, NULL, payload, payload_len,
+ ret = process_buffer_measurement(&init_user_ns, NULL,
+ payload, payload_len,
keyring->description, KEY_CHECK, 0,
keyring->description, false);
}
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 5076a7d9d23e..6790eea88db8 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -118,6 +118,7 @@ void __init ima_load_x509(void)
int __init ima_init(void)
{
+ int measure_rc __maybe_unused;
int rc;
ima_tpm_chip = tpm_default_chip();
@@ -153,8 +154,9 @@ int __init ima_init(void)
ima_init_key_queue();
- ima_measure_critical_data("kernel_info", "kernel_version",
- UTS_RELEASE, strlen(UTS_RELEASE), false);
+ measure_rc = ima_measure_critical_data("kernel_info", "kernel_version",
+ UTS_RELEASE, strlen(UTS_RELEASE),
+ false);
return rc;
}
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 8ef1fa357e0c..3386e7436440 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -827,7 +827,7 @@ int ima_post_load_data(char *buf, loff_t size,
return 0;
}
-/*
+/**
* process_buffer_measurement - Measure the buffer or the buffer data hash
* @mnt_userns: user namespace of the mount the inode was found from
* @inode: inode associated with the object being measured (NULL for KEY_CHECK)
@@ -840,12 +840,15 @@ int ima_post_load_data(char *buf, loff_t size,
* @buf_hash: measure buffer data hash
*
* Based on policy, either the buffer data or buffer data hash is measured
+ *
+ * Return: 0 if the buffer has been successfully measured, a negative value
+ * otherwise.
*/
-void process_buffer_measurement(struct user_namespace *mnt_userns,
- struct inode *inode, const void *buf, int size,
- const char *eventname, enum ima_hooks func,
- int pcr, const char *func_data,
- bool buf_hash)
+int process_buffer_measurement(struct user_namespace *mnt_userns,
+ struct inode *inode, const void *buf, int size,
+ const char *eventname, enum ima_hooks func,
+ int pcr, const char *func_data,
+ bool buf_hash)
{
int ret = 0;
const char *audit_cause = "ENOMEM";
@@ -867,7 +870,7 @@ void process_buffer_measurement(struct user_namespace *mnt_userns,
u32 secid;
if (!ima_policy_flag)
- return;
+ return -ENOENT;
template = ima_template_desc_buf();
if (!template) {
@@ -889,7 +892,7 @@ void process_buffer_measurement(struct user_namespace *mnt_userns,
secid, 0, func, &pcr, &template,
func_data);
if (!(action & IMA_MEASURE))
- return;
+ return -ENOENT;
}
if (!pcr)
@@ -937,7 +940,7 @@ void process_buffer_measurement(struct user_namespace *mnt_userns,
func_measure_str(func),
audit_cause, ret, 0, ret);
- return;
+ return ret;
}
/**
@@ -951,6 +954,7 @@ void process_buffer_measurement(struct user_namespace *mnt_userns,
void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
{
struct fd f;
+ int ret __maybe_unused;
if (!buf || !size)
return;
@@ -959,9 +963,10 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
if (!f.file)
return;
- process_buffer_measurement(file_mnt_user_ns(f.file), file_inode(f.file),
- buf, size, "kexec-cmdline", KEXEC_CMDLINE, 0,
- NULL, false);
+ ret = process_buffer_measurement(file_mnt_user_ns(f.file),
+ file_inode(f.file), buf, size,
+ "kexec-cmdline", KEXEC_CMDLINE, 0,
+ NULL, false);
fdput(f);
}
@@ -977,18 +982,21 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
* and extend the pcr. Examples of critical data could be various data
* structures, policies, and states stored in kernel memory that can
* impact the integrity of the system.
+ *
+ * Return: 0 if the buffer has been successfully measured, a negative value
+ * otherwise.
*/
-void ima_measure_critical_data(const char *event_label,
- const char *event_name,
- const void *buf, size_t buf_len,
- bool hash)
+int ima_measure_critical_data(const char *event_label,
+ const char *event_name,
+ const void *buf, size_t buf_len,
+ bool hash)
{
if (!event_name || !event_label || !buf || !buf_len)
- return;
+ return -ENOPARAM;
- process_buffer_measurement(&init_user_ns, NULL, buf, buf_len, event_name,
- CRITICAL_DATA, 0, event_label,
- hash);
+ return process_buffer_measurement(&init_user_ns, NULL, buf, buf_len,
+ event_name, CRITICAL_DATA, 0,
+ event_label, hash);
}
static int __init init_ima(void)
diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c
index 979ef6c71f3d..e3047ce64f39 100644
--- a/security/integrity/ima/ima_queue_keys.c
+++ b/security/integrity/ima/ima_queue_keys.c
@@ -134,6 +134,7 @@ void ima_process_queued_keys(void)
{
struct ima_key_entry *entry, *tmp;
bool process = false;
+ int ret __maybe_unused;
if (ima_process_keys)
return;
@@ -159,13 +160,13 @@ void ima_process_queued_keys(void)
list_for_each_entry_safe(entry, tmp, &ima_keys, list) {
if (!timer_expired)
- process_buffer_measurement(&init_user_ns, NULL,
- entry->payload,
- entry->payload_len,
- entry->keyring_name,
- KEY_CHECK, 0,
- entry->keyring_name,
- false);
+ ret = process_buffer_measurement(&init_user_ns, NULL,
+ entry->payload,
+ entry->payload_len,
+ entry->keyring_name,
+ KEY_CHECK, 0,
+ entry->keyring_name,
+ false);
list_del(&entry->list);
ima_free_key_entry(entry);
}
diff --git a/security/selinux/ima.c b/security/selinux/ima.c
index 34d421861bfc..4db9fa211638 100644
--- a/security/selinux/ima.c
+++ b/security/selinux/ima.c
@@ -75,6 +75,7 @@ void selinux_ima_measure_state_locked(struct selinux_state *state)
char *state_str = NULL;
void *policy = NULL;
size_t policy_len;
+ int measure_rc __maybe_unused;
int rc = 0;
WARN_ON(!mutex_is_locked(&state->policy_mutex));
@@ -85,8 +86,9 @@ void selinux_ima_measure_state_locked(struct selinux_state *state)
return;
}
- ima_measure_critical_data("selinux", "selinux-state",
- state_str, strlen(state_str), false);
+ measure_rc = ima_measure_critical_data("selinux", "selinux-state",
+ state_str, strlen(state_str),
+ false);
kfree(state_str);
@@ -102,8 +104,8 @@ void selinux_ima_measure_state_locked(struct selinux_state *state)
return;
}
- ima_measure_critical_data("selinux", "selinux-policy-hash",
- policy, policy_len, true);
+ measure_rc = ima_measure_critical_data("selinux", "selinux-policy-hash",
+ policy, policy_len, true);
vfree(policy);
}
--
2.25.1
^ permalink raw reply related
* [PATCH v2 0/3] ima: Provide more info about buffer measurement
From: Roberto Sassu @ 2021-07-01 12:55 UTC (permalink / raw)
To: zohar, paul
Cc: stephen.smalley.work, prsriva02, tusharsu, nramas,
linux-integrity, linux-security-module, linux-kernel, selinux,
Roberto Sassu
This patch set provides more information about buffer measurement.
First, it introduces the new function ima_get_current_hash_algo(), to
obtain the algorithm used to calculate the buffer digest (patch 1).
Second, it changes the type of return value of ima_measure_critical_data()
and process_buffer_measurement() from void to int, to signal to the callers
whether or not the buffer has been measured, or just the digest has been
calculated and written to the supplied location (patch 2).
Lastly, it adds two new parameters to the functions above ('digest' and
'digest_len'), so that those functions can write the buffer digest to the
location supplied by the callers (patch 3).
This patch set replaces the patch 'ima: Add digest, algo, measured
parameters to ima_measure_critical_data()' in:
https://lore.kernel.org/linux-integrity/20210625165614.2284243-1-roberto.sassu@huawei.com/
Changelog
v1:
- add digest_len parameter to ima_measure_critical_data() and
process_buffer_measurement() (suggested by Lakshmi)
- fix doc formatting issues
Huawei Digest Lists patch set:
- introduce ima_get_current_hash_algo() (suggested by Mimi)
- remove algo and measured parameters from ima_measure_critical_data() and
process_buffer_measurement() (suggested by Mimi)
- return an integer from ima_measure_critical_data() and
process_buffer_measurement() (suggested by Mimi)
- correctly check when process_buffer_measurement() should return earlier
Roberto Sassu (3):
ima: Introduce ima_get_current_hash_algo()
ima: Return int in the functions to measure a buffer
ima: Add digest and digest_len params to the functions to measure a
buffer
include/linux/ima.h | 23 ++++--
security/integrity/ima/ima.h | 10 +--
security/integrity/ima/ima_appraise.c | 6 +-
security/integrity/ima/ima_asymmetric_keys.c | 6 +-
security/integrity/ima/ima_init.c | 6 +-
security/integrity/ima/ima_main.c | 73 ++++++++++++++------
security/integrity/ima/ima_queue_keys.c | 15 ++--
security/selinux/ima.c | 11 +--
8 files changed, 100 insertions(+), 50 deletions(-)
--
2.25.1
^ permalink raw reply
* [PATCH v2 1/3] ima: Introduce ima_get_current_hash_algo()
From: Roberto Sassu @ 2021-07-01 12:55 UTC (permalink / raw)
To: zohar, paul
Cc: stephen.smalley.work, prsriva02, tusharsu, nramas,
linux-integrity, linux-security-module, linux-kernel, selinux,
Roberto Sassu
In-Reply-To: <20210701125552.2958008-1-roberto.sassu@huawei.com>
This patch introduces the new function ima_get_current_hash_algo(), that
callers in the other kernel subsystems might use to obtain the hash
algorithm selected by IMA.
Its primary use will be to determine which algorithm has been used to
calculate the digest written by ima_measure_critical_data() to the location
passed as a new parameter (in a subsequent patch).
Since the hash algorithm does not change after the IMA setup phase, there
is no risk of races (obtaining a digest calculated with a different
algorithm than the one returned).
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
include/linux/ima.h | 7 +++++++
security/integrity/ima/ima_main.c | 5 +++++
2 files changed, 12 insertions(+)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 61d5723ec303..81e830d01ced 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -11,9 +11,11 @@
#include <linux/fs.h>
#include <linux/security.h>
#include <linux/kexec.h>
+#include <crypto/hash_info.h>
struct linux_binprm;
#ifdef CONFIG_IMA
+extern enum hash_algo ima_get_current_hash_algo(void);
extern int ima_bprm_check(struct linux_binprm *bprm);
extern int ima_file_check(struct file *file, int mask);
extern void ima_post_create_tmpfile(struct user_namespace *mnt_userns,
@@ -64,6 +66,11 @@ static inline const char * const *arch_get_ima_policy(void)
#endif
#else
+static inline enum hash_algo ima_get_current_hash_algo(void)
+{
+ return HASH_ALGO__LAST;
+}
+
static inline int ima_bprm_check(struct linux_binprm *bprm)
{
return 0;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 287b90509006..8ef1fa357e0c 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -76,6 +76,11 @@ static int __init hash_setup(char *str)
}
__setup("ima_hash=", hash_setup);
+enum hash_algo ima_get_current_hash_algo(void)
+{
+ return ima_hash_algo;
+}
+
/* Prevent mmap'ing a file execute that is already mmap'ed write */
static int mmap_violation_check(enum ima_hooks func, struct file *file,
char **pathbuf, const char **pathname,
--
2.25.1
^ permalink raw reply related
* Re: [RFC PATCH 0/1] xattr: Allow user.* xattr on symlink/special files if caller has CAP_SYS_RESOURCE
From: Vivek Goyal @ 2021-07-01 12:21 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Theodore Ts'o, Daniel Walsh, Casey Schaufler,
Schaufler, Casey, linux-fsdevel@vger.kernel.org,
linux-kernel@vger.kernel.org, viro@zeniv.linux.org.uk,
virtio-fs@redhat.com, berrange@redhat.com, linux-security-module,
selinux@vger.kernel.org
In-Reply-To: <YN2BYXv79PswrN2E@work-vm>
On Thu, Jul 01, 2021 at 09:48:33AM +0100, Dr. David Alan Gilbert wrote:
> * Theodore Ts'o (tytso@mit.edu) wrote:
> > On Wed, Jun 30, 2021 at 04:01:42PM +0100, Dr. David Alan Gilbert wrote:
> > >
> > > Even if you fix symlinks, I don't think it fixes device nodes or
> > > anything else where the permissions bitmap isn't purely used as the
> > > permissions on the inode.
> >
> > I think we're making a mountain out of a molehill. Again, very few
> > people are using quota these days. And if you give someone write
> > access to a 8TB disk, do you really care if they can "steal" 32k worth
> > of space (which is the maximum size of an xattr, enforced by the VFS).
> >
> > OK, but what about character mode devices? First of all, most users
> > don't have access to huge number of devices, but let's assume
> > something absurd. Let's say that a user has write access to *1024*
> > devices. (My /dev has 233 character mode devices, and I have write
> > access to well under a dozen.)
> >
> > An 8TB disk costs about $200. So how much of the "stolen" quota space
> > are we talking about, assuming the user has access to 1024 devices,
> > and the file system actually supports a 32k xattr.
> >
> > 32k * 1024 * $200 / 8TB / (1024*1024*1024) = $0.000763 = 0.0763 cents
> >
> > A 2TB SSD is less around $180, so even if we calculate the prices
> > based on SSD space, we're still talking about a quarter of a penny.
> >
> > Why are we worrying about this?
>
> I'm not worrying about storage cost, but we would need to define what
> the rules are on who can write and change a user.* xattr on a device
> node. It doesn't feel sane to make it anyone who can write to the
> device; then everyone can start leaving droppings on /dev/null.
Looks like tmpfs/devtmpfs might not support setting user.* xattrs. So
devices nodes there should not be a problem.
# touch /dev/foo.txt
# setfattr -n "user.foo" -v "bar" /dev/foo.txt
setfattr: /dev/foo.txt: Operation not supported
Vivek
>
> The other evilness I can imagine, is if there's a 32k limit on xattrs on
> a node, an evil user could write almost 32k of junk to the node
> and then break the next login that tries to add an acl or breaks the
> next relabel.
>
> Dave
>
> > - Ted
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
^ permalink raw reply
* Re: [RFC PATCH 0/1] xattr: Allow user.* xattr on symlink/special files if caller has CAP_SYS_RESOURCE
From: Dr. David Alan Gilbert @ 2021-07-01 8:48 UTC (permalink / raw)
To: Theodore Ts'o
Cc: Daniel Walsh, Vivek Goyal, Casey Schaufler, Schaufler, Casey,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
viro@zeniv.linux.org.uk, virtio-fs@redhat.com,
berrange@redhat.com, linux-security-module,
selinux@vger.kernel.org
In-Reply-To: <YNzNLTxflKbDi8W2@mit.edu>
* Theodore Ts'o (tytso@mit.edu) wrote:
> On Wed, Jun 30, 2021 at 04:01:42PM +0100, Dr. David Alan Gilbert wrote:
> >
> > Even if you fix symlinks, I don't think it fixes device nodes or
> > anything else where the permissions bitmap isn't purely used as the
> > permissions on the inode.
>
> I think we're making a mountain out of a molehill. Again, very few
> people are using quota these days. And if you give someone write
> access to a 8TB disk, do you really care if they can "steal" 32k worth
> of space (which is the maximum size of an xattr, enforced by the VFS).
>
> OK, but what about character mode devices? First of all, most users
> don't have access to huge number of devices, but let's assume
> something absurd. Let's say that a user has write access to *1024*
> devices. (My /dev has 233 character mode devices, and I have write
> access to well under a dozen.)
>
> An 8TB disk costs about $200. So how much of the "stolen" quota space
> are we talking about, assuming the user has access to 1024 devices,
> and the file system actually supports a 32k xattr.
>
> 32k * 1024 * $200 / 8TB / (1024*1024*1024) = $0.000763 = 0.0763 cents
>
> A 2TB SSD is less around $180, so even if we calculate the prices
> based on SSD space, we're still talking about a quarter of a penny.
>
> Why are we worrying about this?
I'm not worrying about storage cost, but we would need to define what
the rules are on who can write and change a user.* xattr on a device
node. It doesn't feel sane to make it anyone who can write to the
device; then everyone can start leaving droppings on /dev/null.
The other evilness I can imagine, is if there's a 32k limit on xattrs on
a node, an evil user could write almost 32k of junk to the node
and then break the next login that tries to add an acl or breaks the
next relabel.
Dave
> - Ted
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply
* [PATCH v2] perf: Require CAP_KILL if sigtrap is requested
From: Marco Elver @ 2021-07-01 8:38 UTC (permalink / raw)
To: elver, peterz
Cc: tglx, mingo, kasan-dev, linux-kernel, mingo, acme, mark.rutland,
alexander.shishkin, jolsa, namhyung, linux-perf-users, ebiederm,
omosnace, serge, linux-security-module, stable, Dmitry Vyukov
If perf_event_open() is called with another task as target and
perf_event_attr::sigtrap is set, and the target task's user does not
match the calling user, also require the CAP_KILL capability.
Otherwise, with the CAP_PERFMON capability alone it would be possible
for a user to send SIGTRAP signals via perf events to another user's
tasks. This could potentially result in those tasks being terminated if
they cannot handle SIGTRAP signals.
Note: The check complements the existing capability check, but is not
supposed to supersede the ptrace_may_access() check. At a high level we
now have:
capable of CAP_PERFMON and (CAP_KILL if sigtrap)
OR
ptrace_may_access() // also checks for same thread-group and uid
Fixes: 97ba62b27867 ("perf: Add support for SIGTRAP on perf events")
Cc: <stable@vger.kernel.org> # 5.13+
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Marco Elver <elver@google.com>
---
v2:
* Drop kill_capable() and just check CAP_KILL (reported by Ondrej Mosnacek).
* Use ns_capable(__task_cred(task)->user_ns, CAP_KILL) to check for
capability in target task's ns (reported by Ondrej Mosnacek).
---
kernel/events/core.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index fe88d6eea3c2..43c99695dc3f 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -12152,10 +12152,23 @@ SYSCALL_DEFINE5(perf_event_open,
}
if (task) {
+ bool is_capable;
+
err = down_read_interruptible(&task->signal->exec_update_lock);
if (err)
goto err_file;
+ is_capable = perfmon_capable();
+ if (attr.sigtrap) {
+ /*
+ * perf_event_attr::sigtrap sends signals to the other
+ * task. Require the current task to have CAP_KILL.
+ */
+ rcu_read_lock();
+ is_capable &= ns_capable(__task_cred(task)->user_ns, CAP_KILL);
+ rcu_read_unlock();
+ }
+
/*
* Preserve ptrace permission check for backwards compatibility.
*
@@ -12165,7 +12178,7 @@ SYSCALL_DEFINE5(perf_event_open,
* perf_event_exit_task() that could imply).
*/
err = -EACCES;
- if (!perfmon_capable() && !ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS))
+ if (!is_capable && !ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS))
goto err_cred;
}
--
2.32.0.93.g670b81a890-goog
^ permalink raw reply related
* Re: [PATCH] perf: Require CAP_KILL if sigtrap is requested
From: Ondrej Mosnacek @ 2021-07-01 7:56 UTC (permalink / raw)
To: Marco Elver
Cc: Peter Zijlstra, Thomas Gleixner, Ingo Molnar, kasan-dev,
Linux kernel mailing list, Serge E. Hallyn, Ingo Molnar,
Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, Linux Security Module list,
linux-perf-users, Eric Biederman, Dmitry Vyukov
In-Reply-To: <YNxmyRYcs/R/8zry@elver.google.com>
On Wed, Jun 30, 2021 at 2:43 PM Marco Elver <elver@google.com> wrote:
> On Wed, Jun 30, 2021 at 01:13PM +0200, Ondrej Mosnacek wrote:
> > On Wed, Jun 30, 2021 at 11:38 AM Marco Elver <elver@google.com> wrote:
> [...]
> > > +static inline bool kill_capable(void)
> > > +{
> > > + return capable(CAP_KILL) || capable(CAP_SYS_ADMIN);
> >
> > Is it really necessary to fall back to CAP_SYS_ADMIN here? CAP_PERFMON
> > and CAP_BPF have been split off from CAP_SYS_ADMIN recently, so they
> > have it for backwards compatibility. You are adding a new restriction
> > for a very specific action, so I don't think the fallback is needed.
>
> That means someone having CAP_SYS_ADMIN, but not CAP_KILL, can't perform
> the desired action. Is this what you'd like?
AFAIK, such user wouldn't be allowed to directly send a signal to a
different process either. So I think it makes more sense to be
consistent with the existing/main CAP_KILL usage rather than with the
CAP_PERFMON usage (which has its own reason to have that fallback).
I'm not the authority on capabilities nor the perf subsystem, it just
didn't seem quite right to me so I wanted to raise the concern.
Hopefully someone wiser than me will speak up if I talk nonsense :)
> If so, I'll just remove the wrapper, and call capable(CAP_KILL)
> directly.
>
> > > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > > index fe88d6eea3c2..1ab4bc867531 100644
> > > --- a/kernel/events/core.c
> > > +++ b/kernel/events/core.c
> > > @@ -12152,10 +12152,21 @@ SYSCALL_DEFINE5(perf_event_open,
> > > }
> > >
> > > if (task) {
> > > + bool is_capable;
> > > +
> > > err = down_read_interruptible(&task->signal->exec_update_lock);
> > > if (err)
> > > goto err_file;
> > >
> > > + is_capable = perfmon_capable();
> > > + if (attr.sigtrap) {
> > > + /*
> > > + * perf_event_attr::sigtrap sends signals to the other
> > > + * task. Require the current task to have CAP_KILL.
> > > + */
> > > + is_capable &= kill_capable();
> >
> > Is it necessary to do all this dance just to call perfmon_capable()
> > first? Couldn't this be simply:
> >
> > err = -EPERM;
> > if (attr.sigtrap && !capable(CAP_KILL))
> > goto err_cred;
>
> Not so much about perfmon_capable() but about the ptrace_may_access()
> check. The condition here is supposed to be:
>
> want CAP_PERFMON and (CAP_KILL if sigtrap)
> OR
> want ptrace access (which includes a check for same thread-group and uid)
>
> If we did what you propose, then the ptrace check is effectively ignored
> if attr.sigtrap, and that's not what we want.
>
> There are lots of other ways of writing the same thing, but it should
> also remain readable and sticking it all into the same condition is not
> readable.
Ah, I see, I missed that semantic difference... So ptrace_may_access()
implies that the process doesn't need CAP_KILL to send a signal to the
task, that makes sense.
In that case I'm fine with this part as it is.
> > Also, looking at kill_ok_by_cred() in kernel/signal.c, would it
> > perhaps be more appropriate to do
> > ns_capable(__task_cred(task)->user_ns, CAP_KILL) instead? (There might
> > also need to be some careful locking around getting the target task's
> > creds - I'm not sure...)
>
> That might make sense. AFAIK, the locking is already in place via
> exec_update_lock. Let me investigate.
>
> > > + }
> > > +
> > > /*
> > > * Preserve ptrace permission check for backwards compatibility.
> > > *
> > > @@ -12165,7 +12176,7 @@ SYSCALL_DEFINE5(perf_event_open,
> > > * perf_event_exit_task() that could imply).
> > > */
> > > err = -EACCES;
> >
> > BTW, shouldn't this (and several other such cases in this file...)
> > actually be EPERM, as is the norm for capability checks?
>
> I'm not a perf maintainer, so I can't give you a definitive answer.
> But, this would change the ABI, so I don't think it's realistic to
> request this change at this point unfortunately.
Indeed... I worry it will make troubleshooting SELinux/capability
errors more confusing, but I agree it would be a potentially risky
change to fix it :/
--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
^ permalink raw reply
* [PATCH v1 0/4] Landlock filesystem caching
From: Mickaël Salaün @ 2021-06-30 22:48 UTC (permalink / raw)
To: Al Viro, James Morris, Serge Hallyn
Cc: Mickaël Salaün, Andy Lutomirski, Jann Horn, Kees Cook,
Shuah Khan, linux-fsdevel, linux-kernel, linux-kselftest,
linux-security-module
Hi,
The goal of this patch series is to reduce the performance impact of
walking through a lot of files while being landlocked. Indeed, because
of the unprivileged nature of Landlock, each file access implies to
check access granted to each directory of the path, which slows down
open time.
Currently, openat(2) calls spend more than 22% of their time in
hook_file_open(). The performance impact for a common worth case
scenario is significantly reduced thanks to this patch series,
theoretically going from O(n) with n as the depth of a path, to O(1)
(cf. benchmarks in the caching patch).
This series adds a new security hook (resolve_path_at) and uses it to
implement access caching in Landlock. I'm planning to build on top of
that for other improvements (using task's working directory and task's
root directory) but that will require other hook changes.
This new hook is also a first step to be able to securely restrict file
descriptors used for path resolution (e.g. dirfd in openat2).
Caching may be difficult to get right especially for security checks. I
extended the current tests and I'm still working on new ones. If you
have test/attack scenarios, please share them. I would really
appreciate constructive reviews for these critical changes. This series
can be applied on top of v5.13 .
Regards,
Mickaël Salaün (4):
fs,security: Add resolve_path_at() hook
landlock: Add filesystem rule caching
selftests/landlock: Work in a temporary directory
selftests/landlock: Check all possible intermediate directories
fs/namei.c | 9 +
include/linux/lsm_hook_defs.h | 2 +
include/linux/lsm_hooks.h | 8 +
include/linux/security.h | 9 +
security/landlock/cache.h | 77 +++++++
security/landlock/cred.c | 15 +-
security/landlock/cred.h | 20 +-
security/landlock/fs.c | 224 +++++++++++++++++++--
security/landlock/fs.h | 29 +++
security/landlock/setup.c | 2 +
security/security.c | 6 +
tools/testing/selftests/landlock/fs_test.c | 205 ++++++++++++++-----
12 files changed, 544 insertions(+), 62 deletions(-)
create mode 100644 security/landlock/cache.h
base-commit: 62fb9874f5da54fdb243003b386128037319b219
--
2.32.0
^ permalink raw reply
* [PATCH v1 1/4] fs,security: Add resolve_path_at() hook
From: Mickaël Salaün @ 2021-06-30 22:48 UTC (permalink / raw)
To: Al Viro, James Morris, Serge Hallyn
Cc: Mickaël Salaün, Andy Lutomirski, Jann Horn, Kees Cook,
Shuah Khan, linux-fsdevel, linux-kernel, linux-kselftest,
linux-security-module, Mickaël Salaün
In-Reply-To: <20210630224856.1313928-1-mic@digikod.net>
From: Mickaël Salaün <mic@linux.microsoft.com>
This new resolve_path_at() security hook enables checking absolute and
relative pathname resolutions according to the task's root directory,
the task's working directory, or an open file descriptor (e.g. thanks to
*at() syscalls).
It is useful to (efficiently) restrict access to file hierarchies (e.g.
leveraging openat2()'s resolve flags) according to file descriptors, and
to improve path access check time, which is used by Landlock in the
following commit.
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: James Morris <jmorris@namei.org>
Cc: Jann Horn <jannh@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
Link: https://lore.kernel.org/r/20210630224856.1313928-2-mic@digikod.net
---
fs/namei.c | 9 +++++++++
include/linux/lsm_hook_defs.h | 2 ++
include/linux/lsm_hooks.h | 8 ++++++++
include/linux/security.h | 9 +++++++++
security/security.c | 6 ++++++
5 files changed, 34 insertions(+)
diff --git a/fs/namei.c b/fs/namei.c
index 79b0ff9b151e..6efdd0b8e2b1 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2331,6 +2331,9 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
error = nd_jump_root(nd);
if (unlikely(error))
return ERR_PTR(error);
+ error = security_resolve_path_at(&nd->path, NULL, flags);
+ if (error)
+ return ERR_PTR(error);
return s;
}
@@ -2350,6 +2353,9 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
get_fs_pwd(current->fs, &nd->path);
nd->inode = nd->path.dentry->d_inode;
}
+ error = security_resolve_path_at(&nd->path, NULL, flags);
+ if (error)
+ return ERR_PTR(error);
} else {
/* Caller must check execute permissions on the starting path component */
struct fd f = fdget_raw(nd->dfd);
@@ -2373,7 +2379,10 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
path_get(&nd->path);
nd->inode = nd->path.dentry->d_inode;
}
+ error = security_resolve_path_at(&nd->path, f.file, flags);
fdput(f);
+ if (error)
+ return ERR_PTR(error);
}
/* For scoped-lookups we need to set the root to the dirfd as well. */
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 04c01794de83..1db6d53b1dfb 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -178,6 +178,8 @@ LSM_HOOK(int, 0, file_send_sigiotask, struct task_struct *tsk,
struct fown_struct *fown, int sig)
LSM_HOOK(int, 0, file_receive, struct file *file)
LSM_HOOK(int, 0, file_open, struct file *file)
+LSM_HOOK(int, 0, resolve_path_at, const struct path *path_at,
+ struct file *file_at, int lookup_flags)
LSM_HOOK(int, 0, task_alloc, struct task_struct *task,
unsigned long clone_flags)
LSM_HOOK(void, LSM_RET_VOID, task_free, struct task_struct *task)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 5c4c5c0602cb..2ef130e36309 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -600,6 +600,14 @@
* Save open-time permission checking state for later use upon
* file_permission, and recheck access if anything has changed
* since inode_permission.
+ * @resolve_path_at:
+ * Check path resolution from a file descriptor, or the current working
+ * directory, or the current root directory.
+ * Can be called in RCU read-side critical section.
+ * @path_at points to the base path.
+ * @file_at can point to the file descriptor used to resolve the path, or
+ * be NULL for AT_FDCWD.
+ * @lookup_flags contains the lookup options (e.g. LOOKUP_IS_SCOPED).
*
* Security hooks for task operations.
*
diff --git a/include/linux/security.h b/include/linux/security.h
index 06f7c50ce77f..e3ae93b9189d 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -391,6 +391,8 @@ int security_file_send_sigiotask(struct task_struct *tsk,
struct fown_struct *fown, int sig);
int security_file_receive(struct file *file);
int security_file_open(struct file *file);
+int security_resolve_path_at(const struct path *path_at, struct file *file_at,
+ int lookup_flags);
int security_task_alloc(struct task_struct *task, unsigned long clone_flags);
void security_task_free(struct task_struct *task);
int security_cred_alloc_blank(struct cred *cred, gfp_t gfp);
@@ -1011,6 +1013,13 @@ static inline int security_file_open(struct file *file)
return 0;
}
+static inline int security_resolve_path_at(const struct path *path_at,
+ struct file *file_at,
+ int lookup_flags)
+{
+ return 0;
+}
+
static inline int security_task_alloc(struct task_struct *task,
unsigned long clone_flags)
{
diff --git a/security/security.c b/security/security.c
index b38155b2de83..98161f5919ee 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1637,6 +1637,12 @@ int security_file_open(struct file *file)
return fsnotify_perm(file, MAY_OPEN);
}
+int security_resolve_path_at(const struct path *path_at, struct file *file_at,
+ int lookup_flags)
+{
+ return call_int_hook(resolve_path_at, 0, path_at, file_at, lookup_flags);
+}
+
int security_task_alloc(struct task_struct *task, unsigned long clone_flags)
{
int rc = lsm_task_alloc(task);
--
2.32.0
^ permalink raw reply related
* [PATCH v1 3/4] selftests/landlock: Work in a temporary directory
From: Mickaël Salaün @ 2021-06-30 22:48 UTC (permalink / raw)
To: Al Viro, James Morris, Serge Hallyn
Cc: Mickaël Salaün, Andy Lutomirski, Jann Horn, Kees Cook,
Shuah Khan, linux-fsdevel, linux-kernel, linux-kselftest,
linux-security-module, Mickaël Salaün
In-Reply-To: <20210630224856.1313928-1-mic@digikod.net>
From: Mickaël Salaün <mic@linux.microsoft.com>
To be able to test the current working directory, run all tests in a
temporary directory instead of in its parent directory. This is
required for the following commit.
Cc: James Morris <jmorris@namei.org>
Cc: Jann Horn <jannh@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Shuah Khan <shuah@kernel.org>
Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
Link: https://lore.kernel.org/r/20210630224856.1313928-4-mic@digikod.net
---
tools/testing/selftests/landlock/fs_test.c | 65 ++++++++++++----------
1 file changed, 35 insertions(+), 30 deletions(-)
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 10c9a1e4ebd9..403c8255311f 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -23,31 +23,31 @@
#include "common.h"
#define TMP_DIR "tmp"
-#define BINARY_PATH "./true"
+#define BINARY_PATH "../true"
/* Paths (sibling number and depth) */
-static const char dir_s1d1[] = TMP_DIR "/s1d1";
-static const char file1_s1d1[] = TMP_DIR "/s1d1/f1";
-static const char file2_s1d1[] = TMP_DIR "/s1d1/f2";
-static const char dir_s1d2[] = TMP_DIR "/s1d1/s1d2";
-static const char file1_s1d2[] = TMP_DIR "/s1d1/s1d2/f1";
-static const char file2_s1d2[] = TMP_DIR "/s1d1/s1d2/f2";
-static const char dir_s1d3[] = TMP_DIR "/s1d1/s1d2/s1d3";
-static const char file1_s1d3[] = TMP_DIR "/s1d1/s1d2/s1d3/f1";
-static const char file2_s1d3[] = TMP_DIR "/s1d1/s1d2/s1d3/f2";
-
-static const char dir_s2d1[] = TMP_DIR "/s2d1";
-static const char file1_s2d1[] = TMP_DIR "/s2d1/f1";
-static const char dir_s2d2[] = TMP_DIR "/s2d1/s2d2";
-static const char file1_s2d2[] = TMP_DIR "/s2d1/s2d2/f1";
-static const char dir_s2d3[] = TMP_DIR "/s2d1/s2d2/s2d3";
-static const char file1_s2d3[] = TMP_DIR "/s2d1/s2d2/s2d3/f1";
-static const char file2_s2d3[] = TMP_DIR "/s2d1/s2d2/s2d3/f2";
-
-static const char dir_s3d1[] = TMP_DIR "/s3d1";
+static const char dir_s1d1[] = "./s1d1";
+static const char file1_s1d1[] = "./s1d1/f1";
+static const char file2_s1d1[] = "./s1d1/f2";
+static const char dir_s1d2[] = "./s1d1/s1d2";
+static const char file1_s1d2[] = "./s1d1/s1d2/f1";
+static const char file2_s1d2[] = "./s1d1/s1d2/f2";
+static const char dir_s1d3[] = "./s1d1/s1d2/s1d3";
+static const char file1_s1d3[] = "./s1d1/s1d2/s1d3/f1";
+static const char file2_s1d3[] = "./s1d1/s1d2/s1d3/f2";
+
+static const char dir_s2d1[] = "./s2d1";
+static const char file1_s2d1[] = "./s2d1/f1";
+static const char dir_s2d2[] = "./s2d1/s2d2";
+static const char file1_s2d2[] = "./s2d1/s2d2/f1";
+static const char dir_s2d3[] = "./s2d1/s2d2/s2d3";
+static const char file1_s2d3[] = "./s2d1/s2d2/s2d3/f1";
+static const char file2_s2d3[] = "./s2d1/s2d2/s2d3/f2";
+
+static const char dir_s3d1[] = "./s3d1";
/* dir_s3d2 is a mount point. */
-static const char dir_s3d2[] = TMP_DIR "/s3d1/s3d2";
-static const char dir_s3d3[] = TMP_DIR "/s3d1/s3d2/s3d3";
+static const char dir_s3d2[] = "./s3d1/s3d2";
+static const char dir_s3d3[] = "./s3d1/s3d2/s3d3";
/*
* layout1 hierarchy:
@@ -140,11 +140,12 @@ static int remove_path(const char *const path)
walker[i] = '\0';
ret = rmdir(walker);
if (ret) {
- if (errno != ENOTEMPTY && errno != EBUSY)
+ if (errno != ENOTEMPTY && errno != EBUSY
+ && errno != EINVAL)
err = errno;
goto out;
}
- if (strcmp(walker, TMP_DIR) == 0)
+ if (strcmp(walker, ".") == 0)
goto out;
}
@@ -168,10 +169,14 @@ static void prepare_layout(struct __test_metadata *const _metadata)
ASSERT_EQ(0, mount("tmp", TMP_DIR, "tmpfs", 0, "size=4m,mode=700"));
ASSERT_EQ(0, mount(NULL, TMP_DIR, NULL, MS_PRIVATE | MS_REC, NULL));
clear_cap(_metadata, CAP_SYS_ADMIN);
+
+ ASSERT_EQ(0, chdir(TMP_DIR));
}
static void cleanup_layout(struct __test_metadata *const _metadata)
{
+ EXPECT_EQ(0, chdir(".."));
+
set_cap(_metadata, CAP_SYS_ADMIN);
EXPECT_EQ(0, umount(TMP_DIR));
clear_cap(_metadata, CAP_SYS_ADMIN);
@@ -1370,7 +1375,7 @@ static void test_relative_path(struct __test_metadata *const _metadata,
*/
const struct rule layer1_base[] = {
{
- .path = TMP_DIR,
+ .path = ".",
.access = ACCESS_RO,
},
{}
@@ -2095,8 +2100,8 @@ FIXTURE_TEARDOWN(layout1_bind)
cleanup_layout(_metadata);
}
-static const char bind_dir_s1d3[] = TMP_DIR "/s2d1/s2d2/s1d3";
-static const char bind_file1_s1d3[] = TMP_DIR "/s2d1/s2d2/s1d3/f1";
+static const char bind_dir_s1d3[] = "./s2d1/s2d2/s1d3";
+static const char bind_file1_s1d3[] = "./s2d1/s2d2/s1d3/f1";
/*
* layout1_bind hierarchy:
@@ -2282,7 +2287,7 @@ TEST_F_FORK(layout1_bind, same_content_same_file)
ASSERT_EQ(EACCES, test_open(bind_file1_s1d3, O_WRONLY));
}
-#define LOWER_BASE TMP_DIR "/lower"
+#define LOWER_BASE "./lower"
#define LOWER_DATA LOWER_BASE "/data"
static const char lower_fl1[] = LOWER_DATA "/fl1";
static const char lower_dl1[] = LOWER_DATA "/dl1";
@@ -2309,7 +2314,7 @@ static const char (*lower_sub_files[])[] = {
NULL
};
-#define UPPER_BASE TMP_DIR "/upper"
+#define UPPER_BASE "./upper"
#define UPPER_DATA UPPER_BASE "/data"
#define UPPER_WORK UPPER_BASE "/work"
static const char upper_fu1[] = UPPER_DATA "/fu1";
@@ -2337,7 +2342,7 @@ static const char (*upper_sub_files[])[] = {
NULL
};
-#define MERGE_BASE TMP_DIR "/merge"
+#define MERGE_BASE "./merge"
#define MERGE_DATA MERGE_BASE "/data"
static const char merge_fl1[] = MERGE_DATA "/fl1";
static const char merge_dl1[] = MERGE_DATA "/dl1";
--
2.32.0
^ permalink raw reply related
* [PATCH v1 4/4] selftests/landlock: Check all possible intermediate directories
From: Mickaël Salaün @ 2021-06-30 22:48 UTC (permalink / raw)
To: Al Viro, James Morris, Serge Hallyn
Cc: Mickaël Salaün, Andy Lutomirski, Jann Horn, Kees Cook,
Shuah Khan, linux-fsdevel, linux-kernel, linux-kselftest,
linux-security-module, Mickaël Salaün
In-Reply-To: <20210630224856.1313928-1-mic@digikod.net>
From: Mickaël Salaün <mic@linux.microsoft.com>
Open and store file descriptors for the initial test directory at layout
initialization and at sandbox creation. This enables checking relative
path opening with different scenarios. To be sure to test all
combinations, the intermediate path checks are of exponential
complexity, which is OK for these use case though. All meaningful tests
now also check relative paths.
Use some macros to avoid useless helper call rewrites with a context
tied to each test.
Cc: James Morris <jmorris@namei.org>
Cc: Jann Horn <jannh@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Shuah Khan <shuah@kernel.org>
Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
Link: https://lore.kernel.org/r/20210630224856.1313928-5-mic@digikod.net
---
tools/testing/selftests/landlock/fs_test.c | 140 ++++++++++++++++++---
1 file changed, 125 insertions(+), 15 deletions(-)
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 403c8255311f..b1a91fdd0f88 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -154,7 +154,13 @@ static int remove_path(const char *const path)
return err;
}
-static void prepare_layout(struct __test_metadata *const _metadata)
+struct layout_context {
+ int init_layout_cwd_fd;
+ int init_sandbox_cwd_fd;
+};
+
+static void prepare_layout(struct __test_metadata *const _metadata,
+ struct layout_context *const ctx)
{
disable_caps(_metadata);
umask(0077);
@@ -171,10 +177,18 @@ static void prepare_layout(struct __test_metadata *const _metadata)
clear_cap(_metadata, CAP_SYS_ADMIN);
ASSERT_EQ(0, chdir(TMP_DIR));
+ ctx->init_layout_cwd_fd = open(".", O_DIRECTORY | O_CLOEXEC);
+ ASSERT_LE(0, ctx->init_layout_cwd_fd);
+ ctx->init_sandbox_cwd_fd = -1;
}
-static void cleanup_layout(struct __test_metadata *const _metadata)
+static void cleanup_layout(struct __test_metadata *const _metadata,
+ const struct layout_context *const ctx)
{
+ EXPECT_EQ(0, close(ctx->init_layout_cwd_fd));
+ if (ctx->init_sandbox_cwd_fd != -1) {
+ EXPECT_EQ(0, close(ctx->init_sandbox_cwd_fd));
+ }
EXPECT_EQ(0, chdir(".."));
set_cap(_metadata, CAP_SYS_ADMIN);
@@ -227,11 +241,12 @@ static void remove_layout1(struct __test_metadata *const _metadata)
}
FIXTURE(layout1) {
+ struct layout_context ctx;
};
FIXTURE_SETUP(layout1)
{
- prepare_layout(_metadata);
+ prepare_layout(_metadata, &self->ctx);
create_layout1(_metadata);
}
@@ -240,7 +255,7 @@ FIXTURE_TEARDOWN(layout1)
{
remove_layout1(_metadata);
- cleanup_layout(_metadata);
+ cleanup_layout(_metadata, &self->ctx);
}
/*
@@ -264,11 +279,84 @@ static int test_open_rel(const int dirfd, const char *const path, const int flag
return 0;
}
-static int test_open(const char *const path, const int flags)
+static int _test_open_all(const int base_fd, char *const path, const int flags)
+{
+ int ret_child, i;
+
+ ret_child = test_open_rel(base_fd, path, flags);
+ if (ret_child == ENOENT)
+ return ret_child;
+
+ for (i = strlen(path); i > 0; i--) {
+ int inter_fd, ret_parent;
+
+ if (path[i] != '/')
+ continue;
+ path[i] = '\0';
+ /* Using a non-O_PATH FD fills the cache. */
+ inter_fd = openat(base_fd, path, O_CLOEXEC | O_DIRECTORY);
+ path[i] = '/';
+ if (inter_fd < 0) {
+ if (errno != EACCES)
+ return -1;
+ /* If the sandbox denies access, then use O_PATH. */
+ path[i] = '\0';
+ inter_fd = openat(base_fd, path, O_CLOEXEC |
+ O_DIRECTORY | O_PATH);
+ path[i] = '/';
+ if (inter_fd < 0)
+ return -1;
+ }
+
+ /*
+ * Checks all possible inter_fd combinations with
+ * recursive calls.
+ */
+ ret_parent = _test_open_all(inter_fd, path + i + 1, flags);
+ close(inter_fd);
+
+ /* Checks inconsistencies that may come from the cache. */
+ if (ret_parent != ret_child)
+ return -1;
+ }
+ return ret_child;
+}
+
+static int _test_open_ctx(const struct layout_context *const ctx,
+ const char *const path, const int flags)
{
- return test_open_rel(AT_FDCWD, path, flags);
+ char *walker;
+ int ret_init, ret_next, i;
+ /*
+ * Checks with a FD opened without sandbox, and another FD opened
+ * within a sandbox (but maybe not the same).
+ */
+ const int extra_dirfd[] = {
+ ctx->init_layout_cwd_fd,
+ ctx->init_sandbox_cwd_fd
+ };
+
+ walker = strdup(path);
+ if (!walker)
+ return ENOMEM;
+
+ ret_init = _test_open_all(AT_FDCWD, walker, flags);
+ for (i = 0; i < ARRAY_SIZE(extra_dirfd); i++) {
+ const int dirfd = extra_dirfd[i];
+
+ if (dirfd != -1) {
+ ret_next = _test_open_all(dirfd, walker, flags);
+ if (ret_next != ret_init)
+ return -1;
+ }
+ }
+
+ free(walker);
+ return ret_init;
}
+#define test_open(path, flags) _test_open_ctx(&self->ctx, path, flags)
+
TEST_F_FORK(layout1, no_restriction)
{
ASSERT_EQ(0, test_open(dir_s1d1, O_RDONLY));
@@ -493,15 +581,28 @@ static int create_ruleset(struct __test_metadata *const _metadata,
return ruleset_fd;
}
-static void enforce_ruleset(struct __test_metadata *const _metadata,
- const int ruleset_fd)
+static void _enforce_ruleset(struct layout_context *const ctx,
+ struct __test_metadata *const _metadata, const int ruleset_fd)
{
ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0));
ASSERT_EQ(0, landlock_restrict_self(ruleset_fd, 0)) {
TH_LOG("Failed to enforce ruleset: %s", strerror(errno));
}
+
+ if (ctx->init_sandbox_cwd_fd == -1) {
+ ctx->init_sandbox_cwd_fd = open(".", O_DIRECTORY | O_CLOEXEC);
+ if (ctx->init_sandbox_cwd_fd == -1) {
+ ASSERT_EQ(EACCES, errno);
+ ctx->init_sandbox_cwd_fd = open(".", O_DIRECTORY |
+ O_CLOEXEC | O_PATH);
+ ASSERT_LE(0, ctx->init_sandbox_cwd_fd);
+ }
+ }
}
+#define enforce_ruleset(_metadata, ruleset_fd) \
+ _enforce_ruleset(&self->ctx, _metadata, ruleset_fd)
+
TEST_F_FORK(layout1, proc_nsfs)
{
const struct rule rules[] = {
@@ -1265,7 +1366,6 @@ TEST_F_FORK(layout1, rule_inside_mount_ns)
enforce_ruleset(_metadata, ruleset_fd);
ASSERT_EQ(0, close(ruleset_fd));
- ASSERT_EQ(0, test_open("s3d3", O_RDONLY));
ASSERT_EQ(EACCES, test_open("/", O_RDONLY));
}
@@ -1366,7 +1466,8 @@ enum relative_access {
REL_CHROOT_CHDIR,
};
-static void test_relative_path(struct __test_metadata *const _metadata,
+static void _test_relative_path(struct __test_metadata *const _metadata,
+ FIXTURE_DATA(layout1) *const self,
const enum relative_access rel)
{
/*
@@ -1479,6 +1580,8 @@ static void test_relative_path(struct __test_metadata *const _metadata,
ASSERT_EQ(0, close(ruleset_fd));
}
+#define test_relative_path(_metadata, rel) _test_relative_path(_metadata, self, rel)
+
TEST_F_FORK(layout1, relative_open)
{
test_relative_path(_metadata, REL_OPEN);
@@ -1809,7 +1912,8 @@ TEST_F_FORK(layout1, remove_file)
ASSERT_EQ(0, unlinkat(AT_FDCWD, file1_s1d3, 0));
}
-static void test_make_file(struct __test_metadata *const _metadata,
+static void _test_make_file(struct __test_metadata *const _metadata,
+ FIXTURE_DATA(layout1) *self,
const __u64 access, const mode_t mode, const dev_t dev)
{
const struct rule rules[] = {
@@ -1860,6 +1964,10 @@ static void test_make_file(struct __test_metadata *const _metadata,
ASSERT_EQ(0, rename(file1_s1d3, file2_s1d3));
}
+#define test_make_file(_metadata, access, mode, dev) \
+ _test_make_file(_metadata, self, access, mode, dev)
+
+
TEST_F_FORK(layout1, make_char)
{
/* Creates a /dev/null device. */
@@ -2076,11 +2184,12 @@ TEST_F_FORK(layout1, proc_pipe)
}
FIXTURE(layout1_bind) {
+ struct layout_context ctx;
};
FIXTURE_SETUP(layout1_bind)
{
- prepare_layout(_metadata);
+ prepare_layout(_metadata, &self->ctx);
create_layout1(_metadata);
@@ -2097,7 +2206,7 @@ FIXTURE_TEARDOWN(layout1_bind)
remove_layout1(_metadata);
- cleanup_layout(_metadata);
+ cleanup_layout(_metadata, &self->ctx);
}
static const char bind_dir_s1d3[] = "./s2d1/s2d2/s1d3";
@@ -2417,11 +2526,12 @@ static const char (*merge_sub_files[])[] = {
*/
FIXTURE(layout2_overlay) {
+ struct layout_context ctx;
};
FIXTURE_SETUP(layout2_overlay)
{
- prepare_layout(_metadata);
+ prepare_layout(_metadata, &self->ctx);
create_directory(_metadata, LOWER_BASE);
set_cap(_metadata, CAP_SYS_ADMIN);
@@ -2484,7 +2594,7 @@ FIXTURE_TEARDOWN(layout2_overlay)
clear_cap(_metadata, CAP_SYS_ADMIN);
EXPECT_EQ(0, remove_path(MERGE_DATA));
- cleanup_layout(_metadata);
+ cleanup_layout(_metadata, &self->ctx);
}
TEST_F_FORK(layout2_overlay, no_restriction)
--
2.32.0
^ permalink raw reply related
* [PATCH v1 2/4] landlock: Add filesystem rule caching
From: Mickaël Salaün @ 2021-06-30 22:48 UTC (permalink / raw)
To: Al Viro, James Morris, Serge Hallyn
Cc: Mickaël Salaün, Andy Lutomirski, Jann Horn, Kees Cook,
Shuah Khan, linux-fsdevel, linux-kernel, linux-kselftest,
linux-security-module, Mickaël Salaün
In-Reply-To: <20210630224856.1313928-1-mic@digikod.net>
From: Mickaël Salaün <mic@linux.microsoft.com>
Because of the unprivileged nature of Landlock, the main issue with the
current implementation is that path access checks require to walk from
the leaf file to the (real) root directory for each open-like syscall.
To limit this path walk, leverage the new resolve_path_at() hook to
reuse cached accesses collected for files at open time, when these files
are then used for relative path resolution (e.g. dirfd in openat()-like
syscalls).
Indeed, in practice, software walking through the filesystem (e.g.
"find" tool) trigger a lot of access checks. However, efficient walk
implementations reuse opened directory file descriptors to continue the
walk to children directories. Being able to cache such access checks
leads to significant performance improvements.
This cache mechanism is implemented thanks to new Landlock security
blobs attached to tasks and files. Each landlocked task keeps a
(weak) reference to the last file used to open a path. If the
referenced base path matches a directory in the current path walk and if
the access request was already granted to the base path, then the path
walk stops early and access is granted.
Each file opened by a landlocked task gets a cache that will be valid as
long as the backing struct file exists. This cache also references the
domain for which the cached accesses are valid.
Running the "find" tool on a directory with Linux source files (version
5.13, which contains a depth of less than 10 nested source directories)
is used as a metric because it is a real-life worse case that requires
to walk through a lot of files in different directories. To highlight
the measured performance impact, we put these source files in directories
of different depth: 1, 10 and 20. Here are the times taken by
Landlock for an open syscall after 10,000 iterations with the mainline
kernel and the caching changes:
Depth of 1: /0/
- v5.13: 28%
- this patch: 22%
Depth of 10: /0/1/.../9/
- v5.13: 43%
- this patch: 25%
Depth of 20: /0/1/.../19/
- v5.13: 54%
- this patch: 30%
The performance impact for such worth case is significantly reduced:
theoretically going from O(n) with n as the depth of a path, to O(1),
but as we can see there is still an increased penalty with the depth.
This may be explained with the relative ".." resolutions that don't
benefit from this caching. As a side note, the getdents64 syscall (not
impacted by Landlock) takes 18 times longer than the openat syscall in
is this scenario.
Cc: James Morris <jmorris@namei.org>
Cc: Jann Horn <jannh@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
Link: https://lore.kernel.org/r/20210630224856.1313928-3-mic@digikod.net
---
security/landlock/cache.h | 77 +++++++++++++
security/landlock/cred.c | 15 ++-
security/landlock/cred.h | 20 +++-
security/landlock/fs.c | 224 +++++++++++++++++++++++++++++++++++---
security/landlock/fs.h | 29 +++++
security/landlock/setup.c | 2 +
6 files changed, 350 insertions(+), 17 deletions(-)
create mode 100644 security/landlock/cache.h
diff --git a/security/landlock/cache.h b/security/landlock/cache.h
new file mode 100644
index 000000000000..7ceaa3b15671
--- /dev/null
+++ b/security/landlock/cache.h
@@ -0,0 +1,77 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Landlock LSM - Generic cache management
+ *
+ * Copyright © 2021 Microsoft Corporation
+ */
+
+#ifndef _SECURITY_LANDLOCK_CACHE_H
+#define _SECURITY_LANDLOCK_CACHE_H
+
+#include <linux/compiler.h>
+#include <linux/refcount.h>
+#include <linux/types.h>
+
+#include "ruleset.h"
+
+/**
+ * struct landlock_cache - Generic access cache for an object
+ *
+ * Store cached access rights for a Landlock object (i.e. tied to specific
+ * domain). Allowed accesses are set once (e.g. at file opening) and never
+ * change after that. As a side effect, this means that such cache created
+ * before a domain transition will not get an up to date allowed accesses.
+ * This implies to always check a cached domain against the current domain
+ * thanks to landlock_cache_is_valid().
+ *
+ * This struct is part of a typed cache (e.g. &struct landlock_fs_cache.core)
+ * that identifies the tied object.
+ */
+struct landlock_cache {
+ /**
+ * @dangling_domain: If not NULL, points to the domain for which
+ * @allowed_accesses is valid. This brings two constraints:
+ * - @dangling_domain must only be read with READ_ONCE() and written
+ * with WRITE_ONCE() (except at initialization).
+ * - @dangling_domain can only be safely dereferenced by the cache
+ * owner (e.g. with landlock_disable_cache() when the underlying file
+ * is being closed).
+ */
+ void *dangling_domain;
+ /**
+ * @usage: This counter is used to keep a cache alive while it can
+ * still be checked against.
+ */
+ refcount_t usage;
+ /**
+ * @allowed_accesses: Mask of absolute known-to-be allowed accesses to
+ * an object at creation-time (e.g. at open-time for the file hierarchy
+ * of a file descriptor). A bit not set doesn't mean that the related
+ * access is denied. The type of access is inferred from the type of
+ * the related object. The task domain may not be the same as the
+ * cached one and they must then be checked against each other when
+ * evaluating @allowed_accesses thanks to landlock_cache_is_valid().
+ */
+ u16 allowed_accesses;
+};
+
+static inline void landlock_disable_cache(struct landlock_cache *cache)
+{
+ struct landlock_ruleset *const dom = cache->dangling_domain;
+
+ /* Atomically marks the cache as disabled. */
+ WRITE_ONCE(cache->dangling_domain, NULL);
+ /*
+ * There is no need for other synchronisation mechanism because the
+ * domain is never dereferenced elsewhere.
+ */
+ landlock_put_ruleset(dom);
+}
+
+static inline bool landlock_cache_is_valid(const struct landlock_cache *cache,
+ const struct landlock_ruleset *domain)
+{
+ return (domain && domain == READ_ONCE(cache->dangling_domain));
+}
+
+#endif /* _SECURITY_LANDLOCK_CACHE_H */
diff --git a/security/landlock/cred.c b/security/landlock/cred.c
index 6725af24c684..8c9408dd46e1 100644
--- a/security/landlock/cred.c
+++ b/security/landlock/cred.c
@@ -1,16 +1,19 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
- * Landlock LSM - Credential hooks
+ * Landlock LSM - Credential and task hooks
*
* Copyright © 2017-2020 Mickaël Salaün <mic@digikod.net>
* Copyright © 2018-2020 ANSSI
+ * Copyright © 2021 Microsoft Corporation
*/
#include <linux/cred.h>
#include <linux/lsm_hooks.h>
+#include <linux/sched.h>
#include "common.h"
#include "cred.h"
+#include "fs.h"
#include "ruleset.h"
#include "setup.h"
@@ -34,9 +37,19 @@ static void hook_cred_free(struct cred *const cred)
landlock_put_ruleset_deferred(dom);
}
+static void hook_task_free(struct task_struct *const task)
+{
+ struct landlock_fs_cache *const last_at_cache =
+ landlock_task(task)->cache.last_at;
+
+ landlock_put_fs_cache(last_at_cache);
+}
+
static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(cred_prepare, hook_cred_prepare),
LSM_HOOK_INIT(cred_free, hook_cred_free),
+
+ LSM_HOOK_INIT(task_free, hook_task_free),
};
__init void landlock_add_cred_hooks(void)
diff --git a/security/landlock/cred.h b/security/landlock/cred.h
index 5f99d3decade..0734a14933c1 100644
--- a/security/landlock/cred.h
+++ b/security/landlock/cred.h
@@ -1,9 +1,10 @@
/* SPDX-License-Identifier: GPL-2.0-only */
/*
- * Landlock LSM - Credential hooks
+ * Landlock LSM - Credential and task hooks
*
* Copyright © 2019-2020 Mickaël Salaün <mic@digikod.net>
* Copyright © 2019-2020 ANSSI
+ * Copyright © 2021 Microsoft Corporation
*/
#ifndef _SECURITY_LANDLOCK_CRED_H
@@ -13,6 +14,7 @@
#include <linux/init.h>
#include <linux/rcupdate.h>
+#include "fs.h"
#include "ruleset.h"
#include "setup.h"
@@ -20,13 +22,27 @@ struct landlock_cred_security {
struct landlock_ruleset *domain;
};
+struct landlock_task_cache {
+ struct landlock_fs_cache *last_at;
+};
+
+struct landlock_task_security {
+ struct landlock_task_cache cache;
+};
+
static inline struct landlock_cred_security *landlock_cred(
const struct cred *cred)
{
return cred->security + landlock_blob_sizes.lbs_cred;
}
-static inline const struct landlock_ruleset *landlock_get_current_domain(void)
+static inline struct landlock_task_security *landlock_task(
+ const struct task_struct *task)
+{
+ return task->security + landlock_blob_sizes.lbs_task;
+}
+
+static inline struct landlock_ruleset *landlock_get_current_domain(void)
{
return landlock_cred(current_cred())->domain;
}
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index 97b8e421f617..f9ea08f4078f 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -4,6 +4,7 @@
*
* Copyright © 2016-2020 Mickaël Salaün <mic@digikod.net>
* Copyright © 2018-2020 ANSSI
+ * Copyright © 2021 Microsoft Corporation
*/
#include <linux/atomic.h>
@@ -20,6 +21,7 @@
#include <linux/lsm_hooks.h>
#include <linux/mount.h>
#include <linux/namei.h>
+#include <linux/overflow.h>
#include <linux/path.h>
#include <linux/rcupdate.h>
#include <linux/spinlock.h>
@@ -29,6 +31,7 @@
#include <linux/workqueue.h>
#include <uapi/linux/landlock.h>
+#include "cache.h"
#include "common.h"
#include "cred.h"
#include "fs.h"
@@ -37,6 +40,78 @@
#include "ruleset.h"
#include "setup.h"
+/* Cache management */
+
+/**
+ * struct landlock_fs_cache - Landlock access cache for the filesystem
+ *
+ * Cached accesses for a filesystem object relative to a specific domain.
+ */
+struct landlock_fs_cache {
+ /**
+ * @dangling_path: Enables identifying a path (same mnt and dentry
+ * fields), iff the other path is held and
+ * READ_ONCE(core->dangling_domain) is not NULL. Because there is no
+ * synchronization mechanism, @dangling_path can only be safely
+ * derefenced by the cache owner (e.g. with create_fs_cache()).
+ */
+ struct path dangling_path;
+ /**
+ * @core: Stores the generic caching.
+ */
+ struct landlock_cache core;
+};
+
+static void build_check_cache(void)
+{
+ const struct landlock_cache cache = {
+ .allowed_accesses = ~0,
+ };
+ struct landlock_layer *layer;
+ struct landlock_ruleset *ruleset;
+
+ BUILD_BUG_ON(cache.allowed_accesses < LANDLOCK_MASK_ACCESS_FS);
+ BUILD_BUG_ON(!__same_type(cache.allowed_accesses, layer->access));
+ BUILD_BUG_ON(!__same_type(cache.allowed_accesses,
+ ruleset->fs_access_masks[0]));
+}
+
+static struct landlock_fs_cache *create_fs_cache(
+ struct landlock_ruleset *domain, const u16 allowed_accesses,
+ struct path *const path)
+{
+ struct landlock_fs_cache *cache;
+
+ build_check_cache();
+
+ /* Only creates useful cache. */
+ if (!allowed_accesses)
+ return NULL;
+
+ cache = kzalloc(sizeof(*cache), GFP_KERNEL_ACCOUNT);
+ if (!cache)
+ return NULL;
+
+ landlock_get_ruleset(domain);
+ cache->core.dangling_domain = domain;
+ cache->core.allowed_accesses = allowed_accesses;
+ path_get(path);
+ cache->dangling_path = *path;
+ refcount_set(&cache->core.usage, 1);
+ return cache;
+}
+
+static void landlock_get_fs_cache(struct landlock_fs_cache *cache)
+{
+ refcount_inc(&cache->core.usage);
+}
+
+void landlock_put_fs_cache(struct landlock_fs_cache *cache)
+{
+ if (cache && refcount_dec_and_test(&cache->core.usage))
+ kfree(cache);
+}
+
/* Underlying object management */
static void release_inode(struct landlock_object *const object)
@@ -180,10 +255,42 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
/* Access-control management */
+static inline bool is_allowed_in_cache(
+ const struct landlock_ruleset *const domain,
+ const struct path *const path, const u32 access_request,
+ u16 (*const denied_cache)[])
+{
+ size_t i;
+ struct landlock_fs_cache *const fs_cache =
+ landlock_task(current)->cache.last_at;
+
+ if (!fs_cache)
+ return false;
+ /*
+ * This domain check protects against race conditions when checking if
+ * @path is equal to fs_cache->dangling_path because we own @path and
+ * fs_cache->dangling_path is only reset after the cache is disabled.
+ */
+ if (!landlock_cache_is_valid(&fs_cache->core, domain))
+ return false;
+ /* path_equal() doesn't dereference the mnt and dentry fields. */
+ if (!path_equal(path, &fs_cache->dangling_path))
+ return false;
+
+ /* Fills the returned cache as much as possible. */
+ for (i = 0; i < domain->num_layers; i++)
+ /* Removes allowed accesses from the denied lists. */
+ (*denied_cache)[i] &= ~fs_cache->core.allowed_accesses;
+
+ if ((access_request & fs_cache->core.allowed_accesses) != access_request)
+ return false;
+ return true;
+}
+
static inline u64 unmask_layers(
const struct landlock_ruleset *const domain,
const struct path *const path, const u32 access_request,
- u64 layer_mask)
+ u64 layer_mask, u16 (*const denied_cache)[])
{
const struct landlock_rule *rule;
const struct inode *inode;
@@ -212,19 +319,20 @@ static inline u64 unmask_layers(
const u64 layer_level = BIT_ULL(layer->level - 1);
/* Checks that the layer grants access to the full request. */
- if ((layer->access & access_request) == access_request) {
+ if ((layer->access & access_request) == access_request)
layer_mask &= ~layer_level;
- if (layer_mask == 0)
- return layer_mask;
- }
+ /* Removes allowed accesses from the denied lists. */
+ (*denied_cache)[layer->level - 1] &= ~layer->access;
}
return layer_mask;
}
static int check_access_path(const struct landlock_ruleset *const domain,
- const struct path *const path, u32 access_request)
+ const struct path *const path, const u32 access_request,
+ u16 *const allowed_accesses)
{
+ typeof(*allowed_accesses) denied_cache[LANDLOCK_MAX_NUM_LAYERS];
bool allowed = false;
struct path walker_path;
u64 layer_mask;
@@ -252,6 +360,8 @@ static int check_access_path(const struct landlock_ruleset *const domain,
/* Saves all layers handling a subset of requested accesses. */
layer_mask = 0;
for (i = 0; i < domain->num_layers; i++) {
+ /* Stores potentially denied accesses. */
+ denied_cache[i] = domain->fs_access_masks[i];
if (domain->fs_access_masks[i] & access_request)
layer_mask |= BIT_ULL(i);
}
@@ -268,8 +378,15 @@ static int check_access_path(const struct landlock_ruleset *const domain,
while (true) {
struct dentry *parent_dentry;
+ if (is_allowed_in_cache(domain, &walker_path,
+ access_request, &denied_cache)) {
+ /* Stops when access request is allowed by a cache. */
+ allowed = true;
+ break;
+ }
+
layer_mask = unmask_layers(domain, &walker_path,
- access_request, layer_mask);
+ access_request, layer_mask, &denied_cache);
if (layer_mask == 0) {
/* Stops when a rule from each layer grants access. */
allowed = true;
@@ -304,6 +421,15 @@ static int check_access_path(const struct landlock_ruleset *const domain,
walker_path.dentry = parent_dentry;
}
path_put(&walker_path);
+
+ if (allowed_accesses) {
+ typeof(*allowed_accesses) allowed_cache =
+ LANDLOCK_MASK_ACCESS_FS;
+
+ for (i = 0; i < domain->num_layers; i++)
+ allowed_cache &= ~denied_cache[i];
+ *allowed_accesses = allowed_cache;
+ }
return allowed ? 0 : -EACCES;
}
@@ -315,7 +441,7 @@ static inline int current_check_access_path(const struct path *const path,
if (!dom)
return 0;
- return check_access_path(dom, path, access_request);
+ return check_access_path(dom, path, access_request, NULL);
}
/* Inode hooks */
@@ -560,7 +686,8 @@ static int hook_path_link(struct dentry *const old_dentry,
if (unlikely(d_is_negative(old_dentry)))
return -ENOENT;
return check_access_path(dom, new_dir,
- get_mode_access(d_backing_inode(old_dentry)->i_mode));
+ get_mode_access(d_backing_inode(old_dentry)->i_mode),
+ NULL);
}
static inline u32 maybe_remove(const struct dentry *const dentry)
@@ -590,7 +717,8 @@ static int hook_path_rename(const struct path *const old_dir,
/* RENAME_EXCHANGE is handled because directories are the same. */
return check_access_path(dom, old_dir, maybe_remove(old_dentry) |
maybe_remove(new_dentry) |
- get_mode_access(d_backing_inode(old_dentry)->i_mode));
+ get_mode_access(d_backing_inode(old_dentry)->i_mode),
+ NULL);
}
static int hook_path_mkdir(const struct path *const dir,
@@ -608,7 +736,7 @@ static int hook_path_mknod(const struct path *const dir,
if (!dom)
return 0;
- return check_access_path(dom, dir, get_mode_access(mode));
+ return check_access_path(dom, dir, get_mode_access(mode), NULL);
}
static int hook_path_symlink(const struct path *const dir,
@@ -651,17 +779,83 @@ static inline u32 get_file_access(const struct file *const file)
static int hook_file_open(struct file *const file)
{
- const struct landlock_ruleset *const dom =
- landlock_get_current_domain();
+ int ret;
+ u16 allowed_accesses = 0;
+ struct landlock_ruleset *const dom = landlock_get_current_domain();
if (!dom)
return 0;
+
/*
* Because a file may be opened with O_PATH, get_file_access() may
* return 0. This case will be handled with a future Landlock
* evolution.
*/
- return check_access_path(dom, &file->f_path, get_file_access(file));
+ ret = check_access_path(dom, &file->f_path, get_file_access(file),
+ &allowed_accesses);
+
+ /*
+ * Ignores (accounted) memory allocation errors: just don't
+ * create a cache.
+ */
+ landlock_file(file)->cache = create_fs_cache(dom,
+ allowed_accesses, &file->f_path);
+ return ret;
+}
+
+static void hook_file_free_security(struct file *const file)
+{
+ struct landlock_fs_cache *const file_cache =
+ landlock_file(file)->cache;
+
+ if (!file_cache)
+ return;
+
+ /*
+ * If there is a cache, it is well alive because it was created at this
+ * file opening and is owned by this file. Disables the cache and puts
+ * the (dangling) object reference; the order doesn't matter. There is
+ * no access-control race-condition in is_allowed_in_cache() because
+ * the dangling path can only match if it is also owned by the caller.
+ */
+ landlock_disable_cache(&file_cache->core);
+ /*
+ * Releases dangling_path; it may be freed at the end of file_free().
+ * Even if it is not required, resets dangling_path as a safety
+ * measure.
+ */
+ path_put_init(&file_cache->dangling_path);
+ landlock_put_fs_cache(file_cache);
+}
+
+static int hook_resolve_path_at(const struct path *path_at,
+ struct file *file_at, int lookup_flags)
+{
+ const struct landlock_ruleset *const dom =
+ landlock_get_current_domain();
+ struct landlock_fs_cache **const current_at_cache =
+ &landlock_task(current)->cache.last_at;
+
+ if (!dom)
+ return 0;
+
+ /* Clean up previous cache, if any. */
+ landlock_put_fs_cache(*current_at_cache);
+ *current_at_cache = NULL;
+
+ if (file_at) {
+ struct landlock_fs_cache *const file_at_cache =
+ landlock_file(file_at)->cache;
+
+ if (!file_at_cache)
+ return 0;
+ /* Don't update existing cached accesses. */
+ if (!landlock_cache_is_valid(&file_at_cache->core, dom))
+ return 0;
+ landlock_get_fs_cache(file_at_cache);
+ *current_at_cache = file_at_cache;
+ }
+ return 0;
}
static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
@@ -683,6 +877,8 @@ static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(path_rmdir, hook_path_rmdir),
LSM_HOOK_INIT(file_open, hook_file_open),
+ LSM_HOOK_INIT(file_free_security, hook_file_free_security),
+ LSM_HOOK_INIT(resolve_path_at, hook_resolve_path_at),
};
__init void landlock_add_fs_hooks(void)
diff --git a/security/landlock/fs.h b/security/landlock/fs.h
index 187284b421c9..2c57406f73b4 100644
--- a/security/landlock/fs.h
+++ b/security/landlock/fs.h
@@ -4,6 +4,7 @@
*
* Copyright © 2017-2020 Mickaël Salaün <mic@digikod.net>
* Copyright © 2018-2020 ANSSI
+ * Copyright © 2021 Microsoft Corporation
*/
#ifndef _SECURITY_LANDLOCK_FS_H
@@ -12,7 +13,9 @@
#include <linux/fs.h>
#include <linux/init.h>
#include <linux/rcupdate.h>
+#include <linux/refcount.h>
+#include "object.h"
#include "ruleset.h"
#include "setup.h"
@@ -50,6 +53,24 @@ struct landlock_superblock_security {
atomic_long_t inode_refs;
};
+struct landlock_fs_cache;
+
+/**
+ * struct landlock_file_security - File security blob
+ *
+ * Cached access right for an open file.
+ */
+struct landlock_file_security {
+ /**
+ * @cache: This cache is set once at the file opening and never change
+ * after that (except when freeing the file). This means that a file
+ * open before a domain transition will not get an updated cache, but
+ * the domain tied to the cache must always be checked with
+ * landlock_cache_is_valid().
+ */
+ struct landlock_fs_cache *cache;
+};
+
static inline struct landlock_inode_security *landlock_inode(
const struct inode *const inode)
{
@@ -62,8 +83,16 @@ static inline struct landlock_superblock_security *landlock_superblock(
return superblock->s_security + landlock_blob_sizes.lbs_superblock;
}
+static inline struct landlock_file_security *landlock_file(
+ const struct file *const file)
+{
+ return file->f_security + landlock_blob_sizes.lbs_file;
+}
+
__init void landlock_add_fs_hooks(void);
+void landlock_put_fs_cache(struct landlock_fs_cache *cache);
+
int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
const struct path *const path, u32 access_hierarchy);
diff --git a/security/landlock/setup.c b/security/landlock/setup.c
index f8e8e980454c..42d381dd0f0c 100644
--- a/security/landlock/setup.c
+++ b/security/landlock/setup.c
@@ -19,8 +19,10 @@ bool landlock_initialized __lsm_ro_after_init = false;
struct lsm_blob_sizes landlock_blob_sizes __lsm_ro_after_init = {
.lbs_cred = sizeof(struct landlock_cred_security),
+ .lbs_file = sizeof(struct landlock_file_security),
.lbs_inode = sizeof(struct landlock_inode_security),
.lbs_superblock = sizeof(struct landlock_superblock_security),
+ .lbs_task = sizeof(struct landlock_task_security),
};
static int __init landlock_init(void)
--
2.32.0
^ permalink raw reply related
* Re: [GIT PULL] SafeSetID changes for v5.14
From: pr-tracker-bot @ 2021-06-30 22:34 UTC (permalink / raw)
To: Micah Morton
Cc: Linus Torvalds, Linux Kernel Mailing List, linux-security-module
In-Reply-To: <CAJ-EccODUD45ZFgqqSxwZ9-DkqJL7F9fYOiHt+2tLZBss3VoAA@mail.gmail.com>
The pull request you sent on Wed, 30 Jun 2021 10:22:11 -1000:
> https://github.com/micah-morton/linux.git tags/safesetid-5.14
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/92183137e6c14b68ff4de51f6ef371b2b1fe6e68
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox