[security] Allow IP list is not verified

[jmue]

Hi,

the list of allowed host is not verified anymore. The check was removed with commit 4acb4313 in file socket.c:92. This is a security-critical bug as anyone with network acces is allowed to change heater settings.

Regards, Jens

[hmueller01]

You are right! Nice finding. Have you tried adding the old (slightly modified) code?

        if (checkP && !(*checkP)(clienthost)) {
            logIT(LOG_NOTICE, "Access denied %s:%s", clienthost, clientservice);
            close(connfd);
            continue;
        }

The further code should be changed too

        if (connfd < 0) {
            logIT(LOG_NOTICE, "accept on host %s: port %s", clienthost, clientservice);
            close(connfd);
            continue;
        }

as a negative return value of accept indicates a failure. I do not see this in the log output. Shouldn't it say something like "accept failed on host ..."? Wouldn't be LOG_ERR be more suitable than LOG_NOTICE?

[speters]

IMO, it is a bad design choice to do access control check, but omitting it on the protocol level by leaving out status info and just close connection. According to DRY and fail-early principles, exact same behaviour can already be done at the networking level/iptables/firewall.

[hmueller01]

Yes, might be a bad design. But documentation specifies an allow ip, e.g.

<allow ip='127.0.0.1'/>

and this is currently not used by the code. This might be misleading ...

[jmue]

@hmueller01: No, i didn't try the modified code. I setup a firewall on that machine.

I agree, having the check function is counterproductive. But as a newbie to vcontrol I followed the tutorials. So I plead to remove the check function itself and also the references in provided config files and documentation.

While speaking about good design, from my perspective it would be wise to open only a localhost network socket as the default option.

[l3u]

While speaking about good design, from my perspective it would be wise to open only a localhost network socket as the default option.

This would be a good idea for sure!

Just speaking of me, I run vcontrold on a Raspberry Pi with an attached hone- brew optolink adapter. And all scripts run by cronjobs that communicate with the heating run on that computer.

I think this is quite a common use-case. So imo it would be fully sufficient (and of course way more secure) to only accept connections from localhost by default, and having the config to be changed to do otherwise.

Cheers, Tobias