x86/ima: require signed kernel modules
diff mbox series

Message ID 1548962339-10681-2-git-send-email-zohar@linux.ibm.com
State New
Headers show
Series
  • x86/ima: require signed kernel modules
Related show

Commit Message

Mimi Zohar Jan. 31, 2019, 7:18 p.m. UTC
Require signed kernel modules on systems with secure boot mode enabled.

To coordinate between appended kernel module signatures and IMA
signatures, only define an IMA MODULE_CHECK policy rule if
CONFIG_MODULE_SIG is not enabled.

This patch defines a function named set_module_sig_required() and renames
is_module_sig_enforced() to is_module_sig_enforced_or_required().  The
call to set_module_sig_required() is dependent on CONFIG_IMA_ARCH_POLICY
being enabled.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 arch/x86/kernel/ima_arch.c        |  9 ++++++++-
 include/linux/module.h            |  7 ++++++-
 kernel/module.c                   | 15 +++++++++++----
 security/integrity/ima/ima_main.c |  2 +-
 4 files changed, 26 insertions(+), 7 deletions(-)

Comments

Luis Chamberlain Feb. 4, 2019, 8:38 p.m. UTC | #1
On Thu, Jan 31, 2019 at 02:18:59PM -0500, Mimi Zohar wrote:
> diff --git a/kernel/module.c b/kernel/module.c
> index 2ad1b5239910..70a9709d19eb 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -275,16 +275,23 @@ static void module_assert_mutex_or_preempt(void)
>  
>  static bool sig_enforce = IS_ENABLED(CONFIG_MODULE_SIG_FORCE);
>  module_param(sig_enforce, bool_enable_only, 0644);
> +static bool sig_required;
>  
>  /*
>   * Export sig_enforce kernel cmdline parameter to allow other subsystems rely
>   * on that instead of directly to CONFIG_MODULE_SIG_FORCE config.

But the docs were't updated.

>   */
> -bool is_module_sig_enforced(void)
> +bool is_module_sig_enforced_or_required(void)
>  {
> -	return sig_enforce;
> +	return sig_enforce || sig_required;
>  }
> -EXPORT_SYMBOL(is_module_sig_enforced);
> +EXPORT_SYMBOL(is_module_sig_enforced_or_required);

Meh, this is getting sloppy, the module signing infrastructure should
just be LSM'ified now that we have stacked LSMs. That would
compartamentaliz that code and make this much easier to read / understand
and mantain.

Can you take a look at doing it that way instead?

> +
> +void set_module_sig_required(void)
> +{
> +	sig_required = true;
> +}
> +EXPORT_SYMBOL(set_module_sig_required);

Since this is a *new* symbol, not yet used, and only used by IMA I'd
prefer this to be EXPORT_SYMBOL_GPL().

>  /* Block module loading/unloading? */
>  int modules_disabled = 0;
> @@ -2789,7 +2796,7 @@ static int module_sig_check(struct load_info *info, int flags)
>  	}
>  
>  	/* Not having a signature is only an error if we're strict. */
> -	if (err == -ENOKEY && !is_module_sig_enforced())
> +	if (err == -ENOKEY && !is_module_sig_enforced_or_required())

This is where I think a proper LSM hook would make sense. I think
that these "questions" model for signing don't work well on the LSM
hook model, perhaps just:

kernel_module_signed()

Suffices, therefore if not enforced or required its signed. If its
enforced or required and really signed, then it signed.

>  		err = 0;
>  
>  	return err;
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 357edd140c09..bbaf87f688be 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -563,7 +563,7 @@ int ima_load_data(enum kernel_load_data_id id)
>  		}
>  		break;
>  	case LOADING_MODULE:
> -		sig_enforce = is_module_sig_enforced();
> +		sig_enforce = is_module_sig_enforced_or_required();

Yet another user.

