Search / View

[sig11]

I applied and tested it. Thanks a lot ! It is in the upcoming version !

I made a few changes and since this forum isn't too public, I think I can post a bit of code in here ...
- I moved the unlinking code into unix_main.c in Sys_Exit - that way all kinds of exits will remove the PID file
- The unlink code uses Cvar_VariableStringBuffer("sv_pidfile", pidfile, sizeof(pidfile)); to get the pid file name (Cvar_Get also inits the Cvar if it does not exist)
- The code creating the PID file uses c_pid = Cvar_Get ("sv_pidfile", va("/var/run/%s.pid", basename(argv[0])) , CVAR_INIT); i.e. basename and CVAR_INIT. The first should be obvious ... the second makes the cvar read-only, which is a good idea ...

Just FYI, a modified patch for this part is here (EDit: @Smoking Guns: don't collect this path - it will be part of the bigger one for 0.4)

I probably won't change things for chroot/setuid stuff. In these cases, the user should know what he's doing and will have to take care about a number of other things, too, anyways

[hika]

- The code creating the PID file uses c_pid = Cvar_Get ("sv_pidfile", va("/var/run/%s.pid", basename(argv[0])) , CVAR_INIT); i.e. basename and CVAR_INIT

Oh yeah, basename() is really mandatory here, since argv[0] can be a full path or relative path. What a shame I have missed this

Just FYI, a modified patch for this part is here

Thanks, I will apply your patch too.

I probably won't change things for chroot/setuid stuff

This is something I could code, next.

[sig11]

This is something I could code, next.

Well, as I said, I'm half way there. this is what I have so far. However, this screws up the pk3 search path setup which is done before the chroot call. With a carefully populated (only the WQ3 specific files, no libraries or the like !) chroot directory (including a place to write the PID file to ...) it still works for testing, though.

Either way, I somehow feel, it's a bit late in the code. I really would want to do this much earlier in the code, but if it is done with cvar's it has to wait until the cvar system is initialized ... ... I could go back to good old flags (Q3 does have the -v flag, btw ...) ... I still have not decided, yet.

[hika]

Either way, I somehow feel, it's a bit late in the code. I really would want to do this much earlier in the code, but if it is done with cvar's it has to wait until the cvar system is initialized ...

I don't know if my following proposition is acceptable :
About the cvars, what if we use ones read from command line ?
In that way, some cvars like sv_pidfile, sv_user, sv_chroot can be read, before any file access (exec default.cfg for example).
The problem is that it means to move the Com_ParseCommandLine() from Com_Init() to main() .
Either way, it is just an idea.

[hika]

I have tested your code that manage chroot / setuid and it works quite well !
As you have mentioned, it is a bit late in the code because of the search path done before the chroot / setuid.
This is the standard output I have, with the following command :

Code: Select all

ioq3ded +set fs_game westernq3 +set dedicated 1 +set net_port 27961 +set sv_strictAuth 0 \
 +set sv_user quake3 +set sv_chroot /home/quake3 \
 +set com_hunkmeg 25 +sets gamestartup \"`date +"%D %T"`\" \
 +set sv_pidfile /pid/q3ded.pid +exec server.cfg


Code: Select all

UID 0 EUID 0
ioQ3 1.34-rc1 Custom Engine freebsd-i386 Sep  2 2006
----- FS_Startup -----
Current search path:
/root/.q3a/westernq3
/usr/local/share/quake3/westernq3
./westernq3
/root/.q3a/baseq3
/usr/local/share/quake3/baseq3/pak8.pk3 (9 files)
/usr/local/share/quake3/baseq3/pak7.pk3 (4 files)
/usr/local/share/quake3/baseq3/pak6.pk3 (64 files)
/usr/local/share/quake3/baseq3/pak5.pk3 (7 files)
/usr/local/share/quake3/baseq3/pak4.pk3 (272 files)
/usr/local/share/quake3/baseq3/pak3.pk3 (4 files)
/usr/local/share/quake3/baseq3/pak2.pk3 (148 files)
/usr/local/share/quake3/baseq3/pak1.pk3 (26 files)
/usr/local/share/quake3/baseq3/pak0.pk3 (3539 files)
/usr/local/share/quake3/baseq3
./baseq3

----------------------
4073 files in pk3 files
execing default.cfg
couldn't exec q3config.cfg
couldn't exec autoexec.cfg
Hunk_Clear: reset the hunk ok
--- Common Initialization Complete ---
Chrooted to /home/quake3
Switched to user quake3
Opening IP socket: localhost:27961
Started tty console (use +set ttycon 0 to disable)
Cannot open /pid/q3ded.pid for write : Permission denied
couldn't exec server.cfg
Hitch warning: 845 msec frame time

The problem is that the Q3 filesystem is initialized before chroot AND with the root's home.
It seems that the important point is to chroot / setuid before the FS_InitFilesystem() call.
I think I will try to move things in the source code, in the risk to break things.

[sig11]

It seems that the important point is to chroot / setuid before the FS_InitFilesystem() call.
I think I will try to move things in the source code, in the risk to break things.

My latest attempt is to call the lock in qcommon/common.c

Code: Select all

        // override anything from the config files with command line args
        Com_StartupVariable( NULL );

#ifdef DEDICATED
        //Now we can lock
        Sys_LockMyself();
#endif

        // get the developer cvar set as early as possible
        Com_StartupVariable( "developer" );



And a corresponding definition in qcommon/qcommon.h

However, to be absolutely clean, then the function Sys_LockMyself(); probably should move into uni_shared.c so that MacOSX picks it up, too, and there should be a dummy function for Windows, too.

For some reason, the cvars are not fully accessible before Com_StartupVariable( NULL ); but I haven't looked through this function yet.

Furthermore, I would not move anything out of Com_Init and into main. First, the corresponding moves have to be done for Mac and Win, too. And second, only God knows (or Carmack ...) what side effects that has.

[hika]

My latest attempt is to call the lock in qcommon/common.c

Code: Select all

        // override anything from the config files with command line args
        Com_StartupVariable( NULL );

#ifdef DEDICATED
        //Now we can lock
        Sys_LockMyself();
#endif

        // get the developer cvar set as early as possible
        Com_StartupVariable( "developer" );



[...snip...]
there should be a dummy function for Windows, too.

I have a way to avoid defining a dummy function for Windows and tweaking the common.c since we only do stuff for *NIX system.
In unix_main.c, we can call Sys_LockMyself() before Com_Init() and before that, creating sv_chroot and sv_user cvars from argc, argv.

For some reason, the cvars are not fully accessible before Com_StartupVariable( NULL ); but I haven't looked through this function yet.

Yeah, Com_StartupVariable() created cvar from command line and must be called after Com_ParseCommandLine()

Furthermore, I would not move anything out of Com_Init and into main.

I agree. It would be a real mess

Anyway, I will post some patches when I will have something acceptable and safe.

[hika]

OK, I have some small patches to manage chroot and setuid correctly IMO.
You can find it at the following URL http://wq3.bsdmon.com/stuff/patch-engine.diff
Again, it is generated from the original q3 source code engine.

Here are some comments about how I have done this :
1) I have moved Sys_LockMyself() from unix_main.c to unix_shared.c, and have slightly modified the function to handle the HOME env as it is needed by Sys_DefaultHomePath().
I have also changed its definition, since cvar cannot be initialized before Com_Init().
2) I have added Sys_LockMyself() definition in ./code/qcommon/qcommon.h but only for Linux and FreeBSD at the moment.
3) In unix_main.c, I have added some code to retrieve special startup cvar, I mean sv_chroot and sv_user. Then Sys_LockMyself() can be called before Com_Init().
4) A special bonus as I needed it : the possibility to daemonize the q3ded
But you can ignore this one for whatever reasons ...

