Re: smack: Possible NULL pointer deref in cred_free hook.

Re: smack: Possible NULL pointer deref in cred_free hook.

[Tetsuo Handa]

Hello, Casey.

I confirmed using fault injection shown below that smack_cred_free() is not
prepared for being called without successful smack_cred_prepare().

diff --git a/security/security.c b/security/security.c
index 10adc4d3c5e0..690136b60d28 100644
--- a/security/security.c
+++ b/security/security.c
@@ -3059,7 +3059,7 @@ int security_prepare_creds(struct cred *new, const struct cred *old, gfp_t gfp)
        if (rc)
                return rc;

-       rc = call_int_hook(cred_prepare, 0, new, old, gfp);
+       rc = -EPERM; //call_int_hook(cred_prepare, 0, new, old, gfp);
        if (unlikely(rc))
                security_cred_free(new);
        return rc;

[   11.366204] LSM: initializing lsm=capability,yama,smack,integrity
[   11.368005] Yama: becoming mindful.
[   11.371997] Smack:  Initializing.
[   11.373957] Smack:  IPv6 port labeling enabled.
[   11.379072] Mount-cache hash table entries: 16384 (order: 5, 131072 bytes, linear)
[   11.383676] Mountpoint-cache hash table entries: 16384 (order: 5, 131072 bytes, linear)
[   11.400071] Running RCU synchronous self tests
[   11.450967] Running RCU synchronous self tests
[   11.453956] BUG: kernel NULL pointer dereference, address: 0000000000000000
[   11.454948] #PF: supervisor read access in kernel mode
[   11.454948] #PF: error_code(0x0000) - not-present page
[   11.454948] PGD 0 P4D 0 
[   11.454948] Oops: 0000 [#1] PREEMPT SMP PTI
[   11.454948] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 6.8.0-rc3+ #47
[   11.454948] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 11/12/2020
[   11.454948] RIP: 0010:smk_destroy_label_list+0x11/0x40
[   11.454948] Code: 2e 0f 1f 84 00 00 00 00 00 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 66 0f 1f 00 0f 1f 44 00 00 55 48 89 fd 53 48 8b 3f <48> 8b 1f 48 39 fd 74 13 e8 c2 bd e2 ff 48 89 d8 48 89 df 48 8b 1b
[   11.454948] RSP: 0000:ffffffffa3003c50 EFLAGS: 00010286
[   11.454948] RAX: ffffffffa1f763e0 RBX: ffffffffa2f19b00 RCX: 0000000000000100
[   11.454948] RDX: 00000000000000d8 RSI: 0000000000000000 RDI: 0000000000000000
[   11.454948] RBP: ffff8d71c03300c8 R08: 0000000000400dc0 R09: 00000000ffffffff
[   11.454948] R10: ffffffffa3003c48 R11: ffff8d7259ffc220 R12: ffff8d71c0330018
[   11.454948] R13: ffffffffa3003e48 R14: ffff8d71c03c8000 R15: 0000000000000001
[   11.454948] FS:  0000000000000000(0000) GS:ffff8d7259e00000(0000) knlGS:0000000000000000
[   11.454948] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   11.454948] CR2: 0000000000000000 CR3: 000000014e03c000 CR4: 0000000000350ef0
[   11.454948] Call Trace:
[   11.454948]  <TASK>
[   11.454948]  ? __die_body+0x1e/0x60
[   11.454948]  ? page_fault_oops+0x15b/0x470
[   11.454948]  ? local_clock+0x15/0x30
[   11.454948]  ? lock_release+0x275/0x350
[   11.454948]  ? exc_page_fault+0x74/0x1e0
[   11.454948]  ? asm_exc_page_fault+0x26/0x30
[   11.454948]  ? __pfx_smack_cred_free+0x10/0x10
[   11.454948]  ? smk_destroy_label_list+0x11/0x40
[   11.454948]  smack_cred_free+0x30/0xa0
[   11.454948]  security_cred_free+0x2f/0x60
[   11.454948]  security_prepare_creds+0x2b/0x60
[   11.454948]  prepare_creds+0x1b3/0x2c0
[   11.454948]  copy_creds+0x2e/0x380
[   11.454948]  copy_process+0x47e/0x2080
[   11.454948]  ? find_held_lock+0x34/0xa0
[   11.454948]  kernel_clone+0x9f/0x6c0
[   11.454948]  ? __mutex_unlock_slowpath+0x43/0x2c0
[   11.454948]  ? synchronize_rcu_expedited+0x3fa/0x4e0
[   11.454948]  user_mode_thread+0x5f/0x90
[   11.454948]  ? __pfx_kernel_init+0x10/0x10
[   11.454948]  rest_init+0x22/0x1d0
[   11.454948]  arch_call_rest_init+0xe/0x30
[   11.454948]  start_kernel+0x63e/0x720
[   11.454948]  x86_64_start_reservations+0x18/0x30
[   11.454948]  x86_64_start_kernel+0x91/0xa0
[   11.454948]  secondary_startup_64_no_verify+0x184/0x18b
[   11.454948]  </TASK>
[   11.454948] Modules linked in:
[   11.454948] CR2: 0000000000000000
[   11.454948] ---[ end trace 0000000000000000 ]---
[   11.454948] RIP: 0010:smk_destroy_label_list+0x11/0x40
[   11.454948] Code: 2e 0f 1f 84 00 00 00 00 00 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 66 0f 1f 00 0f 1f 44 00 00 55 48 89 fd 53 48 8b 3f <48> 8b 1f 48 39 fd 74 13 e8 c2 bd e2 ff 48 89 d8 48 89 df 48 8b 1b
[   11.454948] RSP: 0000:ffffffffa3003c50 EFLAGS: 00010286
[   11.454948] RAX: ffffffffa1f763e0 RBX: ffffffffa2f19b00 RCX: 0000000000000100
[   11.454948] RDX: 00000000000000d8 RSI: 0000000000000000 RDI: 0000000000000000
[   11.454948] RBP: ffff8d71c03300c8 R08: 0000000000400dc0 R09: 00000000ffffffff
[   11.454948] R10: ffffffffa3003c48 R11: ffff8d7259ffc220 R12: ffff8d71c0330018
[   11.454948] R13: ffffffffa3003e48 R14: ffff8d71c03c8000 R15: 0000000000000001
[   11.454948] FS:  0000000000000000(0000) GS:ffff8d7259e00000(0000) knlGS:0000000000000000
[   11.454948] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   11.454948] CR2: 0000000000000000 CR3: 000000014e03c000 CR4: 0000000000350ef0
[   11.454948] Kernel panic - not syncing: Fatal exception
[   11.454948] ---[ end Kernel panic - not syncing: Fatal exception ]---

