Permalink
Browse files

node: more secure state file handling

Currently, plugins which run as root mix their state files in the same
directory as non-root plugins. The state directory is owned by munin:munin and
is group-writable. Because of these facts, it is possible for an attacker who
operates as user munin to cause a root-run plugin to run arbitrary code as
root.

This commit introduces :
- a new plugin state directory root, owned by uid 0
- each plugin runs in its own UID plugin state directory, owned by the said UID.

Closes: #1234, D#679897, R#849830, CVE-2012-3512
  • Loading branch information...
1 parent 71f7ccb commit 780634c4a48fc57b6631d644fca3649f1417d211 @steveschnepp steveschnepp committed Aug 29, 2012
Showing with 27 additions and 8 deletions.
  1. +2 −3 Makefile
  2. +5 −2 Makefile.config
  3. +9 −0 node/lib/Munin/Node/OS.pm
  4. +9 −1 node/lib/Munin/Node/Service.pm
  5. +2 −2 plugins/lib/Munin/Plugin.pm
View
@@ -144,9 +144,8 @@ install-plugins-prime: install-plugins build $(PLUGINS) Makefile Makefile.config
mkdir -p $(LIBDIR)/plugins
mkdir -p $(PLUGSTATE)
- $(CHOWN) $(PLUGINUSER):$(GROUP) $(PLUGSTATE)
- # using g+rwxs, so plugins can create and modify their state file without help
- $(CHMOD) 02775 $(PLUGSTATE)
+ $(CHOWN) root:root $(PLUGSTATE)
+ $(CHMOD) 0755 $(PLUGSTATE)
$(CHMOD) 0755 $(CONFDIR)/plugin-conf.d
for p in build/plugins/node.d/* build/plugins/node.d.$(OSTYPE)/* ; do \
View
@@ -41,16 +41,19 @@ LIBDIR = $(PREFIX)/lib
HTMLDIR = $(PREFIX)/www/docs
CGIDIR = $(PREFIX)/www/cgi
-# Where to put RRD files and other internal data, both master and node
+# Where to put internal data for master (RRD, internal files, ...)
DBDIR = $(DESTDIR)/var/opt/munin
+# Where to put internal data for node (plugin state, ...)
+DBDIRNODE = $(DESTDIR)/var/opt/munin-node
+
# Client only - Where the spool files are written. Must be writable by
# group "munin", and should be preserved between reboots
SPOOLDIR = $(DBDIR)/spool
# Client only - Where plugins should put their states. Must be writable by
# group "munin", and should be preserved between reboots
-PLUGSTATE = $(DBDIR)/plugin-state
+PLUGSTATE = $(DBDIRNODE)/plugin-state
# Where Munin should place its logs.
LOGDIR = $(PREFIX)/log/munin
@@ -13,6 +13,7 @@ use Munin::Common::Timeout;
use POSIX ();
use Sys::Hostname;
+use File::Path qw(make_path);
sub get_uid {
my ($class, $user) = @_;
@@ -249,6 +250,14 @@ sub _set_xid {
sub set_umask { umask(0002) or croak "Unable to set umask: $!\n"; }
+sub mkdir_subdir {
+ my ($path, $user) = @_;
+
+ unless (-d "$path/$user") {
+ mkdir("$path/$user");
+ chown($user, "root", "$path/$user");
+ }
+}
1;
@@ -119,8 +119,12 @@ sub export_service_environment {
my ($self, $service) = @_;
print STDERR "# Setting up environment\n" if $config->{DEBUG};
+ # We append the USER to the MUNIN_PLUGSTATE, to avoid CVE-2012-3512
+ my $user = $config->{sconf}{$service}{user};
+ $ENV{MUNIN_PLUGSTATE} = "$Munin::Common::Defaults::MUNIN_PLUGSTATE/$user";
+
# Provide a consistent default state-file.
- $ENV{MUNIN_STATEFILE} = "$Munin::Common::Defaults::MUNIN_PLUGSTATE/$service-$ENV{MUNIN_MASTER_IP}";
+ $ENV{MUNIN_STATEFILE} = "$ENV{MUNIN_PLUGSTATE}/$service-$ENV{MUNIN_MASTER_IP}";
my $env = $config->{sconf}{$service}{env} or return;
@@ -236,6 +240,10 @@ sub exec_service
{
my ($self, $service, $arg) = @_;
+ # XXX - Create the statedir for the user
+ my $user = $config->{sconf}{$service}{user};
+ Munin::Node::OS->mkdir_subdir("$Munin::Common::Defaults::MUNIN_PLUGSTATE", $user);
+
$self->change_real_and_effective_user_and_group($service);
unless (Munin::Node::OS->check_perms_if_paranoid("$self->{servicedir}/$service")) {
@@ -42,7 +42,7 @@ If your Munin installation predates the MUNIN_* environment variables
(introduced in 1.3.3) you can put this in your plugin configuration:
[*]
- env.MUNIN_PLUGSTATE /lib/munin/plugin-state
+ env.MUNIN_PLUGSTATE /var/lib/munin-node/plugin-state
env.MUNIN_LIBDIR /usr/share/munin
IF, indeed that is the munin plugin state directory. The default
@@ -602,7 +602,7 @@ need to be run as a special user or need special priveliges.
There is some test stuff in this module.
Test like this:
- MUNIN_PLUGSTATE=/var/lib/munin/plugin-state -e 'require "Plugin.pm.in"; Munin::Plugin::_test;' -- or something.
+ MUNIN_PLUGSTATE=/var/lib/munin-node/plugin-state -e 'require "Plugin.pm.in"; Munin::Plugin::_test;' -- or something.
sub _test () {
my $pos;

0 comments on commit 780634c

Please sign in to comment.