Hi Linus,
Here are the security subsystem updates for 4.14. Highlights:
AppArmor:
- Add mediation of mountpoints and signals
- Add support for absolute root view based labels
- add base infastructure for socket mediation
LSM:
- Remove unused security_task_create() hook
TPM:
- Some constification and minor updates.
IMA:
- A new integrity_read file operation method, avoids races when
calculating file hashes
SELinux:
- from Paul Moore:
"A relatively quiet period for SELinux, 11 patches with only two/three
having any substantive changes. These noteworthy changes include
another tweak to the NNP/nosuid handling, per-file labeling for
cgroups, and an object class fix for AF_UNIX/SOCK_RAW sockets; the rest
of the changes are minor tweaks or administrative updates (Stephen's
email update explains the file explosion in the diffstat)."
Seccomp:
- from Kees Cook:
"Major additions:
- sysctl and seccomp operation to discover available actions. (tyhicks)
- new per-filter configurable logging infrastructure and sysctl. (tyhicks)
- SECCOMP_RET_LOG to log allowed syscalls. (tyhicks)
- SECCOMP_RET_KILL_PROCESS as the new strictest possible action.
- self-tests for new behaviors."
And nothing for Smack, for the first time perhaps.
Please pull.
---
The following changes since commit 81a84ad3cb5711cec79f4dd53a4ce026b092c432:
Merge branch 'docs-next' of git://git.lwn.net/linux (2017-09-03 21:07:29 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next
Antonio Murdaca (1):
selinux: allow per-file labeling for cgroupfs
Arvind Yadav (3):
tpm: tpm_crb: constify acpi_device_id.
tpm: vtpm: constify vio_device_id
selinux: constify nf_hook_ops
Christoph Hellwig (1):
ima: use fs method to read integrity data
Christos Gkekas (1):
apparmor: Fix logical error in verify_header()
Dan Carpenter (1):
apparmor: Fix an error code in aafs_create()
Enric Balletbo i Serra (1):
Documentation: tpm: add powered-while-suspended binding documentation
Geert Uytterhoeven (1):
apparmor: Fix shadowed local variable in unpack_trans_table()
Hamza Attak (1):
tpm: replace msleep() with usleep_range() in TPM 1.2/2.0 generic drivers
James Morris (3):
sync to Linus v4.13-rc2 for subsystem developers to work against
Merge tag 'seccomp-next' of git://git.kernel.org/.../kees/linux into next
Merge tag 'selinux-pr-20170831' of git://git.kernel.org/.../pcmoore/selinux into next
John Johansen (13):
apparmor: Redundant condition: prev_ns. in [label.c:1498]
apparmor: add the ability to mediate signals
apparmor: add mount mediation
apparmor: cleanup conditional check for label in label_print
apparmor: add support for absolute root view based labels
apparmor: make policy_unpack able to audit different info messages
apparmor: add more debug asserts to apparmorfs
apparmor: add base infastructure for socket mediation
apparmor: move new_null_profile to after profile lookup fns()
apparmor: fix race condition in null profile creation
apparmor: ensure unconfined profiles have dfas initialized
apparmor: fix incorrect type assignment when freeing proxies
apparmor: fix build failure on sparc caused by undeclared, signals
Kees Cook (9):
selftests/seccomp: Add tests for basic ptrace actions
selftests/seccomp: Add simple seccomp overhead benchmark
selftests/seccomp: Refactor RET_ERRNO tests
seccomp: Provide matching filter for introspection
seccomp: Rename SECCOMP_RET_KILL to SECCOMP_RET_KILL_THREAD
seccomp: Introduce SECCOMP_RET_KILL_PROCESS
seccomp: Implement SECCOMP_RET_KILL_PROCESS action
selftests/seccomp: Test thread vs process killing
samples: Unrename SECCOMP_RET_KILL
Luis Ressel (1):
selinux: Assign proper class to PF_UNIX/SOCK_RAW sockets
Michal Hocko (1):
selinux: use GFP_NOWAIT in the AVC kmem_caches
Michal Suchanek (1):
tpm: ibmvtpm: simplify crq initialization and document crq format
Mimi Zohar (6):
ima: don't remove the securityfs policy file
libfs: define simple_read_iter_from_buffer
efivarfs: replaces the read file operation with read_iter
ima: always measure and audit files in policy
ima: define "dont_failsafe" policy action rule
ima: define "fs_unsafe" builtin policy
Paul Moore (4):
credits: update Paul Moore's info
selinux: update the selinux info in MAINTAINERS
MAINTAINERS: update the NetLabel and Labeled Networking information
MAINTAINERS: update the NetLabel and Labeled Networking information
Stefan Berger (1):
security: fix description of values returned by cap_inode_need_killpriv
Stephen Smalley (4):
selinux: genheaders should fail if too many permissions are defined
selinux: Generalize support for NNP/nosuid SELinux domain transitions
selinux: update my email address
lsm_audit: update my email address
Tetsuo Handa (2):
LSM: Remove security_task_create() hook.
tomoyo: Update URLs in Documentation/admin-guide/LSM/tomoyo.rst
Tyler Hicks (6):
seccomp: Sysctl to display available actions
seccomp: Operation for checking if an action is available
seccomp: Sysctl to configure actions that are allowed to be logged
seccomp: Selftest for detection of filter flag support
seccomp: Filter flag to log all actions except SECCOMP_RET_ALLOW
seccomp: Action to log before allowing
CREDITS | 8 +-
Documentation/ABI/testing/ima_policy | 3 +-
Documentation/admin-guide/LSM/tomoyo.rst | 24 +-
Documentation/admin-guide/kernel-parameters.txt | 8 +-
.../devicetree/bindings/security/tpm/tpm-i2c.txt | 6 +
Documentation/networking/filter.txt | 2 +-
Documentation/sysctl/kernel.txt | 1 +
Documentation/userspace-api/seccomp_filter.rst | 52 ++-
MAINTAINERS | 29 +-
drivers/char/tpm/tpm-interface.c | 10 +-
drivers/char/tpm/tpm.h | 9 +-
drivers/char/tpm/tpm2-cmd.c | 2 +-
drivers/char/tpm/tpm_crb.c | 2 +-
drivers/char/tpm/tpm_ibmvtpm.c | 98 ++-
drivers/char/tpm/tpm_infineon.c | 6 +-
drivers/char/tpm/tpm_tis_core.c | 8 +-
fs/btrfs/file.c | 1 +
fs/efivarfs/file.c | 12 +-
fs/ext2/file.c | 17 +
fs/ext4/file.c | 20 +
fs/f2fs/file.c | 1 +
fs/jffs2/file.c | 1 +
fs/jfs/file.c | 1 +
fs/libfs.c | 32 +
fs/nilfs2/file.c | 1 +
fs/ramfs/file-mmu.c | 1 +
fs/ramfs/file-nommu.c | 1 +
fs/ubifs/file.c | 1 +
fs/xfs/xfs_file.c | 21 +
include/linux/audit.h | 6 +-
include/linux/fs.h | 3 +
include/linux/lsm_audit.h | 2 +-
include/linux/lsm_hooks.h | 7 -
include/linux/seccomp.h | 3 +-
include/linux/security.h | 6 -
include/uapi/linux/seccomp.h | 23 +-
kernel/fork.c | 4 -
kernel/seccomp.c | 321 +++++++++-
mm/shmem.c | 1 +
scripts/selinux/genheaders/genheaders.c | 7 +-
security/apparmor/.gitignore | 1 +
security/apparmor/Makefile | 43 ++-
security/apparmor/apparmorfs.c | 37 +-
security/apparmor/domain.c | 4 +-
security/apparmor/file.c | 30 +
security/apparmor/include/apparmor.h | 2 +
security/apparmor/include/audit.h | 39 +-
security/apparmor/include/domain.h | 5 +
security/apparmor/include/ipc.h | 6 +
security/apparmor/include/label.h | 1 +
security/apparmor/include/mount.h | 54 ++
security/apparmor/include/net.h | 114 ++++
security/apparmor/include/perms.h | 5 +-
security/apparmor/include/policy.h | 13 +
security/apparmor/include/sig_names.h | 98 +++
security/apparmor/ipc.c | 99 +++
security/apparmor/label.c | 36 +-
security/apparmor/lib.c | 5 +-
security/apparmor/lsm.c | 472 +++++++++++++
security/apparmor/mount.c | 696 ++++++++++++++++++++
security/apparmor/net.c | 184 ++++++
security/apparmor/policy.c | 166 +++---
security/apparmor/policy_ns.c | 2 +
security/apparmor/policy_unpack.c | 105 +++-
security/commoncap.c | 6 +-
security/integrity/iint.c | 20 +-
security/integrity/ima/ima.h | 1 +
security/integrity/ima/ima_api.c | 67 ++-
security/integrity/ima/ima_crypto.c | 10 +
security/integrity/ima/ima_fs.c | 4 +-
security/integrity/ima/ima_main.c | 19 +-
security/integrity/ima/ima_policy.c | 41 ++-
security/lsm_audit.c | 2 +-
security/security.c | 5 -
security/selinux/avc.c | 16 +-
security/selinux/hooks.c | 56 ++-
security/selinux/include/avc.h | 2 +-
security/selinux/include/avc_ss.h | 2 +-
security/selinux/include/classmap.h | 2 +
security/selinux/include/objsec.h | 2 +-
security/selinux/include/security.h | 4 +-
security/selinux/ss/avtab.c | 2 +-
security/selinux/ss/avtab.h | 2 +-
security/selinux/ss/constraint.h | 2 +-
security/selinux/ss/context.h | 2 +-
security/selinux/ss/ebitmap.c | 2 +-
security/selinux/ss/ebitmap.h | 2 +-
security/selinux/ss/hashtab.c | 2 +-
security/selinux/ss/hashtab.h | 2 +-
security/selinux/ss/mls.c | 2 +-
security/selinux/ss/mls.h | 2 +-
security/selinux/ss/mls_types.h | 2 +-
security/selinux/ss/policydb.c | 2 +-
security/selinux/ss/policydb.h | 2 +-
security/selinux/ss/services.c | 9 +-
security/selinux/ss/services.h | 2 +-
security/selinux/ss/sidtab.c | 2 +-
security/selinux/ss/sidtab.h | 2 +-
security/selinux/ss/symtab.c | 2 +-
security/selinux/ss/symtab.h | 2 +-
tools/testing/selftests/seccomp/Makefile | 18 +-
.../testing/selftests/seccomp/seccomp_benchmark.c | 99 +++
tools/testing/selftests/seccomp/seccomp_bpf.c | 610 +++++++++++++++---
103 files changed, 3540 insertions(+), 469 deletions(-)
create mode 100644 security/apparmor/include/mount.h
create mode 100644 security/apparmor/include/net.h
create mode 100644 security/apparmor/include/sig_names.h
create mode 100644 security/apparmor/mount.c
create mode 100644 security/apparmor/net.c
create mode 100644 tools/testing/selftests/seccomp/seccomp_benchmark.c
On Mon, Sep 4, 2017 at 3:29 AM, James Morris <[email protected]> wrote:
>
> IMA:
> - A new integrity_read file operation method, avoids races when
> calculating file hashes
Honestly, this seems really odd.
It documents that it needs to be called with i_rwsem held exclusively,
and even has a lockdep assert to that effect (well, not really: the
code claims "exclusive", but the lockdep assert does not), but I'm not
actually seeing anybody doing it.
Quite the reverse, I just see integrity_read_file() doing filp_open()
on the pathname and passing it to integrity_kernel_read() with no
locking.
It really looks like just pure garbage to me. I pulled, and I'm not
unpulling the whole thing. I don't think it's been tested, and I don't
think it can be right.
Tell me why I'm wrong, or tell me why that garbage made it in in the
first place?
Linus
On Thu, 7 Sep 2017, Linus Torvalds wrote:
> On Mon, Sep 4, 2017 at 3:29 AM, James Morris <[email protected]> wrote:
> >
> > IMA:
> > - A new integrity_read file operation method, avoids races when
> > calculating file hashes
>
> Honestly, this seems really odd.
>
> It documents that it needs to be called with i_rwsem held exclusively,
> and even has a lockdep assert to that effect (well, not really: the
> code claims "exclusive", but the lockdep assert does not), but I'm not
> actually seeing anybody doing it.
>
> Quite the reverse, I just see integrity_read_file() doing filp_open()
> on the pathname and passing it to integrity_kernel_read() with no
> locking.
>
> It really looks like just pure garbage to me. I pulled, and I'm not
> unpulling the whole thing. I don't think it's been tested, and I don't
> think it can be right.
>
> Tell me why I'm wrong, or tell me why that garbage made it in in the
> first place?
Mimi and Christoph worked together on this over several iterations -- I'll
let them respond.
--
James Morris
<[email protected]>
The reason why I send out the original version of this patch
is because IMA used to call ->read under i_rwsem, and that deadlocked
on XFS and NFS, or ext3/4 with DAX. The call path for that is
process_measurement (takes i_rwsem)
-> ima_collect_measurement
-> ima_calc_file_hash
-> ima_calc_file_ahash / ima_calc_file_shash
-> ima_calc_file_hash_atfm / ima_calc_file_hash_tfm
-> integrity_kernel_read
ima_check_last_writer (takes i_rwsem)
-> ima_update_xattr
-> ima_collect_measurement
-> (as above)
But yes, for the init-time integrity_read_file this is incorrect.
It never tripped up, and I explicitly added the lockdep annotations
so that anything would show up, and it's been half a year since
I sent that first RFC patch..
On Fri, Sep 8, 2017 at 12:09 AM, Christoph Hellwig <[email protected]> wrote:
>
> But yes, for the init-time integrity_read_file this is incorrect.
> It never tripped up, and I explicitly added the lockdep annotations
> so that anything would show up, and it's been half a year since
> I sent that first RFC patch..
I don't think anybody actually tests linux-next kernels in any big
way, and the automated tests that do get run probably don't run with
any integrity checking enabled.
Which is why I actually look at the code when merging unexpected stuff.
This is also why I tend to prefer getting multiple branches for
independent things.
Now the whole security pull will be ignored because of this thing. I
refuse to pull garbage where I notice major fundamental problems in
code that has obviously never ever been tested.
Side note: one of the reasons why I _looked_ at this code was because
the exclusive lock requirement was entirely unexplained in the first
place.
Linus
On Fri, Sep 8, 2017 at 1:25 PM, Linus Torvalds
<[email protected]> wrote:
> On Fri, Sep 8, 2017 at 12:09 AM, Christoph Hellwig <[email protected]> wrote:
>>
>> But yes, for the init-time integrity_read_file this is incorrect.
>> It never tripped up, and I explicitly added the lockdep annotations
>> so that anything would show up, and it's been half a year since
>> I sent that first RFC patch..
>
> I don't think anybody actually tests linux-next kernels in any big
> way, and the automated tests that do get run probably don't run with
> any integrity checking enabled.
>
> Which is why I actually look at the code when merging unexpected stuff.
>
> This is also why I tend to prefer getting multiple branches for
> independent things.
>
> Now the whole security pull will be ignored because of this thing. I
> refuse to pull garbage where I notice major fundamental problems in
> code that has obviously never ever been tested.
Is it time to start sending pull request for each LSM and thing under
security/ directly? I'm not sure I have a strong preference either
way, I just don't want to see the SELinux changes ignored during the
merge window.
--
paul moore
http://www.paul-moore.com
On Fri, 8 Sep 2017, Linus Torvalds wrote:
> Now the whole security pull will be ignored because of this thing. I
> refuse to pull garbage where I notice major fundamental problems in
> code that has obviously never ever been tested.
If I revert the change from my my tree, will you pull it then?
--
James Morris
<[email protected]>
On Fri, Sep 08, 2017 at 02:48:51PM +1000, James Morris wrote:
>
> Mimi and Christoph worked together on this over several iterations -- I'll
> let them respond.
Mimi --- we should chat next week in LA. I've been working on a
design internally at work which proposes a generic VFS-layer library
(ala how fscrypt in fs/crypto works) which provides data integrity
using per-file Merkle trees.
The goals of this design:
* Simplicity; for ease in security review and upstream review and
acceptance
* Useful for multiple use cases. It is *not* Android/APK specific,
and indeed can be used for other things
* A better way of providing Linux IMA/EVM support for immutable
files by moving the verification from time-of-open to
time-of-readpage. (This significantly reduces the performance
impact, since we don't need to lock down the file while the kernel
needs to run SHA1 on potentially gigabytes worth of file data.)
* Most use cases for file-level checksums are for files that
don’t change over time (e.g., for Video, Audio, Backup files,
etc.) This allows us to provide a cheap and efficient way to
provide checksum protect against storage-level corruption
fairly easily. So by supporting both SHA and CRC-32, we can
make this feature useful for more than just the security heads. :-)
* Like the encryption/fscrypt feature, most of the code to this
feature can be in a VFS-level library, with minimal hooks needed
to those file systems (ext4, f2fs) that wish to provide this
functionality.
- Ted
On Fri, 8 Sep 2017, Theodore Ts'o wrote:
> On Fri, Sep 08, 2017 at 02:48:51PM +1000, James Morris wrote:
> >
> > Mimi and Christoph worked together on this over several iterations -- I'll
> > let them respond.
>
> Mimi --- we should chat next week in LA. I've been working on a
> design internally at work which proposes a generic VFS-layer library
> (ala how fscrypt in fs/crypto works) which provides data integrity
> using per-file Merkle trees.
It's possible we could add a BoF at LSS on the Thursday (from 5:45pm), or
even Friday afternoon:
http://events.linuxfoundation.org/events/linux-security-summit/program/schedule
Let me know if you want to schedule something.
--
James Morris
<[email protected]>
On Fri, 8 Sep 2017, Paul Moore wrote:
> > This is also why I tend to prefer getting multiple branches for
> > independent things.
[...]
>
> Is it time to start sending pull request for each LSM and thing under
> security/ directly? I'm not sure I have a strong preference either
> way, I just don't want to see the SELinux changes ignored during the
> merge window.
They won't be ignored, we just need to get this issue resolved now and
figure out how to implement multiple branches in the security tree.
Looking at other git repos, the x86 folk have multiple branches.
One option for me would be to publish the trees I pull from as branches
along side mine, with 'next' being a merge of all of directly applied
patchsets and those ready for Linus to pull as one.
So, branches in
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security
might be:
next-selinux (Paul's next branch)
next-apparmor-next (JJ's next branch)
next-integrity-next (Mimi's)
next-tpm-next (Jarkko's)
[etc.]
next (merge all of the above to here)
That way, we have a coherent 'next' branch for people to develop against
and to push to Linus, but he can pull individual branches feeding into it
if something is broken in one of them.
Does that sound useful?
--
James Morris
<[email protected]>
On Sun, 10 Sep 2017, James Morris wrote:
> next-apparmor-next (JJ's next branch)
> next-integrity-next (Mimi's)
> next-tpm-next (Jarkko's)
without '-next' on the end... (editing while jetlagged).
--
James Morris
<[email protected]>
On Thu, 2017-09-07 at 11:19 -0700, Linus Torvalds wrote:
> On Mon, Sep 4, 2017 at 3:29 AM, James Morris <[email protected]> wrote:
> >
> > IMA:
> > - A new integrity_read file operation method, avoids races when
> > calculating file hashes
>
> Honestly, this seems really odd.
>
> It documents that it needs to be called with i_rwsem held exclusively,
> and even has a lockdep assert to that effect (well, not really: the
> code claims "exclusive", but the lockdep assert does not), but I'm not
> actually seeing anybody doing it.
>
> Quite the reverse, I just see integrity_read_file() doing filp_open()
> on the pathname and passing it to integrity_kernel_read() with no
> locking.
>
> It really looks like just pure garbage to me. I pulled, and I'm not
> unpulling the whole thing. I don't think it's been tested, and I don't
> think it can be right.
>
> Tell me why I'm wrong, or tell me why that garbage made it in in the
> first place?
I'm really sorry for the long delay in responding. I've been on
vacation the last week, mostly without cell phone and very limited
wifi access.
True, there is a side case where integrity_read_file() is being called
without first taking the i_rwsem. This side case permits signed x509
certificates to be loaded onto the trusted IMA/EVM keyrings, without
verifying the file signature stored as security.ima/security.evm
xattrs. Basically, the xattr signatures can not be verified until the
keys are loaded. The main use case is embedded systems which do not
have an initramfs, but have a specially crafted init script. It
requires enabling CONFIG_IMA_LOAD_X509 or CONFIG_EVM_LOAD_X509. The
new VFS integrity_read() file operation method would not be called.
The main use case for the new VFS integrity_read() file operation
method is to calculate the file hash, as Christoph described.
Mimi
On Fri, 2017-09-08 at 18:38 -0400, Theodore Ts'o wrote:
> On Fri, Sep 08, 2017 at 02:48:51PM +1000, James Morris wrote:
> >
> > Mimi and Christoph worked together on this over several iterations -- I'll
> > let them respond.
>
> Mimi --- we should chat next week in LA. I've been working on a
> design internally at work which proposes a generic VFS-layer library
> (ala how fscrypt in fs/crypto works) which provides data integrity
> using per-file Merkle trees.
>
> The goals of this design:
>
> * Simplicity; for ease in security review and upstream review and
> acceptance
> * Useful for multiple use cases. It is *not* Android/APK specific,
> and indeed can be used for other things
> * A better way of providing Linux IMA/EVM support for immutable
> files by moving the verification from time-of-open to
> time-of-readpage. (This significantly reduces the performance
> impact, since we don't need to lock down the file while the kernel
> needs to run SHA1 on potentially gigabytes worth of file data.)
> * Most use cases for file-level checksums are for files that
> don’t change over time (e.g., for Video, Audio, Backup files,
> etc.) This allows us to provide a cheap and efficient way to
> provide checksum protect against storage-level corruption
> fairly easily. So by supporting both SHA and CRC-32, we can
> make this feature useful for more than just the security heads. :-)
> * Like the encryption/fscrypt feature, most of the code to this
> feature can be in a VFS-level library, with minimal hooks needed
> to those file systems (ext4, f2fs) that wish to provide this
> functionality.
>From a file integrity perspective this would be interesting, but that
only addresses IMA-appraisal, not IMA-integrity or IMA-audit. We
would still need to calculate the file hash to be included in the
measurement list and used for auditing.
Have you done any work on protecting the directory information itself
(eg. file names) using Merkle trees?
Mimi
On Fri, Sep 08, 2017 at 10:25:53AM -0700, Linus Torvalds wrote:
> I don't think anybody actually tests linux-next kernels in any big
> way, and the automated tests that do get run probably don't run with
> any integrity checking enabled.
Well, for the atual IMA deadlock issue I asked Mimi to produce automated
tests and we get started on it. I was pretty pissed about the
assumptions IMA made on the fs, whch weren't documented or automatically
tested - coming from the XFS background where we want all our features
to run through automated tests that was just not how I'd expect thing
to work.
But now as part of that I messed up the other caller of it, so I
shouldn't complain too loud..
That being said - I really hink the certificate loading should not
even go thorught this whole call path, but use our common helper to
load a file into a buffer. Something like the patch below, I'm just
not sure if the last policy argument is what people want or if we'd need
to add a new policy type for certificates.
---
>From 3092ca95dfeff0dbd4e03ae62a51762facb15592 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <[email protected]>
Date: Sun, 10 Sep 2017 09:49:45 +0200
Subject: security/digisig: read file using kernel_read_file_from_path
This avoid using the new integrity_read file operation which requires
i_rwsem already to be held, and avoids a lot of code duplication and
call the proper LSM hooks.
[also constifies the path argument to kernel_read_file_from_path,
as the callers needs it. Should probably be split into a separate patch]
Signed-off-by: Christoph Hellwig <[email protected]>
---
fs/exec.c | 2 +-
include/linux/fs.h | 2 +-
security/integrity/digsig.c | 8 ++++---
security/integrity/iint.c | 49 ------------------------------------------
security/integrity/integrity.h | 2 --
5 files changed, 7 insertions(+), 56 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index 01a9fb9d8ac3..957a8ce294af 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -974,7 +974,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
}
EXPORT_SYMBOL_GPL(kernel_read_file);
-int kernel_read_file_from_path(char *path, void **buf, loff_t *size,
+int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
loff_t max_size, enum kernel_read_file_id id)
{
struct file *file;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 2d0e6748e46e..58855daba1eb 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2791,7 +2791,7 @@ static inline const char *kernel_read_file_id_str(enum kernel_read_file_id id)
extern int kernel_read(struct file *, loff_t, char *, unsigned long);
extern int kernel_read_file(struct file *, void **, loff_t *, loff_t,
enum kernel_read_file_id);
-extern int kernel_read_file_from_path(char *, void **, loff_t *, loff_t,
+extern int kernel_read_file_from_path(const char *, void **, loff_t *, loff_t,
enum kernel_read_file_id);
extern int kernel_read_file_from_fd(int, void **, loff_t *, loff_t,
enum kernel_read_file_id);
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 06554c448dce..8112cdeeee3c 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -112,13 +112,15 @@ int __init integrity_init_keyring(const unsigned int id)
int __init integrity_load_x509(const unsigned int id, const char *path)
{
key_ref_t key;
- char *data;
+ void *data = NULL;
+ loff_t size;
int rc;
if (!keyring[id])
return -EINVAL;
- rc = integrity_read_file(path, &data);
+ rc = kernel_read_file_from_path(path, data, &size,
+ 0, READING_POLICY);
if (rc < 0)
return rc;
@@ -139,6 +141,6 @@ int __init integrity_load_x509(const unsigned int id, const char *path)
key_ref_to_ptr(key)->description, path);
key_ref_put(key);
}
- kfree(data);
+ vfree(data);
return 0;
}
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index 6fc888ca468e..c84e05866052 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -200,55 +200,6 @@ int integrity_kernel_read(struct file *file, loff_t offset,
}
/*
- * integrity_read_file - read entire file content into the buffer
- *
- * This is function opens a file, allocates the buffer of required
- * size, read entire file content to the buffer and closes the file
- *
- * It is used only by init code.
- *
- */
-int __init integrity_read_file(const char *path, char **data)
-{
- struct file *file;
- loff_t size;
- char *buf;
- int rc = -EINVAL;
-
- if (!path || !*path)
- return -EINVAL;
-
- file = filp_open(path, O_RDONLY, 0);
- if (IS_ERR(file)) {
- rc = PTR_ERR(file);
- pr_err("Unable to open file: %s (%d)", path, rc);
- return rc;
- }
-
- size = i_size_read(file_inode(file));
- if (size <= 0)
- goto out;
-
- buf = kmalloc(size, GFP_KERNEL);
- if (!buf) {
- rc = -ENOMEM;
- goto out;
- }
-
- rc = integrity_kernel_read(file, 0, buf, size);
- if (rc == size) {
- *data = buf;
- } else {
- kfree(buf);
- if (rc >= 0)
- rc = -EIO;
- }
-out:
- fput(file);
- return rc;
-}
-
-/*
* integrity_load_keys - load integrity keys hook
*
* Hooks is called from init/main.c:kernel_init_freeable()
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index a53e7e4ab06c..e1bf040fb110 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -120,8 +120,6 @@ struct integrity_iint_cache *integrity_iint_find(struct inode *inode);
int integrity_kernel_read(struct file *file, loff_t offset,
void *addr, unsigned long count);
-int __init integrity_read_file(const char *path, char **data);
-
#define INTEGRITY_KEYRING_EVM 0
#define INTEGRITY_KEYRING_IMA 1
#define INTEGRITY_KEYRING_MODULE 2
--
2.11.0
On Sun, Sep 10, 2017 at 03:13:23AM -0400, Mimi Zohar wrote:
>
> From a file integrity perspective this would be interesting, but that
> only addresses IMA-appraisal, not IMA-integrity or IMA-audit. ?We
> would still need to calculate the file hash to be included in the
> measurement list and used for auditing.
>
> Have you done any work on protecting the directory information itself
> (eg. file names) using Merkle trees?
I have not, because the problem that I was trying to address was
primarily concerned with immutable files. I did do some brainstorming
about adding the filename into the data integrity header to prevent
someone who had direct access to the flash exchanging the inode
numbers for "rm" and "ls", such that if you had a policy which
required that all ELF executables be signed, that a trickster couldn't
cause the user to run "rm" when they meant to run, say, "ls", just for
the lulz. :-)
But that would break various symlink or hardlinks that are
legitimately present (e.g., /sbin/mkfs.ext[234] being a link to
/sbin/mke2fs), and if the adversary can carry out a chip-off attack
against your root file system, (a) it's not clear how much this would
help, and (b) this is really what dm-verity is for.
The main security problem I was looking at is one where the system
image is already protected using dm-verity, but you might have (for
example) some APK files which are downloaded once and stored in some
shared-user directory hierarchy (so file-based encryption can't even
provide pretend integrity protection), integrity checked at download
time, and never checked again. Adding some kind of per-file Merkle
tree might be useful in that particular use case.
Cheers,
- Ted
On Sun, 2017-09-10 at 01:10 -0700, Christoph Hellwig wrote:
> On Fri, Sep 08, 2017 at 10:25:53AM -0700, Linus Torvalds wrote:
> > I don't think anybody actually tests linux-next kernels in any big
> > way, and the automated tests that do get run probably don't run with
> > any integrity checking enabled.
>
> Well, for the atual IMA deadlock issue I asked Mimi to produce automated
> tests and we get started on it. I was pretty pissed about the
> assumptions IMA made on the fs, whch weren't documented or automatically
> tested - coming from the XFS background where we want all our features
> to run through automated tests that was just not how I'd expect thing
> to work.
>
> But now as part of that I messed up the other caller of it, so I
> shouldn't complain too loud..
>
> That being said - I really hink the certificate loading should not
> even go thorught this whole call path, but use our common helper to
> load a file into a buffer. Something like the patch below, I'm just
> not sure if the last policy argument is what people want or if we'd need
> to add a new policy type for certificates.
We need to differentiate between policies and x509 certificates. In
the policy case, they need to be signed and appraised, while in the
x509 certificate case, the certificate itself is signed so the file
doesn't need to be signed or verified.
>
> ---
> From 3092ca95dfeff0dbd4e03ae62a51762facb15592 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <[email protected]>
> Date: Sun, 10 Sep 2017 09:49:45 +0200
> Subject: security/digisig: read file using kernel_read_file_from_path
>
> This avoid using the new integrity_read file operation which requires
> i_rwsem already to be held, and avoids a lot of code duplication and
> call the proper LSM hooks.
>
> [also constifies the path argument to kernel_read_file_from_path,
> as the callers needs it. Should probably be split into a separate patch]
>
> Signed-off-by: Christoph Hellwig <[email protected]>
> ---
> fs/exec.c | 2 +-
> include/linux/fs.h | 2 +-
> security/integrity/digsig.c | 8 ++++---
> security/integrity/iint.c | 49 ------------------------------------------
> security/integrity/integrity.h | 2 --
> 5 files changed, 7 insertions(+), 56 deletions(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 01a9fb9d8ac3..957a8ce294af 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -974,7 +974,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
> }
> EXPORT_SYMBOL_GPL(kernel_read_file);
>
> -int kernel_read_file_from_path(char *path, void **buf, loff_t *size,
> +int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
> loff_t max_size, enum kernel_read_file_id id)
> {
> struct file *file;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 2d0e6748e46e..58855daba1eb 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2791,7 +2791,7 @@ static inline const char *kernel_read_file_id_str(enum kernel_read_file_id id)
> extern int kernel_read(struct file *, loff_t, char *, unsigned long);
> extern int kernel_read_file(struct file *, void **, loff_t *, loff_t,
> enum kernel_read_file_id);
> -extern int kernel_read_file_from_path(char *, void **, loff_t *, loff_t,
> +extern int kernel_read_file_from_path(const char *, void **, loff_t *, loff_t,
> enum kernel_read_file_id);
> extern int kernel_read_file_from_fd(int, void **, loff_t *, loff_t,
> enum kernel_read_file_id);
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index 06554c448dce..8112cdeeee3c 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -112,13 +112,15 @@ int __init integrity_init_keyring(const unsigned int id)
> int __init integrity_load_x509(const unsigned int id, const char *path)
> {
> key_ref_t key;
> - char *data;
> + void *data = NULL;
> + loff_t size;
> int rc;
>
> if (!keyring[id])
> return -EINVAL;
>
> - rc = integrity_read_file(path, &data);
> + rc = kernel_read_file_from_path(path, data, &size,
> + 0, READING_POLICY);
READING_X509_CERT?
> if (rc < 0)
> return rc;
>
> @@ -139,6 +141,6 @@ int __init integrity_load_x509(const unsigned int id, const char *path)
> key_ref_to_ptr(key)->description, path);
> key_ref_put(key);
> }
> - kfree(data);
> + vfree(data);
> return 0;
> }
> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index 6fc888ca468e..c84e05866052 100644
> --- a/security/integrity/iint.c
> +++ b/security/integrity/iint.c
> @@ -200,55 +200,6 @@ int integrity_kernel_read(struct file *file, loff_t offset,
> }
>
> /*
> - * integrity_read_file - read entire file content into the buffer
> - *
> - * This is function opens a file, allocates the buffer of required
> - * size, read entire file content to the buffer and closes the file
> - *
> - * It is used only by init code.
> - *
> - */
> -int __init integrity_read_file(const char *path, char **data)
> -{
> - struct file *file;
> - loff_t size;
> - char *buf;
> - int rc = -EINVAL;
> -
> - if (!path || !*path)
> - return -EINVAL;
> -
> - file = filp_open(path, O_RDONLY, 0);
> - if (IS_ERR(file)) {
> - rc = PTR_ERR(file);
> - pr_err("Unable to open file: %s (%d)", path, rc);
> - return rc;
> - }
> -
> - size = i_size_read(file_inode(file));
> - if (size <= 0)
> - goto out;
> -
> - buf = kmalloc(size, GFP_KERNEL);
> - if (!buf) {
> - rc = -ENOMEM;
> - goto out;
> - }
> -
> - rc = integrity_kernel_read(file, 0, buf, size);
> - if (rc == size) {
> - *data = buf;
> - } else {
> - kfree(buf);
> - if (rc >= 0)
> - rc = -EIO;
> - }
> -out:
> - fput(file);
> - return rc;
> -}
> -
> -/*
> * integrity_load_keys - load integrity keys hook
> *
> * Hooks is called from init/main.c:kernel_init_freeable()
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index a53e7e4ab06c..e1bf040fb110 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -120,8 +120,6 @@ struct integrity_iint_cache *integrity_iint_find(struct inode *inode);
> int integrity_kernel_read(struct file *file, loff_t offset,
> void *addr, unsigned long count);
>
> -int __init integrity_read_file(const char *path, char **data);
> -
> #define INTEGRITY_KEYRING_EVM 0
> #define INTEGRITY_KEYRING_IMA 1
> #define INTEGRITY_KEYRING_MODULE 2
On Sun, Sep 10, 2017 at 10:02:42AM -0400, Mimi Zohar wrote:
> We need to differentiate between policies and x509 certificates. ?In
> the policy case, they need to be signed and appraised, while in the
> x509 certificate case, the certificate itself is signed so the file
> doesn't need to be signed or verified.
How about you take this sketch over - I don't know much about the
integrity code, and it seems like you actually wrote
kernel_read_file_from_path as well - so you're at least 3 times as
qualified as I am in this area..
On Sun, 2017-09-10 at 23:38 -0700, Christoph Hellwig wrote:
> On Sun, Sep 10, 2017 at 10:02:42AM -0400, Mimi Zohar wrote:
> > We need to differentiate between policies and x509 certificates. In
> > the policy case, they need to be signed and appraised, while in the
> > x509 certificate case, the certificate itself is signed so the file
> > doesn't need to be signed or verified.
>
> How about you take this sketch over - I don't know much about the
> integrity code, and it seems like you actually wrote
> kernel_read_file_from_path as well - so you're at least 3 times as
> qualified as I am in this area..
Sure, it's been on my to do list.
Mimi
On Sun, Sep 10, 2017 at 12:32 AM, James Morris <[email protected]> wrote:
> On Fri, 8 Sep 2017, Paul Moore wrote:
>
>> > This is also why I tend to prefer getting multiple branches for
>> > independent things.
>
> [...]
>
>>
>> Is it time to start sending pull request for each LSM and thing under
>> security/ directly? I'm not sure I have a strong preference either
>> way, I just don't want to see the SELinux changes ignored during the
>> merge window.
>
> They won't be ignored, we just need to get this issue resolved now and
> figure out how to implement multiple branches in the security tree.
Once again, I don't really care too much either way. My only selfish
motivation is to make it as frictionless as possible to get the
SELinux tree merged into Linus' tree.
> Looking at other git repos, the x86 folk have multiple branches.
I don't really understand what advantage one repo with multiple
branches has over multiple repos, e.g. Linus' just pulling from the
individual LSM trees directly. I suppose one could make an argument
about linux-next, but I know they prefer to pull from the individual
repos directly (they pull selinux/next directly). Is it to help
reduce the load on Linus?
>From my perspective, the linux-security tree only introduces another
opportunity for things to go wrong during the merge window (as
evidenced by this latest snafu). Help me understand why a single tree
with multiple branches is beneficial to multiple trees?
Also, to be clear, I'm not picking on IMA or Mimi; this could have
easily been SELinux screwing things up for IMA (or Smack, or AppArmor,
etc.).
> One option for me would be to publish the trees I pull from as branches
> along side mine, with 'next' being a merge of all of directly applied
> patchsets and those ready for Linus to pull as one.
>
> So, branches in
> git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security
>
> might be:
>
> next-selinux (Paul's next branch)
> next-apparmor-next (JJ's next branch)
> next-integrity-next (Mimi's)
> next-tpm-next (Jarkko's)
> [etc.]
>
> next (merge all of the above to here)
>
> That way, we have a coherent 'next' branch for people to develop against
> and to push to Linus, but he can pull individual branches feeding into it
> if something is broken in one of them.
>
> Does that sound useful?
--
paul moore
http://www.paul-moore.com
On Sat, Sep 9, 2017 at 9:32 PM, James Morris <[email protected]> wrote:
> On Fri, 8 Sep 2017, Paul Moore wrote:
>
>> > This is also why I tend to prefer getting multiple branches for
>> > independent things.
>
> [...]
>
>>
>> Is it time to start sending pull request for each LSM and thing under
>> security/ directly? I'm not sure I have a strong preference either
>> way, I just don't want to see the SELinux changes ignored during the
>> merge window.
>
> They won't be ignored, we just need to get this issue resolved now and
> figure out how to implement multiple branches in the security tree.
>
> Looking at other git repos, the x86 folk have multiple branches.
Yeah, the x86 approach is what inspired my tree layout.
> One option for me would be to publish the trees I pull from as branches
> along side mine, with 'next' being a merge of all of directly applied
> patchsets and those ready for Linus to pull as one.
>
> So, branches in
> git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security
>
> might be:
>
> next-selinux (Paul's next branch)
> next-apparmor (JJ's next branch)
> next-integrity (Mimi's)
> next-tpm (Jarkko's)
> [etc.]
>
> next (merge all of the above to here)
>
> That way, we have a coherent 'next' branch for people to develop against
> and to push to Linus, but he can pull individual branches feeding into it
> if something is broken in one of them.
>
> Does that sound useful?
This is what I do with the KSPP tree (since it has a few unrelated
things in it), but you run the risk of getting too fine-grain and
creating dependencies between trees (e.g. adding a new hook that two
LSMs implement means either they depend on each other or both depend
on some third "core" tree).
How separable are the patches, normally?
-Kees
--
Kees Cook
Pixel Security
On Thu, 14 Sep 2017, Kees Cook wrote:
> How separable are the patches, normally?
They're usually mostly separate, but may depend on some core LSM change,
or similar.
Casey has mentioned off-list that it is useful to have a coherent up to
date security branch to work against when making core LSM changes.
--
James Morris
<[email protected]>
On Thu, Sep 14, 2017 at 2:13 PM, James Morris <[email protected]> wrote:
> On Thu, 14 Sep 2017, Kees Cook wrote:
>
>> How separable are the patches, normally?
>
> They're usually mostly separate, but may depend on some core LSM change,
> or similar.
>
> Casey has mentioned off-list that it is useful to have a coherent up to
> date security branch to work against when making core LSM changes.
Yeah, for sure. This "merge all topics" tree is what I have for KSPP
(and it's what I hand to -next).
-Kees
--
Kees Cook
Pixel Security
On Sat, 2017-09-09 at 05:57 +1000, James Morris wrote:
> On Fri, 8 Sep 2017, Linus Torvalds wrote:
>
> > Now the whole security pull will be ignored because of this thing. I
> > refuse to pull garbage where I notice major fundamental problems in
> > code that has obviously never ever been tested.
>
> If I revert the change from my my tree, will you pull it then?
Sigh, none of the patches, other than those included in Paul's pull
request, are in 4.14-rc1. I feel really horrible!
Linus, there would never have been a good time for something like this
to happen, but this past week, in case you didn't realize, most of us
were at the Linux Security Summit. The timing couldn't have been
worse.
Mimi