[PATCH v3 0/2] ima: change how MODULE_SIG_FORCE is checked on modules checking policy

[PATCH v3 0/2] ima: change how MODULE_SIG_FORCE is checked on modules checking policy

[Bruno E. O. Meneguele]

This patchset ensure that IMA's modules checking policy:

    measure func=MODULE_CHECK uid=0

rely on the correct value of CONFIG_MODULE_SIG_FORCE, since the way it
is today the code completely ignores the module.sig_enforce cmdline
param, which behaves in a OR logic with the CONFIG value
(CONFIG_MODULE_SIG_FORCE || module.sig_enforce). That said, everytime a
module would load, in the current checking code, when the kernel was not
compiled with the CONFIG set the call to init_module syscall fails with
-EACCES:

# strace -f -v modprobe <any-module> | grep init_module
init_module(0x55b9bcc9bba0, 17763, "") = -1 EACCES (Permission denied)

With this patchset the result would rely on the module.sig_enforce
cmdline as well. Once the CONFIG is not set, but the param is, the
result would be 'success', as it should be:

# strace -f -v modprobe <any-module> | grep init_module
init_module(0x7f9602d6e010, 386646, "") = 0

The patchset was tested in two different kernels: 4.13.6 (Fedora 27) and
4.14.0-rc4 (integrity-next tree).

Changes:
    v3: 
        1/2 - added dummy is_module_sig_enforced() function to the case
        where CONFIG_MODULES is not set and IMA=y.
    v2:
        1/2 - code changes to correct checkpatch.pl warnings.

Bruno E. O. Meneguele (2):
  module: export module signature enforcement status
  ima: check signature enforcement against cmdline param instead of
    CONFIG

 include/linux/module.h            |  7 +++++++
 kernel/module.c                   | 10 ++++++++++
 security/integrity/ima/ima_main.c |  6 +++---
 3 files changed, 20 insertions(+), 3 deletions(-)

-- 
2.13.6

[PATCH v3 0/2] ima: change how MODULE_SIG_FORCE is checked on modules checking policy

[Bruno E. O. Meneguele]

This patchset ensure that IMA's modules checking policy:

    measure func=MODULE_CHECK uid=0

rely on the correct value of CONFIG_MODULE_SIG_FORCE, since the way it
is today the code completely ignores the module.sig_enforce cmdline
param, which behaves in a OR logic with the CONFIG value
(CONFIG_MODULE_SIG_FORCE || module.sig_enforce). That said, everytime a
module would load, in the current checking code, when the kernel was not
compiled with the CONFIG set the call to init_module syscall fails with
-EACCES:

# strace -f -v modprobe <any-module> | grep init_module
init_module(0x55b9bcc9bba0, 17763, "") = -1 EACCES (Permission denied)

With this patchset the result would rely on the module.sig_enforce
cmdline as well. Once the CONFIG is not set, but the param is, the
result would be 'success', as it should be:

# strace -f -v modprobe <any-module> | grep init_module
init_module(0x7f9602d6e010, 386646, "") = 0

The patchset was tested in two different kernels: 4.13.6 (Fedora 27) and
4.14.0-rc4 (integrity-next tree).

Changes:
    v3: 
        1/2 - added dummy is_module_sig_enforced() function to the case
        where CONFIG_MODULES is not set and IMA=y.
    v2:
        1/2 - code changes to correct checkpatch.pl warnings.

Bruno E. O. Meneguele (2):
  module: export module signature enforcement status
  ima: check signature enforcement against cmdline param instead of
    CONFIG

 include/linux/module.h            |  7 +++++++
 kernel/module.c                   | 10 ++++++++++
 security/integrity/ima/ima_main.c |  6 +++---
 3 files changed, 20 insertions(+), 3 deletions(-)

-- 
2.13.6

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 1/2] module: export module signature enforcement status

[Bruno E. O. Meneguele]

A static variable sig_enforce is used as status var to indicate the real
value of CONFIG_MODULE_SIG_FORCE, once this one is set the var will hold
true, but if the CONFIG is not set the status var will hold whatever
value is present in the module.sig_enforce kernel cmdline param: true
when =1 and false when =0 or not present.

