drivers/main.c: storeval() should honour VAR_SENSITIVE and not publish its default/override values

[jimklimov]

Moderate security risk - possible credential leak, probably an unrealistic setup is needed to trigger it however.
Inspired by code revision in #1652

[jimklimov]

First, checked the visibility to not be a practical problem for the few variables (addvar() in snmp-ups.c and netxml-ups.c) defined with VAR_SENSITIVE flag when used properly:

:;  ./drivers/snmp-ups -s test -x mibs=ietf -x community=secret -d 1 -DDDDDD -x port=1.2.3.4
...
device.type: ups
driver.debug: 6
driver.flag.allow_killpower: 0
driver.name: snmp-ups
driver.parameter.mibs: ietf
driver.parameter.pollinterval: 2
driver.parameter.port: 1.2.3.4
driver.parameter.synchronous: auto
driver.state: dumping
driver.version: 2.8.0-2324-gba8de0982
driver.version.data: ietf MIB 1.55
driver.version.internal: 1.30
input.bypass.phases: 1
input.phases: 1
output.phases: 1
ups.status:

So there is no driver.parameter.community here.

• Using "default" instead (and an instrumented copy of snmp-ups to print more debugs:

:;  (make -s -j 16 && ./drivers/snmp-ups -s test  -x mibs=auto -x default.community=secret -d 1 -DDDDDDDDD -x port=1.2.3.4 2>&1) | less
...
   0.001490     [D9] nut_snmp_init: using community=public
...
community: secret
device.mfr: APC
device.type: ats
driver.debug: 9
driver.flag.allow_killpower: 0
driver.name: snmp-ups
driver.parameter.default.community: secret
driver.parameter.mibs: auto
driver.parameter.pollinterval: 2
driver.parameter.port: 1.2.3.4
driver.parameter.synchronous: auto
driver.state: dumping
driver.version: 2.8.0-2324-gba8de0982
driver.version.data: apc_ats MIB 0.6
driver.version.internal: 1.30
ups.status:

Here on one hand, it got exposed; on another - not used (driver treated community as not set => public).

Same for override.community.

Practical uses of those existing sensitive values do not pass through the actual dstate, but are queried directly from vartab_h (see main.c methods like getval() and addvar()). So the best value for these to be hidden in default/override form is pedantic, they would be ignored anyway. Maybe if future uses appear, that is another matter.

[jimklimov]

As a side note, NUT variables use a structured (dot-separated) nomenclature. The default/override mechanism effectively allows to dump any string into those lists (e.g. community in examples above), MAYBE there should be a sanity-check for presence of a dot in the name (after stripping the prefix)?

[jimklimov]

The storeval() itself only gets called from:

• do_upsconf_args() - only if testvar_reloadable(var, ...) > 0
• splitxarg() /* split -x foo=bar into 'foo' and 'bar' */ - if not main_arg() (so a driver arg) => this is the one used for my CLI experiments, apparently

The method itself has two parts: handles override/default essentially vs. dstate info only (no direct looks into vartab_h and accepting any key-name values and patterns). If neither of these two, then it has a loop to check against vartab (and consider sensitivity) for direct hits by verbatim name or fatalx() for unrecognized CLI/upsconf arguments.

[jimklimov]

In the end, the solution is to do nothing :) Currently those overrides for certain ups.conf fields are possible (technologically), but ignored. It may be more worthwhile to keep them exposed to highlight a problem with that sort of contrived setup.