John Dennis wrote:
There are various strategies to assure the newly created log file has the right ownership:
* drop privileges prior to calling fopen() * call chown() after fclose() at the exit of the logging call. * pre-create the file if necessary very early during start up.
I think the latter is preferable as it avoid the expense of setting or checking for the right ownership for every log message emitted (ouch).
Attached is a patch that fixes the issue. Given the way that freeradius checks for the ability to write to the logfile, it should perform like the latter (in my testing, it does exactly that). The patch does a couple of things: 1) properly handles setuid changes in early configuration times 2) enables fr_suid_down/up/down_permanently noop calls so that compile works when HAVE_SETUID is not defined Philip diff -urNp a/src/main/mainconfig.c b/src/main/mainconfig.c --- a/src/main/mainconfig.c 2009-05-18 06:13:55.000000000 -0500 +++ b/src/main/mainconfig.c 2009-07-16 10:39:34.000000000 -0500 @@ -78,7 +78,7 @@ static cached_config_t *cs_cache = NULL; /* * Systems that have set/getresuid also have setuid. */ -uid_t server_uid; +static uid_t server_uid; static gid_t server_gid; static const char *uid_name = NULL; static const char *gid_name = NULL; @@ -413,9 +413,9 @@ static int r_mkdir(const char *part) #ifdef HAVE_SETUID -int did_setuid = FALSE; +static int has_setuid = FALSE; -#if defined(HAVE_SETRESUID) && defined (HAVE_GETRESUID) +#if defined(HAVE_SETRESUID) && defined(HAVE_GETRESUID) void fr_suid_up(void) { uid_t ruid, euid, suid; @@ -438,7 +438,7 @@ void fr_suid_up(void) void fr_suid_down(void) { - if (!did_setuid) return; + if (!has_setuid) return; if (setresuid(-1, server_uid, geteuid()) < 0) { fprintf(stderr, "%s: Failed switching to uid %s: %s\n", @@ -457,12 +457,7 @@ void fr_suid_down_permanent(void) { uid_t ruid, euid, suid; - if (!did_setuid) return; - - if (getresuid(&ruid, &euid, &suid) < 0) { - radlog(L_ERR, "Failed getting saved uid's"); - _exit(1); - } + if (!has_setuid) return; if (setresuid(server_uid, server_uid, server_uid) < 0) { radlog(L_ERR, "Failed in permanent switch to uid %s: %s", @@ -474,13 +469,6 @@ void fr_suid_down_permanent(void) radlog(L_ERR, "Switched to unknown uid"); _exit(1); } - - - if (getresuid(&ruid, &euid, &suid) < 0) { - radlog(L_ERR, "Failed getting saved uid's: %s", - strerror(errno)); - _exit(1); - } } #else /* @@ -491,7 +479,7 @@ void fr_suid_up(void) } void fr_suid_down(void) { - if (!uid_name) return; + if (!has_setuid) return; if (setuid(server_uid) < 0) { fprintf(stderr, "%s: Failed switching to uid %s: %s\n", @@ -502,8 +490,20 @@ void fr_suid_down(void) void fr_suid_down_permanent(void) { } -#endif +#endif /* HAVE_SETRESUID && HAVE_GETRESUID */ +#else +void fr_suid_up(void) +{ +} +void fr_suid_down(void) +{ +} +void fr_suid_down_permanent(void) +{ +} +#endif /* HAVE_SETUID */ +#ifdef HAVE_SETUID /* * Do chroot, if requested. * @@ -609,13 +609,8 @@ static int switch_users(CONF_SECTION *cs #ifdef HAVE_PWD_H if (uid_name) { + has_setuid = TRUE; fr_suid_down(); - - /* - * Now core dumps are disabled on most secure systems. - */ - - did_setuid = TRUE; } #endif @@ -657,7 +652,7 @@ static int switch_users(CONF_SECTION *cs * Otherwise, disable core dumps for security. * */ - if (!(debug_flag || allow_core_dumps || did_setuid)) { + if (!(debug_flag || allow_core_dumps || has_setuid)) { #ifdef HAVE_SYS_RESOURCE_H struct rlimit no_core; @@ -676,7 +671,7 @@ static int switch_users(CONF_SECTION *cs * running as a daemon, AND core dumps are * allowed, AND we changed UID's. */ - } else if ((debug_flag == 0) && allow_core_dumps && did_setuid) { + } else if ((debug_flag == 0) && allow_core_dumps && has_setuid) { /* * Set the dumpable flag. */