[v3,07/12] ima: Fail rule parsing when appraise_flag=blacklist is unsupportable
diff mbox series

Message ID 20200709061911.954326-8-tyhicks@linux.microsoft.com
State New
Headers show
Series
  • ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support
Related show

Commit Message

Tyler Hicks July 9, 2020, 6:19 a.m. UTC
The "appraise_flag" option is only appropriate for appraise actions
and its "blacklist" value is only appropriate when
CONFIG_IMA_APPRAISE_MODSIG is enabled and "appraise_flag=blacklist" is
only appropriate when "appraise_type=imasig|modsig" is also present.
Make this clear at policy load so that IMA policy authors don't assume
that other uses of "appraise_flag=blacklist" are supported.

Fixes: 273df864cf74 ("ima: Check against blacklisted hashes for files with modsig")
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Cc: Nayna Jain <nayna@linux.ibm.com>
---

* v3
  - New patch

 security/integrity/ima/ima_policy.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Comments

Mimi Zohar July 16, 2020, 6:14 p.m. UTC | #1
On Thu, 2020-07-09 at 01:19 -0500, Tyler Hicks wrote:
> The "appraise_flag" option is only appropriate for appraise actions
> and its "blacklist" value is only appropriate when
> CONFIG_IMA_APPRAISE_MODSIG is enabled and "appraise_flag=blacklist" is
> only appropriate when "appraise_type=imasig|modsig" is also present.
> Make this clear at policy load so that IMA policy authors don't assume
> that other uses of "appraise_flag=blacklist" are supported.

The code looks correct, but this patch description could be written at
a higher level.  Perhaps it just needs to be prefixed with something
like this:

Verifying that a file hash is not blacklisted is currently only
supported for files with appended signatures (modsig).  In the future,
this might change.  For now, ...

Mimi

> 
> Fixes: 273df864cf74 ("ima: Check against blacklisted hashes for files with modsig")
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> Cc: Nayna Jain <nayna@linux.ibm.com>

> ---
> 
> * v3
>   - New patch
> 
>  security/integrity/ima/ima_policy.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 81da02071d41..9842e2e0bc6d 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -1035,6 +1035,11 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
>  		return false;
>  	}
>  
> +	/* Ensure that combinations of flags are compatible with each other */
> +	if (entry->flags & IMA_CHECK_BLACKLIST &&
> +	    !(entry->flags & IMA_MODSIG_ALLOWED))
> +		return false;
> +
>  	return true;
>  }
>  
> @@ -1371,8 +1376,14 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>  				result = -EINVAL;
>  			break;
>  		case Opt_appraise_flag:
> +			if (entry->action != APPRAISE) {
> +				result = -EINVAL;
> +				break;
> +			}
> +
>  			ima_log_string(ab, "appraise_flag", args[0].from);
> -			if (strstr(args[0].from, "blacklist"))
> +			if (IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG) &&
> +			    strstr(args[0].from, "blacklist"))
>  				entry->flags |= IMA_CHECK_BLACKLIST;
>  			break;
>  		case Opt_permit_directio:
Tyler Hicks July 16, 2020, 6:20 p.m. UTC | #2
On 2020-07-16 14:14:50, Mimi Zohar wrote:
> On Thu, 2020-07-09 at 01:19 -0500, Tyler Hicks wrote:
> > The "appraise_flag" option is only appropriate for appraise actions
> > and its "blacklist" value is only appropriate when
> > CONFIG_IMA_APPRAISE_MODSIG is enabled and "appraise_flag=blacklist" is
> > only appropriate when "appraise_type=imasig|modsig" is also present.
> > Make this clear at policy load so that IMA policy authors don't assume
> > that other uses of "appraise_flag=blacklist" are supported.
> 
> The code looks correct, but this patch description could be written at
> a higher level.  Perhaps it just needs to be prefixed with something
> like this:
> 
> Verifying that a file hash is not blacklisted is currently only
> supported for files with appended signatures (modsig).  In the future,
> this might change.  For now, ...

That makes sense. I'm not up to speed on the intent behind the blacklist
feature or where it may go in the future so I didn't think to add
anything along those lines.

If you are happy with the rest of the series, please feel free to append
this to the commit message. Otherwise, I can add it if I need to submit
a new revision of the series.

Tyler