Considering this cmdline param take place over the CONFIG value when
it's not set, other places in the kernel could misbehave since they
would have only the CONFIG_MODULE_SIG_FORCE value to rely on. Exporting
this status var allows the kernel to rely in the effective value of
module signature enforcement, being it from CONFIG value or cmdline
param.

Signed-off-by: Bruno E. O. Meneguele <brdeoliv@redhat.com>
---
 include/linux/module.h |  7 +++++++
 kernel/module.c        | 10 ++++++++++
 2 files changed, 17 insertions(+)

diff --git a/include/linux/module.h b/include/linux/module.h
index fe5aa3736707..c69b49abe877 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -639,6 +639,8 @@ static inline bool is_livepatch_module(struct module *mod)
 }
 #endif /* CONFIG_LIVEPATCH */
 
+bool is_module_sig_enforced(void);
+
 #else /* !CONFIG_MODULES... */
 
 static inline struct module *__module_address(unsigned long addr)
@@ -753,6 +755,11 @@ static inline bool module_requested_async_probing(struct module *module)
 	return false;
 }
 
+static inline bool is_module_sig_enforced(void)
+{
+	return false;
+}
+
 #endif /* CONFIG_MODULES */
 
 #ifdef CONFIG_SYSFS
diff --git a/kernel/module.c b/kernel/module.c
index de66ec825992..d1c194b057a2 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -278,6 +278,16 @@ static bool sig_enforce = IS_ENABLED(CONFIG_MODULE_SIG_FORCE);
 module_param(sig_enforce, bool_enable_only, 0644);
 #endif /* !CONFIG_MODULE_SIG_FORCE */
 
+/*
+ * 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)
+{
+	return sig_enforce;
+}
+EXPORT_SYMBOL(is_module_sig_enforced);
+
 /* Block module loading/unloading? */
 int modules_disabled = 0;
 core_param(nomodule, modules_disabled, bint, 0);
-- 
2.13.6

[PATCH v3 1/2] module: export module signature enforcement status

[Bruno E. O. Meneguele]

A static variable sig_enforce is used as status var to indicate the real
value of CONFIG_MODULE_SIG_FORCE, once this one is set the var will hold
true, but if the CONFIG is not set the status var will hold whatever
value is present in the module.sig_enforce kernel cmdline param: true
when =1 and false when =0 or not present.

Considering this cmdline param take place over the CONFIG value when
it's not set, other places in the kernel could misbehave since they
would have only the CONFIG_MODULE_SIG_FORCE value to rely on. Exporting
this status var allows the kernel to rely in the effective value of
module signature enforcement, being it from CONFIG value or cmdline
param.

Signed-off-by: Bruno E. O. Meneguele <brdeoliv@redhat.com>
---
 include/linux/module.h |  7 +++++++
 kernel/module.c        | 10 ++++++++++
 2 files changed, 17 insertions(+)

diff --git a/include/linux/module.h b/include/linux/module.h
index fe5aa3736707..c69b49abe877 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -639,6 +639,8 @@ static inline bool is_livepatch_module(struct module *mod)
 }
 #endif /* CONFIG_LIVEPATCH */
 
+bool is_module_sig_enforced(void);
+
 #else /* !CONFIG_MODULES... */
 
 static inline struct module *__module_address(unsigned long addr)
@@ -753,6 +755,11 @@ static inline bool module_requested_async_probing(struct module *module)
 	return false;
 }
 
+static inline bool is_module_sig_enforced(void)
+{
+	return false;
+}
+
 #endif /* CONFIG_MODULES */
 
 #ifdef CONFIG_SYSFS
diff --git a/kernel/module.c b/kernel/module.c
index de66ec825992..d1c194b057a2 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -278,6 +278,16 @@ static bool sig_enforce = IS_ENABLED(CONFIG_MODULE_SIG_FORCE);
 module_param(sig_enforce, bool_enable_only, 0644);
 #endif /* !CONFIG_MODULE_SIG_FORCE */
 
