* [PATCH 5/7] keys: Make __key_link_begin() handle lockdep nesting
From: David Howells @ 2019-05-22 22:28 UTC (permalink / raw)
To: keyrings; +Cc: dhowells, linux-security-module, linux-kernel
In-Reply-To: <155856408314.10428.17035328117829912815.stgit@warthog.procyon.org.uk>
Make __key_link_begin() handle lockdep nesting for the implementation of
key_move() where we have to lock two keyrings.
Signed-off-by: David Howells <dhowells@redhat.com>
---
security/keys/internal.h | 2 +-
security/keys/key.c | 6 +++---
security/keys/keyring.c | 6 +++---
security/keys/request_key.c | 2 +-
4 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 8f533c81aa8d..93513667ff9a 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -93,7 +93,7 @@ extern wait_queue_head_t request_key_conswq;
extern struct key_type *key_type_lookup(const char *type);
extern void key_type_put(struct key_type *ktype);
-extern int __key_link_begin(struct key *keyring,
+extern int __key_link_begin(struct key *keyring, unsigned int lock_nesting,
const struct keyring_index_key *index_key,
struct assoc_array_edit **_edit);
extern int __key_link_check_live_key(struct key *keyring, struct key *key);
diff --git a/security/keys/key.c b/security/keys/key.c
index 696f1c092c50..e0750bc85b68 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -515,7 +515,7 @@ int key_instantiate_and_link(struct key *key,
}
if (keyring) {
- ret = __key_link_begin(keyring, &key->index_key, &edit);
+ ret = __key_link_begin(keyring, 0, &key->index_key, &edit);
if (ret < 0)
goto error;
@@ -583,7 +583,7 @@ int key_reject_and_link(struct key *key,
if (keyring->restrict_link)
return -EPERM;
- link_ret = __key_link_begin(keyring, &key->index_key, &edit);
+ link_ret = __key_link_begin(keyring, 0, &key->index_key, &edit);
}
mutex_lock(&key_construction_mutex);
@@ -860,7 +860,7 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
}
index_key.desc_len = strlen(index_key.description);
- ret = __key_link_begin(keyring, &index_key, &edit);
+ ret = __key_link_begin(keyring, 0, &index_key, &edit);
if (ret < 0) {
key_ref = ERR_PTR(ret);
goto error_free_prep;
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 2cfeeeaa1ffa..cd669f758632 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -1202,7 +1202,7 @@ static int keyring_detect_cycle(struct key *A, struct key *B)
/*
* Preallocate memory so that a key can be linked into to a keyring.
*/
-int __key_link_begin(struct key *keyring,
+int __key_link_begin(struct key *keyring, unsigned int lock_nesting,
const struct keyring_index_key *index_key,
struct assoc_array_edit **_edit)
__acquires(&keyring->sem)
@@ -1219,7 +1219,7 @@ int __key_link_begin(struct key *keyring,
if (keyring->type != &key_type_keyring)
return -ENOTDIR;
- down_write(&keyring->sem);
+ down_write_nested(&keyring->sem, lock_nesting);
ret = -EKEYREVOKED;
if (test_bit(KEY_FLAG_REVOKED, &keyring->flags))
@@ -1366,7 +1366,7 @@ int key_link(struct key *keyring, struct key *key)
key_check(keyring);
key_check(key);
- ret = __key_link_begin(keyring, &key->index_key, &edit);
+ ret = __key_link_begin(keyring, 0, &key->index_key, &edit);
if (ret == 0) {
kdebug("begun {%d,%d}", keyring->serial, refcount_read(&keyring->usage));
ret = __key_link_check_restriction(keyring, key);
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 1f234b019437..e3653c6f85c6 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -372,7 +372,7 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
set_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags);
if (dest_keyring) {
- ret = __key_link_begin(dest_keyring, &ctx->index_key, &edit);
+ ret = __key_link_begin(dest_keyring, 0, &ctx->index_key, &edit);
if (ret < 0)
goto link_prealloc_failed;
}
^ permalink raw reply related
* [PATCH 4/7] keys: Break bits out of key_unlink()
From: David Howells @ 2019-05-22 22:28 UTC (permalink / raw)
To: keyrings; +Cc: dhowells, linux-security-module, linux-kernel
In-Reply-To: <155856408314.10428.17035328117829912815.stgit@warthog.procyon.org.uk>
Break bits out of key_unlink() into helper functions so that they can be
used in implementing key_move().
Signed-off-by: David Howells <dhowells@redhat.com>
---
security/keys/keyring.c | 86 ++++++++++++++++++++++++++++++++++++-----------
1 file changed, 65 insertions(+), 21 deletions(-)
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 5b218b270598..2cfeeeaa1ffa 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -1382,6 +1382,65 @@ int key_link(struct key *keyring, struct key *key)
}
EXPORT_SYMBOL(key_link);
+/*
+ * Begin the process of unlinking a key from a keyring.
+ */
+static int __key_unlink_begin(struct key *keyring, unsigned int lock_nesting,
+ struct key *key, struct assoc_array_edit **_edit)
+ __acquires(&keyring->sem)
+{
+ struct assoc_array_edit *edit;
+ int ret;
+
+ if (keyring->type != &key_type_keyring)
+ return -ENOTDIR;
+
+ down_write_nested(&keyring->sem, lock_nesting);
+
+ edit = assoc_array_delete(&keyring->keys, &keyring_assoc_array_ops,
+ &key->index_key);
+ if (IS_ERR(edit)) {
+ ret = PTR_ERR(edit);
+ goto error;
+ }
+
+ if (!edit) {
+ ret = -ENOENT;
+ goto error;
+ }
+
+ *_edit = edit;
+ return 0;
+
+error:
+ up_write(&keyring->sem);
+ return ret;
+}
+
+/*
+ * Apply an unlink change.
+ */
+static void __key_unlink(struct key *keyring, struct key *key,
+ struct assoc_array_edit **_edit)
+{
+ assoc_array_apply_edit(*_edit);
+ *_edit = NULL;
+ key_payload_reserve(keyring, keyring->datalen - KEYQUOTA_LINK_BYTES);
+}
+
+/*
+ * Finish unlinking a key from to a keyring.
+ */
+static void __key_unlink_end(struct key *keyring,
+ struct key *key,
+ struct assoc_array_edit *edit)
+ __releases(&keyring->sem)
+{
+ if (edit)
+ assoc_array_cancel_edit(edit);
+ up_write(&keyring->sem);
+}
+
/**
* key_unlink - Unlink the first link to a key from a keyring.
* @keyring: The keyring to remove the link from.
@@ -1407,28 +1466,13 @@ int key_unlink(struct key *keyring, struct key *key)
key_check(keyring);
key_check(key);
- if (keyring->type != &key_type_keyring)
- return -ENOTDIR;
-
- down_write(&keyring->sem);
-
- edit = assoc_array_delete(&keyring->keys, &keyring_assoc_array_ops,
- &key->index_key);
- if (IS_ERR(edit)) {
- ret = PTR_ERR(edit);
- goto error;
- }
- ret = -ENOENT;
- if (edit == NULL)
- goto error;
-
- assoc_array_apply_edit(edit);
- key_payload_reserve(keyring, keyring->datalen - KEYQUOTA_LINK_BYTES);
- ret = 0;
+ ret = __key_unlink_begin(keyring, 0, key, &edit);
+ if (ret < 0)
+ return ret;
-error:
- up_write(&keyring->sem);
- return ret;
+ __key_unlink(keyring, key, &edit);
+ __key_unlink_end(keyring, key, edit);
+ return 0;
}
EXPORT_SYMBOL(key_unlink);
^ permalink raw reply related
* [PATCH 3/7] keys: sparse: Fix kdoc mismatches
From: David Howells @ 2019-05-22 22:28 UTC (permalink / raw)
To: keyrings; +Cc: dhowells, linux-security-module, linux-kernel
In-Reply-To: <155856408314.10428.17035328117829912815.stgit@warthog.procyon.org.uk>
Fix some kdoc argument description mismatches reported by sparse and give
keyring_restrict() a description.
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Mat Martineau <mathew.j.martineau@linux.intel.com>
---
security/keys/keyring.c | 10 +++++++---
security/keys/request_key.c | 2 +-
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index e14f09e3a4b0..5b218b270598 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -520,7 +520,7 @@ EXPORT_SYMBOL(keyring_alloc);
* @keyring: The keyring being added to.
* @type: The type of key being added.
* @payload: The payload of the key intended to be added.
- * @data: Additional data for evaluating restriction.
+ * @restriction_key: Keys providing additional data for evaluating restriction.
*
* Reject the addition of any links to a keyring. It can be overridden by
* passing KEY_ALLOC_BYPASS_RESTRICTION to key_instantiate_and_link() when
@@ -976,9 +976,13 @@ static bool keyring_detect_restriction_cycle(const struct key *dest_keyring,
/**
* keyring_restrict - Look up and apply a restriction to a keyring
- *
- * @keyring: The keyring to be restricted
+ * @keyring_ref: The keyring to be restricted
+ * @type: The key type that will provide the restriction checker.
* @restriction: The restriction options to apply to the keyring
+ *
+ * Look up a keyring and apply a restriction to it. The restriction is managed
+ * by the specific key type, but can be configured by the options specified in
+ * the restriction string.
*/
int keyring_restrict(key_ref_t keyring_ref, const char *type,
const char *restriction)
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 75d87f9e0f49..1f234b019437 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -24,7 +24,7 @@
/**
* complete_request_key - Complete the construction of a key.
- * @auth_key: The authorisation key.
+ * @authkey: The authorisation key.
* @error: The success or failute of the construction.
*
* Complete the attempt to construct a key. The key will be negated
^ permalink raw reply related
* [PATCH 2/7] keys: sparse: Fix incorrect RCU accesses
From: David Howells @ 2019-05-22 22:28 UTC (permalink / raw)
To: keyrings; +Cc: dhowells, linux-security-module, linux-kernel
In-Reply-To: <155856408314.10428.17035328117829912815.stgit@warthog.procyon.org.uk>
Fix a pair of accesses that should be using RCU protection.
rcu_dereference_protected() is needed to access task_struct::real_parent.
current_cred() should be used to access current->cred.
Signed-off-by: David Howells <dhowells@redhat.com>
---
security/keys/keyctl.c | 3 ++-
security/keys/request_key_auth.c | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 3e4053a217c3..0f947bcbad46 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -1524,7 +1524,8 @@ long keyctl_session_to_parent(void)
ret = -EPERM;
oldwork = NULL;
- parent = me->real_parent;
+ parent = rcu_dereference_protected(me->real_parent,
+ lockdep_is_held(&tasklist_lock));
/* the parent mustn't be init and mustn't be a kernel thread */
if (parent->pid <= 1 || !parent->mm)
diff --git a/security/keys/request_key_auth.c b/security/keys/request_key_auth.c
index bda6201c6c45..572c7a60473a 100644
--- a/security/keys/request_key_auth.c
+++ b/security/keys/request_key_auth.c
@@ -152,7 +152,7 @@ struct key *request_key_auth_new(struct key *target, const char *op,
struct key *dest_keyring)
{
struct request_key_auth *rka, *irka;
- const struct cred *cred = current->cred;
+ const struct cred *cred = current_cred();
struct key *authkey = NULL;
char desc[20];
int ret = -ENOMEM;
^ permalink raw reply related
* [PATCH 1/7] keys: sparse: Fix key_fs[ug]id_changed()
From: David Howells @ 2019-05-22 22:28 UTC (permalink / raw)
To: keyrings; +Cc: dhowells, linux-security-module, linux-kernel
In-Reply-To: <155856408314.10428.17035328117829912815.stgit@warthog.procyon.org.uk>
Sparse warnings are incurred by key_fs[ug]id_changed() due to unprotected
accesses of tsk->cred, which is marked __rcu.
Fix this by passing the new cred struct to these functions from
commit_creds() rather than the task pointer.
Signed-off-by: David Howells <dhowells@redhat.com>
---
include/linux/key.h | 8 ++++----
kernel/cred.c | 4 ++--
security/keys/process_keys.c | 22 ++++++++++------------
3 files changed, 16 insertions(+), 18 deletions(-)
diff --git a/include/linux/key.h b/include/linux/key.h
index 7099985e35a9..1f09aad1c98c 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -402,8 +402,8 @@ extern struct ctl_table key_sysctls[];
* the userspace interface
*/
extern int install_thread_keyring_to_cred(struct cred *cred);
-extern void key_fsuid_changed(struct task_struct *tsk);
-extern void key_fsgid_changed(struct task_struct *tsk);
+extern void key_fsuid_changed(struct cred *new_cred);
+extern void key_fsgid_changed(struct cred *new_cred);
extern void key_init(void);
#else /* CONFIG_KEYS */
@@ -418,8 +418,8 @@ extern void key_init(void);
#define make_key_ref(k, p) NULL
#define key_ref_to_ptr(k) NULL
#define is_key_possessed(k) 0
-#define key_fsuid_changed(t) do { } while(0)
-#define key_fsgid_changed(t) do { } while(0)
+#define key_fsuid_changed(c) do { } while(0)
+#define key_fsgid_changed(c) do { } while(0)
#define key_init() do { } while(0)
#endif /* CONFIG_KEYS */
diff --git a/kernel/cred.c b/kernel/cred.c
index 45d77284aed0..3bd40de9e192 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -455,9 +455,9 @@ int commit_creds(struct cred *new)
/* alter the thread keyring */
if (!uid_eq(new->fsuid, old->fsuid))
- key_fsuid_changed(task);
+ key_fsuid_changed(new);
if (!gid_eq(new->fsgid, old->fsgid))
- key_fsgid_changed(task);
+ key_fsgid_changed(new);
/* do it
* RLIMIT_NPROC limits on user->processes have already been checked
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index f05f7125a7d5..ba5d3172cafe 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -293,28 +293,26 @@ static int install_session_keyring(struct key *keyring)
/*
* Handle the fsuid changing.
*/
-void key_fsuid_changed(struct task_struct *tsk)
+void key_fsuid_changed(struct cred *new_cred)
{
/* update the ownership of the thread keyring */
- BUG_ON(!tsk->cred);
- if (tsk->cred->thread_keyring) {
- down_write(&tsk->cred->thread_keyring->sem);
- tsk->cred->thread_keyring->uid = tsk->cred->fsuid;
- up_write(&tsk->cred->thread_keyring->sem);
+ if (new_cred->thread_keyring) {
+ down_write(&new_cred->thread_keyring->sem);
+ new_cred->thread_keyring->uid = new_cred->fsuid;
+ up_write(&new_cred->thread_keyring->sem);
}
}
/*
* Handle the fsgid changing.
*/
-void key_fsgid_changed(struct task_struct *tsk)
+void key_fsgid_changed(struct cred *new_cred)
{
/* update the ownership of the thread keyring */
- BUG_ON(!tsk->cred);
- if (tsk->cred->thread_keyring) {
- down_write(&tsk->cred->thread_keyring->sem);
- tsk->cred->thread_keyring->gid = tsk->cred->fsgid;
- up_write(&tsk->cred->thread_keyring->sem);
+ if (new_cred->thread_keyring) {
+ down_write(&new_cred->thread_keyring->sem);
+ new_cred->thread_keyring->gid = new_cred->fsgid;
+ up_write(&new_cred->thread_keyring->sem);
}
}
^ permalink raw reply related
* [PATCH 0/7] keys: Miscellany
From: David Howells @ 2019-05-22 22:28 UTC (permalink / raw)
To: keyrings; +Cc: dhowells, linux-security-module, linux-kernel
Here are some miscellaneous keyrings fixes and improvements intended for
the next merge window:
(1) Fix a bunch of warnings from sparse, including missing RCU bits and
kdoc-function argument mismatches
(2) Implement a keyctl to allow a key to be moved from one keyring to
another, with the option of prohibiting key replacement in the
destination keyring.
(3) Grant Link permission to possessors of request_key_auth tokens so that
upcall servicing daemons can more easily arrange things such that only
the necessary auth key is passed to the actual service program, and
not all the auth keys a daemon might possesss.
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-misc
David
---
David Howells (7):
keys: sparse: Fix key_fs[ug]id_changed()
keys: sparse: Fix incorrect RCU accesses
keys: sparse: Fix kdoc mismatches
keys: Break bits out of key_unlink()
keys: Make __key_link_begin() handle lockdep nesting
keys: Add a keyctl to move a key between keyrings
keys: Grant Link permission to possessers of request_key auth keys
include/linux/key.h | 13 ++-
include/uapi/linux/keyctl.h | 3 +
kernel/cred.c | 4 -
security/keys/compat.c | 3 +
security/keys/internal.h | 3 -
security/keys/key.c | 6 +
security/keys/keyctl.c | 58 ++++++++++++
security/keys/keyring.c | 178 ++++++++++++++++++++++++++++++++++----
security/keys/process_keys.c | 22 ++---
security/keys/request_key.c | 4 -
security/keys/request_key_auth.c | 4 -
11 files changed, 250 insertions(+), 48 deletions(-)
^ permalink raw reply
* Re: [PATCH v3 2/2] initramfs: introduce do_readxattrs()
From: Taras Kondratiuk @ 2019-05-22 20:21 UTC (permalink / raw)
To: Arvind Sankar, Rob Landley, Roberto Sassu, hpa
Cc: viro, linux-security-module, linux-integrity, initramfs,
linux-api, linux-fsdevel, linux-kernel, zohar, silviu.vlasceanu,
dmitry.kasatkin, kamensky, arnd, james.w.mcmechan, niveditas98
In-Reply-To: <3839583c-5466-6573-3048-0da7e6778c88@landley.net>
Quoting Rob Landley (2019-05-22 12:26:43)
>
>
> On 5/22/19 11:17 AM, hpa@zytor.com wrote:
> > On May 20, 2019 2:39:46 AM PDT, Roberto Sassu <roberto.sassu@huawei.com> wrote:
> >> On 5/18/2019 12:17 AM, Arvind Sankar wrote:
> >>> On Fri, May 17, 2019 at 02:47:31PM -0700, H. Peter Anvin wrote:
> >>>> On 5/17/19 2:02 PM, Arvind Sankar wrote:
> >>>>> On Fri, May 17, 2019 at 01:18:11PM -0700, hpa@zytor.com wrote:
> >>>>>>
> >>>>>> Ok... I just realized this does not work for a modular initramfs,
> >> composed at load time from multiple files, which is a very real
> >> problem. Should be easy enough to deal with: instead of one large file,
> >> use one companion file per source file, perhaps something like
> >> filename..xattrs (suggesting double dots to make it less likely to
> >> conflict with a "real" file.) No leading dot, as it makes it more
> >> likely that archivers will sort them before the file proper.
> >>>>> This version of the patch was changed from the previous one exactly
> >> to deal with this case --
> >>>>> it allows for the bootloader to load multiple initramfs archives,
> >> each
> >>>>> with its own .xattr-list file, and to have that work properly.
> >>>>> Could you elaborate on the issue that you see?
> >>>>>
> >>>>
> >>>> Well, for one thing, how do you define "cpio archive", each with its
> >> own
> >>>> .xattr-list file? Second, that would seem to depend on the ordering,
> >> no,
> >>>> in which case you depend critically on .xattr-list file following
> >> the
> >>>> files, which most archivers won't do.
> >>>>
> >>>> Either way it seems cleaner to have this per file; especially if/as
> >> it
> >>>> can be done without actually mucking up the format.
> >>>>
> >>>> I need to run, but I'll post a more detailed explanation of what I
> >> did
> >>>> in a little bit.
> >>>>
> >>>> -hpa
> >>>>
> >>> Not sure what you mean by how do I define it? Each cpio archive will
> >>> contain its own .xattr-list file with signatures for the files within
> >>> it, that was the idea.
> >>>
> >>> You need to review the code more closely I think -- it does not
> >> depend
> >>> on the .xattr-list file following the files to which it applies.
> >>>
> >>> The code first extracts .xattr-list as though it was a regular file.
> >> If
> >>> a later dupe shows up (presumably from a second archive, although the
> >>> patch will actually allow a second one in the same archive), it will
> >>> then process the existing .xattr-list file and apply the attributes
> >>> listed within it. It then will proceed to read the second one and
> >>> overwrite the first one with it (this is the normal behaviour in the
> >>> kernel cpio parser). At the end once all the archives have been
> >>> extracted, if there is an .xattr-list file in the rootfs it will be
> >>> parsed (it would've been the last one encountered, which hasn't been
> >>> parsed yet, just extracted).
> >>>
> >>> Regarding the idea to use the high 16 bits of the mode field in
> >>> the header that's another possibility. It would just require
> >> additional
> >>> support in the program that actually creates the archive though,
> >> which
> >>> the current patch doesn't.
> >>
> >> Yes, for adding signatures for a subset of files, no changes to the ram
> >> disk generator are necessary. Everything is done by a custom module. To
> >> support a generic use case, it would be necessary to modify the
> >> generator to execute getfattr and the awk script after files have been
> >> placed in the temporary directory.
> >>
> >> If I understood the new proposal correctly, it would be task for cpio
> >> to
> >> read file metadata after the content and create a new record for each
> >> file with mode 0x18000, type of metadata encoded in the file name and
> >> metadata as file content. I don't know how easy it would be to modify
> >> cpio. Probably the amount of changes would be reasonable.
>
> I could make toybox cpio do it in a weekend, and could probably throw a patch at
> usr/gen_init_cpio.c while I'm at it. I prototyped something like that a couple
> years ago, it's not hard.
>
> The real question is scripts/gen_initramfs_list.sh and the text format it
> produces. We can currently generate cpio files with different ownership and
> permissions than the host system can represent (when not building as root, on a
> filesystem that may not support xattrs or would get unhappy about conflicting
> selinux annotations). We work around it by having the metadata represented
> textually in the initramfs_list file gen_initramfs_list.sh produces and
> gen_init_cpio.c consumes.
>
> xattrs are a terrible idea the Macintosh invented so Finder could remember where
> you moved a file's icon in its folder without having to modify the file, and
> then things like OS/2 copied it and Windows picked it up from there and went "Of
> course, this is a security mechanism!" and... sigh.
>
> This is "data that is not data", it's metadata of unbounded size. It seems like
> it should go in gen_initramfs_list.sh but as what, keyword=value pairs that
> might have embedded newlines in them? A base64 encoding? Something else?
I the previous try to add xattrs to cpio I've used hex encoding in
gen_initramfs_list.sh:
https://lkml.org/lkml/2018/1/24/851 - gen_init_cpio: set extended attributes for newcx format
https://lkml.org/lkml/2018/1/24/852 - gen_initramfs_list.sh: add -x option to enable newcx format
^ permalink raw reply
* Re: [RFC] Turn lockdown into an LSM
From: Stephen Smalley @ 2019-05-22 20:03 UTC (permalink / raw)
To: James Morris
Cc: Andy Lutomirski, Matthew Garrett, LSM List,
Linux Kernel Mailing List
In-Reply-To: <alpine.LRH.2.21.1905230457000.18826@namei.org>
On 5/22/19 3:19 PM, James Morris wrote:
> On Wed, 22 May 2019, Stephen Smalley wrote:
>
>> That seems to violate the intent of lockdown as I understood it, and
>> turns security_is_locked_down() into a finer-grained capable() call.
>> Also, if I understand correctly, this could only be done if one were to
>> disable the lockdown module in the lsm list, since the security
>> framework will return non-zero (i.e. the operation is locked down) if
>> any module that implements the hook returns non-zero; LSM is
>> "restrictive". At that point SELinux or the other LSM would be the sole
>> arbiter of lockdown decisions. SELinux or the other LSM also wouldn't
>> have access to the kernel_locked_down level unless that was exported in
>> some manner from the lockdown module. Not sure how to compose these.
>
> Right, I was envisaging the LSM replacing the default.
>
> i.e. the default is tristate OR fine grained LSM policy.
>
> They could in theory be composed restrictively, but this is likely not
> useful given the coarse grained default policy. All the LSM could do is
> either further restrict none or integrity.
>
> We'd need to figure out how to avoid confusing users in the case where
> multiple LSMs are registered for the hooks, possibly by having the
> lockdown LSM gate this and update the securityfs lockdown node with
> something like "lsm:smack".
Some kind of transition from the lockdown module to other security
modules might be needed, e.g. you might need to start with
lockdown=integrity to protect the kernel up to the point where a policy
is loaded, then hand off to SELinux or another security module to handle
further requests.
^ permalink raw reply
* Re: [RFC] Turn lockdown into an LSM
From: Casey Schaufler @ 2019-05-22 19:57 UTC (permalink / raw)
To: James Morris, Stephen Smalley
Cc: Andy Lutomirski, Matthew Garrett, LSM List,
Linux Kernel Mailing List
In-Reply-To: <alpine.LRH.2.21.1905230457000.18826@namei.org>
On 5/22/2019 12:19 PM, James Morris wrote:
> On Wed, 22 May 2019, Stephen Smalley wrote:
>
>> That seems to violate the intent of lockdown as I understood it, and
>> turns security_is_locked_down() into a finer-grained capable() call.
>> Also, if I understand correctly, this could only be done if one were to
>> disable the lockdown module in the lsm list, since the security
>> framework will return non-zero (i.e. the operation is locked down) if
>> any module that implements the hook returns non-zero; LSM is
>> "restrictive". At that point SELinux or the other LSM would be the sole
>> arbiter of lockdown decisions. SELinux or the other LSM also wouldn't
>> have access to the kernel_locked_down level unless that was exported in
>> some manner from the lockdown module. Not sure how to compose these.
> Right, I was envisaging the LSM replacing the default.
>
> i.e. the default is tristate OR fine grained LSM policy.
>
> They could in theory be composed restrictively, but this is likely not
> useful given the coarse grained default policy. All the LSM could do is
> either further restrict none or integrity.
>
> We'd need to figure out how to avoid confusing users in the case where
> multiple LSMs are registered for the hooks, possibly by having the
> lockdown LSM gate this and update the securityfs lockdown node with
> something like "lsm:smack".
The way I'd propose dealing with multiple LSMs using the
securityfs interface is the same as I'm proposing for
/proc/.../attr/current and SO_PEERSEC. A new interface
/proc/self/attr/display contains the name of the LSM that
the current process will see when looking at process or
security attributes that are "shared". Writing to display
is unprivileged and changes which LSM you get information
for.
Adornments like "lsm:smack" often require modification of
programs that fear change. The same would be true of a prctl().
The "display" file approach is no harder for applications that
are getting modified and much easier for scripts.
^ permalink raw reply
* Re: [PATCH v3 2/2] initramfs: introduce do_readxattrs()
From: Rob Landley @ 2019-05-22 19:26 UTC (permalink / raw)
To: hpa, Roberto Sassu, Arvind Sankar
Cc: viro, linux-security-module, linux-integrity, initramfs,
linux-api, linux-fsdevel, linux-kernel, zohar, silviu.vlasceanu,
dmitry.kasatkin, takondra, kamensky, arnd, james.w.mcmechan,
niveditas98
In-Reply-To: <9C5B9F98-2067-43D3-B149-57613F38DCD4@zytor.com>
On 5/22/19 11:17 AM, hpa@zytor.com wrote:
> On May 20, 2019 2:39:46 AM PDT, Roberto Sassu <roberto.sassu@huawei.com> wrote:
>> On 5/18/2019 12:17 AM, Arvind Sankar wrote:
>>> On Fri, May 17, 2019 at 02:47:31PM -0700, H. Peter Anvin wrote:
>>>> On 5/17/19 2:02 PM, Arvind Sankar wrote:
>>>>> On Fri, May 17, 2019 at 01:18:11PM -0700, hpa@zytor.com wrote:
>>>>>>
>>>>>> Ok... I just realized this does not work for a modular initramfs,
>> composed at load time from multiple files, which is a very real
>> problem. Should be easy enough to deal with: instead of one large file,
>> use one companion file per source file, perhaps something like
>> filename..xattrs (suggesting double dots to make it less likely to
>> conflict with a "real" file.) No leading dot, as it makes it more
>> likely that archivers will sort them before the file proper.
>>>>> This version of the patch was changed from the previous one exactly
>> to deal with this case --
>>>>> it allows for the bootloader to load multiple initramfs archives,
>> each
>>>>> with its own .xattr-list file, and to have that work properly.
>>>>> Could you elaborate on the issue that you see?
>>>>>
>>>>
>>>> Well, for one thing, how do you define "cpio archive", each with its
>> own
>>>> .xattr-list file? Second, that would seem to depend on the ordering,
>> no,
>>>> in which case you depend critically on .xattr-list file following
>> the
>>>> files, which most archivers won't do.
>>>>
>>>> Either way it seems cleaner to have this per file; especially if/as
>> it
>>>> can be done without actually mucking up the format.
>>>>
>>>> I need to run, but I'll post a more detailed explanation of what I
>> did
>>>> in a little bit.
>>>>
>>>> -hpa
>>>>
>>> Not sure what you mean by how do I define it? Each cpio archive will
>>> contain its own .xattr-list file with signatures for the files within
>>> it, that was the idea.
>>>
>>> You need to review the code more closely I think -- it does not
>> depend
>>> on the .xattr-list file following the files to which it applies.
>>>
>>> The code first extracts .xattr-list as though it was a regular file.
>> If
>>> a later dupe shows up (presumably from a second archive, although the
>>> patch will actually allow a second one in the same archive), it will
>>> then process the existing .xattr-list file and apply the attributes
>>> listed within it. It then will proceed to read the second one and
>>> overwrite the first one with it (this is the normal behaviour in the
>>> kernel cpio parser). At the end once all the archives have been
>>> extracted, if there is an .xattr-list file in the rootfs it will be
>>> parsed (it would've been the last one encountered, which hasn't been
>>> parsed yet, just extracted).
>>>
>>> Regarding the idea to use the high 16 bits of the mode field in
>>> the header that's another possibility. It would just require
>> additional
>>> support in the program that actually creates the archive though,
>> which
>>> the current patch doesn't.
>>
>> Yes, for adding signatures for a subset of files, no changes to the ram
>> disk generator are necessary. Everything is done by a custom module. To
>> support a generic use case, it would be necessary to modify the
>> generator to execute getfattr and the awk script after files have been
>> placed in the temporary directory.
>>
>> If I understood the new proposal correctly, it would be task for cpio
>> to
>> read file metadata after the content and create a new record for each
>> file with mode 0x18000, type of metadata encoded in the file name and
>> metadata as file content. I don't know how easy it would be to modify
>> cpio. Probably the amount of changes would be reasonable.
I could make toybox cpio do it in a weekend, and could probably throw a patch at
usr/gen_init_cpio.c while I'm at it. I prototyped something like that a couple
years ago, it's not hard.
The real question is scripts/gen_initramfs_list.sh and the text format it
produces. We can currently generate cpio files with different ownership and
permissions than the host system can represent (when not building as root, on a
filesystem that may not support xattrs or would get unhappy about conflicting
selinux annotations). We work around it by having the metadata represented
textually in the initramfs_list file gen_initramfs_list.sh produces and
gen_init_cpio.c consumes.
xattrs are a terrible idea the Macintosh invented so Finder could remember where
you moved a file's icon in its folder without having to modify the file, and
then things like OS/2 copied it and Windows picked it up from there and went "Of
course, this is a security mechanism!" and... sigh.
This is "data that is not data", it's metadata of unbounded size. It seems like
it should go in gen_initramfs_list.sh but as what, keyword=value pairs that
might have embedded newlines in them? A base64 encoding? Something else?
>> The kernel will behave in a similar way. It will call do_readxattrs()
>> in
>> do_copy() for each file. Since the only difference between the current
>> and the new proposal would be two additional calls to do_readxattrs()
>> in
>> do_name() and unpack_to_rootfs(), maybe we could support both.
>>
>> Roberto
>
> The nice thing with explicit metadata is that it doesn't have to contain the filename per se, and each file is self-contained. There is a reason why each cpio header starts with the magic number: each cpio record is formally independent and can be processed in isolation. The TRAILER!!! thing is a huge wart in the format, although in practice TRAILER!!! always has a mode of 0 and so can be distinguished from an actual file.
Not adding the requirement that the cpio.gz must be generated as root from a
filesystem with the same users and selinux rules as the target system would be nice.
> The use of mode 0x18000 for metadata allows for optional backwards compatibility for extraction; for encoding this can be handled with very simple postprocessing.
The representation within the cpio file was never a huge deal to me. 0x18000
sounds fine for that.
> So my suggestion would be to have mode 0x18000 indicate extended file metadata, with the filename of the form:
>
> optional_filename!XXXXX!
>
> ... where XXXXX indicates the type of metadata (e.g. !XATTR!). The optional_filename prefix allows an unaware decoder to extract to a well-defined name; simple postprocessing would be able to either remove (for size) or add (for compatibility) this prefix. It would be an error for this prefix, if present, to not match the name of the previous file.
I'd suggest METADATA!!! to look like TRAILER!!!. (METADATA!!!XXXXX! if you
really think a keyword=value pair store is _not_ universal and we're going to
invent entire new _categories_ of this side channel nonsense.)
And extracting conflicting filenames is presumably already covered, it either
replaces or the new one fails to create the file and the extractor moves on.
(You need a working error recovery path that skips the right amount of data so
you can handle the next file properly, but you should have that anyway.)
Rob
^ permalink raw reply
* Re: [RFC] Turn lockdown into an LSM
From: James Morris @ 2019-05-22 19:19 UTC (permalink / raw)
To: Stephen Smalley
Cc: Andy Lutomirski, Matthew Garrett, LSM List,
Linux Kernel Mailing List
In-Reply-To: <14ed1f30-a1d0-f973-5c8c-241337c8fc09@tycho.nsa.gov>
On Wed, 22 May 2019, Stephen Smalley wrote:
> That seems to violate the intent of lockdown as I understood it, and
> turns security_is_locked_down() into a finer-grained capable() call.
> Also, if I understand correctly, this could only be done if one were to
> disable the lockdown module in the lsm list, since the security
> framework will return non-zero (i.e. the operation is locked down) if
> any module that implements the hook returns non-zero; LSM is
> "restrictive". At that point SELinux or the other LSM would be the sole
> arbiter of lockdown decisions. SELinux or the other LSM also wouldn't
> have access to the kernel_locked_down level unless that was exported in
> some manner from the lockdown module. Not sure how to compose these.
Right, I was envisaging the LSM replacing the default.
i.e. the default is tristate OR fine grained LSM policy.
They could in theory be composed restrictively, but this is likely not
useful given the coarse grained default policy. All the LSM could do is
either further restrict none or integrity.
We'd need to figure out how to avoid confusing users in the case where
multiple LSMs are registered for the hooks, possibly by having the
lockdown LSM gate this and update the securityfs lockdown node with
something like "lsm:smack".
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [RFC] Turn lockdown into an LSM
From: Stephen Smalley @ 2019-05-22 18:30 UTC (permalink / raw)
To: Andy Lutomirski, Matthew Garrett
Cc: James Morris, LSM List, Linux Kernel Mailing List
In-Reply-To: <CALCETrVow8U=xhQdJt8kSMX16Lf0Mstf3+QxY4iz4DHVp=PYWA@mail.gmail.com>
On 5/22/19 1:08 PM, Andy Lutomirski wrote:
> On Wed, May 22, 2019 at 9:49 AM Matthew Garrett <mjg59@google.com> wrote:
>>
>> On Tue, May 21, 2019 at 7:40 PM James Morris <jmorris@namei.org> wrote:
>>> An LSM could also potentially implement its own policy for the hook.
>>
>> That was my plan. Right now the hook just gets an ASCII description of
>> the reason for the lockdown - that seems suboptimal for cases like
>> SELinux. What information would you want? My initial thinking was to
>> just have a stable enum of lockdown reasons that's in the UAPI headers
>> and then let other LSM tooling consume that, but I haven't spent
>> enough time with the internals of SELinux to know if there'd be a more
>> attractive solution.
>
> I may be in the minority here, but I see this issue as a significant
> downside of making lockdown more flexible. If we stick with just
> "this may violate integrity" and "this may violate confidentiality",
> then the ABI surface is nice and narrow. If we start having a big
> uapi list of things that might qualify for lockdown, we need to worry
> about compatibility issues.
>
> This isn't purely theoretical. Lockdown has some interesting
> interactions with eBPF. I don't want to be in a situation where v1 of
> lockdown has a few eBPF hooks, but a later update improves the eBPF vs
> lockdown interaction so that you can do more with eBPF on a locked
> down kernel. But now any such change has to worry about breaking the
> lockdown LSM ABI.
I think we could keep the enum itself private to the kernel, and
internally each security module could map the values to whatever
permissions it wants to define. SELinux itself has the ability to
add/remove/re-arrange its permission definitions without breaking
userspace/policy; they are dynamically mapped at policy load time. If a
given permission doesn't exist in the loaded policy, there is a policy
flag to control whether the policy should be rejected, or the permission
should always be denied, or the permission should always be allowed.
There is also support for an extensible set of "policy capabilities" to
control whether new/changed permission checking logic for a given
permission/operation should be enabled for that policy or if the kernel
should continue using the older logic.
> And I still think it would be nice to have some credible use case for
> a more fine grained policy than just the tri-state. Having a lockdown
> policy of "may not violate kernel confidentiality except using
> kprobes" may be convenient, but it's also basically worthless, since
> kernel confidentiality is gone.
I would expect that distros and users would have to make pragmatic
tradeoffs in this area, and it would be nice if they could have a choice
beyond all-or-nothing. Do all of the interfaces being locked down
expose the same degree of confidentiality or integrity risk?
> All this being said, I do see one big benefit for LSM integration:
> SELinux or another LSM could allow certain privileged tasks to bypass
> lockdown.
That seems to violate the intent of lockdown as I understood it, and
turns security_is_locked_down() into a finer-grained capable() call.
Also, if I understand correctly, this could only be done if one were to
disable the lockdown module in the lsm list, since the security
framework will return non-zero (i.e. the operation is locked down) if
any module that implements the hook returns non-zero; LSM is
"restrictive". At that point, SELinux or the other LSM would be the sole
arbiter of lockdown decisions. SELinux or the other LSM also wouldn't
have access to the kernel_locked_down level unless that was exported in
some manner from the lockdown module. Not sure how to compose these.
This seems fine, except that there's potential nastiness
> where current->cred isn't actually a valid thing to look at in the
> current context.
>
> So I guess my proposal is: use LSM, but make the hook very coarse
> grained: int security_violate_confidentiality(const struct cred *) and
> int security_violate_integrity(const struct cred *).
Not sure one can construct a useful policy at that granularity.
^ permalink raw reply
* Re: [RFC] Turn lockdown into an LSM
From: James Morris @ 2019-05-22 18:05 UTC (permalink / raw)
To: Andy Lutomirski; +Cc: Matthew Garrett, LSM List, Linux Kernel Mailing List
In-Reply-To: <CALCETrVow8U=xhQdJt8kSMX16Lf0Mstf3+QxY4iz4DHVp=PYWA@mail.gmail.com>
On Wed, 22 May 2019, Andy Lutomirski wrote:
> And I still think it would be nice to have some credible use case for
> a more fine grained policy than just the tri-state. Having a lockdown
> policy of "may not violate kernel confidentiality except using
> kprobes" may be convenient, but it's also basically worthless, since
> kernel confidentiality is gone.
This is an important point, but there's also "can't use any lockdown
features because the admin might need to use kprobes". I mention a
use-case below.
I think it's fine (and probably preferred) to keep the default behavior
tri-state and allow LSMs to implement finer-grained policies.
> All this being said, I do see one big benefit for LSM integration:
> SELinux or another LSM could allow certain privileged tasks to bypass
> lockdown.
Some environments _need_ a "break glass" option, and a well-defined policy
(e.g. an SELinux domain which can only be entered via serial console, with
2FA or JIT credentials) to selectively un-lock the kernel lockdown in
production would mean the difference between having a fleet of millions of
nodes 99.999% locked down vs 0%.
> This seems fine, except that there's potential nastiness
> where current->cred isn't actually a valid thing to look at in the
> current context.
Right.
Can we identify any such cases in the current patchset?
One option would be for the LSM to assign a default (untrusted/unknown)
value for the subject and then apply policy as needed (e.g. allow or deny
these).
> So I guess my proposal is: use LSM, but make the hook very coarse
> grained: int security_violate_confidentiality(const struct cred *) and
> int security_violate_integrity(const struct cred *).
Perhaps security_kernel_unlock_*
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH] LSM: Update MAINTAINERS file for SafeSetID LSM.
From: James Morris @ 2019-05-22 17:42 UTC (permalink / raw)
To: Micah Morton; +Cc: linux-security-module
In-Reply-To: <20190522172241.192669-1-mortonm@chromium.org>
On Wed, 22 May 2019, Micah Morton wrote:
> This is in preparation for SafeSetID to be maintained in its own tree,
> rather than going via the security tree.
>
> Signed-off-by: Micah Morton <mortonm@chromium.org>
Acked-by: James Morris <jamorris@linux.microsoft.com>
> ---
> MAINTAINERS | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3e5a5d263f29..0fcd34e64fa7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13561,6 +13561,12 @@ F: drivers/media/common/saa7146/
> F: drivers/media/pci/saa7146/
> F: include/media/drv-intf/saa7146*
>
> +SAFESETID SECURITY MODULE
> +M: Micah Morton <mortonm@chromium.org>
> +S: Supported
> +F: security/safesetid/
> +F: Documentation/admin-guide/LSM/SafeSetID.rst
> +
> SAMSUNG AUDIO (ASoC) DRIVERS
> M: Krzysztof Kozlowski <krzk@kernel.org>
> M: Sangbeom Kim <sbkim73@samsung.com>
>
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* [PATCH] LSM: Update MAINTAINERS file for SafeSetID LSM.
From: Micah Morton @ 2019-05-22 17:22 UTC (permalink / raw)
To: jmorris, linux-security-module; +Cc: Micah Morton
This is in preparation for SafeSetID to be maintained in its own tree,
rather than going via the security tree.
Signed-off-by: Micah Morton <mortonm@chromium.org>
---
MAINTAINERS | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 3e5a5d263f29..0fcd34e64fa7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13561,6 +13561,12 @@ F: drivers/media/common/saa7146/
F: drivers/media/pci/saa7146/
F: include/media/drv-intf/saa7146*
+SAFESETID SECURITY MODULE
+M: Micah Morton <mortonm@chromium.org>
+S: Supported
+F: security/safesetid/
+F: Documentation/admin-guide/LSM/SafeSetID.rst
+
SAMSUNG AUDIO (ASoC) DRIVERS
M: Krzysztof Kozlowski <krzk@kernel.org>
M: Sangbeom Kim <sbkim73@samsung.com>
--
2.21.0.1020.gf2820cf01a-goog
^ permalink raw reply related
* Re: [PATCH v3 2/2] initramfs: introduce do_readxattrs()
From: Roberto Sassu @ 2019-05-22 17:22 UTC (permalink / raw)
To: hpa, Arvind Sankar
Cc: viro, linux-security-module, linux-integrity, initramfs,
linux-api, linux-fsdevel, linux-kernel, zohar, silviu.vlasceanu,
dmitry.kasatkin, takondra, kamensky, arnd, rob, james.w.mcmechan,
niveditas98
In-Reply-To: <9C5B9F98-2067-43D3-B149-57613F38DCD4@zytor.com>
On 5/22/2019 6:17 PM, hpa@zytor.com wrote:
> On May 20, 2019 2:39:46 AM PDT, Roberto Sassu <roberto.sassu@huawei.com> wrote:
>> On 5/18/2019 12:17 AM, Arvind Sankar wrote:
>>> On Fri, May 17, 2019 at 02:47:31PM -0700, H. Peter Anvin wrote:
>>>> On 5/17/19 2:02 PM, Arvind Sankar wrote:
>>>>> On Fri, May 17, 2019 at 01:18:11PM -0700, hpa@zytor.com wrote:
>>>>>>
>>>>>> Ok... I just realized this does not work for a modular initramfs,
>> composed at load time from multiple files, which is a very real
>> problem. Should be easy enough to deal with: instead of one large file,
>> use one companion file per source file, perhaps something like
>> filename..xattrs (suggesting double dots to make it less likely to
>> conflict with a "real" file.) No leading dot, as it makes it more
>> likely that archivers will sort them before the file proper.
>>>>> This version of the patch was changed from the previous one exactly
>> to deal with this case --
>>>>> it allows for the bootloader to load multiple initramfs archives,
>> each
>>>>> with its own .xattr-list file, and to have that work properly.
>>>>> Could you elaborate on the issue that you see?
>>>>>
>>>>
>>>> Well, for one thing, how do you define "cpio archive", each with its
>> own
>>>> .xattr-list file? Second, that would seem to depend on the ordering,
>> no,
>>>> in which case you depend critically on .xattr-list file following
>> the
>>>> files, which most archivers won't do.
>>>>
>>>> Either way it seems cleaner to have this per file; especially if/as
>> it
>>>> can be done without actually mucking up the format.
>>>>
>>>> I need to run, but I'll post a more detailed explanation of what I
>> did
>>>> in a little bit.
>>>>
>>>> -hpa
>>>>
>>> Not sure what you mean by how do I define it? Each cpio archive will
>>> contain its own .xattr-list file with signatures for the files within
>>> it, that was the idea.
>>>
>>> You need to review the code more closely I think -- it does not
>> depend
>>> on the .xattr-list file following the files to which it applies.
>>>
>>> The code first extracts .xattr-list as though it was a regular file.
>> If
>>> a later dupe shows up (presumably from a second archive, although the
>>> patch will actually allow a second one in the same archive), it will
>>> then process the existing .xattr-list file and apply the attributes
>>> listed within it. It then will proceed to read the second one and
>>> overwrite the first one with it (this is the normal behaviour in the
>>> kernel cpio parser). At the end once all the archives have been
>>> extracted, if there is an .xattr-list file in the rootfs it will be
>>> parsed (it would've been the last one encountered, which hasn't been
>>> parsed yet, just extracted).
>>>
>>> Regarding the idea to use the high 16 bits of the mode field in
>>> the header that's another possibility. It would just require
>> additional
>>> support in the program that actually creates the archive though,
>> which
>>> the current patch doesn't.
>>
>> Yes, for adding signatures for a subset of files, no changes to the ram
>> disk generator are necessary. Everything is done by a custom module. To
>> support a generic use case, it would be necessary to modify the
>> generator to execute getfattr and the awk script after files have been
>> placed in the temporary directory.
>>
>> If I understood the new proposal correctly, it would be task for cpio
>> to
>> read file metadata after the content and create a new record for each
>> file with mode 0x18000, type of metadata encoded in the file name and
>> metadata as file content. I don't know how easy it would be to modify
>> cpio. Probably the amount of changes would be reasonable.
>>
>> The kernel will behave in a similar way. It will call do_readxattrs()
>> in
>> do_copy() for each file. Since the only difference between the current
>> and the new proposal would be two additional calls to do_readxattrs()
>> in
>> do_name() and unpack_to_rootfs(), maybe we could support both.
>>
>> Roberto
>
> The nice thing with explicit metadata is that it doesn't have to contain the filename per se, and each file is self-contained. There is a reason why each cpio header starts with the magic number: each cpio record is formally independent and can be processed in isolation. The TRAILER!!! thing is a huge wart in the format, although in practice TRAILER!!! always has a mode of 0 and so can be distinguished from an actual file.
>
> The use of mode 0x18000 for metadata allows for optional backwards compatibility for extraction; for encoding this can be handled with very simple postprocessing.
>
> So my suggestion would be to have mode 0x18000 indicate extended file metadata, with the filename of the form:
>
> optional_filename!XXXXX!
>
> ... where XXXXX indicates the type of metadata (e.g. !XATTR!). The optional_filename prefix allows an unaware decoder to extract to a well-defined name; simple postprocessing would be able to either remove (for size) or add (for compatibility) this prefix. It would be an error for this prefix, if present, to not match the name of the previous file.
Actually, I defined '..metadata..' as special name to indicate that the
file contains metadata. Then, the content of the file is a set of:
struct metadata_hdr {
char c_size[8]; /* total size including c_size field */
char c_version; /* header version */
char c_type; /* metadata type */
char c_metadata[]; /* metadata */
} __packed;
init/initramfs.c now has a specific parser for c_type. Currently, I
implemented a parser for xattrs, which expects data in the format:
<xattr #N name>\0<xattr #N value>
I checked if it is possible to use bit 17:16 to identify files with
metadata, but both the cpio and the kernel use unsigned short.
I already modified gen_init_cpio and cpio. I modify at run-time the list
of files to be included in the image by adding a temporary file, that
each time is set with the xattrs of the previously processed file.
The output of cpio -t looks like:
--
.
..metadata..
bin
..metadata..
dev
..metadata..
dev/console
..metadata..
--
Would it be ok? If you prefer that I add the format to the file name or
you/anyone has a comment about this proposal, please let me know so that
I make the changes before sending a new version of the patch set.
Thanks
Roberto
--
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI
^ permalink raw reply
* Re: [RFC] Turn lockdown into an LSM
From: Andy Lutomirski @ 2019-05-22 17:08 UTC (permalink / raw)
To: Matthew Garrett
Cc: James Morris, LSM List, Linux Kernel Mailing List,
Andy Lutomirski
In-Reply-To: <CACdnJuuTR=Ut4giPKC=kdxgY9yPv8+3PZyEzuxvON3Jr_92XnQ@mail.gmail.com>
On Wed, May 22, 2019 at 9:49 AM Matthew Garrett <mjg59@google.com> wrote:
>
> On Tue, May 21, 2019 at 7:40 PM James Morris <jmorris@namei.org> wrote:
> > An LSM could also potentially implement its own policy for the hook.
>
> That was my plan. Right now the hook just gets an ASCII description of
> the reason for the lockdown - that seems suboptimal for cases like
> SELinux. What information would you want? My initial thinking was to
> just have a stable enum of lockdown reasons that's in the UAPI headers
> and then let other LSM tooling consume that, but I haven't spent
> enough time with the internals of SELinux to know if there'd be a more
> attractive solution.
I may be in the minority here, but I see this issue as a significant
downside of making lockdown more flexible. If we stick with just
"this may violate integrity" and "this may violate confidentiality",
then the ABI surface is nice and narrow. If we start having a big
uapi list of things that might qualify for lockdown, we need to worry
about compatibility issues.
This isn't purely theoretical. Lockdown has some interesting
interactions with eBPF. I don't want to be in a situation where v1 of
lockdown has a few eBPF hooks, but a later update improves the eBPF vs
lockdown interaction so that you can do more with eBPF on a locked
down kernel. But now any such change has to worry about breaking the
lockdown LSM ABI.
And I still think it would be nice to have some credible use case for
a more fine grained policy than just the tri-state. Having a lockdown
policy of "may not violate kernel confidentiality except using
kprobes" may be convenient, but it's also basically worthless, since
kernel confidentiality is gone.
All this being said, I do see one big benefit for LSM integration:
SELinux or another LSM could allow certain privileged tasks to bypass
lockdown. This seems fine, except that there's potential nastiness
where current->cred isn't actually a valid thing to look at in the
current context.
So I guess my proposal is: use LSM, but make the hook very coarse
grained: int security_violate_confidentiality(const struct cred *) and
int security_violate_integrity(const struct cred *).
--Andy
^ permalink raw reply
* Re: [RFC] Turn lockdown into an LSM
From: Matthew Garrett @ 2019-05-22 16:48 UTC (permalink / raw)
To: James Morris; +Cc: LSM List, Linux Kernel Mailing List, Andy Lutomirski
In-Reply-To: <alpine.LRH.2.21.1905221203070.3967@namei.org>
On Tue, May 21, 2019 at 7:40 PM James Morris <jmorris@namei.org> wrote:
> An LSM could also potentially implement its own policy for the hook.
That was my plan. Right now the hook just gets an ASCII description of
the reason for the lockdown - that seems suboptimal for cases like
SELinux. What information would you want? My initial thinking was to
just have a stable enum of lockdown reasons that's in the UAPI headers
and then let other LSM tooling consume that, but I haven't spent
enough time with the internals of SELinux to know if there'd be a more
attractive solution.
^ permalink raw reply
* Re: [PATCH v3 2/2] initramfs: introduce do_readxattrs()
From: hpa @ 2019-05-22 16:18 UTC (permalink / raw)
To: Rob Landley, Roberto Sassu, viro
Cc: linux-security-module, linux-integrity, initramfs, linux-api,
linux-fsdevel, linux-kernel, zohar, silviu.vlasceanu,
dmitry.kasatkin, takondra, kamensky, arnd, james.w.mcmechan,
niveditas98
In-Reply-To: <a0afd58f-c682-66b5-7478-c405a179d72a@landley.net>
On May 17, 2019 7:16:04 PM PDT, Rob Landley <rob@landley.net> wrote:
>On 5/17/19 4:41 PM, H. Peter Anvin wrote:
>> On 5/17/19 1:18 PM, hpa@zytor.com wrote:
>>>
>>> Ok... I just realized this does not work for a modular initramfs,
>composed at load time from multiple files, which is a very real
>problem. Should be easy enough to deal with: instead of one large file,
>use one companion file per source file, perhaps something like
>filename..xattrs (suggesting double dots to make it less likely to
>conflict with a "real" file.) No leading dot, as it makes it more
>likely that archivers will sort them before the file proper.
>>>
>>> A side benefit is that the format can be simpler as there is no need
>to encode the filename.
>>>
>>> A technically cleaner solution still, but which would need archiver
>modifications, would be to encode the xattrs as an optionally nameless
>file (just an empty string) with a new file mode value, immediately
>following the original file. The advantage there is that the archiver
>itself could support xattrs and other extended metadata (which has been
>requested elsewhere); the disadvantage obviously is that that it
>requires new support in the archiver. However, at least it ought to be
>simpler since it is still a higher protocol level than the cpio archive
>itself.
>>>
>>> There's already one special case in cpio, which is the
>"!!!TRAILER!!!" filename; although I don't think it is part of the
>formal spec, to the extent there is one, I would expect that in
>practice it is always encoded with a mode of 0, which incidentally
>could be used to unbreak the case where such a filename actually
>exists. So one way to support such extended metadata would be to set
>mode to 0 and use the filename to encode the type of metadata. I wonder
>how existing GNU or BSD cpio (the BSD one is better maintained these
>days) would deal with reading such a file; it would at least not be a
>regression if it just read it still, possibly with warnings. It could
>also be possible to use bits 17:16 in the mode, which are traditionally
>always zero (mode_t being 16 bits), but I believe are present in most
>or all of the cpio formats for historical reasons. It might be accepted
>better by existing implementations to use one of these high bits
>combined with S_IFREG, I dont know.
>>
>>
>> Correction: it's just !!!TRAILER!!!.
>
>We documented it as "TRAILER!!!" without leading !!!, and that its
>purpose is to
>flush hardlinks:
>
>https://www.kernel.org/doc/Documentation/early-userspace/buffer-format.txt
>
>That's what toybox cpio has been producing. Kernel consumes it just
>fine. Just
>checked busybox cpio and that's what they're producing as well...
>
>Rob
Yes, TRAILER!!! is correct. Somehow I managed to get it wrong twice.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* Re: [PATCH v3 2/2] initramfs: introduce do_readxattrs()
From: hpa @ 2019-05-22 16:17 UTC (permalink / raw)
To: Roberto Sassu, Arvind Sankar
Cc: viro, linux-security-module, linux-integrity, initramfs,
linux-api, linux-fsdevel, linux-kernel, zohar, silviu.vlasceanu,
dmitry.kasatkin, takondra, kamensky, arnd, rob, james.w.mcmechan,
niveditas98
In-Reply-To: <7bdca169-7a01-8c55-40e4-a832e876a0e5@huawei.com>
On May 20, 2019 2:39:46 AM PDT, Roberto Sassu <roberto.sassu@huawei.com> wrote:
>On 5/18/2019 12:17 AM, Arvind Sankar wrote:
>> On Fri, May 17, 2019 at 02:47:31PM -0700, H. Peter Anvin wrote:
>>> On 5/17/19 2:02 PM, Arvind Sankar wrote:
>>>> On Fri, May 17, 2019 at 01:18:11PM -0700, hpa@zytor.com wrote:
>>>>>
>>>>> Ok... I just realized this does not work for a modular initramfs,
>composed at load time from multiple files, which is a very real
>problem. Should be easy enough to deal with: instead of one large file,
>use one companion file per source file, perhaps something like
>filename..xattrs (suggesting double dots to make it less likely to
>conflict with a "real" file.) No leading dot, as it makes it more
>likely that archivers will sort them before the file proper.
>>>> This version of the patch was changed from the previous one exactly
>to deal with this case --
>>>> it allows for the bootloader to load multiple initramfs archives,
>each
>>>> with its own .xattr-list file, and to have that work properly.
>>>> Could you elaborate on the issue that you see?
>>>>
>>>
>>> Well, for one thing, how do you define "cpio archive", each with its
>own
>>> .xattr-list file? Second, that would seem to depend on the ordering,
>no,
>>> in which case you depend critically on .xattr-list file following
>the
>>> files, which most archivers won't do.
>>>
>>> Either way it seems cleaner to have this per file; especially if/as
>it
>>> can be done without actually mucking up the format.
>>>
>>> I need to run, but I'll post a more detailed explanation of what I
>did
>>> in a little bit.
>>>
>>> -hpa
>>>
>> Not sure what you mean by how do I define it? Each cpio archive will
>> contain its own .xattr-list file with signatures for the files within
>> it, that was the idea.
>>
>> You need to review the code more closely I think -- it does not
>depend
>> on the .xattr-list file following the files to which it applies.
>>
>> The code first extracts .xattr-list as though it was a regular file.
>If
>> a later dupe shows up (presumably from a second archive, although the
>> patch will actually allow a second one in the same archive), it will
>> then process the existing .xattr-list file and apply the attributes
>> listed within it. It then will proceed to read the second one and
>> overwrite the first one with it (this is the normal behaviour in the
>> kernel cpio parser). At the end once all the archives have been
>> extracted, if there is an .xattr-list file in the rootfs it will be
>> parsed (it would've been the last one encountered, which hasn't been
>> parsed yet, just extracted).
>>
>> Regarding the idea to use the high 16 bits of the mode field in
>> the header that's another possibility. It would just require
>additional
>> support in the program that actually creates the archive though,
>which
>> the current patch doesn't.
>
>Yes, for adding signatures for a subset of files, no changes to the ram
>disk generator are necessary. Everything is done by a custom module. To
>support a generic use case, it would be necessary to modify the
>generator to execute getfattr and the awk script after files have been
>placed in the temporary directory.
>
>If I understood the new proposal correctly, it would be task for cpio
>to
>read file metadata after the content and create a new record for each
>file with mode 0x18000, type of metadata encoded in the file name and
>metadata as file content. I don't know how easy it would be to modify
>cpio. Probably the amount of changes would be reasonable.
>
>The kernel will behave in a similar way. It will call do_readxattrs()
>in
>do_copy() for each file. Since the only difference between the current
>and the new proposal would be two additional calls to do_readxattrs()
>in
>do_name() and unpack_to_rootfs(), maybe we could support both.
>
>Roberto
The nice thing with explicit metadata is that it doesn't have to contain the filename per se, and each file is self-contained. There is a reason why each cpio header starts with the magic number: each cpio record is formally independent and can be processed in isolation. The TRAILER!!! thing is a huge wart in the format, although in practice TRAILER!!! always has a mode of 0 and so can be distinguished from an actual file.
The use of mode 0x18000 for metadata allows for optional backwards compatibility for extraction; for encoding this can be handled with very simple postprocessing.
So my suggestion would be to have mode 0x18000 indicate extended file metadata, with the filename of the form:
optional_filename!XXXXX!
... where XXXXX indicates the type of metadata (e.g. !XATTR!). The optional_filename prefix allows an unaware decoder to extract to a well-defined name; simple postprocessing would be able to either remove (for size) or add (for compatibility) this prefix. It would be an error for this prefix, if present, to not match the name of the previous file.
I do agree that the delayed processing of an .xattr-list as you describe ought to work even with a modular initramfs.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Sean Christopherson @ 2019-05-22 15:38 UTC (permalink / raw)
To: Stephen Smalley
Cc: Jarkko Sakkinen, Andy Lutomirski, James Morris, Serge E. Hallyn,
LSM List, Paul Moore, Eric Paris, selinux, Jethro Beekman,
Xing, Cedric, Hansen, Dave, Thomas Gleixner, Dr. Greg,
Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes
In-Reply-To: <0e183cce-c4b4-0e10-dbb6-bd81bea58b66@tycho.nsa.gov>
On Wed, May 22, 2019 at 09:56:30AM -0400, Stephen Smalley wrote:
> On 5/22/19 9:22 AM, Jarkko Sakkinen wrote:
> >On Wed, May 22, 2019 at 04:20:22PM +0300, Jarkko Sakkinen wrote:
> >>On Tue, May 21, 2019 at 08:51:40AM -0700, Sean Christopherson wrote:
> >>>Except that mmap() is more or less required to guarantee that ELRANGE
> >>>established by ECREATE is available. And we want to disallow mmap() as
> >>>soon as the first EADD is done so that userspace can't remap the enclave's
> >>>VMAs via munmap()->mmap() and gain execute permissions to pages that were
> >>>EADD'd as NX.
> >>
> >>We don't want to guarantee such thing and it is not guaranteed. It does
> >>not fit at all to the multi process work done. Enclaves are detached
> >>from any particular process addresse spaces. It is responsibility of
> >>process to open windows to them.
> >>
> >>That would be completely against work that we've done lately.
> >
> >Example use case: you have a process that just constructs an enclave
> >and sends it to another process or processes for use. The constructor
> >process could have basically anything on that range. This was the key
> >goal of the fd based enclave work.
>
> What exactly happens in the constructor versus the recipient processes?
> Which process performs each of the necessary open(), mmap(), and ioctl()
> calls for setting up the enclave? Can you provide a high level overview of
> the sequence of userspace calls by the constructor and by the recipient
> similar to what Sean showed earlier for just a single process?
Hmm, what we had talked about was allowing the SGX ioctls to work without
an associated VMA, with the end goal of letting userspace restrict access
to /dev/sgx/enclave. Very roughly...
Enclave Owner:
connect(builder, ...);
send(builder, "/home/sean/path/to/my/enclave");
recv(builder, &enclave_fd);
for_each_chunk {
mmap(enclave_addr + offset, size, ..., MAP_SHARED, enclave_fd, 0);
}
Enclave Builder:
recv(sock, &enclave_path);
source_fd = open(enclave_path, O_RDONLY);
for_each_chunk {
<hand waving - mmap()/mprotect() the enclave file into regular memory>
}
enclave_fd = open("/dev/sgx/enclave", O_RDWR);
ioctl(enclave_fd, ENCLAVE_CREATE, ...);
for_each_chunk {
struct sgx_enclave_add ioctlargs = {
.offset = chunk.offset,
.source = chunk.addr,
.size = chunk.size,
.type = chunk.type, /* SGX specific metadata */
}
ioctl(fd, ENCLAVE_ADD, &ioctlargs); /* modifies enclave's VMAs */
}
ioctl(enclave_fd, ENCLAVE_INIT, ...);
write(sock, enclave_fd);
But the above flow is flawed because there'a catch-22: ENCLAVE_ECREATE
takes the virtual address of the enclave, but in the above flow that's
not established until "mmap(..., enclave_fd)". And because an enclave's
virtual range needs to be naturally aligned (hardware requirements), the
enclave owner would need to do something like:
source_fd = open("/home/sean/path/to/my/enclave", O_RDONLY);
size = <parse size from source_fd>
enclave_range = mmap(NULL, size*2, PROT_READ, ???, NULL, 0);
enclave_addr = (enclave_range + (size - 1)) & ~(size - 1);
connect(builder, ...);
send(builder, {"/home/sean/path/to/my/enclave", enclave_addr});
recv(builder, &enclave_fd);
munmap(enclave_range);
for_each_chunk {
addr = mmap(enclave_addr + c.offset, c.size, ..., MAP_SHARED, enclave_fd, 0);
if (addr != enclave_addr + c.offset)
exit(1);
}
And that straight up doesn't work with the v20 driver because mmap() with
the enclave_fd will run through sgx_get_unmapped_area(), which also does
the natural alignment adjustments (the idea being that mmap() is mapping
the entire enclave). E.g. mmap() will map the wrong address if the offset
of a chunk is less than its size due to the driver adjusting the address.
Eliminating sgx_get_unmapped_area() means userspace is once again on the
hook for naturally aligning the enclave, which is less than desirable.
Looking back at the original API discussions around a builder process[1],
we never fleshed out the end-to-end flow. While having a builder process
*sounds* reasonable, in practice it adds a lot of complexity without
providing much in the way of added security. E.g. in addition to the
above mmap() issues, since the order of EADDs affects the enclave
measurement, the enclave owner would need to communicate the exact steps
to build the enclave, or the builder would need a priori knowledge of the
enclave format.
Userspace can still restrict access to /dev/sgx/enclave, e.g. by having a
daemon that requires additional credentials to obtain a new enclave_fd.
So AFAICT, the only benefit to having a dedicated builder is that it can
do its own whitelisting of enclaves, but since we're trending towards
supporting whitelisting enclaves in the kernel, e.g. via sigstruct,
whitelisting in userspace purely in userspace also provides marginal value.
TL;DR: Requiring VMA backing to build an enclave seems reasonable and sane.
[1] https://lkml.kernel.org/r/CALCETrX+KisMCbptrnPSO79-YF4E3nR1XHt+a7hCs1GXsxAbtw@mail.gmail.com
^ permalink raw reply
* Re: sleep in selinux_audit_rule_init
From: Stephen Smalley @ 2019-05-22 15:27 UTC (permalink / raw)
To: Mimi Zohar, Janne Karhunen, paul; +Cc: linux-integrity, linux-security-module
In-Reply-To: <1558533420.4347.30.camel@linux.ibm.com>
On 5/22/19 9:57 AM, Mimi Zohar wrote:
> On Wed, 2019-05-22 at 09:16 -0400, Stephen Smalley wrote:
>> On 5/22/19 9:00 AM, Mimi Zohar wrote:
>>> On Wed, 2019-05-22 at 08:41 -0400, Stephen Smalley wrote:
>>>> Another potentially worrisome aspect of the current
>>>> ima_lsm_update_rules() logic is that it does a BUG_ON() if the attempt
>>>> to update the rule fails, which could occur if e.g. one had an IMA
>>>> policy rule based on a given domain/type and that domain/type were
>>>> removed from policy (e.g. via policy module removal). Contrast with the
>>>> handling in audit_dupe_lsm_field(). The existing ima_lsm_update_rules()
>>>> logic could also yield a BUG_ON upon transient memory allocation failure.
>>>
>>> The original design was based on the assumption that SELinux labels
>>> could not be removed, only new ones could be added. Sounds like that
>>> isn't the case any longer.
>>
>> That's never really been the case for SELinux; it has always been
>> possible to reload with a policy that renders previously valid security
>> contexts invalid. What has changed over time is the ability of SELinux
>> to gracefully handle the situation where a security context is rendered
>> invalid upon a policy reload and then later restored to validity via a
>> subsequent policy reload (e.g. removing a policy module and then
>> re-adding it), but even that deferred mapping of contexts support has
>> been around since 2008.
>>
>> What you are likely thinking of is the conventional practice of
>> distributions, which is generally to not remove domains/types from their
>> policy or to at least retain a type alias for compatibility reasons.
>> But that's just a convention, not guaranteed by any mechanism, and users
>> are free to remove policy modules.
>
> Ok. The question is then how should IMA handle missing domains/types.
> Just dropping IMA policy rules doesn't sound safe, nor does skipping
> rules in case the domains/types are restored.
You can just do what audit_dupe_lsm_field() does. It effectively
disables the rule upon the invalidation (which makes sense, since it can
no longer match anything since nothing can have that domain/type) but
retains the string value so it can later re-activate the rule if the
domain/type becomes valid again later.
^ permalink raw reply
* Re: sleep in selinux_audit_rule_init
From: Casey Schaufler @ 2019-05-22 15:10 UTC (permalink / raw)
To: Mimi Zohar, Stephen Smalley, Janne Karhunen, paul
Cc: linux-integrity, linux-security-module
In-Reply-To: <1558533420.4347.30.camel@linux.ibm.com>
On 5/22/2019 6:57 AM, Mimi Zohar wrote:
> On Wed, 2019-05-22 at 09:16 -0400, Stephen Smalley wrote:
>> On 5/22/19 9:00 AM, Mimi Zohar wrote:
>>> On Wed, 2019-05-22 at 08:41 -0400, Stephen Smalley wrote:
>>>> Another potentially worrisome aspect of the current
>>>> ima_lsm_update_rules() logic is that it does a BUG_ON() if the attempt
>>>> to update the rule fails, which could occur if e.g. one had an IMA
>>>> policy rule based on a given domain/type and that domain/type were
>>>> removed from policy (e.g. via policy module removal). Contrast with the
>>>> handling in audit_dupe_lsm_field(). The existing ima_lsm_update_rules()
>>>> logic could also yield a BUG_ON upon transient memory allocation failure.
>>> The original design was based on the assumption that SELinux labels
>>> could not be removed, only new ones could be added. ??Sounds like that
>>> isn't the case any longer.
>> That's never really been the case for SELinux; it has always been
>> possible to reload with a policy that renders previously valid security
>> contexts invalid. What has changed over time is the ability of SELinux
>> to gracefully handle the situation where a security context is rendered
>> invalid upon a policy reload and then later restored to validity via a
>> subsequent policy reload (e.g. removing a policy module and then
>> re-adding it), but even that deferred mapping of contexts support has
>> been around since 2008.
>>
>> What you are likely thinking of is the conventional practice of
>> distributions, which is generally to not remove domains/types from their
>> policy or to at least retain a type alias for compatibility reasons.
>> But that's just a convention, not guaranteed by any mechanism, and users
>> are free to remove policy modules.
> Ok. ??The question is then how should IMA handle missing domains/types.
> ??Just dropping IMA policy rules doesn't sound safe, nor does skipping
> rules in case the domains/types are restored.
Smack has a case where the subject label might never have been
seen by the system before, and hence can't be in any rules. This
can occur when a labeled packet comes from another host. Because
a subject with the star ("*") label is never allowed access to
anything, that is a convenient value to use. It is never used as
the subject label otherwise.
You could do something similar if there is a SELinux domain/type
that you can rely on being present. I fear that there may not be
any such element, but it wouldn't hurt (too much) too look.
>
> Mimi ??
>
>
^ permalink raw reply
* Re: sleep in selinux_audit_rule_init
From: Mimi Zohar @ 2019-05-22 13:57 UTC (permalink / raw)
To: Stephen Smalley, Janne Karhunen, paul
Cc: linux-integrity, linux-security-module
In-Reply-To: <4db98b76-8637-edf6-c7df-3e244be0f11e@tycho.nsa.gov>
On Wed, 2019-05-22 at 09:16 -0400, Stephen Smalley wrote:
> On 5/22/19 9:00 AM, Mimi Zohar wrote:
> > On Wed, 2019-05-22 at 08:41 -0400, Stephen Smalley wrote:
> >> Another potentially worrisome aspect of the current
> >> ima_lsm_update_rules() logic is that it does a BUG_ON() if the attempt
> >> to update the rule fails, which could occur if e.g. one had an IMA
> >> policy rule based on a given domain/type and that domain/type were
> >> removed from policy (e.g. via policy module removal). Contrast with the
> >> handling in audit_dupe_lsm_field(). The existing ima_lsm_update_rules()
> >> logic could also yield a BUG_ON upon transient memory allocation failure.
> >
> > The original design was based on the assumption that SELinux labels
> > could not be removed, only new ones could be added. Sounds like that
> > isn't the case any longer.
>
> That's never really been the case for SELinux; it has always been
> possible to reload with a policy that renders previously valid security
> contexts invalid. What has changed over time is the ability of SELinux
> to gracefully handle the situation where a security context is rendered
> invalid upon a policy reload and then later restored to validity via a
> subsequent policy reload (e.g. removing a policy module and then
> re-adding it), but even that deferred mapping of contexts support has
> been around since 2008.
>
> What you are likely thinking of is the conventional practice of
> distributions, which is generally to not remove domains/types from their
> policy or to at least retain a type alias for compatibility reasons.
> But that's just a convention, not guaranteed by any mechanism, and users
> are free to remove policy modules.
Ok. The question is then how should IMA handle missing domains/types.
Just dropping IMA policy rules doesn't sound safe, nor does skipping
rules in case the domains/types are restored.
Mimi
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Stephen Smalley @ 2019-05-22 13:56 UTC (permalink / raw)
To: Jarkko Sakkinen, Sean Christopherson
Cc: Andy Lutomirski, James Morris, Serge E. Hallyn, LSM List,
Paul Moore, Eric Paris, selinux, Jethro Beekman, Xing, Cedric,
Hansen, Dave, Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML,
X86 ML, linux-sgx@vger.kernel.org, Andrew Morton,
nhorman@redhat.com, npmccallum@redhat.com, Ayoun, Serge,
Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko, Svahn, Kai,
Borislav Petkov, Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <20190522132227.GD31176@linux.intel.com>
On 5/22/19 9:22 AM, Jarkko Sakkinen wrote:
> On Wed, May 22, 2019 at 04:20:22PM +0300, Jarkko Sakkinen wrote:
>> On Tue, May 21, 2019 at 08:51:40AM -0700, Sean Christopherson wrote:
>>> Except that mmap() is more or less required to guarantee that ELRANGE
>>> established by ECREATE is available. And we want to disallow mmap() as
>>> soon as the first EADD is done so that userspace can't remap the enclave's
>>> VMAs via munmap()->mmap() and gain execute permissions to pages that were
>>> EADD'd as NX.
>>
>> We don't want to guarantee such thing and it is not guaranteed. It does
>> not fit at all to the multi process work done. Enclaves are detached
>> from any particular process addresse spaces. It is responsibility of
>> process to open windows to them.
>>
>> That would be completely against work that we've done lately.
>
> Example use case: you have a process that just constructs an enclave
> and sends it to another process or processes for use. The constructor
> process could have basically anything on that range. This was the key
> goal of the fd based enclave work.
What exactly happens in the constructor versus the recipient processes?
Which process performs each of the necessary open(), mmap(), and ioctl()
calls for setting up the enclave? Can you provide a high level overview
of the sequence of userspace calls by the constructor and by the
recipient similar to what Sean showed earlier for just a single process?
^ permalink raw reply
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