>  		if (ima_enforce && (!sig_enforce
>  				    && (ima_appraise & IMA_APPRAISE_MODULES))) {
> -- 
> 2.7.5

Plus I think LSM'ifying module signing may help cleaning up some of the
#ifdery and config options around module signing. I'm suggestin this now 
as this has been on my mental TODO list for a while, and just not sure
when we'd get to it, if not you, not sure when it'd get done.

Then, do we have proper unit tests for the mixture of options to ensure
we can easily ensure we don't regress?

  Luis
Mimi Zohar Feb. 4, 2019, 10:05 p.m. UTC | #2
On Mon, 2019-02-04 at 12:38 -0800, Luis Chamberlain wrote:
> On Thu, Jan 31, 2019 at 02:18:59PM -0500, Mimi Zohar wrote:
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 2ad1b5239910..70a9709d19eb 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -275,16 +275,23 @@ static void module_assert_mutex_or_preempt(void)
> >  
> >  static bool sig_enforce = IS_ENABLED(CONFIG_MODULE_SIG_FORCE);
> >  module_param(sig_enforce, bool_enable_only, 0644);
> > +static bool sig_required;
> >  
> >  /*
> >   * Export sig_enforce kernel cmdline parameter to allow other subsystems rely
> >   * on that instead of directly to CONFIG_MODULE_SIG_FORCE config.
> 
> But the docs were't updated.

Neither "CONFIG_MODULE_SIG_FORCE" nor "module.sig_enforce" has
changed.  Which docs are you referring to? 

> 
> >   */
> > -bool is_module_sig_enforced(void)
> > +bool is_module_sig_enforced_or_required(void)
> >  {
> > -	return sig_enforce;
> > +	return sig_enforce || sig_required;
> >  }
> > -EXPORT_SYMBOL(is_module_sig_enforced);
> > +EXPORT_SYMBOL(is_module_sig_enforced_or_required);
> 
> Meh, this is getting sloppy, the module signing infrastructure should
> just be LSM'ified now that we have stacked LSMs. That would
> compartamentaliz that code and make this much easier to read / understand
> and mantain.
> 
> Can you take a look at doing it that way instead?

This patch is about coordinating the existing methods of verifying
kernel module signatures.

> 
> > +
> > +void set_module_sig_required(void)
> > +{
> > +	sig_required = true;
> > +}
> > +EXPORT_SYMBOL(set_module_sig_required);
> 
> Since this is a *new* symbol, not yet used, and only used by IMA I'd
> prefer this to be EXPORT_SYMBOL_GPL().

Oops, this function shouldn't be exported.

> 
> >  /* Block module loading/unloading? */
> >  int modules_disabled = 0;
> > @@ -2789,7 +2796,7 @@ static int module_sig_check(struct load_info *info, int flags)
> >  	}
> >  
> >  	/* Not having a signature is only an error if we're strict. */
> > -	if (err == -ENOKEY && !is_module_sig_enforced())
> > +	if (err == -ENOKEY && !is_module_sig_enforced_or_required())
> 
> This is where I think a proper LSM hook would make sense. I think
> that these "questions" model for signing don't work well on the LSM
> hook model, perhaps just:
> 
> kernel_module_signed()
> 
> Suffices, therefore if not enforced or required its signed. If its
> enforced or required and really signed, then it signed.
> 
> >  		err = 0;
> >  
> >  	return err;
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index 357edd140c09..bbaf87f688be 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -563,7 +563,7 @@ int ima_load_data(enum kernel_load_data_id id)
> >  		}
> >  		break;
> >  	case LOADING_MODULE:
> > -		sig_enforce = is_module_sig_enforced();
> > +		sig_enforce = is_module_sig_enforced_or_required();
> 
> Yet another user.
> 
> >  		if (ima_enforce && (!sig_enforce
> >  				    && (ima_appraise & IMA_APPRAISE_MODULES))) {
> > -- 
> > 2.7.5
> 
> Plus I think LSM'ifying module signing may help cleaning up some of the
> #ifdery and config options around module signing. I'm suggestin this now 
> as this has been on my mental TODO list for a while, and just not sure
> when we'd get to it, if not you, not sure when it'd get done.
> 
> Then, do we have proper unit tests for the mixture of options to ensure
> we can easily ensure we don't regress?
> 

There are already two methods  - appended signatures and IMA xattrs -
for validating kernel modules.

Kernel modules shouldn't be treated any differently than any other file.  Based on the IMA policy, the kernel module signature can be verified.  Also based on the IMA policy, the file hash added to the measurement list, and the file hash used to extend the TPM PCR.  Lastly, based on policy the file hash can be added to the audit log.

I don't see a need for an additional LSM just for verifying kernel module signatures.

Mimi
Luis Chamberlain Feb. 4, 2019, 10:30 p.m. UTC | #3
On Mon, Feb 04, 2019 at 05:05:10PM -0500, Mimi Zohar wrote:
> On Mon, 2019-02-04 at 12:38 -0800, Luis Chamberlain wrote:
> > On Thu, Jan 31, 2019 at 02:18:59PM -0500, Mimi Zohar wrote:
> > > diff --git a/kernel/module.c b/kernel/module.c
> > > index 2ad1b5239910..70a9709d19eb 100644
> > > --- a/kernel/module.c
> > > +++ b/kernel/module.c
> > > @@ -275,16 +275,23 @@ static void module_assert_mutex_or_preempt(void)
> > >  
> > >  static bool sig_enforce = IS_ENABLED(CONFIG_MODULE_SIG_FORCE);
> > >  module_param(sig_enforce, bool_enable_only, 0644);
> > > +static bool sig_required;
> > >  
> > >  /*
> > >   * Export sig_enforce kernel cmdline parameter to allow other subsystems rely
> > >   * on that instead of directly to CONFIG_MODULE_SIG_FORCE config.
> > 
> > But the docs were't updated.
> 
> Neither "CONFIG_MODULE_SIG_FORCE" nor "module.sig_enforce" has
> changed.  Which docs are you referring to? 

You renamed is_module_sig_enforced() to is_module_sig_enforced_or_required()
and left the above doc which only justifies the enforced path.

> > >   */
> > > -bool is_module_sig_enforced(void)
> > > +bool is_module_sig_enforced_or_required(void)
> > >  {
> > > -	return sig_enforce;
> > > +	return sig_enforce || sig_required;
> > >  }
> > > -EXPORT_SYMBOL(is_module_sig_enforced);
> > > +EXPORT_SYMBOL(is_module_sig_enforced_or_required);
> > 
> > Meh, this is getting sloppy, the module signing infrastructure should
> > just be LSM'ified now that we have stacked LSMs. That would
> > compartamentaliz that code and make this much easier to read / understand
> > and mantain.
> > 
> > Can you take a look at doing it that way instead?
> 
> This patch is about coordinating the existing methods of verifying
> kernel module signatures.

I understand.

> > 
> > >  /* Block module loading/unloading? */
> > >  int modules_disabled = 0;
> > > @@ -2789,7 +2796,7 @@ static int module_sig_check(struct load_info *info, int flags)
> > >  	}
> > >  
> > >  	/* Not having a signature is only an error if we're strict. */
> > > -	if (err == -ENOKEY && !is_module_sig_enforced())
> > > +	if (err == -ENOKEY && !is_module_sig_enforced_or_required())
> > 
> > This is where I think a proper LSM hook would make sense. I think
> > that these "questions" model for signing don't work well on the LSM
> > hook model, perhaps just:
> > 
> > kernel_module_signed()
> > 
> > Suffices, therefore if not enforced or required its signed. If its
> > enforced or required and really signed, then it signed.
> > 
> > >  		err = 0;
> > >  
> > >  	return err;
> > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > > index 357edd140c09..bbaf87f688be 100644
> > > --- a/security/integrity/ima/ima_main.c
> > > +++ b/security/integrity/ima/ima_main.c
> > > @@ -563,7 +563,7 @@ int ima_load_data(enum kernel_load_data_id id)
> > >  		}
> > >  		break;
> > >  	case LOADING_MODULE:
> > > -		sig_enforce = is_module_sig_enforced();
> > > +		sig_enforce = is_module_sig_enforced_or_required();
> > 
> > Yet another user.
> > 
> > >  		if (ima_enforce && (!sig_enforce
> > >  				    && (ima_appraise & IMA_APPRAISE_MODULES))) {
> > > -- 
> > > 2.7.5
> > 
> > Plus I think LSM'ifying module signing may help cleaning up some of the
> > #ifdery and config options around module signing. I'm suggestin this now 
> > as this has been on my mental TODO list for a while, and just not sure
> > when we'd get to it, if not you, not sure when it'd get done.
> > 
> > Then, do we have proper unit tests for the mixture of options to ensure
> > we can easily ensure we don't regress?
> > 
> 
> There are already two methods  - appended signatures and IMA xattrs -
> for validating kernel modules.
> 
> Kernel modules shouldn't be treated any differently than any other
> file.

The good 'ol kernel module signing code *does* treat it as such.

> Based on the IMA policy, the kernel module signature can be
> verified.  Also based on the IMA policy, the file hash added to the
> measurement list, and the file hash used to extend the TPM PCR.
>  Lastly, based on policy the file hash can be added to the audit log.

Sure...

> I don't see a need for an additional LSM just for verifying kernel
> module signatures.

But it is one, module signing was just spawned pre the boom of LSMs.

I do believe that treating the code as such would help with its reading
and long term maintenance.

Anyway, I had to try to convince you.

 Luis
Mimi Zohar Feb. 5, 2019, 12:24 p.m. UTC | #4
On Mon, 2019-02-04 at 14:30 -0800, Luis Chamberlain wrote:
> On Mon, Feb 04, 2019 at 05:05:10PM -0500, Mimi Zohar wrote:
> > On Mon, 2019-02-04 at 12:38 -0800, Luis Chamberlain wrote:

> > I don't see a need for an additional LSM just for verifying kernel
> > module signatures.
> 
> But it is one, module signing was just spawned pre the boom of LSMs.
> 
> I do believe that treating the code as such would help with its reading
> and long term maintenance.
> 
> Anyway, I had to try to convince you.

Perhaps, after IMA supports appended signatures (for kernel modules),
I could see making the existing kernel module appended signature
verification an LSM.

For now, other than updating the comment, would you be willing to add
your Review/Ack to this patch?

Mimi
Seth Forshee Feb. 5, 2019, 3:18 p.m. UTC | #5
On Thu, Jan 31, 2019 at 02:18:59PM -0500, Mimi Zohar wrote:
> Require signed kernel modules on systems with secure boot mode enabled.
> 
> To coordinate between appended kernel module signatures and IMA
> signatures, only define an IMA MODULE_CHECK policy rule if
> CONFIG_MODULE_SIG is not enabled.
> 
> This patch defines a function named set_module_sig_required() and renames
> is_module_sig_enforced() to is_module_sig_enforced_or_required().  The
> call to set_module_sig_required() is dependent on CONFIG_IMA_ARCH_POLICY
> being enabled.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>

With respect to interactions with the kernel lockdown patches, this
looks better than the patches I saw previously. I don't feel like I know
enough about what's going on with IMA to ack the patch, but I feel
confident that it's at least not going to break signature enforcement
for us.

Thanks,
Seth
Nayna Feb. 5, 2019, 4:10 p.m. UTC | #6
On 01/31/2019 02:18 PM, Mimi Zohar wrote:
> Require signed kernel modules on systems with secure boot mode enabled.
>
> To coordinate between appended kernel module signatures and IMA
> signatures, only define an IMA MODULE_CHECK policy rule if
> CONFIG_MODULE_SIG is not enabled.
>
> This patch defines a function named set_module_sig_required() and renames
> is_module_sig_enforced() to is_module_sig_enforced_or_required().  The
> call to set_module_sig_required() is dependent on CONFIG_IMA_ARCH_POLICY
> being enabled.
>
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>

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

Thanks & Regards,
      - Nayna
Mimi Zohar Feb. 5, 2019, 4:47 p.m. UTC | #7
Hi Seth,

On Tue, 2019-02-05 at 09:18 -0600, Seth Forshee wrote:
> On Thu, Jan 31, 2019 at 02:18:59PM -0500, Mimi Zohar wrote:
> > Require signed kernel modules on systems with secure boot mode enabled.
> > 
> > To coordinate between appended kernel module signatures and IMA
> > signatures, only define an IMA MODULE_CHECK policy rule if
> > CONFIG_MODULE_SIG is not enabled.
> > 
> > This patch defines a function named set_module_sig_required() and renames
> > is_module_sig_enforced() to is_module_sig_enforced_or_required().  The
> > call to set_module_sig_required() is dependent on CONFIG_IMA_ARCH_POLICY
> > being enabled.
> > 
> > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> 
> With respect to interactions with the kernel lockdown patches, this
> looks better than the patches I saw previously. I don't feel like I know
> enough about what's going on with IMA to ack the patch, but I feel
> confident that it's at least not going to break signature enforcement
> for us.

Thank you for testing!  Could this be translated into a "tested-by"
"(for w/lockdown patches)"?

Mimi
Seth Forshee Feb. 5, 2019, 6:32 p.m. UTC | #8
On Tue, Feb 05, 2019 at 11:47:24AM -0500, Mimi Zohar wrote:
> Hi Seth,
> 
> On Tue, 2019-02-05 at 09:18 -0600, Seth Forshee wrote:
> > On Thu, Jan 31, 2019 at 02:18:59PM -0500, Mimi Zohar wrote:
> > > Require signed kernel modules on systems with secure boot mode enabled.
> > > 
> > > To coordinate between appended kernel module signatures and IMA
> > > signatures, only define an IMA MODULE_CHECK policy rule if
> > > CONFIG_MODULE_SIG is not enabled.
> > > 
> > > This patch defines a function named set_module_sig_required() and renames
> > > is_module_sig_enforced() to is_module_sig_enforced_or_required().  The
> > > call to set_module_sig_required() is dependent on CONFIG_IMA_ARCH_POLICY
> > > being enabled.
> > > 
> > > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> > 
> > With respect to interactions with the kernel lockdown patches, this
> > looks better than the patches I saw previously. I don't feel like I know
> > enough about what's going on with IMA to ack the patch, but I feel
> > confident that it's at least not going to break signature enforcement
> > for us.
> 
> Thank you for testing!  Could this be translated into a "tested-by"
> "(for w/lockdown patches)"?

Yeah, that's fine. To be clear about what I tested, I've confirmed that
it doesn't interfere with requiring signed modules under lockdown with
CONFIG_IMA_ARCH_POLICY=n and IMA appraisal enabled.

Tested-by: Seth Forshee <seth.forshee@canonical.com>
Mimi Zohar Feb. 5, 2019, 6:52 p.m. UTC | #9
On Tue, 2019-02-05 at 12:32 -0600, Seth Forshee wrote:
> On Tue, Feb 05, 2019 at 11:47:24AM -0500, Mimi Zohar wrote:
> > Hi Seth,
> > 
> > On Tue, 2019-02-05 at 09:18 -0600, Seth Forshee wrote:
> > > On Thu, Jan 31, 2019 at 02:18:59PM -0500, Mimi Zohar wrote:
> > > > Require signed kernel modules on systems with secure boot mode enabled.
> > > > 
> > > > To coordinate between appended kernel module signatures and IMA
> > > > signatures, only define an IMA MODULE_CHECK policy rule if
> > > > CONFIG_MODULE_SIG is not enabled.
> > > > 
> > > > This patch defines a function named set_module_sig_required() and renames
> > > > is_module_sig_enforced() to is_module_sig_enforced_or_required().  The
> > > > call to set_module_sig_required() is dependent on CONFIG_IMA_ARCH_POLICY
> > > > being enabled.
> > > > 
> > > > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> > > 
> > > With respect to interactions with the kernel lockdown patches, this
> > > looks better than the patches I saw previously. I don't feel like I know
> > > enough about what's going on with IMA to ack the patch, but I feel
> > > confident that it's at least not going to break signature enforcement
> > > for us.
> > 
> > Thank you for testing!  Could this be translated into a "tested-by"
> > "(for w/lockdown patches)"?
> 
> Yeah, that's fine. To be clear about what I tested, I've confirmed that
> it doesn't interfere with requiring signed modules under lockdown with
> CONFIG_IMA_ARCH_POLICY=n and IMA appraisal enabled.
> 
> Tested-by: Seth Forshee <seth.forshee@canonical.com>

Oh!  You've disabled the coordination of the two signature
verification methods.  Any chance you could test with
"CONFIG_IMA_ARCH_POLICY" enabled?

Mimi
Luis Chamberlain Feb. 5, 2019, 9:13 p.m. UTC | #10
On Tue, Feb 05, 2019 at 07:24:39AM -0500, Mimi Zohar wrote:
> On Mon, 2019-02-04 at 14:30 -0800, Luis Chamberlain wrote:
> > On Mon, Feb 04, 2019 at 05:05:10PM -0500, Mimi Zohar wrote:
> > > On Mon, 2019-02-04 at 12:38 -0800, Luis Chamberlain wrote:
> 
> > > I don't see a need for an additional LSM just for verifying kernel
> > > module signatures.
> > 
> > But it is one, module signing was just spawned pre the boom of LSMs.
> > 
> > I do believe that treating the code as such would help with its reading
> > and long term maintenance.
> > 
> > Anyway, I had to try to convince you.
> 
> Perhaps, after IMA supports appended signatures (for kernel modules),
> I could see making the existing kernel module appended signature
> verification an LSM.

I don't see why wait.

> For now, other than updating the comment, would you be willing to add
> your Review/Ack to this patch?

But I don't particularly like the changes, I still believe trying to
LSM'ify kernel module signing would be a better start to help with
long term maintenace on this code.

Also, do we have selftests implemented to ensure we don't regress with
your changes?

  Luis
Mimi Zohar Feb. 5, 2019, 11:13 p.m. UTC | #11
On Tue, 2019-02-05 at 13:13 -0800, Luis Chamberlain wrote:
> Also, do we have selftests implemented to ensure we don't regress with
> your changes?

We're working on a regression test.  "modprobe" currently first
attempts to use finit_module, and then falls back to using
init_module.  Before writing a small program to load kernel modules,
are you aware of any that allows you to specify the syscall?

Mimi
Seth Forshee Feb. 8, 2019, 7:21 p.m. UTC | #12
On Tue, Feb 05, 2019 at 01:52:21PM -0500, Mimi Zohar wrote:
> On Tue, 2019-02-05 at 12:32 -0600, Seth Forshee wrote:
> > On Tue, Feb 05, 2019 at 11:47:24AM -0500, Mimi Zohar wrote:
> > > Hi Seth,
> > > 
> > > On Tue, 2019-02-05 at 09:18 -0600, Seth Forshee wrote:
> > > > On Thu, Jan 31, 2019 at 02:18:59PM -0500, Mimi Zohar wrote:
> > > > > Require signed kernel modules on systems with secure boot mode enabled.
> > > > > 
> > > > > To coordinate between appended kernel module signatures and IMA
> > > > > signatures, only define an IMA MODULE_CHECK policy rule if
> > > > > CONFIG_MODULE_SIG is not enabled.
> > > > > 
> > > > > This patch defines a function named set_module_sig_required() and renames
> > > > > is_module_sig_enforced() to is_module_sig_enforced_or_required().  The
> > > > > call to set_module_sig_required() is dependent on CONFIG_IMA_ARCH_POLICY
> > > > > being enabled.
> > > > > 
> > > > > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> > > > 
> > > > With respect to interactions with the kernel lockdown patches, this
> > > > looks better than the patches I saw previously. I don't feel like I know
> > > > enough about what's going on with IMA to ack the patch, but I feel
> > > > confident that it's at least not going to break signature enforcement
> > > > for us.
> > > 
> > > Thank you for testing!  Could this be translated into a "tested-by"
> > > "(for w/lockdown patches)"?
> > 
> > Yeah, that's fine. To be clear about what I tested, I've confirmed that
> > it doesn't interfere with requiring signed modules under lockdown with
> > CONFIG_IMA_ARCH_POLICY=n and IMA appraisal enabled.
> > 
> > Tested-by: Seth Forshee <seth.forshee@canonical.com>
> 
> Oh!  You've disabled the coordination of the two signature
> verification methods.  Any chance you could test with
> "CONFIG_IMA_ARCH_POLICY" enabled?

Ok, I've tested this now and it also seems to be working. However it
does seem redundant to have CONFIG_IMA_ARCH_POLICY alongside lockdown,
as it doesn't enforce anything not already being enforced by lockdown.

Seth
Mimi Zohar Feb. 10, 2019, 3:39 p.m. UTC | #13
[Cc'ing Bruno E. O. Meneguele]

On Fri, 2019-02-08 at 13:21 -0600, Seth Forshee wrote:
> On Tue, Feb 05, 2019 at 01:52:21PM -0500, Mimi Zohar wrote:
> > On Tue, 2019-02-05 at 12:32 -0600, Seth Forshee wrote:
> > > On Tue, Feb 05, 2019 at 11:47:24AM -0500, Mimi Zohar wrote:
> > > > Hi Seth,
> > > > 
> > > > On Tue, 2019-02-05 at 09:18 -0600, Seth Forshee wrote:
> > > > > On Thu, Jan 31, 2019 at 02:18:59PM -0500, Mimi Zohar wrote:
> > > > > > Require signed kernel modules on systems with secure boot mode enabled.
> > > > > > 
> > > > > > To coordinate between appended kernel module signatures and IMA
> > > > > > signatures, only define an IMA MODULE_CHECK policy rule if
> > > > > > CONFIG_MODULE_SIG is not enabled.
> > > > > > 
> > > > > > This patch defines a function named set_module_sig_required() and renames
> > > > > > is_module_sig_enforced() to is_module_sig_enforced_or_required().  The
> > > > > > call to set_module_sig_required() is dependent on CONFIG_IMA_ARCH_POLICY
> > > > > > being enabled.
> > > > > > 
> > > > > > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> > > > > 
> > > > > With respect to interactions with the kernel lockdown patches, this
> > > > > looks better than the patches I saw previously. I don't feel like I know
> > > > > enough about what's going on with IMA to ack the patch, but I feel
> > > > > confident that it's at least not going to break signature enforcement
> > > > > for us.
> > > > 
> > > > Thank you for testing!  Could this be translated into a "tested-by"
> > > > "(for w/lockdown patches)"?
> > > 
> > > Yeah, that's fine. To be clear about what I tested, I've confirmed that
> > > it doesn't interfere with requiring signed modules under lockdown with
> > > CONFIG_IMA_ARCH_POLICY=n and IMA appraisal enabled.
> > > 
> > > Tested-by: Seth Forshee <seth.forshee@canonical.com>
> > 
> > Oh!  You've disabled the coordination of the two signature
> > verification methods.  Any chance you could test with
> > "CONFIG_IMA_ARCH_POLICY" enabled?
> 
> Ok, I've tested this now and it also seems to be working. However it
> does seem redundant to have CONFIG_IMA_ARCH_POLICY alongside lockdown,
> as it doesn't enforce anything not already being enforced by lockdown.

Ok.  Based on Luis' and your comments, I'll defer this discussion to
after IMA appended signature support is upstreamed.  At that point,
for the finit_module syscall, IMA-appraisal at least won't fail the
signature verification.  The same appended signature will be verified
not once, but twice - once on the LSM security_kernel_read_file() hook
and then again by module_sig_check().

For the init_module() syscall, the only coordination needed will be
updating is_module_sig_enforced(), based on either the "lockdown" or
some other flag.

Mimi
Jessica Yu Feb. 11, 2019, 3:56 p.m. UTC | #14
+++ Mimi Zohar [31/01/19 14:18 -0500]:
>Require signed kernel modules on systems with secure boot mode enabled.
>
>To coordinate between appended kernel module signatures and IMA
>signatures, only define an IMA MODULE_CHECK policy rule if
>CONFIG_MODULE_SIG is not enabled.
>
>This patch defines a function named set_module_sig_required() and renames
>is_module_sig_enforced() to is_module_sig_enforced_or_required().  The
>call to set_module_sig_required() is dependent on CONFIG_IMA_ARCH_POLICY
>being enabled.
>
>Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
>---
> arch/x86/kernel/ima_arch.c        |  9 ++++++++-
> include/linux/module.h            |  7 ++++++-
> kernel/module.c                   | 15 +++++++++++----
> security/integrity/ima/ima_main.c |  2 +-
> 4 files changed, 26 insertions(+), 7 deletions(-)
>
>diff --git a/arch/x86/kernel/ima_arch.c b/arch/x86/kernel/ima_arch.c
>index e47cd9390ab4..96a023238a83 100644
>--- a/arch/x86/kernel/ima_arch.c
>+++ b/arch/x86/kernel/ima_arch.c
>@@ -64,12 +64,19 @@ static const char * const sb_arch_rules[] = {
> 	"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig",
> #endif /* CONFIG_KEXEC_VERIFY_SIG */
> 	"measure func=KEXEC_KERNEL_CHECK",
>+#if !IS_ENABLED(CONFIG_MODULE_SIG)
>+	"appraise func=MODULE_CHECK appraise_type=imasig",
>+#endif
>+	"measure func=MODULE_CHECK",
> 	NULL
> };
>
> const char * const *arch_get_ima_policy(void)
> {
>-	if (IS_ENABLED(CONFIG_IMA_ARCH_POLICY) && arch_ima_get_secureboot())
>+	if (IS_ENABLED(CONFIG_IMA_ARCH_POLICY) && arch_ima_get_secureboot()) {
>+		if (IS_ENABLED(CONFIG_MODULE_SIG))
>+			set_module_sig_required();
> 		return sb_arch_rules;
>+	}
> 	return NULL;
> }
>diff --git a/include/linux/module.h b/include/linux/module.h
>index 8fa38d3e7538..af51c8ec755f 100644
>--- a/include/linux/module.h
>+++ b/include/linux/module.h
>@@ -659,7 +659,8 @@ static inline bool is_livepatch_module(struct module *mod)
> }
> #endif /* CONFIG_LIVEPATCH */
>
>-bool is_module_sig_enforced(void);
>+bool is_module_sig_enforced_or_required(void);
>+void set_module_sig_required(void);
>
> #else /* !CONFIG_MODULES... */
>
>@@ -780,6 +781,10 @@ static inline bool is_module_sig_enforced(void)
> 	return false;
> }
>
>+static inline void set_module_sig_required(void)
>+{
>+}
>+
> /* Dereference module function descriptor */
> static inline
> void *dereference_module_function_descriptor(struct module *mod, void *ptr)
>diff --git a/kernel/module.c b/kernel/module.c
>index 2ad1b5239910..70a9709d19eb 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -275,16 +275,23 @@ static void module_assert_mutex_or_preempt(void)
>
> static bool sig_enforce = IS_ENABLED(CONFIG_MODULE_SIG_FORCE);
> module_param(sig_enforce, bool_enable_only, 0644);
>+static bool sig_required;
>
> /*
>  * Export sig_enforce kernel cmdline parameter to allow other subsystems rely
>  * on that instead of directly to CONFIG_MODULE_SIG_FORCE config.
>  */
>-bool is_module_sig_enforced(void)
>+bool is_module_sig_enforced_or_required(void)
> {
>-	return sig_enforce;
>+	return sig_enforce || sig_required;
> }