+/*
+ * 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)
+{
+	return sig_enforce;
+}
+EXPORT_SYMBOL(is_module_sig_enforced);
+
 /* Block module loading/unloading? */
 int modules_disabled = 0;
 core_param(nomodule, modules_disabled, bint, 0);
-- 
2.13.6

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/2] ima: check signature enforcement against cmdline param instead of CONFIG

[Bruno E. O. Meneguele]

When the user requests MODULE_CHECK policy and its kernel is compiled
with CONFIG_MODULE_SIG_FORCE not set, all modules would not load, just
those loaded in initram time. One option the user would have would be
set a kernel cmdline param (module.sig_enforce) to true, but the IMA
module check code doesn't rely on this value, it checks just
CONFIG_MODULE_SIG_FORCE.

This patch solves this problem checking for the exported value of
module.sig_enforce cmdline param intead of CONFIG_MODULE_SIG_FORCE,
which holds the effective value (CONFIG || param).

Signed-off-by: Bruno E. O. Meneguele <brdeoliv@redhat.com>
---
 security/integrity/ima/ima_main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index e4ab8ef8016e..d11a7fcc5c8b 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -356,12 +356,12 @@ void ima_post_path_mknod(struct dentry *dentry)
  */
 int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
 {
+	bool sig_enforce = is_module_sig_enforced();
+
 	if (!file && read_id == READING_MODULE) {
-#ifndef CONFIG_MODULE_SIG_FORCE
-		if ((ima_appraise & IMA_APPRAISE_MODULES) &&
+		if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES) &&
 		    (ima_appraise & IMA_APPRAISE_ENFORCE))
 			return -EACCES;	/* INTEGRITY_UNKNOWN */
-#endif
 		return 0;	/* We rely on module signature checking */
 	}
 	return 0;
-- 
2.13.6

[PATCH v3 2/2] ima: check signature enforcement against cmdline param instead of CONFIG

[Bruno E. O. Meneguele]

When the user requests MODULE_CHECK policy and its kernel is compiled
with CONFIG_MODULE_SIG_FORCE not set, all modules would not load, just
those loaded in initram time. One option the user would have would be
set a kernel cmdline param (module.sig_enforce) to true, but the IMA
module check code doesn't rely on this value, it checks just
CONFIG_MODULE_SIG_FORCE.

This patch solves this problem checking for the exported value of
module.sig_enforce cmdline param intead of CONFIG_MODULE_SIG_FORCE,
which holds the effective value (CONFIG || param).

Signed-off-by: Bruno E. O. Meneguele <brdeoliv@redhat.com>
---
 security/integrity/ima/ima_main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index e4ab8ef8016e..d11a7fcc5c8b 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -356,12 +356,12 @@ void ima_post_path_mknod(struct dentry *dentry)
  */
 int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
 {
+	bool sig_enforce = is_module_sig_enforced();
+
 	if (!file && read_id == READING_MODULE) {
-#ifndef CONFIG_MODULE_SIG_FORCE
-		if ((ima_appraise & IMA_APPRAISE_MODULES) &&
+		if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES) &&
 		    (ima_appraise & IMA_APPRAISE_ENFORCE))
 			return -EACCES;	/* INTEGRITY_UNKNOWN */
-#endif
 		return 0;	/* We rely on module signature checking */
 	}
 	return 0;
-- 
2.13.6

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] ima: check signature enforcement against cmdline param instead of CONFIG

[Mimi Zohar]

