netdev - Re: [PATCH v2 bpf-next 3/9] bpf: Hooks for sys

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 30 Mar 2018 01:06:55 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Alexei Starovoitov <ast@...nel.org>, davem@...emloft.net
Cc:     netdev@...r.kernel.org, kernel-team@...com
Subject: Re: [PATCH v2 bpf-next 3/9] bpf: Hooks for sys_bind

On 03/28/2018 05:41 AM, Alexei Starovoitov wrote:
[...]
> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index e8c7fad8c329..2dec266507dc 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -450,6 +450,13 @@ int inet_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
>  	if (addr_len < sizeof(struct sockaddr_in))
>  		goto out;
>  
> +	/* BPF prog is run before any checks are done so that if the prog
> +	 * changes context in a wrong way it will be caught.
> +	 */
> +	err = BPF_CGROUP_RUN_PROG_INET4_BIND(sk, uaddr);
> +	if (err)
> +		goto out;
> +

Should the hook not come at the very beginning?

        /* If the socket has its own bind function then use it. (RAW) */
        if (sk->sk_prot->bind) {
                err = sk->sk_prot->bind(sk, uaddr, addr_len);
                goto out;
        }
        err = -EINVAL;
        if (addr_len < sizeof(struct sockaddr_in))
                goto out;

        /* BPF prog is run before any checks are done so that if the prog
         * changes context in a wrong way it will be caught.
         */
        err = BPF_CGROUP_RUN_PROG_INET4_BIND(sk, uaddr);
        if (err)
                goto out;

E.g. when you have v4/v6 ping or raw sockets used from language runtimes
or apps, then they provide their own bind handler here in kernel, thus any
bind rewrite won't be caught for them. Shouldn't this be covered as well
and the BPF_CGROUP_RUN_PROG_INET4_BIND() come first?

>  	if (addr->sin_family != AF_INET) {
>  		/* Compatibility games : accept AF_UNSPEC (mapped to AF_INET)
>  		 * only if s_addr is INADDR_ANY.
> diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
> index dbbe04018813..fa24e3f06ac6 100644
> --- a/net/ipv6/af_inet6.c
> +++ b/net/ipv6/af_inet6.c
> @@ -295,6 +295,13 @@ int inet6_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
>  	if (addr_len < SIN6_LEN_RFC2133)
>  		return -EINVAL;
>  
> +	/* BPF prog is run before any checks are done so that if the prog
> +	 * changes context in a wrong way it will be caught.
> +	 */
> +	err = BPF_CGROUP_RUN_PROG_INET6_BIND(sk, uaddr);
> +	if (err)
> +		return err;
> +
>  	if (addr->sin6_family != AF_INET6)
>  		return -EAFNOSUPPORT;

(Same here?)