Hi Mimi,

Just wondering, is there any particular reason why a distinction is
made between sig_enforce and sig_required? Doesn't sig_enforce imply
that signed modules are required? In other words, why introduce
another variable instead of just using sig_enforce? It may be
confusing in the case of a user looking at /sys/module/module/parameters/sig_enforce
and it having a value of 0 yet module signatures are being required by ima.

Thanks,

Jessica

>-EXPORT_SYMBOL(is_module_sig_enforced);
>+EXPORT_SYMBOL(is_module_sig_enforced_or_required);
>+
>+void set_module_sig_required(void)
>+{
>+	sig_required = true;
>+}
>+EXPORT_SYMBOL(set_module_sig_required);
>
> /* Block module loading/unloading? */
> int modules_disabled = 0;
>@@ -2789,7 +2796,7 @@ static int module_sig_check(struct load_info *info, int flags)
> 	}
>
> 	/* Not having a signature is only an error if we're strict. */
>-	if (err == -ENOKEY && !is_module_sig_enforced())
>+	if (err == -ENOKEY && !is_module_sig_enforced_or_required())
> 		err = 0;
>
> 	return err;
>diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
>index 357edd140c09..bbaf87f688be 100644
>--- a/security/integrity/ima/ima_main.c
>+++ b/security/integrity/ima/ima_main.c
>@@ -563,7 +563,7 @@ int ima_load_data(enum kernel_load_data_id id)
> 		}
> 		break;
> 	case LOADING_MODULE:
>-		sig_enforce = is_module_sig_enforced();
>+		sig_enforce = is_module_sig_enforced_or_required();
>
> 		if (ima_enforce && (!sig_enforce
> 				    && (ima_appraise & IMA_APPRAISE_MODULES))) {
>-- 
>2.7.5
>
Mimi Zohar Feb. 11, 2019, 4:19 p.m. UTC | #15
On Mon, 2019-02-11 at 16:56 +0100, Jessica Yu wrote:
> +++ Mimi Zohar [31/01/19 14:18 -0500]:
> >Require signed kernel modules on systems with secure boot mode enabled.
> >
> >To coordinate between appended kernel module signatures and IMA
> >signatures, only define an IMA MODULE_CHECK policy rule if
> >CONFIG_MODULE_SIG is not enabled.
> >
> >This patch defines a function named set_module_sig_required() and renames
> >is_module_sig_enforced() to is_module_sig_enforced_or_required().  The
> >call to set_module_sig_required() is dependent on CONFIG_IMA_ARCH_POLICY
> >being enabled.
> >
> >Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> >---
> > arch/x86/kernel/ima_arch.c        |  9 ++++++++-
> > include/linux/module.h            |  7 ++++++-
> > kernel/module.c                   | 15 +++++++++++----
> > security/integrity/ima/ima_main.c |  2 +-
> > 4 files changed, 26 insertions(+), 7 deletions(-)
> >
> >diff --git a/arch/x86/kernel/ima_arch.c b/arch/x86/kernel/ima_arch.c
> >index e47cd9390ab4..96a023238a83 100644
> >--- a/arch/x86/kernel/ima_arch.c
> >+++ b/arch/x86/kernel/ima_arch.c
> >@@ -64,12 +64,19 @@ static const char * const sb_arch_rules[] = {
> > 	"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig",
> > #endif /* CONFIG_KEXEC_VERIFY_SIG */
> > 	"measure func=KEXEC_KERNEL_CHECK",
> >+#if !IS_ENABLED(CONFIG_MODULE_SIG)
> >+	"appraise func=MODULE_CHECK appraise_type=imasig",
> >+#endif
> >+	"measure func=MODULE_CHECK",
> > 	NULL
> > };
> >
> > const char * const *arch_get_ima_policy(void)
> > {
> >-	if (IS_ENABLED(CONFIG_IMA_ARCH_POLICY) && arch_ima_get_secureboot())
> >+	if (IS_ENABLED(CONFIG_IMA_ARCH_POLICY) && arch_ima_get_secureboot()) {
> >+		if (IS_ENABLED(CONFIG_MODULE_SIG))
> >+			set_module_sig_required();
> > 		return sb_arch_rules;
> >+	}
> > 	return NULL;
> > }
> >diff --git a/include/linux/module.h b/include/linux/module.h
> >index 8fa38d3e7538..af51c8ec755f 100644
> >--- a/include/linux/module.h
> >+++ b/include/linux/module.h
> >@@ -659,7 +659,8 @@ static inline bool is_livepatch_module(struct module *mod)
> > }
> > #endif /* CONFIG_LIVEPATCH */
> >
> >-bool is_module_sig_enforced(void);
> >+bool is_module_sig_enforced_or_required(void);
> >+void set_module_sig_required(void);
> >
> > #else /* !CONFIG_MODULES... */
> >
> >@@ -780,6 +781,10 @@ static inline bool is_module_sig_enforced(void)
> > 	return false;
> > }
> >
> >+static inline void set_module_sig_required(void)
> >+{
> >+}
> >+
> > /* Dereference module function descriptor */
> > static inline
> > void *dereference_module_function_descriptor(struct module *mod, void *ptr)
> >diff --git a/kernel/module.c b/kernel/module.c
> >index 2ad1b5239910..70a9709d19eb 100644
> >--- a/kernel/module.c
> >+++ b/kernel/module.c
> >@@ -275,16 +275,23 @@ static void module_assert_mutex_or_preempt(void)
> >
> > static bool sig_enforce = IS_ENABLED(CONFIG_MODULE_SIG_FORCE);
> > module_param(sig_enforce, bool_enable_only, 0644);
> >+static bool sig_required;
> >
> > /*
> >  * Export sig_enforce kernel cmdline parameter to allow other subsystems rely
> >  * on that instead of directly to CONFIG_MODULE_SIG_FORCE config.
> >  */
> >-bool is_module_sig_enforced(void)
> >+bool is_module_sig_enforced_or_required(void)
> > {
> >-	return sig_enforce;
> >+	return sig_enforce || sig_required;
> > }
> 
> Hi Mimi,
> 
> Just wondering, is there any particular reason why a distinction is
> made between sig_enforce and sig_required? Doesn't sig_enforce imply
> that signed modules are required? In other words, why introduce
> another variable instead of just using sig_enforce? It may be
> confusing in the case of a user looking at /sys/module/module/parameters/sig_enforce
> and it having a value of 0 yet module signatures are being required by ima.

