* [PATCH] integrity: prevent deadlock during digsig verification.
@ 2018-06-27 13:33 Mikhail Kurinnoi
2018-06-28 16:39 ` Matthias Gerstner
2018-06-28 18:43 ` Mimi Zohar
0 siblings, 2 replies; 6+ messages in thread
From: Mikhail Kurinnoi @ 2018-06-27 13:33 UTC (permalink / raw)
To: linux-integrity; +Cc: Matthias Gerstner, Mimi Zohar
This patch aimed to prevent deadlock during digsig verification.The point
of issue - user space utility modprobe and/or it's dependencies (ld-*.so,
libz.so.*, libc-*.so and /lib/modules/ files) that could be used for
kernel modules load during digsig verification and could be signed by
digsig in the same time.
First at all, look at crypto_alloc_tfm() work algorithm:
crypto_alloc_tfm() will first attempt to locate an already loaded
algorithm. If that fails and the kernel supports dynamically loadable
modules, it will then attempt to load a module of the same name or alias.
If that fails it will send a query to any loaded crypto manager to
construct an algorithm on the fly.
We have situation, when public_key_verify_signature() in case of RSA
algorithm use alg_name to store internal information in order to construct
an algorithm on the fly, but crypto_larval_lookup() will try to use
alg_name in order to load kernel module with same name.
1) we can't do anything with crypto module work, since it designed to work
exactly in this way;
2) we can't globally filter module requests for modprobe, since it
designed to work with any requests.
In this patch, I propose add an exception for "crypto-pkcs1pad(rsa,*)"
module requests only in case of enabled integrity asymmetric keys support.
Since we don't have any real "crypto-pkcs1pad(rsa,*)" kernel modules for
sure, we are safe to fail such module request from crypto_larval_lookup().
In this way we prevent modprobe execution during digsig verification and
avoid possible deadlock if modprobe and/or it's dependencies also signed
with digsig.
Requested "crypto-pkcs1pad(rsa,*)" kernel module name formed by:
1) "pkcs1pad(rsa,%s)" in public_key_verify_signature();
2) "crypto-%s" / "crypto-%s-all" in crypto_larval_lookup().
"crypto-pkcs1pad(rsa," part of request is a constant and unique and could
be used as filter.
Signed-off-by: Mikhail Kurinnoi <viewizard@viewizard.com>
include/linux/integrity.h | 13 +++++++++++++
security/integrity/digsig_asymmetric.c | 23 +++++++++++++++++++++++
security/security.c | 7 ++++++-
3 files changed, 42 insertions(+), 1 deletion(-)
diff --git a/include/linux/integrity.h b/include/linux/integrity.h
index c2d6082..8678e32 100644
--- a/include/linux/integrity.h
+++ b/include/linux/integrity.h
@@ -43,4 +43,17 @@ static inline void integrity_load_keys(void)
}
#endif /* CONFIG_INTEGRITY */
+#ifdef CONFIG_INTEGRITY_ASYMMETRIC_KEYS
+
+extern int integrity_kernel_module_request(char *kmod_name);
+
+#else
+
+static inline int integrity_kernel_module_request(char *kmod_name)
+{
+ return 0;
+}
+
+#endif /* CONFIG_INTEGRITY_ASYMMETRIC_KEYS */
+
#endif /* _LINUX_INTEGRITY_H */
diff --git a/security/integrity/digsig_asymmetric.c b/security/integrity/digsig_asymmetric.c
index 80052ed..f1ab90c 100644
--- a/security/integrity/digsig_asymmetric.c
+++ b/security/integrity/digsig_asymmetric.c
@@ -115,3 +115,26 @@ int asymmetric_verify(struct key *keyring, const char *sig,
pr_debug("%s() = %d\n", __func__, ret);
return ret;
}
+
+/**
+ * integrity_kernel_module_request - prevent crypto-pkcs1pad(rsa,*) requests
+ * @kmod_name: kernel module name
+ *
+ * We have situation, when public_key_verify_signature() in case of RSA
+ * algorithm use alg_name to store internal information in order to
+ * construct an algorithm on the fly, but crypto_larval_lookup() will try
+ * to use alg_name in order to load kernel module with same name.
+ * Since we don't have any real "crypto-pkcs1pad(rsa,*)" kernel modules,
+ * we are safe to fail such module request from crypto_larval_lookup().
+ *
+ * In this way we prevent modprobe execution during digsig verification
+ * and avoid possible deadlock if modprobe and/or it's dependencies
+ * also signed with digsig.
+ */
+int integrity_kernel_module_request(char *kmod_name)
+{
+ if (strncmp(kmod_name, "crypto-pkcs1pad(rsa,", 20) == 0)
+ return -EINVAL;
+
+ return 0;
+}
diff --git a/security/security.c b/security/security.c
index f825304..d53b4cb 100644
--- a/security/security.c
+++ b/security/security.c
@@ -929,7 +929,12 @@ int security_kernel_create_files_as(struct cred *new, struct inode *inode)
int security_kernel_module_request(char *kmod_name)
{
- return call_int_hook(kernel_module_request, 0, kmod_name);
+ int ret;
+
+ ret = call_int_hook(kernel_module_request, 0, kmod_name);
+ if (ret)
+ return ret;
+ return integrity_kernel_module_request(kmod_name);
}
int security_kernel_read_file(struct file *file, enum kernel_read_file_id id)
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] integrity: prevent deadlock during digsig verification.
2018-06-27 13:33 [PATCH] integrity: prevent deadlock during digsig verification Mikhail Kurinnoi
@ 2018-06-28 16:39 ` Matthias Gerstner
2018-06-28 19:14 ` Mimi Zohar
2018-06-28 18:43 ` Mimi Zohar
1 sibling, 1 reply; 6+ messages in thread
From: Matthias Gerstner @ 2018-06-28 16:39 UTC (permalink / raw)
To: Mikhail Kurinnoi; +Cc: linux-integrity, Mimi Zohar
[-- Attachment #1: Type: multipart/signed, Size: 370 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] integrity: prevent deadlock during digsig verification.
2018-06-27 13:33 [PATCH] integrity: prevent deadlock during digsig verification Mikhail Kurinnoi
2018-06-28 16:39 ` Matthias Gerstner
@ 2018-06-28 18:43 ` Mimi Zohar
1 sibling, 0 replies; 6+ messages in thread
From: Mimi Zohar @ 2018-06-28 18:43 UTC (permalink / raw)
To: Mikhail Kurinnoi, linux-integrity; +Cc: Matthias Gerstner
Hi Mikhail,
On Wed, 2018-06-27 at 16:33 +0300, Mikhail Kurinnoi wrote:
> This patch aimed to prevent deadlock during digsig verification.The point
> of issue - user space utility modprobe and/or it's dependencies (ld-*.so,
> libz.so.*, libc-*.so and /lib/modules/ files) that could be used for
> kernel modules load during digsig verification and could be signed by
> digsig in the same time.
>
> First at all, look at crypto_alloc_tfm() work algorithm:
> crypto_alloc_tfm() will first attempt to locate an already loaded
> algorithm. If that fails and the kernel supports dynamically loadable
> modules, it will then attempt to load a module of the same name or alias.
> If that fails it will send a query to any loaded crypto manager to
> construct an algorithm on the fly.
>
> We have situation, when public_key_verify_signature() in case of RSA
> algorithm use alg_name to store internal information in order to construct
> an algorithm on the fly, but crypto_larval_lookup() will try to use
> alg_name in order to load kernel module with same name.
>
> 1) we can't do anything with crypto module work, since it designed to work
> exactly in this way;
> 2) we can't globally filter module requests for modprobe, since it
> designed to work with any requests.
>
> In this patch, I propose add an exception for "crypto-pkcs1pad(rsa,*)"
> module requests only in case of enabled integrity asymmetric keys support.
> Since we don't have any real "crypto-pkcs1pad(rsa,*)" kernel modules for
> sure, we are safe to fail such module request from crypto_larval_lookup().
> In this way we prevent modprobe execution during digsig verification and
> avoid possible deadlock if modprobe and/or it's dependencies also signed
> with digsig.
>
> Requested "crypto-pkcs1pad(rsa,*)" kernel module name formed by:
> 1) "pkcs1pad(rsa,%s)" in public_key_verify_signature();
> 2) "crypto-%s" / "crypto-%s-all" in crypto_larval_lookup().
> "crypto-pkcs1pad(rsa," part of request is a constant and unique and could
> be used as filter.
>
>
> Signed-off-by: Mikhail Kurinnoi <viewizard@viewizard.com>
Thanks! I've rebased the existing queued patches on James' next-
general branch and pushed it out to next-integrity. This patch is now
queued in the next-integrity-queued branch.
Mimi
>
> include/linux/integrity.h | 13 +++++++++++++
> security/integrity/digsig_asymmetric.c | 23 +++++++++++++++++++++++
> security/security.c | 7 ++++++-
> 3 files changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/integrity.h b/include/linux/integrity.h
> index c2d6082..8678e32 100644
> --- a/include/linux/integrity.h
> +++ b/include/linux/integrity.h
> @@ -43,4 +43,17 @@ static inline void integrity_load_keys(void)
> }
> #endif /* CONFIG_INTEGRITY */
>
> +#ifdef CONFIG_INTEGRITY_ASYMMETRIC_KEYS
> +
> +extern int integrity_kernel_module_request(char *kmod_name);
> +
> +#else
> +
> +static inline int integrity_kernel_module_request(char *kmod_name)
> +{
> + return 0;
> +}
> +
> +#endif /* CONFIG_INTEGRITY_ASYMMETRIC_KEYS */
> +
> #endif /* _LINUX_INTEGRITY_H */
> diff --git a/security/integrity/digsig_asymmetric.c b/security/integrity/digsig_asymmetric.c
> index 80052ed..f1ab90c 100644
> --- a/security/integrity/digsig_asymmetric.c
> +++ b/security/integrity/digsig_asymmetric.c
> @@ -115,3 +115,26 @@ int asymmetric_verify(struct key *keyring, const char *sig,
> pr_debug("%s() = %d\n", __func__, ret);
> return ret;
> }
> +
> +/**
> + * integrity_kernel_module_request - prevent crypto-pkcs1pad(rsa,*) requests
> + * @kmod_name: kernel module name
> + *
> + * We have situation, when public_key_verify_signature() in case of RSA
> + * algorithm use alg_name to store internal information in order to
> + * construct an algorithm on the fly, but crypto_larval_lookup() will try
> + * to use alg_name in order to load kernel module with same name.
> + * Since we don't have any real "crypto-pkcs1pad(rsa,*)" kernel modules,
> + * we are safe to fail such module request from crypto_larval_lookup().
> + *
> + * In this way we prevent modprobe execution during digsig verification
> + * and avoid possible deadlock if modprobe and/or it's dependencies
> + * also signed with digsig.
> + */
> +int integrity_kernel_module_request(char *kmod_name)
> +{
> + if (strncmp(kmod_name, "crypto-pkcs1pad(rsa,", 20) == 0)
> + return -EINVAL;
> +
> + return 0;
> +}
> diff --git a/security/security.c b/security/security.c
> index f825304..d53b4cb 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -929,7 +929,12 @@ int security_kernel_create_files_as(struct cred *new, struct inode *inode)
>
> int security_kernel_module_request(char *kmod_name)
> {
> - return call_int_hook(kernel_module_request, 0, kmod_name);
> + int ret;
> +
> + ret = call_int_hook(kernel_module_request, 0, kmod_name);
> + if (ret)
> + return ret;
> + return integrity_kernel_module_request(kmod_name);
> }
>
> int security_kernel_read_file(struct file *file, enum kernel_read_file_id id)
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] integrity: prevent deadlock during digsig verification.
2018-06-28 16:39 ` Matthias Gerstner
@ 2018-06-28 19:14 ` Mimi Zohar
2018-06-28 20:50 ` Mikhail Kurinnoi
0 siblings, 1 reply; 6+ messages in thread
From: Mimi Zohar @ 2018-06-28 19:14 UTC (permalink / raw)
To: Matthias Gerstner, Mikhail Kurinnoi; +Cc: linux-integrity
On Thu, 2018-06-28 at 18:39 +0200, Matthias Gerstner wrote:
> Hi,
>
> > In this patch, I propose add an exception for "crypto-pkcs1pad(rsa,*)"
> > module requests only in case of enabled integrity asymmetric keys support.
>
> I have tested the patch in my test setup and it looks good. No deadlocks
> so far.
I really wish we didn't have to do a string compare "crypto-
pkcs1pad(rsa" each and every time. Is the check once per crypto
algorithm?
Mimi
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] integrity: prevent deadlock during digsig verification.
2018-06-28 19:14 ` Mimi Zohar
@ 2018-06-28 20:50 ` Mikhail Kurinnoi
2018-06-28 21:27 ` Mimi Zohar
0 siblings, 1 reply; 6+ messages in thread
From: Mikhail Kurinnoi @ 2018-06-28 20:50 UTC (permalink / raw)
To: Mimi Zohar; +Cc: Matthias Gerstner, linux-integrity
? Thu, 28 Jun 2018 15:14:38 -0400
Mimi Zohar <zohar@linux.vnet.ibm.com> ?????:
> On Thu, 2018-06-28 at 18:39 +0200, Matthias Gerstner wrote:
> > Hi,
> >
> > > In this patch, I propose add an exception for
> > > "crypto-pkcs1pad(rsa,*)" module requests only in case of enabled
> > > integrity asymmetric keys support.
> >
> > I have tested the patch in my test setup and it looks good. No
> > deadlocks so far.
>
> I really wish we didn't have to do a string compare "crypto-
> pkcs1pad(rsa" each and every time. Is the check once per crypto
> algorithm?
As I understood, it check once per crypto algorithm:
"crypto_alloc_tfm() will first attempt to locate an already loaded
algorithm.
...
If that fails it will send a query to any loaded crypto manager to
construct an algorithm on the fly.
A refcount is grabbed on the algorithm which is then associated with
the new transform."
https://github.com/torvalds/linux/blob/a97d8efd9d350bd9c6cf13689c7cc09049b42acd/crypto/api.c#L515
--
Best regards,
Mikhail Kurinnoi
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] integrity: prevent deadlock during digsig verification.
2018-06-28 20:50 ` Mikhail Kurinnoi
@ 2018-06-28 21:27 ` Mimi Zohar
0 siblings, 0 replies; 6+ messages in thread
From: Mimi Zohar @ 2018-06-28 21:27 UTC (permalink / raw)
To: Mikhail Kurinnoi; +Cc: Matthias Gerstner, linux-integrity
On Thu, 2018-06-28 at 23:50 +0300, Mikhail Kurinnoi wrote:
> ? Thu, 28 Jun 2018 15:14:38 -0400
> Mimi Zohar <zohar@linux.vnet.ibm.com> ?????:
>
> > On Thu, 2018-06-28 at 18:39 +0200, Matthias Gerstner wrote:
> > > Hi,
> > >
> > > > In this patch, I propose add an exception for
> > > > "crypto-pkcs1pad(rsa,*)" module requests only in case of enabled
> > > > integrity asymmetric keys support.
> > >
> > > I have tested the patch in my test setup and it looks good. No
> > > deadlocks so far.
> >
> > I really wish we didn't have to do a string compare "crypto-
> > pkcs1pad(rsa" each and every time. Is the check once per crypto
> > algorithm?
>
> As I understood, it check once per crypto algorithm:
>
> "crypto_alloc_tfm() will first attempt to locate an already loaded
> algorithm.
> ...
> If that fails it will send a query to any loaded crypto manager to
> construct an algorithm on the fly.
> A refcount is grabbed on the algorithm which is then associated with
> the new transform."
>
> https://github.com/torvalds/linux/blob/a97d8efd9d350bd9c6cf13689c7cc09049b42acd/crypto/api.c#L515
After having loaded "all" the crypto algorithms, we wouldn't need to
ever do the string compare again. As this isn't on a critical path,
nor is it likely for all crypto algorithms to be loaded, it probably
doesn't make sense to address it.
Mimi
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-06-28 21:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-27 13:33 [PATCH] integrity: prevent deadlock during digsig verification Mikhail Kurinnoi
2018-06-28 16:39 ` Matthias Gerstner
2018-06-28 19:14 ` Mimi Zohar
2018-06-28 20:50 ` Mikhail Kurinnoi
2018-06-28 21:27 ` Mimi Zohar
2018-06-28 18:43 ` Mimi Zohar
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.