Re: [PATCH] userns/capability: Add user namespace capability

[Richard Weinberger]

Am 19.10.2015 um 14:36 schrieb Yves-Alexis Perez:
> On dim., 2015-10-18 at 20:41 -0500, Serge E. Hallyn wrote:
>> We shouldn't need a long-term solution.  Your concern is bugs.  After
>> some time surely we'll feel that we have achieved a stable solution?
> 
> But this is actually the whole point: we need a long term solution, because
> they will always be bug, whether in user namespaces or in others parts exposed
> by user namespaces. It's fine to fix them when we find them, but that still
> means they're exploitable even before we know about them. We still find bugs
> in code written years ago, it's quite certain there are bugs in current code.

You can replace the term "user namespace" with any other non-trivial kernel 
subsystem.
There will always be bugs.

Thanks,
//richard
--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PULL REQUEST] IMA changes for 4.4

[Mimi Zohar]

Hi James,

This pull request is for a single bug fix from Dimtry to properly load
only signed certificates onto the trusted IMA keyring from the kernel.
(This patch has been in the linux-next tree).

thanks,

Mimi

The following changes since commit
049e6dde7e57f0054fdc49102e7ef4830c698b46:

  Linux 4.3-rc4 (2015-10-04 16:57:17 +0100)

are available in the git repository at:


git://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git
next

for you to fetch changes up to 72e1eed8abb11c79749266d433c817ce36732893:

  integrity: prevent loading untrusted certificates on the IMA trusted
keyring (2015-10-09 15:31:18 -0400)


Dmitry Kasatkin (1):
  integrity: prevent loading untrusted certificates on the IMA
trusted keyring

 security/integrity/digsig.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] userns/capability: Add user namespace capability

[Yves-Alexis Perez]

On dim., 2015-10-18 at 20:41 -0500, Serge E. Hallyn wrote:
> We shouldn't need a long-term solution.  Your concern is bugs.  After
> some time surely we'll feel that we have achieved a stable solution?

But this is actually the whole point: we need a long term solution, because
they will always be bug, whether in user namespaces or in others parts exposed
by user namespaces. It's fine to fix them when we find them, but that still
means they're exploitable even before we know about them. We still find bugs
in code written years ago, it's quite certain there are bugs in current code.

User namespaces are a way to expose more interfaces to unprivileged users,
interfaces which weren't designed to be exposed like that. In a way that's the
opposite of seccomp. That doesn't make it bad, obviously, but that still means
having a way to control it finely could be helpful.

Regards,
-- 

Yves-Alexis


signature.asc
Description: This is a digitally signed message part

Re: GPF in keyring_destroy

[David Howells]

Dmitry Vyukov  wrote:

> > Does the attached patch fix it for you?
> 
> Yes, it fixes the crash for me.

Can I put you down as a Tested-by?

David
--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: GPF in keyring_destroy

[Dmitry Vyukov]

On Thu, Oct 15, 2015 at 9:21 PM, David Howells  wrote:
> Does the attached patch fix it for you?

Yes, it fixes the crash for me.


