From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7C31CC433EF for ; Wed, 3 Nov 2021 22:01:46 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 5EC6660F02 for ; Wed, 3 Nov 2021 22:01:46 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230314AbhKCWEW (ORCPT ); Wed, 3 Nov 2021 18:04:22 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48666 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230202AbhKCWEV (ORCPT ); Wed, 3 Nov 2021 18:04:21 -0400 Received: from mail-ed1-x534.google.com (mail-ed1-x534.google.com [IPv6:2a00:1450:4864:20::534]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 14434C061714 for ; Wed, 3 Nov 2021 15:01:44 -0700 (PDT) Received: by mail-ed1-x534.google.com with SMTP id ee33so14522039edb.8 for ; Wed, 03 Nov 2021 15:01:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=paul-moore-com.20210112.gappssmtp.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=TE+3cwTaC6K5rO3Q3MpOjAlohLPzRsp1yxXkdsv0VUw=; b=erZguU4z0nrKa/oX4nBkvy8iX3GwLs9eWARWEhH5PrLeePPnkGdhdW2+NzQEsXXGWn 4N1UlvYJ/dVTQ763BHT/DYUhuDVR7t/GAYnj8lIls7Wm1WIr/IjBTYfRil6NI3vvR2+R OomE41AGwGwHd0BfNv6GbiEvoCsa8SgIIp21IS3ZtiTPZGZ70PRh1n0/9Ewg9wQGwJJ9 x4dLjIGHC1MDIRycI3qhH2wxbkXW1a2QenNnB79539JwJqYHopkMk3K5PJY3AFUiTOnb om9Dg2qAwG9u1aB0AbDlcnLMWafFbePLZslvMb7ENI0QSBHZ/tRu4GdV9M8lsNnb3M88 y6wQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=TE+3cwTaC6K5rO3Q3MpOjAlohLPzRsp1yxXkdsv0VUw=; b=kDZ+b22Y0P8MaFRUZb7/AYALP4V6eUb5TvF3/ATZhORGJYYFX7h8S2myR4H4qGicOC RSf6GO9OxMV9KqDLwOBAdRIMvXWy1YUOUB/SRt3a0bI1LsG5KZmiCrD/zGyomL1xMpba eGAspnCjGSxnXmE81d3+CMK6LTgLKOnNwDrNzqBKXobjRdWSYB9L3Ds8sBTycB+FyybU URUKDgCaVogWu1m/4gfJJjVt9GFib/207qvisqHLl95vIuVDskt0nI+Hxu843UDkZm11 kblxtbsRVRdG7J9Sy57v6HWpwbyDzNtrpUUxhG0Qs7KhN/uiHdSgkVAZ4phSFuXH2gCu TCDw== X-Gm-Message-State: AOAM533eH9rxFT3SfoL3aJ6ozbbQF5c7DRo0thtwGpHRO8aoa+3j7pb5 tHXkgMwZxYMmnOht59N2sdy8ue4eoqXWVnhbdsz3 X-Google-Smtp-Source: ABdhPJzgb7DlErXb6KNErPXw1SygqoEVFkn2NpqV4M0ehMHW7InlRoMya1mGPXdJ5Wg/d5nXri0sKDLy1Jzb+oc+MqM= X-Received: by 2002:a17:907:2d12:: with SMTP id gs18mr38398502ejc.126.1635976902513; Wed, 03 Nov 2021 15:01:42 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Paul Moore Date: Wed, 3 Nov 2021 18:01:31 -0400 Message-ID: Subject: Re: [PATCHv2 net 4/4] security: implement sctp_assoc_established hook in selinux To: Xin Long Cc: Ondrej Mosnacek , network dev , SElinux list , Linux Security Module list , "linux-sctp @ vger . kernel . org" , "David S. Miller" , Jakub Kicinski , Marcelo Ricardo Leitner , James Morris , Richard Haines Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: selinux@vger.kernel.org On Wed, Nov 3, 2021 at 1:36 PM Xin Long wrote: > On Wed, Nov 3, 2021 at 1:33 PM Xin Long wrote: > > On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek wrote: > > > On Tue, Nov 2, 2021 at 1:03 PM Xin Long wrote: > > > > > > > > Different from selinux_inet_conn_established(), it also gives the > > > > secid to asoc->peer_secid in selinux_sctp_assoc_established(), > > > > as one UDP-type socket may have more than one asocs. > > > > > > > > Note that peer_secid in asoc will save the peer secid for this > > > > asoc connection, and peer_sid in sksec will just keep the peer > > > > secid for the latest connection. So the right use should be do > > > > peeloff for UDP-type socket if there will be multiple asocs in > > > > one socket, so that the peeloff socket has the right label for > > > > its asoc. > > > > > > > > v1->v2: > > > > - call selinux_inet_conn_established() to reduce some code > > > > duplication in selinux_sctp_assoc_established(), as Ondrej > > > > suggested. > > > > - when doing peeloff, it calls sock_create() where it actually > > > > gets secid for socket from socket_sockcreate_sid(). So reuse > > > > SECSID_WILD to ensure the peeloff socket keeps using that > > > > secid after calling selinux_sctp_sk_clone() for client side. > > > > > > Interesting... I find strange that SCTP creates the peeloff socket > > > using sock_create() rather than allocating it directly via > > > sock_alloc() like the other callers of sctp_copy_sock() (which calls > > > security_sctp_sk_clone()) do. Wouldn't it make more sense to avoid the > > > sock_create() call and just rely on the security_sctp_sk_clone() > > > semantic to set up the labels? Would anything break if > > > sctp_do_peeloff() switched to plain sock_alloc()? > > > > > > I'd rather we avoid this SECSID_WILD hack to support the weird > > > created-but-also-cloned socket hybrid and just make the peeloff socket > > > behave the same as an accept()-ed socket (i.e. no > > > security_socket_[post_]create() hook calls, just > > > security_sctp_sk_clone()). I believe the important part is that sctp_do_peeloff() eventually calls security_sctp_sk_clone() via way of sctp_copy_sock(). Assuming we have security_sctp_sk_clone() working properly I would expect that the new socket would be setup properly when sctp_do_peeloff() returns on success. ... and yes, that SECSID_WILD approach is *not* something we want to do. In my mind, selinux_sctp_sk_clone() should end up looking like this. void selinux_sctp_sk_clone(asoc, sk, newsk) { struct sk_security_struct sksec = sk->sk_security; struct sk_security_struct newsksec = newsk->sk_security; if (!selinux_policycap_extsockclass()) return selinux_sk_clone_security(sk, newsk); newsksec->sid = sksec->secid; newsksec->peer_sid = asoc->peer_secid; newsksec->sclass = sksec->sclass; selinux_netlbl_sctp_sk_clone(sk, newsk); } Also, to be clear, the "assoc->secid = SECSID_WILD;" line should be removed from selinux_sctp_assoc_established(). If we are treating SCTP associations similarly to TCP connections, the association's label/secid should be set once and not changed during the life of the association. > > > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") > > > > Reported-by: Prashanth Prahlad > > > > Reviewed-by: Richard Haines > > > > Tested-by: Richard Haines > > > > > > You made non-trivial changes since the last revision in this patch, so > > > you should have also dropped the Reviewed-by and Tested-by here. Now > > > David has merged the patches probably under the impression that they > > > have been reviewed/approved from the SELinux side, which isn't > > > completely true. > > > > Oh, that's a mistake, I thought I didn't add it. > > Will he be able to test this new patchset? While I tend to try to avoid reverts as much as possible, I think the right thing to do is to get these patches reverted out of DaveM's tree while we continue to sort this out and do all of the necessary testing and verification. Xin Long, please work with the netdev folks to get your patchset reverted and then respin this patchset using the feedback provided. -- paul moore www.paul-moore.com