smack: Possible NULL pointer deref in cred_free hook.

[Casey Schaufler]

On 2/15/2024 3:38 PM, Paul Moore wrote:
> On Wed, Feb 14, 2024 at 7:13 PM Casey Schaufler <casey at schaufler-ca.com> wrote:
>> On 2/14/2024 2:15 PM, Tetsuo Handa wrote:
>>> On 2024/02/15 3:55, Paul Moore wrote:
>>>>> Ah, but it turns out that the only LSM that can fail in _cred_prepare()
>>>>> is Smack. Even if smack_cred_prepare() fails it will have called
>>>>> init_task_smack(), so there isn't *currently* a problem. Should another
>>>>> LSM have the possibility of failing in whatever_cred_prepare() this
>>>>> could be an issue.
>>>> Let's make sure we fix this, even if it isn't a problem with the
>>>> current code, it is very possible it could become a problem at some
>>>> point in the future and I don't want to see us get surprised by this
>>>> then.
>>>>
>>> Anyone can built-in an out-of-tree LSM where whatever_cred_prepare() fails.
>>> An in-tree code that fails if an out-of-tree code (possibly BPF based LSM)
>>> is added should be considered as a problem with the current code.
>> Agreed. By the way, this isn't just a Smack problem.
> I've tried to make this clear on previous issues, but let me say it
> again: I don't care what individual LSMs are affected, a bug is a bug
> and we need to fix it.

Yes, I understand that.

>> You get what looks
>> like the same failure on an SELinux system if security_prepare_creds() fails
>> using the suggested "fault injection". It appears that any failure in
>> security_prepare_creds() has the potential to be fatal.
> Perhaps I didn't look at the original problem closely enough, but I
> believe this should only be an issue with LSMs that register a
> cred_free hook that assumes a valid LSM specific credential
> initialization.  While SELinux registers a cred_prepare hook, it does
> not register a cred_free hook.  Or am I missing something?

Yes, you're missing something. If security_prepare_creds() fails prepare_creds()
will fail, and the system will lurch to a halt because it can't create a new
cred. The cred_free hook is a red herring.

> Looking quickly I suspect this affects Smack and AppArmor.

That was my original thought as well. But any failure of security_prepare_creds()
is going to cause the problem. For in-tree LSMs that can only happen with Smack.
If an out-of-tree LSM fails in a cred_prepare hook, or if a BPF program fails
in cred_prepare, we'll have the problem. *Without Smack or AppArmor*.

My current thinking, which could easily change with new information, is that
the cred_prepare hook should be changed to be a void instead of an int hook,
and an LSM that might have a failure case (Smack's memory allocation, for example)
will have to handle the problem on its own. Smack would have to radically change
how it manages supplemental rule lists and label change lists, moving them out
of the cred.

Also to be considered is that the "fault injection" used causes the system to fail
immediately. A more subtle fault injection, provided on a system that has reached
a running state, ought not to have the spectacular behavior seen here. Again, not
an excuse, just an observation.

>   While
> Landlock registers a cred_free hook, it looks like it should properly
> handle being called without a cred_prepare hook first being executed.
> Of course Mickaël is the one who should confirm this.
>