From: Casey Schaufler <casey@schaufler-ca.com>
To: "Serge E. Hallyn" <serge@hallyn.com>, Paul Moore <paul@paul-moore.com>
Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
linux-security-module <linux-security-module@vger.kernel.org>,
Casey Schaufler <casey@schaufler-ca.com>
Subject: Re: smack: Possible NULL pointer deref in cred_free hook.
Date: Wed, 21 Feb 2024 09:40:39 -0800 [thread overview]
Message-ID: <6455c16e-b840-411e-a64e-7b066c2d9ff1@schaufler-ca.com> (raw)
In-Reply-To: <20240216233154.GA1005338@mail.hallyn.com>
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@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@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.
prev parent reply other threads:[~2024-02-21 17:40 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <ad9dddfe-0fa1-40f6-9f8c-f2c01c7a0211@I-love.SAKURA.ne.jp>
2024-02-06 14:31 ` smack: Possible NULL pointer deref in cred_free hook Tetsuo Handa
2024-02-07 1:39 ` Casey Schaufler
2024-02-07 2:54 ` Tetsuo Handa
2024-02-07 18:53 ` Casey Schaufler
2024-02-14 17:10 ` Casey Schaufler
2024-02-14 18:55 ` Paul Moore
2024-02-14 22:15 ` Tetsuo Handa
2024-02-15 0:13 ` Casey Schaufler
2024-02-15 23:38 ` Paul Moore
2024-02-16 0:22 ` Casey Schaufler
2024-02-16 3:32 ` Paul Moore
2024-02-16 23:31 ` Serge E. Hallyn
2024-02-21 17:40 ` Casey Schaufler [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6455c16e-b840-411e-a64e-7b066c2d9ff1@schaufler-ca.com \
--to=casey@schaufler-ca.com \
--cc=linux-security-module@vger.kernel.org \
--cc=paul@paul-moore.com \
--cc=penguin-kernel@i-love.sakura.ne.jp \
--cc=serge@hallyn.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).