jimklimov
commented
2 years ago Well, given that the idea of these tunables is to allow tuning to fit the deployment's needs, and there are many more path layouts especially in embedded systems than just LFS or some "standard Unix" dir tree, I am not sure we can or should constrain this mechanism - but indeed a clearer design idea can help revise the opinion :)
If the user is privileged enough to call NUT in a privileged manner (to pass envvars that point to some paths not accessible to common users) that account could be exploited differently anyway. If this is more about mistake mitigation (like "don't store NUT state data over /etc/shadow"), then again, a lot of other tools might corrupt the system in the same manner and generally don't/shouldn't protect the user from shooting themselves in the foot. System management is for the apt (pun intended).
Finally, one quick thing that might be done is to have a toggle to forbid (by default eventually?) using paths set via env, so "insecure" runs that consider the envvars for that would be something deliberate. Packaged services might as well configure needed paths to bake into the binaries and probably not suffer from lack of envvars - but this sounds like something to ask for the diverse-systems community input.
OTOH, I would expect this to be an issue also with the NUT_STATEPATH, PID path, ALTsomething paths defined for fallbacks, etc. The ability to tweak them via env is invaluable for development and testing of a custom build from command line (e.g. to run networked drivers that do not need much about filesystem privileges) - so quite a use-case that allows for flipping a switch for "less secure mode".
Potentially same "issue" applies to PATH, LD_LIBRARY_PATH and such that could be exploited much more easily. Not sure if we read those with getenv (so maybe no warnings from Coverity - and no easy, portable and safe way to control that content), but user-provided input here could impact which libs we load at run-time.
Over Eaton code analysis of NUT, a number of issues have been reported, including Path manipulation:
A user-controllable string is used as part or all of a filesystem path, filename, or URI (uniform resource identifier). tainted_return_value: Function confpath returns tainted data. (line 76) vararg_transitive: Call to snprintf with tainted argument *confpath() taints fn. (line 76) path_manipulation_sink: Constructing a path or URI using the tainted value fn and passing it to pconf_file_begin. This may allow an attacker to access, modify, or test the existence of critical or sensitive files. (line 80) remediation: ['Path manipulation vulnerabilities can be addressed by proper input validation. Blacklisting characters that allow unsafe path traversal can improve the safety of the input, but the recommended approach is to whitelist the set of expected characters. This should exclude absolute paths and upward directory traversal.'] (line 80) For more information look at Coverity Portal
for upsconf.c, this goes up to common/common.c:421 (confpath(void)), which can accept its value from the env. variable NUT_CONFPATH.
Enforcement must be considered to avoid a leak. The exact technical solution is still to be designed though.