Hi Jessica,

It would definitely be a lot better not having to differentiate
between the builtin CONFIG/module parm enforced and runtime enforced.
 For some reason the "lockdown" patch doesn't directly modify
sig_enforce.

Mimi

> 
> >-EXPORT_SYMBOL(is_module_sig_enforced);
> >+EXPORT_SYMBOL(is_module_sig_enforced_or_required);
> >+
> >+void set_module_sig_required(void)
> >+{
> >+	sig_required = true;
> >+}
> >+EXPORT_SYMBOL(set_module_sig_required);
> >
> > /* Block module loading/unloading? */
> > int modules_disabled = 0;
> >@@ -2789,7 +2796,7 @@ static int module_sig_check(struct load_info *info, int flags)
> > 	}
> >
> > 	/* Not having a signature is only an error if we're strict. */
> >-	if (err == -ENOKEY && !is_module_sig_enforced())
> >+	if (err == -ENOKEY && !is_module_sig_enforced_or_required())
> > 		err = 0;
> >
> > 	return err;
> >diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> >index 357edd140c09..bbaf87f688be 100644
> >--- a/security/integrity/ima/ima_main.c
> >+++ b/security/integrity/ima/ima_main.c
> >@@ -563,7 +563,7 @@ int ima_load_data(enum kernel_load_data_id id)
> > 		}
> > 		break;
> > 	case LOADING_MODULE:
> >-		sig_enforce = is_module_sig_enforced();
> >+		sig_enforce = is_module_sig_enforced_or_required();
> >
> > 		if (ima_enforce && (!sig_enforce
> > 				    && (ima_appraise & IMA_APPRAISE_MODULES))) {
> >-- 
> >2.7.5
> >
>

Patch
diff mbox series

diff --git a/arch/x86/kernel/ima_arch.c b/arch/x86/kernel/ima_arch.c
index e47cd9390ab4..96a023238a83 100644
--- a/arch/x86/kernel/ima_arch.c
+++ b/arch/x86/kernel/ima_arch.c
@@ -64,12 +64,19 @@  static const char * const sb_arch_rules[] = {
 	"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig",
 #endif /* CONFIG_KEXEC_VERIFY_SIG */
 	"measure func=KEXEC_KERNEL_CHECK",
+#if !IS_ENABLED(CONFIG_MODULE_SIG)
+	"appraise func=MODULE_CHECK appraise_type=imasig",
+#endif
+	"measure func=MODULE_CHECK",
 	NULL
 };
 
 const char * const *arch_get_ima_policy(void)
 {
-	if (IS_ENABLED(CONFIG_IMA_ARCH_POLICY) && arch_ima_get_secureboot())
+	if (IS_ENABLED(CONFIG_IMA_ARCH_POLICY) && arch_ima_get_secureboot()) {
+		if (IS_ENABLED(CONFIG_MODULE_SIG))
+			set_module_sig_required();
 		return sb_arch_rules;
+	}
 	return NULL;
 }
diff --git a/include/linux/module.h b/include/linux/module.h
index 8fa38d3e7538..af51c8ec755f 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -659,7 +659,8 @@  static inline bool is_livepatch_module(struct module *mod)
 }
 #endif /* CONFIG_LIVEPATCH */
 
