[v6,25/25] security: Enforce ordering of 'ima' and 'evm' LSMs

Commit Message

Roberto Sassu Nov. 20, 2023, 5:33 p.m. UTC

From: Roberto Sassu <[e-mail]>

The ordering of LSM_ORDER_LAST LSMs depends on how they are placed in the
.lsm_info.init section of the kernel image.

Without making any assumption on the LSM ordering based on how they are
compiled, enforce that ordering at LSM infrastructure level.

Signed-off-by: Roberto Sassu <[e-mail]>
---
 security/security.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

Comments

Casey Schaufler Nov. 21, 2023, 12:50 a.m. UTC | #1

On 11/20/2023 9:33 AM, Roberto Sassu wrote:
> From: Roberto Sassu <[e-mail]>
>
> The ordering of LSM_ORDER_LAST LSMs depends on how they are placed in the
> .lsm_info.init section of the kernel image.
>
> Without making any assumption on the LSM ordering based on how they are
> compiled, enforce that ordering at LSM infrastructure level.
>
> Signed-off-by: Roberto Sassu <[e-mail]>
> ---
>  security/security.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/security/security.c b/security/security.c
> index 351a124b771c..b98db79ca500 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -263,6 +263,18 @@ static void __init initialize_lsm(struct lsm_info *lsm)
>  	}
>  }
>  
> +/* Find an LSM with a given name. */
> +static struct lsm_info __init *find_lsm(const char *name)
> +{
> +	struct lsm_info *lsm;
> +
> +	for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++)
> +		if (!strcmp(lsm->name, name))
> +			return lsm;
> +
> +	return NULL;
> +}
> +
>  /*
>   * Current index to use while initializing the lsm id list.
>   */
> @@ -333,10 +345,23 @@ static void __init ordered_lsm_parse(const char *order, const char *origin)
>  
>  	/* LSM_ORDER_LAST is always last. */
>  	for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) {
> +		/* Do it later, to enforce the expected ordering. */
> +		if (!strcmp(lsm->name, "ima") || !strcmp(lsm->name, "evm"))
> +			continue;
> +

Hard coding the ordering of LSMs is incredibly ugly and unlikely to scale.
Not to mention perplexing the next time someone creates an LSM that "has to be last".

Why isn't LSM_ORDER_LAST sufficient? If it really isn't, how about adding
and using LSM_ORDER_LAST_I_REALLY_MEAN_IT* ?

Alternatively, a declaration of ordering requirements with regard to other
LSMs in lsm_info. You probably don't care where ima is relative to Yama,
but you need to be after SELinux and before evm. lsm_info could have 
must_precede and must_follow lists. Maybe a must_not_combine list, too,
although I'm hoping to make that unnecessary. 

And you should be using LSM_ID values instead of LSM names.

---
* Naming subject to Paul's sensibilities, of course.

>  		if (lsm->order == LSM_ORDER_LAST)
>  			append_ordered_lsm(lsm, "   last");
>  	}
>  
> +	/* Ensure that the 'ima' and 'evm' LSMs are last and in this order. */
> +	lsm = find_lsm("ima");
> +	if (lsm)
> +		append_ordered_lsm(lsm, "   last");
> +
> +	lsm = find_lsm("evm");
> +	if (lsm)
> +		append_ordered_lsm(lsm, "   last");
> +
>  	/* Disable all LSMs not in the ordered list. */
>  	for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) {
>  		if (exists_ordered_lsm(lsm))

Roberto Sassu Nov. 21, 2023, 7:57 a.m. UTC | #2

On Mon, 2023-11-20 at 16:50 -0800, Casey Schaufler wrote:
> On 11/20/2023 9:33 AM, Roberto Sassu wrote:
> > From: Roberto Sassu <[e-mail]>
> > 
> > The ordering of LSM_ORDER_LAST LSMs depends on how they are placed in the
> > .lsm_info.init section of the kernel image.
> > 
> > Without making any assumption on the LSM ordering based on how they are
> > compiled, enforce that ordering at LSM infrastructure level.
> > 
> > Signed-off-by: Roberto Sassu <[e-mail]>
> > ---
> >  security/security.c | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> > 
> > diff --git a/security/security.c b/security/security.c
> > index 351a124b771c..b98db79ca500 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -263,6 +263,18 @@ static void __init initialize_lsm(struct lsm_info *lsm)
> >  	}
> >  }
> >  
> > +/* Find an LSM with a given name. */
> > +static struct lsm_info __init *find_lsm(const char *name)
> > +{
> > +	struct lsm_info *lsm;
> > +
> > +	for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++)
> > +		if (!strcmp(lsm->name, name))
> > +			return lsm;
> > +
> > +	return NULL;
> > +}
> > +
> >  /*
> >   * Current index to use while initializing the lsm id list.
> >   */
> > @@ -333,10 +345,23 @@ static void __init ordered_lsm_parse(const char *order, const char *origin)
> >  
> >  	/* LSM_ORDER_LAST is always last. */
> >  	for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) {
> > +		/* Do it later, to enforce the expected ordering. */
> > +		if (!strcmp(lsm->name, "ima") || !strcmp(lsm->name, "evm"))
> > +			continue;
> > +
> 
> Hard coding the ordering of LSMs is incredibly ugly and unlikely to scale.
> Not to mention perplexing the next time someone creates an LSM that "has to be last".

Uhm, yes, not the best solution.

> Why isn't LSM_ORDER_LAST sufficient? If it really isn't, how about adding
> and using LSM_ORDER_LAST_I_REALLY_MEAN_IT* ?

I don't know if the order at run-time reflects the order in the
Makefile (EVM is compiled after IMA). If it does, there is no need for
this patch.

> Alternatively, a declaration of ordering requirements with regard to other
> LSMs in lsm_info. You probably don't care where ima is relative to Yama,
> but you need to be after SELinux and before evm. lsm_info could have 
> must_precede and must_follow lists. Maybe a must_not_combine list, too,
> although I'm hoping to make that unnecessary. 

Uhm, I agree. Will think about how to make it more straightforward.

> And you should be using LSM_ID values instead of LSM names.

Ok.

Thanks

Roberto

> ---
> * Naming subject to Paul's sensibilities, of course.
> 
> >  		if (lsm->order == LSM_ORDER_LAST)
> >  			append_ordered_lsm(lsm, "   last");
> >  	}
> >  
> > +	/* Ensure that the 'ima' and 'evm' LSMs are last and in this order. */
> > +	lsm = find_lsm("ima");
> > +	if (lsm)
> > +		append_ordered_lsm(lsm, "   last");
> > +
> > +	lsm = find_lsm("evm");
> > +	if (lsm)
> > +		append_ordered_lsm(lsm, "   last");
> > +
> >  	/* Disable all LSMs not in the ordered list. */
> >  	for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) {
> >  		if (exists_ordered_lsm(lsm))

Patch

diff --git a/security/security.c b/security/security.c
index 351a124b771c..b98db79ca500 100644
--- a/security/security.c
+++ b/security/security.c
@@ -263,6 +263,18 @@  static void __init initialize_lsm(struct lsm_info *lsm)
 	}
 }
 
+/* Find an LSM with a given name. */
+static struct lsm_info __init *find_lsm(const char *name)
+{
+	struct lsm_info *lsm;
+
+	for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++)
+		if (!strcmp(lsm->name, name))
+			return lsm;
+
+	return NULL;
+}
+
 /*
  * Current index to use while initializing the lsm id list.
  */
@@ -333,10 +345,23 @@  static void __init ordered_lsm_parse(const char *order, const char *origin)
 
 	/* LSM_ORDER_LAST is always last. */
 	for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) {
+		/* Do it later, to enforce the expected ordering. */
+		if (!strcmp(lsm->name, "ima") || !strcmp(lsm->name, "evm"))
+			continue;
+
 		if (lsm->order == LSM_ORDER_LAST)
 			append_ordered_lsm(lsm, "   last");
 	}
 
+	/* Ensure that the 'ima' and 'evm' LSMs are last and in this order. */
+	lsm = find_lsm("ima");
+	if (lsm)
+		append_ordered_lsm(lsm, "   last");
+
+	lsm = find_lsm("evm");
+	if (lsm)
+		append_ordered_lsm(lsm, "   last");
+
 	/* Disable all LSMs not in the ordered list. */
 	for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) {
 		if (exists_ordered_lsm(lsm))