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=-7.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,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 C7640C433DF for ; Wed, 15 Jul 2020 20:37:25 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id A26382065F for ; Wed, 15 Jul 2020 20:37:25 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="l9A5pBS/" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725917AbgGOUhV (ORCPT ); Wed, 15 Jul 2020 16:37:21 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51836 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726786AbgGOUhS (ORCPT ); Wed, 15 Jul 2020 16:37:18 -0400 Received: from mail-pg1-x542.google.com (mail-pg1-x542.google.com [IPv6:2607:f8b0:4864:20::542]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id AB6C2C08C5DD for ; Wed, 15 Jul 2020 13:37:18 -0700 (PDT) Received: by mail-pg1-x542.google.com with SMTP id s189so3650724pgc.13 for ; Wed, 15 Jul 2020 13:37:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=Zaaf8OFN0Mkv/43hLUAJDeNUstQbcnrUeWnNFq9ntQY=; b=l9A5pBS/4KP4RTCNxMaKKaV+072X9gd7LEGM/3JWklkcz/Drht5gd7TJiFK4GxBxVr jZCmb1notoEqLmxKu0+ocLrw9VKEKpjvh8Va1zKB5Pza0ifi8AupC34OGZedIFseHjn/ 1r7EhDpsHB1nsJQkfGVNJefFs23IWUf/8byNs= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=Zaaf8OFN0Mkv/43hLUAJDeNUstQbcnrUeWnNFq9ntQY=; b=OmZo8yLpaDmMIxhnveT85XaiUr0f6Irk6+EwI5uD0JpMgjb86buTfbsqI8QwS7QwxJ HFZDEuG1f1hz6Ne1yY7XGy5r2ZufVNog8G5ZWVecNhiBywEevdSSPBZ6nRSnJ+C8ygIG BXDUIqSfNp27gKPMSYp/18Y4OWi94o8RUZCB9rfiQOYEyxLNKyjt337UMQXsEeOv+xcn PcvGHC2dwA7KTptZbjELuJo6bh7kmM67CSP7A60QAri/DqGofkpXSWnzitLhChZ2dty4 9CffRRavFtS1p6C3XESsM7VtlIb+uhfb5zQEpIBBEbw0iEcQMsXm6cnKvbXKtztexjJ9 jZfQ== X-Gm-Message-State: AOAM530Np414LNUgvq2Jy+TxQ1Fx6THsdeseCAWn2WZC6I2atuEzXotu k4ZyCmULlqPOurk0Ceypv/4LoA== X-Google-Smtp-Source: ABdhPJzwZ1pGNMV72yuIGcVlwa7f4Vu3SViQldrIVyIwMBZnJrwaLmWw/w85VJ0XhTGIWtU4XLlfyA== X-Received: by 2002:a63:7c4d:: with SMTP id l13mr1353283pgn.12.1594845437856; Wed, 15 Jul 2020 13:37:17 -0700 (PDT) Received: from www.outflux.net (smtp.outflux.net. [198.145.64.163]) by smtp.gmail.com with ESMTPSA id h15sm2871918pfo.192.2020.07.15.13.37.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 15 Jul 2020 13:37:16 -0700 (PDT) Date: Wed, 15 Jul 2020 13:37:15 -0700 From: Kees Cook To: =?iso-8859-1?Q?Micka=EBl_Sala=FCn?= Cc: linux-kernel@vger.kernel.org, Aleksa Sarai , Alexei Starovoitov , Al Viro , Andrew Morton , Andy Lutomirski , Christian Brauner , Christian Heimes , Daniel Borkmann , Deven Bowers , Dmitry Vyukov , Eric Biggers , Eric Chiang , Florian Weimer , James Morris , Jan Kara , Jann Horn , Jonathan Corbet , Lakshmi Ramasubramanian , Matthew Garrett , Matthew Wilcox , Michael Kerrisk , =?iso-8859-1?Q?Micka=EBl_Sala=FCn?= , Mimi Zohar , Philippe =?iso-8859-1?Q?Tr=E9buchet?= , Scott Shell , Sean Christopherson , Shuah Khan , Steve Dower , Steve Grubb , Tetsuo Handa , Thibaut Sautereau , Vincent Strubel , kernel-hardening@lists.openwall.com, linux-api@vger.kernel.org, linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org, linux-fsdevel@vger.kernel.org Subject: Re: [PATCH v6 5/7] fs,doc: Enable to enforce noexec mounts or file exec through O_MAYEXEC Message-ID: <202007151312.C28D112013@keescook> References: <20200714181638.45751-1-mic@digikod.net> <20200714181638.45751-6-mic@digikod.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20200714181638.45751-6-mic@digikod.net> Sender: linux-api-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-api@vger.kernel.org On Tue, Jul 14, 2020 at 08:16:36PM +0200, Mickaël Salaün wrote: > @@ -2849,7 +2855,7 @@ static int may_open(const struct path *path, int acc_mode, int flag) > case S_IFLNK: > return -ELOOP; > case S_IFDIR: > - if (acc_mode & (MAY_WRITE | MAY_EXEC)) > + if (acc_mode & (MAY_WRITE | MAY_EXEC | MAY_OPENEXEC)) > return -EISDIR; > break; (I need to figure out where "open for reading" rejects S_IFDIR, since it's clearly not here...) > case S_IFBLK: > @@ -2859,13 +2865,26 @@ static int may_open(const struct path *path, int acc_mode, int flag) > fallthrough; > case S_IFIFO: > case S_IFSOCK: > - if (acc_mode & MAY_EXEC) > + if (acc_mode & (MAY_EXEC | MAY_OPENEXEC)) > return -EACCES; > flag &= ~O_TRUNC; > break; This will immediately break a system that runs code with MAY_OPENEXEC set but reads from a block, char, fifo, or socket, even in the case of a sysadmin leaving the "file" sysctl disabled. > case S_IFREG: > - if ((acc_mode & MAY_EXEC) && path_noexec(path)) > - return -EACCES; > + if (path_noexec(path)) { > + if (acc_mode & MAY_EXEC) > + return -EACCES; > + if ((acc_mode & MAY_OPENEXEC) && > + (sysctl_open_mayexec_enforce & OPEN_MAYEXEC_ENFORCE_MOUNT)) > + return -EACCES; > + } > + if ((acc_mode & MAY_OPENEXEC) && > + (sysctl_open_mayexec_enforce & OPEN_MAYEXEC_ENFORCE_FILE)) > + /* > + * Because acc_mode may change here, the next and only > + * use of acc_mode should then be by the following call > + * to inode_permission(). > + */ > + acc_mode |= MAY_EXEC; > break; > } Likely very minor, but I'd like to avoid the path_noexec() call in the fast-path (it dereferences a couple pointers where as doing bit tests on acc_mode is fast). Given that and the above observations, I think that may_open() likely needs to start with: if (acc_mode & MAY_OPENEXEC) { /* Reject all file types when mount enforcement set. */ if ((sysctl_open_mayexec_enforce & OPEN_MAYEXEC_ENFORCE_MOUNT) && path_noexec(path)) return -EACCES; /* Treat the same as MAY_EXEC. */ if (sysctl_open_mayexec_enforce & OPEN_MAYEXEC_ENFORCE_FILE)) acc_mode |= MAY_EXEC; } (Though I'm not 100% sure that path_noexec() is safe to be called for all file types: i.e. path->mnt and path->-mnt->mnt_sb *always* non-NULL?) This change would also imply that OPEN_MAYEXEC_ENFORCE_FILE *includes* OPEN_MAYEXEC_ENFORCE_MOUNT (i.e. the sysctl should not be a bitfield), since path_noexec() would get checked for S_ISREG. I can't come up with a rationale where one would want OPEN_MAYEXEC_ENFORCE_FILE but _not_ OPEN_MAYEXEC_ENFORCE_MOUNT? (I can absolutely see wanting only OPEN_MAYEXEC_ENFORCE_MOUNT, or suddenly one has to go mark every loaded thing with the exec bit and most distros haven't done this to, for example, shared libraries. But setting the exec bit and then NOT wanting to enforce the mount check seems... not sensible?) Outside of this change, yes, I like this now -- it's much cleaner because we have all the checks in the same place where they belong. :) > diff --git a/kernel/sysctl.c b/kernel/sysctl.c > index db1ce7af2563..5008a2566e79 100644 > --- a/kernel/sysctl.c > +++ b/kernel/sysctl.c > @@ -113,6 +113,7 @@ static int sixty = 60; > > static int __maybe_unused neg_one = -1; > static int __maybe_unused two = 2; > +static int __maybe_unused three = 3; > static int __maybe_unused four = 4; > static unsigned long zero_ul; > static unsigned long one_ul = 1; Oh, are these still here? I thought they got removed (or at least made const). Where did that series go? Hmpf, see sysctl_vals, but yes, for now, this is fine. > @@ -888,7 +889,6 @@ static int proc_taint(struct ctl_table *table, int write, > return err; > } > > -#ifdef CONFIG_PRINTK > static int proc_dointvec_minmax_sysadmin(struct ctl_table *table, int write, > void *buffer, size_t *lenp, loff_t *ppos) > { > @@ -897,7 +897,6 @@ static int proc_dointvec_minmax_sysadmin(struct ctl_table *table, int write, > > return proc_dointvec_minmax(table, write, buffer, lenp, ppos); > } > -#endif > > /** > * struct do_proc_dointvec_minmax_conv_param - proc_dointvec_minmax() range checking structure > @@ -3264,6 +3263,15 @@ static struct ctl_table fs_table[] = { > .extra1 = SYSCTL_ZERO, > .extra2 = &two, > }, > + { > + .procname = "open_mayexec_enforce", > + .data = &sysctl_open_mayexec_enforce, > + .maxlen = sizeof(int), > + .mode = 0600, > + .proc_handler = proc_dointvec_minmax_sysadmin, > + .extra1 = SYSCTL_ZERO, > + .extra2 = &three, > + }, > #if defined(CONFIG_BINFMT_MISC) || defined(CONFIG_BINFMT_MISC_MODULE) > { > .procname = "binfmt_misc", > -- > 2.27.0 > -- Kees Cook