On 2024/01/27 23:36, Tetsuo Handa wrote:
> The cred_free hook might be called without successful cred_prepare
> as long as cred->security != NULL. It seems to me that smack_cred_free()
> is not prepared for that case. Please check.
> 
> prepare_creds() {
>   new = kmem_cache_alloc(cred_jar, GFP_KERNEL);
>   if (!new)
>     return NULL;
>   new->security = NULL;
>   security_prepare_creds(new, old, GFP_KERNEL_ACCOUNT) {
>     int rc = lsm_cred_alloc(new, gfp);
>     if (rc)
>       return rc;
>     rc = call_int_hook(cred_prepare, 0, new, old, gfp) {
>       // cred->security != NULL and one of callbacks returned an error
>     }
>     if (unlikely(rc)) {
>       security_cred_free(new) {
>         call_void_hook(cred_free, cred) {
>           // at least one callback is called without successful cred_prepare
>         }
>         kfree(cred->security);
>         cred->security = NULL;
>       }
>     }
>   }
>   goto error;
> error:
>   abort_creds(new)
>   return NULL;
> }

Re: smack: Possible NULL pointer deref in cred_free hook.

[Casey Schaufler]

On 2/6/2024 6:31 AM, Tetsuo Handa wrote:
> Hello, Casey.
>
> I confirmed using fault injection shown below that smack_cred_free() is not
> prepared for being called without successful smack_cred_prepare().

The failure cases for smack_cred_prepare() result from memory allocation
failures. Since init_task_smack() is called before either of the potential
memory allocations the state of the cred will be safe for smack_cred_free().
The fault you've described here removes the init_task_smack(), which will
always succeed, and which is sufficient to prevent the smack_cred_free()
failure below. Are you suggesting that there is a case where a cred will
be freed without ever having been "prepared"?