> 
> Mimi
> 
> > 
> > Fixes: 273df864cf74 ("ima: Check against blacklisted hashes for files with modsig")
> > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> > Cc: Nayna Jain <nayna@linux.ibm.com>
> 
> > ---
> > 
> > * v3
> >   - New patch
> > 
> >  security/integrity/ima/ima_policy.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > index 81da02071d41..9842e2e0bc6d 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -1035,6 +1035,11 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> >  		return false;
> >  	}
> >  
> > +	/* Ensure that combinations of flags are compatible with each other */
> > +	if (entry->flags & IMA_CHECK_BLACKLIST &&
> > +	    !(entry->flags & IMA_MODSIG_ALLOWED))
> > +		return false;
> > +
> >  	return true;
> >  }
> >  
> > @@ -1371,8 +1376,14 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
> >  				result = -EINVAL;
> >  			break;
> >  		case Opt_appraise_flag:
> > +			if (entry->action != APPRAISE) {
> > +				result = -EINVAL;
> > +				break;
> > +			}
> > +
> >  			ima_log_string(ab, "appraise_flag", args[0].from);
> > -			if (strstr(args[0].from, "blacklist"))
> > +			if (IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG) &&
> > +			    strstr(args[0].from, "blacklist"))
> >  				entry->flags |= IMA_CHECK_BLACKLIST;
> >  			break;
> >  		case Opt_permit_directio:
Tyler Hicks July 17, 2020, 6:11 p.m. UTC | #3
On 2020-07-17 13:40:22, Nayna wrote:
> 
> On 7/9/20 2:19 AM, Tyler Hicks wrote:
> > The "appraise_flag" option is only appropriate for appraise actions
> > and its "blacklist" value is only appropriate when
> > CONFIG_IMA_APPRAISE_MODSIG is enabled and "appraise_flag=blacklist" is
> > only appropriate when "appraise_type=imasig|modsig" is also present.
> > Make this clear at policy load so that IMA policy authors don't assume
> > that other uses of "appraise_flag=blacklist" are supported.
> > 
> > Fixes: 273df864cf74 ("ima: Check against blacklisted hashes for files with modsig")
> > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> > Cc: Nayna Jain <nayna@linux.ibm.com>
> > ---
> > 
> > * v3
> >    - New patch
> > 
> >   security/integrity/ima/ima_policy.c | 13 ++++++++++++-
> >   1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > index 81da02071d41..9842e2e0bc6d 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -1035,6 +1035,11 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> >   		return false;
> >   	}
> > +	/* Ensure that combinations of flags are compatible with each other */
> > +	if (entry->flags & IMA_CHECK_BLACKLIST &&
> > +	    !(entry->flags & IMA_MODSIG_ALLOWED))
> > +		return false;
> > +
> >   	return true;
> >   }
> > @@ -1371,8 +1376,14 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
> >   				result = -EINVAL;
> >   			break;
> >   		case Opt_appraise_flag:
> > +			if (entry->action != APPRAISE) {
> > +				result = -EINVAL;
> > +				break;
> > +			}
> > +
> >   			ima_log_string(ab, "appraise_flag", args[0].from);
> > -			if (strstr(args[0].from, "blacklist"))
> > +			if (IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG) &&
> > +			    strstr(args[0].from, "blacklist"))
> >   				entry->flags |= IMA_CHECK_BLACKLIST;
> 
> If IMA_APPRAISE_MODSIG is disabled, it will allow the following rule to
> load, which is not as expected.
> 
> "appraise func=xxx_CHECK appraise_flag=blacklist appraise_type=imasig"
> 
> Missing is the "else" condition to immediately reject the policy rule.

Thanks for the review. You're right. This change is needed:

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 9842e2e0bc6d..cf3ddb38dfa8 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1385,6 +1385,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 			if (IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG) &&
 			    strstr(args[0].from, "blacklist"))
 				entry->flags |= IMA_CHECK_BLACKLIST;
+			else
+				result = -EINVAL;
 			break;
 		case Opt_permit_directio:
 			entry->flags |= IMA_PERMIT_DIRECTIO;


Making this change does not conflict with any later patches in the
series.

Mimi, I've rebased and force pushed to my fixup branch with this change,
for your comparison:

 https://git.kernel.org/pub/scm/linux/kernel/git/tyhicks/linux.git/log/?h=next-integrity-testing-fixup

Tyler

