Re: [PATCH 1/1] Smack:- Fix the issue of wrong info printed in ptrace error logs

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

On 12/20/2021 8:41 AM, Casey Schaufler wrote:
> On 12/20/2021 2:13 AM, Vishal Goel wrote:
>> Currently tracer process info is printed in object field in
>> smack error log for ptrace check which is wrong.
>> Object process should print the tracee process info.
>> Tracee info is not printed in the smack error logs.
>> So it is not possible to debug the ptrace smack issues.
>>
>> Now changes has been done to print both tracer and tracee
>> process info in smack error logs for ptrace scenarios
>>
>> Old logs:-
>> [  378.098330] audit: type=1400 audit(1637212273.300:2): lsm=SMACK fn=smack_ptrace_access_check action=denied subject="Tracer_lbl" object="Tracee_lbl" requested= pid=9397 comm="tst_pt" opid=9397 ocomm="tst_pt"
>> [  520.261605] audit: type=1400 audit(1637212415.464:3): lsm=SMACK fn=smack_ptrace_traceme action=denied subject="Tracer_lbl" object="Tracee_lbl" requested= pid=12685 comm="tst_pt_me" opid=12563 ocomm="bash"
>> [ 1445.259319] audit: type=1400 audit(1637213340.460:5): lsm=SMACK fn=smack_bprm_set_creds action=denied subject="Tracer_lbl" object="Tracee_lbl" requested= pid=1778 comm="tst_bprm" opid=1776 ocomm="tst_bprm"
>>
>> New logs:-
>> [  378.098330] audit: type=1400 audit(1637212273.300:2): lsm=SMACK fn=smack_ptrace_access_check action=denied subject="Tracer_lbl" object="Tracee_lbl" requested= tracer-pid=5189 tracer-comm="tst_pt" pid=5189 comm="tst_pt" tracee-pid=962 tracee-comm="test_tracee"
>> [  520.261605] audit: type=1400 audit(1637212415.464:3): lsm=SMACK fn=smack_ptrace_traceme action=denied subject="Tracer_lbl" object="Tracee_lbl" requested= tracer-pid=6161 tracer-comm="bash" pid=6310 comm="tst_pt_me" tracee-pid=6310 tracee-comm="tst_pt_me"
>> [ 1445.259319] audit: type=1400 audit(1637213340.460:5): lsm=SMACK fn=smack_bprm_set_creds action=denied subject="Tracer_lbl" object="Tracee_lbl" requested= tracer-pid=6435 tracer-comm="tst_bprm" pid=6436 comm="tst_bprm" tracee-pid=6436 tracee-comm="tst_bprm"
>>
>> Signed-off-by: Vishal Goel <vishal.goel@samsung.com>

What test case do you have that generates these records?

