On 11/10/2022 3:36 PM, Paul Moore wrote:
> On Wed, Nov 9, 2022 at 6:34 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
>> On Tue, Oct 25, 2022 at 2:48 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
>>> Create a system call lsm_self_attr() to provide the security
>>> module maintained attributes of the current process. Historically
>>> these attributes have been exposed to user space via entries in
>>> procfs under /proc/self/attr.
>>>
>>> Attributes are provided as a collection of lsm_ctx structures
>>> which are placed into a user supplied buffer. Each structure
>>> identifys the security module providing the attribute, which
>>> of the possible attributes is provided, the size of the
>>> attribute, and finally the attribute value. The format of the
>>> attribute value is defined by the security module, but will
>>> always be \0 terminated. The ctx_len value will be larger than
>>> strlen(ctx).
>>>
>>> ------------------------------
>>> | unsigned int id |
>>> ------------------------------
>>> | unsigned int flags |
>>> ------------------------------
>>> | __kernel_size_t ctx_len |
>>> ------------------------------
>>> | unsigned char ctx[ctx_len] |
>>> ------------------------------
>>> | unsigned int id |
>>> ------------------------------
>>> | unsigned int flags |
>>> ------------------------------
>>> | __kernel_size_t ctx_len |
>>> ------------------------------
>>> | unsigned char ctx[ctx_len] |
>>> ------------------------------
>>>
>>> Signed-off-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
>>> ---
>>> include/linux/syscalls.h | 2 +
>>> include/uapi/linux/lsm.h | 21 ++++++
>>> kernel/sys_ni.c | 3 +
>>> security/Makefile | 1 +
>>> security/lsm_syscalls.c | 156 +++++++++++++++++++++++++++++++++++++++
>>> 5 files changed, 183 insertions(+)
>>> create mode 100644 security/lsm_syscalls.c
> ..
>
>>> +/**
>>> + * lsm_self_attr - Return current task's security module attributes
>>> + * @ctx: the LSM contexts
>>> + * @size: size of @ctx, updated on return
>>> + * @flags: reserved for future use, must be zero
>>> + *
>>> + * Returns the calling task's LSM contexts. On success this
>>> + * function returns the number of @ctx array elements. This value
>>> + * may be zero if there are no LSM contexts assigned. If @size is
>>> + * insufficient to contain the return data -E2BIG is returned and
>>> + * @size is set to the minimum required size. In all other cases
>>> + * a negative value indicating the error is returned.
>>> + */
>>> +SYSCALL_DEFINE3(lsm_self_attr,
>>> + struct lsm_ctx __user *, ctx,
>>> + size_t __user *, size,
>>> + int, flags)
>> See my comments above about UAPI types, let's change this to something
>> like this:
>>
>> [NOTE: I'm assuming it is safe to use __XXX types in syscall declarations?]
>>
>> SYSCALL_DEFINE3(lsm_self_attr,
>> struct lsm_ctx __user *, ctx,
>> __kernel_size_t __user *, size,
>> __u32, flags)
>>
> I wanted to clarify how I originally envisioned this syscall/API, as
> it looks like it behaves a bit differently than I originally intended.
That's why we're having a review cycle.
> My thought was that this syscall would be used to fetch one LSM
> attribute context at a time, returning an array of lsm_ctx structs,
> with one, and only one, for each LSM that supports that particular
> attribute.
My thought with the interface I proposed is that we don't want a
separate syscall for each attribute: e.g. lsm_get_current(), lsm_get_prev(),
and we don't want a separate syscall for each LSM, e.g. lsm_get_selinux().
We can either specify which attribute is desired or which have been returned.
In either case we need an attribute identifier.
> If the LSM does not support that attribute, it must not
> enter an entry to the array. Requesting more than one attribute
> context per invocation is not allowed.
Why? That seems like an unnecessary and inconvenient restriction
for the program that wants to see more than one attribute. A service
that wants to check its fscreate, sockcreate and keycreate to see if
they're appropriate for the container it's running in, for example.
> The idea was to closely
> resemble the familiar open("/proc/self/attr/current")/read()/close()
> result without relying on procfs and supporting multiple LSMs with an
> easy(ish) API.
rc = lsm_get_self_attr(struct lsm_ctx *ctx, size, attr_id, flags);
This looks like what you're asking for. It's what I had proposed but with
the attr_id specified in the call rather than returned in the lsm_ctx.
> The new, single syscall should also be faster,
> although none of this should be happening in a performance critical
> section anyway.
Yes.
> In addition, the lsm_ctx::flags field is intended to convey
> information specific to the given LSM, i.e. it should not repeat any
> of the flag information specified in the syscall parameters. I don't
> believe any of the currently in-tree LSMs would require any special
> flags for their contexts, so this should always be zero/clear in this
> initial patchset, but it is something to keep in mind for the future.
>
> Thoughts?
I don't have any problem with *what I think* you're suggesting.
To make sure, I'll send a new patch. I'll also address lsm_set_self_attr().
Thank you for the feedback. Let's see if we can converge.
>
> --
> paul-moore.com