On Tue, 2017-10-24 at 15:37 -0200, Bruno E. O. Meneguele wrote:
> When the user requests MODULE_CHECK policy and its kernel is compiled
> with CONFIG_MODULE_SIG_FORCE not set, all modules would not load, just
> those loaded in initram time. One option the user would have would be
> set a kernel cmdline param (module.sig_enforce) to true, but the IMA
> module check code doesn't rely on this value, it checks just
> CONFIG_MODULE_SIG_FORCE.
> 
> This patch solves this problem checking for the exported value of
> module.sig_enforce cmdline param intead of CONFIG_MODULE_SIG_FORCE,
> which holds the effective value (CONFIG || param).
> 
> Signed-off-by: Bruno E. O. Meneguele <brdeoliv@redhat.com>
> ---
>  security/integrity/ima/ima_main.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index e4ab8ef8016e..d11a7fcc5c8b 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -356,12 +356,12 @@ void ima_post_path_mknod(struct dentry *dentry)
>   */
>  int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
>  {
> +	bool sig_enforce = is_module_sig_enforced();
> +
>  	if (!file && read_id == READING_MODULE) {

The only reason for getting here is that you're using the old module
load syscall.  Is there a reason for not using the new one, which
passes the file descriptor?

thanks,

Mimi

> -#ifndef CONFIG_MODULE_SIG_FORCE
> -		if ((ima_appraise & IMA_APPRAISE_MODULES) &&
> +		if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES) &&
>  		    (ima_appraise & IMA_APPRAISE_ENFORCE))
>  			return -EACCES;	/* INTEGRITY_UNKNOWN */
> -#endif
>  		return 0;	/* We rely on module signature checking */
>  	}
>  	return 0;

Re: [PATCH v3 2/2] ima: check signature enforcement against cmdline param instead of CONFIG

[Mimi Zohar]

On Tue, 2017-10-24 at 15:37 -0200, Bruno E. O. Meneguele wrote:
> When the user requests MODULE_CHECK policy and its kernel is compiled
> with CONFIG_MODULE_SIG_FORCE not set, all modules would not load, just
> those loaded in initram time. One option the user would have would be
> set a kernel cmdline param (module.sig_enforce) to true, but the IMA
> module check code doesn't rely on this value, it checks just
> CONFIG_MODULE_SIG_FORCE.
> 
> This patch solves this problem checking for the exported value of
> module.sig_enforce cmdline param intead of CONFIG_MODULE_SIG_FORCE,
> which holds the effective value (CONFIG || param).
> 
> Signed-off-by: Bruno E. O. Meneguele <brdeoliv@redhat.com>
> ---
>  security/integrity/ima/ima_main.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index e4ab8ef8016e..d11a7fcc5c8b 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -356,12 +356,12 @@ void ima_post_path_mknod(struct dentry *dentry)
>   */
>  int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
>  {
> +	bool sig_enforce = is_module_sig_enforced();
> +
>  	if (!file && read_id == READING_MODULE) {

The only reason for getting here is that you're using the old module
load syscall.  Is there a reason for not using the new one, which
passes the file descriptor?

thanks,

Mimi

> -#ifndef CONFIG_MODULE_SIG_FORCE
> -		if ((ima_appraise & IMA_APPRAISE_MODULES) &&
> +		if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES) &&
>  		    (ima_appraise & IMA_APPRAISE_ENFORCE))
>  			return -EACCES;	/* INTEGRITY_UNKNOWN */
> -#endif
>  		return 0;	/* We rely on module signature checking */
>  	}
>  	return 0;

[PATCH v3 2/2] ima: check signature enforcement against cmdline param instead of CONFIG

[Mimi Zohar]

