Page MenuHomeFreeBSD

Fix for ports/208534
Needs RevisionPublic

Authored by dteske on Apr 5 2016, 4:42 PM.

Details

Summary

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

Test Plan

Execute "service -R" (warning, restarts all running services)

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 3445
Build 3484: arc lint + arc unit

Event Timeline

dteske updated this revision to Diff 14901.Apr 5 2016, 4:42 PM
dteske retitled this revision from to Fix for ports/208534.
dteske updated this object.
dteske edited the test plan for this revision. (Show Details)
dteske added reviewers: allanjude, lme, mandree.
mandree requested changes to this revision.Apr 12 2016, 6:40 AM
mandree edited edge metadata.

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...)

This revision now requires changes to proceed.Apr 12 2016, 6:40 AM

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 ...

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").

dteske updated this revision to Diff 15206.Apr 14 2016, 11:04 PM
dteske edited edge metadata.

Improved solution

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).

jilles requested changes to this revision.Apr 24 2016, 10:42 PM
jilles added a reviewer: jilles.
jilles added a subscriber: jilles.

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.

This revision now requires changes to proceed.Apr 24 2016, 10:42 PM

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}" ]
_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).

dteske added inline comments.Apr 25 2016, 10:02 PM
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.

dteske updated this revision to Diff 15597.Apr 25 2016, 10:04 PM
dteske edited edge metadata.

Incorporate jilles feedback

dteske marked 9 inline comments as done.Apr 25 2016, 10:06 PM

Given last update, mark most recently completed feedback-items as completed.

jilles added inline comments.Apr 29 2016, 7:54 PM
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).

dteske added inline comments.Apr 29 2016, 10:45 PM
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
/etc/rc*)

  1. during boot (shutdown) $0 is /etc/rc (/etc/rc.shutdown),
  2. so get the name of the script from $_file name="$_file" ;;

*)

$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.

jilles added inline comments.Apr 30 2016, 9:22 PM
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.

I'm a little unclear as to whether you (jilles) give approval to move forward

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.

lme resigned from this revision.May 30 2017, 3:03 PM
This revision now requires changes to proceed.May 30 2017, 3:03 PM