From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-yw1-f67.google.com ([209.85.161.67]:42964 "EHLO mail-yw1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727670AbeIMTtg (ORCPT ); Thu, 13 Sep 2018 15:49:36 -0400 Received: by mail-yw1-f67.google.com with SMTP id n207-v6so1243961ywn.9 for ; Thu, 13 Sep 2018 07:39:47 -0700 (PDT) Received: from mail-yb1-f176.google.com (mail-yb1-f176.google.com. [209.85.219.176]) by smtp.gmail.com with ESMTPSA id w207-v6sm5075661yww.17.2018.09.13.07.39.44 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 13 Sep 2018 07:39:44 -0700 (PDT) Received: by mail-yb1-f176.google.com with SMTP id c4-v6so3351694ybl.6 for ; Thu, 13 Sep 2018 07:39:44 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <20180911183909.233413-1-jannh@google.com> From: Kees Cook Date: Thu, 13 Sep 2018 07:39:43 -0700 Message-ID: Subject: Re: [PATCH] proc: restrict kernel stack dumps to root To: Jann Horn Cc: Alexey Dobriyan , Ken Chen , kernel list , "linux-fsdevel@vger.kernel.org" , Will Deacon , Laura Abbott , Andy Lutomirski , Security Officers , Catalin Marinas , Josh Poimboeuf , Thomas Gleixner , Ingo Molnar , "H . Peter Anvin" , Linux API Content-Type: text/plain; charset="UTF-8" Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Thu, Sep 13, 2018 at 4:55 AM, Jann Horn wrote: > On Thu, Sep 13, 2018 at 12:28 AM Kees Cook wrote: >> >> On Wed, Sep 12, 2018 at 8:29 AM, Jann Horn wrote: >> > +linux-api, I guess >> > >> > On Tue, Sep 11, 2018 at 8:39 PM Jann Horn wrote: >> >> >> >> Restrict the ability to inspect kernel stacks of arbitrary tasks to root >> >> in order to prevent a local attacker from exploiting racy stack unwinding >> >> to leak kernel task stack contents. >> >> See the added comment for a longer rationale. >> >> >> >> There don't seem to be any users of this userspace API that can't >> >> gracefully bail out if reading from the file fails. Therefore, I believe >> >> that this change is unlikely to break things. >> >> In the case that this patch does end up needing a revert, the next-best >> >> solution might be to fake a single-entry stack based on wchan. >> >> >> >> Fixes: 2ec220e27f50 ("proc: add /proc/*/stack") >> >> Cc: stable@vger.kernel.org >> >> Signed-off-by: Jann Horn >> >> --- >> >> fs/proc/base.c | 14 ++++++++++++++ >> >> 1 file changed, 14 insertions(+) >> >> >> >> diff --git a/fs/proc/base.c b/fs/proc/base.c >> >> index ccf86f16d9f0..7e9f07bf260d 100644 >> >> --- a/fs/proc/base.c >> >> +++ b/fs/proc/base.c >> >> @@ -407,6 +407,20 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns, >> >> unsigned long *entries; >> >> int err; >> >> >> >> + /* >> >> + * The ability to racily run the kernel stack unwinder on a running task >> >> + * and then observe the unwinder output is scary; while it is useful for >> >> + * debugging kernel issues, it can also allow an attacker to leak kernel >> >> + * stack contents. >> >> + * Doing this in a manner that is at least safe from races would require >> >> + * some work to ensure that the remote task can not be scheduled; and >> >> + * even then, this would still expose the unwinder as local attack >> >> + * surface. >> >> + * Therefore, this interface is restricted to root. >> >> + */ >> >> + if (!file_ns_capable(m->file, &init_user_ns, CAP_SYS_ADMIN)) >> >> + return -EACCES; >> >> In the past, we've avoided hard errors like this in favor of just >> censoring the output. Do we want to be more cautious here? (i.e. >> return 0 or a fuller seq_printf(m, "[<0>] privileged\n"); return 0;) > > In my mind, this is different because it's a place where we don't have > to selectively censor output while preserving parts of it, and it's a > place where, as Laura said, it's useful to make lack of privileges > clearly visible because that informs users that they may have to retry > with more privileges. > > Of course, if you have an example of software that actually breaks due > to this, I'll change it. But I looked at the three things in Debian > codesearch that seem to use it, and from what I can tell, they all > bail out cleanly when the read fails. I prefer -EACCESS too, but I thought I'd mention the alternative. So, I guess: Reviewed-by: Kees Cook :) -Kees -- Kees Cook Pixel Security