| 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
On Wed, Jun 27, 2018 at 04:33:42PM +0300, Mikhail Kurinnoi wrote: > +int integrity_kernel_module_request(char *kmod_name) > +{ > + if (strncmp(kmod_name, "crypto-pkcs1pad(rsa,", 20) == 0) > + return -EINVAL; > + > + return 0; > +} Just a heads-up: The above was applied as commit 6eb864c1d9dd ("integrity: prevent deadlock during digsig verification.") in 2018. In 2021 the kernel gained support for ECDSA signatures, in 2019 for ECRDSA (GOST) signatures. If kernel modules are signed with ECDSA or ECRDSA, you may once again see the deadlock addressed by the above-quoted patch and you may need additional string comparisons to avoid it. There's a (stalled) effort to support RSA PSS, this may likewise require yet another string comparison: https://lore.kernel.org/all/20210420114124.9684-1-varad.gautam@suse.com/ You may have to come up with a solution that doesn't require adding new string comparisons as this isn't scalable and it's easy to forget amending the string comparisons when new algorithms or encodings are added to the crypto subsystem. Thanks, Lukas
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(-)