Fix Bug 208534 - security/openvpn rc.d script breaks "service -R"
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=208534
The openvpn rc.d script expects `name' to be set already
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 3276 Build 3310: arc lint + arc unit
Event Timeline
Comment Actions
It appears that this does not fully solve the problem.
Compared to https://reviews.freebsd.org/D5833, this revision (D5846) still lacks robustness in the load_rc_config_var:
If ${name} happens to come up empty from the eval grep ..., then load_rc_config_var will still see only one argument and barf, making service -R abort.
I wonder if the real action (the entre block inside the respective for file... loops) needs to be in a subshell so that service always attempts to process all affected files even if that fails, so as not to lock the admin out if he is using remote access and service aborts before restarting a critical service (sshd seems safe these days in leaving existing sessions alone, but what about NFS, routing, VPN, DNS...)
Comment Actions
The correct approach, in my honest/humble opinion, is to mirror the approach taken by sysrc(8).
https://svnweb.freebsd.org/base/head/usr.sbin/bsdconfig/share/sysrc.subr?r1=259054&r2=290693
See lines 252 through 271 (20 lines total). In those 20 lines, the technique for extracting the name is to source the rc.d script when appropriate, but do-so in an environment that replaces "load_rc_config" and "run_rc_command").
Comment Actions
I have improved my solution to be more robust. This solution solves the openvpn issue by:
a. Setting $0 to the script path (the logic in the openvpn script that interrogates $0 therefore operates properly)
b. Source the entire script so that the script logic can execute (allowing shell logic to operate properly)
NB: This is safe for three reasons...
b.1. We only do this when we find that the script ``looks like an rc.d script'' (it must contain a line that begins with "rcvar"; retained logic from versions-past)
b.2. The context in which the script is sourced has any/all instances of ``run_rc_command'' replaced with ":" (effectively neutering any ability to interpret the action item).
b.3. The stdout and stderr of the script are obliterated; only variables of interest make it through file-descriptor 9 which is created for sending statements to the parent `eval' running in the parent namespace.
c. Even if the variable name' ends up not being used, we will get the proper value because the script is run in a context that has `load_rc_config'' replaced with a function that sets $name to the first argument (this is canonically correct; the first argument to load_rc_config() should be taken as the name as not all scripts set $name).
Comment Actions
I think the idea of sourcing the rc.d script is good but the implementation here has some issues.
| usr.sbin/service/service.sh | ||
|---|---|---|
| 50 | Invoking a new shell isolates the called script from service a bit more, but is probably slower than sourcing the script in a subshell environment like run_rc_script does. It also adds an additional level of quoting. | |
| 51 | Closing file descriptors 0, 1 or 2 may cause strange results because an unrelated file ends up on a "standard" fd number. The shell itself is unlikely to be affected because it moves internal file descriptors to number 10 or higher but programs called from the shell may be affected. Please open /dev/null instead to discard output. | |
| 52 | If word splitting and pathname generation are not needed (which seems to be the case here), for var do would both be shorter and avoid unnecessary operations. | |
| 53 | This will work for name and rcvar which contain few special characters but will not work in the general case where variable values may contain characters like ", $ and \. Fixing that leads to uglier code, so it may not be appropriate. | |
| 58 | When running this in a subshell environment, we could rely on the fact that /etc/rc.subr has already been sourced by redefining the two functions and sourcing the script as is. This may be ugly though. | |
Comment Actions
I believe all of the nits you pointed out should be easily solvable and I'll provide an update this week for you. Cheers!
| usr.sbin/service/service.sh | ||
|---|---|---|
| 50 | Has to be done; How else are you going to set $0 for the script? E.g., "pkg install -y openvpn; vi /usr/local/etc/rc.d/openvpn" (notice importance of $0). Speed is sacrificed for accuracy (unless you know of a more performant way to set $0 for the current script without invoking a new shell). | |
| 51 | Fair enough; will do in next update | |
| 52 | Wow, didn't know about that feature. What's better? (a) for var do (b) for var; do | |
| 53 | It's trivial to fix and not that ugly in my honest opinion. I will fix it. That way people can copy/paste the function where need-be without running the risk of copying something that has such an edge-case. | |
| 58 | I might do just that so we can eliminate the use of awk. I note however, that the only reason this will even work is because is because /etc/rc.subr contains the following: if [ -z "${_rc_subr_loaded}" ]; then ...fi # [ -z "${_rc_subr_loaded}" ] If /etc/rc.subr didn't include that code, we wouldn't be able to redefine the functions, considering that 98.15% of system startup scripts (in /etc/rc.d) immediately source /etc/rc.subr at start (3 out of 162 do not: msgs, serial, and setup_mach). | |
| usr.sbin/service/service.sh | ||
|---|---|---|
| 52 | Actually, in my testing I've found that some rc.d scripts (e.g., /etc/rc.d/jail) call "shift" which alters the number of arguments before the EXIT trap fires. The next update will take this into consideration (continuing to use $* but doing-so in a fashion that causes the value to be expanded into the trap argument, meaning that the arguments to the function are codified before the rc.d script source has a chance to alter them. | |
| usr.sbin/service/service.sh | ||
|---|---|---|
| 50 | Oh right. The openvpn rc.d script has some hacks to find its name based on $0, $file and $_file. If it detects it is being run from service, it bases name on $file. This is ugly but a change like this will not break it. It is not possible to change $0 in an existing shell and there is also no way to get the pathname of a dot script (other than knowing the variable that contains it). | |
| usr.sbin/service/service.sh | ||
|---|---|---|
| 50 | I'm looking at the following file/revision: $FreeBSD: head/security/openvpn/files/openvpn.in 340872 2014-01-24 00:14:07Z mat $This is from installing openvpn-2.3.10 via "pkg install -y openvpn" on FreeBSD 10.1-STABLE amd64. In this version of the file, I can say that it only detects if it is running from /etc/rc and not from service. I don't know if it has since-been changed, but this is the snippet that I am working against: case "$0" in
*) $0 ;; esac The principle task of this review is to address PR ports/208534 which is on 10.3-RELEASE, so maybe later versions of the openvpn rc.d script detect service as well as /etc/rc, but on the release where I was trying to remediate the issue, the case is that the openvpn script only detects being run under /etc/rc as its parent. Given the above, would you agree that it's probably going to produce the most predictable results and least surprise if we go with the $0 based method (spawning a new shell)? It would certainly help for future cases if other scripts are lackadaisical about detecting when running under /etc/rc and/or service. | |
| usr.sbin/service/service.sh | ||
|---|---|---|
| 50 | I looked at the version in ports head/, $FreeBSD: head/security/openvpn/files/openvpn.in 412541 2016-04-05 02:17:40Z mandree $. This version has a special case for $0 matching */service. Perhaps such logic should be added to /etc/rc.subr, since I think rc.d scripts should not even check $0 for /etc/rc*. Apart from that, spawning a new shell will ease things for rc.d scripts a little at the cost of making service uglier and slower. | |
Comment Actions
I think it is best to use run_rc_script in service and to add a function to rc.subr that determines name from $0/$file/$_file. This will work with the current openvpn rc.d script (versions from older ports branches are not relevant). The function in rc.subr can be used from such time as that older service that grep for ^name= are obsolete.