smack: Possible NULL pointer deref in cred_free hook.

[Casey Schaufler]

On 2/16/2024 3:31 PM, Serge E. Hallyn wrote:
> On Thu, Feb 15, 2024 at 10:32:59PM -0500, Paul Moore wrote:
>> On Thu, Feb 15, 2024 at 7:22 PM Casey Schaufler <casey at schaufler-ca.com> wrote:
>>> 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.
>> Okay, my apologies, I thought the issue was due to one of the LSMs
>> failing their cred_prepare hook and causing security_cred_free() to be
>> called for LSMs that hadn't successfully cred_prepare()'d the new
>> creds.
>>
>> However, if I'm understanding you correctly, the issue is that a
>> failed security_prepare_creds() call can cause both prepare_creds()
>> and prepare_kernel_cred() to fail, yes?  If that is the case, can
>> someone explain to me why this is a problem?  Both prepare_creds() and
>> prepare_kernel_cred() can fail in ways unrelated to the LSM and thus
>> callers must be prepared to handle a failure in both prepare_cred()
>> functions.
> Sure does look that way...

I am going to argue that what we have here is an excessively aggressive
fault injection. Failing in security_prepare_creds() during system
initialization will prevent any credential transitions, which will
prevent the system from booting. This is true regardless of why the
failure occurs.

To prove this to myself I moved the fault injection into
smack_cred_prepare(), and made it check for a specific Smack label on
the "old" cred. Failure is returned if the old cred has that label.
The error condition is handled correctly in this case, with the calling
process being killed because of -ENOMEM being returned. No other error
is detected.

In smack_cred_prepare():

	init_task_smack(new_tsp, old_tsp->smk_task, old_tsp->smk_task);
+	if (old_tsp->smk_task == &smack_known_hat)
+		return -ENOMEM;

Then execute:

# (echo '^' > /proc/self/attr/smack/current ; date)

to see the failure being correctly handled.

A similar injection in any LSM will demonstrate the behavior.
I hope that we can put this "issue" to bed.