> David
> ---
> commit a7609e0bb3973d6ee3c9f1ecd0b6a382d99d6248
> Author: David Howells 
> Date:   Thu Oct 15 17:21:37 2015 +0100
>
> KEYS: Fix crash when attempt to garbage collect an uninstantiated keyring
>
> The following sequence of commands:
>
> i=`keyctl add user a a @s`
> keyctl request2 keyring foo bar @t
> keyctl unlink $i @s
>
> tries to invoke an upcall to instantiate a keyring if one doesn't already
> exist by that name within the user's keyring set.  However, if the upcall
> fails, the code sets keyring->type_data.reject_error to -ENOKEY or some
> other error code.  When the key is garbage collected, the key destroy
> function is called unconditionally and keyring_destroy() uses list_empty()
> on keyring->type_data.link - which is in a union with reject_error.
> Subsequently, the kernel tries to unlink the keyring from the keyring 
> names
> list - which oopses like this:
>
> BUG: unable to handle kernel paging request at ff8a
> IP: [] keyring_destroy+0x3d/0x88
> ...
> Workqueue: events key_garbage_collector
> ...
> RIP: 0010:[] keyring_destroy+0x3d/0x88
> RSP: 0018:88003e2f3d30  EFLAGS: 00010203
> RAX: ff82 RBX: 88003bf1a900 RCX: 
> RDX:  RSI: 3bfc6901 RDI: 81a73a40
> RBP: 88003e2f3d38 R08: 0152 R09: 
> R10: 88003e2f3c18 R11: 865b R12: 88003bf1a900
> R13:  R14: 88003bf1a908 R15: 88003e2f4000
> ...
> CR2: ff8a CR3: 3e3ec000 CR4: 06f0
> ...
> Call Trace:
>  [] key_gc_unused_keys.constprop.1+0x5d/0x10f
>  [] key_garbage_collector+0x1fa/0x351
>  [] process_one_work+0x28e/0x547
>  [] worker_thread+0x26e/0x361
>  [] ? rescuer_thread+0x2a8/0x2a8
>  [] kthread+0xf3/0xfb
>  [] ? kthread_create_on_node+0x1c2/0x1c2
>  [] ret_from_fork+0x3f/0x70
>  [] ? kthread_create_on_node+0x1c2/0x1c2
>
> Note the value in RAX.  This is a 32-bit representation of -ENOKEY.
>
> The solution is to only call ->destroy() if the key was successfully
> instantiated.
>
> Reported-by: Dmitry Vyukov 
> Signed-off-by: David Howells 
>
> diff --git a/security/keys/gc.c b/security/keys/gc.c
> index 39eac1fd5706..addf060399e0 100644
> --- a/security/keys/gc.c
> +++ b/security/keys/gc.c
> @@ -134,8 +134,10 @@ static noinline void key_gc_unused_keys(struct list_head 
> *keys)
> kdebug("- %u", key->serial);
> key_check(key);
>
> -   /* Throw away the key data */
> -   if (key->type->destroy)
> +   /* Throw away the key data if the key is instantiated */
> +   if (test_bit(KEY_FLAG_INSTANTIATED, >flags) &&
> +   !test_bit(KEY_FLAG_NEGATIVE, >flags) &&
> +   key->type->destroy)
> key->type->destroy(key);
>
> security_key_free(key);
--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] userns/capability: Add user namespace capability

[Austin S Hemmelgarn]

On 2015-10-17 11:58, Tobias Markus wrote:

Add capability CAP_SYS_USER_NS.
Tasks having CAP_SYS_USER_NS are allowed to create a new user namespace
when calling clone or unshare with CLONE_NEWUSER.

Rationale:

Linux 3.8 saw the introduction of unpriviledged user namespaces,
allowing unpriviledged users (without CAP_SYS_ADMIN) to be a "fake" root
inside a separate user namespace. Before that, any namespace creation
required CAP_SYS_ADMIN (or, in practice, the user had to be root).
Unfortunately, there have been some security-relevant bugs in the
meantime. Because of the fairly complex nature of user namespaces, it is
reasonable to say that future vulnerabilties can not be excluded. Some
distributions even wholly disable user namespaces because of this.

Both options, user namespaces with and without CAP_SYS_ADMIN, can be
said to represent the extreme end of the spectrum. In practice, there is
no reason for every process to have the abilitiy to create user
namespaces. Indeed, only very few and specialized programs require user
namespaces. This seems to be a perfect fit for the (file) capability
system: Priviledged users could manually allow only a certain executable
to be able to create user namespaces by setting a certain capability,
I'd suggest the name CAP_SYS_USER_NS. Executables completely unrelated
to user namespaces should and can not create them.

The capability should only be required in the "root" user namespace (the
user namespace with level 0) though, to allow nested user namespaces to
work as intended. If a user namespace has a level greater than 0, the
original process must have had CAP_SYS_USER_NS, so it is "trusted" anyway.

One question remains though: Does this break userspace executables that
expect being able to create user namespaces without priviledge? Since
creating user namespaces without CAP_SYS_ADMIN was not possible before
Linux 3.8, programs should already expect a potential EPERM upon calling
clone. Since creating a user namespace without CAP_SYS_USER_NS would
also cause EPERM, we should be on the safe side.


Potentially stupid counter proposal:
Make it CAP_SYS_NS, make it allow access to all namespace types for 
non-root/CAP_SYS_ADMIN users, and teach the stuff that's using userns 
just to get to mount/pid/net/ipc namespaces to use those instead when 
it's something that doesn't really need to think it's running as root.


While this would still add a new capability (which is arguably not a 
good thing), the resultant capability would be significantly more useful 
for many of the use cases.


Potentially more flame resistant counter proposal:
Write a simple LSM to allow selective usage of namespaces (IIRC, working 
LSM stacking is in mainline now).  While this is more complicated than 
just adding a capability, it is also a lot more resilient from a long 
term prospective.




smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH 2/2] KEYS: Don't permit request_key() to construct a new keyring

[David Howells]

If request_key() is used to find a keyring, only do the search part - don't
do the construction part if the keyring was not found by the search.  We
don't really want keyrings in the negative instantiated state since the
rejected/negative instantiation error value in the payload is unioned with
keyring metadata.

Now the kernel gives an error:

request_key("keyring", "#selinux,bdekeyring", "keyring", 
KEY_SPEC_USER_SESSION_KEYRING) = -1 EPERM (Operation not permitted)

Signed-off-by: David Howells 
---

 security/keys/request_key.c |3 +++
 1 file changed, 3 insertions(+)

diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 486ef6fa393b..0d6253124278 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -440,6 +440,9 @@ static struct key *construct_key_and_link(struct 
keyring_search_context *ctx,
 
kenter("");
 
+   if (ctx->index_key.type == _type_keyring)
+   return ERR_PTR(-EPERM);
+   
user = key_user_lookup(current_fsuid());
if (!user)
return ERR_PTR(-ENOMEM);

--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] KEYS: Fix crash when attempt to garbage collect an uninstantiated keyring

[David Howells]

The following sequence of commands:

i=`keyctl add user a a @s`
keyctl request2 keyring foo bar @t
keyctl unlink $i @s

tries to invoke an upcall to instantiate a keyring if one doesn't already
exist by that name within the user's keyring set.  However, if the upcall
fails, the code sets keyring->type_data.reject_error to -ENOKEY or some
other error code.  When the key is garbage collected, the key destroy
function is called unconditionally and keyring_destroy() uses list_empty()
on keyring->type_data.link - which is in a union with reject_error.
Subsequently, the kernel tries to unlink the keyring from the keyring names
list - which oopses like this:

BUG: unable to handle kernel paging request at ff8a
IP: [] keyring_destroy+0x3d/0x88
...
Workqueue: events key_garbage_collector
...
RIP: 0010:[] keyring_destroy+0x3d/0x88
RSP: 0018:88003e2f3d30  EFLAGS: 00010203
RAX: ff82 RBX: 88003bf1a900 RCX: 
RDX:  RSI: 3bfc6901 RDI: 81a73a40
RBP: 88003e2f3d38 R08: 0152 R09: 
R10: 88003e2f3c18 R11: 865b R12: 88003bf1a900
R13:  R14: 88003bf1a908 R15: 88003e2f4000
...
CR2: ff8a CR3: 3e3ec000 CR4: 06f0
...
Call Trace:
 [] key_gc_unused_keys.constprop.1+0x5d/0x10f
 [] key_garbage_collector+0x1fa/0x351
 [] process_one_work+0x28e/0x547
 [] worker_thread+0x26e/0x361
 [] ? rescuer_thread+0x2a8/0x2a8
 [] kthread+0xf3/0xfb
 [] ? kthread_create_on_node+0x1c2/0x1c2
 [] ret_from_fork+0x3f/0x70
 [] ? kthread_create_on_node+0x1c2/0x1c2

Note the value in RAX.  This is a 32-bit representation of -ENOKEY.

The solution is to only call ->destroy() if the key was successfully
instantiated.

Reported-by: Dmitry Vyukov 
Signed-off-by: David Howells 
Tested-by: Dmitry Vyukov 
---

 security/keys/gc.c |6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/security/keys/gc.c b/security/keys/gc.c
index 39eac1fd5706..addf060399e0 100644
--- a/security/keys/gc.c
+++ b/security/keys/gc.c
@@ -134,8 +134,10 @@ static noinline void key_gc_unused_keys(struct list_head 
*keys)
kdebug("- %u", key->serial);
key_check(key);
 
-   /* Throw away the key data */
-   if (key->type->destroy)
+   /* Throw away the key data if the key is instantiated */
+   if (test_bit(KEY_FLAG_INSTANTIATED, >flags) &&
+   !test_bit(KEY_FLAG_NEGATIVE, >flags) &&
+   key->type->destroy)
key->type->destroy(key);
 
security_key_free(key);

--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/3] Allows reading back the current IMA policy;

[Mimi Zohar]

On Fri, 2015-10-16 at 22:31 +0300, Petko Manolov wrote:
> When in development it is useful to read back the IMA policy.  This patch
> provides the functionality.  However, this is a potential security hole so
> it should not be used in production-grade kernels.
 
Like the other IMA securityfs files, only root would be able to read it.
Once we start allowing additional rules to be appended to the policy,
being able to view the resulting policy is important.  Is there a reason
for limiting this option to development?

(Still reviewing the code ...)

Mimi