On Tue, 2017-10-24 at 15:37 -0200, Bruno E. O. Meneguele wrote:
> When the user requests MODULE_CHECK policy and its kernel is compiled
> with CONFIG_MODULE_SIG_FORCE not set, all modules would not load, just
> those loaded in initram time. One option the user would have would be
> set a kernel cmdline param (module.sig_enforce) to true, but the IMA
> module check code doesn't rely on this value, it checks just
> CONFIG_MODULE_SIG_FORCE.
> 
> This patch solves this problem checking for the exported value of
> module.sig_enforce cmdline param intead of CONFIG_MODULE_SIG_FORCE,
> which holds the effective value (CONFIG || param).
> 
> Signed-off-by: Bruno E. O. Meneguele <brdeoliv@redhat.com>
> ---
>  security/integrity/ima/ima_main.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index e4ab8ef8016e..d11a7fcc5c8b 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -356,12 +356,12 @@ void ima_post_path_mknod(struct dentry *dentry)
>   */
>  int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
>  {
> +	bool sig_enforce = is_module_sig_enforced();
> +
>  	if (!file && read_id == READING_MODULE) {

The only reason for getting here is that you're using the old module
load syscall. ?Is there a reason for not using the new one, which
passes the file descriptor?

thanks,

Mimi

> -#ifndef CONFIG_MODULE_SIG_FORCE
> -		if ((ima_appraise & IMA_APPRAISE_MODULES) &&
> +		if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES) &&
>  		    (ima_appraise & IMA_APPRAISE_ENFORCE))
>  			return -EACCES;	/* INTEGRITY_UNKNOWN */
> -#endif
>  		return 0;	/* We rely on module signature checking */
>  	}
>  	return 0;


--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] ima: check signature enforcement against cmdline param instead of CONFIG

[Bruno E. O. Meneguele]