>
> Added linux-audit to the CC list.
>
>> ---
>>   include/linux/lsm_audit.h     |  1 +
>>   security/lsm_audit.c          | 15 +++++++++++++++
>>   security/smack/smack.h        | 19 +++++++++++++++++++
>>   security/smack/smack_access.c | 16 ++++++++++++++++
>>   security/smack/smack_lsm.c    | 33 ++++++++++++++++++++++-----------
>>   5 files changed, 73 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/linux/lsm_audit.h b/include/linux/lsm_audit.h
>> index 17d02eda9..6d752ea16 100644
>> --- a/include/linux/lsm_audit.h
>> +++ b/include/linux/lsm_audit.h
>> @@ -76,6 +76,7 @@ struct common_audit_data {
>>   #define LSM_AUDIT_DATA_IBENDPORT 14
>>   #define LSM_AUDIT_DATA_LOCKDOWN 15
>>   #define LSM_AUDIT_DATA_NOTIFICATION 16
>> +#define LSM_AUDIT_DATA_PTRACE   17
>>       union     {
>>           struct path path;
>>           struct dentry *dentry;
>> diff --git a/security/lsm_audit.c b/security/lsm_audit.c
>> index 1897cbf6f..069e0282c 100644
>> --- a/security/lsm_audit.c
>> +++ b/security/lsm_audit.c
>> @@ -318,6 +318,21 @@ static void dump_common_audit_data(struct audit_buffer *ab,
>>           }
>>           break;
>>       }
>> +    case LSM_AUDIT_DATA_PTRACE: {
>> +        struct task_struct *tsk = a->u.tsk;
>> +        if (tsk) {
>> +            pid_t pid = task_tgid_nr(tsk);
>> +
>> +            if (pid) {
>> +                char comm[sizeof(tsk->comm)];
>> +
>> +                audit_log_format(ab, " tracee-pid=%d tracee-comm=", pid);
>> +                audit_log_untrustedstring(ab,
>> +                    memcpy(comm, tsk->comm, sizeof(comm)));
>> +            }
>> +        }
>> +        break;
>> +    }
>>       case LSM_AUDIT_DATA_NET:
>>           if (a->u.net->sk) {
>>               const struct sock *sk = a->u.net->sk;
>> diff --git a/security/smack/smack.h b/security/smack/smack.h
>> index 99c342259..901228205 100644
>> --- a/security/smack/smack.h
>> +++ b/security/smack/smack.h
>> @@ -266,6 +266,7 @@ struct smack_audit_data {
>>       char *object;
>>       char *request;
>>       int result;
>> +    struct task_struct *tracer_tsk;
>>   };
>>     /*
>> @@ -497,6 +498,16 @@ static inline void smk_ad_setfield_u_net_sk(struct smk_audit_info *a,
>>   {
>>       a->a.u.net->sk = sk;
>>   }
>> +static inline void smk_ad_setfield_u_tracer(struct smk_audit_info *a,
>> +                                        struct task_struct *t)
>> +{
>> +       a->a.smack_audit_data->tracer_tsk = t;
>> +}
>> +static inline void smk_ad_setfield_u_tracee(struct smk_audit_info *a,
>> +                                        struct task_struct *t)
>> +{
>> +       a->a.u.tsk = t;
>> +}
>>     #else /* no AUDIT */
>>   @@ -524,6 +535,14 @@ static inline void smk_ad_setfield_u_net_sk(struct smk_audit_info *a,
>>                           struct sock *sk)
>>   {
>>   }
>> +static inline void smk_ad_setfield_u_tracer(struct smk_audit_info *a,
>> +                        struct task_struct *t)
>> +{
>> +}
>> +static inline void smk_ad_setfield_u_tracee(struct smk_audit_info *a,
>> +                        struct task_struct *t)
>> +{
>> +}
>>   #endif
>>     #endif  /* _SECURITY_SMACK_H */
>> diff --git a/security/smack/smack_access.c b/security/smack/smack_access.c
>> index d2186e275..f39e3ac92 100644
>> --- a/security/smack/smack_access.c
>> +++ b/security/smack/smack_access.c
>> @@ -323,6 +323,22 @@ static void smack_log_callback(struct audit_buffer *ab, void *a)
>>           audit_log_format(ab, " labels_differ");
>>       else
>>           audit_log_format(ab, " requested=%s", sad->request);
>> +
>> +        if (ad->type == LSM_AUDIT_DATA_PTRACE) {
>> +                struct task_struct *tsk = sad->tracer_tsk;
>> +
>> +                if (tsk) {
>> +                        pid_t pid = task_tgid_nr(tsk);
>> +
>> +                        if (pid) {
>> +                                char comm[sizeof(tsk->comm)];
>> +
>> +                                audit_log_format(ab, " tracer-pid=%d tracer-comm=", pid);
>> +                                audit_log_untrustedstring(ab,
>> +                                        memcpy(comm, tsk->comm, sizeof(comm)));
>> +                        }
>> +                }
>> +    }
>>   }
>>     /**
>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>> index efd35b07c..47e8a9461 100644
>> --- a/security/smack/smack_lsm.c
>> +++ b/security/smack/smack_lsm.c
>> @@ -416,20 +416,13 @@ static inline unsigned int smk_ptrace_mode(unsigned int mode)
>>    */
>>   static int smk_ptrace_rule_check(struct task_struct *tracer,
>>                    struct smack_known *tracee_known,
>> -                 unsigned int mode, const char *func)
>> +                 unsigned int mode, struct smk_audit_info *saip)
>>   {
>>       int rc;
>> -    struct smk_audit_info ad, *saip = NULL;
>>       struct task_smack *tsp;
>>       struct smack_known *tracer_known;
>>       const struct cred *tracercred;
>>   -    if ((mode & PTRACE_MODE_NOAUDIT) == 0) {
>> -        smk_ad_init(&ad, func, LSM_AUDIT_DATA_TASK);
>> -        smk_ad_setfield_u_tsk(&ad, tracer);
>> -        saip = &ad;
>> -    }
>> -
>>       rcu_read_lock();
>>       tracercred = __task_cred(tracer);
>>       tsp = smack_cred(tracercred);
>> @@ -480,10 +473,17 @@ static int smk_ptrace_rule_check(struct task_struct *tracer,
>>   static int smack_ptrace_access_check(struct task_struct *ctp, unsigned int mode)
>>   {
>>       struct smack_known *skp;
>> +    struct smk_audit_info ad, *saip = NULL;
>>         skp = smk_of_task_struct_obj(ctp);
>> +    if ((mode & PTRACE_MODE_NOAUDIT) == 0) {
>> +        smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_PTRACE);
>> +        smk_ad_setfield_u_tracer(&ad, current);
>> +        smk_ad_setfield_u_tracee(&ad, ctp);
>> +        saip = &ad;
>> +    }
>>   -    return smk_ptrace_rule_check(current, skp, mode, __func__);
>> +    return smk_ptrace_rule_check(current, skp, mode, saip);
>>   }
>>     /**
>> @@ -498,10 +498,15 @@ static int smack_ptrace_traceme(struct task_struct *ptp)
>>   {
>>       int rc;
>>       struct smack_known *skp;
>> +    struct smk_audit_info ad, *saip = NULL;
>>         skp = smk_of_task(smack_cred(current_cred()));
>> +    smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_PTRACE);
>> +    smk_ad_setfield_u_tracer(&ad, ptp);
>> +    smk_ad_setfield_u_tracee(&ad, current);
>> +    saip = &ad;
>>   -    rc = smk_ptrace_rule_check(ptp, skp, PTRACE_MODE_ATTACH, __func__);
>> +    rc = smk_ptrace_rule_check(ptp, skp, PTRACE_MODE_ATTACH, saip);
>>       return rc;
>>   }
>>   @@ -897,15 +902,21 @@ static int smack_bprm_creds_for_exec(struct linux_binprm *bprm)
>>         if (bprm->unsafe & LSM_UNSAFE_PTRACE) {
>>           struct task_struct *tracer;
>> +        struct smk_audit_info ad, *saip = NULL;
>>           rc = 0;
>>   +        smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_PTRACE);
>> +        smk_ad_setfield_u_tracee(&ad, current);
>> +        saip = &ad;
>> +
>>           rcu_read_lock();
>>           tracer = ptrace_parent(current);
>> +        smk_ad_setfield_u_tracer(&ad, tracer);
>>           if (likely(tracer != NULL))
>>               rc = smk_ptrace_rule_check(tracer,
>>                              isp->smk_task,
>>                              PTRACE_MODE_ATTACH,
>> -                           __func__);
>> +                           saip);
>>           rcu_read_unlock();
>>             if (rc != 0)