radius.log permissions issue
With freeradius 2.1.6, I have a configuration such as this in my radiusd.conf file: user = radiusd group = radiusd When I start up radiusd for the first time, the radius.log file gets created with 0640 permissions, owned by root:radiusd, instead of radiusd:radiusd. This doesn't prevent the RADIUS process from working, but it does prevent any useful information from being logged. Is this a known bug? Is there a workaround other than creating the file by hand and setting its ownership before starting freeradius? Philip
Hi,
Is this a known bug? Is there a workaround other than creating the file by hand and setting its ownership before starting freeradius?
?? how are you starting this server - the file/directory should be radiusd:radiusd and when run it will do the 'correct thing' alan
On Jul 16, 2009, at 4:03 AM, A.L.M.Buxey@lboro.ac.uk wrote:
Hi,
Is this a known bug? Is there a workaround other than creating the file by hand and setting its ownership before starting freeradius?
?? how are you starting this server - the file/directory should be radiusd:radiusd and when run it will do the 'correct thing'
/usr/sbin/radiusd -d /etc/raddb as user root. As posted before, the config file has directives to switch to user radiusd and group radiusd The directory has the proper permissions, but the radius.log file doesn't exist. When the radiusd program starts up, it creates the radius.log file in the proper directory, but the file has 0640 permissions owned by user root, group radiusd. I know that it SHOULD BE radiusd:radiusd. It is not doing the "correct thing".
On 07/16/2009 08:12 AM, Philip Molter wrote:
On Jul 16, 2009, at 4:03 AM, A.L.M.Buxey@lboro.ac.uk wrote:
Hi,
Is this a known bug? Is there a workaround other than creating the file by hand and setting its ownership before starting freeradius?
?? how are you starting this server - the file/directory should be radiusd:radiusd and when run it will do the 'correct thing'
/usr/sbin/radiusd -d /etc/raddb as user root. As posted before, the config file has directives to switch to user radiusd and group radiusd
The directory has the proper permissions, but the radius.log file doesn't exist. When the radiusd program starts up, it creates the radius.log file in the proper directory, but the file has 0640 permissions owned by user root, group radiusd.
FWIW, in our RPM's we force the creation of the radius.log file with ownership radiusd:radiusd at installation time before the server even runs. If you don't force the creation of the file with the right ownership then I think the issue revolves around when a log message is first emitted. The log file gets created the first time a log message is emitted. The server starts as root. During it's initialization phase it raises and lowers it's operating permissions between the root and radiusd user identity via the fr_suid_up() and fr_suid_down() calls. When it gets ready to process events it settles down to radiusd via fr_suid_down_permanent(). If the first log message occurs when the server is in a fr_suid_up() mode (e.g. running as root instead of as radiusd) then you'll get the behavior you've seen. The code paths are way to complicated for static analysis to see if and when a log message might be emitted the server is in a high privilege mode. It does seem like it might happen if you start the server in debug mode because the server is much more verbose. 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). -- John Dennis <jdennis@redhat.com> Looking to carve out IT costs? www.redhat.com/carveoutcosts/
John Dennis wrote:
FWIW, in our RPM's we force the creation of the radius.log file with ownership radiusd:radiusd at installation time before the server even runs.
If you don't force the creation of the file with the right ownership then I think the issue revolves around when a log message is first emitted. The log file gets created the first time a log message is emitted. The server starts as root. During it's initialization phase it raises and lowers it's operating permissions between the root and radiusd user identity via the fr_suid_up() and fr_suid_down() calls. When it gets ready to process events it settles down to radiusd via fr_suid_down_permanent().
The problem is commit 047fe5ca74e3de2c7f32f98154d6655c0cfd7181. Before this commit, in switch_users(), permissions were unconditionally dropped if a user setting was specified, and the 'did_setuid' boolean was set no matter what if setuid capability was even possible (ie. even if a user name wasn't specified, did_setuid was set to true). After this commit, the permission drop was abstracted into fr_suid_down(), which checks did_setuid before it does anything. Since did_setuid isn't set, fr_suid_down() doesn't do anything. After that call, did_setuid is set to TRUE, so future calls to fr_suid_down() work as expected, but all of the time spent between the code there and the code in listen.c is run as root, including a check to see if the directory is writable that immediately follows setuid in switch_users(). Previous to that commit, that wasn't the behavior. Basically, that code is the problem. I'll try to submit a patch later today that fixes the problem. Yes, if an error occurs, there are log messages that get generated before suid operations, but as far as I can tell, they're related to fatal errors or debug messages.
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).
The latter is basically what happens, because in switch_users(), the daemon tries to make sure it can write to the file as the user it is. If the file exists, it's a simple append. If the file doesn't exist, it creates it. If it can't write, it bails. Like I said, it just isn't the user it thinks it is when this is called (mainconfig.c:629, version 2.1.6). Philip
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. */
Philip Molter wrote:
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
OK.
2) enables fr_suid_down/up/down_permanently noop calls so that compile works when HAVE_SETUID is not defined
That's needed, yes. I've committed a fix based on this that: a) does suid down earlier b) lets it build when HAVE_SETUID is not defined c) calls chown() on the log file to ensure it has the correct owner Alan DeKok.
Alan DeKok wrote:
Philip Molter wrote:
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
OK.
2) enables fr_suid_down/up/down_permanently noop calls so that compile works when HAVE_SETUID is not defined
That's needed, yes.
I've committed a fix based on this that:
a) does suid down earlier b) lets it build when HAVE_SETUID is not defined c) calls chown() on the log file to ensure it has the correct owner
Thanks Alan. I'll point out the HAVE_SETUID ifdef used within the switch_users() function is redundant. The entire function is wrapped in HAVE_SETUID. Philip
participants (4)
-
A.L.M.Buxey＠lboro.ac.uk -
Alan DeKok -
John Dennis -
Philip Molter