[radvd-devel-l] [RFC] Add privilege separation capability on
Linux hosts.
Pekka Savola
pekkas at netcore.fi
Thu Jan 24 12:11:32 EST 2008
On Thu, 24 Jan 2008, Jim Paris wrote:
> I agree, and I looked into capabilities a bit, but I think even if we
> could reduce it to a set that says "this process can write root-owned
> files", we're not gaining much in terms of security.
>
> Another idea that I had would be to just open a filehandle to each of
> the files we might need to modify, drop privs, then use
> rewind(fd);
> fprintf(fd, "%d\n", new-value);
> every time we want to change them. For /proc/sys/net, that seems to
> work fine in my tests. However, because the ifname is part of the
> filename, we don't know prior to radvd.conf changes what to open.
> So the support would be limited to only interfaces that were either
> present in radvd.conf initially, or maybe all interfaces present at
> startup. And we also lose the ability to sanity check the values.
> But maybe the benefits (no second process, no privileges) are worth it?
I don't have a very strong opinion. Actually, I'm not sure if the
config_interface() should have been part of radvd in the first place
(instead, something the admin has to do manually), but it crept in and
is causing trouble now..
All radvd configs I've seen have had all the potential interfaces
already defined in config, but:
1) the interfaces might not have existed yet at the time radvd is
first run (e.g., some USB-based links), or
2) advertisements are disabled (sendadvert is off) when starting.
Although in almost all scenarios we should be able to know the
interface names in advance, the first category is tricky because
interfaces don't necessarily have their proc branches when radvd is
first run. As a result at least in this scenario the mechanism you
suggest would not work although it looks tempting...
>> - due to code duplication, I removed privsep_set and replaced it with an
>> improved version of set_interface_var. I'm not completely happy with code
>> structure as it is (two different codepaths in two different files, when
>> privsep is on/off, two different error messages; variable max/min checking
>> only happens with privsep), but it's probably good enough.
>
> It looks like you didn't get rid of privsep_set, uncommitted changes
> maybe? I agree that the code structure is a bit messy, part of my
> reasoning for duplicating rather than calling into existing code was
> so that all of the priviliged stuff would be in one place and easy to
> audit. With calls to dlog(), set_interface_var(), that's no longer
> the case, and we'll have to remember that those are security-related
> when editing them in the future.
Oops -- thanks for noticing that. I had made some changes to device.c
in the working directory and that didn't get applied..
I'll add a remark to the code to remind of that.
Attached is the patch of what I have on top of 1.1rc3.
--
Pekka Savola "You each name yourselves king, yet the
Netcore Oy kingdom bleeds."
Systems. Networks. Security. -- George R.R. Martin: A Clash of Kings
-------------- next part --------------
Index: device-linux.c
===================================================================
RCS file: /work/cvsroot/radvd/device-linux.c,v
retrieving revision 1.24
diff -u -r1.24 device-linux.c
--- device-linux.c 24 Jan 2008 10:10:18 -0000 1.24
+++ device-linux.c 24 Jan 2008 17:07:25 -0000
@@ -226,6 +226,7 @@
return(0);
}
+/* note: also called from the root context */
int
set_interface_var(const char *iface,
const char *var, const char *name,
Index: log.c
===================================================================
RCS file: /work/cvsroot/radvd/log.c,v
retrieving revision 1.10
diff -u -r1.10 log.c
--- log.c 24 Jan 2008 05:38:47 -0000 1.10
+++ log.c 24 Jan 2008 17:07:25 -0000
@@ -65,6 +65,7 @@
return 0;
}
+/* note: [dfv]log() is also called from root context */
static int
vlog(int prio, char *format, va_list ap)
{
Index: privsep-linux.c
===================================================================
RCS file: /work/cvsroot/radvd/privsep-linux.c,v
retrieving revision 1.2
diff -u -r1.2 privsep-linux.c
--- privsep-linux.c 24 Jan 2008 10:10:18 -0000 1.2
+++ privsep-linux.c 24 Jan 2008 17:07:26 -0000
@@ -65,34 +65,41 @@
switch(cmd.type) {
case SET_INTERFACE_LINKMTU:
- if (cmd.val < MIN_AdvLinkMTU || cmd.val > MAX_AdvLinkMTU)
+ if (cmd.val < MIN_AdvLinkMTU || cmd.val > MAX_AdvLinkMTU) {
+ flog(LOG_ERR, "(privsep) %s: LinkMTU (%u) is not within the defined bounds, ignoring", cmd.iface, cmd.val);
break;
- ret = privsep_set(cmd.iface, PROC_SYS_IP6_LINKMTU, cmd.val);
- dlog(LOG_DEBUG, 4, "privsep: set link %s mtu to %u", cmd.iface, cmd.val);
+ }
+ ret = set_interface_var(cmd.iface, PROC_SYS_IP6_LINKMTU, "LinkMTU", cmd.val);
break;
case SET_INTERFACE_CURHLIM:
- if (cmd.val < MIN_AdvCurHopLimit || cmd.val > MAX_AdvCurHopLimit)
+ if (cmd.val < MIN_AdvCurHopLimit || cmd.val > MAX_AdvCurHopLimit) {
+ flog(LOG_ERR, "(privsep) %s: CurHopLimit (%u) is not within the defined bounds, ignoring", cmd.iface, cmd.val);
break;
- ret = privsep_set(cmd.iface, PROC_SYS_IP6_CURHLIM, cmd.val);
+ }
+ ret = set_interface_var(cmd.iface, PROC_SYS_IP6_CURHLIM, "CurHopLimit", cmd.val);
break;
case SET_INTERFACE_REACHTIME:
- if (cmd.val < MIN_AdvReachableTime || cmd.val > MAX_AdvReachableTime)
+ if (cmd.val < MIN_AdvReachableTime || cmd.val > MAX_AdvReachableTime) {
+ flog(LOG_ERR, "(privsep) %s: BaseReachableTimer (%u) is not within the defined bounds, ignoring", cmd.iface, cmd.val);
break;
- ret = privsep_set(cmd.iface, PROC_SYS_IP6_BASEREACHTIME_MS, cmd.val);
+ }
+ ret = set_interface_var(cmd.iface, PROC_SYS_IP6_BASEREACHTIME_MS, "BaseReachableTimer (ms)", cmd.val);
if (ret == 0)
break;
- privsep_set(cmd.iface, PROC_SYS_IP6_BASEREACHTIME, cmd.val / 1000); /* sec */
+ set_interface_var(cmd.iface, PROC_SYS_IP6_BASEREACHTIME, "BaseReachableTimer", cmd.val / 1000);
break;
case SET_INTERFACE_RETRANSTIMER:
- if (cmd.val < MIN_AdvRetransTimer || cmd.val > MAX_AdvRetransTimer)
+ if (cmd.val < MIN_AdvRetransTimer || cmd.val > MAX_AdvRetransTimer) {
+ flog(LOG_ERR, "(privsep) %s: RetransTimer (%u) is not within the defined bounds, ignoring", cmd.iface, cmd.val);
break;
- ret = privsep_set(cmd.iface, PROC_SYS_IP6_RETRANSTIMER_MS, cmd.val);
+ }
+ ret = set_interface_var(cmd.iface, PROC_SYS_IP6_RETRANSTIMER_MS, "RetransTimer (ms)", cmd.val);
if (ret == 0)
break;
- privsep_set(cmd.iface, PROC_SYS_IP6_RETRANSTIMER, cmd.val / 1000 * USER_HZ); /* XXX user_hz */
+ set_interface_var(cmd.iface, PROC_SYS_IP6_RETRANSTIMER, "RetransTimer", cmd.val / 1000 * USER_HZ); /* XXX user_hz */
break;
default:
@@ -102,27 +109,6 @@
}
}
-int
-privsep_set(const char *iface, const char *var, uint32_t val)
-{
- FILE *fp;
- char spath[64+IFNAMSIZ]; /* XXX: magic constant */
-
- if (snprintf(spath, sizeof(spath), var, iface) >= sizeof(spath))
- return (-1);
-
- if (access(spath, F_OK) != 0)
- return -1;
-
- fp = fopen(spath, "w");
- if (!fp)
- return -1;
-
- fprintf(fp, "%u", val);
- fclose(fp);
- return 0;
-}
-
/* Return 1 if privsep is currently enabled */
int
privsep_enabled(void)
@@ -143,13 +129,13 @@
return 0;
if (pipe(pipefds) != 0) {
- flog(LOG_ERR, "Couldn't create privsep pipe\n");
+ flog(LOG_ERR, "Couldn't create privsep pipe.");
return (-1);
}
pid = fork();
if (pid == -1) {
- flog(LOG_ERR, "Couldn't fork for privsep\n");
+ flog(LOG_ERR, "Couldn't fork for privsep.");
return (-1);
}
@@ -169,7 +155,9 @@
}
dup2(nullfd, 0);
dup2(nullfd, 1);
- dup2(nullfd, 2);
+ /* XXX: we'll keep stderr open in debug mode for better logging */
+ if (get_debuglevel() == 0)
+ dup2(nullfd, 2);
privsep_read_loop();
close(pfd);
More information about the radvd-devel-l
mailing list