| Message ID | 20180111120157.23qceywzi6omvvkb@dwarf.suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
I think that your code isn't quite right. Looking at the patched code:
#ifdef CONFIG_KEXEC_SIG
sig_err = arch_kexec_kernel_verify_sig(image, image->kernel_buf,
image->kernel_buf_len);
if (sig_err)
pr_debug("kernel signature verification failed.\n");
else
pr_debug("kernel signature verification successful.\n");
#endif
if (sig_err && IS_ENABLED(CONFIG_KEXEC_SIG_FORCE)) {
ret = sig_err;
goto out;
}
If the signature check fails because the signature is bad, but
CONFIG_KEXEC_SIG_FORCE=n then it now won't fail when it should.
If sig_err is -EKEYREJECTED, -EKEYEXPIRED or -EKEYREVOKED then it must fail,
even if the signature check isn't forced.
David
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 16, 2018 at 04:31:51PM +0000, David Howells wrote: > I think that your code isn't quite right. Looking at the patched code: > > #ifdef CONFIG_KEXEC_SIG > sig_err = arch_kexec_kernel_verify_sig(image, image->kernel_buf, > image->kernel_buf_len); > if (sig_err) > pr_debug("kernel signature verification failed.\n"); > else > pr_debug("kernel signature verification successful.\n"); > #endif > > if (sig_err && IS_ENABLED(CONFIG_KEXEC_SIG_FORCE)) { > ret = sig_err; > goto out; > } > > If the signature check fails because the signature is bad, but > CONFIG_KEXEC_SIG_FORCE=n then it now won't fail when it should. > > If sig_err is -EKEYREJECTED, -EKEYEXPIRED or -EKEYREVOKED then it must fail, > even if the signature check isn't forced. It wasn't my intention to fail in these cases. What additional security does this bring? If simply stripping an invalid signature from the image before loading will make it pass, why should the image with an invalid signature be rejected? Indeed, the module signing code, the semantics of which I wanted to mimic, also won't load modules with invalid signatures. It will load modules without any signatuire, it will load modules with the MODULE_SIG_STRING modified and it will load modules with either of MODULE_INIT_IGNORE_MODVERSIONS or MODULE_INIT_IGNORE_VERMAGIC passed as flags to the finit_module syscall. In all these cases, it will taint the kernel, which might be something we want for kexec_file_load as well (?). But I don't see why it distinguishes between ENOKEY and other errors when it's so easy for the caller to strip the invalid signature. And why kexec_file_load should do the same. What am I missing? Thanks,
Jiri Bohac <jbohac@suse.cz> wrote: > > If sig_err is -EKEYREJECTED, -EKEYEXPIRED or -EKEYREVOKED then it must fail, > > even if the signature check isn't forced. > > It wasn't my intention to fail in these cases. What additional > security does this bring? If simply stripping an invalid > signature from the image before loading will make it pass, why > should the image with an invalid signature be rejected? If there is a signature, then if we're checking signatures, in my opinion we should check it - and fail if the signature can't be parsed, is revoked, we have a key and the signature doesn't match - or even if we run out of memory. The cases for which enforcement is required are when (a) there is no signature, (b) we don't support the algorithms used, or (c) we don't have a key. If we're going to completely discard the result, why do your patches even bother to check the signature at all? David -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 17, 2018 at 04:34:24PM +0000, David Howells wrote: > Jiri Bohac <jbohac@suse.cz> wrote: > > > > If sig_err is -EKEYREJECTED, -EKEYEXPIRED or -EKEYREVOKED then it must fail, > > > even if the signature check isn't forced. > > > > It wasn't my intention to fail in these cases. What additional > > security does this bring? If simply stripping an invalid > > signature from the image before loading will make it pass, why > > should the image with an invalid signature be rejected? > > If there is a signature, then if we're checking signatures, in my opinion we > should check it - and fail if the signature can't be parsed, is revoked, we > have a key and the signature doesn't match - or even if we run out of memory. Key verification may and will fail for lots of reasons which is just going to make a user's life harder. E.g. you want to kexec an old kernel with an expired key. Or your date is just wrong and you get -EKEYEXPIRED. And you don't care about the signing at all; it's just compiled in because your distro also needs to work with secureboot. As a user, you will have to debug what's wrong for no good reason. And an actual attacker will just strip the signature off the image and load it. This makes no sense. > The cases for which enforcement is required are when (a) there is no > signature, (b) we don't support the algorithms used, or (c) we don't have a > key. > > If we're going to completely discard the result, why do your patches even > bother to check the signature at all? I thought that the debug message might be useful. E.g. when you're testing a kernel and you see "kernel signature verification failed" in dmesg then you know this would fail on a system with secure boot. But if ignoring the return code seems like too bad a thing, I would rather skip the signature checking if it's not going to be enforced with lockdown or CONFIG_KEXEC_SIG_FORCE. Also, only now I found that some of the error codes the crypto code returns yield really confusing messages (e.g. kexec_file_load of an unsigned kernel returns -ELIBBAD which makes kexec exit with "kexec_file_load failed: Accessing a corrupted shared library"). Maybe the error code could be unified to -EKEYREJECTED for all sorts of key verification failures? Thanks,
Jiri Bohac <jbohac@suse.cz> wrote: > Key verification may and will fail for lots of reasons which is > just going to make a user's life harder. E.g. you want to kexec > an old kernel with an expired key. Or your date is just wrong and > you get -EKEYEXPIRED. Note that we can't check for expired keys as we can't trust the system clock to be correct at this point. > Also, only now I found that some of the error codes the crypto > code returns yield really confusing messages (e.g. > kexec_file_load of an unsigned kernel returns -ELIBBAD which > makes kexec exit with "kexec_file_load failed: Accessing a > corrupted shared library"). Yeah, that should be fixed. > Maybe the error code could be unified to -EKEYREJECTED for all > sorts of key verification failures? Things like ENOMEM and EINTR definitely need to stay separate (not that I allow interruption at the moment). ENOKEY (couldn't find matching key), EINVAL (didn't recognise identifier), ENOPKG (couldn't find a crypto algo) and EBADMSG (couldn't parse signature) are arguable. I think there's a valid case for treating ENOKEY, EINVAL and ENOPKG differently to EKEYREJECTED - more so for ENOKEY. In my opinion, ENOKEY, EINVAL and ENOPKG are not fatal errors if we're not enforcing signature checking, but EKEYREJECTED and EBADMSG are. David -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 8eed3f94bfc7..f25facb0df96 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -1951,20 +1951,28 @@ config KEXEC_FILE for kernel and initramfs as opposed to list of segments as accepted by previous system call. -config KEXEC_VERIFY_SIG +config KEXEC_SIG bool "Verify kernel signature during kexec_file_load() syscall" depends on KEXEC_FILE ---help--- - This option makes kernel signature verification mandatory for - the kexec_file_load() syscall. + This option makes the kexec_file_load() syscall check for a valid + signature of the kernel image. The image can still be loaded without + a valid signature unless you also enable KEXEC_SIG_FORCE. - In addition to that option, you need to enable signature + In addition to this option, you need to enable signature verification for the corresponding kernel image type being loaded in order for this to work. +config KEXEC_SIG_FORCE + bool "Require a valid signature in kexec_file_load() syscall" + depends on KEXEC_SIG + ---help--- + This option makes kernel signature verification mandatory for + the kexec_file_load() syscall. + config KEXEC_BZIMAGE_VERIFY_SIG bool "Enable bzImage signature verification support" - depends on KEXEC_VERIFY_SIG + depends on KEXEC_SIG depends on SIGNED_PE_FILE_VERIFICATION select SYSTEM_TRUSTED_KEYRING ---help--- diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c index 1f790cf9d38f..3fbe35b923ef 100644 --- a/arch/x86/kernel/machine_kexec_64.c +++ b/arch/x86/kernel/machine_kexec_64.c @@ -406,7 +406,7 @@ int arch_kimage_file_post_load_cleanup(struct kimage *image) return image->fops->cleanup(image->image_loader_data); } -#ifdef CONFIG_KEXEC_VERIFY_SIG +#ifdef CONFIG_KEXEC_SIG int arch_kexec_kernel_verify_sig(struct kimage *image, void *kernel, unsigned long kernel_len) { diff --git a/include/linux/kexec.h b/include/linux/kexec.h index f16f6ceb3875..19652372f3ee 100644 --- a/include/linux/kexec.h +++ b/include/linux/kexec.h @@ -121,7 +121,7 @@ typedef void *(kexec_load_t)(struct kimage *image, char *kernel_buf, unsigned long cmdline_len); typedef int (kexec_cleanup_t)(void *loader_data); -#ifdef CONFIG_KEXEC_VERIFY_SIG +#ifdef CONFIG_KEXEC_SIG typedef int (kexec_verify_sig_t)(const char *kernel_buf, unsigned long kernel_len); #endif @@ -130,7 +130,7 @@ struct kexec_file_ops { kexec_probe_t *probe; kexec_load_t *load; kexec_cleanup_t *cleanup; -#ifdef CONFIG_KEXEC_VERIFY_SIG +#ifdef CONFIG_KEXEC_SIG kexec_verify_sig_t *verify_sig; #endif }; diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c --- a/kernel/kexec_file.c +++ b/kernel/kexec_file.c @@ -45,7 +45,7 @@ int __weak arch_kimage_file_post_load_cleanup(struct kimage *image) return -EINVAL; } -#ifdef CONFIG_KEXEC_VERIFY_SIG +#ifdef CONFIG_KEXEC_SIG int __weak arch_kexec_kernel_verify_sig(struct kimage *image, void *buf, unsigned long buf_len) { @@ -116,7 +116,7 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd, const char __user *cmdline_ptr, unsigned long cmdline_len, unsigned flags) { - int ret = 0; + int ret = 0, sig_err = -EPERM; void *ldata; loff_t size; @@ -135,15 +135,20 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd, if (ret) goto out; -#ifdef CONFIG_KEXEC_VERIFY_SIG - ret = arch_kexec_kernel_verify_sig(image, image->kernel_buf, +#ifdef CONFIG_KEXEC_SIG + sig_err = arch_kexec_kernel_verify_sig(image, image->kernel_buf, image->kernel_buf_len); - if (ret) { + if (sig_err) pr_debug("kernel signature verification failed.\n"); + else + pr_debug("kernel signature verification successful.\n"); +#endif + + if (sig_err && IS_ENABLED(CONFIG_KEXEC_SIG_FORCE)) { + ret = sig_err; goto out; } - pr_debug("kernel signature verification successful.\n"); -#endif + /* It is possible that there no initramfs is being loaded */ if (!(flags & KEXEC_FILE_NO_INITRAMFS)) { ret = kernel_read_file_from_fd(initrd_fd, &image->initrd_buf,
This is a preparatory patch for kexec lockdown. A locked down kernel needs to prevent unsigned kernel images to be loaded with kexec_file_load. Currently, the only way to force the signature verification is compiling with KEXEC_VERIFY_SIG. This prevents loading usigned images even when the kernel is not locked down at runtime. This patch spilts KEXEC_VERIFY_SIG into KEXEC_SIG and KEXEC_SIG_FORCE. Analogous to the MODULE_SIG and MODULE_SIG_FORCE for modules, KEXEC_SIG turns on the signature verification but allows unsigned images to be loaded. KEXEC_SIG_FORCE disallows images without a valid signature. Signed-off-by: Jiri Bohac <jbohac@suse.cz>