Bug 235185 - /etc/rc.d/ environment may need to be cleaned vs services(8)
Summary: /etc/rc.d/ environment may need to be cleaned vs services(8)
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: conf (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Many People
Assignee: Rodney W. Grimes
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-01-25 02:06 UTC by Victor Sudakov
Modified: 2019-02-18 16:46 UTC (History)
8 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Victor Sudakov 2019-01-25 02:06:37 UTC
It is desirable to clean the environment in /usr/local/etc/rc.d/fcgiwrap before actually starting the fcgiwrap daemon. Otherwise, when manually starting/restarting the service from the root account, the whole root's environment is leaked to CGI scripts. I think it can be even considered a security issue.

How to reproduce: write a CGI shell script with "printenv" inside, run "/usr/local/etc/rc.d/fcgiwrap restart" as root, watch the CGI script's output from the Web server.

Workarond: always remember to use "env -i /usr/local/etc/rc.d/fcgiwrap start" when (re)starting manually.
Comment 1 Kyle Evans freebsd_committer 2019-01-25 03:25:08 UTC
(In reply to vas from comment #0)

CC'ing Devin; Devin, what do you think about this as a feature for the rc framework? It seems like we'd want to make sure the environment is sanitized to some extent by default, but I'm not sure exactly what that looks like -- /usr/bin/env isn't necessarily available here, from what I recall.
Comment 2 Rodney W. Grimes freebsd_committer 2019-01-25 04:01:13 UTC
(In reply to vas from comment #0)
While it is bad that fcgiwrap is leaking its environment, it is NOT the responsibility of the rc system to prevent a package from doing that.  Any program started by service or rc.d that exposes its environment data to another place should be classified as a data exfiltration bug and fixed asap.

Most daemons already deal with this, and so should this one.
Comment 3 Jamie Landeg-Jones 2019-01-25 04:12:49 UTC
I personally don't want to trust third party products like that:

For years, I've wrapped "service" to clean the env, close files, set login name to root, and anything else I can do to restart a service with as close an environment to bootup rc as possible.

I also enforce 'daemon' execution rather than rely on the third party product to do so itself.

I'd have thought the less superfluous stuff you give someone elses program at startup, the better!

I also wrap make in a similar way for port building, due to problems with my environment in the past.

But hey, not disagreeing, just putting out another perspective!
Comment 4 Rodney W. Grimes freebsd_committer 2019-01-25 04:20:22 UTC
(In reply to Jamie Landeg-Jones from comment #3)
We do need to look at the rc.d code, as I think this statement in the services(8) man page is not actually true:
    When used for this purpose it will set the same
    restricted environment that is in use at boot time (see below)
    (below)
    ENVIRONMENT
     When used to run rc.d scripts the service command sets HOME to / and PATH
     to /sbin:/bin:/usr/sbin:/usr/bin which is how they are set in /etc/rc at
     boot time.

I think it comes rather short of getting back to the "boot time restricted environment" in many respects, in that it appears to only set the PATH and HOME variables.
Comment 5 Rodney W. Grimes freebsd_committer 2019-01-25 04:30:03 UTC
(In reply to vas from comment #0)
Workarond: always remember to use "env -i /usr/local/etc/rc.d/fcgiwrap start" when (re)starting manually.

I would support adding that to the fcgiwrap script right before the program is invoked.
Comment 6 Victor Sudakov 2019-01-25 06:55:35 UTC
(In reply to Rodney W. Grimes from comment #4)

> When used to run rc.d scripts the service command sets HOME 

I started it as "/usr/local/etc/rc.d/fcgiwrap start", not as "service fcgiwrap start", could that be the reason? Are both variants the same?
Comment 7 Victor Sudakov 2019-01-25 06:57:53 UTC
(In reply to Rodney W. Grimes from comment #2)
> it is NOT the responsibility of the rc system to prevent a package
> from [leaking the environment]

What about other rc systems? For example Solaris' svcs or Linux's systemd, do they sanitize the environment?
Comment 8 Rodney W. Grimes freebsd_committer 2019-01-25 11:34:57 UTC
(In reply to vas from comment #6)
I do not think "at present" that has any effect, as I can not find any place that service(8) actually does sanatize the environment, but I may of missed it in my 3 minute scan of that /bin/sh script.

Either way, I do now fully support that the package specific rc.d/fcgiwrap script should do a env -i when it invokes this binary due to its potential for being a exfiltration point.

Do note that the author of this program is aware of the fact that it can expose its environment and actually has an internal blacklist of env variables, so to me it appears as if the exporting is by design and intentional and the novice user running printenv in a cgi script started by this program has loaded the gun and pulled the trigger.

(In reply to vas from comment #7)
Realize that if you sanitize the environment in a generic way in the "foo" init system you remove the ability of the system admin to use the environment to effect anything that is started, and that would probably be a larger and painful problem to solve.
Comment 9 Victor Sudakov 2019-01-25 14:03:36 UTC
(In reply to Rodney W. Grimes from comment #8)

> Realize that if you sanitize the environment in a generic way in the "foo" init system you remove the ability of the system admin to use the environment to effect anything that is started,

I politely disagree. A well designed init system should have a controlled way for the system admin to pass only the intended environment variables to anything that is started, and not just blindly inherit whatever there is in the admin's shell.
Comment 10 Rodney W. Grimes freebsd_committer 2019-01-25 14:10:50 UTC
(In reply to vas from comment #9)
So you would need to have a filter list for each thing that is going to be started, can you say nightmare to maintain?
Comment 11 Victor Sudakov 2019-01-25 14:21:16 UTC
(In reply to Rodney W. Grimes from comment #8)
> Do note that the author of this program is aware of the fact that it can expose its environment 

Sure, for fcgiwrap it is a feature, no doubt. For the init system, it is an information leak. Do you see the difference?
Comment 12 Victor Sudakov 2019-01-25 14:36:18 UTC
(In reply to Rodney W. Grimes from comment #10)
> So you would need to have a filter list for each thing that is going to be started, can you say nightmare to maintain?

A reasonable policy would be "no environment variable is passed by default, those to be passed must be specified explicitly by the admin". I don't see it as a nightmare.
Comment 13 Rodrigo Osorio freebsd_committer 2019-01-25 15:27:32 UTC
Two questions:

- Are we supposed to start a service 'manually' instead of using the dedicate service binary (I ask that because when started with service, only the original user env is exposed, not the variables used during the session).

- Are we allowed to change the way fastcgi behaves, if we think it can breaks hundred of installed servers ? I mean, introducing a way to sanitize the environment is a good idea as long as it's optional and don't break everyone cgi tools.

My two cents.
Comment 14 Victor Sudakov 2019-01-25 16:02:28 UTC
(In reply to Rodrigo Osorio from comment #13)

> Are we allowed to change the way fastcgi behaves, if we think it can breaks hundred of installed servers ? 

If those servers work fine after a reboot (with a minimail environment available at system startup) this change is not likely to break them.

> Are we supposed to start a service 'manually' instead of using the dedicate service binary 

A good question. The /usr/sbin/service script contains the line:

exec env -i HOME=/ PATH=/sbin:/bin:/usr/sbin:/usr/bin $dir/$script $*

so it probably does make a difference using a) the dedicated service command or b) startup scripts directly. I don't know if this inconsistency is a bug, or a feature.
Comment 15 Devin Teske freebsd_committer 2019-01-25 16:27:26 UTC
(In reply to Kyle Evans from comment #1)
Hi Kyle,

Thanks for looping me in.

I've read through the responses and here is my take:

1. If the sanitization is in rc.d/fcgiwrap then you have to magically know that it cleans its env and that would be why attempts to affect its runtime environment will/would fail. Annoyance forecasted.

2. If the sanitization is in service but the rc.d/fcgiwrap script opts-in to a feature provided by the init system, again the admin has to magically know that it (fcgiwrap) cleans its env. Again, annoyance forecasted.

3. If perhaps instead the init system provided a mechanism for achieving what the OP wants without hiding the setting inside the rc.d script itself, then we'll avoid the above annoyances.

So here's the idea I arrive at:

a. As there is a generic *_enable=YES in rc.conf to enable a service, what if we grew a *_noenv (name up for debate; not married to the name)

b. Any service can benefit from this

c. The admin, faced with rc.conf settings, ought to know if/when a service will refuse any changes from, say, login.conf

This way we can retain the ability to modify login.conf (and subsequently run cap_mkdb) to affect the environment of the user that a particular service runs-as, without ever running into the situation where you find that a port rc.d script version A did not sanitize but version B does (which would cause fits of rage, I am sure). This puts the power in the hands of the sysadmin, keeps it there, and centralizes it to places that sysadmins are known to inhabit (rc.conf, login.conf, etc.).
Comment 16 Rodrigo Osorio freebsd_committer 2019-01-25 16:50:03 UTC
(In reply to Devin Teske from comment #15)

Sounds tempting. And what about having a sanitized environment (here we can probably take inspiration from sudo) to define a minimal set of safe variables.

That way the env variable can take 3 values yes, no, and minimal.
Comment 17 Rodney W. Grimes freebsd_committer 2019-01-25 18:55:38 UTC
(In reply to Devin Teske from comment #15)
This idea is appladable, but what is the default value of this knob?

If it is yes so that the environment is sanitized satisfying vas@'s desires it would be a POLA violation for anyone who has been using environment variables to effect things started by rc.d scripts.

If it is no, leaving the system function as is so no POLA or breakage it would not achieve what vas@ is asking for.

And in either case one would not likely find this subtle knob addition that effects this change for what is now looking to be a small edge case of sloppy admins that work as root with poluted ENV invoking daemon starting scripts directly rather than using the services wrap (which someone did find to be doing the sanitization asked for and hence I now deam the correct solution to this bug report, no change needed.)

All that being said, I would in no way object to:
a)  Adding a env -i to the rc.d/fcgiwrap start script AND submitting a report to the author asking that he clean up its act

b)  Adding a knob to /etc/defaults/rc.conf that does Devin's global type env -i to the rc.d system with a default value of off

c)  Documenting in services.8 more clearly that:
    a)  It really does do a full revoke with only PATH and HOME exported from the environment it seems a bit unclear as it is written today.
    b)  That directly invoking a rc.d/script may or may not have this cleaning done depnding on the new knob in b).
Comment 18 Devin Teske freebsd_committer 2019-01-25 19:06:59 UTC
(In reply to Rodney W. Grimes from comment #17)
There exists a case where "sloppy" may not apply.

Legacy jails may often have the following in login.conf:

default:\
        ...\
       :setenv=MAIL=/var/mail/$,BLOCKSIZE=K,FTP_PASSIVE_MODE=YES,PACKAGESITE=ftp\c//ftp-archive.freebsd.org/pub/FreeBSD-Archive/old-releases/amd64/9.2-RELEASE/packages/Latest/:\
        ...\

Which naturally sets $PACKAGESITE in the environment for all users.

In this case, you may want the environment variable set for all users that login, but you don't want it leaked to services for various reasons (in the OP's case, there may be nothing that can be done about enumerating the environment -- it may be a required setup -- but you don't want this variable to give away pertinent security-specific information that could facilitate hacking your machine by knowing which version of the OS is in-use).

The default value for the proposed new knob would be NO.
The knob would be opt-in only and on a per-service basis.
It would act as value-add on top of existing features like above.

As for your stated options (a, b, c list), I concur with that list.

I would add that as long as the rc.d script uses the rc.subr routines for starting services according to rc.conf settings (descriptive of the fcgiwrap rc.d script), then the new knob would be applied regardless of whether you use service or invoke the rc.d script manually.
Comment 19 Rodney W. Grimes freebsd_committer 2019-01-25 19:52:13 UTC
(In reply to vas from comment #11)
If you are calling this exporting of the environment a feature of fcgiwrap then us revoking the environment with env -i would be removing an authors inteded feature.  Do you see the problem I might have with that point now?  If the authors documentation should now or at some time in the future given an example of "printenv" as a way to expose the environment and we have crafted that feature away it creates a POLA issue.
Comment 20 Devin Teske freebsd_committer 2019-01-25 20:00:27 UTC
(In reply to Rodrigo Osorio from comment #16)
Thought about the yes/no/minimal idea and here's a spin I'd like your thoughts on:

YES = strict filter
NO = no filter
anything other than YES or NO = whitespace list of variables to filter
Comment 21 Devin Teske freebsd_committer 2019-01-25 20:01:18 UTC
(In reply to Devin Teske from comment #20)
Correction, "whitespace list" should have read "whitespace separated list"
Comment 22 Rodney W. Grimes freebsd_committer 2019-01-25 20:14:21 UTC
(In reply to Devin Teske from comment #20)
I like this, so long as that list is what to allow past the filter, and not an attempt to be what to remove from the environment.
Comment 23 Devin Teske freebsd_committer 2019-01-25 20:29:07 UTC
(In reply to Rodney W. Grimes from comment #22)

Very good point. I give that +1

Shall we give this 72h to simmer down and then we can start coding something for phabricator review?
Comment 24 Rodney W. Grimes freebsd_committer 2019-01-25 20:36:36 UTC
(In reply to Devin Teske from comment #23)

48 should be long enough, I think we have covered most things from enough different perspectives that we wont be broadsided by an oversight we did not consider, though 72 does leave this all sit until monday, which may solicit some additional input.  If your coding, it is purely up to you.
Comment 25 Devin Teske freebsd_committer 2019-01-25 20:43:51 UTC
(In reply to Rodney W. Grimes from comment #24)

I'll go ahead and do the coding. It's not entirely dissimilar from the ENV filtering code that I did for both sysrc and rc.subr/network.subr (see list_vars() in rc.subr and how it is used in ifalias_af_common() of network.subr for finding variables that match a particular pattern).

Now that I think about it, making that white-space separated list of variables a list of patterns would be pretty cool.

Also thinking about the variable name, I am thinking maybe *_env_blacklist="" (is that too long? it surely is sufficiently descriptive, but can we do better?)
Comment 26 Rodney W. Grimes freebsd_committer 2019-01-25 21:10:22 UTC
(In reply to Devin Teske from comment #25)
    "Also thinking about the variable name, I am thinking maybe *_env_blacklist="" (is that too long? it surely is sufficiently descriptive, but can we do better?)"

    *_envallow  It is not a blacklist, it is a whitelist.
Comment 27 Jilles Tjoelker freebsd_committer 2019-01-25 22:32:06 UTC
I think restarting fcgiwrap using the command "/usr/local/etc/rc.d/fcgiwrap restart" is wrong; the correct command is "service fcgiwrap restart". Our service(8) basically only sanitizes the environment and then starts the script but service(8) implementations of other init systems may send a message to a daemon telling it to restart the service (so any daemon started is not a descendant of the service(8) process).

I think writing code to fix "/usr/local/etc/rc.d/fcgiwrap restart" is inappropriate.
Comment 28 Devin Teske freebsd_committer 2019-01-25 22:54:29 UTC
(In reply to Jilles Tjoelker from comment #27)
Are you against a generic opt-in environment scrubber that would solve the OP's issue without a single modification to either service or fcgiwrap?

I was looking at the big picture (which encompasses more than fcgiwrap), but maybe you see a bigger picture.
Comment 29 Victor Sudakov 2019-01-26 05:44:19 UTC
(In reply to Jilles Tjoelker from comment #27)
> I think restarting fcgiwrap using the command "/usr/local/etc/rc.d/fcgiwrap restart" is wrong; the correct command is "service fcgiwrap restart".

If these two commands are not equal from the security (!) point of view there must be a big red flag in the handbook, and an appropriate warning when starting a rc.d script directly.
Comment 30 Victor Sudakov 2019-01-26 07:05:08 UTC
(In reply to Rodney W. Grimes from comment #19)
> If you are calling this exporting of the environment a feature of fcgiwrap then us revoking the environment with env -i would be removing an authors inteded feature.

We are already doing that, albeit in an inconsistent manner: we clean the environment in /usr/sbin/service and not clean when running rc.d scripts directly). Now that I think of it, I see the major problem being this inconsistency.

And guys, before you start coding, please remember that login.conf and other environment variable assignments probably should go _after_ the initial environment sanitation. So first you cut off whatever is inherited from the invoker's environment (like it is done in /usr/sbin/service) and then you assign the necessary variables in the rc.d script already.
Comment 31 Jilles Tjoelker freebsd_committer 2019-01-27 23:32:35 UTC
(In reply to Devin Teske from comment #28)
Yes, I am against a generic opt-in environment scrubber that would solve the OP's issue without a single modification to either service or fcgiwrap, since this would add extra rc.conf variables only for the benefit of admins that start rc.d scripts incorrectly.

Generating a warning when an rc.d script is started manually seems better. For backwards compatibility this may have to be enabled per rc.d script.
Comment 32 Devin Teske freebsd_committer 2019-01-27 23:53:44 UTC
(In reply to Jilles Tjoelker from comment #31)
> only for the benefit of admins that start rc.d scripts incorrectly

Maybe I missed something. Is that what it boiled down to?

I am a strong believer that there should be no difference whether you use service or invoke the rc.d script manually. If there is a difference with respect to environment leakage, that is a bug and needs to be fixed.
Comment 33 Victor Sudakov 2019-01-28 01:46:37 UTC
(In reply to Jilles Tjoelker from comment #31)

> only for the benefit of admins that start rc.d scripts incorrectly.

Since when has starting rc.d scripts directly become incorrect?
Comment 34 Rodney W. Grimes freebsd_committer 2019-01-28 02:07:32 UTC
<Rant Warning ON>
First off someone teach bugzilla that top posting this input box is just a royal pain in the ass when your trying to reply to earlier posts, this whole input box belongs at the BOTTOM of the page.
</Rant>

(In reply to Jilles Tjoelker from comment #31)
I support the idea that we may not want to take this to the extreme of a sanatizer, how ever, I can not say that directly invoking /path/rc.d/foo is an incorrect operation as that existed far longer than services(8).

(In reply to Devin Teske from comment #32)
Having services(8) be different than directly invoked scripts can be considered a) a feature (It allows me to force feed ENV stuff) b) a bug cause it can cause evil leaks or c) a POLA violation cause why should they be different.

Presently I believe we are in the a) state of affairs, and without additional input we may wish to stay that way as changing it may cause a POLA issue.

(In reply to vas from comment #33)
I agree with you on the point that invoking rc.d scripts directly is NOT incorrect procedure, see above at reply to #31

In summary my current position:
I am actually starting to come to the opinion that possibly the only action that we should take AT THIS TIME is to place an env -i in the rc/fcigwrap script to revoke its bad programming style of environment exposure to a cgi.  And to take
this idea of a general sanatizer to the next level == arch@freebsd.org
Comment 35 Rodrigo Osorio freebsd_committer 2019-01-28 09:31:44 UTC
As the fcgiwrap port maintainer, this is my position:

1) If we can agree that starting services by invoking the scripts directly (just like not using sysrc to update rc.conf) isn't wrong, it comes with drawbacks and since this is not the 'recommended/standard' way to start a service, users who decide to go that way should live with -no offense-.

2) The use of env -i when calling the fcgiwrap script doesn't come at no cost. The daemon will be started with en empty PATH variable.
If this has no impact in many cases, I found a few ones who makes the script fail. The most problematic one is the 'which' command used by many cgi script to discover if a command exists, and recover its full path. Run in a 'sanitized' environment, 'which' returns nothing even for base tools like ls. 

Once again, I'm not against changing and improving tools but not at the cost of a massive web-server failure on D+1  with a immediate rollback.

And I fully agree if someone wants to fix it at a higher level.
Comment 36 Rodney W. Grimes freebsd_committer 2019-01-28 09:40:32 UTC
(In reply to Rodrigo Osorio from comment #35)
I think you have misunderstood what is meant when we speak of adding env -i,
it would be added as it is in service(8) which is:
exec env -i HOME=/ PATH=/sbin:/bin:/usr/sbin:/usr/bin $dir/$script $*
so there would be no loss of PATH and none of the problems you mention would occur.
Comment 37 Rodrigo Osorio freebsd_committer 2019-01-28 10:53:12 UTC
(In reply to Rodney W. Grimes from comment #36)

Oh, sorry for the misunderstanding.

SO you are thinking about something like this :

fcgiwrap_start() {
    /usr/sbin/daemon \
	-f -p ${pidfile} \
        /usr/bin/env -i \
        "HOME=/" \
        "PATH=/sbin:/bin:%%PREFIX%%/sbin:%%PREFIX%%/bin" \
        $procname -s ${fcgiwrap_socket} ${actual_fcgiwrap_flags}
}
Comment 38 Rodney W. Grimes freebsd_committer 2019-01-28 11:44:43 UTC
(In reply to Rodrigo Osorio from comment #37)
Yes, though I'll defer the details of where exactly env -i is placed and exactly what values go into PATH to Devin and yourself to work out.  I do not believe that service(8) includes %%Prefix%% and if those are included do the not go before /sbin:/bin so that /usr/local can over ride system?  But again I defer to you and Devin on those details.
Comment 39 John Von Essen 2019-02-02 16:20:47 UTC
Sort of related, but any thoughts on setting the default user/group to www/www for fcgiwrap user and socket owner? It would make it more out-of-the-box ready for running with nginx, this avoids having to set:

fcgiwrap_user="www"
fcgiwrap_socket_owner="www"

in rc.conf. By default it runs as nobody.

Thanks
John
Comment 40 Rodrigo Osorio freebsd_committer 2019-02-03 10:34:06 UTC
After rethinking the arguments I don't gonna change the way www/fcgiwrap rc script behaves. I strongly believe that it's not rc script job to sanitize the ENV variables since service(8) is doing that for you in a better way.

I also don't wanna perform code duplication between service(8) and the www/fcgiwrap rc script because it's bad, and it prevents future changes in service(8) to apply to www/fcgiwrap.

Finally, I don't wanna prevent peoples to start the www/fcgiwrap server manually with custom variables in debug purpose.

Regarding the sanitize framework discussed before, you can continue the discussion in a new improvement PR. To me, I do not see the interest, because once you are starting the daemon with service(8) the env is sanitized for free.

Cheers,
- rodrigo

@ John Von Essen :
Regarding the default user changes used by www/fcgiwrap, nobody is the default because it's the less powered user in the system. Changing that breaks POLA for sure without too much benefit for the user.
Comment 41 Rodney W. Grimes freebsd_committer 2019-02-03 14:27:04 UTC
(In reply to Rodrigo Osorio from comment #40)
Independent of how Rodrigo Osorio feels about www/fcgiwrap there is a larger issue here that needs a deeper evaluation, and either documentation needs updated or some code needs changed.

I am taking this bug, considering it a base system rc bug now, and moving forward.  It is silly to start a new bug as there is far to much good information in this one to simply close it.
Comment 42 Kyle Evans freebsd_committer 2019-02-18 16:46:28 UTC
(In reply to Rodney W. Grimes from comment #41)

Hi,

Sorry, I seem to have not received e-mail for incredibly large swaths of this thread after my initial involvement. =(

I think there's some good arguments to be had by, say, pulling in the environment for the daemon login class by default for the rc command execution context.

For instance, in PR 235791 a user is wanting to setup HTTP_PROXY for various services and finding that they'll have to sprinkle it around through tons of individual environments rather than being able to capture most rc bits with the daemon login class as they'd hoped. This seems like a reasonable feature request and behavior, IMO.