>
> diff --git a/security/security.c b/security/security.c
> index 10adc4d3c5e0..690136b60d28 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -3059,7 +3059,7 @@ int security_prepare_creds(struct cred *new, const struct cred *old, gfp_t gfp)
>         if (rc)
>                 return rc;
>
> -       rc = call_int_hook(cred_prepare, 0, new, old, gfp);
> +       rc = -EPERM; //call_int_hook(cred_prepare, 0, new, old, gfp);
>         if (unlikely(rc))
>                 security_cred_free(new);
>         return rc;
>
> [   11.366204] LSM: initializing lsm=capability,yama,smack,integrity
> [   11.368005] Yama: becoming mindful.
> [   11.371997] Smack:  Initializing.
> [   11.373957] Smack:  IPv6 port labeling enabled.
> [   11.379072] Mount-cache hash table entries: 16384 (order: 5, 131072 bytes, linear)
> [   11.383676] Mountpoint-cache hash table entries: 16384 (order: 5, 131072 bytes, linear)
> [   11.400071] Running RCU synchronous self tests
> [   11.450967] Running RCU synchronous self tests
> [   11.453956] BUG: kernel NULL pointer dereference, address: 0000000000000000
> [   11.454948] #PF: supervisor read access in kernel mode
> [   11.454948] #PF: error_code(0x0000) - not-present page
> [   11.454948] PGD 0 P4D 0 
> [   11.454948] Oops: 0000 [#1] PREEMPT SMP PTI
> [   11.454948] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 6.8.0-rc3+ #47
> [   11.454948] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 11/12/2020
> [   11.454948] RIP: 0010:smk_destroy_label_list+0x11/0x40
> [   11.454948] Code: 2e 0f 1f 84 00 00 00 00 00 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 66 0f 1f 00 0f 1f 44 00 00 55 48 89 fd 53 48 8b 3f <48> 8b 1f 48 39 fd 74 13 e8 c2 bd e2 ff 48 89 d8 48 89 df 48 8b 1b
> [   11.454948] RSP: 0000:ffffffffa3003c50 EFLAGS: 00010286
> [   11.454948] RAX: ffffffffa1f763e0 RBX: ffffffffa2f19b00 RCX: 0000000000000100
> [   11.454948] RDX: 00000000000000d8 RSI: 0000000000000000 RDI: 0000000000000000
> [   11.454948] RBP: ffff8d71c03300c8 R08: 0000000000400dc0 R09: 00000000ffffffff
> [   11.454948] R10: ffffffffa3003c48 R11: ffff8d7259ffc220 R12: ffff8d71c0330018
> [   11.454948] R13: ffffffffa3003e48 R14: ffff8d71c03c8000 R15: 0000000000000001
> [   11.454948] FS:  0000000000000000(0000) GS:ffff8d7259e00000(0000) knlGS:0000000000000000
> [   11.454948] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   11.454948] CR2: 0000000000000000 CR3: 000000014e03c000 CR4: 0000000000350ef0
> [   11.454948] Call Trace:
> [   11.454948]  <TASK>
> [   11.454948]  ? __die_body+0x1e/0x60
> [   11.454948]  ? page_fault_oops+0x15b/0x470
> [   11.454948]  ? local_clock+0x15/0x30
> [   11.454948]  ? lock_release+0x275/0x350
> [   11.454948]  ? exc_page_fault+0x74/0x1e0
> [   11.454948]  ? asm_exc_page_fault+0x26/0x30
> [   11.454948]  ? __pfx_smack_cred_free+0x10/0x10
> [   11.454948]  ? smk_destroy_label_list+0x11/0x40
> [   11.454948]  smack_cred_free+0x30/0xa0
> [   11.454948]  security_cred_free+0x2f/0x60
> [   11.454948]  security_prepare_creds+0x2b/0x60
> [   11.454948]  prepare_creds+0x1b3/0x2c0
> [   11.454948]  copy_creds+0x2e/0x380
> [   11.454948]  copy_process+0x47e/0x2080
> [   11.454948]  ? find_held_lock+0x34/0xa0
> [   11.454948]  kernel_clone+0x9f/0x6c0
> [   11.454948]  ? __mutex_unlock_slowpath+0x43/0x2c0
> [   11.454948]  ? synchronize_rcu_expedited+0x3fa/0x4e0
> [   11.454948]  user_mode_thread+0x5f/0x90
> [   11.454948]  ? __pfx_kernel_init+0x10/0x10
> [   11.454948]  rest_init+0x22/0x1d0
> [   11.454948]  arch_call_rest_init+0xe/0x30
> [   11.454948]  start_kernel+0x63e/0x720
> [   11.454948]  x86_64_start_reservations+0x18/0x30
> [   11.454948]  x86_64_start_kernel+0x91/0xa0
> [   11.454948]  secondary_startup_64_no_verify+0x184/0x18b
> [   11.454948]  </TASK>
> [   11.454948] Modules linked in:
> [   11.454948] CR2: 0000000000000000
> [   11.454948] ---[ end trace 0000000000000000 ]---
> [   11.454948] RIP: 0010:smk_destroy_label_list+0x11/0x40
> [   11.454948] Code: 2e 0f 1f 84 00 00 00 00 00 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 66 0f 1f 00 0f 1f 44 00 00 55 48 89 fd 53 48 8b 3f <48> 8b 1f 48 39 fd 74 13 e8 c2 bd e2 ff 48 89 d8 48 89 df 48 8b 1b
> [   11.454948] RSP: 0000:ffffffffa3003c50 EFLAGS: 00010286
> [   11.454948] RAX: ffffffffa1f763e0 RBX: ffffffffa2f19b00 RCX: 0000000000000100
> [   11.454948] RDX: 00000000000000d8 RSI: 0000000000000000 RDI: 0000000000000000
> [   11.454948] RBP: ffff8d71c03300c8 R08: 0000000000400dc0 R09: 00000000ffffffff
> [   11.454948] R10: ffffffffa3003c48 R11: ffff8d7259ffc220 R12: ffff8d71c0330018
> [   11.454948] R13: ffffffffa3003e48 R14: ffff8d71c03c8000 R15: 0000000000000001
> [   11.454948] FS:  0000000000000000(0000) GS:ffff8d7259e00000(0000) knlGS:0000000000000000
> [   11.454948] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   11.454948] CR2: 0000000000000000 CR3: 000000014e03c000 CR4: 0000000000350ef0
> [   11.454948] Kernel panic - not syncing: Fatal exception
> [   11.454948] ---[ end Kernel panic - not syncing: Fatal exception ]---
>
> On 2024/01/27 23:36, Tetsuo Handa wrote:
>> The cred_free hook might be called without successful cred_prepare
>> as long as cred->security != NULL. It seems to me that smack_cred_free()
>> is not prepared for that case. Please check.
>>
>> prepare_creds() {
>>   new = kmem_cache_alloc(cred_jar, GFP_KERNEL);
>>   if (!new)
>>     return NULL;
>>   new->security = NULL;
>>   security_prepare_creds(new, old, GFP_KERNEL_ACCOUNT) {
>>     int rc = lsm_cred_alloc(new, gfp);
>>     if (rc)
>>       return rc;
>>     rc = call_int_hook(cred_prepare, 0, new, old, gfp) {
>>       // cred->security != NULL and one of callbacks returned an error
>>     }
>>     if (unlikely(rc)) {
>>       security_cred_free(new) {
>>         call_void_hook(cred_free, cred) {
>>           // at least one callback is called without successful cred_prepare
>>         }
>>         kfree(cred->security);
>>         cred->security = NULL;
>>       }
>>     }
>>   }
>>   goto error;
>> error:
>>   abort_creds(new)
>>   return NULL;
>> }
>

