* [PATCH 02/13] fs/kernel_read_file: Remove FIRMWARE_PREALLOC_BUFFER enum
From: Kees Cook @ 2020-07-17 17:42 UTC (permalink / raw)
To: Scott Branden
Cc: Kees Cook, stable, Mimi Zohar, Matthew Wilcox, James Morris,
Luis Chamberlain, Greg Kroah-Hartman, Rafael J. Wysocki,
Alexander Viro, Jessica Yu, Dmitry Kasatkin, Serge E. Hallyn,
Casey Schaufler, Eric W. Biederman, Peter Zijlstra,
Matthew Garrett, David Howells, Mauro Carvalho Chehab,
Randy Dunlap, Joel Fernandes (Google), KP Singh, Dave Olsthoorn,
Hans de Goede, Peter Jones, Andrew Morton, Stephen Boyd,
Paul Moore, Stephen Smalley, linux-security-module,
linux-integrity, selinux, linux-fsdevel, kexec, linux-kernel
In-Reply-To: <20200717174309.1164575-1-keescook@chromium.org>
FIRMWARE_PREALLOC_BUFFER is a "how", not a "what", and confuses the LSMs
that are interested in filtering between types of things. The "how"
should be an internal detail made uninteresting to the LSMs.
Fixes: a098ecd2fa7d ("firmware: support loading into a pre-allocated buffer")
Fixes: fd90bc559bfb ("ima: based on policy verify firmware signatures (pre-allocated buffer)")
Fixes: 4f0496d8ffa3 ("ima: based on policy warn about loading firmware (pre-allocated buffer)")
Cc: stable@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
To aid in backporting, this change is made before moving
kernel_read_file() to separate header/source files.
---
drivers/base/firmware_loader/main.c | 5 ++---
fs/exec.c | 7 ++++---
include/linux/fs.h | 2 +-
kernel/module.c | 2 +-
security/integrity/digsig.c | 2 +-
security/integrity/ima/ima_fs.c | 2 +-
security/integrity/ima/ima_main.c | 6 ++----
7 files changed, 12 insertions(+), 14 deletions(-)
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index ca871b13524e..c2f57cedcd6f 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -465,14 +465,12 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
int i, len;
int rc = -ENOENT;
char *path;
- enum kernel_read_file_id id = READING_FIRMWARE;
size_t msize = INT_MAX;
void *buffer = NULL;
/* Already populated data member means we're loading into a buffer */
if (!decompress && fw_priv->data) {
buffer = fw_priv->data;
- id = READING_FIRMWARE_PREALLOC_BUFFER;
msize = fw_priv->allocated_size;
}
@@ -496,7 +494,8 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
/* load firmware files from the mount namespace of init */
rc = kernel_read_file_from_path_initns(path, &buffer,
- &size, msize, id);
+ &size, msize,
+ READING_FIRMWARE);
if (rc) {
if (rc != -ENOENT)
dev_warn(device, "loading %s failed with error %d\n",
diff --git a/fs/exec.c b/fs/exec.c
index e6e8a9a70327..2bf549757ce7 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -927,6 +927,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
{
loff_t i_size, pos;
ssize_t bytes = 0;
+ void *allocated = NULL;
int ret;
if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
@@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
goto out;
}
- if (id != READING_FIRMWARE_PREALLOC_BUFFER)
- *buf = vmalloc(i_size);
+ if (!*buf)
+ *buf = allocated = vmalloc(i_size);
if (!*buf) {
ret = -ENOMEM;
goto out;
@@ -980,7 +981,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
out_free:
if (ret < 0) {
- if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
+ if (allocated) {
vfree(*buf);
*buf = NULL;
}
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3f881a892ea7..95fc775ed937 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2993,10 +2993,10 @@ static inline void i_readcount_inc(struct inode *inode)
#endif
extern int do_pipe_flags(int *, int);
+/* This is a list of *what* is being read, not *how*. */
#define __kernel_read_file_id(id) \
id(UNKNOWN, unknown) \
id(FIRMWARE, firmware) \
- id(FIRMWARE_PREALLOC_BUFFER, firmware) \
id(FIRMWARE_EFI_EMBEDDED, firmware) \
id(MODULE, kernel-module) \
id(KEXEC_IMAGE, kexec-image) \
diff --git a/kernel/module.c b/kernel/module.c
index 0c6573b98c36..26105148f4d2 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3988,7 +3988,7 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags)
{
struct load_info info = { };
loff_t size;
- void *hdr;
+ void *hdr = NULL;
int err;
err = may_init_module();
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index e9cbadade74b..ac02b7632353 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -169,7 +169,7 @@ int __init integrity_add_key(const unsigned int id, const void *data,
int __init integrity_load_x509(const unsigned int id, const char *path)
{
- void *data;
+ void *data = NULL;
loff_t size;
int rc;
key_perm_t perm;
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index e3fcad871861..15a44c5022f7 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -272,7 +272,7 @@ static const struct file_operations ima_ascii_measurements_ops = {
static ssize_t ima_read_policy(char *path)
{
- void *data;
+ void *data = NULL;
char *datap;
loff_t size;
int rc, pathlen = strlen(path);
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index c1583d98c5e5..f80ee4ce4669 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -611,19 +611,17 @@ void ima_post_path_mknod(struct dentry *dentry)
int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
{
/*
- * READING_FIRMWARE_PREALLOC_BUFFER
- *
* Do devices using pre-allocated memory run the risk of the
* firmware being accessible to the device prior to the completion
* of IMA's signature verification any more than when using two
- * buffers?
+ * buffers? It may be desirable to include the buffer address
+ * in this API and walk all the dma_map_single() mappings to check.
*/
return 0;
}
const int read_idmap[READING_MAX_ID] = {
[READING_FIRMWARE] = FIRMWARE_CHECK,
- [READING_FIRMWARE_PREALLOC_BUFFER] = FIRMWARE_CHECK,
[READING_MODULE] = MODULE_CHECK,
[READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK,
[READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK,
--
2.25.1
^ permalink raw reply related
* [PATCH 03/13] fs/kernel_read_file: Remove FIRMWARE_EFI_EMBEDDED enum
From: Kees Cook @ 2020-07-17 17:42 UTC (permalink / raw)
To: Scott Branden
Cc: Kees Cook, stable, Mimi Zohar, Matthew Wilcox, James Morris,
Luis Chamberlain, Greg Kroah-Hartman, Rafael J. Wysocki,
Alexander Viro, Jessica Yu, Dmitry Kasatkin, Serge E. Hallyn,
Casey Schaufler, Eric W. Biederman, Peter Zijlstra,
Matthew Garrett, David Howells, Mauro Carvalho Chehab,
Randy Dunlap, Joel Fernandes (Google), KP Singh, Dave Olsthoorn,
Hans de Goede, Peter Jones, Andrew Morton, Stephen Boyd,
Paul Moore, Stephen Smalley, linux-security-module,
linux-integrity, selinux, linux-fsdevel, kexec, linux-kernel
In-Reply-To: <20200717174309.1164575-1-keescook@chromium.org>
The "FIRMWARE_EFI_EMBEDDED" enum is a "where", not a "what". It
should not be distinguished separately from just "FIRMWARE", as this
confuses the LSMs about what is being loaded. Additionally, there was
no actual validation of the firmware contents happening.
Fixes: e4c2c0ff00ec ("firmware: Add new platform fallback mechanism and firmware_request_platform()")
Cc: stable@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
To aid in backporting, this change is made before moving
kernel_read_file() to separate header/source files.
---
drivers/base/firmware_loader/fallback_platform.c | 2 +-
include/linux/fs.h | 3 +--
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/base/firmware_loader/fallback_platform.c b/drivers/base/firmware_loader/fallback_platform.c
index 685edb7dd05a..6958ab1a8059 100644
--- a/drivers/base/firmware_loader/fallback_platform.c
+++ b/drivers/base/firmware_loader/fallback_platform.c
@@ -17,7 +17,7 @@ int firmware_fallback_platform(struct fw_priv *fw_priv, u32 opt_flags)
if (!(opt_flags & FW_OPT_FALLBACK_PLATFORM))
return -ENOENT;
- rc = security_kernel_load_data(LOADING_FIRMWARE_EFI_EMBEDDED);
+ rc = security_kernel_load_data(LOADING_FIRMWARE);
if (rc)
return rc;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 95fc775ed937..f50a35d54a61 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2993,11 +2993,10 @@ static inline void i_readcount_inc(struct inode *inode)
#endif
extern int do_pipe_flags(int *, int);
-/* This is a list of *what* is being read, not *how*. */
+/* This is a list of *what* is being read, not *how* nor *where*. */
#define __kernel_read_file_id(id) \
id(UNKNOWN, unknown) \
id(FIRMWARE, firmware) \
- id(FIRMWARE_EFI_EMBEDDED, firmware) \
id(MODULE, kernel-module) \
id(KEXEC_IMAGE, kexec-image) \
id(KEXEC_INITRAMFS, kexec-initramfs) \
--
2.25.1
^ permalink raw reply related
* Re: [PATCH v3 08/12] ima: Shallow copy the args_p member of ima_rule_entry.lsm elements
From: Tyler Hicks @ 2020-07-17 16:51 UTC (permalink / raw)
To: Konsta Karsisto, Mimi Zohar
Cc: linux-integrity, Dmitry Kasatkin, James Morris, Serge E . Hallyn,
Lakshmi Ramasubramanian, Prakhar Srivastava, linux-kernel,
linux-security-module
In-Reply-To: <CAAEqDhD-wCGY7ykjSsNgCri4ykWPi9cP3j1zoQPWddB4r92Kqw@mail.gmail.com>
On 2020-07-17 18:35:03, Konsta Karsisto wrote:
> Hi,
>
> Found one glitch with this change, see below:
>
> On Thu, Jul 9, 2020 at 9:22 AM Tyler Hicks <tyhicks@linux.microsoft.com> wrote:
> >
> > The args_p member is a simple string that is allocated by
> > ima_rule_init(). Shallow copy it like other non-LSM references in
> > ima_rule_entry structs.
> >
> > There are no longer any necessary error path cleanups to do in
> > ima_lsm_copy_rule().
> >
> > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> > ---
> >
> > * v3
> > - No change
> > * v2
> > - Adjusted context to account for ima_lsm_copy_rule() directly calling
> > ima_lsm_free_rule() and the lack of explicit reference ownership
> > transfers
> > - Added comment to ima_lsm_copy_rule() to document the args_p
> > reference ownership transfer
> >
> > security/integrity/ima/ima_policy.c | 16 +++++++---------
> > 1 file changed, 7 insertions(+), 9 deletions(-)
> >
> > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > index 9842e2e0bc6d..b02e1ffd10c9 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -300,10 +300,13 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
> > continue;
> >
> > nentry->lsm[i].type = entry->lsm[i].type;
> > - nentry->lsm[i].args_p = kstrdup(entry->lsm[i].args_p,
> > - GFP_KERNEL);
> > - if (!nentry->lsm[i].args_p)
> > - goto out_err;
> > + nentry->lsm[i].args_p = entry->lsm[i].args_p;
> > + /*
> > + * Remove the reference from entry so that the associated
> > + * memory will not be freed during a later call to
> > + * ima_lsm_free_rule(entry).
> > + */
> > + entry->lsm[i].args_p = NULL;
>
> This assignment necessitates a change in the code below...
>
> > security_filter_rule_init(nentry->lsm[i].type,
> > Audit_equal,
> > @@ -314,11 +317,6 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
> > (char *)entry->lsm[i].args_p);
>
> ... you should refer to nentry->lsm[i].args_p here!
>
> Other than that,
>
> Reviewed-by: Konsta Karsisto <konsta.karsisto@gmail.com>
Thank you, Konsta. You're exactly right about the required change.
Without it, the pr_warn() in the error path will always attempt to print
a NULL pointer.
Mimi, the following change (along with adding Konsta's Reviewed-by)
needs to be made to this patch in next-integrity-testing:
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index b02e1ffd10c9..330a4e216349 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -314,7 +314,7 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
&nentry->lsm[i].rule);
if (!nentry->lsm[i].rule)
pr_warn("rule for LSM \'%s\' is undefined\n",
- (char *)entry->lsm[i].args_p);
+ (char *)nentry->lsm[i].args_p);
}
return nentry;
}
It will then cause the next patch in the series to not apply but the
fixup is minor.
Let me know if you are alright with doing these changes yourself or if
you'd like me to submit a new series revision.
If you do this rebase work in next-integrity-testing yourself, I've
prepared a copy branch of next-integrity-testing with all of the
necessary changes for you to compare against:
https://git.kernel.org/pub/scm/linux/kernel/git/tyhicks/linux.git/log/?h=next-integrity-testing-fixup
My apologies for the trouble.
Tyler
>
>
> Konsta
>
> > }
> > return nentry;
> > -
> > -out_err:
> > - ima_lsm_free_rule(nentry);
> > - kfree(nentry);
> > - return NULL;
> > }
> >
> > static int ima_lsm_update_rule(struct ima_rule_entry *entry)
> > --
> > 2.25.1
> >
^ permalink raw reply related
* Re: [PATCH v3 08/12] ima: Shallow copy the args_p member of ima_rule_entry.lsm elements
From: Konsta Karsisto @ 2020-07-17 15:35 UTC (permalink / raw)
To: Tyler Hicks, linux-integrity
Cc: Mimi Zohar, Dmitry Kasatkin, James Morris, Serge E . Hallyn,
Lakshmi Ramasubramanian, Prakhar Srivastava, linux-kernel,
linux-security-module
In-Reply-To: <20200709061911.954326-9-tyhicks@linux.microsoft.com>
Hi,
Found one glitch with this change, see below:
On Thu, Jul 9, 2020 at 9:22 AM Tyler Hicks <tyhicks@linux.microsoft.com> wrote:
>
> The args_p member is a simple string that is allocated by
> ima_rule_init(). Shallow copy it like other non-LSM references in
> ima_rule_entry structs.
>
> There are no longer any necessary error path cleanups to do in
> ima_lsm_copy_rule().
>
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> ---
>
> * v3
> - No change
> * v2
> - Adjusted context to account for ima_lsm_copy_rule() directly calling
> ima_lsm_free_rule() and the lack of explicit reference ownership
> transfers
> - Added comment to ima_lsm_copy_rule() to document the args_p
> reference ownership transfer
>
> security/integrity/ima/ima_policy.c | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 9842e2e0bc6d..b02e1ffd10c9 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -300,10 +300,13 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
> continue;
>
> nentry->lsm[i].type = entry->lsm[i].type;
> - nentry->lsm[i].args_p = kstrdup(entry->lsm[i].args_p,
> - GFP_KERNEL);
> - if (!nentry->lsm[i].args_p)
> - goto out_err;
> + nentry->lsm[i].args_p = entry->lsm[i].args_p;
> + /*
> + * Remove the reference from entry so that the associated
> + * memory will not be freed during a later call to
> + * ima_lsm_free_rule(entry).
> + */
> + entry->lsm[i].args_p = NULL;
This assignment necessitates a change in the code below...
> security_filter_rule_init(nentry->lsm[i].type,
> Audit_equal,
> @@ -314,11 +317,6 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
> (char *)entry->lsm[i].args_p);
... you should refer to nentry->lsm[i].args_p here!
Other than that,
Reviewed-by: Konsta Karsisto <konsta.karsisto@gmail.com>
Konsta
> }
> return nentry;
> -
> -out_err:
> - ima_lsm_free_rule(nentry);
> - kfree(nentry);
> - return NULL;
> }
>
> static int ima_lsm_update_rule(struct ima_rule_entry *entry)
> --
> 2.25.1
>
^ permalink raw reply
* Reporting a use-after-free read bug in userfaultfd_release()
From: Peilin Ye @ 2020-07-17 11:45 UTC (permalink / raw)
To: Daniel Colascione
Cc: timmurray, selinux, linux-security-module, linux-fsdevel,
linux-kernel, kvm, viro, paul, nnk, sds, lokeshgidra, jmorris
Hi all,
Syzbot reported the following use-after-free bug in
userfaultfd_release():
https://syzkaller.appspot.com/bug?id=4b9e5aea757b678d9939c364e50212354a3480a6
It seems to be caused by this patch. I took a look at the stack trace.
In the patch:
fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
if (fd < 0) {
fput(file);
goto out;
}
If get_unused_fd_flags() fails, `ctx` is freed. Later however, before
returning back to userland, userfaultfd_release() is called and tries to
use `ctx` again, causing a use-after-free bug.
The syzbot reproducer does a setrlimit() then a userfaultfd(). The
former sets a hard limit on number of open files to zero, which causes
get_unused_fd_flags() to fail.
Thank you,
Peilin Ye
^ permalink raw reply
* Re: [PATCH 16/16] capsh.c: Spelling fixes in usage() message
From: Michael Kerrisk (man-pages) @ 2020-07-17 6:00 UTC (permalink / raw)
To: Andrew G. Morgan; +Cc: LSM List
In-Reply-To: <CALQRfL6dAEgiUiEckUN9x_g0J+sywz+Q_zBfPqPFTBsf2zRt=A@mail.gmail.com>
On Thu, 16 Jul 2020 at 17:08, Andrew G. Morgan <morgan@kernel.org> wrote:
>
> Thanks! Applied all of them except 07_16. Instead, I've hopefully
> clarified the intent of the text with some quotes.
Okay. Thanks, Andrew.
Cheers,
Michael
> https://git.kernel.org/pub/scm/libs/libcap/libcap.git/commit/?id=34e4e00b983a2c0fc5f13b403871a8fb5860bb89
>
> On Thu, Jul 16, 2020 at 3:19 AM Michael Kerrisk (man-pages)
> <mtk.manpages@gmail.com> wrote:
> >
> > Signed-off-by: Michael Kerrisk (man-pages) <mtk.manpages@gmail.com>
> > ---
> > progs/capsh.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/progs/capsh.c b/progs/capsh.c
> > index 94bf57d..7bed98e 100644
> > --- a/progs/capsh.c
> > +++ b/progs/capsh.c
> > @@ -879,10 +879,10 @@ int main(int argc, char *argv[], char *envp[])
> > " --delamb=xxx remove xxx,... capabilities from ambient\n"
> > " --noamb reset (drop) all ambient capabilities\n"
> > " --caps=xxx set caps as per cap_from_text()\n"
> > - " --inh=xxx set xxx,.. inheritiable set\n"
> > + " --inh=xxx set xxx,.. inheritable set\n"
> > " --secbits=<n> write a new value for securebits\n"
> > " --iab=... use cap_iab_from_text() to set iab\n"
> > - " --keep=<n> set keep-capabability bit to <n>\n"
> > + " --keep=<n> set keep-capability bit to <n>\n"
> > " --uid=<n> set uid to <n> (hint: id <username>)\n"
> > " --cap-uid=<n> libcap cap_setuid() to change uid\n"
> > " --is-uid=<n> exit 1 if uid != <n>\n"
> > --
> > 2.26.2
> >
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
^ permalink raw reply
* Re: [PATCH v3 00/12] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support
From: Tyler Hicks @ 2020-07-17 4:34 UTC (permalink / raw)
To: Mimi Zohar
Cc: Dmitry Kasatkin, James Morris, Serge E . Hallyn,
Lakshmi Ramasubramanian, Prakhar Srivastava, linux-kernel,
linux-integrity, linux-security-module, Janne Karhunen,
Eric Biederman, kexec, Casey Schaufler, Nayna Jain
In-Reply-To: <1594960293.27397.2.camel@linux.ibm.com>
On 2020-07-17 00:31:33, Mimi Zohar wrote:
> On Thu, 2020-07-09 at 01:18 -0500, Tyler Hicks wrote:
> > This series ultimately extends the supported IMA rule conditionals for
> > the KEXEC_CMDLINE hook function. As of today, there's an imbalance in
> > IMA language conditional support for KEXEC_CMDLINE rules in comparison
> > to KEXEC_KERNEL_CHECK and KEXEC_INITRAMFS_CHECK rules. The KEXEC_CMDLINE
> > rules do not support *any* conditionals so you cannot have a sequence of
> > rules like this:
> >
> > dont_measure func=KEXEC_KERNEL_CHECK obj_type=foo_t
> > dont_measure func=KEXEC_INITRAMFS_CHECK obj_type=foo_t
> > dont_measure func=KEXEC_CMDLINE obj_type=foo_t
> > measure func=KEXEC_KERNEL_CHECK
> > measure func=KEXEC_INITRAMFS_CHECK
> > measure func=KEXEC_CMDLINE
> >
> > Instead, KEXEC_CMDLINE rules can only be measured or not measured and
> > there's no additional flexibility in today's implementation of the
> > KEXEC_CMDLINE hook function.
> >
> > With this series, the above sequence of rules becomes valid and any
> > calls to kexec_file_load() with a kernel and initramfs inode type of
> > foo_t will not be measured (that includes the kernel cmdline buffer)
> > while all other objects given to a kexec_file_load() syscall will be
> > measured. There's obviously not an inode directly associated with the
> > kernel cmdline buffer but this patch series ties the inode based
> > decision making for KEXEC_CMDLINE to the kernel's inode. I think this
> > will be intuitive to policy authors.
> >
> > While reading IMA code and preparing to make this change, I realized
> > that the buffer based hook functions (KEXEC_CMDLINE and KEY_CHECK) are
> > quite special in comparison to longer standing hook functions. These
> > buffer based hook functions can only support measure actions and there
> > are some restrictions on the conditionals that they support. However,
> > the rule parser isn't enforcing any of those restrictions and IMA policy
> > authors wouldn't have any immediate way of knowing that the policy that
> > they wrote is invalid. For example, the sequence of rules above parses
> > successfully in today's kernel but the
> > "dont_measure func=KEXEC_CMDLINE ..." rule is incorrectly handled in
> > ima_match_rules(). The dont_measure rule is *always* considered to be a
> > match so, surprisingly, no KEXEC_CMDLINE measurements are made.
> >
> > While making the rule parser more strict, I realized that the parser
> > does not correctly free all of the allocated memory associated with an
> > ima_rule_entry when going down some error paths. Invalid policy loaded
> > by the policy administrator could result in small memory leaks.
> >
> > I envision patches 1-7 going to stable. The series is ordered in a way
> > that has all the fixes up front, followed by cleanups, followed by the
> > feature patch. The breakdown of patches looks like so:
> >
> > Memory leak fixes: 1-3
> > Parser strictness fixes: 4-7
> > Code cleanups made possible by the fixes: 8-11
> > Extend KEXEC_CMDLINE rule support: 12
>
> Thanks, Tyler. This is a really nice patch set. The patches are now
> in the "next-integrity-testing" branch.
Thank you for all the helpful review comments. You know where to find me
if any bugs pop up during testing. :)
Tyler
>
> Mimi
^ permalink raw reply
* Re: [PATCH v3 00/12] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support
From: Mimi Zohar @ 2020-07-17 4:31 UTC (permalink / raw)
To: Tyler Hicks, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module, Janne Karhunen, Eric Biederman, kexec,
Casey Schaufler, Nayna Jain
In-Reply-To: <20200709061911.954326-1-tyhicks@linux.microsoft.com>
On Thu, 2020-07-09 at 01:18 -0500, Tyler Hicks wrote:
> This series ultimately extends the supported IMA rule conditionals for
> the KEXEC_CMDLINE hook function. As of today, there's an imbalance in
> IMA language conditional support for KEXEC_CMDLINE rules in comparison
> to KEXEC_KERNEL_CHECK and KEXEC_INITRAMFS_CHECK rules. The KEXEC_CMDLINE
> rules do not support *any* conditionals so you cannot have a sequence of
> rules like this:
>
> dont_measure func=KEXEC_KERNEL_CHECK obj_type=foo_t
> dont_measure func=KEXEC_INITRAMFS_CHECK obj_type=foo_t
> dont_measure func=KEXEC_CMDLINE obj_type=foo_t
> measure func=KEXEC_KERNEL_CHECK
> measure func=KEXEC_INITRAMFS_CHECK
> measure func=KEXEC_CMDLINE
>
> Instead, KEXEC_CMDLINE rules can only be measured or not measured and
> there's no additional flexibility in today's implementation of the
> KEXEC_CMDLINE hook function.
>
> With this series, the above sequence of rules becomes valid and any
> calls to kexec_file_load() with a kernel and initramfs inode type of
> foo_t will not be measured (that includes the kernel cmdline buffer)
> while all other objects given to a kexec_file_load() syscall will be
> measured. There's obviously not an inode directly associated with the
> kernel cmdline buffer but this patch series ties the inode based
> decision making for KEXEC_CMDLINE to the kernel's inode. I think this
> will be intuitive to policy authors.
>
> While reading IMA code and preparing to make this change, I realized
> that the buffer based hook functions (KEXEC_CMDLINE and KEY_CHECK) are
> quite special in comparison to longer standing hook functions. These
> buffer based hook functions can only support measure actions and there
> are some restrictions on the conditionals that they support. However,
> the rule parser isn't enforcing any of those restrictions and IMA policy
> authors wouldn't have any immediate way of knowing that the policy that
> they wrote is invalid. For example, the sequence of rules above parses
> successfully in today's kernel but the
> "dont_measure func=KEXEC_CMDLINE ..." rule is incorrectly handled in
> ima_match_rules(). The dont_measure rule is *always* considered to be a
> match so, surprisingly, no KEXEC_CMDLINE measurements are made.
>
> While making the rule parser more strict, I realized that the parser
> does not correctly free all of the allocated memory associated with an
> ima_rule_entry when going down some error paths. Invalid policy loaded
> by the policy administrator could result in small memory leaks.
>
> I envision patches 1-7 going to stable. The series is ordered in a way
> that has all the fixes up front, followed by cleanups, followed by the
> feature patch. The breakdown of patches looks like so:
>
> Memory leak fixes: 1-3
> Parser strictness fixes: 4-7
> Code cleanups made possible by the fixes: 8-11
> Extend KEXEC_CMDLINE rule support: 12
Thanks, Tyler. This is a really nice patch set. The patches are now
in the "next-integrity-testing" branch.
Mimi
^ permalink raw reply
* Re: [PATCH v2 4/5] LSM: Define SELinux function to measure security state
From: Lakshmi Ramasubramanian @ 2020-07-16 22:03 UTC (permalink / raw)
To: Stephen Smalley
Cc: Mimi Zohar, Casey Schaufler, James Morris, linux-integrity,
SElinux list, LSM List, linux-kernel
In-Reply-To: <CAEjxPJ5p_T+C1NDz3iF7fvQzQAURpAcipvQfQXLZTfLP4Wiqbg@mail.gmail.com>
On 7/16/20 12:45 PM, Stephen Smalley wrote:
> On Thu, Jul 16, 2020 at 3:13 PM Lakshmi Ramasubramanian
> <nramas@linux.microsoft.com> wrote:
>>
>> On 7/16/20 11:54 AM, Stephen Smalley wrote:
>>> Not sure about this error handling approach (silent, proceeding as if
>>> the length was zero and then later failing with ENOMEM on every
>>> attempt?). I'd be more inclined to panic/BUG here but I know Linus
>>> doesn't like that.
>> I am not sure if failing (kernel panic/BUG) to "measure" LSM data under
>> memory pressure conditions is the right thing. But I am open to treating
>> this error as a fatal error. Please let me know.
>
> Let's at least log an error message since it otherwise silently
> disables all measuring of security state.
Agree - will log error messages as appropriate.
> Also not sure why we bother returning errors from
> selinux_measure_data() since nothing appears to check or use the
> result.
Maybe SELinux can log audit messages on failures, but I guess it may be
better to do that closer to where the error occurs.
Will change selinux_measure_data() to void function.
> Don't know if integrity/IMA has any equivalent to the audit
> subsystem's concept of audit_failure settings to control whether
> errors that prevent auditing (measuring) are handled silently, with a
> log message, or via a panic. If not, I guess that can be explored
> separately.
>
Yes - integrity subsystem logs audit messages for errors\failures.
-lakshmi
^ permalink raw reply
* Re: [PATCH 2/4] fs: Remove FIRMWARE_PREALLOC_BUFFER from kernel_read_file() enums
From: Kees Cook @ 2020-07-16 21:16 UTC (permalink / raw)
To: Scott Branden
Cc: Matthew Wilcox, James Morris, Luis Chamberlain, Mimi Zohar,
Greg Kroah-Hartman, Rafael J. Wysocki, Alexander Viro, Jessica Yu,
Dmitry Kasatkin, Serge E. Hallyn, Casey Schaufler,
Eric W. Biederman, Peter Zijlstra, Matthew Garrett, David Howells,
Mauro Carvalho Chehab, Randy Dunlap, Joel Fernandes (Google),
KP Singh, Dave Olsthoorn, Hans de Goede, Peter Jones,
Andrew Morton, Stephen Boyd, Paul Moore, linux-kernel,
linux-fsdevel, linux-integrity, linux-security-module
In-Reply-To: <9ba08503-e515-6761-63de-a3b611720b1b@broadcom.com>
On Thu, Jul 16, 2020 at 01:35:17PM -0700, Scott Branden wrote:
> On 2020-07-10 3:44 p.m., Kees Cook wrote:
> > On Fri, Jul 10, 2020 at 03:10:25PM -0700, Scott Branden wrote:
> > >
> > > On 2020-07-10 3:04 p.m., Matthew Wilcox wrote:
> > > > On Fri, Jul 10, 2020 at 02:00:32PM -0700, Scott Branden wrote:
> > > > > > @@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
> > > > > > goto out;
> > > > > > }
> > > > > > - if (id != READING_FIRMWARE_PREALLOC_BUFFER)
> > > > > > - *buf = vmalloc(i_size);
> > > > > > + if (!*buf)
> > > > > The assumption that *buf is always NULL when id !=
> > > > > READING_FIRMWARE_PREALLOC_BUFFER doesn't appear to be correct.
> > > > > I get unhandled page faults due to this change on boot.
> > > > Did it give you a stack backtrace?
> > > Yes, but there's no requirement that *buf need to be NULL when calling this
> > > function.
> > > To fix my particular crash I added the following locally:
> > >
> > > --- a/kernel/module.c
> > > +++ b/kernel/module.c
> > > @@ -3989,7 +3989,7 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char
> > > __user *, uargs, int, flags)
> > > {
> > > struct load_info info = { };
> > > loff_t size;
> > > - void *hdr;
> > > + void *hdr = NULL;
> > > int err;
> > >
> > > err = may_init_module();
> > Thanks for the diagnosis and fix! I haven't had time to cycle back
> > around to this series yet. Hopefully soon. :)
> >
> In order to assist in your patchset I have combined it with my patch series
> here:
> https://github.com/sbranden/linux/tree/kernel_read_file_for_kees
>
> Please let me know if this matches your expectations for my patches or if
> there is something else I need to change.
Thanks! I was working on the next revision of this last night, and I'm
trying to get through today's email to finish it. I'll take a look!
--
Kees Cook
^ permalink raw reply
* [RFC PATCH 4/5] keys: Split the search perms between KEY_NEED_USE and KEY_NEED_SEARCH
From: David Howells @ 2020-07-16 20:35 UTC (permalink / raw)
To: Stephen Smalley, Casey Schaufler
Cc: dhowells, Jarkko Sakkinen, Eric Biederman, jlayton, christian,
selinux, linux-afs, linux-nfs, linux-cifs, linux-api,
linux-fsdevel, linux-security-module, linux-kernel, containers
In-Reply-To: <159493167778.3249370.8145886688150701997.stgit@warthog.procyon.org.uk>
Allow the permission needed for a keyring search to be specified and split
the permissions between KEY_NEED_USE (the kernel wants to do something with
the key) and KEY_NEED_SEARCH (userspace wants to do something with the
key).
This primarily affects how request_key() works, differentiating implicit
calls (e.g. from filesystems) from userspace calling the request_key()
system call.
This will allow the kernel to find keys in a hidden container keyring, but
not the denizens of the container.
Signed-off-by: David Howells <dhowells@redhat.com>
---
certs/blacklist.c | 2 +-
crypto/asymmetric_keys/asymmetric_type.c | 2 +-
include/linux/key.h | 2 ++
net/rxrpc/security.c | 2 +-
security/keys/internal.h | 4 ++++
security/keys/keyctl.c | 6 ++++--
security/keys/keyring.c | 13 ++++++++-----
security/keys/permission.c | 31 ++++++++++++++++++++++++++++++
security/keys/proc.c | 1 +
security/keys/process_keys.c | 8 ++++++--
security/keys/request_key.c | 5 +++++
security/keys/request_key_auth.c | 1 +
12 files changed, 65 insertions(+), 12 deletions(-)
diff --git a/certs/blacklist.c b/certs/blacklist.c
index aff83e3a9f49..29c3cb6254d9 100644
--- a/certs/blacklist.c
+++ b/certs/blacklist.c
@@ -123,7 +123,7 @@ int is_hash_blacklisted(const u8 *hash, size_t hash_len, const char *type)
*p = 0;
kref = keyring_search(make_key_ref(blacklist_keyring, true),
- &key_type_blacklist, buffer, false);
+ &key_type_blacklist, buffer, KEY_NEED_USE, false);
if (!IS_ERR(kref)) {
key_ref_put(kref);
ret = -EKEYREJECTED;
diff --git a/crypto/asymmetric_keys/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c
index 6e5fc8e31f01..4559ac2f0bb7 100644
--- a/crypto/asymmetric_keys/asymmetric_type.c
+++ b/crypto/asymmetric_keys/asymmetric_type.c
@@ -83,7 +83,7 @@ struct key *find_asymmetric_key(struct key *keyring,
pr_debug("Look up: \"%s\"\n", req);
ref = keyring_search(make_key_ref(keyring, 1),
- &key_type_asymmetric, req, true);
+ &key_type_asymmetric, req, KEY_NEED_USE, true);
if (IS_ERR(ref))
pr_debug("Request for key '%s' err %ld\n", req, PTR_ERR(ref));
kfree(req);
diff --git a/include/linux/key.h b/include/linux/key.h
index 94a6d51464b5..0db5539366e7 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -296,6 +296,7 @@ extern struct key *key_alloc(struct key_type *type,
#define KEY_ALLOC_BUILT_IN 0x0004 /* Key is built into kernel */
#define KEY_ALLOC_BYPASS_RESTRICTION 0x0008 /* Override the check on restricted keyrings */
#define KEY_ALLOC_UID_KEYRING 0x0010 /* allocating a user or user session keyring */
+#define KEY_ALLOC_USERSPACE_REQUEST 0x0020 /* Userspace requested the key */
extern void key_revoke(struct key *key);
extern void key_invalidate(struct key *key);
@@ -432,6 +433,7 @@ extern int keyring_clear(struct key *keyring);
extern key_ref_t keyring_search(key_ref_t keyring,
struct key_type *type,
const char *description,
+ enum key_need_perm need_perm,
bool recurse);
extern int keyring_add_key(struct key *keyring,
diff --git a/net/rxrpc/security.c b/net/rxrpc/security.c
index 9b1fb9ed0717..23077cfe3d44 100644
--- a/net/rxrpc/security.c
+++ b/net/rxrpc/security.c
@@ -141,7 +141,7 @@ bool rxrpc_look_up_server_security(struct rxrpc_local *local, struct rxrpc_sock
/* look through the service's keyring */
kref = keyring_search(make_key_ref(rx->securities, 1UL),
- &key_type_rxrpc_s, kdesc, true);
+ &key_type_rxrpc_s, kdesc, KEY_NEED_USE, true);
if (IS_ERR(kref)) {
trace_rxrpc_abort(0, "SVK",
sp->hdr.cid, sp->hdr.callNumber, sp->hdr.seq,
diff --git a/security/keys/internal.h b/security/keys/internal.h
index af2c9531c435..d0d1bce95674 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -131,6 +131,7 @@ struct keyring_search_context {
struct keyring_index_key index_key;
const struct cred *cred;
struct key_match_data match_data;
+ enum key_need_perm need_perm; /* Permission required for search */
unsigned flags;
#define KEYRING_SEARCH_NO_STATE_CHECK 0x0001 /* Skip state checks */
#define KEYRING_SEARCH_DO_STATE_CHECK 0x0002 /* Override NO_STATE_CHECK */
@@ -196,6 +197,9 @@ extern void key_gc_keytype(struct key_type *ktype);
extern int key_task_permission(const key_ref_t key_ref,
const struct cred *cred,
enum key_need_perm need_perm);
+extern int key_search_permission(const key_ref_t key_ref,
+ struct keyring_search_context *ctx,
+ enum key_need_perm need_perm);
extern unsigned int key_acl_to_perm(const struct key_acl *acl);
extern long key_set_acl(struct key *key, struct key_acl *acl);
extern void key_put_acl(struct key_acl *acl);
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index fae2df676e30..54a2bfff9af2 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -225,7 +225,8 @@ SYSCALL_DEFINE4(request_key, const char __user *, _type,
key = request_key_and_link(ktype, description, NULL, callout_info,
callout_len, NULL, NULL,
key_ref_to_ptr(dest_ref),
- KEY_ALLOC_IN_QUOTA);
+ KEY_ALLOC_IN_QUOTA |
+ KEY_ALLOC_USERSPACE_REQUEST);
if (IS_ERR(key)) {
ret = PTR_ERR(key);
goto error5;
@@ -685,7 +686,8 @@ long keyctl_keyring_search(key_serial_t ringid,
}
/* do the search */
- key_ref = keyring_search(keyring_ref, ktype, description, true);
+ key_ref = keyring_search(keyring_ref, ktype, description,
+ KEY_NEED_SEARCH, true);
if (IS_ERR(key_ref)) {
ret = PTR_ERR(key_ref);
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index f14aabf27a51..1779c95b428c 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -621,8 +621,8 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
/* key must have search permissions */
if (!(ctx->flags & KEYRING_SEARCH_NO_CHECK_PERM) &&
- key_task_permission(make_key_ref(key, ctx->possessed),
- ctx->cred, KEY_NEED_SEARCH) < 0) {
+ key_search_permission(make_key_ref(key, ctx->possessed),
+ ctx, ctx->need_perm) < 0) {
ctx->result = ERR_PTR(-EACCES);
kleave(" = %d [!perm]", ctx->skipped_ret);
goto skipped;
@@ -798,8 +798,8 @@ static bool search_nested_keyrings(struct key *keyring,
/* Search a nested keyring */
if (!(ctx->flags & KEYRING_SEARCH_NO_CHECK_PERM) &&
- key_task_permission(make_key_ref(key, ctx->possessed),
- ctx->cred, KEY_NEED_SEARCH) < 0)
+ key_search_permission(make_key_ref(key, ctx->possessed),
+ ctx, KEY_NEED_SEARCH) < 0)
continue;
/* stack the current position */
@@ -921,7 +921,7 @@ key_ref_t keyring_search_rcu(key_ref_t keyring_ref,
return ERR_PTR(-ENOTDIR);
if (!(ctx->flags & KEYRING_SEARCH_NO_CHECK_PERM)) {
- err = key_task_permission(keyring_ref, ctx->cred, KEY_NEED_SEARCH);
+ err = key_search_permission(keyring_ref, ctx, ctx->need_perm);
if (err < 0)
return ERR_PTR(err);
}
@@ -937,6 +937,7 @@ key_ref_t keyring_search_rcu(key_ref_t keyring_ref,
* @keyring: The root of the keyring tree to be searched.
* @type: The type of keyring we want to find.
* @description: The name of the keyring we want to find.
+ * @need_perm: The permission required of the target key.
* @recurse: True to search the children of @keyring also
*
* As keyring_search_rcu() above, but using the current task's credentials and
@@ -945,6 +946,7 @@ key_ref_t keyring_search_rcu(key_ref_t keyring_ref,
key_ref_t keyring_search(key_ref_t keyring,
struct key_type *type,
const char *description,
+ enum key_need_perm need_perm,
bool recurse)
{
struct keyring_search_context ctx = {
@@ -955,6 +957,7 @@ key_ref_t keyring_search(key_ref_t keyring,
.match_data.cmp = key_default_cmp,
.match_data.raw_data = description,
.match_data.lookup_type = KEYRING_SEARCH_LOOKUP_DIRECT,
+ .need_perm = need_perm,
.flags = KEYRING_SEARCH_DO_STATE_CHECK,
};
key_ref_t key;
diff --git a/security/keys/permission.c b/security/keys/permission.c
index 0bb7f6b695f4..3ae4d9aedc3a 100644
--- a/security/keys/permission.c
+++ b/security/keys/permission.c
@@ -253,6 +253,37 @@ int key_task_permission(const key_ref_t key_ref, const struct cred *cred,
return security_key_permission(key_ref, cred, need_perm, notes);
}
+/**
+ * key_search_permission - Check a key can be searched for
+ * @key_ref: The key to check.
+ * @cred: The credentials to use.
+ * @need_perm: The permission required.
+ *
+ * Check to see whether permission is granted to use a key in the desired way,
+ * but permit the security modules to override.
+ *
+ * The caller must hold the RCU readlock.
+ *
+ * Returns 0 if successful, -EACCES if access is denied based on the
+ * permissions bits or the LSM check.
+ */
+int key_search_permission(const key_ref_t key_ref,
+ struct keyring_search_context *ctx,
+ enum key_need_perm need_perm)
+{
+ unsigned int allow, notes = 0;
+ int ret;
+
+ allow = key_resolve_acl(key_ref, ctx->cred);
+
+ ret = check_key_permission(key_ref, ctx->cred, allow, need_perm, ¬es);
+ if (ret < 0)
+ return ret;
+
+ /* Let the LSMs be the final arbiter */
+ return security_key_permission(key_ref, ctx->cred, need_perm, notes);
+}
+
/**
* key_validate - Validate a key.
* @key: The key to be validated.
diff --git a/security/keys/proc.c b/security/keys/proc.c
index c68ec5f98659..a6b349ee1759 100644
--- a/security/keys/proc.c
+++ b/security/keys/proc.c
@@ -174,6 +174,7 @@ static int proc_keys_show(struct seq_file *m, void *v)
.match_data.cmp = lookup_user_key_possessed,
.match_data.raw_data = key,
.match_data.lookup_type = KEYRING_SEARCH_LOOKUP_DIRECT,
+ .need_perm = KEY_NEED_SEARCH,
.flags = (KEYRING_SEARCH_NO_STATE_CHECK |
KEYRING_SEARCH_RECURSE),
};
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index 11227101bea0..3721f96dd6fb 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -135,7 +135,8 @@ int look_up_user_keyrings(struct key **_user_keyring,
*/
snprintf(buf, sizeof(buf), "_uid.%u", uid);
uid_keyring_r = keyring_search(make_key_ref(reg_keyring, true),
- &key_type_keyring, buf, false);
+ &key_type_keyring, buf, KEY_NEED_SEARCH,
+ false);
kdebug("_uid %p", uid_keyring_r);
if (uid_keyring_r == ERR_PTR(-EAGAIN)) {
uid_keyring = keyring_alloc(buf, cred->user->uid, INVALID_GID,
@@ -157,7 +158,8 @@ int look_up_user_keyrings(struct key **_user_keyring,
/* Get a default session keyring (which might also exist already) */
snprintf(buf, sizeof(buf), "_uid_ses.%u", uid);
session_keyring_r = keyring_search(make_key_ref(reg_keyring, true),
- &key_type_keyring, buf, false);
+ &key_type_keyring, buf, KEY_NEED_SEARCH,
+ false);
kdebug("_uid_ses %p", session_keyring_r);
if (session_keyring_r == ERR_PTR(-EAGAIN)) {
session_keyring = keyring_alloc(buf, cred->user->uid, INVALID_GID,
@@ -230,6 +232,7 @@ struct key *get_user_session_keyring_rcu(const struct cred *cred)
.match_data.cmp = key_default_cmp,
.match_data.raw_data = buf,
.match_data.lookup_type = KEYRING_SEARCH_LOOKUP_DIRECT,
+ .need_perm = KEY_NEED_SEARCH,
.flags = KEYRING_SEARCH_DO_STATE_CHECK,
};
@@ -648,6 +651,7 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
struct keyring_search_context ctx = {
.match_data.cmp = lookup_user_key_possessed,
.match_data.lookup_type = KEYRING_SEARCH_LOOKUP_DIRECT,
+ .need_perm = KEY_NEED_SEARCH,
.flags = (KEYRING_SEARCH_NO_STATE_CHECK |
KEYRING_SEARCH_RECURSE),
};
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 2b84efb420cb..479ae0573d1e 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -567,6 +567,7 @@ struct key *request_key_and_link(struct key_type *type,
.match_data.cmp = key_default_cmp,
.match_data.raw_data = description,
.match_data.lookup_type = KEYRING_SEARCH_LOOKUP_DIRECT,
+ .need_perm = KEY_NEED_USE,
.flags = (KEYRING_SEARCH_DO_STATE_CHECK |
KEYRING_SEARCH_SKIP_EXPIRED |
KEYRING_SEARCH_RECURSE),
@@ -579,6 +580,9 @@ struct key *request_key_and_link(struct key_type *type,
ctx.index_key.type->name, ctx.index_key.description,
callout_info, callout_len, aux, dest_keyring, flags);
+ if (flags & KEY_ALLOC_USERSPACE_REQUEST)
+ ctx.need_perm = KEY_NEED_SEARCH;
+
if (type->match_preparse) {
ret = type->match_preparse(&ctx.match_data);
if (ret < 0) {
@@ -774,6 +778,7 @@ struct key *request_key_rcu(struct key_type *type,
.match_data.cmp = key_default_cmp,
.match_data.raw_data = description,
.match_data.lookup_type = KEYRING_SEARCH_LOOKUP_DIRECT,
+ .need_perm = KEY_NEED_USE,
.flags = (KEYRING_SEARCH_DO_STATE_CHECK |
KEYRING_SEARCH_SKIP_EXPIRED),
};
diff --git a/security/keys/request_key_auth.c b/security/keys/request_key_auth.c
index ee8c5fe6ed61..f8f77af152de 100644
--- a/security/keys/request_key_auth.c
+++ b/security/keys/request_key_auth.c
@@ -264,6 +264,7 @@ struct key *key_get_instantiation_authkey(key_serial_t target_id)
.match_data.cmp = key_default_cmp,
.match_data.raw_data = description,
.match_data.lookup_type = KEYRING_SEARCH_LOOKUP_DIRECT,
+ .need_perm = KEY_NEED_USE,
.flags = (KEYRING_SEARCH_DO_STATE_CHECK |
KEYRING_SEARCH_RECURSE),
};
^ permalink raw reply related
* [RFC PATCH 5/5] keys: Implement a 'container' keyring
From: David Howells @ 2020-07-16 20:35 UTC (permalink / raw)
To: Stephen Smalley, Casey Schaufler
Cc: dhowells, Jarkko Sakkinen, Eric Biederman, jlayton, christian,
selinux, linux-afs, linux-nfs, linux-cifs, linux-api,
linux-fsdevel, linux-security-module, linux-kernel, containers
In-Reply-To: <159493167778.3249370.8145886688150701997.stgit@warthog.procyon.org.uk>
Implement a per-container keyring, dangling it off of a user_namespace.
The properties of this keyring are such that it's searched by request_key()
selectively before or after the other keyrings have been searched, but the
keys in it don't grant possession to the denizens of the container, and so
the denizens can't see such keys unless those keys grant direct access
through the ACL. The kernel recurses up the user_namespace stack looking
for keyrings.
The container manager, however, can access the keyring, and can add, update
and remove keys therein.
This allows the container manager to push filesystem authentication keys,
for example, into the container and to keep them refreshed without the
denizens of the container needing to know anything about it.
To this end, the following pieces are also added:
(1) A new keyctl function, KEYCTL_GET_CONTAINER_KEYRING, to get the
container keyring from a user namespace:
keyring = keyctl_get_userns_keyring(int userns_fd,
key_serial_t dest_keyring);
Get the container keyring attached to a user namespace, creating it if
it doesn't exist. A file descriptor pointing to the user namespace
must be supplied. The keyring will be linked into the destination
keyring if one is supplied (ie. not 0). The keyring will be owned by
the user_namespace's owner and will grant various permissions to the
possessor.
(2) An ACL ACE type that allows access to a key by a container:
keyctl_grant_acl(key_serial_t key,
KEY_ACE_SUBJ_CONTAINER,
int userns_fd,
KEY_ACE_SEARCH);
This grants the kernel the ability to use a key on behalf of the
denizens of a container, but doesn't grant any other rights, including
the ability of the denizens see the key even exists.
This can then be tested with something like the following from the command
line:
(1) Get the container keyring for a user namespace and link it to the
session keyring. The container is referenced as file descriptor 5.
# keyctl get_container 5 @s 5</proc/self/ns/user
197321290
(2) Get a key that should be placed into the container, e.g.:
# kinit foo@EXAMPLE.COM
# aklog-kafs example.com
This, say, adds key 748104263 to the session keyring.
(3) Grant permission to the container to use the key:
# keyctl grant 748104263 cont:5 s 5</proc/self/ns/user
(4) Move (or link) the key into the container keyring:
# keyctl move 748104263 @s 197321290
(5) View the resultant keyrings:
# keyctl show
Session Keyring
711486290 --alswrv 0 0 keyring: _ses
468790230 ---lswrv 0 65534 \_ keyring: _uid.0
197321290 ----swrv 0 65534 \_ keyring: .container
748104263 --alswrv 0 0 \_ rxrpc: afs@example.com
Signed-off-by: David Howells <dhowells@redhat.com>
---
include/linux/key.h | 13 +++-
include/linux/user_namespace.h | 7 ++
include/uapi/linux/keyctl.h | 3 +
security/keys/Kconfig | 11 +++
security/keys/compat.c | 3 +
security/keys/internal.h | 10 +++
security/keys/key.c | 2 -
security/keys/keyctl.c | 128 ++++++++++++++++++++++++++++++++++++++++
security/keys/keyring.c | 7 ++
security/keys/permission.c | 94 +++++++++++++++++++++++++++--
security/keys/proc.c | 2 -
security/keys/process_keys.c | 36 +++++++++++
12 files changed, 302 insertions(+), 14 deletions(-)
diff --git a/include/linux/key.h b/include/linux/key.h
index 0db5539366e7..810ea1ce01f4 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -112,7 +112,8 @@ struct key_ace {
union {
kuid_t uid;
kgid_t gid;
- unsigned int subject_id;
+ unsigned long subject_id;
+ struct key_tag *subject_tag;
};
};
@@ -124,13 +125,13 @@ struct key_acl {
struct key_ace aces[];
};
-#define KEY_POSSESSOR_ACE(perms) { \
+#define KEY_POSSESSOR_ACE(perms) (struct key_ace){ \
.type = KEY_ACE_SUBJ_STANDARD, \
.perm = perms, \
.subject_id = KEY_ACE_POSSESSOR \
}
-#define KEY_OWNER_ACE(perms) { \
+#define KEY_OWNER_ACE(perms) (struct key_ace){ \
.type = KEY_ACE_SUBJ_STANDARD, \
.perm = perms, \
.subject_id = KEY_ACE_OWNER \
@@ -320,6 +321,12 @@ static inline void key_ref_put(key_ref_t key_ref)
key_put(key_ref_to_ptr(key_ref));
}
+static inline struct key_tag *key_get_tag(struct key_tag *tag)
+{
+ refcount_inc(&tag->usage);
+ return tag;
+}
+
extern struct key *request_key_tag(struct key_type *type,
const char *description,
struct key_tag *domain_tag,
diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
index 6ef1c7109fc4..f007258bd3d9 100644
--- a/include/linux/user_namespace.h
+++ b/include/linux/user_namespace.h
@@ -79,6 +79,13 @@ struct user_namespace {
/* Register of per-UID persistent keyrings for this namespace */
#ifdef CONFIG_PERSISTENT_KEYRINGS
struct key *persistent_keyring_register;
+#endif
+ /* Ring of keys that the namespace owner can insert into the
+ * namespace for transparent access by the denizens.
+ */
+#ifdef CONFIG_CONTAINER_KEYRINGS
+ struct key *container_keyring;
+ struct key_tag *container_subj; /* The ACE subject to match */
#endif
struct work_struct work;
#ifdef CONFIG_SYSCTL
diff --git a/include/uapi/linux/keyctl.h b/include/uapi/linux/keyctl.h
index a5938f2c3e66..2579fe75b93f 100644
--- a/include/uapi/linux/keyctl.h
+++ b/include/uapi/linux/keyctl.h
@@ -20,6 +20,7 @@
*/
enum key_ace_subject_type {
KEY_ACE_SUBJ_STANDARD = 0, /* subject is one of key_ace_standard_subject */
+ KEY_ACE_SUBJ_CONTAINER = 1, /* Subject is an fd referring to a container (eg. userns) */
nr__key_ace_subject_type
};
@@ -134,6 +135,7 @@ enum key_ace_standard_subject {
#define KEYCTL_CAPABILITIES 31 /* Find capabilities of keyrings subsystem */
#define KEYCTL_WATCH_KEY 32 /* Watch a key or ring of keys for changes */
#define KEYCTL_GRANT_PERMISSION 33 /* Grant a permit to a key */
+#define KEYCTL_GET_CONTAINER_KEYRING 34 /* Get a container keyring */
/* keyctl structures */
struct keyctl_dh_params {
@@ -198,5 +200,6 @@ struct keyctl_pkey_params {
#define KEYCTL_CAPS1_NOTIFICATIONS 0x04 /* Keys generate watchable notifications */
#define KEYCTL_CAPS1_ACL 0x08 /* Keys have ACLs rather than a p-u-g-o bitmask */
#define KEYCTL_CAPS1_GRANT_PERMISSION 0x10 /* KEYCTL_GRANT_PERMISSION is supported */
+#define KEYCTL_CAPS1_CONTAINER_KEYRINGS 0x20 /* Container keyrings are supported */
#endif /* _LINUX_KEYCTL_H */
diff --git a/security/keys/Kconfig b/security/keys/Kconfig
index 83bc23409164..df138027942e 100644
--- a/security/keys/Kconfig
+++ b/security/keys/Kconfig
@@ -123,3 +123,14 @@ config KEY_NOTIFICATIONS
and keyrings on which the caller has View permission. This makes use
of the /dev/watch_queue misc device to handle the notification
buffer and provides KEYCTL_WATCH_KEY to enable/disable watches.
+
+config CONTAINER_KEYRINGS
+ bool "Provide per-container keyrings"
+ depends on KEYS && USER_NS
+ help
+ This option provides a keyring on each user_namespace that is
+ searched by request_key() after it has searched the normal process
+ keyrings. This is a place that the container manager can insert
+ filesystem authentication keys into a container so that the denizens
+ can use authenticated storage without having to do anything for
+ themselves - the manager can take care of that.
diff --git a/security/keys/compat.c b/security/keys/compat.c
index 2b675f9a6162..3c9eecb43bba 100644
--- a/security/keys/compat.c
+++ b/security/keys/compat.c
@@ -161,6 +161,9 @@ COMPAT_SYSCALL_DEFINE5(keyctl, u32, option,
case KEYCTL_WATCH_KEY:
return keyctl_watch_key(arg2, arg3, arg4);
+ case KEYCTL_GET_CONTAINER_KEYRING:
+ return keyctl_get_container_keyring(arg2, arg3);
+
default:
return -EOPNOTSUPP;
}
diff --git a/security/keys/internal.h b/security/keys/internal.h
index d0d1bce95674..e3218aa72d4c 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -144,6 +144,7 @@ struct keyring_search_context {
int (*iterator)(const void *object, void *iterator_data);
/* Internal stuff */
+ const struct key_tag *container_subj; /* The ACE container subject or NULL */
int skipped_ret;
bool possessed;
key_ref_t result;
@@ -386,6 +387,15 @@ extern long keyctl_grant_permission(key_serial_t keyid,
unsigned int subject,
unsigned int perm);
+#ifdef CONFIG_KEY_NOTIFICATIONS
+extern long keyctl_get_container_keyring(int container_fd, key_serial_t destringid);
+#else
+static inline long keyctl_get_container_keyring(int container_fd, key_serial_t destringid)
+{
+ return -EOPNOTSUPP;
+}
+#endif
+
/*
* Debugging key validation
*/
diff --git a/security/keys/key.c b/security/keys/key.c
index 51491c07d7b9..17da784de9e8 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -318,7 +318,7 @@ struct key *key_alloc(struct key_type *type, const char *desc,
goto security_error;
/* publish the key by giving it a serial number */
- refcount_inc(&key->domain_tag->usage);
+ key_get_tag(key->domain_tag);
atomic_inc(&user->nkeys);
key_alloc_serial(key);
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 54a2bfff9af2..786da7382b7a 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -21,6 +21,10 @@
#include <linux/security.h>
#include <linux/uio.h>
#include <linux/uaccess.h>
+#include <linux/file.h>
+#include <linux/proc_fs.h>
+#include <linux/proc_ns.h>
+#include <linux/user_namespace.h>
#include <keys/request_key_auth-type.h>
#include "internal.h"
@@ -40,7 +44,8 @@ static const unsigned char keyrings_capabilities[2] = {
KEYCTL_CAPS1_NS_KEY_TAG |
(IS_ENABLED(CONFIG_KEY_NOTIFICATIONS) ? KEYCTL_CAPS1_NOTIFICATIONS : 0) |
KEYCTL_CAPS1_ACL |
- KEYCTL_CAPS1_GRANT_PERMISSION
+ KEYCTL_CAPS1_GRANT_PERMISSION |
+ (IS_ENABLED(CONFIG_CONTAINER_KEYRINGS) ? KEYCTL_CAPS1_CONTAINER_KEYRINGS : 0)
),
};
@@ -1758,6 +1763,124 @@ long keyctl_watch_key(key_serial_t id, int watch_queue_fd, int watch_id)
}
#endif /* CONFIG_KEY_NOTIFICATIONS */
+#ifdef CONFIG_CONTAINER_KEYRINGS
+/*
+ * Create a container keyring for a user namespace and add it.
+ */
+static struct key *key_create_container_keyring(struct user_namespace *user_ns)
+{
+ struct key_tag *tag;
+ struct key_acl *acl;
+ struct key *keyring;
+
+ keyring = key_get(user_ns->container_keyring);
+ if (keyring)
+ return keyring;
+
+ /* We're going to need a subject tag... */
+ tag = user_ns->container_subj;
+ if (!tag) {
+ tag = kzalloc(sizeof(struct key_tag), GFP_KERNEL);
+ if (!tag)
+ return ERR_PTR(-ENOMEM);
+ refcount_set(&tag->usage, 1);
+ user_ns->container_subj = tag;
+ }
+
+ /* ... so that we can grant the container denizens search permission
+ * on the keyring.
+ */
+ acl = kzalloc(struct_size(acl, aces, 3), GFP_KERNEL);
+ if (!acl)
+ return ERR_PTR(-ENOMEM);
+
+ refcount_set(&acl->usage, 1);
+ acl->possessor_viewable = true;
+ acl->nr_ace = 3;
+
+ acl->aces[0] = KEY_POSSESSOR_ACE(KEY_ACE_VIEW | KEY_ACE_READ | KEY_ACE_WRITE |
+ KEY_ACE_CLEAR | KEY_ACE_SEARCH);
+ acl->aces[1] = KEY_OWNER_ACE(KEY_ACE_VIEW | KEY_ACE_READ);
+
+ acl->aces[2].type = KEY_ACE_SUBJ_CONTAINER;
+ acl->aces[2].subject_tag = key_get_tag(tag);
+ acl->aces[2].perm = KEY_ACE_SEARCH;
+
+ keyring = keyring_alloc(".container", user_ns->owner, INVALID_GID,
+ current_cred(), acl, 0, NULL, NULL);
+ key_put_acl(acl);
+ if (IS_ERR(keyring))
+ return keyring;
+
+ smp_store_release(&user_ns->container_keyring, key_get(keyring));
+ return keyring;
+}
+
+/*
+ * Get the container keyring attached to a container. The container is
+ * referenced by a file descriptor referring to, say, a user_namespace.
+ */
+long keyctl_get_container_keyring(int container_fd, key_serial_t destringid)
+{
+ struct user_namespace *user_ns;
+ struct ns_common *ns;
+ struct file *f;
+ struct key *keyring;
+ key_ref_t dest_ref;
+ int ret = -EINVAL;
+
+ f = fget(container_fd);
+ if (!f)
+ return -EBADF;
+
+ if (!proc_ns_file(f))
+ goto error_file;
+ ns = get_proc_ns(file_inode(f));
+ if (ns->ops->type != CLONE_NEWUSER)
+ goto error_file;
+ user_ns = container_of(ns, struct user_namespace, ns);
+
+ keyring = key_get(READ_ONCE(user_ns->container_keyring));
+ if (!keyring) {
+ down_write(&user_ns->keyring_sem);
+ keyring = key_create_container_keyring(user_ns);
+ up_write(&user_ns->keyring_sem);
+ if (IS_ERR(keyring)) {
+ ret = PTR_ERR(keyring);
+ goto error_file;
+ }
+ }
+
+ /* Get the destination keyring if specified. We don't need LINK
+ * permission on the container keyring as having the container fd is
+ * sufficient to grant us that.
+ */
+ dest_ref = NULL;
+ if (destringid) {
+ dest_ref = lookup_user_key(destringid, KEY_LOOKUP_CREATE,
+ KEY_NEED_KEYRING_ADD);
+ if (IS_ERR(dest_ref)) {
+ ret = PTR_ERR(dest_ref);
+ goto error_keyring;
+ }
+
+ ret = key_link(key_ref_to_ptr(dest_ref), keyring);
+ if (ret < 0)
+ goto error_dest;
+ }
+
+ ret = key_serial(keyring);
+
+error_dest:
+ key_ref_put(dest_ref);
+error_keyring:
+ key_put(keyring);
+error_file:
+ fput(f);
+ return ret;
+}
+#endif
+
/*
* Get keyrings subsystem capabilities.
*/
@@ -1935,6 +2058,9 @@ SYSCALL_DEFINE5(keyctl, int, option, unsigned long, arg2, unsigned long, arg3,
case KEYCTL_WATCH_KEY:
return keyctl_watch_key((key_serial_t)arg2, (int)arg3, (int)arg4);
+ case KEYCTL_GET_CONTAINER_KEYRING:
+ return keyctl_get_container_keyring((int)arg2, (key_serial_t)arg3);
+
default:
return -EOPNOTSUPP;
}
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 1779c95b428c..eb311987bb9b 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -64,6 +64,13 @@ void key_free_user_ns(struct user_namespace *ns)
#ifdef CONFIG_PERSISTENT_KEYRINGS
key_put(ns->persistent_keyring_register);
#endif
+#ifdef CONFIG_CONTAINER_KEYRINGS
+ if (ns->container_subj) {
+ ns->container_subj->removed = true;
+ key_put_tag(ns->container_subj);
+ }
+ key_put(ns->container_keyring);
+#endif
}
/*
diff --git a/security/keys/permission.c b/security/keys/permission.c
index 3ae4d9aedc3a..b984e1ce7e5d 100644
--- a/security/keys/permission.c
+++ b/security/keys/permission.c
@@ -7,6 +7,10 @@
#include <linux/export.h>
#include <linux/security.h>
+#include <linux/file.h>
+#include <linux/proc_fs.h>
+#include <linux/proc_ns.h>
+#include <linux/user_namespace.h>
#include <keys/request_key_auth-type.h>
#include "internal.h"
@@ -177,7 +181,9 @@ static int check_key_permission(const key_ref_t key_ref, const struct cred *cred
/*
* Resolve an ACL to a mask.
*/
-static unsigned int key_resolve_acl(const key_ref_t key_ref, const struct cred *cred)
+static unsigned int key_resolve_acl(const key_ref_t key_ref,
+ const struct cred *cred,
+ const struct key_tag *tag)
{
const struct key *key = key_ref_to_ptr(key_ref);
const struct key_acl *acl;
@@ -215,6 +221,11 @@ static unsigned int key_resolve_acl(const key_ref_t key_ref, const struct cred *
break;
}
break;
+
+ case KEY_ACE_SUBJ_CONTAINER:
+ if (ace->subject_tag == tag)
+ allow |= ace->perm;
+ break;
}
}
@@ -242,7 +253,7 @@ int key_task_permission(const key_ref_t key_ref, const struct cred *cred,
int ret;
rcu_read_lock();
- allow = key_resolve_acl(key_ref, cred);
+ allow = key_resolve_acl(key_ref, cred, NULL);
rcu_read_unlock();
ret = check_key_permission(key_ref, cred, allow, need_perm, ¬es);
@@ -274,7 +285,7 @@ int key_search_permission(const key_ref_t key_ref,
unsigned int allow, notes = 0;
int ret;
- allow = key_resolve_acl(key_ref, ctx->cred);
+ allow = key_resolve_acl(key_ref, ctx->cred, ctx->container_subj);
ret = check_key_permission(key_ref, ctx->cred, allow, need_perm, ¬es);
if (ret < 0)
@@ -374,13 +385,24 @@ unsigned int key_acl_to_perm(const struct key_acl *acl)
return perm;
}
+static void key_free_acl(struct rcu_head *rcu)
+{
+ struct key_acl *acl = container_of(rcu, struct key_acl, rcu);
+ unsigned int i;
+
+ for (i = 0; i < acl->nr_ace; i++)
+ if (acl->aces[i].type == KEY_ACE_SUBJ_CONTAINER)
+ key_put_tag(acl->aces[i].subject_tag);
+ kfree(acl);
+}
+
/*
* Destroy a key's ACL.
*/
void key_put_acl(struct key_acl *acl)
{
if (acl && refcount_dec_and_test(&acl->usage))
- kfree_rcu(acl, rcu);
+ call_rcu(&acl->rcu, key_free_acl);
}
/*
@@ -440,7 +462,8 @@ static struct key_acl *key_alloc_acl(const struct key_acl *old_acl, int nr, int
}
/*
- * Generate the revised ACL.
+ * Generate the revised ACL. If the new ACE contains a key_tag and we don't
+ * have the tag in ACL yet, we steal the tag and clear the caller's pointer.
*/
static long key_change_acl(struct key *key, struct key_ace *new_ace)
{
@@ -461,6 +484,7 @@ static long key_change_acl(struct key *key, struct key_ace *new_ace)
if (IS_ERR(acl))
return PTR_ERR(acl);
acl->aces[i] = *new_ace;
+ new_ace->subject_tag = NULL; /* Stole the tag */
goto change;
found_match:
@@ -484,6 +508,49 @@ static long key_change_acl(struct key *key, struct key_ace *new_ace)
return key_set_acl(key, acl);
}
+/*
+ * Look up the user namespace tag associated with a fd.
+ */
+static struct key_tag *key_get_ns_tag(int fd)
+{
+#ifdef CONFIG_CONTAINER_KEYRINGS
+ struct user_namespace *userns;
+ struct ns_common *ns;
+ struct key_tag *tag = ERR_PTR(-EINVAL), *candidate;
+ struct file *f;
+
+ f = fget(fd);
+ if (!f)
+ return ERR_PTR(-EBADF);
+
+ if (!proc_ns_file(f))
+ goto error;
+ ns = get_proc_ns(file_inode(f));
+ if (ns->ops->type != CLONE_NEWUSER)
+ goto error;
+
+ userns = container_of(ns, struct user_namespace, ns);
+ if (!userns->container_subj) {
+ candidate = kzalloc(sizeof(struct key_tag), GFP_KERNEL);
+ refcount_set(&candidate->usage, 1);
+ down_write(&userns->keyring_sem);
+ if (!userns->container_subj) {
+ userns->container_subj = candidate;
+ candidate = NULL;
+ }
+ up_write(&userns->keyring_sem);
+ kfree(candidate);
+ }
+
+ tag = key_get_tag(userns->container_subj);
+error:
+ fput(f);
+ return tag;
+#else
+ return ERR_PTR(-EOPNOTSUPP);
+#endif
+}
+
/*
* Add, alter or remove (if perm == 0) an ACE in a key's ACL.
*/
@@ -492,13 +559,15 @@ long keyctl_grant_permission(key_serial_t keyid,
unsigned int subject,
unsigned int perm)
{
- struct key_ace new_ace;
+ struct key_tag *tag;
struct key *key;
key_ref_t key_ref;
long ret;
- new_ace.type = type;
- new_ace.perm = perm;
+ struct key_ace new_ace = {
+ .type = type,
+ .perm = perm,
+ };
switch (type) {
case KEY_ACE_SUBJ_STANDARD:
@@ -507,6 +576,13 @@ long keyctl_grant_permission(key_serial_t keyid,
new_ace.subject_id = subject;
break;
+ case KEY_ACE_SUBJ_CONTAINER:
+ tag = key_get_ns_tag(subject);
+ if (IS_ERR(tag))
+ return PTR_ERR(tag);
+ new_ace.subject_tag = tag;
+ break;
+
default:
return -ENOENT;
}
@@ -529,5 +605,7 @@ long keyctl_grant_permission(key_serial_t keyid,
up_write(&key->sem);
key_put(key);
error:
+ if (new_ace.type == KEY_ACE_SUBJ_CONTAINER && new_ace.subject_tag)
+ key_put_tag(new_ace.subject_tag);
return ret;
}
diff --git a/security/keys/proc.c b/security/keys/proc.c
index a6b349ee1759..ba3be0e9c97d 100644
--- a/security/keys/proc.c
+++ b/security/keys/proc.c
@@ -183,7 +183,7 @@ static int proc_keys_show(struct seq_file *m, void *v)
check_pos = acl->possessor_viewable;
/* determine if the key is possessed by this process (a test we can
- * skip if the key does not indicate the possessor can view it
+ * skip if the key does not indicate the possessor can view it)
*/
key_ref = make_key_ref(key, 0);
if (check_pos) {
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index 3721f96dd6fb..af09db1e5984 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -458,6 +458,7 @@ void key_fsgid_changed(struct cred *new_cred)
*/
key_ref_t search_cred_keyrings_rcu(struct keyring_search_context *ctx)
{
+ struct user_namespace *userns;
struct key *user_session;
key_ref_t key_ref, ret, err;
const struct cred *cred = ctx->cred;
@@ -556,6 +557,41 @@ key_ref_t search_cred_keyrings_rcu(struct keyring_search_context *ctx)
}
}
+#ifdef CONFIG_CONTAINER_KEYRINGS
+ for (userns = cred->user_ns; userns; userns = userns->parent) {
+ if (!userns->container_keyring || !userns->container_subj)
+ continue;
+
+ /* The denizens of the container don't possess the key
+ * and have a special subject to match.
+ */
+ ctx->container_subj = userns->container_subj;
+ key_ref = keyring_search_rcu(make_key_ref(userns->container_keyring,
+ false), ctx);
+ ctx->container_subj = NULL;
+
+ if (!IS_ERR(key_ref))
+ goto found;
+
+ switch (PTR_ERR(key_ref)) {
+ case -EAGAIN: /* no key */
+ if (ret)
+ break;
+ /* fall through */
+ case -ENOKEY: /* negative key */
+ ret = key_ref;
+ break;
+ default:
+ /* Hmmm... should we admit to the denizens of
+ * the container that a key exists?
+ */
+ err = key_ref;
+ break;
+ }
+ }
+
+#endif
+
/* no key - decide on the error we're going to go for */
key_ref = ret ? ret : err;
^ permalink raw reply related
* Re: [PATCH 2/4] fs: Remove FIRMWARE_PREALLOC_BUFFER from kernel_read_file() enums
From: Scott Branden @ 2020-07-16 20:35 UTC (permalink / raw)
To: Kees Cook
Cc: Matthew Wilcox, James Morris, Luis Chamberlain, Mimi Zohar,
Greg Kroah-Hartman, Rafael J. Wysocki, Alexander Viro, Jessica Yu,
Dmitry Kasatkin, Serge E. Hallyn, Casey Schaufler,
Eric W. Biederman, Peter Zijlstra, Matthew Garrett, David Howells,
Mauro Carvalho Chehab, Randy Dunlap, Joel Fernandes (Google),
KP Singh, Dave Olsthoorn, Hans de Goede, Peter Jones,
Andrew Morton, Stephen Boyd, Paul Moore, linux-kernel,
linux-fsdevel, linux-integrity, linux-security-module
In-Reply-To: <202007101543.912633AA73@keescook>
Hi Kees
On 2020-07-10 3:44 p.m., Kees Cook wrote:
> On Fri, Jul 10, 2020 at 03:10:25PM -0700, Scott Branden wrote:
>>
>> On 2020-07-10 3:04 p.m., Matthew Wilcox wrote:
>>> On Fri, Jul 10, 2020 at 02:00:32PM -0700, Scott Branden wrote:
>>>>> @@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
>>>>> goto out;
>>>>> }
>>>>> - if (id != READING_FIRMWARE_PREALLOC_BUFFER)
>>>>> - *buf = vmalloc(i_size);
>>>>> + if (!*buf)
>>>> The assumption that *buf is always NULL when id !=
>>>> READING_FIRMWARE_PREALLOC_BUFFER doesn't appear to be correct.
>>>> I get unhandled page faults due to this change on boot.
>>> Did it give you a stack backtrace?
>> Yes, but there's no requirement that *buf need to be NULL when calling this
>> function.
>> To fix my particular crash I added the following locally:
>>
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -3989,7 +3989,7 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char
>> __user *, uargs, int, flags)
>> {
>> struct load_info info = { };
>> loff_t size;
>> - void *hdr;
>> + void *hdr = NULL;
>> int err;
>>
>> err = may_init_module();
> Thanks for the diagnosis and fix! I haven't had time to cycle back
> around to this series yet. Hopefully soon. :)
>
In order to assist in your patchset I have combined it with my patch
series here:
https://github.com/sbranden/linux/tree/kernel_read_file_for_kees
Please let me know if this matches your expectations for my patches or
if there is something else I need to change.
Thanks,
Scott
^ permalink raw reply
* [RFC PATCH 3/5] keys: Provide KEYCTL_GRANT_PERMISSION
From: David Howells @ 2020-07-16 20:35 UTC (permalink / raw)
To: Stephen Smalley, Casey Schaufler
Cc: dhowells, Jarkko Sakkinen, Eric Biederman, jlayton, christian,
selinux, linux-afs, linux-nfs, linux-cifs, linux-api,
linux-fsdevel, linux-security-module, linux-kernel, containers
In-Reply-To: <159493167778.3249370.8145886688150701997.stgit@warthog.procyon.org.uk>
Provide a keyctl() operation to grant/remove permissions. The grant
operation, wrapped by libkeyutils, looks like:
int ret = keyctl_grant_permission(key_serial_t key,
enum key_ace_subject_type type,
unsigned int subject,
unsigned int perm);
Where key is the key to be modified, type and subject represent the subject
to which permission is to be granted (or removed) and perm is the set of
permissions to be granted. 0 is returned on success. SETSEC permission is
required for this.
The subject type currently must be KEY_ACE_SUBJ_STANDARD for the moment
(other subject types will come along later).
For subject type KEY_ACE_SUBJ_STANDARD, the following subject values are
available:
KEY_ACE_POSSESSOR The possessor of the key
KEY_ACE_OWNER The owner of the key
KEY_ACE_GROUP The key's group
KEY_ACE_EVERYONE Everyone
perm lists the permissions to be granted:
KEY_ACE_VIEW Can view the key metadata
KEY_ACE_READ Can read the key content
KEY_ACE_WRITE Can update/modify the key content
KEY_ACE_SEARCH Can find the key by searching/requesting
KEY_ACE_LINK Can make a link to the key
KEY_ACE_SETSEC Can set security
KEY_ACE_INVAL Can invalidate
KEY_ACE_REVOKE Can revoke
KEY_ACE_JOIN Can join this keyring
KEY_ACE_CLEAR Can clear this keyring
If an ACE already exists for the subject, then the permissions mask will be
overwritten; if perm is 0, it will be deleted.
Currently, the internal ACL is limited to a maximum of 16 entries.
For example:
int ret = keyctl_grant_permission(key,
KEY_ACE_SUBJ_STANDARD,
KEY_ACE_OWNER,
KEY_ACE_VIEW | KEY_ACE_READ);
Signed-off-by: David Howells <dhowells@redhat.com>
---
include/uapi/linux/keyctl.h | 2 +
security/keys/compat.c | 2 +
security/keys/internal.h | 5 ++
security/keys/keyctl.c | 8 +++
security/keys/permission.c | 120 +++++++++++++++++++++++++++++++++++++++++++
5 files changed, 136 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/keyctl.h b/include/uapi/linux/keyctl.h
index 998d4e50bd41..a5938f2c3e66 100644
--- a/include/uapi/linux/keyctl.h
+++ b/include/uapi/linux/keyctl.h
@@ -133,6 +133,7 @@ enum key_ace_standard_subject {
#define KEYCTL_MOVE 30 /* Move keys between keyrings */
#define KEYCTL_CAPABILITIES 31 /* Find capabilities of keyrings subsystem */
#define KEYCTL_WATCH_KEY 32 /* Watch a key or ring of keys for changes */
+#define KEYCTL_GRANT_PERMISSION 33 /* Grant a permit to a key */
/* keyctl structures */
struct keyctl_dh_params {
@@ -196,5 +197,6 @@ struct keyctl_pkey_params {
#define KEYCTL_CAPS1_NS_KEY_TAG 0x02 /* Key indexing can include a namespace tag */
#define KEYCTL_CAPS1_NOTIFICATIONS 0x04 /* Keys generate watchable notifications */
#define KEYCTL_CAPS1_ACL 0x08 /* Keys have ACLs rather than a p-u-g-o bitmask */
+#define KEYCTL_CAPS1_GRANT_PERMISSION 0x10 /* KEYCTL_GRANT_PERMISSION is supported */
#endif /* _LINUX_KEYCTL_H */
diff --git a/security/keys/compat.c b/security/keys/compat.c
index 6ee9d8f6a4a5..2b675f9a6162 100644
--- a/security/keys/compat.c
+++ b/security/keys/compat.c
@@ -152,6 +152,8 @@ COMPAT_SYSCALL_DEFINE5(keyctl, u32, option,
case KEYCTL_MOVE:
return keyctl_keyring_move(arg2, arg3, arg4, arg5);
+ case KEYCTL_GRANT_PERMISSION:
+ return keyctl_grant_permission(arg2, arg3, arg4, arg5);
case KEYCTL_CAPABILITIES:
return keyctl_capabilities(compat_ptr(arg2), arg3);
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 3b2114d00d5c..af2c9531c435 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -377,6 +377,11 @@ static inline long keyctl_watch_key(key_serial_t key_id, int watch_fd, int watch
}
#endif
+extern long keyctl_grant_permission(key_serial_t keyid,
+ enum key_ace_subject_type type,
+ unsigned int subject,
+ unsigned int perm);
+
/*
* Debugging key validation
*/
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 8689d4331285..fae2df676e30 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -39,7 +39,8 @@ static const unsigned char keyrings_capabilities[2] = {
[1] = (KEYCTL_CAPS1_NS_KEYRING_NAME |
KEYCTL_CAPS1_NS_KEY_TAG |
(IS_ENABLED(CONFIG_KEY_NOTIFICATIONS) ? KEYCTL_CAPS1_NOTIFICATIONS : 0) |
- KEYCTL_CAPS1_ACL
+ KEYCTL_CAPS1_ACL |
+ KEYCTL_CAPS1_GRANT_PERMISSION
),
};
@@ -1920,6 +1921,11 @@ SYSCALL_DEFINE5(keyctl, int, option, unsigned long, arg2, unsigned long, arg3,
(key_serial_t)arg3,
(key_serial_t)arg4,
(unsigned int)arg5);
+ case KEYCTL_GRANT_PERMISSION:
+ return keyctl_grant_permission((key_serial_t)arg2,
+ (enum key_ace_subject_type)arg3,
+ (unsigned int)arg4,
+ (unsigned int)arg5);
case KEYCTL_CAPABILITIES:
return keyctl_capabilities((unsigned char __user *)arg2, (size_t)arg3);
diff --git a/security/keys/permission.c b/security/keys/permission.c
index 37bad810bc16..0bb7f6b695f4 100644
--- a/security/keys/permission.c
+++ b/security/keys/permission.c
@@ -380,3 +380,123 @@ long key_set_acl(struct key *key, struct key_acl *acl)
key_put_acl(acl);
return 0;
}
+
+/*
+ * Allocate a new ACL with an extra ACE slot.
+ */
+static struct key_acl *key_alloc_acl(const struct key_acl *old_acl, int nr, int skip)
+{
+ struct key_acl *acl;
+ int nr_ace, i, j = 0;
+
+ nr_ace = old_acl->nr_ace + nr;
+ if (nr_ace > 16)
+ return ERR_PTR(-EINVAL);
+
+ acl = kzalloc(struct_size(acl, aces, nr_ace), GFP_KERNEL);
+ if (!acl)
+ return ERR_PTR(-ENOMEM);
+
+ refcount_set(&acl->usage, 1);
+ acl->nr_ace = nr_ace;
+ for (i = 0; i < old_acl->nr_ace; i++) {
+ if (i == skip)
+ continue;
+ acl->aces[j] = old_acl->aces[i];
+ j++;
+ }
+ return acl;
+}
+
+/*
+ * Generate the revised ACL.
+ */
+static long key_change_acl(struct key *key, struct key_ace *new_ace)
+{
+ struct key_acl *acl, *old;
+ int i;
+
+ old = rcu_dereference_protected(key->acl, lockdep_is_held(&key->sem));
+
+ for (i = 0; i < old->nr_ace; i++)
+ if (old->aces[i].type == new_ace->type &&
+ old->aces[i].subject_id == new_ace->subject_id)
+ goto found_match;
+
+ if (new_ace->perm == 0)
+ return 0; /* No permissions to remove. Add deny record? */
+
+ acl = key_alloc_acl(old, 1, -1);
+ if (IS_ERR(acl))
+ return PTR_ERR(acl);
+ acl->aces[i] = *new_ace;
+ goto change;
+
+found_match:
+ if (new_ace->perm == 0)
+ goto delete_ace;
+ if (new_ace->perm == old->aces[i].perm)
+ return 0;
+ acl = key_alloc_acl(old, 0, -1);
+ if (IS_ERR(acl))
+ return PTR_ERR(acl);
+ acl->aces[i].perm = new_ace->perm;
+ goto change;
+
+delete_ace:
+ acl = key_alloc_acl(old, -1, i);
+ if (IS_ERR(acl))
+ return PTR_ERR(acl);
+ goto change;
+
+change:
+ return key_set_acl(key, acl);
+}
+
+/*
+ * Add, alter or remove (if perm == 0) an ACE in a key's ACL.
+ */
+long keyctl_grant_permission(key_serial_t keyid,
+ enum key_ace_subject_type type,
+ unsigned int subject,
+ unsigned int perm)
+{
+ struct key_ace new_ace;
+ struct key *key;
+ key_ref_t key_ref;
+ long ret;
+
+ new_ace.type = type;
+ new_ace.perm = perm;
+
+ switch (type) {
+ case KEY_ACE_SUBJ_STANDARD:
+ if (subject >= nr__key_ace_standard_subject)
+ return -ENOENT;
+ new_ace.subject_id = subject;
+ break;
+
+ default:
+ return -ENOENT;
+ }
+
+ key_ref = lookup_user_key(keyid, KEY_LOOKUP_PARTIAL, KEY_NEED_CHANGE_ACL);
+ if (IS_ERR(key_ref)) {
+ ret = PTR_ERR(key_ref);
+ goto error;
+ }
+
+ key = key_ref_to_ptr(key_ref);
+
+ down_write(&key->sem);
+
+ /* If we're not the sysadmin, we can only change a key that we own */
+ ret = -EACCES;
+ if (capable(CAP_SYS_ADMIN) || uid_eq(key->uid, current_fsuid()))
+ ret = key_change_acl(key, &new_ace);
+
+ up_write(&key->sem);
+ key_put(key);
+error:
+ return ret;
+}
^ permalink raw reply related
* [RFC PATCH 1/5] keys: Move permissions checking decisions into the checking code
From: David Howells @ 2020-07-16 20:34 UTC (permalink / raw)
To: Stephen Smalley, Casey Schaufler
Cc: Jarkko Sakkinen, Paul Moore, keyrings, selinux, dhowells,
Jarkko Sakkinen, Eric Biederman, jlayton, christian, selinux,
linux-afs, linux-nfs, linux-cifs, linux-api, linux-fsdevel,
linux-security-module, linux-kernel, containers
In-Reply-To: <159493167778.3249370.8145886688150701997.stgit@warthog.procyon.org.uk>
Overhaul the permissions checking, moving the decisions of which permits to
request for what operation and what overrides to allow into the permissions
checking functions in keyrings, SELinux and Smack.
To this end, the KEY_NEED_* constants are turned into an enum and expanded
in number to cover operation types individually.
Note that some more tweaking is probably needed to separate kernel uses,
e.g. AFS using rxrpc keys, from direct userspace users.
Some overrides are available and this needs to be handled specially:
(1) KEY_FLAG_KEEP in key->flags - The key may not be deleted and/or things
may not be removed from the keyring. This can only be set inside the
kernel. It's used to protect the blacklist keyring and the keys in
it, for example.
(2) KEY_FLAG_ROOT_CAN_CLEAR in key->flags - The keyring can be cleared by
CAP_SYS_ADMIN. This can only be set by the kernel and is used to
allow the system admin to manually clear a keyring that is used for
caching network filesystem information (eg. DNS) upcall results.
(3) KEY_FLAG_ROOT_CAN_INVAL in key->flags - The key can be invalidated by
CAP_SYS_ADMIN. This can only be set by the kernel and is used to
allow the system admin to manually invalidate a key that is caching
the result of a netfs information upcall (eg. DNS).
(4) An appropriate auth token being set in cred->request_key_auth that
gives a process transient permission to view and instantiate a key.
This is used by the kernel to delegate instantiation to userspace. It
can only be automatically generated by request_key() to allow an
upcalled instantiator program access to the key to be instantiated and
the keyrings of the process that called request_key().
Possibly an additional permission check should be imposed to allow the
upcall process to actually access the calling process's keyrings.
Note that this requires some tweaks to the testsuite as some of the error
codes change:
(*) KEYCTL_DH_COMPUTE now passes the error from lookup_user_key() back
rather than replacing it unconditionally with ENOKEY. This means that
permission and other errors are now correctly seen (including now
getting EINVAL for a key ID of 0 - which is never valid).
(*) KEYCTL_READ now sees EINVAL rather than ENOKEY for a key ID of 0.
Internally, this is because the code trying to work out whether a key
can be read can now be moved into the permissions checking code - where
it's easier to evaluate - and lookup_user_key() can be called without
deferral of the permission check.
Signed-off-by: David Howells <dhowells@redhat.com>
Reported-by: Stephen Smalley <stephen.smalley.work@gmail.com>
cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
cc: Paul Moore <paul@paul-moore.com>
cc: Stephen Smalley <stephen.smalley.work@gmail.com>
cc: Casey Schaufler <casey@schaufler-ca.com>
cc: keyrings@vger.kernel.org
cc: selinux@vger.kernel.org
---
include/linux/key.h | 33 ++-
include/linux/lsm_hook_defs.h | 2
include/linux/lsm_hooks.h | 8 +
include/linux/security.h | 9 +
security/keys/dh.c | 7 -
security/keys/internal.h | 13 +
security/keys/key.c | 66 ++++---
security/keys/keyctl.c | 373 ++++++++++++--------------------------
security/keys/keyctl_pkey.c | 17 +-
security/keys/keyring.c | 2
security/keys/permission.c | 143 ++++++++++++---
security/keys/persistent.c | 2
security/keys/proc.c | 2
security/keys/process_keys.c | 21 +-
security/keys/request_key.c | 10 +
security/keys/request_key_auth.c | 7 +
security/security.c | 4
security/selinux/hooks.c | 163 +++++++++++++----
security/smack/smack_lsm.c | 90 +++++++--
19 files changed, 562 insertions(+), 410 deletions(-)
diff --git a/include/linux/key.h b/include/linux/key.h
index 0f2e24f13c2b..5ab146cdeb08 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -75,17 +75,28 @@ struct net;
* The permissions required on a key that we're looking up.
*/
enum key_need_perm {
- KEY_NEED_UNSPECIFIED, /* Needed permission unspecified */
- KEY_NEED_VIEW, /* Require permission to view attributes */
- KEY_NEED_READ, /* Require permission to read content */
- KEY_NEED_WRITE, /* Require permission to update / modify */
- KEY_NEED_SEARCH, /* Require permission to search (keyring) or find (key) */
- KEY_NEED_LINK, /* Require permission to link */
- KEY_NEED_SETATTR, /* Require permission to change attributes */
- KEY_NEED_UNLINK, /* Require permission to unlink key */
- KEY_SYSADMIN_OVERRIDE, /* Special: override by CAP_SYS_ADMIN */
- KEY_AUTHTOKEN_OVERRIDE, /* Special: override by possession of auth token */
- KEY_DEFER_PERM_CHECK, /* Special: permission check is deferred */
+ KEY_NEED_UNSPECIFIED, /* Needed permission unspecified */
+ KEY_NEED_ASSUME_AUTHORITY, /* Want to assume instantiation authority */
+ KEY_NEED_CHOWN, /* Want to change key's ownership/group */
+ KEY_NEED_DESCRIBE, /* Want to get a key's attributes */
+ KEY_NEED_GET_SECURITY, /* Want to get a key's security label */
+ KEY_NEED_INSTANTIATE, /* Want to instantiate a key */
+ KEY_NEED_INVALIDATE, /* Want to invalidate key */
+ KEY_NEED_JOIN, /* Want to set a keyring as the session keyring */
+ KEY_NEED_KEYRING_ADD, /* Want to add a link to a keyring */
+ KEY_NEED_KEYRING_CLEAR, /* Want to clear a keyring */
+ KEY_NEED_KEYRING_DELETE, /* Want to remove a link from a keyring */
+ KEY_NEED_LINK, /* Want to create a link to a key */
+ KEY_NEED_READ, /* Want to read content to userspace */
+ KEY_NEED_REVOKE, /* Want to revoke a key */
+ KEY_NEED_SEARCH, /* Want to find a key in a search */
+ KEY_NEED_SETPERM, /* Want to set the permissions mask */
+ KEY_NEED_SET_RESTRICTION, /* Want to set a restriction on a keyring */
+ KEY_NEED_SET_TIMEOUT, /* Want to set the expiration time on a key */
+ KEY_NEED_UNLINK, /* Want to remove a link from a key */
+ KEY_NEED_UPDATE, /* Want to update a key's payload */
+ KEY_NEED_USE, /* Want to use a key (in kernel) */
+ KEY_NEED_WATCH, /* Want to watch a key for events */
};
struct seq_file;
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index af998f93d256..000f8db4706d 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -360,7 +360,7 @@ LSM_HOOK(int, 0, key_alloc, struct key *key, const struct cred *cred,
unsigned long flags)
LSM_HOOK(void, LSM_RET_VOID, key_free, struct key *key)
LSM_HOOK(int, 0, key_permission, key_ref_t key_ref, const struct cred *cred,
- enum key_need_perm need_perm)
+ enum key_need_perm need_perm, unsigned int flags)
LSM_HOOK(int, 0, key_getsecurity, struct key *key, char **_buffer)
#endif /* CONFIG_KEYS */
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 95b7c1d32062..f1130bd699bc 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1119,6 +1119,14 @@
* @cred points to the credentials to provide the context against which to
* evaluate the security data on the key.
* @perm describes the combination of permissions required of this key.
+ * @flags indicates any special conditions set in the normal checks, such
+ * as:
+ * KEY_PERMISSION_USED_AUTH_OVERRIDE - A lack of permission was
+ * overridden by the presence of an instantiation authorisation
+ * token.
+ * KEY_PERMISSION_USED_SYSADMIN_OVERRIDE - A lack of permission was
+ * overridden by the presence of a KEY_FLAG_ROOT_CAN_xxx flag on
+ * the key an the success of a CAP_SYS_ADMIN check.
* Return 0 if permission is granted, -ve error otherwise.
* @key_getsecurity:
* Get a textual representation of the security context attached to a key
diff --git a/include/linux/security.h b/include/linux/security.h
index 0a0a03b36a3b..4f51e3aa2440 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1778,13 +1778,17 @@ static inline int security_path_chroot(const struct path *path)
}
#endif /* CONFIG_SECURITY_PATH */
+/* Flags for security_key_permission() */
+#define KEY_PERMISSION_USED_AUTH_OVERRIDE 0x01 /* Auth token overrode lack of permission */
+#define KEY_PERMISSION_USED_SYSADMIN_OVERRIDE 0x02 /* Sysadmin overrode lack of permission */
+
#ifdef CONFIG_KEYS
#ifdef CONFIG_SECURITY
int security_key_alloc(struct key *key, const struct cred *cred, unsigned long flags);
void security_key_free(struct key *key);
int security_key_permission(key_ref_t key_ref, const struct cred *cred,
- enum key_need_perm need_perm);
+ enum key_need_perm need_perm, unsigned int flags);
int security_key_getsecurity(struct key *key, char **_buffer);
#else
@@ -1802,7 +1806,8 @@ static inline void security_key_free(struct key *key)
static inline int security_key_permission(key_ref_t key_ref,
const struct cred *cred,
- enum key_need_perm need_perm)
+ enum key_need_perm need_perm,
+ unsigned int flags)
{
return 0;
}
diff --git a/security/keys/dh.c b/security/keys/dh.c
index c4c629bb1c03..e43731d22310 100644
--- a/security/keys/dh.c
+++ b/security/keys/dh.c
@@ -22,10 +22,8 @@ static ssize_t dh_data_from_key(key_serial_t keyid, void **data)
ssize_t ret;
key_ref = lookup_user_key(keyid, 0, KEY_NEED_READ);
- if (IS_ERR(key_ref)) {
- ret = -ENOKEY;
- goto error;
- }
+ if (IS_ERR(key_ref))
+ return PTR_ERR(key_ref);
key = key_ref_to_ptr(key_ref);
@@ -52,7 +50,6 @@ static ssize_t dh_data_from_key(key_serial_t keyid, void **data)
}
key_put(key);
-error:
return ret;
}
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 338a526cbfa5..68faf0f0bda8 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -108,6 +108,14 @@ extern void __key_link_end(struct key *keyring,
extern key_ref_t find_key_to_update(key_ref_t keyring_ref,
const struct keyring_index_key *index_key);
+extern key_ref_t key_create_or_update_perm_checked(key_ref_t keyring_ref,
+ const char *type,
+ const char *description,
+ const void *payload,
+ size_t plen,
+ key_perm_t perm,
+ unsigned long flags);
+extern int key_update_perm_checked(key_ref_t key_ref, const void *payload, size_t plen);
extern struct key *keyring_search_instkey(struct key *keyring,
key_serial_t target_id);
@@ -165,8 +173,9 @@ extern struct key *request_key_and_link(struct key_type *type,
extern bool lookup_user_key_possessed(const struct key *key,
const struct key_match_data *match_data);
-#define KEY_LOOKUP_CREATE 0x01
-#define KEY_LOOKUP_PARTIAL 0x02
+#define KEY_LOOKUP_CREATE 0x01
+#define KEY_LOOKUP_PARTIAL 0x02
+#define KEY_LOOKUP_AUTH_OVERRIDE 0x04
extern long join_session_keyring(const char *name);
extern void key_change_session_keyring(struct callback_head *twork);
diff --git a/security/keys/key.c b/security/keys/key.c
index e282c6179b21..6b12eae4e612 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -755,7 +755,7 @@ static inline key_ref_t __key_update(key_ref_t key_ref,
int ret;
/* need write permission on the key to update it */
- ret = key_permission(key_ref, KEY_NEED_WRITE);
+ ret = key_permission(key_ref, KEY_NEED_UPDATE);
if (ret < 0)
goto error;
@@ -810,13 +810,13 @@ static inline key_ref_t __key_update(key_ref_t key_ref,
* On success, the possession flag from the keyring ref will be tacked on to
* the key ref before it is returned.
*/
-key_ref_t key_create_or_update(key_ref_t keyring_ref,
- const char *type,
- const char *description,
- const void *payload,
- size_t plen,
- key_perm_t perm,
- unsigned long flags)
+key_ref_t key_create_or_update_perm_checked(key_ref_t keyring_ref,
+ const char *type,
+ const char *description,
+ const void *payload,
+ size_t plen,
+ key_perm_t perm,
+ unsigned long flags)
{
struct keyring_index_key index_key = {
.description = description,
@@ -894,14 +894,6 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
}
}
- /* if we're going to allocate a new key, we're going to have
- * to modify the keyring */
- ret = key_permission(keyring_ref, KEY_NEED_WRITE);
- if (ret < 0) {
- key_ref = ERR_PTR(ret);
- goto error_link_end;
- }
-
/* if it's possible to update this type of key, search for an existing
* key of the same type and description in the destination keyring and
* update that instead if possible
@@ -981,6 +973,25 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
goto error_free_prep;
}
+
+key_ref_t key_create_or_update(key_ref_t keyring_ref,
+ const char *type,
+ const char *description,
+ const void *payload,
+ size_t plen,
+ key_perm_t perm,
+ unsigned long flags)
+{
+ int ret;
+
+ ret = key_permission(keyring_ref, KEY_NEED_KEYRING_ADD);
+ if (ret < 0)
+ return ERR_PTR(ret);
+
+ return key_create_or_update_perm_checked(keyring_ref, type,
+ description, payload,
+ plen, perm, flags);
+}
EXPORT_SYMBOL(key_create_or_update);
/**
@@ -996,19 +1007,12 @@ EXPORT_SYMBOL(key_create_or_update);
* Returns 0 on success, -EACCES if not permitted and -EOPNOTSUPP if the key
* type does not support updating. The key type may return other errors.
*/
-int key_update(key_ref_t key_ref, const void *payload, size_t plen)
+int key_update_perm_checked(key_ref_t key_ref, const void *payload, size_t plen)
{
struct key_preparsed_payload prep;
struct key *key = key_ref_to_ptr(key_ref);
int ret;
- key_check(key);
-
- /* the key must be writable */
- ret = key_permission(key_ref, KEY_NEED_WRITE);
- if (ret < 0)
- return ret;
-
/* attempt to update it if supported */
if (!key->type->update)
return -EOPNOTSUPP;
@@ -1040,6 +1044,20 @@ int key_update(key_ref_t key_ref, const void *payload, size_t plen)
key->type->free_preparse(&prep);
return ret;
}
+
+int key_update(key_ref_t key_ref, const void *payload, size_t plen)
+{
+ int ret;
+
+ key_check(key_ref_to_ptr(key_ref));
+
+ /* the key must be writable */
+ ret = key_permission(key_ref, KEY_NEED_UPDATE);
+ if (ret < 0)
+ return ret;
+
+ return key_update_perm_checked(key_ref, payload, plen);
+}
EXPORT_SYMBOL(key_update);
/**
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 9febd37a168f..b75326e15a96 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -123,7 +123,8 @@ SYSCALL_DEFINE5(add_key, const char __user *, _type,
}
/* find the target keyring (which must be writable) */
- keyring_ref = lookup_user_key(ringid, KEY_LOOKUP_CREATE, KEY_NEED_WRITE);
+ keyring_ref = lookup_user_key(ringid, KEY_LOOKUP_CREATE,
+ KEY_NEED_KEYRING_ADD);
if (IS_ERR(keyring_ref)) {
ret = PTR_ERR(keyring_ref);
goto error3;
@@ -131,9 +132,9 @@ SYSCALL_DEFINE5(add_key, const char __user *, _type,
/* create or update the requested key and add it to the target
* keyring */
- key_ref = key_create_or_update(keyring_ref, type, description,
- payload, plen, KEY_PERM_UNDEF,
- KEY_ALLOC_IN_QUOTA);
+ key_ref = key_create_or_update_perm_checked(keyring_ref, type, description,
+ payload, plen, KEY_PERM_UNDEF,
+ KEY_ALLOC_IN_QUOTA);
if (!IS_ERR(key_ref)) {
ret = key_ref_to_ptr(key_ref)->serial;
key_ref_put(key_ref);
@@ -204,7 +205,7 @@ SYSCALL_DEFINE4(request_key, const char __user *, _type,
dest_ref = NULL;
if (destringid) {
dest_ref = lookup_user_key(destringid, KEY_LOOKUP_CREATE,
- KEY_NEED_WRITE);
+ KEY_NEED_KEYRING_ADD);
if (IS_ERR(dest_ref)) {
ret = PTR_ERR(dest_ref);
goto error3;
@@ -348,14 +349,14 @@ long keyctl_update_key(key_serial_t id,
}
/* find the target key (which must be writable) */
- key_ref = lookup_user_key(id, 0, KEY_NEED_WRITE);
+ key_ref = lookup_user_key(id, 0, KEY_NEED_UPDATE);
if (IS_ERR(key_ref)) {
ret = PTR_ERR(key_ref);
goto error2;
}
/* update the key */
- ret = key_update(key_ref, payload, plen);
+ ret = key_update_perm_checked(key_ref, payload, plen);
key_ref_put(key_ref);
error2:
@@ -379,31 +380,14 @@ long keyctl_update_key(key_serial_t id,
long keyctl_revoke_key(key_serial_t id)
{
key_ref_t key_ref;
- struct key *key;
- long ret;
-
- key_ref = lookup_user_key(id, 0, KEY_NEED_WRITE);
- if (IS_ERR(key_ref)) {
- ret = PTR_ERR(key_ref);
- if (ret != -EACCES)
- goto error;
- key_ref = lookup_user_key(id, 0, KEY_NEED_SETATTR);
- if (IS_ERR(key_ref)) {
- ret = PTR_ERR(key_ref);
- goto error;
- }
- }
- key = key_ref_to_ptr(key_ref);
- ret = 0;
- if (test_bit(KEY_FLAG_KEEP, &key->flags))
- ret = -EPERM;
- else
- key_revoke(key);
+ key_ref = lookup_user_key(id, 0, KEY_NEED_REVOKE);
+ if (IS_ERR(key_ref))
+ return PTR_ERR(key_ref);
+ key_revoke(key_ref_to_ptr(key_ref));
key_ref_put(key_ref);
-error:
- return ret;
+ return 0;
}
/*
@@ -420,41 +404,16 @@ long keyctl_revoke_key(key_serial_t id)
long keyctl_invalidate_key(key_serial_t id)
{
key_ref_t key_ref;
- struct key *key;
- long ret;
kenter("%d", id);
- key_ref = lookup_user_key(id, 0, KEY_NEED_SEARCH);
- if (IS_ERR(key_ref)) {
- ret = PTR_ERR(key_ref);
-
- /* Root is permitted to invalidate certain special keys */
- if (capable(CAP_SYS_ADMIN)) {
- key_ref = lookup_user_key(id, 0, KEY_SYSADMIN_OVERRIDE);
- if (IS_ERR(key_ref))
- goto error;
- if (test_bit(KEY_FLAG_ROOT_CAN_INVAL,
- &key_ref_to_ptr(key_ref)->flags))
- goto invalidate;
- goto error_put;
- }
-
- goto error;
- }
+ key_ref = lookup_user_key(id, 0, KEY_NEED_INVALIDATE);
+ if (IS_ERR(key_ref))
+ return PTR_ERR(key_ref);
-invalidate:
- key = key_ref_to_ptr(key_ref);
- ret = 0;
- if (test_bit(KEY_FLAG_KEEP, &key->flags))
- ret = -EPERM;
- else
- key_invalidate(key);
-error_put:
+ key_invalidate(key_ref_to_ptr(key_ref));
key_ref_put(key_ref);
-error:
- kleave(" = %ld", ret);
- return ret;
+ return 0;
}
/*
@@ -467,37 +426,15 @@ long keyctl_invalidate_key(key_serial_t id)
long keyctl_keyring_clear(key_serial_t ringid)
{
key_ref_t keyring_ref;
- struct key *keyring;
long ret;
- keyring_ref = lookup_user_key(ringid, KEY_LOOKUP_CREATE, KEY_NEED_WRITE);
- if (IS_ERR(keyring_ref)) {
- ret = PTR_ERR(keyring_ref);
+ keyring_ref = lookup_user_key(ringid, KEY_LOOKUP_CREATE,
+ KEY_NEED_KEYRING_CLEAR);
+ if (IS_ERR(keyring_ref))
+ return PTR_ERR(keyring_ref);
- /* Root is permitted to invalidate certain special keyrings */
- if (capable(CAP_SYS_ADMIN)) {
- keyring_ref = lookup_user_key(ringid, 0,
- KEY_SYSADMIN_OVERRIDE);
- if (IS_ERR(keyring_ref))
- goto error;
- if (test_bit(KEY_FLAG_ROOT_CAN_CLEAR,
- &key_ref_to_ptr(keyring_ref)->flags))
- goto clear;
- goto error_put;
- }
-
- goto error;
- }
-
-clear:
- keyring = key_ref_to_ptr(keyring_ref);
- if (test_bit(KEY_FLAG_KEEP, &keyring->flags))
- ret = -EPERM;
- else
- ret = keyring_clear(keyring);
-error_put:
+ ret = keyring_clear(key_ref_to_ptr(keyring_ref));
key_ref_put(keyring_ref);
-error:
return ret;
}
@@ -517,7 +454,8 @@ long keyctl_keyring_link(key_serial_t id, key_serial_t ringid)
key_ref_t keyring_ref, key_ref;
long ret;
- keyring_ref = lookup_user_key(ringid, KEY_LOOKUP_CREATE, KEY_NEED_WRITE);
+ keyring_ref = lookup_user_key(ringid, KEY_LOOKUP_CREATE,
+ KEY_NEED_KEYRING_ADD);
if (IS_ERR(keyring_ref)) {
ret = PTR_ERR(keyring_ref);
goto error;
@@ -552,10 +490,9 @@ long keyctl_keyring_link(key_serial_t id, key_serial_t ringid)
long keyctl_keyring_unlink(key_serial_t id, key_serial_t ringid)
{
key_ref_t keyring_ref, key_ref;
- struct key *keyring, *key;
long ret;
- keyring_ref = lookup_user_key(ringid, 0, KEY_NEED_WRITE);
+ keyring_ref = lookup_user_key(ringid, 0, KEY_NEED_KEYRING_DELETE);
if (IS_ERR(keyring_ref)) {
ret = PTR_ERR(keyring_ref);
goto error;
@@ -567,13 +504,7 @@ long keyctl_keyring_unlink(key_serial_t id, key_serial_t ringid)
goto error2;
}
- keyring = key_ref_to_ptr(keyring_ref);
- key = key_ref_to_ptr(key_ref);
- if (test_bit(KEY_FLAG_KEEP, &keyring->flags) &&
- test_bit(KEY_FLAG_KEEP, &key->flags))
- ret = -EPERM;
- else
- ret = key_unlink(keyring, key);
+ ret = key_unlink(key_ref_to_ptr(keyring_ref), key_ref_to_ptr(key_ref));
key_ref_put(key_ref);
error2:
@@ -605,13 +536,13 @@ long keyctl_keyring_move(key_serial_t id, key_serial_t from_ringid,
if (IS_ERR(key_ref))
return PTR_ERR(key_ref);
- from_ref = lookup_user_key(from_ringid, 0, KEY_NEED_WRITE);
+ from_ref = lookup_user_key(from_ringid, 0, KEY_NEED_KEYRING_DELETE);
if (IS_ERR(from_ref)) {
ret = PTR_ERR(from_ref);
goto error2;
}
- to_ref = lookup_user_key(to_ringid, KEY_LOOKUP_CREATE, KEY_NEED_WRITE);
+ to_ref = lookup_user_key(to_ringid, KEY_LOOKUP_CREATE, KEY_NEED_KEYRING_ADD);
if (IS_ERR(to_ref)) {
ret = PTR_ERR(to_ref);
goto error3;
@@ -645,33 +576,21 @@ long keyctl_describe_key(key_serial_t keyid,
char __user *buffer,
size_t buflen)
{
- struct key *key, *instkey;
+ struct key *key;
key_ref_t key_ref;
char *infobuf;
long ret;
int desclen, infolen;
- key_ref = lookup_user_key(keyid, KEY_LOOKUP_PARTIAL, KEY_NEED_VIEW);
- if (IS_ERR(key_ref)) {
- /* viewing a key under construction is permitted if we have the
- * authorisation token handy */
- if (PTR_ERR(key_ref) == -EACCES) {
- instkey = key_get_instantiation_authkey(keyid);
- if (!IS_ERR(instkey)) {
- key_put(instkey);
- key_ref = lookup_user_key(keyid,
- KEY_LOOKUP_PARTIAL,
- KEY_AUTHTOKEN_OVERRIDE);
- if (!IS_ERR(key_ref))
- goto okay;
- }
- }
-
- ret = PTR_ERR(key_ref);
- goto error;
- }
+ /* Viewing a key under construction is permitted if we have the
+ * authorisation token handy.
+ */
+ key_ref = lookup_user_key(keyid,
+ KEY_LOOKUP_PARTIAL | KEY_LOOKUP_AUTH_OVERRIDE,
+ KEY_NEED_DESCRIBE);
+ if (IS_ERR(key_ref))
+ return PTR_ERR(key_ref);
-okay:
key = key_ref_to_ptr(key_ref);
desclen = strlen(key->description);
@@ -683,23 +602,21 @@ long keyctl_describe_key(key_serial_t keyid,
from_kuid_munged(current_user_ns(), key->uid),
from_kgid_munged(current_user_ns(), key->gid),
key->perm);
- if (!infobuf)
- goto error2;
- infolen = strlen(infobuf);
- ret = infolen + desclen + 1;
-
- /* consider returning the data */
- if (buffer && buflen >= ret) {
- if (copy_to_user(buffer, infobuf, infolen) != 0 ||
- copy_to_user(buffer + infolen, key->description,
- desclen + 1) != 0)
- ret = -EFAULT;
- }
+ if (infobuf) {
+ infolen = strlen(infobuf);
+ ret = infolen + desclen + 1;
+
+ /* consider returning the data */
+ if (buffer && buflen >= ret) {
+ if (copy_to_user(buffer, infobuf, infolen) != 0 ||
+ copy_to_user(buffer + infolen, key->description,
+ desclen + 1) != 0)
+ ret = -EFAULT;
+ }
- kfree(infobuf);
-error2:
+ kfree(infobuf);
+ }
key_ref_put(key_ref);
-error:
return ret;
}
@@ -745,7 +662,7 @@ long keyctl_keyring_search(key_serial_t ringid,
dest_ref = NULL;
if (destringid) {
dest_ref = lookup_user_key(destringid, KEY_LOOKUP_CREATE,
- KEY_NEED_WRITE);
+ KEY_NEED_KEYRING_ADD);
if (IS_ERR(dest_ref)) {
ret = PTR_ERR(dest_ref);
goto error3;
@@ -815,9 +732,6 @@ static long __keyctl_read_key(struct key *key, char *buffer, size_t buflen)
/*
* Read a key's payload.
*
- * The key must either grant the caller Read permission, or it must grant the
- * caller Search permission when searched for from the process keyrings.
- *
* If successful, we place up to buflen bytes of data into the buffer, if one
* is provided, and return the amount of data that is available in the key,
* irrespective of how much we copied into the buffer.
@@ -831,36 +745,11 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
size_t key_data_len;
/* find the key first */
- key_ref = lookup_user_key(keyid, 0, KEY_DEFER_PERM_CHECK);
- if (IS_ERR(key_ref)) {
- ret = -ENOKEY;
- goto out;
- }
+ key_ref = lookup_user_key(keyid, 0, KEY_NEED_READ);
+ if (IS_ERR(key_ref))
+ return PTR_ERR(key_ref);
key = key_ref_to_ptr(key_ref);
-
- ret = key_read_state(key);
- if (ret < 0)
- goto key_put_out; /* Negatively instantiated */
-
- /* see if we can read it directly */
- ret = key_permission(key_ref, KEY_NEED_READ);
- if (ret == 0)
- goto can_read_key;
- if (ret != -EACCES)
- goto key_put_out;
-
- /* we can't; see if it's searchable from this process's keyrings
- * - we automatically take account of the fact that it may be
- * dangling off an instantiation key
- */
- if (!is_key_possessed(key_ref)) {
- ret = -EACCES;
- goto key_put_out;
- }
-
- /* the key is probably readable - now try to read it */
-can_read_key:
if (!key->type->read) {
ret = -EOPNOTSUPP;
goto key_put_out;
@@ -927,18 +816,16 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
key_put_out:
key_put(key);
-out:
return ret;
}
/*
* Change the ownership of a key
*
- * The key must grant the caller Setattr permission for this to work, though
- * the key need not be fully instantiated yet. For the UID to be changed, or
- * for the GID to be changed to a group the caller is not a member of, the
- * caller must have sysadmin capability. If either uid or gid is -1 then that
- * attribute is not changed.
+ * The key need not be fully instantiated for this operation to be applied.
+ * For the UID to be changed, or for the GID to be changed to a group the
+ * caller is not a member of, the caller must have sysadmin capability. If
+ * either uid or gid is -1 then that attribute is not changed.
*
* If the UID is to be changed, the new user must have sufficient quota to
* accept the key. The quota deduction will be removed from the old user to
@@ -968,7 +855,7 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
goto error;
key_ref = lookup_user_key(id, KEY_LOOKUP_CREATE | KEY_LOOKUP_PARTIAL,
- KEY_NEED_SETATTR);
+ KEY_NEED_CHOWN);
if (IS_ERR(key_ref)) {
ret = PTR_ERR(key_ref);
goto error;
@@ -1060,9 +947,9 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
/*
* Change the permission mask on a key.
*
- * The key must grant the caller Setattr permission for this to work, though
- * the key need not be fully instantiated yet. If the caller does not have
- * sysadmin capability, it may only change the permission on keys that it owns.
+ * The key doesn't have to be fully instantiated yet for this to work. If the
+ * caller does not have sysadmin capability, it may only change the permission
+ * on keys that it owns.
*/
long keyctl_setperm_key(key_serial_t id, key_perm_t perm)
{
@@ -1075,7 +962,7 @@ long keyctl_setperm_key(key_serial_t id, key_perm_t perm)
goto error;
key_ref = lookup_user_key(id, KEY_LOOKUP_CREATE | KEY_LOOKUP_PARTIAL,
- KEY_NEED_SETATTR);
+ KEY_NEED_SETPERM);
if (IS_ERR(key_ref)) {
ret = PTR_ERR(key_ref);
goto error;
@@ -1102,7 +989,7 @@ long keyctl_setperm_key(key_serial_t id, key_perm_t perm)
/*
* Get the destination keyring for instantiation and check that the caller has
- * Write permission on it.
+ * permission to add a key to it.
*/
static long get_instantiation_keyring(key_serial_t ringid,
struct request_key_auth *rka,
@@ -1118,7 +1005,8 @@ static long get_instantiation_keyring(key_serial_t ringid,
/* if a specific keyring is nominated by ID, then use that */
if (ringid > 0) {
- dkref = lookup_user_key(ringid, KEY_LOOKUP_CREATE, KEY_NEED_WRITE);
+ dkref = lookup_user_key(ringid, KEY_LOOKUP_CREATE,
+ KEY_NEED_KEYRING_ADD);
if (IS_ERR(dkref))
return PTR_ERR(dkref);
*_dest_keyring = key_ref_to_ptr(dkref);
@@ -1159,7 +1047,7 @@ static int keyctl_change_reqkey_auth(struct key *key)
* Instantiate a key with the specified payload and link the key into the
* destination keyring if one is given.
*
- * The caller must have the appropriate instantiation permit set for this to
+ * The caller must have the appropriate instantiation token set for this to
* work (see keyctl_assume_authority). No other permissions are required.
*
* If successful, 0 will be returned.
@@ -1170,29 +1058,34 @@ long keyctl_instantiate_key_common(key_serial_t id,
{
const struct cred *cred = current_cred();
struct request_key_auth *rka;
- struct key *instkey, *dest_keyring;
+ struct key *key, *instkey, *dest_keyring;
+ key_ref_t kref;
size_t plen = from ? iov_iter_count(from) : 0;
void *payload;
long ret;
kenter("%d,,%zu,%d", id, plen, ringid);
+ if (plen > 1024 * 1024 - 1)
+ return -EINVAL;
+
if (!plen)
from = NULL;
- ret = -EINVAL;
- if (plen > 1024 * 1024 - 1)
- goto error;
-
/* the appropriate instantiation authorisation key must have been
* assumed before calling this */
- ret = -EPERM;
instkey = cred->request_key_auth;
if (!instkey)
- goto error;
+ return -EPERM;
+
+ kref = lookup_user_key(id, KEY_LOOKUP_PARTIAL, KEY_NEED_INSTANTIATE);
+ if (IS_ERR(kref))
+ return PTR_ERR(kref);
+ key = key_ref_to_ptr(kref);
+ ret = -EPERM;
rka = instkey->payload.data[0];
- if (rka->target_key->serial != id)
+ if (rka->target_key != key)
goto error;
/* pull the payload in if one was supplied */
@@ -1216,7 +1109,7 @@ long keyctl_instantiate_key_common(key_serial_t id,
goto error2;
/* instantiate the key and link it into a keyring */
- ret = key_instantiate_and_link(rka->target_key, payload, plen,
+ ret = key_instantiate_and_link(key, payload, plen,
dest_keyring, instkey);
key_put(dest_keyring);
@@ -1229,6 +1122,7 @@ long keyctl_instantiate_key_common(key_serial_t id,
error2:
kvfree_sensitive(payload, plen);
error:
+ key_put(key);
return ret;
}
@@ -1332,7 +1226,8 @@ long keyctl_reject_key(key_serial_t id, unsigned timeout, unsigned error,
{
const struct cred *cred = current_cred();
struct request_key_auth *rka;
- struct key *instkey, *dest_keyring;
+ struct key *key, *instkey, *dest_keyring;
+ key_ref_t kref;
long ret;
kenter("%d,%u,%u,%d", id, timeout, error, ringid);
@@ -1348,13 +1243,18 @@ long keyctl_reject_key(key_serial_t id, unsigned timeout, unsigned error,
/* the appropriate instantiation authorisation key must have been
* assumed before calling this */
- ret = -EPERM;
instkey = cred->request_key_auth;
if (!instkey)
- goto error;
+ return -EPERM;
+
+ kref = lookup_user_key(id, KEY_LOOKUP_PARTIAL, KEY_NEED_INSTANTIATE);
+ if (IS_ERR(kref))
+ return PTR_ERR(kref);
+ key = key_ref_to_ptr(kref);
+ ret = -EPERM;
rka = instkey->payload.data[0];
- if (rka->target_key->serial != id)
+ if (rka->target_key != key)
goto error;
/* find the destination keyring if present (which must also be
@@ -1375,6 +1275,7 @@ long keyctl_reject_key(key_serial_t id, unsigned timeout, unsigned error,
keyctl_change_reqkey_auth(NULL);
error:
+ key_put(key);
return ret;
}
@@ -1438,8 +1339,8 @@ long keyctl_set_reqkey_keyring(int reqkey_defl)
/*
* Set or clear the timeout on a key.
*
- * Either the key must grant the caller Setattr permission or else the caller
- * must hold an instantiation authorisation token for the key.
+ * Either the key must grant the caller permission or else the caller must hold
+ * an instantiation authorisation token for the key.
*
* The timeout is either 0 to clear the timeout, or a number of seconds from
* the current time. The key and any links to the key will be automatically
@@ -1451,44 +1352,25 @@ long keyctl_set_reqkey_keyring(int reqkey_defl)
*/
long keyctl_set_timeout(key_serial_t id, unsigned timeout)
{
- struct key *key, *instkey;
+ struct key *key;
key_ref_t key_ref;
- long ret;
-
- key_ref = lookup_user_key(id, KEY_LOOKUP_CREATE | KEY_LOOKUP_PARTIAL,
- KEY_NEED_SETATTR);
- if (IS_ERR(key_ref)) {
- /* setting the timeout on a key under construction is permitted
- * if we have the authorisation token handy */
- if (PTR_ERR(key_ref) == -EACCES) {
- instkey = key_get_instantiation_authkey(id);
- if (!IS_ERR(instkey)) {
- key_put(instkey);
- key_ref = lookup_user_key(id,
- KEY_LOOKUP_PARTIAL,
- KEY_AUTHTOKEN_OVERRIDE);
- if (!IS_ERR(key_ref))
- goto okay;
- }
- }
- ret = PTR_ERR(key_ref);
- goto error;
- }
+ /* Setting the timeout on a key under construction is permitted if we
+ * have the authorisation token handy
+ */
+ key_ref = lookup_user_key(id,
+ KEY_LOOKUP_CREATE |
+ KEY_LOOKUP_PARTIAL |
+ KEY_LOOKUP_AUTH_OVERRIDE,
+ KEY_NEED_SET_TIMEOUT);
+ if (IS_ERR(key_ref))
+ return PTR_ERR(key_ref);
-okay:
key = key_ref_to_ptr(key_ref);
- ret = 0;
- if (test_bit(KEY_FLAG_KEEP, &key->flags)) {
- ret = -EPERM;
- } else {
- key_set_timeout(key, timeout);
- notify_key(key, NOTIFY_KEY_SETATTR, 0);
- }
+ key_set_timeout(key, timeout);
+ notify_key(key, NOTIFY_KEY_SETATTR, 0);
key_put(key);
-
-error:
- return ret;
+ return 0;
}
/*
@@ -1557,28 +1439,17 @@ long keyctl_get_security(key_serial_t keyid,
char __user *buffer,
size_t buflen)
{
- struct key *key, *instkey;
+ struct key *key;
key_ref_t key_ref;
char *context;
long ret;
- key_ref = lookup_user_key(keyid, KEY_LOOKUP_PARTIAL, KEY_NEED_VIEW);
- if (IS_ERR(key_ref)) {
- if (PTR_ERR(key_ref) != -EACCES)
- return PTR_ERR(key_ref);
-
- /* viewing a key under construction is also permitted if we
- * have the authorisation token handy */
- instkey = key_get_instantiation_authkey(keyid);
- if (IS_ERR(instkey))
- return PTR_ERR(instkey);
- key_put(instkey);
-
- key_ref = lookup_user_key(keyid, KEY_LOOKUP_PARTIAL,
- KEY_AUTHTOKEN_OVERRIDE);
- if (IS_ERR(key_ref))
- return PTR_ERR(key_ref);
- }
+ key_ref = lookup_user_key(keyid,
+ KEY_LOOKUP_PARTIAL |
+ KEY_LOOKUP_AUTH_OVERRIDE,
+ KEY_NEED_GET_SECURITY);
+ if (IS_ERR(key_ref))
+ return PTR_ERR(key_ref);
key = key_ref_to_ptr(key_ref);
ret = security_key_getsecurity(key, &context);
@@ -1610,8 +1481,8 @@ long keyctl_get_security(key_serial_t keyid,
* Attempt to install the calling process's session keyring on the process's
* parent process.
*
- * The keyring must exist and must grant the caller LINK permission, and the
- * parent process must be single-threaded and must have the same effective
+ * The keyring must exist and must grant the caller permission to join it, and
+ * the parent process must be single-threaded and must have the same effective
* ownership as this process and mustn't be SUID/SGID.
*
* The keyring will be emplaced on the parent when it next resumes userspace.
@@ -1627,7 +1498,7 @@ long keyctl_session_to_parent(void)
struct cred *cred;
int ret;
- keyring_r = lookup_user_key(KEY_SPEC_SESSION_KEYRING, 0, KEY_NEED_LINK);
+ keyring_r = lookup_user_key(KEY_SPEC_SESSION_KEYRING, 0, KEY_NEED_JOIN);
if (IS_ERR(keyring_r))
return PTR_ERR(keyring_r);
@@ -1729,7 +1600,7 @@ long keyctl_restrict_keyring(key_serial_t id, const char __user *_type,
char *restriction = NULL;
long ret;
- key_ref = lookup_user_key(id, 0, KEY_NEED_SETATTR);
+ key_ref = lookup_user_key(id, 0, KEY_NEED_SET_RESTRICTION);
if (IS_ERR(key_ref))
return PTR_ERR(key_ref);
@@ -1777,7 +1648,7 @@ long keyctl_watch_key(key_serial_t id, int watch_queue_fd, int watch_id)
if (watch_id < -1 || watch_id > 0xff)
return -EINVAL;
- key_ref = lookup_user_key(id, KEY_LOOKUP_CREATE, KEY_NEED_VIEW);
+ key_ref = lookup_user_key(id, KEY_LOOKUP_CREATE, KEY_NEED_WATCH);
if (IS_ERR(key_ref))
return PTR_ERR(key_ref);
key = key_ref_to_ptr(key_ref);
diff --git a/security/keys/keyctl_pkey.c b/security/keys/keyctl_pkey.c
index 931d8dfb4a7f..aece0651eeae 100644
--- a/security/keys/keyctl_pkey.c
+++ b/security/keys/keyctl_pkey.c
@@ -77,7 +77,8 @@ static int keyctl_pkey_params_parse(struct kernel_pkey_params *params)
*/
static int keyctl_pkey_params_get(key_serial_t id,
const char __user *_info,
- struct kernel_pkey_params *params)
+ struct kernel_pkey_params *params,
+ enum key_need_perm need_perm)
{
key_ref_t key_ref;
void *p;
@@ -95,7 +96,7 @@ static int keyctl_pkey_params_get(key_serial_t id,
if (ret < 0)
return ret;
- key_ref = lookup_user_key(id, 0, KEY_NEED_SEARCH);
+ key_ref = lookup_user_key(id, 0, need_perm);
if (IS_ERR(key_ref))
return PTR_ERR(key_ref);
params->key = key_ref_to_ptr(key_ref);
@@ -113,7 +114,8 @@ static int keyctl_pkey_params_get(key_serial_t id,
static int keyctl_pkey_params_get_2(const struct keyctl_pkey_params __user *_params,
const char __user *_info,
int op,
- struct kernel_pkey_params *params)
+ struct kernel_pkey_params *params,
+ enum key_need_perm need_perm)
{
struct keyctl_pkey_params uparams;
struct kernel_pkey_query info;
@@ -125,7 +127,7 @@ static int keyctl_pkey_params_get_2(const struct keyctl_pkey_params __user *_par
if (copy_from_user(&uparams, _params, sizeof(uparams)) != 0)
return -EFAULT;
- ret = keyctl_pkey_params_get(uparams.key_id, _info, params);
+ ret = keyctl_pkey_params_get(uparams.key_id, _info, params, need_perm);
if (ret < 0)
return ret;
@@ -168,7 +170,7 @@ long keyctl_pkey_query(key_serial_t id,
memset(¶ms, 0, sizeof(params));
- ret = keyctl_pkey_params_get(id, _info, ¶ms);
+ ret = keyctl_pkey_params_get(id, _info, ¶ms, KEY_NEED_DESCRIBE);
if (ret < 0)
goto error;
@@ -213,7 +215,8 @@ long keyctl_pkey_e_d_s(int op,
void *in, *out;
long ret;
- ret = keyctl_pkey_params_get_2(_params, _info, op, ¶ms);
+ ret = keyctl_pkey_params_get_2(_params, _info, op, ¶ms,
+ KEY_NEED_USE);
if (ret < 0)
goto error_params;
@@ -289,7 +292,7 @@ long keyctl_pkey_verify(const struct keyctl_pkey_params __user *_params,
long ret;
ret = keyctl_pkey_params_get_2(_params, _info, KEYCTL_PKEY_VERIFY,
- ¶ms);
+ ¶ms, KEY_NEED_USE);
if (ret < 0)
goto error_params;
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 14abfe765b7e..6199efbe19b4 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -1167,7 +1167,7 @@ struct key *find_keyring_by_name(const char *name, bool uid_keyring)
continue;
} else {
if (key_permission(make_key_ref(keyring, 0),
- KEY_NEED_SEARCH) < 0)
+ KEY_NEED_JOIN) < 0)
continue;
}
diff --git a/security/keys/permission.c b/security/keys/permission.c
index 4a61f804e80f..2c06fb92d9bd 100644
--- a/security/keys/permission.c
+++ b/security/keys/permission.c
@@ -7,8 +7,118 @@
#include <linux/export.h>
#include <linux/security.h>
+#include <keys/request_key_auth-type.h>
#include "internal.h"
+/*
+ * Determine if we have sufficient permission to perform an operation.
+ */
+static int check_key_permission(struct key *key, const struct cred *cred,
+ key_perm_t perms, enum key_need_perm need_perm,
+ unsigned int *_notes)
+{
+ struct request_key_auth *rka;
+
+ switch (need_perm) {
+ case KEY_NEED_ASSUME_AUTHORITY:
+ return 0;
+
+ case KEY_NEED_DESCRIBE:
+ case KEY_NEED_GET_SECURITY:
+ if (perms & KEY_OTH_VIEW)
+ return 0;
+ goto check_auth_override;
+
+ case KEY_NEED_CHOWN:
+ case KEY_NEED_SETPERM:
+ case KEY_NEED_SET_RESTRICTION:
+ return perms & KEY_OTH_SETATTR ? 0 : -EACCES;
+
+ case KEY_NEED_INSTANTIATE:
+ goto check_auth_override;
+
+ case KEY_NEED_INVALIDATE:
+ if (test_bit(KEY_FLAG_KEEP, &key->flags))
+ return -EPERM;
+ if (perms & KEY_OTH_SEARCH)
+ return 0;
+ if (test_bit(KEY_FLAG_ROOT_CAN_INVAL, &key->flags))
+ goto check_sysadmin_override;
+ return -EACCES;
+
+ case KEY_NEED_JOIN:
+ case KEY_NEED_LINK:
+ return perms & KEY_OTH_LINK ? 0 : -EACCES;
+
+ case KEY_NEED_KEYRING_DELETE:
+ if (test_bit(KEY_FLAG_KEEP, &key->flags))
+ return -EPERM;
+ /* Fall through. */
+ case KEY_NEED_KEYRING_ADD:
+ return perms & KEY_OTH_WRITE ? 0 : -EACCES;
+
+ case KEY_NEED_KEYRING_CLEAR:
+ if (test_bit(KEY_FLAG_KEEP, &key->flags))
+ return -EPERM;
+ if (perms & KEY_OTH_WRITE)
+ return 0;
+ if (test_bit(KEY_FLAG_ROOT_CAN_CLEAR, &key->flags))
+ goto check_sysadmin_override;
+ return -EACCES;
+
+ case KEY_NEED_READ:
+ return perms & (KEY_OTH_READ | KEY_OTH_SEARCH) ? 0 : -EACCES;
+
+ case KEY_NEED_REVOKE:
+ if (test_bit(KEY_FLAG_KEEP, &key->flags))
+ return -EPERM;
+ return perms & (KEY_OTH_WRITE | KEY_OTH_SETATTR) ? 0 : -EACCES;
+
+ case KEY_NEED_SEARCH:
+ return perms & KEY_OTH_SEARCH ? 0 : -EACCES;
+
+ case KEY_NEED_SET_TIMEOUT:
+ if (test_bit(KEY_FLAG_KEEP, &key->flags))
+ return -EPERM;
+ if (perms & KEY_OTH_SETATTR)
+ return 0;
+ goto check_auth_override;
+
+ case KEY_NEED_UNLINK:
+ if (test_bit(KEY_FLAG_KEEP, &key->flags))
+ return -EPERM;
+ return 0;
+
+ case KEY_NEED_UPDATE:
+ return perms & KEY_OTH_WRITE ? 0 : -EACCES;
+
+ case KEY_NEED_USE:
+ return perms & (KEY_OTH_READ | KEY_OTH_SEARCH) ? 0 : -EACCES;
+
+ case KEY_NEED_WATCH:
+ return perms & KEY_OTH_VIEW ? 0 : -EACCES;
+
+ default:
+ WARN_ON(1);
+ return -EACCES;
+ }
+
+check_auth_override:
+ if (!cred->request_key_auth)
+ return -EACCES;
+ rka = cred->request_key_auth->payload.data[0];
+ if (rka->target_key != key)
+ return -EACCES;
+ *_notes |= KEY_PERMISSION_USED_AUTH_OVERRIDE;
+ return 0;
+
+check_sysadmin_override:
+ if (!capable(CAP_SYS_ADMIN))
+ return -EACCES;
+ *_notes |= KEY_PERMISSION_USED_SYSADMIN_OVERRIDE;
+ return 0;
+}
+
/**
* key_task_permission - Check a key can be used
* @key_ref: The key to check.
@@ -27,27 +137,10 @@ int key_task_permission(const key_ref_t key_ref, const struct cred *cred,
enum key_need_perm need_perm)
{
struct key *key;
- key_perm_t kperm, mask;
+ unsigned int notes = 0;
+ key_perm_t kperm;
int ret;
- switch (need_perm) {
- default:
- WARN_ON(1);
- return -EACCES;
- case KEY_NEED_UNLINK:
- case KEY_SYSADMIN_OVERRIDE:
- case KEY_AUTHTOKEN_OVERRIDE:
- case KEY_DEFER_PERM_CHECK:
- goto lsm;
-
- case KEY_NEED_VIEW: mask = KEY_OTH_VIEW; break;
- case KEY_NEED_READ: mask = KEY_OTH_READ; break;
- case KEY_NEED_WRITE: mask = KEY_OTH_WRITE; break;
- case KEY_NEED_SEARCH: mask = KEY_OTH_SEARCH; break;
- case KEY_NEED_LINK: mask = KEY_OTH_LINK; break;
- case KEY_NEED_SETATTR: mask = KEY_OTH_SETATTR; break;
- }
-
key = key_ref_to_ptr(key_ref);
/* use the second 8-bits of permissions for keys the caller owns */
@@ -75,19 +168,19 @@ int key_task_permission(const key_ref_t key_ref, const struct cred *cred,
kperm = key->perm;
use_these_perms:
-
/* use the top 8-bits of permissions for keys the caller possesses
* - possessor permissions are additive with other permissions
*/
if (is_key_possessed(key_ref))
kperm |= key->perm >> 24;
- if ((kperm & mask) != mask)
- return -EACCES;
+ ret = check_key_permission(key, cred, kperm & KEY_OTH_ALL, need_perm,
+ ¬es);
+ if (ret < 0)
+ return ret;
- /* let LSM be the final arbiter */
-lsm:
- return security_key_permission(key_ref, cred, need_perm);
+ /* Let the LSMs be the final arbiter */
+ return security_key_permission(key_ref, cred, need_perm, notes);
}
EXPORT_SYMBOL(key_task_permission);
diff --git a/security/keys/persistent.c b/security/keys/persistent.c
index 97af230aa4b2..6131a1528680 100644
--- a/security/keys/persistent.c
+++ b/security/keys/persistent.c
@@ -151,7 +151,7 @@ long keyctl_get_persistent(uid_t _uid, key_serial_t destid)
}
/* There must be a destination keyring */
- dest_ref = lookup_user_key(destid, KEY_LOOKUP_CREATE, KEY_NEED_WRITE);
+ dest_ref = lookup_user_key(destid, KEY_LOOKUP_CREATE, KEY_NEED_KEYRING_ADD);
if (IS_ERR(dest_ref))
return PTR_ERR(dest_ref);
if (key_ref_to_ptr(dest_ref)->type != &key_type_keyring) {
diff --git a/security/keys/proc.c b/security/keys/proc.c
index d0cde6685627..373e62556fa5 100644
--- a/security/keys/proc.c
+++ b/security/keys/proc.c
@@ -188,7 +188,7 @@ static int proc_keys_show(struct seq_file *m, void *v)
}
/* check whether the current task is allowed to view the key */
- rc = key_task_permission(key_ref, ctx.cred, KEY_NEED_VIEW);
+ rc = key_task_permission(key_ref, ctx.cred, KEY_NEED_DESCRIBE);
if (rc < 0)
return 0;
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index 7e0232db1707..e39d9033c34c 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -776,26 +776,17 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
if (need_perm != KEY_NEED_UNLINK) {
if (!(lflags & KEY_LOOKUP_PARTIAL)) {
ret = wait_for_key_construction(key, true);
- switch (ret) {
- case -ERESTARTSYS:
+ if (ret < 0)
goto invalid_key;
- default:
- if (need_perm != KEY_AUTHTOKEN_OVERRIDE &&
- need_perm != KEY_DEFER_PERM_CHECK)
- goto invalid_key;
- case 0:
- break;
- }
- } else if (need_perm != KEY_DEFER_PERM_CHECK) {
+
+ ret = -EIO;
+ if (key_read_state(key) == KEY_IS_UNINSTANTIATED)
+ goto invalid_key;
+ } else {
ret = key_validate(key);
if (ret < 0)
goto invalid_key;
}
-
- ret = -EIO;
- if (!(lflags & KEY_LOOKUP_PARTIAL) &&
- key_read_state(key) == KEY_IS_UNINSTANTIATED)
- goto invalid_key;
}
/* check the permissions */
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index e1b9f1a80676..c835b7407a5f 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -332,10 +332,10 @@ static int construct_get_dest_keyring(struct key **_dest_keyring)
BUG();
}
- /*
- * Require Write permission on the keyring. This is essential
- * because the default keyring may be the session keyring, and
- * joining a keyring only requires Search permission.
+ /* Require permission to add a link to the keyring. This is
+ * essential because the default keyring may be the session
+ * keyring, and joining a keyring only requires Search
+ * permission.
*
* However, this check is skipped for the "requestor keyring" so
* that /sbin/request-key can itself use request_key() to add
@@ -343,7 +343,7 @@ static int construct_get_dest_keyring(struct key **_dest_keyring)
*/
if (dest_keyring && do_perm_check) {
ret = key_permission(make_key_ref(dest_keyring, 1),
- KEY_NEED_WRITE);
+ KEY_NEED_KEYRING_ADD);
if (ret) {
key_put(dest_keyring);
return ret;
diff --git a/security/keys/request_key_auth.c b/security/keys/request_key_auth.c
index 41e9735006d0..588130b631b8 100644
--- a/security/keys/request_key_auth.c
+++ b/security/keys/request_key_auth.c
@@ -258,6 +258,7 @@ struct key *key_get_instantiation_authkey(key_serial_t target_id)
};
struct key *authkey;
key_ref_t authkey_ref;
+ int ret;
ctx.index_key.desc_len = sprintf(description, "%x", target_id);
@@ -272,6 +273,12 @@ struct key *key_get_instantiation_authkey(key_serial_t target_id)
goto error;
}
+ ret = key_permission(authkey_ref, KEY_NEED_ASSUME_AUTHORITY);
+ if (ret < 0) {
+ key_ref_put(authkey_ref);
+ authkey = ERR_PTR(ret);
+ }
+
authkey = key_ref_to_ptr(authkey_ref);
if (test_bit(KEY_FLAG_REVOKED, &authkey->flags)) {
key_put(authkey);
diff --git a/security/security.c b/security/security.c
index 70a7ad357bc6..4989f8963b89 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2443,9 +2443,9 @@ void security_key_free(struct key *key)
}
int security_key_permission(key_ref_t key_ref, const struct cred *cred,
- enum key_need_perm need_perm)
+ enum key_need_perm need_perm, unsigned int flags)
{
- return call_int_hook(key_permission, 0, key_ref, cred, need_perm);
+ return call_int_hook(key_permission, 0, key_ref, cred, need_perm, flags);
}
int security_key_getsecurity(struct key *key, char **_buffer)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index efa6108b1ce9..3bed89539160 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -91,6 +91,7 @@
#include <uapi/linux/mount.h>
#include <linux/fsnotify.h>
#include <linux/fanotify.h>
+#include <keys/request_key_auth-type.h>
#include "avc.h"
#include "objsec.h"
@@ -6529,6 +6530,121 @@ static int selinux_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen)
}
#ifdef CONFIG_KEYS
+/*
+ * Convert the requested KEY_NEED_* permit into an SELinux KEY__* permission.
+ *
+ * flags may also convey override flags such as
+ * KEY_PERMISSION_USED_AUTH/SYSADMIN_OVERRIDE to indicate when the main
+ * permission check overrode the permissions on the key.
+ *
+ * Returns the perms to check for in *_perm and *_perm2. If either perm is
+ * present, then the operation is allowed.
+ */
+static int selinux_keyperm_to_av(struct key *key, const struct cred *cred,
+ unsigned int need_perm, unsigned int flags,
+ u32 *_perm, u32 *_perm2)
+{
+ bool auth_can_override = false; /* See KEYCTL_ASSUME_AUTHORITY */
+ bool sysadmin_can_override = false;
+
+ switch (need_perm) {
+ case KEY_NEED_ASSUME_AUTHORITY:
+ return 0;
+
+ case KEY_NEED_DESCRIBE:
+ case KEY_NEED_GET_SECURITY:
+ *_perm = KEY__VIEW;
+ auth_can_override = true;
+ break;
+
+ case KEY_NEED_CHOWN:
+ case KEY_NEED_SETPERM:
+ case KEY_NEED_SET_RESTRICTION:
+ *_perm = KEY__SETATTR;
+ break;
+
+ case KEY_NEED_INSTANTIATE:
+ auth_can_override = true;
+ break;
+
+ case KEY_NEED_INVALIDATE:
+ *_perm = KEY__SEARCH;
+ if (test_bit(KEY_FLAG_ROOT_CAN_INVAL, &key->flags))
+ sysadmin_can_override = true;
+ break;
+
+ case KEY_NEED_JOIN:
+ case KEY_NEED_LINK:
+ *_perm = KEY__LINK;
+ break;
+
+ case KEY_NEED_KEYRING_ADD:
+ case KEY_NEED_KEYRING_DELETE:
+ *_perm = KEY__WRITE;
+ break;
+
+ case KEY_NEED_KEYRING_CLEAR:
+ *_perm = KEY__WRITE;
+ if (test_bit(KEY_FLAG_ROOT_CAN_CLEAR, &key->flags))
+ sysadmin_can_override = true;
+ break;
+
+ case KEY_NEED_READ:
+ *_perm = KEY__READ;
+ break;
+
+ case KEY_NEED_REVOKE:
+ *_perm = KEY__SETATTR;
+ *_perm2 = KEY__WRITE;
+ break;
+
+ case KEY_NEED_SEARCH:
+ *_perm = KEY__SEARCH;
+ break;
+
+ case KEY_NEED_SET_TIMEOUT:
+ *_perm = KEY__SETATTR;
+ auth_can_override = true;
+ break;
+
+ case KEY_NEED_UNLINK:
+ return 0; /* Mustn't prevent this; KEY_FLAG_KEEP is already
+ * dealt with. */
+
+ case KEY_NEED_UPDATE:
+ *_perm = KEY__WRITE;
+ break;
+
+ case KEY_NEED_USE:
+ *_perm = KEY__READ;
+ *_perm2 = KEY__SEARCH;
+ break;
+
+ case KEY_NEED_WATCH:
+ *_perm = KEY__VIEW;
+ break;
+
+ default:
+ WARN_ON(1);
+ return -EPERM;
+ }
+
+ /* Just allow the operation if the process has an authorisation token.
+ * The presence of the token means that the kernel delegated
+ * instantiation of a key to the process - which is problematic if we
+ * then say that the process isn't allowed to get the description of
+ * the key or actually instantiate it.
+ */
+ if (auth_can_override && cred->request_key_auth) {
+ struct request_key_auth *rka =
+ cred->request_key_auth->payload.data[0];
+ if (rka->target_key == key)
+ *_perm = 0;
+ }
+
+ return 0;
+}
+
static int selinux_key_alloc(struct key *k, const struct cred *cred,
unsigned long flags)
{
@@ -6559,48 +6675,29 @@ static void selinux_key_free(struct key *k)
static int selinux_key_permission(key_ref_t key_ref,
const struct cred *cred,
- enum key_need_perm need_perm)
+ enum key_need_perm need_perm,
+ unsigned int flags)
{
struct key *key;
struct key_security_struct *ksec;
- u32 perm, sid;
-
- switch (need_perm) {
- case KEY_NEED_VIEW:
- perm = KEY__VIEW;
- break;
- case KEY_NEED_READ:
- perm = KEY__READ;
- break;
- case KEY_NEED_WRITE:
- perm = KEY__WRITE;
- break;
- case KEY_NEED_SEARCH:
- perm = KEY__SEARCH;
- break;
- case KEY_NEED_LINK:
- perm = KEY__LINK;
- break;
- case KEY_NEED_SETATTR:
- perm = KEY__SETATTR;
- break;
- case KEY_NEED_UNLINK:
- case KEY_SYSADMIN_OVERRIDE:
- case KEY_AUTHTOKEN_OVERRIDE:
- case KEY_DEFER_PERM_CHECK:
- return 0;
- default:
- WARN_ON(1);
- return -EPERM;
-
- }
+ u32 sid, perm = 0, perm2 = 0;
+ int ret;
sid = cred_sid(cred);
key = key_ref_to_ptr(key_ref);
ksec = key->security;
+ ret = selinux_keyperm_to_av(key, cred, need_perm, flags, &perm, &perm2);
+ if (ret < 0 || !perm)
+ return ret;
+
+ ret = avc_has_perm(&selinux_state,
+ sid, ksec->sid, SECCLASS_KEY, perm, NULL);
+ if (ret == 0 || !perm2)
+ return ret;
+
return avc_has_perm(&selinux_state,
- sid, ksec->sid, SECCLASS_KEY, perm, NULL);
+ sid, ksec->sid, SECCLASS_KEY, perm2, NULL);
}
static int selinux_key_getsecurity(struct key *key, char **_buffer)
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 8ffbf951b7ed..2246b4dc99ab 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4221,7 +4221,8 @@ static void smack_key_free(struct key *key)
*/
static int smack_key_permission(key_ref_t key_ref,
const struct cred *cred,
- enum key_need_perm need_perm)
+ enum key_need_perm need_perm,
+ unsigned int flags)
{
struct key *keyp;
struct smk_audit_info ad;
@@ -4229,46 +4230,87 @@ static int smack_key_permission(key_ref_t key_ref,
int request = 0;
int rc;
+ keyp = key_ref_to_ptr(key_ref);
+ if (keyp == NULL)
+ return -EINVAL;
+ /*
+ * If the key hasn't been initialized give it access so that
+ * it may do so.
+ */
+ if (keyp->security == NULL)
+ return 0;
+ /*
+ * This should not occur
+ */
+ if (tkp == NULL)
+ return -EACCES;
+
/*
* Validate requested permissions
*/
switch (need_perm) {
+ case KEY_NEED_ASSUME_AUTHORITY:
+ return 0;
+
+ case KEY_NEED_DESCRIBE:
+ case KEY_NEED_GET_SECURITY:
+ request |= MAY_READ;
+ auth_can_override = true;
+ break;
+
+ case KEY_NEED_CHOWN:
+ case KEY_NEED_INVALIDATE:
+ case KEY_NEED_JOIN:
+ case KEY_NEED_LINK:
+ case KEY_NEED_KEYRING_ADD:
+ case KEY_NEED_KEYRING_CLEAR:
+ case KEY_NEED_KEYRING_DELETE:
+ case KEY_NEED_REVOKE:
+ case KEY_NEED_SETPERM:
+ case KEY_NEED_SET_RESTRICTION:
+ case KEY_NEED_UPDATE:
+ request |= MAY_WRITE;
+ break;
+
+ case KEY_NEED_INSTANTIATE:
+ auth_can_override = true;
+ break;
+
case KEY_NEED_READ:
case KEY_NEED_SEARCH:
- case KEY_NEED_VIEW:
+ case KEY_NEED_USE:
+ case KEY_NEED_WATCH:
request |= MAY_READ;
break;
- case KEY_NEED_WRITE:
- case KEY_NEED_LINK:
- case KEY_NEED_SETATTR:
+
+ case KEY_NEED_SET_TIMEOUT:
request |= MAY_WRITE;
+ auth_can_override = true;
break;
- case KEY_NEED_UNSPECIFIED:
+
case KEY_NEED_UNLINK:
- case KEY_SYSADMIN_OVERRIDE:
- case KEY_AUTHTOKEN_OVERRIDE:
- case KEY_DEFER_PERM_CHECK:
- return 0;
+ return 0; /* Mustn't prevent this; KEY_FLAG_KEEP is already
+ * dealt with. */
+
default:
+ WARN_ON(1);
return -EINVAL;
}
- keyp = key_ref_to_ptr(key_ref);
- if (keyp == NULL)
- return -EINVAL;
- /*
- * If the key hasn't been initialized give it access so that
- * it may do so.
+ /* Just allow the operation if the process has an authorisation token.
+ * The presence of the token means that the kernel delegated
+ * instantiation of a key to the process - which is problematic if we
+ * then say that the process isn't allowed to get the description of
+ * the key or actually instantiate it.
*/
- if (keyp->security == NULL)
- return 0;
- /*
- * This should not occur
- */
- if (tkp == NULL)
- return -EACCES;
+ if (auth_can_override && cred->request_key_auth) {
+ struct request_key_auth *rka =
+ cred->request_key_auth->payload.data[0];
+ if (rka->target_key == key)
+ *_perm = 0;
+ }
- if (smack_privileged(CAP_MAC_OVERRIDE))
+ if (smack_privileged_cred(CAP_MAC_OVERRIDE, cred))
return 0;
#ifdef CONFIG_AUDIT
^ permalink raw reply related
* [RFC PATCH 0/5] keys: Security changes, ACLs and Container keyring
From: David Howells @ 2020-07-16 20:34 UTC (permalink / raw)
To: Stephen Smalley, Casey Schaufler
Cc: keyrings, Jarkko Sakkinen, Paul Moore, selinux, dhowells,
Jarkko Sakkinen, Eric Biederman, jlayton, christian, selinux,
linux-afs, linux-nfs, linux-cifs, linux-api, linux-fsdevel,
linux-security-module, linux-kernel, containers
Here are some patches to provide some security changes and some container
support:
(1) The mapping from KEY_NEED_* symbols to what is permitted is pushed
further down into the general permissions checking routines and the
LSMs.
More KEY_NEED_* symbols are provided to give finer grained control in
the mapping.
(2) The permissions mask is replaced internally with an ACL that contains
a list of ACEs, each with a specific subject and granted permissions
mask. Potted default ACLs are available for new keys and keyrings.
ACE subjects include:
- The owner of the key (uid)
- The key's group (gid)
- Anyone who possesses the key
- Everyone
- Containers (ie. user_namespaces)
(3) The ACE permissions are split to give finer control. Examples include
splitting the revocation permit from the change-attributes permit,
thereby allowing someone to be granted permission to revoke a key
without allowing them to change the owner; also the ability to join a
keyring is split from the ability to link to it, thereby stopping a
process accessing a keyring by joining it and thus acquiring use of
possessor permits.
This is only accessible through the ACL interface; the old setperm
interface concocts an ACL from what it is given.
(4) A keyctl is provided to grant or remove a grant of one or more permits
to a specific subject. Direct access to the ACL is not permitted, and
the ACL cannot be viewed.
(5) A container keyring is made available on the user-namespace and
created when needed. This is searched by request_key() after it has
searched the normal thread/process/session keyrings, but it can't
normally be accessed from inside the container. The container
manager, however, *can* manipulate the contents of the keyring.
This allows the manager to push keys into the container to allow the
use of authenticated network filesystems without any need for the
denizens inside the container to do anything as the manager can add
and refresh the keys.
The patches can be found on the following branch:
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-acl
David
---
David Howells (1):
keys: Implement a 'container' keyring
Documentation/security/keys/core.rst | 128 ++++-
Documentation/security/keys/request-key.rst | 9 +-
certs/blacklist.c | 9 +-
certs/system_keyring.c | 12 +-
crypto/asymmetric_keys/asymmetric_type.c | 2 +-
drivers/md/dm-crypt.c | 2 +-
drivers/md/dm-verity-verify-sig.c | 3 +-
drivers/nvdimm/security.c | 2 +-
fs/afs/security.c | 2 +-
fs/cifs/cifs_spnego.c | 25 +-
fs/cifs/cifsacl.c | 28 +-
fs/cifs/connect.c | 4 +-
fs/crypto/keyring.c | 29 +-
fs/crypto/keysetup_v1.c | 2 +-
fs/ecryptfs/ecryptfs_kernel.h | 2 +-
fs/ecryptfs/keystore.c | 2 +-
fs/fscache/object-list.c | 2 +-
fs/nfs/nfs4idmap.c | 30 +-
fs/ubifs/auth.c | 2 +-
fs/verity/signature.c | 14 +-
include/linux/key.h | 142 +++--
include/linux/lsm_hook_defs.h | 2 +-
include/linux/lsm_hooks.h | 8 +
include/linux/security.h | 9 +-
include/linux/user_namespace.h | 7 +
include/uapi/linux/keyctl.h | 69 +++
lib/digsig.c | 2 +-
net/ceph/ceph_common.c | 2 +-
net/dns_resolver/dns_key.c | 12 +-
net/dns_resolver/dns_query.c | 15 +-
net/rxrpc/key.c | 19 +-
net/rxrpc/security.c | 2 +-
net/wireless/reg.c | 6 +-
security/integrity/digsig.c | 31 +-
security/integrity/digsig_asymmetric.c | 2 +-
security/integrity/evm/evm_crypto.c | 2 +-
security/integrity/ima/ima_mok.c | 13 +-
security/integrity/integrity.h | 6 +-
.../integrity/platform_certs/platform_keyring.c | 14 +-
security/keys/Kconfig | 11 +
security/keys/compat.c | 5 +
security/keys/dh.c | 7 +-
security/keys/encrypted-keys/encrypted.c | 2 +-
security/keys/encrypted-keys/masterkey_trusted.c | 2 +-
security/keys/gc.c | 2 +-
security/keys/internal.h | 41 +-
security/keys/key.c | 98 ++--
security/keys/keyctl.c | 580 +++++++++++---------
security/keys/keyctl_pkey.c | 17 +-
security/keys/keyring.c | 47 +-
security/keys/permission.c | 600 +++++++++++++++++++--
security/keys/persistent.c | 29 +-
security/keys/proc.c | 27 +-
security/keys/process_keys.c | 137 +++--
security/keys/request_key.c | 49 +-
security/keys/request_key_auth.c | 23 +-
security/security.c | 4 +-
security/selinux/hooks.c | 163 ++++--
security/smack/smack_lsm.c | 90 +++-
59 files changed, 1885 insertions(+), 721 deletions(-)
^ permalink raw reply
* Re: [PATCH v2 4/5] LSM: Define SELinux function to measure security state
From: Stephen Smalley @ 2020-07-16 19:45 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: Mimi Zohar, Casey Schaufler, James Morris, linux-integrity,
SElinux list, LSM List, linux-kernel
In-Reply-To: <9478ddca-8298-5170-836d-8cbc7a070df2@linux.microsoft.com>
On Thu, Jul 16, 2020 at 3:13 PM Lakshmi Ramasubramanian
<nramas@linux.microsoft.com> wrote:
>
> On 7/16/20 11:54 AM, Stephen Smalley wrote:
> > Not sure about this error handling approach (silent, proceeding as if
> > the length was zero and then later failing with ENOMEM on every
> > attempt?). I'd be more inclined to panic/BUG here but I know Linus
> > doesn't like that.
> I am not sure if failing (kernel panic/BUG) to "measure" LSM data under
> memory pressure conditions is the right thing. But I am open to treating
> this error as a fatal error. Please let me know.
Let's at least log an error message since it otherwise silently
disables all measuring of security state.
Also not sure why we bother returning errors from
selinux_measure_data() since nothing appears to check or use the
result.
Don't know if integrity/IMA has any equivalent to the audit
subsystem's concept of audit_failure settings to control whether
errors that prevent auditing (measuring) are handled silently, with a
log message, or via a panic. If not, I guess that can be explored
separately.
^ permalink raw reply
* Re: [PATCH v6 7/7] ima: add policy support for the new file open MAY_OPENEXEC flag
From: Kees Cook @ 2020-07-16 19:13 UTC (permalink / raw)
To: Randy Dunlap
Cc: Mickaël Salaün, linux-kernel, Aleksa Sarai,
Alexei Starovoitov, Al Viro, Andrew Morton, Andy Lutomirski,
Christian Brauner, Christian Heimes, Daniel Borkmann,
Deven Bowers, Dmitry Vyukov, Eric Biggers, Eric Chiang,
Florian Weimer, James Morris, Jan Kara, Jann Horn,
Jonathan Corbet, Lakshmi Ramasubramanian, Matthew Garrett,
Matthew Wilcox, Michael Kerrisk, Mickaël Salaün,
Mimi Zohar, Philippe Trébuchet, Scott Shell,
Sean Christopherson, Shuah Khan, Steve Dower, Steve Grubb,
Tetsuo Handa, Thibaut Sautereau, Vincent Strubel,
kernel-hardening, linux-api, linux-integrity,
linux-security-module, linux-fsdevel
In-Reply-To: <61c05cb0-a956-3cc7-5dab-e11ebf0e95bf@infradead.org>
On Thu, Jul 16, 2020 at 07:59:20AM -0700, Randy Dunlap wrote:
> On 7/16/20 7:40 AM, Mickaël Salaün wrote:
> >
> > On 15/07/2020 22:40, Kees Cook wrote:
> >> On Tue, Jul 14, 2020 at 08:16:38PM +0200, Mickaël Salaün wrote:
> >>> From: Mimi Zohar <zohar@linux.ibm.com>
> >>>
> >>> The kernel has no way of differentiating between a file containing data
> >>> or code being opened by an interpreter. The proposed O_MAYEXEC
> >>> openat2(2) flag bridges this gap by defining and enabling the
> >>> MAY_OPENEXEC flag.
> >>>
> >>> This patch adds IMA policy support for the new MAY_OPENEXEC flag.
> >>>
> >>> Example:
> >>> measure func=FILE_CHECK mask=^MAY_OPENEXEC
> >>> appraise func=FILE_CHECK appraise_type=imasig mask=^MAY_OPENEXEC
> >>>
> >>> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> >>> Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> >>> Acked-by: Mickaël Salaün <mic@digikod.net>
> >>
> >> (Process nit: if you're sending this on behalf of another author, then
> >> this should be Signed-off-by rather than Acked-by.)
> >
> > I'm not a co-author of this patch.
> >
>
> from Documentation/process/submitting-patches.rst:
>
> The Signed-off-by: tag indicates that the signer was involved in the
> development of the patch, or that he/she was in the patch's delivery path.
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Randy beat me to it. :)
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v2 4/5] LSM: Define SELinux function to measure security state
From: Lakshmi Ramasubramanian @ 2020-07-16 19:13 UTC (permalink / raw)
To: Stephen Smalley
Cc: Mimi Zohar, Casey Schaufler, James Morris, linux-integrity,
SElinux list, LSM List, linux-kernel
In-Reply-To: <CAEjxPJ43eXK0xgrE=gDxZVg2SDTz4bkd7N4otjk-cvxf3fKL-g@mail.gmail.com>
On 7/16/20 11:54 AM, Stephen Smalley wrote:
>> The data for selinux-state in the above measurement is:
>> enabled=1;enforcing=0;checkreqprot=1;network_peer_controls=1;open_perms=1;extended_socket_class=1;always_check_network=0;cgroup_seclabel=1;nnp_nosuid_transition=1;genfs_seclabel_symlinks=0;
>>
>> The data for selinux-policy-hash in the above measurement is
>> the SHA256 hash of the SELinux policy.
>
> Can you show an example of how to verify that the above measurement
> matches a given state and policy, e.g. the sha256sum commands and
> inputs to reproduce the same from an expected state and policy?
Sure - I'll provide an example.
>> +/* Pre-allocated buffer used for measuring state */
>> +static char *selinux_state_string;
>> +static size_t selinux_state_string_len;
>> +static char *selinux_state_string_fmt =
>> + "%s=%d;%s=%d;%s=%d;%s=%d;%s=%d;%s=%d;%s=%d;%s=%d;%s=%d;%s=%d;";
>> +
>> +void __init selinux_init_measurement(void)
>> +{
>> + selinux_state_string_len =
>> + snprintf(NULL, 0, selinux_state_string_fmt,
>> + "enabled", 0,
>> + "enforcing", 0,
>> + "checkreqprot", 0,
>> + selinux_policycap_names[POLICYDB_CAPABILITY_NETPEER], 0,
>> + selinux_policycap_names[POLICYDB_CAPABILITY_OPENPERM], 0,
>> + selinux_policycap_names[POLICYDB_CAPABILITY_EXTSOCKCLASS], 0,
>> + selinux_policycap_names[POLICYDB_CAPABILITY_ALWAYSNETWORK], 0,
>> + selinux_policycap_names[POLICYDB_CAPABILITY_CGROUPSECLABEL], 0,
>> + selinux_policycap_names[POLICYDB_CAPABILITY_NNP_NOSUID_TRANSITION], 0,
>> + selinux_policycap_names[POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS],
>> + 0);
>
> I was thinking you'd dynamically construct the format string with a
> for loop from 0 to POLICYDB_CAPABILITY_MAX
> and likewise for the values so that we wouldn't have to patch this
> code every time we add a new one.
That's a good point - will do.
>
>> +
>> + if (selinux_state_string_len < 0)
>> + return;
>
> How can this happen legitimately (i.e. as a result of something other
> than a kernel bug)?
Since snprintf can return an error I wanted to handle that. But I agree
this should not happen for the input data to snprintf used here.
>
>> +
>> + ++selinux_state_string_len;
>> +
>> + selinux_state_string = kzalloc(selinux_state_string_len, GFP_KERNEL);
>> + if (!selinux_state_string)
>> + selinux_state_string_len = 0;
>> +}
>
> Not sure about this error handling approach (silent, proceeding as if
> the length was zero and then later failing with ENOMEM on every
> attempt?). I'd be more inclined to panic/BUG here but I know Linus
> doesn't like that.
I am not sure if failing (kernel panic/BUG) to "measure" LSM data under
memory pressure conditions is the right thing. But I am open to treating
this error as a fatal error. Please let me know.
>
>> + if (ret)
>> + pr_err("%s: error %d\n", __func__, ret);
>
> This doesn't seem terribly useful as an error message; I'd be inclined
> to drop it.
>
Will do.
thanks,
-lakshmi
^ permalink raw reply
* Re: [PATCH v6 7/7] ima: add policy support for the new file open MAY_OPENEXEC flag
From: Kees Cook @ 2020-07-16 19:12 UTC (permalink / raw)
To: Mickaël Salaün
Cc: linux-kernel, Aleksa Sarai, Alexei Starovoitov, Al Viro,
Andrew Morton, Andy Lutomirski, Christian Brauner,
Christian Heimes, Daniel Borkmann, Deven Bowers, Dmitry Vyukov,
Eric Biggers, Eric Chiang, Florian Weimer, James Morris, Jan Kara,
Jann Horn, Jonathan Corbet, Lakshmi Ramasubramanian,
Matthew Garrett, Matthew Wilcox, Michael Kerrisk,
Mickaël Salaün, Mimi Zohar, Philippe Trébuchet,
Scott Shell, Sean Christopherson, Shuah Khan, Steve Dower,
Steve Grubb, Tetsuo Handa, Thibaut Sautereau, Vincent Strubel,
kernel-hardening, linux-api, linux-integrity,
linux-security-module, linux-fsdevel
In-Reply-To: <8df69733-0088-3e3c-9c3d-2610414cea2b@digikod.net>
On Thu, Jul 16, 2020 at 04:40:15PM +0200, Mickaël Salaün wrote:
>
> On 15/07/2020 22:40, Kees Cook wrote:
> > On Tue, Jul 14, 2020 at 08:16:38PM +0200, Mickaël Salaün wrote:
> >> From: Mimi Zohar <zohar@linux.ibm.com>
> >>
> >> The kernel has no way of differentiating between a file containing data
> >> or code being opened by an interpreter. The proposed O_MAYEXEC
> >> openat2(2) flag bridges this gap by defining and enabling the
> >> MAY_OPENEXEC flag.
> >>
> >> This patch adds IMA policy support for the new MAY_OPENEXEC flag.
> >>
> >> Example:
> >> measure func=FILE_CHECK mask=^MAY_OPENEXEC
> >> appraise func=FILE_CHECK appraise_type=imasig mask=^MAY_OPENEXEC
> >>
> >> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> >> Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> >> Acked-by: Mickaël Salaün <mic@digikod.net>
> >
> > (Process nit: if you're sending this on behalf of another author, then
> > this should be Signed-off-by rather than Acked-by.)
>
> I'm not a co-author of this patch.
Correct, but you are part of the delivery path to its entry to the
tree. If you were co-author, you would include "Co-developed-by" with
a Signed-off-by. (So my nit stands)
For excruciating details:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
"The Signed-off-by: tag indicates that the signer was ... in the patch’s
delivery path."
"Co-developed-by: ... is a used to give attribution to co-authors ..."
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v2 4/5] LSM: Define SELinux function to measure security state
From: Stephen Smalley @ 2020-07-16 18:54 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: Mimi Zohar, Casey Schaufler, James Morris, linux-integrity,
SElinux list, LSM List, linux-kernel
In-Reply-To: <20200716174351.20128-5-nramas@linux.microsoft.com>
On Thu, Jul 16, 2020 at 1:44 PM Lakshmi Ramasubramanian
<nramas@linux.microsoft.com> wrote:
>
> SELinux configuration and policy are some of the critical data for this
> security module that needs to be measured. To enable this measurement
> SELinux needs to implement the interface function,
> security_measure_data(), that the LSM can call.
>
> Define the security_state() function in SELinux to measure SELinux
> configuration and policy. Call this function to measure SELinux data
> when there is a change in the security module's state.
>
> Sample measurement of SELinux state and hash of the policy:
>
> 10 e32e...5ac3 ima-buf sha256:86e8...4594 selinux-state 656e61626c65643d313b656e666f7263696e673d303b636865636b72657170726f743d313b6e6574706565723d313b6f70656e7065726d3d313b657874736f636b636c6173733d313b616c776179736e6574776f726b3d303b6367726f75707365636c6162656c3d313b6e6e706e6f737569647472616e736974696f6e3d313b67656e66737365636c6162656c73796d6c696e6b3d303b
> 10 f4a7...9408 ima-buf sha256:4941...68fc selinux-policy-hash 8d1d...1834
>
> The data for selinux-state in the above measurement is:
> enabled=1;enforcing=0;checkreqprot=1;network_peer_controls=1;open_perms=1;extended_socket_class=1;always_check_network=0;cgroup_seclabel=1;nnp_nosuid_transition=1;genfs_seclabel_symlinks=0;
>
> The data for selinux-policy-hash in the above measurement is
> the SHA256 hash of the SELinux policy.
Can you show an example of how to verify that the above measurement
matches a given state and policy, e.g. the sha256sum commands and
inputs to reproduce the same from an expected state and policy?
>
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> ---
> diff --git a/security/selinux/measure.c b/security/selinux/measure.c
> new file mode 100644
> index 000000000000..27cbb309e926
> --- /dev/null
> +++ b/security/selinux/measure.c
> @@ -0,0 +1,158 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Measure SELinux state using IMA subsystem.
> + */
> +#include <linux/ima.h>
> +#include "security.h"
> +
> +/* Pre-allocated buffer used for measuring state */
> +static char *selinux_state_string;
> +static size_t selinux_state_string_len;
> +static char *selinux_state_string_fmt =
> + "%s=%d;%s=%d;%s=%d;%s=%d;%s=%d;%s=%d;%s=%d;%s=%d;%s=%d;%s=%d;";
> +
> +void __init selinux_init_measurement(void)
> +{
> + selinux_state_string_len =
> + snprintf(NULL, 0, selinux_state_string_fmt,
> + "enabled", 0,
> + "enforcing", 0,
> + "checkreqprot", 0,
> + selinux_policycap_names[POLICYDB_CAPABILITY_NETPEER], 0,
> + selinux_policycap_names[POLICYDB_CAPABILITY_OPENPERM], 0,
> + selinux_policycap_names[POLICYDB_CAPABILITY_EXTSOCKCLASS], 0,
> + selinux_policycap_names[POLICYDB_CAPABILITY_ALWAYSNETWORK], 0,
> + selinux_policycap_names[POLICYDB_CAPABILITY_CGROUPSECLABEL], 0,
> + selinux_policycap_names[POLICYDB_CAPABILITY_NNP_NOSUID_TRANSITION], 0,
> + selinux_policycap_names[POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS],
> + 0);
I was thinking you'd dynamically construct the format string with a
for loop from 0 to POLICYDB_CAPABILITY_MAX
and likewise for the values so that we wouldn't have to patch this
code every time we add a new one.
> +
> + if (selinux_state_string_len < 0)
> + return;
How can this happen legitimately (i.e. as a result of something other
than a kernel bug)?
> +
> + ++selinux_state_string_len;
> +
> + selinux_state_string = kzalloc(selinux_state_string_len, GFP_KERNEL);
> + if (!selinux_state_string)
> + selinux_state_string_len = 0;
> +}
Not sure about this error handling approach (silent, proceeding as if
the length was zero and then later failing with ENOMEM on every
attempt?). I'd be more inclined to panic/BUG here but I know Linus
doesn't like that.
> + if (ret)
> + pr_err("%s: error %d\n", __func__, ret);
This doesn't seem terribly useful as an error message; I'd be inclined
to drop it.
^ permalink raw reply
* Re: [PATCH v3 07/12] ima: Fail rule parsing when appraise_flag=blacklist is unsupportable
From: Tyler Hicks @ 2020-07-16 18:20 UTC (permalink / raw)
To: Mimi Zohar
Cc: Dmitry Kasatkin, James Morris, Serge E . Hallyn,
Lakshmi Ramasubramanian, Prakhar Srivastava, linux-kernel,
linux-integrity, linux-security-module, Nayna Jain
In-Reply-To: <1594923290.12900.376.camel@linux.ibm.com>
On 2020-07-16 14:14:50, Mimi Zohar wrote:
> On Thu, 2020-07-09 at 01:19 -0500, Tyler Hicks wrote:
> > The "appraise_flag" option is only appropriate for appraise actions
> > and its "blacklist" value is only appropriate when
> > CONFIG_IMA_APPRAISE_MODSIG is enabled and "appraise_flag=blacklist" is
> > only appropriate when "appraise_type=imasig|modsig" is also present.
> > Make this clear at policy load so that IMA policy authors don't assume
> > that other uses of "appraise_flag=blacklist" are supported.
>
> The code looks correct, but this patch description could be written at
> a higher level. Perhaps it just needs to be prefixed with something
> like this:
>
> Verifying that a file hash is not blacklisted is currently only
> supported for files with appended signatures (modsig). In the future,
> this might change. For now, ...
That makes sense. I'm not up to speed on the intent behind the blacklist
feature or where it may go in the future so I didn't think to add
anything along those lines.
If you are happy with the rest of the series, please feel free to append
this to the commit message. Otherwise, I can add it if I need to submit
a new revision of the series.
Tyler
>
> Mimi
>
> >
> > Fixes: 273df864cf74 ("ima: Check against blacklisted hashes for files with modsig")
> > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> > Cc: Nayna Jain <nayna@linux.ibm.com>
>
> > ---
> >
> > * v3
> > - New patch
> >
> > security/integrity/ima/ima_policy.c | 13 ++++++++++++-
> > 1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > index 81da02071d41..9842e2e0bc6d 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -1035,6 +1035,11 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> > return false;
> > }
> >
> > + /* Ensure that combinations of flags are compatible with each other */
> > + if (entry->flags & IMA_CHECK_BLACKLIST &&
> > + !(entry->flags & IMA_MODSIG_ALLOWED))
> > + return false;
> > +
> > return true;
> > }
> >
> > @@ -1371,8 +1376,14 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
> > result = -EINVAL;
> > break;
> > case Opt_appraise_flag:
> > + if (entry->action != APPRAISE) {
> > + result = -EINVAL;
> > + break;
> > + }
> > +
> > ima_log_string(ab, "appraise_flag", args[0].from);
> > - if (strstr(args[0].from, "blacklist"))
> > + if (IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG) &&
> > + strstr(args[0].from, "blacklist"))
> > entry->flags |= IMA_CHECK_BLACKLIST;
> > break;
> > case Opt_permit_directio:
^ permalink raw reply
* Re: [PATCH v3 07/12] ima: Fail rule parsing when appraise_flag=blacklist is unsupportable
From: Mimi Zohar @ 2020-07-16 18:14 UTC (permalink / raw)
To: Tyler Hicks, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module, Nayna Jain
In-Reply-To: <20200709061911.954326-8-tyhicks@linux.microsoft.com>
On Thu, 2020-07-09 at 01:19 -0500, Tyler Hicks wrote:
> The "appraise_flag" option is only appropriate for appraise actions
> and its "blacklist" value is only appropriate when
> CONFIG_IMA_APPRAISE_MODSIG is enabled and "appraise_flag=blacklist" is
> only appropriate when "appraise_type=imasig|modsig" is also present.
> Make this clear at policy load so that IMA policy authors don't assume
> that other uses of "appraise_flag=blacklist" are supported.
The code looks correct, but this patch description could be written at
a higher level. Perhaps it just needs to be prefixed with something
like this:
Verifying that a file hash is not blacklisted is currently only
supported for files with appended signatures (modsig). In the future,
this might change. For now, ...
Mimi
>
> Fixes: 273df864cf74 ("ima: Check against blacklisted hashes for files with modsig")
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> Cc: Nayna Jain <nayna@linux.ibm.com>
> ---
>
> * v3
> - New patch
>
> security/integrity/ima/ima_policy.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 81da02071d41..9842e2e0bc6d 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -1035,6 +1035,11 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> return false;
> }
>
> + /* Ensure that combinations of flags are compatible with each other */
> + if (entry->flags & IMA_CHECK_BLACKLIST &&
> + !(entry->flags & IMA_MODSIG_ALLOWED))
> + return false;
> +
> return true;
> }
>
> @@ -1371,8 +1376,14 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
> result = -EINVAL;
> break;
> case Opt_appraise_flag:
> + if (entry->action != APPRAISE) {
> + result = -EINVAL;
> + break;
> + }
> +
> ima_log_string(ab, "appraise_flag", args[0].from);
> - if (strstr(args[0].from, "blacklist"))
> + if (IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG) &&
> + strstr(args[0].from, "blacklist"))
> entry->flags |= IMA_CHECK_BLACKLIST;
> break;
> case Opt_permit_directio:
^ permalink raw reply
* Re: [V2 PATCH 1/3] Refactoring powerpc code for carrying over IMA measurement logs, to move non architecture specific code to security/ima.
From: Thiago Jung Bauermann @ 2020-07-16 17:51 UTC (permalink / raw)
To: Prakhar Srivastava
Cc: linux-arm-kernel, linux-kernel, linuxppc-dev, devicetree,
linux-integrity, linux-security-module, catalin.marinas, will,
mpe, benh, paulus, robh+dt, frowand.list, zohar, dmitry.kasatkin,
jmorris, serge, pasha.tatashin, allison, kstewart,
takahiro.akashi, tglx, vincenzo.frascino, mark.rutland, masahiroy,
james.morse, bhsharma, mbrugger, hsinyi, tao.li, christophe.leroy,
gregkh, nramas, tusharsu, balajib
In-Reply-To: <1385c8bb-cd25-8dc4-7224-8e27135f3356@linux.microsoft.com>
Hello Prakhar,
Prakhar Srivastava <prsriva@linux.microsoft.com> writes:
> On 6/19/20 5:19 PM, Thiago Jung Bauermann wrote:
>>
>> Prakhar Srivastava <prsriva@linux.microsoft.com> writes:
>>
>>> Powerpc has support to carry over the IMA measurement logs. Refatoring the
>>> non-architecture specific code out of arch/powerpc and into security/ima.
>>>
>>> The code adds support for reserving and freeing up of memory for IMA measurement
>>> logs.
>>
>> Last week, Mimi provided this feedback:
>>
>> "From your patch description, this patch should be broken up. Moving
>> the non-architecture specific code out of powerpc should be one patch.
>> Additional support should be in another patch. After each patch, the
>> code should work properly."
>>
>> That's not what you do here. You move the code, but you also make other
>> changes at the same time. This has two problems:
>>
>> 1. It makes the patch harder to review, because it's very easy to miss a
>> change.
>>
>> 2. If in the future a git bisect later points to this patch, it's not
>> clear whether the problem is because of the code movement, or because
>> of the other changes.
>>
>> When you move code, ideally the patch should only make the changes
>> necessary to make the code work at its new location. The patch which
>> does code movement should not cause any change in behavior.
>>
>> Other changes should go in separate patches, either before or after the
>> one moving the code.
>>
>> More comments below.
>>
> Hi Thiago,
>
> Apologies for the delayed response i was away for a few days.
> I am working on breaking up the changes so that its easier to review and update
> as well.
No problem.
>
> Thanks,
> Prakhar Srivastava
>
>>>
>>> ---
>>> arch/powerpc/include/asm/ima.h | 10 ---
>>> arch/powerpc/kexec/ima.c | 126 ++---------------------------
>>> security/integrity/ima/ima_kexec.c | 116 ++++++++++++++++++++++++++
>>> 3 files changed, 124 insertions(+), 128 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/ima.h b/arch/powerpc/include/asm/ima.h
>>> index ead488cf3981..c29ec86498f8 100644
>>> --- a/arch/powerpc/include/asm/ima.h
>>> +++ b/arch/powerpc/include/asm/ima.h
>>> @@ -4,15 +4,6 @@
>>>
>>> struct kimage;
>>>
>>> -int ima_get_kexec_buffer(void **addr, size_t *size);
>>> -int ima_free_kexec_buffer(void);
>>> -
>>> -#ifdef CONFIG_IMA
>>> -void remove_ima_buffer(void *fdt, int chosen_node);
>>> -#else
>>> -static inline void remove_ima_buffer(void *fdt, int chosen_node) {}
>>> -#endif
>>> -
>>> #ifdef CONFIG_IMA_KEXEC
>>> int arch_ima_add_kexec_buffer(struct kimage *image, unsigned long load_addr,
>>> size_t size);
>>> @@ -22,7 +13,6 @@ int setup_ima_buffer(const struct kimage *image, void *fdt, int chosen_node);
>>> static inline int setup_ima_buffer(const struct kimage *image, void *fdt,
>>> int chosen_node)
>>> {
>>> - remove_ima_buffer(fdt, chosen_node);
>>> return 0;
>>> }
>>
>> This is wrong. Even if the currently running kernel doesn't have
>> CONFIG_IMA_KEXEC, it should remove the IMA buffer property and memory
>> reservation from the FDT that is being prepared for the next kernel.
>>
>> This is because the IMA kexec buffer is useless for the next kernel,
>> regardless of whether the current kernel supports CONFIG_IMA_KEXEC or
>> not. Keeping it around would be a waste of memory.
>>
> I will keep it in my next revision.
> My understanding was the reserved memory is freed and property removed when IMA
> loads the logs on init.
If CONFIG_IMA_KEXEC is set, then yes. If it isn't then that needs to
happen in the function above.
> During setup_fdt in kexec, a duplicate copy of the dt is
> used, but memory still needs to be allocated, thus the property itself indicats
> presence of reserved memory.
>
>>> @@ -179,13 +64,18 @@ int setup_ima_buffer(const struct kimage *image, void *fdt, int chosen_node)
>>> int ret, addr_cells, size_cells, entry_size;
>>> u8 value[16];
>>>
>>> - remove_ima_buffer(fdt, chosen_node);
>>
>> This is wrong, for the same reason stated above.
>>
>>> if (!image->arch.ima_buffer_size)
>>> return 0;
>>>
>>> - ret = get_addr_size_cells(&addr_cells, &size_cells);
>>> - if (ret)
>>> + ret = fdt_address_cells(fdt, chosen_node);
>>> + if (ret < 0)
>>> + return ret;
>>> + addr_cells = ret;
>>> +
>>> + ret = fdt_size_cells(fdt, chosen_node);
>>> + if (ret < 0)
>>> return ret;
>>> + size_cells = ret;
>>>
>>> entry_size = 4 * (addr_cells + size_cells);
>>>
>>
>> I liked this change. Thanks! I agree it's better to use
>> fdt_address_cells() and fdt_size_cells() here.
>>
>> But it should be in a separate patch. Either before or after the one
>> moving the code.
>>
>>> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
>>> index 121de3e04af2..e1e6d6154015 100644
>>> --- a/security/integrity/ima/ima_kexec.c
>>> +++ b/security/integrity/ima/ima_kexec.c
>>> @@ -10,8 +10,124 @@
>>> #include <linux/seq_file.h>
>>> #include <linux/vmalloc.h>
>>> #include <linux/kexec.h>
>>> +#include <linux/of.h>
>>> +#include <linux/memblock.h>
>>> +#include <linux/libfdt.h>
>>> #include "ima.h"
>>>
>>> +static int get_addr_size_cells(int *addr_cells, int *size_cells)
>>> +{
>>> + struct device_node *root;
>>> +
>>> + root = of_find_node_by_path("/");
>>> + if (!root)
>>> + return -EINVAL;
>>> +
>>> + *addr_cells = of_n_addr_cells(root);
>>> + *size_cells = of_n_size_cells(root);
>>> +
>>> + of_node_put(root);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int do_get_kexec_buffer(const void *prop, int len, unsigned long *addr,
>>> + size_t *size)
>>> +{
>>> + int ret, addr_cells, size_cells;
>>> +
>>> + ret = get_addr_size_cells(&addr_cells, &size_cells);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + if (len < 4 * (addr_cells + size_cells))
>>> + return -ENOENT;
>>> +
>>> + *addr = of_read_number(prop, addr_cells);
>>> + *size = of_read_number(prop + 4 * addr_cells, size_cells);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +/**
>>> + * ima_get_kexec_buffer - get IMA buffer from the previous kernel
>>> + * @addr: On successful return, set to point to the buffer contents.
>>> + * @size: On successful return, set to the buffer size.
>>> + *
>>> + * Return: 0 on success, negative errno on error.
>>> + */
>>> +int ima_get_kexec_buffer(void **addr, size_t *size)
>>> +{
>>> + int ret, len;
>>> + unsigned long tmp_addr;
>>> + size_t tmp_size;
>>> + const void *prop;
>>> +
>>> + prop = of_get_property(of_chosen, "linux,ima-kexec-buffer", &len);
>>> + if (!prop)
>>> + return -ENOENT;
>>> +
>>> + ret = do_get_kexec_buffer(prop, len, &tmp_addr, &tmp_size);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + *addr = __va(tmp_addr);
>>> + *size = tmp_size;
>>> +
>>> + return 0;
>>> +}
>>
>> The functions above were moved without being changed. Good.
>>
>>> +/**
>>> + * ima_free_kexec_buffer - free memory used by the IMA buffer
>>> + */
>>> +int ima_free_kexec_buffer(void)
>>> +{
>>> + int ret;
>>> + unsigned long addr;
>>> + size_t size;
>>> + struct property *prop;
>>> +
>>> + prop = of_find_property(of_chosen, "linux,ima-kexec-buffer", NULL);
>>> + if (!prop)
>>> + return -ENOENT;
>>> +
>>> + ret = do_get_kexec_buffer(prop->value, prop->length, &addr, &size);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = of_remove_property(of_chosen, prop);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + return memblock_free(__pa(addr), size);
>>
>> Here you added a __pa() call. Do you store a virtual address in
>> linux,ima-kexec-buffer property? Doesn't it make more sense to store a
>> physical address?
>>
> trying to minimize the changes here as do_get_kexec_buffer return the va.
> I will refactor this to remove the double translation.
In the powerpc version, do_get_kexec_buffer() returns the pa, and one of
its callers does the va translation. I think that worked well.
>> Even if making this change is the correct thing to do, it should be a
>> separate patch, unless it can't be avoided. And if that is the case,
>> then it should be explained in the commit message.
>>
>>> +
>>> +}
>>> +
>>> +/**
>>> + * remove_ima_buffer - remove the IMA buffer property and reservation from @fdt
>>> + *
>>> + * The IMA measurement buffer is of no use to a subsequent kernel, so we always
>>> + * remove it from the device tree.
>>> + */
>>> +void remove_ima_buffer(void *fdt, int chosen_node)
>>> +{
>>> + int ret, len;
>>> + unsigned long addr;
>>> + size_t size;
>>> + const void *prop;
>>> +
>>> + prop = fdt_getprop(fdt, chosen_node, "linux,ima-kexec-buffer", &len);
>>> + if (!prop)
>>> + return;
>>> +
>>> + do_get_kexec_buffer(prop, len, &addr, &size);
>>> + ret = fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer");
>>> + if (ret < 0)
>>> + return;
>>> +
>>> + memblock_free(addr, size);
>>> +}
>>
>> Here is another function that changed when moved. This one I know to be
>> wrong. You're confusing the purposes of remove_ima_buffer() and
>> ima_free_kexec_buffer().
>>
>> You did send me a question about them nearly three weeks ago which I
>> only answered today, so I apologize. Also, their names could more
>> clearly reflect their differences, so it's bad naming on my part.
>>
>> With IMA kexec buffers, there are two kernels (and thus their two
>> respective, separate device trees) to be concerned about:
>>
>> 1. the currently running kernel, which uses the live device tree
>> (accessed with the of_* functions) and the memblock subsystem;
>>
>> 2. the kernel which is being loaded by kexec, which will use the FDT
>> blob being passed around as argument to these functions, and the memory
>> reservations in the memory reservation table of the FDT blob.
>>
>> ima_free_kexec_buffer() is used by IMA in the currently running kernel.
>> Therefore the device tree it is concerned about is the live one, and
>> thus uses the of_* functions to access it. And uses memblock to change
>> the memory reservation.
>>
>> remove_ima_buffer() on the other hand is used by the kexec code to
>> prepare an FDT blob for the kernel that is being loaded. It should not
>> make any changes to live device tree, nor to memblock allocations. It
>> should only make changes to the FDT blob.
>>
> Thank you for this, greatly appreciate clearing my misunderstandings.
You're welcome. Sorry again for not answering your question before you
sent this patch series.
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* [PATCH v2 2/5] IMA: Define an IMA hook to measure LSM data
From: Lakshmi Ramasubramanian @ 2020-07-16 17:43 UTC (permalink / raw)
To: zohar, stephen.smalley.work, casey
Cc: jmorris, linux-integrity, selinux, linux-security-module,
linux-kernel
In-Reply-To: <20200716174351.20128-1-nramas@linux.microsoft.com>
IMA subsystem needs to define an IMA hook that the security modules can
call to measure critical data of the security modules.
Define a new IMA hook, namely ima_lsm_state(), that the security modules
can call to measure data.
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
include/linux/ima.h | 4 ++++
security/integrity/ima/ima_main.c | 17 +++++++++++++++++
2 files changed, 21 insertions(+)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 9164e1534ec9..7e2686f4953a 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -26,6 +26,7 @@ extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
extern void ima_post_path_mknod(struct dentry *dentry);
extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
extern void ima_kexec_cmdline(const void *buf, int size);
+extern void ima_lsm_state(const char *lsm_event_name, const void *buf, int size);
#ifdef CONFIG_IMA_KEXEC
extern void ima_add_kexec_buffer(struct kimage *image);
@@ -104,6 +105,9 @@ static inline int ima_file_hash(struct file *file, char *buf, size_t buf_size)
}
static inline void ima_kexec_cmdline(const void *buf, int size) {}
+
+static inline void ima_lsm_state(const char *lsm_event_name,
+ const void *buf, int size) {}
#endif /* CONFIG_IMA */
#ifndef CONFIG_IMA_KEXEC
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 8351b2fd48e0..04d9a1d35300 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -835,6 +835,23 @@ void ima_kexec_cmdline(const void *buf, int size)
KEXEC_CMDLINE, 0, NULL);
}
+/**
+ * ima_lsm_state - measure LSM specific state
+ * @lsm_event_name: LSM event
+ * @buf: pointer to buffer containing LSM specific state
+ * @size: Number of bytes in buf
+ *
+ * Buffers can only be measured, not appraised.
+ */
+void ima_lsm_state(const char *lsm_event_name, const void *buf, int size)
+{
+ if (!lsm_event_name || !buf || !size)
+ return;
+
+ process_buffer_measurement(buf, size, lsm_event_name,
+ LSM_STATE, 0, NULL);
+}
+
static int __init init_ima(void)
{
int error;
--
2.27.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox