From patchwork Thu May 10 16:05:29 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paul Moore X-Patchwork-Id: 10392043 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id A18EE6053D for ; Thu, 10 May 2018 16:05:35 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 8FCE52236A for ; Thu, 10 May 2018 16:05:35 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 8176D28501; Thu, 10 May 2018 16:05:35 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 650DA2236A for ; Thu, 10 May 2018 16:05:34 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966321AbeEJQFd (ORCPT ); Thu, 10 May 2018 12:05:33 -0400 Received: from mail-lf0-f52.google.com ([209.85.215.52]:45409 "EHLO mail-lf0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965033AbeEJQFc (ORCPT ); Thu, 10 May 2018 12:05:32 -0400 Received: by mail-lf0-f52.google.com with SMTP id q2-v6so3699392lfc.12 for ; Thu, 10 May 2018 09:05:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=paul-moore-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=V6PlBQslvi/q906Xbu9nieRGN7JziGU3EbXNmzTouog=; b=zzfxQsJMoDiFt66KgrvqvsvBlFYt/209MH/flmbizatC5AHjSnGBjzh7FwZAhGlxGn o7uRedECCdZ6infoKEkzY+4SOwfPwqoT5cHZcSNkmLlWAi6+wAJ9yKZDjHQZZZDC7Umt KiGLVXbX7zJhgAJg2Qq+75G/WQy6B5YVXNrKlGeznXCk8JcbvTDN9d2LzGeeZzjPosEx D87+9eqyLeliNvjnK5+SXiS5CP20L8L8ER1by8vwYtedm1KrQDNOmE4HTLO3/dX1DvEr UCO/BY2RL1r+uJwz59726EMCd2P38QhlJjfziiOZoeOqp/01tj4UQIZT/XiTAsRj4//Q gARQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=V6PlBQslvi/q906Xbu9nieRGN7JziGU3EbXNmzTouog=; b=IKz0w3tZK6hGu0+4h0ZbbPPKnp36BI76XBiezjBQKquCzwVmX0k8FprgyXOwrVH5xI eYyU+qMVg1aEbpjhFFD/S8usoaoUAJUpuijZWQ5ClAlbcFhn79UXYQZ3tOjV+5vFY+vV y3jczEWcSil7PVxLjATGgrbHCcKpkE1CI3AtnA7pUznNkrffVNJcSc+NmOK1vM09ucf2 K4str77oR3gSGnX+xsR9htMmVFyXw3n8wGTlvp0I5zZE3h9OLAg8Ms3vRrpLHpCWuoRs aSJ1NV5dJWb5U68sxMeJQuvzgFu1Q1m8LMn7bNnq7DCtoIPLlwqBgqVG7+MVf8f6rtCe C1zw== X-Gm-Message-State: ALKqPwcl7ASnc6Vs6F5vEqWGTRKoikD+6Exodfk+JJ00r61RSX4kHR+P Bhm9HmMMdcoDNDk75Lcw9seKvYNVzfbf3ewB6gZj X-Google-Smtp-Source: AB8JxZpbWfl+Z+mNEvEJFhf9AguEYaJET69QD711NhpvMOAI9mnFfD0krb+0dyaEmtUBAJaS6yvv5T2ZcC1MBtaXSBA= X-Received: by 2002:a2e:8858:: with SMTP id z24-v6mr1661652ljj.106.1525968330677; Thu, 10 May 2018 09:05:30 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a19:a947:0:0:0:0:0 with HTTP; Thu, 10 May 2018 09:05:29 -0700 (PDT) X-Originating-IP: [68.177.129.184] In-Reply-To: References: <1525788303-23244-1-git-send-email-alexey.kodanev@oracle.com> <1eb10913-8802-e2dd-68f0-9483435cd949@tycho.nsa.gov> <7fdbaf13-fea2-4a2c-213d-fa291db67081@tycho.nsa.gov> From: Paul Moore Date: Thu, 10 May 2018 12:05:29 -0400 Message-ID: Subject: Re: [PATCH] selinux: add AF_UNSPEC and INADDR_ANY checks to selinux_socket_bind() To: Alexey Kodanev Cc: Stephen Smalley , Richard Haines , selinux@tycho.nsa.gov, Eric Paris , linux-security-module@vger.kernel.org, netdev Sender: owner-linux-security-module@vger.kernel.org Precedence: bulk List-ID: X-Virus-Scanned: ClamAV using ClamSMTP On Thu, May 10, 2018 at 5:28 AM, Alexey Kodanev wrote: > On 10.05.2018 01:02, Paul Moore wrote: > ... >> I just had a better look at this and I believe that Alexey and Stephen >> are right: this is the best option. My apologies for the noise >> earlier. However, while looking at the code I think there are some >> additional necessary changes: >> >> * In the case of an SCTP socket, we should return -EINVAL, just as we >> do with other address families. > > Right. > >> * While not strictly related to AF_UNSPEC, we really should be passing >> the address family of the sockaddr, and not the socket, to functions >> that need to interpret the bind address/port. > > That looks like a correct solution. I guess we need the same fix for > sctp_connectx(), in selinux_socket_connect_helper(). Yes. I think we also need a small change to selinux_sctp_bind_connect() to both not error out on AF_UNSPEC, and to return EINVAL on a bad address family (some of the callers pass on the return value, some don't). >> I'm waiting for my kernel to compile so I haven't given this any >> sanity testing, but the patch below is what I think we need ... >> >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >> index 4cafe6a19167..5f30045b2053 100644 >> --- a/security/selinux/hooks.c >> +++ b/security/selinux/hooks.c >> @@ -4576,6 +4576,7 @@ static int selinux_socket_post_create(struct socket *sock, >> int family, >> static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, i >> nt addrlen) >> { >> struct sock *sk = sock->sk; >> + struct sk_security_struct *sksec = sk->sk_security; >> u16 family; >> int err; >> >> @@ -4587,13 +4588,13 @@ static int selinux_socket_bind(struct socket *sock, stru >> ct sockaddr *address, in >> family = sk->sk_family; >> if (family == PF_INET || family == PF_INET6) { >> char *addrp; >> - struct sk_security_struct *sksec = sk->sk_security; >> struct common_audit_data ad; >> struct lsm_network_audit net = {0,}; >> struct sockaddr_in *addr4 = NULL; >> struct sockaddr_in6 *addr6 = NULL; >> unsigned short snum; >> u32 sid, node_perm; >> + u16 family_sa = address->sa_family; >> >> /* >> * sctp_bindx(3) calls via selinux_sctp_bind_connect() >> @@ -4601,11 +4602,19 @@ static int selinux_socket_bind(struct socket *sock, stru >> ct sockaddr *address, in >> * need to check address->sa_family as it is possible to have >> * sk->sk_family = PF_INET6 with addr->sa_family = AF_INET. >> */ >> - switch (address->sa_family) { >> + switch (family_sa) { >> + case AF_UNSPEC: >> case AF_INET: >> if (addrlen < sizeof(struct sockaddr_in)) >> return -EINVAL; >> addr4 = (struct sockaddr_in *)address; >> + if (family_sa == AF_UNSPEC) { >> + /* see "__inet_bind()", we only want to allow >> + * AF_UNSPEC if the address is INADDR_ANY */ >> + if (addr4->sin_addr.s_addr != htonl(INADDR_ANY)) >> + goto err_af; >> + family_sa = AF_INET; >> + } >> snum = ntohs(addr4->sin_port); >> addrp = (char *)&addr4->sin_addr.s_addr; >> break; >> @@ -4617,15 +4626,14 @@ static int selinux_socket_bind(struct socket *sock, stru >> ct sockaddr *address, in >> addrp = (char *)&addr6->sin6_addr.s6_addr; >> break; >> default: >> - /* Note that SCTP services expect -EINVAL, whereas >> - * others expect -EAFNOSUPPORT. >> - */ >> - if (sksec->sclass == SECCLASS_SCTP_SOCKET) >> - return -EINVAL; >> - else >> - return -EAFNOSUPPORT; >> + goto err_af; >> } >> >> + ad.type = LSM_AUDIT_DATA_NET; >> + ad.u.net = &net; >> + ad.u.net->sport = htons(snum); >> + ad.u.net->family = family_sa; >> + > > May be we could move setting ad.u.net->v{4|6}info.saddr here as well? I looked at that too, the problem is that if we set the IP address here it will be reported in the audit record for a name_bind failure, which is a change from the current behavior. One could argue this is the correct thing to do, but I would like to limit the number of changes for patches that are destined for the -rcX stream. Let's leave them separate for now. > Will send a v2 of this patch so that SCTP socket returns EINVAL with > AF_UNSPEC. Should I prepare a patch with correcting 'ad.u.net->family' > and sel_netnode_sid()? Please, that would be helpful. I think all of the issues we have identified in this thread should be fixed during the v4.17-rcX releases, so if you don't do it I'll need to do it :) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 5f30045b2053..a8bac9b37ee7 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -5277,6 +5277,7 @@ static int selinux_sctp_bind_connect(struct sock *sk, int> while (walk_size < addrlen) { addr = addr_buf; switch (addr->sa_family) { + case AF_UNSPEC: case AF_INET: len = sizeof(struct sockaddr_in); break; @@ -5284,7 +5285,7 @@ static int selinux_sctp_bind_connect(struct sock *sk, int> len = sizeof(struct sockaddr_in6); break; default: - return -EAFNOSUPPORT; + return -EINVAL; } err = -EINVAL;