Re: [PATCH] Adding helpers for unix_stream_connect.

* Re: [PATCH] Adding helpers for unix_stream_connect.
       [not found] <ZgD8lTToWEhtNmpg@tahera-OptiPlex-5000>
@ 2024-03-26 11:28 ` Mickaël Salaün
  2024-03-28  3:46   ` Tahera Fahimi
  0 siblings, 1 reply; 5+ messages in thread
From: Mickaël Salaün @ 2024-03-26 11:28 UTC (permalink / raw)
  To: TaheraFahimi; +Cc: Paul Moore, outreachy

CCing Outreachy mailing list and Paul.

Related GitHub issue: https://github.com/landlock-lsm/linux/issues/7

On Sun, Mar 24, 2024 at 10:24:53PM -0600, TaheraFahimi wrote:
> Signed-off-by: TaheraFahimi <fahimitahera@gmail.com>

I guess there is a space missing between your first and last name.

> ---
>  security/landlock/task.c | 89 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 89 insertions(+)
> 
> diff --git a/security/landlock/task.c b/security/landlock/task.c
> index 849f5123610b..10195492c169 100644
> --- a/security/landlock/task.c
> +++ b/security/landlock/task.c
> @@ -14,6 +14,10 @@
>  #include <linux/rcupdate.h>
>  #include <linux/sched.h>
>  
> +/*to use sock structure*/
> +#include <net/sock.h>
> +#include <linux/spinlock.h>
> +
>  #include "common.h"
>  #include "cred.h"
>  #include "ruleset.h"
> @@ -108,9 +112,94 @@ static int hook_ptrace_traceme(struct task_struct *const parent)
>  	return task_ptrace(parent, current);
>  }
>  
> +
> +/** 
> + * sk_get_peer_cred : Helper Function from socket.c
> + * This helper is used to retrieve the sk_peer credentioals.
> + */
> +static const struct cred *sk_get_peer_cred(struct sock *sk)
> +{
> +        const struct cred *cred;
> +        spin_lock(&sk->sk_peer_lock);
> +        cred = get_cred(sk->sk_peer_cred);
> +        spin_unlock(&sk->sk_peer_lock);
> +        return cred;
> +}
> +/**
> + * Use domain_scope_le() to scope the access same as ptrace trask.
> + */
> +static bool unix_sock_is_scoped(struct sock *const sock,
> +                                struct sock *const other)
> +{
> +
> +        bool is_scoped = true;
> +        /* retrieve the credential of socks.
> +	 * other->sk_peer_cred shows the listening socket credentials (This credential
> +	 *  is created at the init_cred function and will be coped to 
> +	 *  sock->sk_peer_cred later.) 
> +	 * Since sock is the connecting sock, then the credential of current task is equal
> +	 * to cred_sock.
> +	 *  */
> +        const struct cred *cred_other;
> +	cred_other = sk_get_peer_cred(other);
> +	
> +	
> +	const struct cred *cred_sock;
> +	spin_lock(&sock->sk_peer_lock);

You don't need a lock for get_current_cred(), and sk_peer_lock is
unrelated to current.  Moreover, security_unix_stream_connect() is
already called while the lock being held.

> +	cred_sock = get_current_cred();
> +	spin_unlock(&sock->sk_peer_lock);

You should just use landlock_get_current_domain().

> +
> +	printk(KERN_DEBUG "cred uid: %u\n", cred_other->gid);

You can use pr_warn() instead.

> +	//printk("the cred_sock kuid is : %llx\n", cred_sock->uid);
> +        /* retrieve the landlock_rulesets*/
> +	
> +	printk("The credentials is retrieved!!\n");
> +        
> +	const struct landlock_cred_security *dom_parent, *dom_child;

dom_* variables should only be used to refer do a Landlock domain, not
credential.

> +        const struct landlock_ruleset *parent, *child;
> +        rcu_read_lock();
> +        dom_parent = landlock_cred(cred_other);
> +	dom_child = landlock_cred(cred_sock);
> +        parent = dom_parent->domain;
> +        child = dom_child->domain;
> +	/* by uncommenting either the following if statement or calling 
> +	 * the domain_scope_le the kernel will not load(make task dead)*/
> +	//if (dom_parent)
> +	//	printk("parent num rules %u\n", parent->num_rules);
> +	if (child)
> +		printk("child num rules %u\n", child->num_rules);
> +	//is_scoped = domain_scope_le(parent, child);
> +        rcu_read_unlock();
> +	
> +
> +        return is_scoped;
> +}
> +/**
> + * unix_stream_connect -
> + */
> +static int task_unix_stream_connect(struct sock *const sock,
> +                               struct sock *const other,
> +                               struct sock *const newsk)
> +{
> +        /* How do we check if the scoping is optional?  */
> +        if (unix_sock_is_scoped(sock, other))
> +                return 0;
> +        return -EPERM;
> +}
> +
> +/**
> + * hook_unix_stream_connect -
> + */
> +static int hook_unix_stream_connect(struct sock *const sock, struct sock *const other,
> +                                    struct sock *const newsk)
> +{
> +        return task_unix_stream_connect(sock, other, newsk);
> +}
> +
>  static struct security_hook_list landlock_hooks[] __ro_after_init = {
>  	LSM_HOOK_INIT(ptrace_access_check, hook_ptrace_access_check),
>  	LSM_HOOK_INIT(ptrace_traceme, hook_ptrace_traceme),
> +        LSM_HOOK_INIT(unix_stream_connect, hook_unix_stream_connect),
>  };
>  
>  __init void landlock_add_task_hooks(void)
> -- 
> 2.34.1
> 
> Hello Mickaël, I made the following changes to have an lsm hook function for "unix_stream_connect". I have an issue, and I was wondering  if you could give me a hint. At first, I retrieve the Socks' credentials through "sk_get_peer_cred" and "get_current_cred". As "get_current_cred" gives us the credential related to the sock and "sk_get_peer_cred" gives the sock's peer credential(this credential will be copied to the field sock->sk_peer_cred later). Then I uses both credentials to get their "landlock_cred_security". Since "landlock_cred_security" has a pointer to the "landlock_ruleset", we can pass their rulesets to the "domain_scope_le" (same as ptrace task) to check if it is scoped or not.

Hello Tahera, this approach looks good.  However, Landlock credential's
domain may be NULL (when the task is not sandboxed).

> The problem is that If I uncomment the "domain_scope_le", the kernel will not work. I think it can not get the "landlock_cred_security". Thank you :)

"the kernel will not work" could means a lot of things. We need a more
precise description.

I guess this is just a draft and the patch is not ready yet so it's
fine. Before sending a first official version you should follow and read
the referenced links and especially the kernel patch how-to here:
https://lore.kernel.org/outreachy/ZeZzLWtk2nZ1Pcgw@aschofie-mobl2/
For instance, you should run checkpatch.pl
For Landlock-specific patches, they need to be formatted with
clang-format (-i) and pass all the checked done by check-linux.sh
provided here: https://github.com/landlock-lsm/landlock-test-tools

^ permalink raw reply	[flat|nested] 5+ messages in thread