GNOME Bugzilla – Bug 701112
"make check" fails as root
Last modified: 2015-05-19 08:29:49 UTC
In a built tree, running "sudo make check" fails in ake[7]: Entering directory `/home/martin/upstream/NetworkManager/src/settings/plugins/keyfile/tests' /home/martin/upstream/NetworkManager/src/settings/plugins/keyfile/tests/test-keyfile FAIL: (connection-read) failed to read /home/martin/upstream/NetworkManager/src/settings/plugins/keyfile/tests/keyfiles/Test_Wired_Connection make[7]: *** [check-local] Error 1 That's because nm_keyfile_plugin_connection_from_file() does this check: bad_owner = getuid () != statbuf.st_uid; i. e. if you run the checks (or NM) as root it considers a non-root file a bad owner. Could we allow a special exception for root here? Also, why do we need this check at all? Standard Unix file permissions/ACLs/MAC systems should already control whether user A can read a file from user B, after all?
Created attachment 245427 [details] [review] keyfile: Allow root to read files from other users That's the conservative approach to only disable the "bad owner" check for root. This is not a security issue -- a process which already has root can chown and chmod the files in any way it likes.
I was thinking about this some time ago and my opinion is that we should generally let the operating system do its job and check the files. Some advanced checks could be performed by selinux or other security tools. But this is actually a reverse check, not much different from SSH sanity checks. It is intended to check for inconsistent configuration (from the security point of view) and teach the administrator to avoid such configuration. So it's not an access control tool but a learning tool, at least in my opinion, as I don't think it's documented anywhere. Your patch is wrong, as it specifically disables the check in the only situation where it's useful. A better way would be to remove it entirely, as you already suggested. So, basically, the bug report is about two questions: 1) Is 'sudo make check' a valid use case for NetworkManager code base? Alternative approach is to only do root checks with a (optionally virtualized) root account where the whole build tree is owned by root. 2) Is this sanity check necessary? My personal answer is that it isn't. But in my opinion it doesn't cause any harm, see below. You should know that NetworkManager is historically built upon assumption that it manages its own configuration files. AFAIK (I wasn't a developer then) many automatic runtime writes were removed over the time in favor of files outside the configuration tree. But it still modifies on-disk configuration on user request and Jiří Klimeš is working on nmcli which through libnm-glib and dbus API manages the whole configuration. If you do any advanced testing, NetworkManager can re-create the configuration files at any time, effectively assuming ownership over the files. Therefore the configuration files are by definition writable by NetworkManager and when writing, NetworkManager should avoid leaking secrets to non-root users. Therefore even if you disable the sanity check, you are not safe mixing non-root and root access to the source tree. This makes the bug report an adept for WONTFIX or NOTABUG. But there's an actual issue behind the bug report that the behavior is surprising. In my opinion, 'make check' should sanity-check the current UID and some of the files for ownership and report the problem immediately, not through checks like that one. Note that there are other problems with the build system and especially 'make check' so feel free to contact me whenever you step on something. I'll try to report some of them so that we can keep track of them.
Other than at "make check" time, the code will only ever be run as root, so leaving the code there, but disabled for root, would be silly. When I ran into this, I assumed the code was to make sure that unprivileged users could not somehow spoof NM into reading keyfiles that they had created themselves. Although it's not clear why we would worry about this more in the keyfile case than in the other plugins... The code was has been there since the keyfile plugin was first written, so there's no hint in the git log as to why. (And it was written by Tambet, not Dan.) I'm thinking we can just lose that check.
Comment on attachment 245427 [details] [review] keyfile: Allow root to read files from other users Thanks, Dan and Pawel for the explanations. So the proposed patch is definitively wrong, and we should either kill the check entirely, or change the test to make a copy to a temporary file, and run the test against that one.
(In reply to comment #3) > When I ran into this, I assumed the code was to make sure that unprivileged > users could not somehow spoof NM into reading keyfiles that they had created > themselves. Which is not possible unless you deliberately give them write access to the respective directory. > Although it's not clear why we would worry about this more in the > keyfile case than in the other plugins... I think this is because of the possibility that a user reads a configuration file with a password included. AFAIK ifcfg-rh stores passwords separately. > I'm thinking we can just lose that check. +1 from me. (In reply to comment #4) > we should either kill the check entirely, or change the > test to make a copy to a temporary file, and run the test against that one. As stated above, +1 for removal, OK with tmp file as a second option.
Hi Martin, what's the current status of this issue?
Created attachment 250120 [details] [review] keyfile: Drop owner check Ah, sorry for forgetting about this for so long. As you both agree that the check doesn't make much sense any more, this patch just drops it instead.
Pushed, thanks.
(In reply to comment #3) > Other than at "make check" time, the code will only ever be run as root, so > leaving the code there, but disabled for root, would be silly. > > When I ran into this, I assumed the code was to make sure that unprivileged > users could not somehow spoof NM into reading keyfiles that they had created > themselves. Although it's not clear why we would worry about this more in the > keyfile case than in the other plugins... ifcfg-rh handles this specifically by storing secret data in 'keys-xxxx' files that *are* only owned by root, while the ifcfg files themselves can be readable by any user since they don't contain secrets. In keyfile since the secrets are in the same file, we need to be really careful about permissions and owners to ensure we don't leak out secrets. > The code was has been there since the keyfile plugin was first written, so > there's no hint in the git log as to why. (And it was written by Tambet, not > Dan.) I can't think of any good reason that non-root users should have access to the keyfiles, since they store secrets, and those secrets may not be visible to your user (due to the permissions key, or policykit, or whatever). Thus it was prudent to ensure that NM would not read any keyfile that was not owned by root to explicitly ensure that secrets never, ever got leaked out to users who shouldn't be allowed to see them. Otherwise all the PolicyKit and visibility stuff we do in NM itself is pointless. So what if by some mistake /etc/NetworkManager/system-connections becomes world-readable/executable? The question is how much do we try to prevent fallout from user mistakes, and in this case since there's no reason to have normal users see these files, I think it's prudent and reasonable to ensure they have the correct permissions. If the testcases are failing due to those checks, shouldn't we fix the testcases to work around it, instead of removing the checks that could result in a security problem later on?
(In reply to comment #9) > So what if by some mistake /etc/NetworkManager/system-connections becomes > world-readable/executable? Note that we only removed the owner check. Therefore the change doesn't affect root-owned files *at all*. The question actually stands: What if by some mistake /etc/NetworkManager/system-connections becomes world-rx *and* the administrator deliberately places a user-owned file there? Does that sound better? > If the testcases are failing due to those checks, shouldn't we fix the > testcases to work around it, instead of removing the checks that could result > in a security problem later on? In this case the problem is not in the testcases but rather in NetworkManager code which prevents from being run in test environments. A possible solution would be to be able to tell NetworkManager (via environment or command line arguments) to refrain from performing security checks. Also I don't think we are doing any real security here. Making /etc/NetworkManager/system-connections world-readable is a bad action and you could just as well check only that directory when NetworkManager is started (also subject to ignoring for testing purposes as described above) and stop forcing administrators to play with chmod for each new connection. Another option would be to use LD_PRELOAD to fool NetworkManager when being run for testing purposes, see for example: http://www.linuxjournal.com/article/7795 The more magic you do in order to please the security deities, the more problems you will bring upon yourself ;).
(In reply to comment #9) > So what if by some mistake /etc/NetworkManager/system-connections becomes > world-readable/executable? /etc/NetworkManager/system-connections *is* world-readable. It's just that all the files in it are mode 0600. > in this case since there's no reason to have > normal users see these files, I think it's prudent and reasonable to ensure > they have the correct permissions. which we don't do; we check the ownership, but not the mode. > If the testcases are failing due to those checks, shouldn't we fix the > testcases to work around it, instead of removing the checks that could result > in a security problem later on? The issue is that I don't think there's actually a coherent security model for connection files right now. Why don't we come up with a list of what we think the plugins *should* be doing, and then make sure that they're actually doing that, and then figure out how that affects the tests.
(In reply to comment #11) > (In reply to comment #9) > > So what if by some mistake /etc/NetworkManager/system-connections becomes > > world-readable/executable? > > /etc/NetworkManager/system-connections *is* world-readable. It's just that all > the files in it are mode 0600. Could be changed, if so. > > in this case since there's no reason to have > > normal users see these files, I think it's prudent and reasonable to ensure > > they have the correct permissions. > > which we don't do; we check the ownership, but not the mode. Before the patch we checked both, now we check mode, not ownership, speaking about keyfile. > Why don't we come up with a list of what we think the plugins *should* be > doing, and then make sure that they're actually doing that, and then figure out > how that affects the tests. Sounds reasonable.
Hi Martin, pity we didn't meet at FOSDEM as I heard from RH QA folks that you were in contact but only too late. Is there a status update on the testing stuff and this very bugzilla?
Hey Pavel, indeed I said hello to Vadim yesterday, but we didn't actually discuss the NM tests. These fell off my radar TBH, at least their upstream submission; the tests themselves are running happily in our CI (we currently have version 0.9.8.8), so they are still valid. As for this particular bug, the original fix (http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=5dc4be54) is still in place, and "make check" as root succeeds. Back then it seemed there was some discussion between Dan and you as to how that commit should be changed, but TBH I'm not sure that there was a definitive outcome? Thanks, Martin
Created attachment 294519 [details] [review] core/tests: add nm_utils_get_testing() function Code that is testable often needs decidated hooks and workaround to work both for unit-tests and production. Add a function nm_utils_get_testing() that will return TRUE if run as part of a unit-test.
Created attachment 294520 [details] [review] keyfile: readd owner check of files Commit 5dc4be54e62aa0168478a3c9d6cf45c551c5ace8 dropped the owner check for keyfiles to allow running `make check` as root. Re-add it, but disable the check for tests.
Looks good.
patches reworked and pushed to branch th/keyfile-owner-check-bgo701112
(In reply to Thomas Haller from comment #18) > patches reworked and pushed to branch th/keyfile-owner-check-bgo701112 For the reasoning: - first I added nm_utils_get_testing(), which is similar to g_test_initialized(). Actually, I forgot that g_test_initialized() existed, hence I added nm_utils_get_testing(). We could only use g_test_initialized(). OTOH, I don't mind having our own test utility helpers that are distinct from glibs. Also, g_test_initialized() only exists since glib 2.36. - later I added flags (NMUtilTestFlags) instead of one boolean value. The idea is that the you can explicitly enable workarounds in the library to make the code testable -- contrary, to just having one flag. With this, you can grep for "NM_UTILS_TEST_NO_KEYFILE_OWNER_CHECK" and find the tests that use this workaround and find the code that is affected. This only makes sense, if we think this is a good pattern and we add more flags later on. I don't really mind which way, I want to re-enable the owner check.
LGTM
Rebased on master
Merged to master: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=046115b58885e6cc8694ae04b8e5bb4f2bd093bf This re-enables the root-check, but disables it for tests. Closing BZ now.