From patchwork Tue May 8 17:05:48 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paul Moore X-Patchwork-Id: 10386577 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 CC77460353 for ; Tue, 8 May 2018 17:05:55 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id B72CA26E69 for ; Tue, 8 May 2018 17:05:55 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id AA314283CA; Tue, 8 May 2018 17:05:55 +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 B184C26E69 for ; Tue, 8 May 2018 17:05:53 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755014AbeEHRFw (ORCPT ); Tue, 8 May 2018 13:05:52 -0400 Received: from mail-lf0-f54.google.com ([209.85.215.54]:33700 "EHLO mail-lf0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754223AbeEHRFv (ORCPT ); Tue, 8 May 2018 13:05:51 -0400 Received: by mail-lf0-f54.google.com with SMTP id m18-v6so46941847lfb.0 for ; Tue, 08 May 2018 10:05:50 -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=5HxAXu4LfWZY5NXJAgfgeVUAaCYmgWwbscYUq6a2Y3g=; b=MwAa6rYIV5mpHiB+UqjRvftnqtKYhD3VdXj9OjaKaPbzAUqGbieFRT3jpT3FHbDV2s z2EV1TyAcFbRQhHknrh99UQncaJDuqvCSSYh3h5bRLmmJ2/mKNIb99+j+OggKbtqgKmV HGYe2jKJyHjR0LzLUpxTcNXe5S6JuNzhJ7kjz0eldsrvH5CE/sTkx8flksvic8MVZ5SJ yyp5FDMgciTpuijdHZfRdvxdU8fhoOsDI4//LNg3h0jdulTLusX9NUCXWGymg9EEt+Cu TxuMrFsQFpQPUv51VQGMDbkZ3kYSV1zcp+5XU++I/+suS/W6KeCKl8D4eSDJVbdHw7+S NOBw== 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=5HxAXu4LfWZY5NXJAgfgeVUAaCYmgWwbscYUq6a2Y3g=; b=Qh+ozrLAEFTNcOLO1vyy1ZKu+CSO1rmvBDDJKErU7BshVMqTNuey7EiVM5SU/qA89J qBm+M9R3pJKS4ZaLZouxN5YsjVXNT+mCCo43zy12/tPBYzbBGSI7WWs4YN8VRkoDeexg 3fRqTYYXq9wKSSY5B1oRksT1A2HihVeFfeazehQuDAosp5qc+4Jap0CAYI6UQcx/rTAB XodYEt+7UClZGis5j8X910uGRuaLbaOKECeCGLFrDEYmZATtzdfmUXTjgVasfyV6iAzf K4dG6fp1vyfRvtAzHswuVkQwg6UH9M9Nk3dCEk4eBxD4RwwB7d90KWmWraU9wZaNrLZB JWUg== X-Gm-Message-State: ALQs6tCCBHNHIlEfpzJDmDrslw8KnSkXGxOFPjR0nwfke+AK+te7sI7C AsrFjFIMNNBmp/xsdj1WPXWXEG1c9xMopLUT44Mp X-Google-Smtp-Source: AB8JxZqmfitUCIJIgcFqjASsvctB4xWZBBY6t2EKEiwbUzSqlnp19d5N1LTQVqbMYZkBVVIEUElzgWyR23lpKHpMpVE= X-Received: by 2002:a19:4ed1:: with SMTP id u78-v6mr10885049lfk.40.1525799149645; Tue, 08 May 2018 10:05:49 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a19:a947:0:0:0:0:0 with HTTP; Tue, 8 May 2018 10:05:48 -0700 (PDT) X-Originating-IP: [68.177.129.184] In-Reply-To: <1525788303-23244-1-git-send-email-alexey.kodanev@oracle.com> References: <1525788303-23244-1-git-send-email-alexey.kodanev@oracle.com> From: Paul Moore Date: Tue, 8 May 2018 13:05:48 -0400 Message-ID: Subject: Re: [PATCH] selinux: add AF_UNSPEC and INADDR_ANY checks to selinux_socket_bind() To: Alexey Kodanev , Richard Haines Cc: selinux@tycho.nsa.gov, Stephen Smalley , 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 Tue, May 8, 2018 at 10:05 AM, Alexey Kodanev wrote: > Commit d452930fd3b9 ("selinux: Add SCTP support") breaks compatibility > with the old programs that can pass sockaddr_in with AF_UNSPEC and > INADDR_ANY to bind(). As a result, bind() returns EAFNOSUPPORT error. > It was found with LTP/asapi_01 test. > > Similar to commit 29c486df6a20 ("net: ipv4: relax AF_INET check in > bind()"), which relaxed AF_INET check for compatibility, add AF_UNSPEC > case to AF_INET and make sure that the address is INADDR_ANY. > > Also, in the end of selinux_socket_bind(), instead of adding AF_UNSPEC > to 'address->sa_family == AF_INET', verify AF_INET6 first. > > Fixes: d452930fd3b9 ("selinux: Add SCTP support") > Signed-off-by: Alexey Kodanev > --- > security/selinux/hooks.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) Thanks for finding and reporting this regression. I think I would prefer to avoid having to duplicate the AF_UNSPEC/INADDR_ANY checking logic in the SELinux hook, even though it is a small bit of code and unlikely to change. I'm wondering if it would be better to check both the socket and sockaddr address family in the main if conditional inside selinux_socket_bind(), what do you think? Another option would be to go back to just checking the soackaddr address family; we moved away from that for a reason which escapes at the moment (code cleanliness?), but perhaps that was a mistake. > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 4cafe6a..649a3be 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -4602,10 +4602,16 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in > * sk->sk_family = PF_INET6 with addr->sa_family = AF_INET. > */ > switch (address->sa_family) { > + case AF_UNSPEC: > case AF_INET: > if (addrlen < sizeof(struct sockaddr_in)) > return -EINVAL; > addr4 = (struct sockaddr_in *)address; > + > + if (address->sa_family == AF_UNSPEC && > + addr4->sin_addr.s_addr != htonl(INADDR_ANY)) > + return -EAFNOSUPPORT; > + > snum = ntohs(addr4->sin_port); > addrp = (char *)&addr4->sin_addr.s_addr; > break; > @@ -4681,10 +4687,10 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in > ad.u.net->sport = htons(snum); > ad.u.net->family = family; > > - if (address->sa_family == AF_INET) > - ad.u.net->v4info.saddr = addr4->sin_addr.s_addr; > - else > + if (address->sa_family == AF_INET6) > ad.u.net->v6info.saddr = addr6->sin6_addr; > + else > + ad.u.net->v4info.saddr = addr4->sin_addr.s_addr; > > err = avc_has_perm(&selinux_state, > sksec->sid, sid, > -- > 1.8.3.1 > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 4cafe6a19167..a3789b167667 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -4577,6 +4577,7 @@ static int selinux_socket_bind(struct socket *sock, struc> { struct sock *sk = sock->sk; u16 family; + u16 family_sa; int err; err = sock_has_perm(sk, SOCKET__BIND); @@ -4585,7 +4586,9 @@ static int selinux_socket_bind(struct socket *sock, struc> /* If PF_INET or PF_INET6, check name_bind permission for the port. */ family = sk->sk_family; - if (family == PF_INET || family == PF_INET6) { + family_sa = address->sa_family; + if ((family == PF_INET || family == PF_INET6) && + (family_sa == PF_INET || family_sa == PF_INET6)) { char *addrp; struct sk_security_struct *sksec = sk->sk_security; struct common_audit_data ad; @@ -4601,7 +4604,7 @@ static int selinux_socket_bind(struct socket *sock, struc> * 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_INET: if (addrlen < sizeof(struct sockaddr_in)) return -EINVAL;