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

[Andreas Ericsson]

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()).

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));

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

> 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.

-- 
Andreas Ericsson                   andreas.ericsson at op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231