radius.log permissions issue

[Philip Molter]

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