| Message ID | 1526568530-9144-4-git-send-email-zohar@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 5/17/2018 7:48 AM, Mimi Zohar wrote: > In order for LSMs and IMA-appraisal to differentiate between the original > and new syscalls (eg. kexec, kernel modules, firmware), both the original > and new syscalls must call an LSM hook. > > Commit 2e72d51b4ac3 ("security: introduce kernel_module_from_file hook") > introduced calling security_kernel_module_from_file() in both the original > and new syscalls. Commit a1db74209483 ("module: replace > copy_module_from_fd with kernel version") replaced these LSM calls with > security_kernel_read_file(). > > Commit e40ba6d56b41 ("firmware: replace call to fw_read_file_contents() > with kernel version") and commit b804defe4297 ("kexec: replace call to > copy_file_from_fd() with kernel version") replaced their own version of > reading a file from the kernel with the generic > kernel_read_file_from_path/fd() versions, which call the pre and post > security_kernel_read_file LSM hooks. > > Missing are LSM calls in the original kexec syscall and firmware sysfs > fallback method. From a technical perspective there is no justification > for defining a new LSM hook, as the existing security_kernel_read_file() > works just fine. The original syscalls, however, do not read a file, so > the security hook name is inappropriate. Instead of defining a new LSM > hook, this patch defines security_kernel_read_blob() as a wrapper for > the existing LSM security_kernel_file_read() hook. What a marvelous opportunity to bikeshed! I really dislike adding another security_ interface just because the name isn't quite right. Especially a wrapper, which is just code and execution overhead. Why not change security_kernel_read_file() to security_kernel_read_blob() everywhere and be done? > > Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com> > Cc: Eric Biederman <ebiederm@xmission.com> > Cc: Luis R. Rodriguez <mcgrof@kernel.org> > Cc: Kees Cook <keescook@chromium.org> > Cc: David Howells <dhowells@redhat.com> > Cc: Casey Schaufler <casey@schaufler-ca.com> > > Changelog v2: > - Define a generic wrapper named security_kernel_read_blob() for > security_kernel_read_file(). > > Changelog v1: > - Define and call security_kexec_load(), a wrapper for > security_kernel_read_file(). > --- > include/linux/security.h | 6 ++++++ > security/security.c | 6 ++++++ > 2 files changed, 12 insertions(+) > > diff --git a/include/linux/security.h b/include/linux/security.h > index 63030c85ee19..4db1967a688b 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -323,6 +323,7 @@ int security_kernel_module_request(char *kmod_name); > int security_kernel_read_file(struct file *file, enum kernel_read_file_id id); > int security_kernel_post_read_file(struct file *file, char *buf, loff_t size, > enum kernel_read_file_id id); > +int security_kernel_read_blob(enum kernel_read_file_id id); > int security_task_fix_setuid(struct cred *new, const struct cred *old, > int flags); > int security_task_setpgid(struct task_struct *p, pid_t pgid); > @@ -922,6 +923,11 @@ static inline int security_kernel_post_read_file(struct file *file, > return 0; > } > > +static inline int security_kernel_read_blob(enum kernel_read_file_id id) > +{ > + return 0; > +} > + > static inline int security_task_fix_setuid(struct cred *new, > const struct cred *old, > int flags) > diff --git a/security/security.c b/security/security.c > index 68f46d849abe..8f199b2bf4a2 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -1044,6 +1044,12 @@ int security_kernel_read_file(struct file *file, enum kernel_read_file_id id) > } > EXPORT_SYMBOL_GPL(security_kernel_read_file); > > +int security_kernel_read_blob(enum kernel_read_file_id id) > +{ > + return security_kernel_read_file(NULL, id); > +} > +EXPORT_SYMBOL_GPL(security_kernel_read_blob); > + > int security_kernel_post_read_file(struct file *file, char *buf, loff_t size, > enum kernel_read_file_id id) > {
Casey Schaufler <casey@schaufler-ca.com> writes: > On 5/17/2018 7:48 AM, Mimi Zohar wrote: >> In order for LSMs and IMA-appraisal to differentiate between the original >> and new syscalls (eg. kexec, kernel modules, firmware), both the original >> and new syscalls must call an LSM hook. >> >> Commit 2e72d51b4ac3 ("security: introduce kernel_module_from_file hook") >> introduced calling security_kernel_module_from_file() in both the original >> and new syscalls. Commit a1db74209483 ("module: replace >> copy_module_from_fd with kernel version") replaced these LSM calls with >> security_kernel_read_file(). >> >> Commit e40ba6d56b41 ("firmware: replace call to fw_read_file_contents() >> with kernel version") and commit b804defe4297 ("kexec: replace call to >> copy_file_from_fd() with kernel version") replaced their own version of >> reading a file from the kernel with the generic >> kernel_read_file_from_path/fd() versions, which call the pre and post >> security_kernel_read_file LSM hooks. >> >> Missing are LSM calls in the original kexec syscall and firmware sysfs >> fallback method. From a technical perspective there is no justification >> for defining a new LSM hook, as the existing security_kernel_read_file() >> works just fine. The original syscalls, however, do not read a file, so >> the security hook name is inappropriate. Instead of defining a new LSM >> hook, this patch defines security_kernel_read_blob() as a wrapper for >> the existing LSM security_kernel_file_read() hook. > > What a marvelous opportunity to bikeshed! > > I really dislike adding another security_ interface just because > the name isn't quite right. Especially a wrapper, which is just > code and execution overhead. Why not change security_kernel_read_file() > to security_kernel_read_blob() everywhere and be done? Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com> Nack on this sharing nonsense. These two interfaces do not share any code in their implementations other than the if statement to distinguish between the two cases. Casey you are wrong. We need something different here. Mimi a wrapper does not cut it. The code is not shared. Despite using a single function call today. If we want comprehensible and maintainable code in the security modules we need to split these two pieces of functionality apart. Eric
On Thu, 2018-05-17 at 22:37 -0500, Eric W. Biederman wrote: > Casey Schaufler <casey@schaufler-ca.com> writes: > > > On 5/17/2018 7:48 AM, Mimi Zohar wrote: > >> In order for LSMs and IMA-appraisal to differentiate between the original > >> and new syscalls (eg. kexec, kernel modules, firmware), both the original > >> and new syscalls must call an LSM hook. > >> > >> Commit 2e72d51b4ac3 ("security: introduce kernel_module_from_file hook") > >> introduced calling security_kernel_module_from_file() in both the original > >> and new syscalls. Commit a1db74209483 ("module: replace > >> copy_module_from_fd with kernel version") replaced these LSM calls with > >> security_kernel_read_file(). > >> > >> Commit e40ba6d56b41 ("firmware: replace call to fw_read_file_contents() > >> with kernel version") and commit b804defe4297 ("kexec: replace call to > >> copy_file_from_fd() with kernel version") replaced their own version of > >> reading a file from the kernel with the generic > >> kernel_read_file_from_path/fd() versions, which call the pre and post > >> security_kernel_read_file LSM hooks. > >> > >> Missing are LSM calls in the original kexec syscall and firmware sysfs > >> fallback method. From a technical perspective there is no justification > >> for defining a new LSM hook, as the existing security_kernel_read_file() > >> works just fine. The original syscalls, however, do not read a file, so > >> the security hook name is inappropriate. Instead of defining a new LSM > >> hook, this patch defines security_kernel_read_blob() as a wrapper for > >> the existing LSM security_kernel_file_read() hook. > > > > What a marvelous opportunity to bikeshed! > > > > I really dislike adding another security_ interface just because > > the name isn't quite right. Especially a wrapper, which is just > > code and execution overhead. Why not change security_kernel_read_file() > > to security_kernel_read_blob() everywhere and be done? > > Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com> > > Nack on this sharing nonsense. These two interfaces do not share any > code in their implementations other than the if statement to distinguish > between the two cases. > > Casey you are wrong. We need something different here. > > Mimi a wrapper does not cut it. The code is not shared. Despite using > a single function call today. > > If we want comprehensible and maintainable code in the security modules > we need to split these two pieces of functionality apart. kernel_read_file() is a common, generic method of reading a file from the kernel, which is being called from a number of places. The kernel_read_file_id enumeration is needed to differentiate between the callers. The purpose of the new security_kernel_read_file() calls is not for the kernel to read a file, but as a method of identifying the original buffer based methods containing a file. Having to define a separate LSM hook for each of the original, non kernel_read_file(), buffer based method callers, kind of makes sense, as the callers themselves are specific, but is it really necessary? Could we define a new, generic LSM hook named security_kernel_buffer_data() for this purpose? Mimi
On 5/18/2018 4:30 AM, Mimi Zohar wrote: > On Thu, 2018-05-17 at 22:37 -0500, Eric W. Biederman wrote: >> Casey Schaufler <casey@schaufler-ca.com> writes: >> >>> On 5/17/2018 7:48 AM, Mimi Zohar wrote: >>>> In order for LSMs and IMA-appraisal to differentiate between the original >>>> and new syscalls (eg. kexec, kernel modules, firmware), both the original >>>> and new syscalls must call an LSM hook. >>>> >>>> Commit 2e72d51b4ac3 ("security: introduce kernel_module_from_file hook") >>>> introduced calling security_kernel_module_from_file() in both the original >>>> and new syscalls. Commit a1db74209483 ("module: replace >>>> copy_module_from_fd with kernel version") replaced these LSM calls with >>>> security_kernel_read_file(). >>>> >>>> Commit e40ba6d56b41 ("firmware: replace call to fw_read_file_contents() >>>> with kernel version") and commit b804defe4297 ("kexec: replace call to >>>> copy_file_from_fd() with kernel version") replaced their own version of >>>> reading a file from the kernel with the generic >>>> kernel_read_file_from_path/fd() versions, which call the pre and post >>>> security_kernel_read_file LSM hooks. >>>> >>>> Missing are LSM calls in the original kexec syscall and firmware sysfs >>>> fallback method. From a technical perspective there is no justification >>>> for defining a new LSM hook, as the existing security_kernel_read_file() >>>> works just fine. The original syscalls, however, do not read a file, so >>>> the security hook name is inappropriate. Instead of defining a new LSM >>>> hook, this patch defines security_kernel_read_blob() as a wrapper for >>>> the existing LSM security_kernel_file_read() hook. >>> What a marvelous opportunity to bikeshed! >>> >>> I really dislike adding another security_ interface just because >>> the name isn't quite right. Especially a wrapper, which is just >>> code and execution overhead. Why not change security_kernel_read_file() >>> to security_kernel_read_blob() everywhere and be done? >> Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com> >> >> Nack on this sharing nonsense. These two interfaces do not share any >> code in their implementations other than the if statement to distinguish >> between the two cases. >> >> Casey you are wrong. We need something different here. >> >> Mimi a wrapper does not cut it. The code is not shared. Despite using >> a single function call today. >> >> If we want comprehensible and maintainable code in the security modules >> we need to split these two pieces of functionality apart. > kernel_read_file() is a common, generic method of reading a file from > the kernel, which is being called from a number of places. The > kernel_read_file_id enumeration is needed to differentiate between the > callers. The purpose of the new security_kernel_read_file() calls is > not for the kernel to read a file, but as a method of identifying the > original buffer based methods containing a file. > > Having to define a separate LSM hook for each of the original, non > kernel_read_file(), buffer based method callers, kind of makes sense, > as the callers themselves are specific, but is it really necessary? > Could we define a new, generic LSM hook named > security_kernel_buffer_data() for this purpose? If there are two disparate behaviors wrapped into kernel_read_file() Eric (bless him) is right. It should be broken into two hooks. I think that if we look (too) carefully we'll find other places where hooks should get broken up, or combined*. My question is just how important is it that this gets changed? --- * calling security_inode_secid() and then immediately security_secid_to_secctx() grates on my sensibilities.
On Fri, 2018-05-18 at 07:58 -0700, Casey Schaufler wrote: > On 5/18/2018 4:30 AM, Mimi Zohar wrote: > > Having to define a separate LSM hook for each of the original, non > > kernel_read_file(), buffer based method callers, kind of makes sense, > > as the callers themselves are specific, but is it really necessary? > > Could we define a new, generic LSM hook named > > security_kernel_buffer_data() for this purpose? > > If there are two disparate behaviors wrapped into kernel_read_file() > Eric (bless him) is right. It should be broken into two hooks. I think > that if we look (too) carefully we'll find other places where hooks > should get broken up, or combined*. My question is just how important > is it that this gets changed? Other than the LSM call in copy_module_from_user(), this patch set is adding the LSM call in kexec_load() and firmware_fallback_sysfs(). Eric, the question remains whether we need distinct LSM hooks in each of these places or can we have a single, generic LSM hook named security_kernel_buffer_data()? Mimi
On Thu, 17 May 2018, Eric W. Biederman wrote: > Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com> > > Nack on this sharing nonsense. These two interfaces do not share any > code in their implementations other than the if statement to distinguish > between the two cases. Hmm, it's not even doing that. There's already an if(!file && read_id == X) { } check and this is another one being added. > If we want comprehensible and maintainable code in the security modules > we need to split these two pieces of functionality apart. All ima_read is doing in both the old and new case is checking if there's no file then if it's a certain operation, returning an error. To echo Eric and Casey's suggestions, how about changing the name of the hook to security_kernel_read_data() ? Then ima_read_file() can be changed to ima_read_data(), and then instead of two if (!file && read_id == X) checks, have: if (!file) { switch (read_id) { } }
On Sat, 2018-05-19 at 03:13 +1000, James Morris wrote: > On Thu, 17 May 2018, Eric W. Biederman wrote: > > > Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com> > > > > Nack on this sharing nonsense. These two interfaces do not share any > > code in their implementations other than the if statement to distinguish > > between the two cases. > > Hmm, it's not even doing that. > > There's already an if(!file && read_id == X) { } check and this is another > one being added. > > > If we want comprehensible and maintainable code in the security modules > > we need to split these two pieces of functionality apart. > > All ima_read is doing in both the old and new case is checking if there's > no file then if it's a certain operation, returning an error. > > To echo Eric and Casey's suggestions, how about changing the name of the > hook to security_kernel_read_data() ? Thanks, James. Somehow I missed this option. Renaming the existing hook, would be the easiest solution. Eric, are you in agreement with James' naming suggestion/solution? > Then ima_read_file() can be changed to ima_read_data(), and then instead > of two if (!file && read_id == X) checks, have: > > if (!file) { > switch (read_id) { > } > } > > > >
diff --git a/include/linux/security.h b/include/linux/security.h index 63030c85ee19..4db1967a688b 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -323,6 +323,7 @@ int security_kernel_module_request(char *kmod_name); int security_kernel_read_file(struct file *file, enum kernel_read_file_id id); int security_kernel_post_read_file(struct file *file, char *buf, loff_t size, enum kernel_read_file_id id); +int security_kernel_read_blob(enum kernel_read_file_id id); int security_task_fix_setuid(struct cred *new, const struct cred *old, int flags); int security_task_setpgid(struct task_struct *p, pid_t pgid); @@ -922,6 +923,11 @@ static inline int security_kernel_post_read_file(struct file *file, return 0; } +static inline int security_kernel_read_blob(enum kernel_read_file_id id) +{ + return 0; +} + static inline int security_task_fix_setuid(struct cred *new, const struct cred *old, int flags) diff --git a/security/security.c b/security/security.c index 68f46d849abe..8f199b2bf4a2 100644 --- a/security/security.c +++ b/security/security.c @@ -1044,6 +1044,12 @@ int security_kernel_read_file(struct file *file, enum kernel_read_file_id id) } EXPORT_SYMBOL_GPL(security_kernel_read_file); +int security_kernel_read_blob(enum kernel_read_file_id id) +{ + return security_kernel_read_file(NULL, id); +} +EXPORT_SYMBOL_GPL(security_kernel_read_blob); + int security_kernel_post_read_file(struct file *file, char *buf, loff_t size, enum kernel_read_file_id id) {
In order for LSMs and IMA-appraisal to differentiate between the original and new syscalls (eg. kexec, kernel modules, firmware), both the original and new syscalls must call an LSM hook. Commit 2e72d51b4ac3 ("security: introduce kernel_module_from_file hook") introduced calling security_kernel_module_from_file() in both the original and new syscalls. Commit a1db74209483 ("module: replace copy_module_from_fd with kernel version") replaced these LSM calls with security_kernel_read_file(). Commit e40ba6d56b41 ("firmware: replace call to fw_read_file_contents() with kernel version") and commit b804defe4297 ("kexec: replace call to copy_file_from_fd() with kernel version") replaced their own version of reading a file from the kernel with the generic kernel_read_file_from_path/fd() versions, which call the pre and post security_kernel_read_file LSM hooks. Missing are LSM calls in the original kexec syscall and firmware sysfs fallback method. From a technical perspective there is no justification for defining a new LSM hook, as the existing security_kernel_read_file() works just fine. The original syscalls, however, do not read a file, so the security hook name is inappropriate. Instead of defining a new LSM hook, this patch defines security_kernel_read_blob() as a wrapper for the existing LSM security_kernel_file_read() hook. Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com> Cc: Eric Biederman <ebiederm@xmission.com> Cc: Luis R. Rodriguez <mcgrof@kernel.org> Cc: Kees Cook <keescook@chromium.org> Cc: David Howells <dhowells@redhat.com> Cc: Casey Schaufler <casey@schaufler-ca.com> Changelog v2: - Define a generic wrapper named security_kernel_read_blob() for security_kernel_read_file(). Changelog v1: - Define and call security_kexec_load(), a wrapper for security_kernel_read_file(). --- include/linux/security.h | 6 ++++++ security/security.c | 6 ++++++ 2 files changed, 12 insertions(+)