> 
> Thanks & Regards,
> 
>      - Nayna
>
Nayna July 20, 2020, 5:02 p.m. UTC | #4
On 7/17/20 2:11 PM, Tyler Hicks wrote:
> On 2020-07-17 13:40:22, Nayna wrote:
>> On 7/9/20 2:19 AM, Tyler Hicks wrote:
>>> The "appraise_flag" option is only appropriate for appraise actions
>>> and its "blacklist" value is only appropriate when
>>> CONFIG_IMA_APPRAISE_MODSIG is enabled and "appraise_flag=blacklist" is
>>> only appropriate when "appraise_type=imasig|modsig" is also present.
>>> Make this clear at policy load so that IMA policy authors don't assume
>>> that other uses of "appraise_flag=blacklist" are supported.
>>>
>>> Fixes: 273df864cf74 ("ima: Check against blacklisted hashes for files with modsig")
>>> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
>>> Cc: Nayna Jain <nayna@linux.ibm.com>
>>> ---
>>>
>>> * v3
>>>     - New patch
>>>
>>>    security/integrity/ima/ima_policy.c | 13 ++++++++++++-
>>>    1 file changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
>>> index 81da02071d41..9842e2e0bc6d 100644
>>> --- a/security/integrity/ima/ima_policy.c
>>> +++ b/security/integrity/ima/ima_policy.c
>>> @@ -1035,6 +1035,11 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
>>>    		return false;
>>>    	}
>>> +	/* Ensure that combinations of flags are compatible with each other */
>>> +	if (entry->flags & IMA_CHECK_BLACKLIST &&
>>> +	    !(entry->flags & IMA_MODSIG_ALLOWED))
>>> +		return false;
>>> +
>>>    	return true;
>>>    }
>>> @@ -1371,8 +1376,14 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>>>    				result = -EINVAL;
>>>    			break;
>>>    		case Opt_appraise_flag:
>>> +			if (entry->action != APPRAISE) {
>>> +				result = -EINVAL;
>>> +				break;
>>> +			}
>>> +
>>>    			ima_log_string(ab, "appraise_flag", args[0].from);
>>> -			if (strstr(args[0].from, "blacklist"))
>>> +			if (IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG) &&
>>> +			    strstr(args[0].from, "blacklist"))
>>>    				entry->flags |= IMA_CHECK_BLACKLIST;
>> If IMA_APPRAISE_MODSIG is disabled, it will allow the following rule to
>> load, which is not as expected.
>>
>> "appraise func=xxx_CHECK appraise_flag=blacklist appraise_type=imasig"
>>
>> Missing is the "else" condition to immediately reject the policy rule.
> Thanks for the review. You're right. This change is needed:
>
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 9842e2e0bc6d..cf3ddb38dfa8 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -1385,6 +1385,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>   			if (IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG) &&
>   			    strstr(args[0].from, "blacklist"))
>   				entry->flags |= IMA_CHECK_BLACKLIST;
> +			else
> +				result = -EINVAL;
>   			break;
>   		case Opt_permit_directio:
>   			entry->flags |= IMA_PERMIT_DIRECTIO;
>
Reviewed-by: Nayna Jain<nayna@linux.ibm.com>

Tested-by: Nayna Jain<nayna@linux.ibm.com>

Thanks & Regards,

       - Nayna

Patch
diff mbox series

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 81da02071d41..9842e2e0bc6d 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1035,6 +1035,11 @@  static bool ima_validate_rule(struct ima_rule_entry *entry)
 		return false;
 	}
 
+	/* Ensure that combinations of flags are compatible with each other */
+	if (entry->flags & IMA_CHECK_BLACKLIST &&
+	    !(entry->flags & IMA_MODSIG_ALLOWED))
+		return false;
+
 	return true;
 }
 
@@ -1371,8 +1376,14 @@  static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 				result = -EINVAL;
 			break;
 		case Opt_appraise_flag:
+			if (entry->action != APPRAISE) {
+				result = -EINVAL;
+				break;
+			}
+
 			ima_log_string(ab, "appraise_flag", args[0].from);
-			if (strstr(args[0].from, "blacklist"))
+			if (IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG) &&
+			    strstr(args[0].from, "blacklist"))
 				entry->flags |= IMA_CHECK_BLACKLIST;
 			break;
 		case Opt_permit_directio: