Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <c2c6efe8b2903949fb7118b56991988ba9c4f582.camel@linux.ibm.com>
Subject: Re: [PATCH v5 6/7] IMA: add critical_data to the built-in policy
 rules
From:   Mimi Zohar <zohar@linux.ibm.com>
To:     Lakshmi Ramasubramanian <nramas@linux.microsoft.com>,
        Tushar Sugandhi <tusharsu@linux.microsoft.com>,
        stephen.smalley.work@gmail.com, casey@schaufler-ca.com,
        agk@redhat.com, snitzer@redhat.com, gmazyland@gmail.com,
        paul@paul-moore.com
Cc:     tyhicks@linux.microsoft.com, sashal@kernel.org, jmorris@namei.org,
        linux-integrity@vger.kernel.org, selinux@vger.kernel.org,
        linux-security-module@vger.kernel.org,
        linux-kernel@vger.kernel.org, dm-devel@redhat.com
Date:   Sun, 08 Nov 2020 10:46:39 -0500
In-Reply-To: <d92869b5-7244-e29e-5d30-c0e06cf45be1@linux.microsoft.com>
References: <20201101222626.6111-1-tusharsu@linux.microsoft.com>
         <20201101222626.6111-7-tusharsu@linux.microsoft.com>
         <7219f4404bc1bed6eb090b94363c283ec3266a17.camel@linux.ibm.com>
         <cdcd63f7-ce1f-4463-f886-c36832d7a706@linux.microsoft.com>
         <d92869b5-7244-e29e-5d30-c0e06cf45be1@linux.microsoft.com>
Content-Type: text/plain; charset="ISO-8859-15"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Lakshmi,

On Fri, 2020-11-06 at 15:51 -0800, Lakshmi Ramasubramanian wrote:
> 
> >>> diff --git a/security/integrity/ima/ima_policy.c 
> >>> b/security/integrity/ima/ima_policy.c
> >>> index ec99e0bb6c6f..dc8fe969d3fe 100644
> >>> --- a/security/integrity/ima/ima_policy.c
> >>> +++ b/security/integrity/ima/ima_policy.c
> >>
> >>> @@ -875,6 +884,29 @@ void __init ima_init_policy(void)
> >>>                 ARRAY_SIZE(default_appraise_rules),
> >>>                 IMA_DEFAULT_POLICY);
> >>> +    if (ima_use_critical_data) {
> >>> +        template = lookup_template_desc("ima-buf");
> >>> +        if (!template) {
> >>> +            ret = -EINVAL;
> >>> +            goto out;
> >>> +        }
> >>> +
> >>> +        ret = template_desc_init_fields(template->fmt,
> >>> +                        &(template->fields),
> >>> +                        &(template->num_fields));
> >>
> >> The default IMA template when measuring buffer data is "ima_buf".   Is
> >> there a reason for allocating and initializing it here and not
> >> deferring it until process_buffer_measurement()?
> >>
> > 
> > You are right - good catch.
> > I will remove the above and validate.
> > 
> 
> process_buffer_measurement() allocates and initializes "ima-buf" 
> template only when the parameter "func" is NONE. Currently, only 
> ima_check_blacklist() passes NONE for func when calling 
> process_buffer_measurement().
> 
> If "func" is anything other than NONE, ima_match_policy() picks
> the default IMA template if the IMA policy rule does not specify a template.
> 
> We need to add "ima-buf" in the built-in policy for critical_data so 
> that the default template is not used for buffer measurement.
> 
> Please let me know if I am missing something.
> 

Let's explain a bit further what is happening and why.   As you said
ima_get_action() returns the template format, which may be the default
IMA template or the specific IMA policy rule template format.  This
works properly for both the arch specific and custom policies, but not
for builtin policies, because the policy rules may contain a rule
specific .template field.   When the rules don't contain a rule
specific template field, they default to the IMA default template.  In
the case of builtin policies, the policy rules cannot contain the
.template field.

The default template field for process_buffer_measurement() should
always be "ima-buf", not the default IMA template format.   Let's fix
this prior to this patch.

Probably something like this:
- In addition to initializing the default IMA template, initialize the
"ima-buf" template.  Maybe something similiar to
ima_template_desc_current().
- Set the default in process_buffer_measurement() to "ima-buf", before
calling ima_get_action().
- modify ima_match_policy() so that the default policy isn't reset when
already specified.

thanks,

Mimi


> >>
> >>> +        if (ret)
> >>> +            goto out;
> >>> +
> >>> +        critical_data_rules[0].template = template;
> >>> +        add_rules(critical_data_rules,
> >>> +              ARRAY_SIZE(critical_data_rules),
> >>> +              IMA_DEFAULT_POLICY);
> >>> +    }
> >>> +
> >>> +out:
> >>> +    if (ret)
> >>> +        pr_err("%s failed, result: %d\n", __func__, ret);
> >>> +
> >>>       ima_update_policy_flag();
> >>>   }
> >>
> > 
>