Re: [PATCH v7 4/8] IMA: add policy rule to measure critical data

[Tushar Sugandhi]

On 2020-12-10 3:10 p.m., Tyler Hicks wrote:
> On 2020-12-09 11:42:08, Tushar Sugandhi wrote:
> > A new IMA policy rule is needed for the IMA hook
> > ima_measure_critical_data() and the corresponding func CRITICAL_DATA for
> > measuring the input buffer. The policy rule should ensure the buffer
> > would get measured only when the policy rule allows the action. The
> > policy rule should also support the necessary constraints (flags etc.)
> > for integrity critical buffer data measurements.
> >
> > Add a policy rule to define the constraints for restricting integrity
> > critical data measurements.
> >
> > Signed-off-by: Tushar Sugandhi <tusharsu@xxxxxxxxxxxxxxxxxxx>
> > ---
> >   security/integrity/ima/ima_policy.c | 35 +++++++++++++++++++++++++----
> >   1 file changed, 31 insertions(+), 4 deletions(-)
> >
> > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > index 2a0c0603626e..9a8ee80a3128 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -34,6 +34,7 @@
> >   #define IMA_PCR		0x0100
> >   #define IMA_FSNAME	0x0200
> >   #define IMA_KEYRINGS	0x0400
> > +#define IMA_DATA_SOURCE	0x0800
>
> You introduce data_source= in the next patch. This macro shouldn't be
> added until the next patch.
Ok I will move IMA_DATA_SOURCE to the next patch.

> >   
> >   #define UNKNOWN		0
> >   #define MEASURE		0x0001	/* same as IMA_MEASURE */
> > @@ -85,6 +86,7 @@ struct ima_rule_entry {
> >   	} lsm[MAX_LSM_RULES];
> >   	char *fsname;
> >   	struct ima_rule_opt_list *keyrings; /* Measure keys added to these keyrings */
> > +	struct ima_rule_opt_list *data_source; /* Measure data from this source */
> >   	struct ima_template_desc *template;
> >   };
> >   
> > @@ -479,6 +481,12 @@ static bool ima_match_rule_data(struct ima_rule_entry *rule,
> >   		else
> >   			opt_list = rule->keyrings;
> >   		break;
> > +	case CRITICAL_DATA:
> > +		if (!rule->data_source)
> > +			return true;
> > +		else
> > +			opt_list = rule->data_source;
>
> If you take my suggestions on patch #1, remove the else and simply
> assign opt_list here, too.
Yup. Will do.
> > +		break;
> >   	default:
> >   		break;
> >   	}
> > @@ -518,13 +526,19 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
> >   {
> >   	int i;
> >   
> > -	if (func == KEY_CHECK) {
> > -		return (rule->flags & IMA_FUNC) && (rule->func == func) &&
> > -			ima_match_rule_data(rule, func_data, cred);
> > -	}
> >   	if ((rule->flags & IMA_FUNC) &&
> >   	    (rule->func != func && func != POST_SETATTR))
> >   		return false;
> > +
> > +	switch (func) {
> > +	case KEY_CHECK:
> > +	case CRITICAL_DATA:
> > +		return ((rule->func == func) &&
> > +			ima_match_rule_data(rule, func_data, cred));
> > +	default:
> > +		break;
> > +	}
> > +
> >   	if ((rule->flags & IMA_MASK) &&
> >   	    (rule->mask != mask && func != POST_SETATTR))
> >   		return false;
> > @@ -1119,6 +1133,19 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> >   		if (ima_rule_contains_lsm_cond(entry))
> >   			return false;
> >   
> > +		break;
> > +	case CRITICAL_DATA:
> > +		if (entry->action & ~(MEASURE | DONT_MEASURE))
> > +			return false;
> > +
> > +		if (!(entry->flags & IMA_DATA_SOURCE) ||
> > +		    (entry->flags & ~(IMA_FUNC | IMA_UID | IMA_PCR |
> > +		    IMA_DATA_SOURCE)))
>
> IMA_DATA_SOURCE shouldn't exist in this patch. This isn't the right
> indentation, either. See how IMA_KEYRINGS is indented in the KEY_CHECK
> case above.
Will do.
~Tushar
> Tyler
>
> > +			return false;
> > +
> > +		if (ima_rule_contains_lsm_cond(entry))
> > +			return false;
> > +
> >   		break;
> >   	default:
> >   		return false;
> > --
> > 2.17.1