-bool is_module_sig_enforced(void);
+bool is_module_sig_enforced_or_required(void);
+void set_module_sig_required(void);
 
 #else /* !CONFIG_MODULES... */
 
@@ -780,6 +781,10 @@  static inline bool is_module_sig_enforced(void)
 	return false;
 }
 
+static inline void set_module_sig_required(void)
+{
+}
+
 /* Dereference module function descriptor */
 static inline
 void *dereference_module_function_descriptor(struct module *mod, void *ptr)
diff --git a/kernel/module.c b/kernel/module.c
index 2ad1b5239910..70a9709d19eb 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -275,16 +275,23 @@  static void module_assert_mutex_or_preempt(void)
 
 static bool sig_enforce = IS_ENABLED(CONFIG_MODULE_SIG_FORCE);
 module_param(sig_enforce, bool_enable_only, 0644);
+static bool sig_required;
 
 /*
  * Export sig_enforce kernel cmdline parameter to allow other subsystems rely
  * on that instead of directly to CONFIG_MODULE_SIG_FORCE config.
  */
-bool is_module_sig_enforced(void)
+bool is_module_sig_enforced_or_required(void)
 {
-	return sig_enforce;
+	return sig_enforce || sig_required;
 }
-EXPORT_SYMBOL(is_module_sig_enforced);
+EXPORT_SYMBOL(is_module_sig_enforced_or_required);
+
+void set_module_sig_required(void)
+{
+	sig_required = true;
+}
+EXPORT_SYMBOL(set_module_sig_required);
 
 /* Block module loading/unloading? */
 int modules_disabled = 0;
@@ -2789,7 +2796,7 @@  static int module_sig_check(struct load_info *info, int flags)
 	}
 
 	/* Not having a signature is only an error if we're strict. */
-	if (err == -ENOKEY && !is_module_sig_enforced())
+	if (err == -ENOKEY && !is_module_sig_enforced_or_required())
 		err = 0;
 
 	return err;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 357edd140c09..bbaf87f688be 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -563,7 +563,7 @@  int ima_load_data(enum kernel_load_data_id id)
 		}
 		break;
 	case LOADING_MODULE:
-		sig_enforce = is_module_sig_enforced();
+		sig_enforce = is_module_sig_enforced_or_required();
 
 		if (ima_enforce && (!sig_enforce
 				    && (ima_appraise & IMA_APPRAISE_MODULES))) {