Re: smack: Possible NULL pointer deref in cred_free hook.

[Tetsuo Handa]

On 2024/02/07 10:39, Casey Schaufler wrote:
> On 2/6/2024 6:31 AM, Tetsuo Handa wrote:
>> Hello, Casey.
>>
>> I confirmed using fault injection shown below that smack_cred_free() is not
>> prepared for being called without successful smack_cred_prepare().
> 
> The failure cases for smack_cred_prepare() result from memory allocation
> failures. Since init_task_smack() is called before either of the potential
> memory allocations the state of the cred will be safe for smack_cred_free().
> The fault you've described here removes the init_task_smack(), which will
> always succeed, and which is sufficient to prevent the smack_cred_free()
> failure below. Are you suggesting that there is a case where a cred will
> be freed without ever having been "prepared"?

Yes. If smack_cred_prepare() is not the first entry of the cred_prepare list
and the first entry of the cred_prepare list failed, smack_cred_prepare()
will not be called (and therefore init_task_smack() will not be called).

Re: smack: Possible NULL pointer deref in cred_free hook.

[Casey Schaufler]

On 2/6/2024 6:54 PM, Tetsuo Handa wrote:
> On 2024/02/07 10:39, Casey Schaufler wrote:
>> On 2/6/2024 6:31 AM, Tetsuo Handa wrote:
>>> Hello, Casey.
>>>
>>> I confirmed using fault injection shown below that smack_cred_free() is not
>>> prepared for being called without successful smack_cred_prepare().
>> The failure cases for smack_cred_prepare() result from memory allocation
>> failures. Since init_task_smack() is called before either of the potential
>> memory allocations the state of the cred will be safe for smack_cred_free().
>> The fault you've described here removes the init_task_smack(), which will
>> always succeed, and which is sufficient to prevent the smack_cred_free()
>> failure below. Are you suggesting that there is a case where a cred will
>> be freed without ever having been "prepared"?
> Yes. If smack_cred_prepare() is not the first entry of the cred_prepare list
> and the first entry of the cred_prepare list failed, smack_cred_prepare()
> will not be called (and therefore init_task_smack() will not be called).

