From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id CFCC63857011 for ; Mon, 18 Jan 2021 01:13:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org CFCC63857011 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-17-TDWb7vX1PEW8T-YlbgDwbg-1; Sun, 17 Jan 2021 20:13:09 -0500 X-MC-Unique: TDWb7vX1PEW8T-YlbgDwbg-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 1DF89806664 for ; Mon, 18 Jan 2021 01:13:08 +0000 (UTC) Received: from greed.delorie.com (ovpn-113-151.rdu2.redhat.com [10.10.113.151]) by smtp.corp.redhat.com (Postfix) with ESMTPS id E786F3828; Mon, 18 Jan 2021 01:13:07 +0000 (UTC) Received: from greed.delorie.com.redhat.com (localhost [127.0.0.1]) by greed.delorie.com (8.14.7/8.14.7) with ESMTP id 10I1D7WF000665; Sun, 17 Jan 2021 20:13:07 -0500 From: DJ Delorie To: Florian Weimer Cc: libc-alpha@sourceware.org Subject: Re: nsswitch: do not reload if "/" changes In-Reply-To: <87pn25nog9.fsf@oldenburg.str.redhat.com> (message from Florian Weimer on Sat, 16 Jan 2021 11:52:38 +0100) Date: Sun, 17 Jan 2021 20:13:07 -0500 Message-ID: MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-13.0 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 18 Jan 2021 01:13:14 -0000 Florian Weimer writes: > dev_t for root_dev? Oops. > Perhaps you can match the declaration order in the initialization? Fixed. > This does not seem to be needed? Heh, I had removed the code, but forgot to save that buffer. Fixed. > I have one remaining question: Should we load service modules after / > has changed? Disabling reloading brings us back to the old behavior in > terms of exposure to untrusted /, but maybe we can do even better and > stop loading service modules altogether? Assuming that this change is > compatible with init systems. This patch makes it "no worse than before" but I'm not sure we can make it better than before, because we have no hints that we're entering a container, and by the time we have, it's too late to load the right module. The options become (1) don't load the module and definitely fail, or (2) maybe load the module in the container and work (and, depending on your app, open a security hole?) (which is the "old way"). We would either need a new API that says "about to enter container" (or hack into the namespace syscalls) or at least dlopen all mentioned modules when we parse nsswitch.conf >From f4e9b7d9f9f6f513a9e1264e69b1e666d441de80 Mon Sep 17 00:00:00 2001 From: DJ Delorie Date: Fri, 15 Jan 2021 19:50:00 -0500 Subject: nsswitch: do not reload if "/" changes https://sourceware.org/bugzilla/show_bug.cgi?id=27077 Before reloading nsswitch.conf, verify that the root directory hasn't changed - if it has, it's likely that we've entered a container and should not trust the nsswitch inside the container nor load any shared objects therein. diff --git a/nss/nss_database.c b/nss/nss_database.c index e719ec0865..4b4730b114 100644 --- a/nss/nss_database.c +++ b/nss/nss_database.c @@ -33,6 +33,11 @@ struct nss_database_state { struct nss_database_data data; __libc_lock_define (, lock); + /* If "/" changes, we switched into a container and do NOT want to + reload anything. This data must be persistent across + reloads. */ + ino64_t root_ino; + dev_t root_dev; }; @@ -54,6 +59,8 @@ global_state_allocate (void *closure) result->data.initialized = true; result->data.reload_disabled = false; __libc_lock_init (result->lock); + result->root_ino = 0; + result->root_dev = 0; } return result; } @@ -356,6 +363,8 @@ nss_database_check_reload_and_get (struct nss_database_state *local, nss_action_list *result, enum nss_database database_index) { + struct stat64 str; + /* Acquire MO is needed because the thread that sets reload_disabled may have loaded the configuration first, so synchronize with the Release MO store there. */ @@ -379,6 +388,25 @@ nss_database_check_reload_and_get (struct nss_database_state *local, __libc_lock_unlock (local->lock); return true; } + + /* Before we reload, verify that "/" hasn't changed. We assume that + errors here are very unlikely, but the chance that we're entering + a container is also very unlikely, so we err on the side of both + very unlikely things not happening at the same time. */ + if (__stat64 ("/", &str) == 0) + { + if (local->root_ino != 0 + && (str.st_ino != local->root_ino + || str.st_dev != local->root_dev)) + { + /* Change detected; disable reloading. */ + atomic_store_release (&local->data.reload_disabled, 1); + __libc_lock_unlock (local->lock); + return true; + } + local->root_ino = str.st_ino; + local->root_dev = str.st_dev; + } __libc_lock_unlock (local->lock); /* Avoid overwriting the global configuration until we have loaded