From patchwork Fri Jan 21 08:40:12 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Johannes Segitz X-Patchwork-Id: 12719511 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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id BB23BC433EF for ; Fri, 21 Jan 2022 08:40:19 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231691AbiAUIkS (ORCPT ); Fri, 21 Jan 2022 03:40:18 -0500 Received: from smtp-out1.suse.de ([195.135.220.28]:34284 "EHLO smtp-out1.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229688AbiAUIkQ (ORCPT ); Fri, 21 Jan 2022 03:40:16 -0500 Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id 1CECE21910 for ; Fri, 21 Jan 2022 08:40:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1642754414; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc: mime-version:mime-version:content-type:content-type; bh=h2pRRYCk1RJJIWuSqUtOtGJXuG9Oa/NJR36JZAvfVVw=; b=dt+CO/pWYE2UuaVI9E02Aqr+wHgJ4hCwa/E7NW2is6M5CA6YUgAYgQ/3TA7VXnBi6MkOFG DqZRmQTQ0fncTo3A3zDqSZyMnlBPczONrTl+9tHU65+tKExKUXMeFrEf5hYMgPN8bQoeJl GD3y2GBGtkQC+BfpomNuI9+IayzQoR4= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1642754414; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc: mime-version:mime-version:content-type:content-type; bh=h2pRRYCk1RJJIWuSqUtOtGJXuG9Oa/NJR36JZAvfVVw=; b=NQNF94DQeWuRF8gJ/yWm+u6Ohng4vUiFJx2yXa9pMjuCzStwTNRLUnl9T1K6TsbODnvJ3j TR7OCj3fKan2P0AA== Received: from suse.com (unknown [10.163.27.146]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id 0BC4CA3B83 for ; Fri, 21 Jan 2022 08:40:14 +0000 (UTC) Date: Fri, 21 Jan 2022 09:40:12 +0100 From: Johannes Segitz To: selinux@vger.kernel.org Subject: [PATCH] libselinux: Cached security context not accurate Message-ID: <20220121084012.GS7643@suse.com> MIME-Version: 1.0 Content-Disposition: inline User-Agent: Mutt/1.10.1 (2018-07-13) Precedence: bulk List-ID: X-Mailing-List: selinux@vger.kernel.org Hello, a colleague of mine (Olaf Kirch) identified a problem with libselinux that has a small security impact. I discussed this with some people on this list privately before and we think it doesn't have a big impact and can be discussed here without harm. The reproducer below works on SUSE and RH systems. # gcc -Wall -o test -lselinux test.c # ./test Parent: child context according to library unconfined_u:unconfined_r:unconfined_t:s0 Parent: child context according to procfs: unconfined_u:unconfined_r:passwd_t:s0 bummer, context mismatch You need to make you system permissive to allow the initial context change. The problem is the procattr cache in libselinux, which doesn't work properly. Attached is a small patch that "fixes" the issue at the cost of not using the cache as soon as a pid is specified. With this applied we see the correct result: # ./test Parent: child context according to library unconfined_u:unconfined_r:passwd_t:s0 Parent: child context according to procfs: unconfined_u:unconfined_r:passwd_t:s0 In most use cases this issue will not be much of a problem, but it's still a small security issue, since this incorrect information might lead to incorrect access decisions. This way of doing it is inherently racy and should not be used for security decision, it's also probably pretty rare to see this. ATM not CVE was assigned. Strictly speaking we probably need one, but I won't push for it given the low impact. Thanks, Johannes /* * This demonstrates some odd behavior in libselinux. * * procattr query functions seem to return cached content from * previous calls to the corresponding set function, but without * taking the pid into account. * * This means that getpidcon() returns the context installed * by the most recent setcon() call, rather than the actual * context of the process we wanted to query. * * Apparently, this was introduced by commit 1d403326a * * Enjoy, * Olaf Kirch */ #include #include #include #include #include #include static void fatal(const char *msg) { perror(msg); exit(1); } static char * getpidcon_sinatra(pid_t pid) { static char linebuf[128]; char path[1024]; FILE *fp; snprintf(path, sizeof(path), "/proc/%d/attr/current", pid); if ((fp = fopen(path, "r")) == NULL) fatal(path); if (fgets(linebuf, sizeof(linebuf), fp) == NULL) fatal("read from /proc file"); fclose(fp); return linebuf; } int main(void) { char *lib_context = NULL, *actual_context; pid_t pid; pid = fork(); if (pid < 0) fatal("fork"); if (pid == 0) { if (setcon("unconfined_u:unconfined_r:passwd_t:s0") < 0) fatal("child setcon"); sleep(15); exit(0); } /* Set my own process context. * This will addle the library's brain by setting prev_current * in procattr.c */ if (setcon("unconfined_u:unconfined_r:unconfined_t:s0") < 0) fatal("parent setcon"); /* Wait for the child process to complete initialization */ sleep(1); /* Ask the library about the security context */ if (getpidcon(pid, &lib_context) < 0) fatal("getpidcon"); printf("Parent: child context according to library %s\n", lib_context); /* Query /proc/$pid/attr/current directly, which is what * the library is supposed to do. */ actual_context = getpidcon_sinatra(pid); printf("Parent: child context according to procfs: %s\n", actual_context); kill(pid, 9); if (strcmp(lib_context, actual_context) != 0) { printf("bummer, context mismatch\n"); return 1; } return 0; } Johannes Index: libselinux-3.3/src/procattr.c =================================================================== --- libselinux-3.3.orig/src/procattr.c +++ libselinux-3.3/src/procattr.c @@ -148,7 +148,7 @@ static int getprocattrcon_raw(char ** co return -1; } - if (prev_context && prev_context != UNSET) { + if (prev_context && prev_context != UNSET && !pid) { *context = strdup(prev_context); if (!(*context)) { return -1;