I see your point. Thank you for the insight. This is the first real
case I've seen where the "bail on fail" approach leads to a problem.
Now, on to the fix ...

Re: smack: Possible NULL pointer deref in cred_free hook.

[Casey Schaufler]

On 2/7/2024 10:53 AM, Casey Schaufler wrote:
> On 2/6/2024 6:54 PM, Tetsuo Handa wrote:
>> On 2024/02/07 10:39, Casey Schaufler wrote:
>>> On 2/6/2024 6:31 AM, Tetsuo Handa wrote:
>>>> Hello, Casey.
>>>>
>>>> I confirmed using fault injection shown below that smack_cred_free() is not
>>>> prepared for being called without successful smack_cred_prepare().
>>> The failure cases for smack_cred_prepare() result from memory allocation
>>> failures. Since init_task_smack() is called before either of the potential
>>> memory allocations the state of the cred will be safe for smack_cred_free().
>>> The fault you've described here removes the init_task_smack(), which will
>>> always succeed, and which is sufficient to prevent the smack_cred_free()
>>> failure below. Are you suggesting that there is a case where a cred will
>>> be freed without ever having been "prepared"?
>> Yes. If smack_cred_prepare() is not the first entry of the cred_prepare list
>> and the first entry of the cred_prepare list failed, smack_cred_prepare()
>> will not be called (and therefore init_task_smack() will not be called).

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.

Your "fault injection" is too aggressive. It should return an error
from smack_cred_prepare() after the call to init_task_smack() rather
than commenting out the call entirely.

> I see your point. Thank you for the insight. This is the first real
> case I've seen where the "bail on fail" approach leads to a problem.
> Now, on to the fix ...
>
>

Re: smack: Possible NULL pointer deref in cred_free hook.

[Paul Moore]

On Wed, Feb 14, 2024 at 12:11 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 2/7/2024 10:53 AM, Casey Schaufler wrote:
> > On 2/6/2024 6:54 PM, Tetsuo Handa wrote:
> >> On 2024/02/07 10:39, Casey Schaufler wrote:
> >>> On 2/6/2024 6:31 AM, Tetsuo Handa wrote:
> >>>> Hello, Casey.
> >>>>
> >>>> I confirmed using fault injection shown below that smack_cred_free() is not
> >>>> prepared for being called without successful smack_cred_prepare().
> >>> The failure cases for smack_cred_prepare() result from memory allocation
> >>> failures. Since init_task_smack() is called before either of the potential
> >>> memory allocations the state of the cred will be safe for smack_cred_free().
> >>> The fault you've described here removes the init_task_smack(), which will
> >>> always succeed, and which is sufficient to prevent the smack_cred_free()
> >>> failure below. Are you suggesting that there is a case where a cred will
> >>> be freed without ever having been "prepared"?
> >> Yes. If smack_cred_prepare() is not the first entry of the cred_prepare list
> >> and the first entry of the cred_prepare list failed, smack_cred_prepare()
> >> will not be called (and therefore init_task_smack() will not be called).
>
> 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.

-- 
paul-moore.com

Re: smack: Possible NULL pointer deref in cred_free hook.

[Tetsuo Handa]

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.

Re: smack: Possible NULL pointer deref in cred_free hook.

[Casey Schaufler]

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. 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.

Re: smack: Possible NULL pointer deref in cred_free hook.

[Paul Moore]

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.

> 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?

Looking quickly I suspect this affects Smack and AppArmor.  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.

-- 
paul-moore.com

Re: 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@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.
>

Re: smack: Possible NULL pointer deref in cred_free hook.

[Paul Moore]

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.

-- 
paul-moore.com

Re: smack: Possible NULL pointer deref in cred_free hook.

[Serge E. Hallyn]

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...

Re: 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@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.