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 X-Spam-Level: X-Spam-Status: No, score=-6.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5381DC2B9F7 for ; Fri, 28 May 2021 13:42:38 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 31314613E5 for ; Fri, 28 May 2021 13:42:38 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235999AbhE1NoL (ORCPT ); Fri, 28 May 2021 09:44:11 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:50768 "EHLO us-smtp-delivery-124.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233884AbhE1NoA (ORCPT ); Fri, 28 May 2021 09:44:00 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1622209345; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=U2IqSZIHmJs1U626ov1801TPHqiAMQZgW3aXqcOf9ok=; b=WqMQew3kBl6n/WouXs1ojIx7TVgLWfEtvGO1Mv6g9S84R+6xfqFJQ/sEgyF3qa8OMm/0BL lMtDzi4etBvfcmDaNKVv+FjzbjtZwq7kY9W/KT6OtXjLfD6eslFUSXZusK08Yi+HNkOQwg 0zq63u5e7OVJjHuNyvQRGfqGZ+0Ft/M= Received: from mail-yb1-f199.google.com (mail-yb1-f199.google.com [209.85.219.199]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-360-Wr4MwoejMwiSw_RbrV14lw-1; Fri, 28 May 2021 09:42:22 -0400 X-MC-Unique: Wr4MwoejMwiSw_RbrV14lw-1 Received: by mail-yb1-f199.google.com with SMTP id x187-20020a25e0c40000b029052a5f0bf9acso4513925ybg.1 for ; Fri, 28 May 2021 06:42:22 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=U2IqSZIHmJs1U626ov1801TPHqiAMQZgW3aXqcOf9ok=; b=YTKObJxuRWHC4YXdj9dhBrB0jwQOftmUw/eCB3JpTBt1QJ2Upt2OmoQo1fytCMC+wU CnGxdqwl8fFkKrrtAEjnpG1GqYVTdIrtcXM9fW9TdOTAY8QOqEi+PQ0ZVl1pVPtJ810j JePutn+hEiwfwCBgTTVRQFFdapiM72HSX1EeLgZpQ6YQ0b6q+Td6fGcXoX5LoFH9QL73 XobYE9OsnclL4qT3kpcpWYHaYoFxKMSxjooiaj0lfH7ZnYTXpLCbX1/SyDZtkgjB4Ba1 22xk6sWkeiE6HsRU+6hh4amKXxHFfeSvPqvKMFBURjv0+C08dcYQqH0Rqjoasx9agvhM i3kg== X-Gm-Message-State: AOAM5331hxpeMgIQzzDoDpwjyU5rcNnme5azlD5oVeZL3LsEXBW5DFSa B+NQF2bQwsOq9ObQUb3BJa5opdnJFSl4zsgIaRF8Ym3RGysa+DgEdAxwdgwPU1WAL2AetJPljIv +3mIVJ6TGkmsZ0dKfSRGPrrRG8O9bzH6FAERxRrXB X-Received: by 2002:a25:f208:: with SMTP id i8mr12505982ybe.340.1622209341653; Fri, 28 May 2021 06:42:21 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzY+Q3lAZD5z2I0qCaLzE8vPZbw30C0qe9jw96SwP96ICqH8giHKwWORUSXlm+cwx1tCc/b+Qf60iJexuKP47M= X-Received: by 2002:a25:f208:: with SMTP id i8mr12505951ybe.340.1622209341353; Fri, 28 May 2021 06:42:21 -0700 (PDT) MIME-Version: 1.0 References: <20210517092006.803332-1-omosnace@redhat.com> <01135120-8bf7-df2e-cff0-1d73f1f841c3@iogearbox.net> <4fee8c12-194f-3f85-e28b-f7f24ab03c91@iogearbox.net> In-Reply-To: <4fee8c12-194f-3f85-e28b-f7f24ab03c91@iogearbox.net> From: Ondrej Mosnacek Date: Fri, 28 May 2021 15:42:06 +0200 Message-ID: Subject: Re: [PATCH v2] lockdown,selinux: avoid bogus SELinux lockdown permission checks To: Daniel Borkmann Cc: Paul Moore , Linux Security Module list , James Morris , Steven Rostedt , Ingo Molnar , Stephen Smalley , SElinux list , linuxppc-dev@lists.ozlabs.org, Linux FS Devel , bpf , network dev , Linux kernel mailing list , Casey Schaufler , Jiri Olsa , andrii.nakryiko@gmail.com Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org (I'm off work today and plan to reply also to Paul's comments next week, but for now let me at least share a couple quick thoughts on Daniel's patch.) On Fri, May 28, 2021 at 11:56 AM Daniel Borkmann wrote: > > On 5/28/21 9:09 AM, Daniel Borkmann wrote: > > On 5/28/21 3:37 AM, Paul Moore wrote: > >> On Mon, May 17, 2021 at 5:22 AM Ondrej Mosnacek wrote: > >>> > >>> Commit 59438b46471a ("security,lockdown,selinux: implement SELinux > >>> lockdown") added an implementation of the locked_down LSM hook to > >>> SELinux, with the aim to restrict which domains are allowed to perform > >>> operations that would breach lockdown. > >>> > >>> However, in several places the security_locked_down() hook is called in > >>> situations where the current task isn't doing any action that would > >>> directly breach lockdown, leading to SELinux checks that are basically > >>> bogus. > >>> > >>> Since in most of these situations converting the callers such that > >>> security_locked_down() is called in a context where the current task > >>> would be meaningful for SELinux is impossible or very non-trivial (and > >>> could lead to TOCTOU issues for the classic Lockdown LSM > >>> implementation), fix this by modifying the hook to accept a struct cred > >>> pointer as argument, where NULL will be interpreted as a request for a > >>> "global", task-independent lockdown decision only. Then modify SELinux > >>> to ignore calls with cred == NULL. > >> > >> I'm not overly excited about skipping the access check when cred is > >> NULL. Based on the description and the little bit that I've dug into > >> thus far it looks like using SECINITSID_KERNEL as the subject would be > >> much more appropriate. *Something* (the kernel in most of the > >> relevant cases it looks like) is requesting that a potentially > >> sensitive disclosure be made, and ignoring it seems like the wrong > >> thing to do. Leaving the access control intact also provides a nice > >> avenue to audit these requests should users want to do that. > > > > I think the rationale/workaround for ignoring calls with cred == NULL (or the previous > > patch with the unimplemented hook) from Ondrej was two-fold, at least speaking for his > > seen tracing cases: > > > > i) The audit events that are triggered due to calls to security_locked_down() > > can OOM kill a machine, see below details [0]. > > > > ii) It seems to be causing a deadlock via slow_avc_audit() -> audit_log_end() > > when presumingly trying to wake up kauditd [1]. Actually, I wasn't aware of the deadlock... But calling an LSM hook [that is backed by a SELinux access check] from within a BPF helper is calling for all kinds of trouble, so I'm not surprised :) > Ondrej / Paul / Jiri: at least for the BPF tracing case specifically (I haven't looked > at the rest but it's also kind of independent), the attached fix should address both > reported issues, please take a look & test. Thanks, I like this solution, although there are a few gotchas: 1. This patch creates a slight "regression" in that if someone flips the Lockdown LSM into lockdown mode on runtime, existing (already loaded) BPF programs will still be able to call the confidentiality-breaching helpers, while before the lockdown would apply also to them. Personally, I don't think it's a big deal (and I bet there are other existing cases where some handle kept from before lockdown could leak data), but I wanted to mention it in case someone thinks the opposite. 2. IIUC. when a BPF program is rejected due to lockdown/SELinux, the kernel will return -EINVAL to userspace (looking at check_helper_call() in kernel/bpf/verifier.c; didn't have time to look at other callers...). It would be nicer if the error code from the security_locked_down() call would be passed through the call chain and eventually returned to the caller. It should be relatively straightforward to convert bpf_base_func_proto() to return a PTR_ERR() instead of NULL on error, but it looks like this would result in quite a big patch updating all the callers (and callers of callers, etc.) with a not-so-small chance of missing some NULL check and introducing a bug... I guess we could live with EINVAL-on-denied in stable kernels and only have the error path refactoring in -next; I'm not sure... 3. This is a bit of a shot-in-the-dark, but I suppose there might be some BPF programs that would be able to do something useful also when the read_kernel helpers return an error, yet the kernel will now outright refuse to load them (when the lockdown hook returns nonzero). I have no idea if such BPF programs realistically exist in practice, but perhaps it would be worth returning some dummy always-error-returning helper function instead of NULL from bpf_base_func_proto() when security_locked_down() returns an error. That would also resolve (2.), basically. (Then there is the question of what error code to use (because Lockdown LSM uses -EPERM, while SELinux -EACCESS), but I think always returning -EPERM from such stub helpers would be a viable choice.) -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc. 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 X-Spam-Level: X-Spam-Status: No, score=-3.5 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 826C2C2B9F7 for ; Fri, 28 May 2021 13:43:01 +0000 (UTC) Received: from lists.ozlabs.org (lists.ozlabs.org [112.213.38.117]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id D528761284 for ; Fri, 28 May 2021 13:43:00 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org D528761284 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4Fs5VC35qQz3bnb for ; Fri, 28 May 2021 23:42:59 +1000 (AEST) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=aZjWNNFp; dkim=fail reason="signature verification failed" (1024-bit key) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=KKg0wS9n; dkim-atps=neutral Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=redhat.com (client-ip=170.10.133.124; helo=us-smtp-delivery-124.mimecast.com; envelope-from=omosnace@redhat.com; receiver=) Authentication-Results: lists.ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=aZjWNNFp; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=KKg0wS9n; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4Fs5Tc6QGKz2yRQ for ; Fri, 28 May 2021 23:42:26 +1000 (AEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1622209343; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=U2IqSZIHmJs1U626ov1801TPHqiAMQZgW3aXqcOf9ok=; b=aZjWNNFpyQHmN4fJRFe2e1L+qm+2GVOLTllFx5KP0SBVVo9oBsx5uX+ndH5Q9Svr313pvP Gsm9NeqK5jvoDMhBiHJePpVoXMV+mN/NpApf1f9trz6pzFYzh437ZPufESchFrRSSL+65c aFaDfcLOQKs2XWnSzXhtjyfpH8JLdbE= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1622209344; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=U2IqSZIHmJs1U626ov1801TPHqiAMQZgW3aXqcOf9ok=; b=KKg0wS9ntg4tZQJLLQC2IDwVlN81IrJpnEAisHXKbD1RadfNjDJWE/HDRpRQkr7gEkseI8 geBUkTRozdNzJgALeHMx59yG88Y9jipMmm+wAp6ANlVBviPL0SyPWzqoBFlE8A77ShjNsw NP5oRxnoOoaP9ZquF5X0ebWk8DDZraU= Received: from mail-yb1-f199.google.com (mail-yb1-f199.google.com [209.85.219.199]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-360-m6g0lzFMO_iCRymQvASQFw-1; Fri, 28 May 2021 09:42:22 -0400 X-MC-Unique: m6g0lzFMO_iCRymQvASQFw-1 Received: by mail-yb1-f199.google.com with SMTP id 22-20020a250d160000b0290532b914c9f4so1236628ybn.9 for ; Fri, 28 May 2021 06:42:22 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=U2IqSZIHmJs1U626ov1801TPHqiAMQZgW3aXqcOf9ok=; b=RWplQwWW3NCqSEf8EW/ggo9D0EZ98Blk90C+aP7pVKF/L7uMFrtlVsY5oZxaF/2eKU I1EOMrCP1nmiiSOfPiY4Pr1UJNxP9WKztpR8XI6oc49DG4UFivAR8Hp+ItAqHWvHxSCt 0FsHTPr8RIm2z/EVOoRvE8ZKPYTGcMb+mcFdGgxpDy8SU1xjkB75eDMiJO5o0YkQcp6F QcEMLTDKHKaS9qEZKV4FF2qEgtZkcCDLygE+dPznfFYBmcWehLE9aegMZwqTzKmHh7qQ aVGtCxRy6+N6Vq+CdQbojPlDcwNUeuxn+zi/PH+LZx4EiimxD5WHQmR2UHZxfitROGb/ N+mA== X-Gm-Message-State: AOAM532k2ILjy1YO3S++3alANT39r0HJOcgRjFvBYOLAJN88trDsFbn4 ph+Ve5+1OTTaLuD7w71iAx5pnqKcBXGWG1+5Adaz4vuS+RFS1rpoDkZAgj8R7wS7ux2fM0sBD1w 1r7edfrK0ztzPbWwpbp5PcM68+kvx0ZESVRgCoZdJlw== X-Received: by 2002:a25:f208:: with SMTP id i8mr12505990ybe.340.1622209341654; Fri, 28 May 2021 06:42:21 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzY+Q3lAZD5z2I0qCaLzE8vPZbw30C0qe9jw96SwP96ICqH8giHKwWORUSXlm+cwx1tCc/b+Qf60iJexuKP47M= X-Received: by 2002:a25:f208:: with SMTP id i8mr12505951ybe.340.1622209341353; Fri, 28 May 2021 06:42:21 -0700 (PDT) MIME-Version: 1.0 References: <20210517092006.803332-1-omosnace@redhat.com> <01135120-8bf7-df2e-cff0-1d73f1f841c3@iogearbox.net> <4fee8c12-194f-3f85-e28b-f7f24ab03c91@iogearbox.net> In-Reply-To: <4fee8c12-194f-3f85-e28b-f7f24ab03c91@iogearbox.net> From: Ondrej Mosnacek Date: Fri, 28 May 2021 15:42:06 +0200 Message-ID: Subject: Re: [PATCH v2] lockdown,selinux: avoid bogus SELinux lockdown permission checks To: Daniel Borkmann Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=omosnace@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Jiri Olsa , Paul Moore , SElinux list , network dev , Stephen Smalley , James Morris , Steven Rostedt , Linux kernel mailing list , Casey Schaufler , Linux Security Module list , Ingo Molnar , Linux FS Devel , bpf , andrii.nakryiko@gmail.com, linuxppc-dev@lists.ozlabs.org Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" (I'm off work today and plan to reply also to Paul's comments next week, but for now let me at least share a couple quick thoughts on Daniel's patch.) On Fri, May 28, 2021 at 11:56 AM Daniel Borkmann wrote: > > On 5/28/21 9:09 AM, Daniel Borkmann wrote: > > On 5/28/21 3:37 AM, Paul Moore wrote: > >> On Mon, May 17, 2021 at 5:22 AM Ondrej Mosnacek wrote: > >>> > >>> Commit 59438b46471a ("security,lockdown,selinux: implement SELinux > >>> lockdown") added an implementation of the locked_down LSM hook to > >>> SELinux, with the aim to restrict which domains are allowed to perform > >>> operations that would breach lockdown. > >>> > >>> However, in several places the security_locked_down() hook is called in > >>> situations where the current task isn't doing any action that would > >>> directly breach lockdown, leading to SELinux checks that are basically > >>> bogus. > >>> > >>> Since in most of these situations converting the callers such that > >>> security_locked_down() is called in a context where the current task > >>> would be meaningful for SELinux is impossible or very non-trivial (and > >>> could lead to TOCTOU issues for the classic Lockdown LSM > >>> implementation), fix this by modifying the hook to accept a struct cred > >>> pointer as argument, where NULL will be interpreted as a request for a > >>> "global", task-independent lockdown decision only. Then modify SELinux > >>> to ignore calls with cred == NULL. > >> > >> I'm not overly excited about skipping the access check when cred is > >> NULL. Based on the description and the little bit that I've dug into > >> thus far it looks like using SECINITSID_KERNEL as the subject would be > >> much more appropriate. *Something* (the kernel in most of the > >> relevant cases it looks like) is requesting that a potentially > >> sensitive disclosure be made, and ignoring it seems like the wrong > >> thing to do. Leaving the access control intact also provides a nice > >> avenue to audit these requests should users want to do that. > > > > I think the rationale/workaround for ignoring calls with cred == NULL (or the previous > > patch with the unimplemented hook) from Ondrej was two-fold, at least speaking for his > > seen tracing cases: > > > > i) The audit events that are triggered due to calls to security_locked_down() > > can OOM kill a machine, see below details [0]. > > > > ii) It seems to be causing a deadlock via slow_avc_audit() -> audit_log_end() > > when presumingly trying to wake up kauditd [1]. Actually, I wasn't aware of the deadlock... But calling an LSM hook [that is backed by a SELinux access check] from within a BPF helper is calling for all kinds of trouble, so I'm not surprised :) > Ondrej / Paul / Jiri: at least for the BPF tracing case specifically (I haven't looked > at the rest but it's also kind of independent), the attached fix should address both > reported issues, please take a look & test. Thanks, I like this solution, although there are a few gotchas: 1. This patch creates a slight "regression" in that if someone flips the Lockdown LSM into lockdown mode on runtime, existing (already loaded) BPF programs will still be able to call the confidentiality-breaching helpers, while before the lockdown would apply also to them. Personally, I don't think it's a big deal (and I bet there are other existing cases where some handle kept from before lockdown could leak data), but I wanted to mention it in case someone thinks the opposite. 2. IIUC. when a BPF program is rejected due to lockdown/SELinux, the kernel will return -EINVAL to userspace (looking at check_helper_call() in kernel/bpf/verifier.c; didn't have time to look at other callers...). It would be nicer if the error code from the security_locked_down() call would be passed through the call chain and eventually returned to the caller. It should be relatively straightforward to convert bpf_base_func_proto() to return a PTR_ERR() instead of NULL on error, but it looks like this would result in quite a big patch updating all the callers (and callers of callers, etc.) with a not-so-small chance of missing some NULL check and introducing a bug... I guess we could live with EINVAL-on-denied in stable kernels and only have the error path refactoring in -next; I'm not sure... 3. This is a bit of a shot-in-the-dark, but I suppose there might be some BPF programs that would be able to do something useful also when the read_kernel helpers return an error, yet the kernel will now outright refuse to load them (when the lockdown hook returns nonzero). I have no idea if such BPF programs realistically exist in practice, but perhaps it would be worth returning some dummy always-error-returning helper function instead of NULL from bpf_base_func_proto() when security_locked_down() returns an error. That would also resolve (2.), basically. (Then there is the question of what error code to use (because Lockdown LSM uses -EPERM, while SELinux -EACCESS), but I think always returning -EPERM from such stub helpers would be a viable choice.) -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.