| Message ID | 20180627163342.3e1d6333@totoro (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. Regards Matthias
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) >
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
В 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
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
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)
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(-)