I guess I will update my test server engine, soon but this time, I will use IOQ3 instead of CleanQ3.
It works quite well in my home test server.

[sig11]

That even makes sense - at least if we consider the alternative of adding regular options. Because it really only is a syntax difference if the options are "-u user" or "+set sv_user user" - the code is more or less the same.

I will test it in the coming days. One thing that comes to mind: what happens when you SIGHUP a chrooted server ?

[hika]

One thing that comes to mind: what happens when you SIGHUP a chrooted server ?

I have tested this feature too with chroot / setuid. No problem about that, since I manage to setenv("HOME") after the setuid.
Then, the search path should match the filesystem.
In my case, I have chroot the server to the user's home directory, say /home/quake3 .
All necessary files are in /home/quake3/.q3a
Then I have done the following :

Code: Select all

$ mkdir /home/quake3/home/quake3
$ ln -s /.q3a /home/quake3/home/quake3/.q3a


In that way, in the chroot environment, /home/quake3/.q3a will be accessible since /home/quake3/.q3a is a symlink to /.q3a
Now we can be proud of our more secure Q3 server

EDIT: BTW, I have update my test server 's engine

[sig11]

During the last few days I merged your patch into the current version. I still was moving around parts of the code since the standalone version has a slightly different FS setup. Also, I restrained the patch to unix only (i.e. no qcommon/ files are touched). My test server is running chrooted and as a low privileged user now (all of mine do the latter, but up until now I used "su" ...).

And now it's commited to the last development version ! Thanks again !

Just for the archives: one may need a working /etc/resolv.conf inside the chroot jail to be able to report to ID's master server.