Re: [PATCH v10 5/8] rust: security: add abstraction for secctx

[Casey Schaufler <casey@schaufler-ca.com>]

On 9/15/2024 2:07 PM, Alice Ryhl wrote:
> On Sun, Sep 15, 2024 at 10:58 PM Kees Cook <kees@kernel.org> wrote:
>> On Sun, Sep 15, 2024 at 02:31:31PM +0000, Alice Ryhl wrote:
>>> Add an abstraction for viewing the string representation of a security
>>> context.
>> Hm, this may collide with "LSM: Move away from secids" is going to happen.
>> https://lore.kernel.org/all/20240830003411.16818-1-casey@schaufler-ca.com/
>>
>> This series is not yet landed, but in the future, the API changes should
>> be something like this, though the "lsmblob" name is likely to change to
>> "lsmprop"?
>> security_cred_getsecid()   -> security_cred_getlsmblob()
>> security_secid_to_secctx() -> security_lsmblob_to_secctx()

The referenced patch set does not change security_cred_getsecid()
nor remove security_secid_to_secctx(). There remain networking interfaces
that are unlikely to ever be allowed to move away from secids. It will
be necessary to either retain some of the secid interfaces or introduce
scaffolding around the lsm_prop structure.

Binder is currently only supported in SELinux, so this isn't a real issue
today. The BPF LSM could conceivably support binder, but only in cases where
SELinux isn't enabled. Should there be additional LSMs that support binder
the hooks would have to be changed to use lsm_prop interfaces, but I have
not included that *yet*.

> Thanks for the heads up. I'll make sure to look into how this
> interacts with those changes.

There will be a follow on patch set as well that replaces the LSMs use
of string/length pairs with a structure. This becomes necessary in cases
where more than one active LSM uses secids and security contexts. This
will affect binder.

>
>>> This is needed by Rust Binder because it has a feature where a process
>>> can view the string representation of the security context for incoming
>>> transactions. The process can use that to authenticate incoming
>>> transactions, and since the feature is provided by the kernel, the
>>> process can trust that the security context is legitimate.
>>>
>>> This abstraction makes the following assumptions about the C side:
>>> * When a call to `security_secid_to_secctx` is successful, it returns a
>>>   pointer and length. The pointer references a byte string and is valid
>>>   for reading for that many bytes.
>> Yes. (len includes trailing C-String NUL character.)
> I suppose the NUL character implies that this API always returns a
> non-zero length? I could simplify the patch a little bit by not
> handling empty strings.
>
> It looks like the CONFIG_SECURITY=n case returns -EOPNOTSUPP, so we
> don't get an empty string from that case, at least.
>
>>> * The string may be referenced until `security_release_secctx` is
>>>   called.
>> Yes.
>>
>>> * If CONFIG_SECURITY is set, then the three methods mentioned in
>>>   rust/helpers are available without a helper. (That is, they are not a
>>>   #define or `static inline`.)
>> Yes.
>>
>>> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
>>> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
>>> Reviewed-by: Trevor Gross <tmgross@umich.edu>
>>> Reviewed-by: Gary Guo <gary@garyguo.net>
>>> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>> Reviewed-by: Kees Cook <kees@kernel.org>
> Thanks for the review!
>
> Alice
>