[-- Attachment #1: Type: multipart/signed, Size: 2656 bytes --]

Re: [PATCH v3 2/2] ima: check signature enforcement against cmdline param instead of CONFIG

[Bruno E. O. Meneguele]

[-- Attachment #1: Type: text/plain, Size: 2608 bytes --]

On 24-10, Mimi Zohar wrote:
> On Tue, 2017-10-24 at 15:37 -0200, Bruno E. O. Meneguele wrote:
> > When the user requests MODULE_CHECK policy and its kernel is compiled
> > with CONFIG_MODULE_SIG_FORCE not set, all modules would not load, just
> > those loaded in initram time. One option the user would have would be
> > set a kernel cmdline param (module.sig_enforce) to true, but the IMA
> > module check code doesn't rely on this value, it checks just
> > CONFIG_MODULE_SIG_FORCE.
> > 
> > This patch solves this problem checking for the exported value of
> > module.sig_enforce cmdline param intead of CONFIG_MODULE_SIG_FORCE,
> > which holds the effective value (CONFIG || param).
> > 
> > Signed-off-by: Bruno E. O. Meneguele <brdeoliv@redhat.com>
> > ---
> >  security/integrity/ima/ima_main.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index e4ab8ef8016e..d11a7fcc5c8b 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -356,12 +356,12 @@ void ima_post_path_mknod(struct dentry *dentry)
> >   */
> >  int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
> >  {
> > +	bool sig_enforce = is_module_sig_enforced();
> > +
> >  	if (!file && read_id == READING_MODULE) {
> 
> The only reason for getting here is that you're using the old module
> load syscall.  Is there a reason for not using the new one, which
> passes the file descriptor?
> 
> thanks,
> 
> Mimi
> 

Basicaly because the way kmod handles compressed (gz/xz) modules. The
way it's today would require major changes in the code or some kind of
memfd_create() + xattrs reassignement in order to finit_module() be used
correctly.

Considering it would take some time to be accepted or even to figure out
the correct way to tackle it, the current IMA module check code works
aside kernel module signature validation, which is fine for now for me,
but has the problem that this patch tries to solve in the
CONFIG_MODULE_SIG_FORCE check (ignoring module.sig_enforce cmdline
param).

> > -#ifndef CONFIG_MODULE_SIG_FORCE
> > -		if ((ima_appraise & IMA_APPRAISE_MODULES) &&
> > +		if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES) &&
> >  		    (ima_appraise & IMA_APPRAISE_ENFORCE))
> >  			return -EACCES;	/* INTEGRITY_UNKNOWN */
> > -#endif
> >  		return 0;	/* We rely on module signature checking */
> >  	}
> >  	return 0;
> 
> 

-- 
bmeneg 
PGP Key: http://bmeneg.com/pubkey.txt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 2/2] ima: check signature enforcement against cmdline param instead of CONFIG

[Mimi Zohar]

On Wed, 2017-10-25 at 13:05 -0200, Bruno E. O. Meneguele wrote:
> On 24-10, Mimi Zohar wrote:
> > On Tue, 2017-10-24 at 15:37 -0200, Bruno E. O. Meneguele wrote:
> > > When the user requests MODULE_CHECK policy and its kernel is compiled
> > > with CONFIG_MODULE_SIG_FORCE not set, all modules would not load, just
> > > those loaded in initram time. One option the user would have would be
> > > set a kernel cmdline param (module.sig_enforce) to true, but the IMA
> > > module check code doesn't rely on this value, it checks just
> > > CONFIG_MODULE_SIG_FORCE.
> > > 
> > > This patch solves this problem checking for the exported value of
> > > module.sig_enforce cmdline param intead of CONFIG_MODULE_SIG_FORCE,
> > > which holds the effective value (CONFIG || param).
> > > 
> > > Signed-off-by: Bruno E. O. Meneguele <brdeoliv@redhat.com>
> > > ---
> > >  security/integrity/ima/ima_main.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > > index e4ab8ef8016e..d11a7fcc5c8b 100644
> > > --- a/security/integrity/ima/ima_main.c
> > > +++ b/security/integrity/ima/ima_main.c
> > > @@ -356,12 +356,12 @@ void ima_post_path_mknod(struct dentry *dentry)
> > >   */
> > >  int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
> > >  {
> > > +	bool sig_enforce = is_module_sig_enforced();
> > > +
> > >  	if (!file && read_id == READING_MODULE) {
> > 
> > The only reason for getting here is that you're using the old module
> > load syscall.  Is there a reason for not using the new one, which
> > passes the file descriptor?

> Basicaly because the way kmod handles compressed (gz/xz) modules. The
> way it's today would require major changes in the code or some kind of
> memfd_create() + xattrs reassignement in order to finit_module() be used
> correctly.
> 
> Considering it would take some time to be accepted or even to figure out
> the correct way to tackle it, the current IMA module check code works
> aside kernel module signature validation, which is fine for now for me,
> but has the problem that this patch tries to solve in the
> CONFIG_MODULE_SIG_FORCE check (ignoring module.sig_enforce cmdline
> param).

Thank you for the reasoning.  BTW, these patches are now queued.


> > > -#ifndef CONFIG_MODULE_SIG_FORCE
> > > -		if ((ima_appraise & IMA_APPRAISE_MODULES) &&
> > > +		if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES) &&
> > >  		    (ima_appraise & IMA_APPRAISE_ENFORCE))
> > >  			return -EACCES;	/* INTEGRITY_UNKNOWN */
> > > -#endif
> > >  		return 0;	/* We rely on module signature checking */
> > >  	}
> > >  	return 0;
> > 
> > 
> 

Re: [PATCH v3 2/2] ima: check signature enforcement against cmdline param instead of CONFIG

[Mimi Zohar]

On Wed, 2017-10-25 at 13:05 -0200, Bruno E. O. Meneguele wrote:
> On 24-10, Mimi Zohar wrote:
> > On Tue, 2017-10-24 at 15:37 -0200, Bruno E. O. Meneguele wrote:
> > > When the user requests MODULE_CHECK policy and its kernel is compiled
> > > with CONFIG_MODULE_SIG_FORCE not set, all modules would not load, just
> > > those loaded in initram time. One option the user would have would be
> > > set a kernel cmdline param (module.sig_enforce) to true, but the IMA
> > > module check code doesn't rely on this value, it checks just
> > > CONFIG_MODULE_SIG_FORCE.
> > > 
> > > This patch solves this problem checking for the exported value of
> > > module.sig_enforce cmdline param intead of CONFIG_MODULE_SIG_FORCE,
> > > which holds the effective value (CONFIG || param).
> > > 
> > > Signed-off-by: Bruno E. O. Meneguele <brdeoliv@redhat.com>
> > > ---
> > >  security/integrity/ima/ima_main.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > > index e4ab8ef8016e..d11a7fcc5c8b 100644
> > > --- a/security/integrity/ima/ima_main.c
> > > +++ b/security/integrity/ima/ima_main.c
> > > @@ -356,12 +356,12 @@ void ima_post_path_mknod(struct dentry *dentry)
> > >   */
> > >  int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
> > >  {
> > > +	bool sig_enforce = is_module_sig_enforced();
> > > +
> > >  	if (!file && read_id == READING_MODULE) {
> > 
> > The only reason for getting here is that you're using the old module
> > load syscall.  Is there a reason for not using the new one, which
> > passes the file descriptor?

> Basicaly because the way kmod handles compressed (gz/xz) modules. The
> way it's today would require major changes in the code or some kind of
> memfd_create() + xattrs reassignement in order to finit_module() be used
> correctly.
> 
> Considering it would take some time to be accepted or even to figure out
> the correct way to tackle it, the current IMA module check code works
> aside kernel module signature validation, which is fine for now for me,
> but has the problem that this patch tries to solve in the
> CONFIG_MODULE_SIG_FORCE check (ignoring module.sig_enforce cmdline
> param).

Thank you for the reasoning.  BTW, these patches are now queued.


> > > -#ifndef CONFIG_MODULE_SIG_FORCE
> > > -		if ((ima_appraise & IMA_APPRAISE_MODULES) &&
> > > +		if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES) &&
> > >  		    (ima_appraise & IMA_APPRAISE_ENFORCE))
> > >  			return -EACCES;	/* INTEGRITY_UNKNOWN */
> > > -#endif
> > >  		return 0;	/* We rely on module signature checking */
> > >  	}
> > >  	return 0;
> > 
> > 
> 

[PATCH v3 2/2] ima: check signature enforcement against cmdline param instead of CONFIG

[Mimi Zohar]

On Wed, 2017-10-25 at 13:05 -0200, Bruno E. O. Meneguele wrote:
> On 24-10, Mimi Zohar wrote:
> > On Tue, 2017-10-24 at 15:37 -0200, Bruno E. O. Meneguele wrote:
> > > When the user requests MODULE_CHECK policy and its kernel is compiled
> > > with CONFIG_MODULE_SIG_FORCE not set, all modules would not load, just
> > > those loaded in initram time. One option the user would have would be
> > > set a kernel cmdline param (module.sig_enforce) to true, but the IMA
> > > module check code doesn't rely on this value, it checks just
> > > CONFIG_MODULE_SIG_FORCE.
> > > 
> > > This patch solves this problem checking for the exported value of
> > > module.sig_enforce cmdline param intead of CONFIG_MODULE_SIG_FORCE,
> > > which holds the effective value (CONFIG || param).
> > > 
> > > Signed-off-by: Bruno E. O. Meneguele <brdeoliv@redhat.com>
> > > ---
> > >  security/integrity/ima/ima_main.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > > index e4ab8ef8016e..d11a7fcc5c8b 100644
> > > --- a/security/integrity/ima/ima_main.c
> > > +++ b/security/integrity/ima/ima_main.c
> > > @@ -356,12 +356,12 @@ void ima_post_path_mknod(struct dentry *dentry)
> > >   */
> > >  int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
> > >  {
> > > +	bool sig_enforce = is_module_sig_enforced();
> > > +
> > >  	if (!file && read_id == READING_MODULE) {
> > 
> > The only reason for getting here is that you're using the old module
> > load syscall. ?Is there a reason for not using the new one, which
> > passes the file descriptor?

> Basicaly because the way kmod handles compressed (gz/xz) modules. The
> way it's today would require major changes in the code or some kind of
> memfd_create() + xattrs reassignement in order to finit_module() be used
> correctly.
> 
> Considering it would take some time to be accepted or even to figure out
> the correct way to tackle it, the current IMA module check code works
> aside kernel module signature validation, which is fine for now for me,
> but has the problem that this patch tries to solve in the
> CONFIG_MODULE_SIG_FORCE check (ignoring module.sig_enforce cmdline
> param).

Thank you for the reasoning. ?BTW, these patches are now queued.


> > > -#ifndef CONFIG_MODULE_SIG_FORCE
> > > -		if ((ima_appraise & IMA_APPRAISE_MODULES) &&
> > > +		if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES) &&
> > >  		    (ima_appraise & IMA_APPRAISE_ENFORCE))
> > >  			return -EACCES;	/* INTEGRITY_UNKNOWN */
> > > -#endif
> > >  		return 0;	/* We rely on module signature checking */
> > >  	}
> > >  	return 0;
> > 
> > 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] ima: check signature enforcement against cmdline param instead of CONFIG

[Bruno E. O. Meneguele]

[-- Attachment #1: Type: multipart/signed, Size: 3075 bytes --]

Re: [PATCH v3 2/2] ima: check signature enforcement against cmdline param instead of CONFIG

[Bruno E. O. Meneguele]

[-- Attachment #1: Type: text/plain, Size: 3032 bytes --]

On 25-10, Mimi Zohar wrote:
> On Wed, 2017-10-25 at 13:05 -0200, Bruno E. O. Meneguele wrote:
> > On 24-10, Mimi Zohar wrote:
> > > On Tue, 2017-10-24 at 15:37 -0200, Bruno E. O. Meneguele wrote:
> > > > When the user requests MODULE_CHECK policy and its kernel is compiled
> > > > with CONFIG_MODULE_SIG_FORCE not set, all modules would not load, just
> > > > those loaded in initram time. One option the user would have would be
> > > > set a kernel cmdline param (module.sig_enforce) to true, but the IMA
> > > > module check code doesn't rely on this value, it checks just
> > > > CONFIG_MODULE_SIG_FORCE.
> > > > 
> > > > This patch solves this problem checking for the exported value of
> > > > module.sig_enforce cmdline param intead of CONFIG_MODULE_SIG_FORCE,
> > > > which holds the effective value (CONFIG || param).
> > > > 
> > > > Signed-off-by: Bruno E. O. Meneguele <brdeoliv@redhat.com>
> > > > ---
> > > >  security/integrity/ima/ima_main.c | 6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > > > index e4ab8ef8016e..d11a7fcc5c8b 100644
> > > > --- a/security/integrity/ima/ima_main.c
> > > > +++ b/security/integrity/ima/ima_main.c
> > > > @@ -356,12 +356,12 @@ void ima_post_path_mknod(struct dentry *dentry)
> > > >   */
> > > >  int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
> > > >  {
> > > > +	bool sig_enforce = is_module_sig_enforced();
> > > > +
> > > >  	if (!file && read_id == READING_MODULE) {
> > > 
> > > The only reason for getting here is that you're using the old module
> > > load syscall.  Is there a reason for not using the new one, which
> > > passes the file descriptor?
> 
> > Basicaly because the way kmod handles compressed (gz/xz) modules. The
> > way it's today would require major changes in the code or some kind of
> > memfd_create() + xattrs reassignement in order to finit_module() be used
> > correctly.
> > 
> > Considering it would take some time to be accepted or even to figure out
> > the correct way to tackle it, the current IMA module check code works
> > aside kernel module signature validation, which is fine for now for me,
> > but has the problem that this patch tries to solve in the
> > CONFIG_MODULE_SIG_FORCE check (ignoring module.sig_enforce cmdline
> > param).
> 
> Thank you for the reasoning.  BTW, these patches are now queued.
> 
> 

You're welcome. Thank you for the feedback.

> > > > -#ifndef CONFIG_MODULE_SIG_FORCE
> > > > -		if ((ima_appraise & IMA_APPRAISE_MODULES) &&
> > > > +		if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES) &&
> > > >  		    (ima_appraise & IMA_APPRAISE_ENFORCE))
> > > >  			return -EACCES;	/* INTEGRITY_UNKNOWN */
> > > > -#endif
> > > >  		return 0;	/* We rely on module signature checking */
> > > >  	}
> > > >  	return 0;
> > > 
> > > 
> > 
> 

-- 
bmeneg 
PGP Key: http://bmeneg.com/pubkey.txt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]