[Nagiosplug-devel] Security discussion - don't run as root plugins

[Thomas Guyot-Sionnest]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Andreas Ericsson wrote:
| Thomas Guyot-Sionnest wrote:
|> -----BEGIN PGP SIGNED MESSAGE-----
|> Hash: SHA1
|>
|> On 21/07/08 03:46 AM, Andreas Ericsson wrote:
|>> Thomas Guyot-Sionnest wrote:
|>>> One more though about it... I talked about a switch so far, but I think
|>>> it could be a better idea to make it an environment variable, so we
|>>> could drop root even before parsing arguments. Bugs in argument
|>>> processing could become a security issue if untrusted users has the
|>>> possibility to specify/alter arguments. While I'm aware there are many
|>>> other security implication regarding this, it's not a reason not to do
|>>> our best on the part we control.
|>>>
|>> The user controls the environment as well, so the net gain is zero.
|>>
|> When you parse arguments you usually parse them all, so you can have
|> bugs there... By using environment I can simply fetch the pointer to the
|> string in the environment before going any argument parsing. And you
|> won't have much to do before dropping privileges.
|>
|
| You're working on the assumption that someone by mistake will cause
| something bad to happen because he or she is running a plugin as root
| but without malicious intent. The attack vector is rather that someone
| might set out to do something malicious from the start, in which case
| we shouldn't even touch input from userland before we drop privileges,
| because the environment data may not be nicely formatted in the manner
| which we expect, and we can't guarantee library calls to be bugfree.
|
| The two scenarios where a plugin run as root is bad are as follows:
|
| An attacker has gained access to the system, sufficient enough to run
| various programs on it but without elevated (root) privileges. In
| order to gain further access, the attacker locates binaries with the
| setuid bit set and tries to exploit possible security holes in them
| in order to elevate his or her own privileges.
| A program parsing ANY KIND of user input is, at this stage, potentially
| vulnerable. In other words, the only safe thing that remains to do is
| setuid(getuid()); *before* we have even considered any user input at
| all.
|
| The other scenario is one where someone logged in as root runs a plugin
| with no malicious intent what so ever. However, someone has managed to
| feed the plugin some evil data from somewhere else, triggering a bug in
| said plugin and thereby gaining some sort of access through it.
|
| Now, in order to protect against scenario 1, it's impossible to protect
| against scenario 2, because we mustn't parse any userspace data at all
| before dropping privileges.
|
| To protect against scenario 2, we cannot drop privileges before we have
| parsed some data from userspace.
|
| The conclusion is that we'd need two solutions to get this to work
| properly.
|
| For scenario 1, we do the setuid(getuid()); thing, meaning it's perfectly
| safe to install all plugins suid root (except we don't know why the user
| did so in the first place, so we might actually break something for them,
| as it has to have been a conscious choice and not the default).
|
| For scenario 1, we must only do the extra parsing if (!(setuid() |
seteuid()).

The idea is far from suggesting users running plugins as setuid, and if
that's a problem we could make is an on/off switch only (so they're
would be absolutely no reason one would want to setuid his binaries). I
really can't be of any help if sysadmins start seuitd random binaries
for shit and giggles... For now it works and don't even drop privileges,
and refusing to run as root would piss-off most admins so it's not a
solution imo. Again keep in mind that is can only increase security.

Also, both scenarios assume the system is already vulnerable trough
libraries, so there might be plenty of other attacks possible.

| Imo, this snippet could work well:
|
| const char *run_as_uid;
| if (!(setuid() | seteuid()) { /* yes, we're actually root, not just
setsuid */
| 	int i;
| 	for (i = 1; i < argc; i++)
| 		char *arg = argv[i];
| 		if (!strncmp("--run-as=", arg, 9)) {
| 			run_as_uid = arg + 9;
| 			break;
| 		}
| 	}
| }
|
| if (run_as_uid && setuid(strtol(run_as_uid)) < 0)
| 	error("Failed to setuid(%s): %s\n", run_as_uid, strerror(errno));

Sure, but now you're going to update every plugin to no-op on that
argument... Unless you re-write argv and update argc.

| Relying on the environment is not very stable imo, as it's one area where
| memory exhaustion is so easily accomplished.

We only read there... It's not as we were adding new environment values.

|> Overall the security is still enhanced as for this extra code to run the
|> plugin must be root already (and this will avoid much more code running
|> as root).
|>
|
| That depends. If the plugin and its libraries has (exploitable) bugs, the
| extra code adds protection.
| If not, then this extra layer adds nothing and if the extra code is
| buggy in an exploitable way it even reduces security.

I'm sure we can handle such a small thing if we're careful enough.

Anyways I won't do anything before having more feedback, especially from
the team; I'm more concerned about the user impact...

Thanks for your input and suggestions.

- --
Thomas
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFIhPpF6dZ+Kt5BchYRAuWXAJ0WVsOlJHuXyBrJT3Q79g+fKrYcwwCgx1Y0
1tFCQ/KN5V0OM6m26HFR3WA=
=Uiak
-----END PGP SIGNATURE-----