> Signed-off-by: Petko Manolov 
> ---
>  security/integrity/ima/Kconfig  |  13 +++
>  security/integrity/ima/ima.h|  13 ++-
>  security/integrity/ima/ima_fs.c |  29 +-
>  security/integrity/ima/ima_policy.c | 190 
> 
>  4 files changed, 239 insertions(+), 6 deletions(-)
> 
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index 235b3c2..040778f 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -121,6 +121,19 @@ config IMA_WRITE_POLICY
> 
> If unsure, say N.
> 
> +config IMA_READ_POLICY
> + bool "Enable reading back the current IMA policy"
> + depends on IMA
> + default n
> + help
> +When developing and debugging an IMA enabled system it is often
> +useful to be able to read the IMA policy.  This patch allows
> +for doing so.  However, being able to read the IMA policy is
> +considered insecure and is strongly discouraged for
> +production-grade kernels.
> +
> +If unsure, say N.
> +
>  config IMA_APPRAISE
>   bool "Appraise integrity measurements"
>   depends on IMA
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index e2a60c3..ebc3554 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -166,6 +166,10 @@ void ima_update_policy(void);
>  void ima_update_policy_flag(void);
>  ssize_t ima_parse_add_rule(char *);
>  void ima_delete_rules(void);
> +void *ima_policy_start(struct seq_file *m, loff_t *pos);
> +void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos);
> +void ima_policy_stop(struct seq_file *m, void *v);
> +int ima_policy_show(struct seq_file *m, void *v);
> 
>  /* Appraise integrity measurements */
>  #define IMA_APPRAISE_ENFORCE 0x01
> @@ -263,4 +267,11 @@ static inline int ima_init_keyring(const unsigned int id)
>   return 0;
>  }
>  #endif /* CONFIG_IMA_TRUSTED_KEYRING */
> -#endif
> +
> +#ifdef   CONFIG_IMA_READ_POLICY
> +#define  POLICY_FILE_FLAGS   (S_IWUSR | S_IRUSR)
> +#else
> +#define  POLICY_FILE_FLAGS   S_IWUSR
> +#endif /* CONFIG_IMA_WRITE_POLICY */
> +
> +#endif /* __LINUX_IMA_H */
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index a3cf5c0..85bd129 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -311,14 +311,29 @@ enum ima_fs_flags {
> 
>  static unsigned long ima_fs_flags;
> 
> +#ifdef   CONFIG_IMA_READ_POLICY
> +static const struct seq_operations ima_policy_seqops = {
> + .start = ima_policy_start,
> + .next = ima_policy_next,
> + .stop = ima_policy_stop,
> + .show = ima_policy_show,
> +};
> +#endif
> +
>  /*
>   * ima_open_policy: sequentialize access to the policy file
>   */
>  static int ima_open_policy(struct inode *inode, struct file *filp)
>  {
> - /* No point in being allowed to open it if you aren't going to write */
> - if (!(filp->f_flags & O_WRONLY))
> + if (!(filp->f_flags & O_WRONLY)) {
> +#ifndef  CONFIG_IMA_READ_POLICY
>   return -EACCES;
> +#else
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
> + return seq_open(filp, _policy_seqops);
> +#endif
> + }
>   if (test_and_set_bit(IMA_FS_BUSY, _fs_flags))
>   return -EBUSY;
>   return 0;
> @@ -334,6 +349,10 @@ static int ima_open_policy(struct inode *inode, struct 
> file *filp)
>  static int ima_release_policy(struct inode *inode, struct file *file)
>  {
>   const char *cause = valid_policy ? "completed" : "failed";
> + int policy_write = (file->f_flags & O_WRONLY);
> +
> + if (!policy_write)
> + return 0;
> 
>   pr_info("IMA: policy update %s\n", cause);
>   integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL,
> @@ -342,9 +361,9 @@ static int ima_release_policy(struct inode *inode, struct 
> file *file)
>   if (!valid_policy) {
>   ima_delete_rules();
>   valid_policy = 1;
> - clear_bit(IMA_FS_BUSY, _fs_flags);
>   return 0;
>   }
> +
>   ima_update_policy();
>  #ifndef  CONFIG_IMA_WRITE_POLICY
>   securityfs_remove(ima_policy);
> @@ -358,6 +377,7 @@ static int ima_release_policy(struct inode *inode, struct 
> file *file)
> 

Re: [PATCH v5] Smack: limited capability for changing process label

[Casey Schaufler]

On 10/19/2015 9:23 AM, Rafal Krypa wrote:
> From: Zbigniew Jasinski 
>
> This feature introduces new kernel interface:
>
> - /relabel-self - for setting transition labels list
>
> This list is used to control smack label transition mechanism.
> List is set by, and per process. Process can transit to new label only if
> label is on the list. Only process with CAP_MAC_ADMIN capability can add
> labels to this list. With this list, process can change it's label without
> CAP_MAC_ADMIN but only once. After label changing, list is unset.
>
> Changes in v2:
> * use list_for_each_entry instead of _rcu during label write
> * added missing description in security/Smack.txt
>
> Changes in v3:
> * squashed into one commit
>
> Changes in v4:
> * switch from global list to per-task list
> * since the per-task list is accessed only by the task itself
>   there is no need to use synchronization mechanisms on it
>
> Changes in v5:
> * change smackfs interface of relabel-self to the one used for onlycap
>   multiple labels are accepted, separated by space, which
>   replace the previous list upon write
>
> Signed-off-by: Zbigniew Jasinski 
> Signed-off-by: Rafal Krypa 

Acked-by: Casey Schaufler 
Applied-to: https://github.com/cschaufler/smack-next.git#smack-for-4.4

> ---
>  Documentation/security/Smack.txt |  10 ++
>  security/smack/smack.h   |   4 +-
>  security/smack/smack_access.c|   6 +-
>  security/smack/smack_lsm.c   |  57 ++-
>  security/smack/smackfs.c | 202 
> ---
>  5 files changed, 238 insertions(+), 41 deletions(-)
>
> diff --git a/Documentation/security/Smack.txt 
> b/Documentation/security/Smack.txt
> index 5e6d07f..945cc63 100644
> --- a/Documentation/security/Smack.txt
> +++ b/Documentation/security/Smack.txt
> @@ -255,6 +255,16 @@ unconfined
>   the access permitted if it wouldn't be otherwise. Note that this
>   is dangerous and can ruin the proper labeling of your system.
>   It should never be used in production.
> +relabel-self
> + This interface contains a list of labels to which the process can
> + transition to, by writing to /proc/self/attr/current.
> + Normally a process can change its own label to any legal value, but only
> + if it has CAP_MAC_ADMIN. This interface allows a process without
> + CAP_MAC_ADMIN to relabel itself to one of labels from predefined list.
> + A process without CAP_MAC_ADMIN can change its label only once. When it
> + does, this list will be cleared.
> + The values are set by writing the desired labels, separated
> + by spaces, to the file or cleared by writing "-" to the file.
>  
>  If you are using the smackload utility
>  you can add access rules in /etc/smack/accesses. They take the form:
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index fff0c61..6c91156 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -115,6 +115,7 @@ struct task_smack {
>   struct smack_known  *smk_forked;/* label when forked */
>   struct list_headsmk_rules;  /* per task access rules */
>   struct mutexsmk_rules_lock; /* lock for the rules */
> + struct list_headsmk_relabel;/* transit allowed labels */
>  };
>  
>  #define  SMK_INODE_INSTANT   0x01/* inode is instantiated */
> @@ -169,7 +170,7 @@ struct smk_port_label {
>  };
>  #endif /* SMACK_IPV6_PORT_LABELING */
>  
> -struct smack_onlycap {
> +struct smack_known_list_elem {
>   struct list_headlist;
>   struct smack_known  *smk_label;
>  };
> @@ -301,6 +302,7 @@ struct smack_known *smk_import_entry(const char *, int);
>  void smk_insert_entry(struct smack_known *skp);
>  struct smack_known *smk_find_entry(const char *);
>  int smack_privileged(int cap);
> +void smk_destroy_label_list(struct list_head *list);
>  
>  /*
>   * Shared data.
> diff --git a/security/smack/smack_access.c b/security/smack/smack_access.c
> index bc1053f..a283f9e 100644
> --- a/security/smack/smack_access.c
> +++ b/security/smack/smack_access.c
> @@ -637,7 +637,7 @@ DEFINE_MUTEX(smack_onlycap_lock);
>  int smack_privileged(int cap)
>  {
>   struct smack_known *skp = smk_of_current();
> - struct smack_onlycap *sop;
> + struct smack_known_list_elem *sklep;
>  
>   /*
>* All kernel tasks are privileged
> @@ -654,8 +654,8 @@ int smack_privileged(int cap)
>   return 1;
>   }
>  
> - list_for_each_entry_rcu(sop, _onlycap_list, list) {
> - if (sop->smk_label == skp) {
> + list_for_each_entry_rcu(sklep, _onlycap_list, list) {
> + if (sklep->smk_label == skp) {
>   rcu_read_unlock();
>   return 1;
>   }
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 996c889..036d8b2 100644
> --- 

[PULL] Smack - Changes for 4.4

[Casey Schaufler]

The following changes since commit 049e6dde7e57f0054fdc49102e7ef4830c698b46:

  Linux 4.3-rc4 (2015-10-04 16:57:17 +0100)

are available in the git repository at:

  https://github.com/cschaufler/smack-next.git smack-for-4.4

for you to fetch changes up to 38416e53936ecf896948fdeffc36b76979117952:

  Smack: limited capability for changing process label (2015-10-19
12:06:47 -0700)


Geliang Tang (1):
  smack: smk_ipv6_port_list should be static

José Bollo (1):
  Smack: Minor initialisation improvement

Lukasz Pawelczyk (1):
  Smack: fix a NULL dereference in wrong smack_import_entry() usage

Roman Kubiak (1):
  Smack: pipefs fix in smack_d_instantiate

Zbigniew Jasinski (1):
  Smack: limited capability for changing process label

 Documentation/security/Smack.txt |  10 ++
 security/smack/smack.h   |   4 +-
 security/smack/smack_access.c|   6 +-
 security/smack/smack_lsm.c   |  67 -
 security/smack/smackfs.c | 208
---
 5 files changed, 248 insertions(+), 47 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v3 2/5] lsm: introduce hooks for kdbus

[Paul Moore]

On Friday, October 09, 2015 10:56:12 AM Stephen Smalley wrote:
> On 10/07/2015 07:08 PM, Paul Moore wrote:
> > diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
> > index ef63d65..1cb87b3 100644
> > --- a/ipc/kdbus/connection.c
> > +++ b/ipc/kdbus/connection.c
> > @@ -108,6 +109,14 @@ static struct kdbus_conn *kdbus_conn_new(struct
> > kdbus_ep *ep,> 
> > if (!owner && (creds || pids || seclabel))
> > 
> > return ERR_PTR(-EPERM);
> > 
> > +   ret = security_kdbus_conn_new(get_cred(file->f_cred),
> 
> You only need to use get_cred() if saving a reference; otherwise, you'll
> leak one here.

Yes, that was a typo on my part, thanks.

> Also, do we want file->f_cred here or ep->bus->node.creds (the latter is
> what is used by their own checks; the former is typically the same as
> current cred IIUC).  For that matter, what about ep->node.creds vs ep->bus-
> node.creds vs. ep->bus->domain->node.creds?  Can they differ?  Do we care?

We don't want file->f_cred, per our previous discussions.  I was working on 
this patchset in small chunks and while I added credential storing in the 
nodes, I forgot to update the hooks before I hit send, my apologies.

My current thinking is to pass both the endpoint and bus credentials, as I 
believe they can differ.  Both the bus and the endpoint inherit their security 
labels from their creator and while I don't have any specifics, I think it is 
reasonable to imagine those two processes having different security labels.  
Assuming we pass both credentials down to the LSM, I'm currently thinking of 
the following SELinux access controls for this hook:

  allow  bus_t:kdbus { connect };
  allow  ep_t:kdbus { use privileged activator monitor policy };

... besides the additional label, I added the kdbus:use permission and dropped 
the kdbus:owner permission.  Considering that the endpoint label, ep_t, in the 
examples above, could be different from the current process, it seemed 
reasonable to want to control that interaction and I felt the fd:use 
permission was the closest existing control so I reused the permission name.  
I decided to drop the "owner" permission as it really wasn't the useful for 
anything anymore, it simply indicates that the current task is the DAC owner 
of the endpoint.

> > @@ -1435,12 +1444,12 @@ bool kdbus_conn_policy_own_name(struct kdbus_conn
> > *conn,> 
> > return false;
> > 
> > }
> > 
> > -   if (conn->owner)
> > -   return true;
> > +   if (!conn->owner &&
> > +   kdbus_policy_query(>ep->bus->policy_db, conn_creds, name,
> > +  hash) < KDBUS_POLICY_OWN)
> > +   return false;
> > 
> > -   res = kdbus_policy_query(>ep->bus->policy_db, conn_creds,
> > -name, hash);
> > -   return res >= KDBUS_POLICY_OWN;
> > +   return (security_kdbus_own_name(conn_creds, name) == 0);
> 
> Similar question here.  conn_creds is the credentials of the creator of
> the connection, typically the client/sender, right?
> conn->ep->bus->node.creds are the credentials of the bus owner, so don't
> we want to ask "Can I own this name on this bus?".

Yes, I think so.

>From a SELinux point of view I imagine we would want access controls along the 
lines of the following:

  allow current name_t:kdbus { own_name };
  allow current bus_t:kdbus { own_name };

... do we want to use different permissions?  I doubt it would matter much 
either way.

> Note that their policy checks are based on conn->ep->policy_db, i.e. the
> policy associated with the endpoint, and conn->owner is only true if the
> connection creator has the same uid as the bus.

I don't think this is significant for us.

> > @@ -1465,14 +1474,13 @@ bool kdbus_conn_policy_talk(struct kdbus_conn
> > *conn,> 
> >  to, KDBUS_POLICY_TALK))
> > 
> > return false;
> > 
> > -   if (conn->owner)
> > -   return true;
> > -   if (uid_eq(conn_creds->euid, to->cred->uid))
> > -   return true;
> > +   if (!conn->owner && !uid_eq(conn_creds->euid, to->cred->uid) &&
> > +   !kdbus_conn_policy_query_all(conn, conn_creds,
> > +>ep->bus->policy_db, to,
> > +KDBUS_POLICY_TALK))
> > +   return false;
> > 
> > -   return kdbus_conn_policy_query_all(conn, conn_creds,
> > -  >ep->bus->policy_db, to,
> > -  KDBUS_POLICY_TALK);
> > +   return (security_kdbus_conn_talk(conn_creds, to->cred) == 0);
> 
> Here at least we have a notion of client and peer.  But we still aren't
> considering conn->ep or conn->ep->bus, whereas they are querying both
> policy dbs for their decision.  The parallel would be checking access to
> the labels of both I suppose, unless we institute a control up front
> over the relationship between the label of the endpoint and the label of
> the bus.

While 

Re: [PATCH v4 1/3] Enable multiple writes to the IMA policy;

[Mimi Zohar]

On Mon, 2015-10-19 at 14:21 -0400, Mimi Zohar wrote:
> On Fri, 2015-10-16 at 22:31 +0300, Petko Manolov wrote:

> > diff --git a/security/integrity/ima/ima_fs.c 
> > b/security/integrity/ima/ima_fs.c
> > index 816d175..a3cf5c0 100644
> > --- a/security/integrity/ima/ima_fs.c
> > +++ b/security/integrity/ima/ima_fs.c
> > @@ -25,6 +25,8 @@
> > 
> >  #include "ima.h"
> > 
> > +static DEFINE_MUTEX(ima_write_mutex);
> > +
> >  static int valid_policy = 1;
> >  #define TMPBUFLEN 12
> >  static ssize_t ima_show_htable_value(char __user *buf, size_t count,
> > @@ -261,6 +263,11 @@ static ssize_t ima_write_policy(struct file *file, 
> > const char __user *buf,
> >  {
> > char *data = NULL;
> > ssize_t result;
> > +   int res;
> > +
> > +   res = mutex_lock_interruptible(_write_mutex);
> > +   if (res)
> > +   return res;
> > 
> > if (datalen >= PAGE_SIZE)
> > datalen = PAGE_SIZE - 1;
> > @@ -286,6 +293,8 @@ out:
> > if (result < 0)
> > valid_policy = 0;
> > kfree(data);
> > +   mutex_unlock(_write_mutex);
> > +
> > return result;
> >  }
> > 
> > @@ -337,8 +346,12 @@ static int ima_release_policy(struct inode *inode, 
> > struct file *file)
> > return 0;
> > }
> > ima_update_policy();
> > +#ifndefCONFIG_IMA_WRITE_POLICY
> > securityfs_remove(ima_policy);
> > ima_policy = NULL;
> > +#else
> > +   clear_bit(IMA_FS_BUSY, _fs_flags);
> > +#endif
> > return 0;
> >  }
> > 

The IMA_FS_BUSY flag needs to be cleared, like here, above for !
valid_policy.

Mimi

--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[GIT PULL] Keys bugfixes

[James Morris]

Please pull these key susbystem fixes for 4.3, per the message from David 
Howells:

"Here are two patches, the first of which at least should go upstream
immediately:

 (1) Prevent a user-triggerable crash in the keyrings destructor when a
 negatively instantiated keyring is garbage collected.  I have also 
 seen this triggered for user type keys.

 (2) Prevent the user from using requesting that a keyring be created and
 instantiated through an upcall.  Doing so is probably safe since the 
 keyring type ignores the arguments to its instantiation function - 
 but we probably shouldn't let keyrings be created in this manner."

---

The following changes since commit 1099f86044111e9a7807f09523e42d4c9d0fb781:

  Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net (2015-10-19 
09:55:40 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git 
for-linus

David Howells (2):
  KEYS: Fix crash when attempt to garbage collect an uninstantiated keyring
  KEYS: Don't permit request_key() to construct a new keyring

 security/keys/gc.c  |6 --
 security/keys/request_key.c |3 +++
 2 files changed, 7 insertions(+), 2 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PULL REQUEST] IMA changes for 4.4

[James Morris]

On Mon, 19 Oct 2015, Mimi Zohar wrote:

> Hi James,
> 
> This pull request is for a single bug fix from Dimtry to properly load
> only signed certificates onto the trusted IMA keyring from the kernel.
> (This patch has been in the linux-next tree).
> 
> thanks,
> 
> Mimi
> 
> The following changes since commit
> 049e6dde7e57f0054fdc49102e7ef4830c698b46:
> 
>   Linux 4.3-rc4 (2015-10-04 16:57:17 +0100)
> 
> are available in the git repository at:
> 
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git
> next
> 

Pulled, thanks.

-- 
James Morris


--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5] Smack: limited capability for changing process label

[Rafal Krypa]

From: Zbigniew Jasinski 

This feature introduces new kernel interface:

- /relabel-self - for setting transition labels list

This list is used to control smack label transition mechanism.
List is set by, and per process. Process can transit to new label only if
label is on the list. Only process with CAP_MAC_ADMIN capability can add
labels to this list. With this list, process can change it's label without
CAP_MAC_ADMIN but only once. After label changing, list is unset.

Changes in v2:
* use list_for_each_entry instead of _rcu during label write
* added missing description in security/Smack.txt

Changes in v3:
* squashed into one commit

Changes in v4:
* switch from global list to per-task list
* since the per-task list is accessed only by the task itself
  there is no need to use synchronization mechanisms on it

Changes in v5:
* change smackfs interface of relabel-self to the one used for onlycap
  multiple labels are accepted, separated by space, which
  replace the previous list upon write

Signed-off-by: Zbigniew Jasinski 
Signed-off-by: Rafal Krypa 
---
 Documentation/security/Smack.txt |  10 ++
 security/smack/smack.h   |   4 +-
 security/smack/smack_access.c|   6 +-
 security/smack/smack_lsm.c   |  57 ++-
 security/smack/smackfs.c | 202 ---
 5 files changed, 238 insertions(+), 41 deletions(-)

diff --git a/Documentation/security/Smack.txt b/Documentation/security/Smack.txt
index 5e6d07f..945cc63 100644
--- a/Documentation/security/Smack.txt
+++ b/Documentation/security/Smack.txt
@@ -255,6 +255,16 @@ unconfined
the access permitted if it wouldn't be otherwise. Note that this
is dangerous and can ruin the proper labeling of your system.
It should never be used in production.
+relabel-self
+   This interface contains a list of labels to which the process can
+   transition to, by writing to /proc/self/attr/current.
+   Normally a process can change its own label to any legal value, but only
+   if it has CAP_MAC_ADMIN. This interface allows a process without
+   CAP_MAC_ADMIN to relabel itself to one of labels from predefined list.
+   A process without CAP_MAC_ADMIN can change its label only once. When it
+   does, this list will be cleared.
+   The values are set by writing the desired labels, separated
+   by spaces, to the file or cleared by writing "-" to the file.
 
 If you are using the smackload utility
 you can add access rules in /etc/smack/accesses. They take the form:
diff --git a/security/smack/smack.h b/security/smack/smack.h
index fff0c61..6c91156 100644
--- a/security/smack/smack.h
+++ b/security/smack/smack.h
@@ -115,6 +115,7 @@ struct task_smack {
struct smack_known  *smk_forked;/* label when forked */
struct list_headsmk_rules;  /* per task access rules */
struct mutexsmk_rules_lock; /* lock for the rules */
+   struct list_headsmk_relabel;/* transit allowed labels */
 };
 
 #defineSMK_INODE_INSTANT   0x01/* inode is instantiated */
@@ -169,7 +170,7 @@ struct smk_port_label {
 };
 #endif /* SMACK_IPV6_PORT_LABELING */
 
-struct smack_onlycap {
+struct smack_known_list_elem {
struct list_headlist;
struct smack_known  *smk_label;
 };
@@ -301,6 +302,7 @@ struct smack_known *smk_import_entry(const char *, int);
 void smk_insert_entry(struct smack_known *skp);
 struct smack_known *smk_find_entry(const char *);
 int smack_privileged(int cap);
+void smk_destroy_label_list(struct list_head *list);
 
 /*
  * Shared data.
diff --git a/security/smack/smack_access.c b/security/smack/smack_access.c
index bc1053f..a283f9e 100644
--- a/security/smack/smack_access.c
+++ b/security/smack/smack_access.c
@@ -637,7 +637,7 @@ DEFINE_MUTEX(smack_onlycap_lock);
 int smack_privileged(int cap)
 {
struct smack_known *skp = smk_of_current();
-   struct smack_onlycap *sop;
+   struct smack_known_list_elem *sklep;
 
/*
 * All kernel tasks are privileged
@@ -654,8 +654,8 @@ int smack_privileged(int cap)
return 1;
}
 
-   list_for_each_entry_rcu(sop, _onlycap_list, list) {
-   if (sop->smk_label == skp) {
+   list_for_each_entry_rcu(sklep, _onlycap_list, list) {
+   if (sklep->smk_label == skp) {
rcu_read_unlock();
return 1;
}
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 996c889..036d8b2 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -326,6 +326,7 @@ static struct task_smack *new_task_smack(struct smack_known 
*task,
tsp->smk_task = task;
tsp->smk_forked = forked;
INIT_LIST_HEAD(>smk_rules);
+   INIT_LIST_HEAD(>smk_relabel);
mutex_init(>smk_rules_lock);