@fkmotion opened this Issue on October 29th 2021

Expected Behavior

No DNS Request for network entries just for hostnames in login_whitelist_ip/login_allowlist_ip

Current Behavior

All entries in login_whitelist_ip/login_allowlist_ip for networks that was specified with an "/" are attempted to be resolved using DNS.
For Example:
We have "192.168.50.0/24" in our login_whitelist_ip/login_allowlist_ip. Now our DNS Server recivies a ton of request (around 175000 per day) trying to resolve a IP for hostname "192.168.50.0/24" and "192.168.50.0/24.ourdomain.com"

login_whitelist_ip/login_allowlist_ip still is working as expected.
We have this issue since our update to matomo 4.5.0

Possible Solution

Don't try to resolve a DNS Name for a network.

Steps to Reproduce (for Bugs)

  1. Enter a "/x" Network into login_whitelist_ip or login_allowlist_ip like "192.168.50.0/24"
  2. Check all DNS Request send by Matomo

Context

Your Environment

  • Matomo Version: 4.5.0
  • PHP Version: 7.4
  • Server Operating System: Ubuntu 20.04.3 LTS
@sgiehl commented on October 29th 2021 Member

Hi @fkmotion. Thanks for taking the time to create this bug report. I would assume that is a side effect of #18046.
We are currently checking IPs that way:
https://github.com/matomo-org/matomo/blob/4764df9b0c795301976788df87fe314ecc7acece/config/global.php#L170-L172
But seems the filter_var isn't counting ranges as valid IPs. Guess we need to extend that check for IP ranges somehow, so no host lookup will be started.

@tsteur The config is also loaded for tracker request, right? Should we avoid doing the lookups in that case, as the allowlist actually isn't used for tracking requests? Or may the allowlist apply if a token_auth is sent with the request?

@tsteur commented on October 31st 2021 Member

If I see this right then we aren't using the login allowlist yet in the tracking code (eg when authenticating requests). I reckon therefore it could be maybe fine to do this and would need to mention this in https://matomo.org/faq/how-to/faq_25543/

Not sure if there could be any side effects from this as the tracking code could launch the regular Matomo to populate general cache AFAIK. But I don't think in any of these cases would rely on login allow list (at least not yet).

I wonder if something like below would fix this too (not tested). Of course it wouldn't fix it in other cases where a hostname is provided. Generally I'm bit scared that there may be a security regression in the future because of it but shouldn't be. Generally though, I think this DI config shouldn't even be loaded during tracking requests? If so, then it may be needed after all to resolve these entries during tracking mode.

I don't think the user mentioned tracking requests being the issue so this might not be the problem? @fkmotion do you know if there are a lot of UI requests or is this caused by tracking requests?

Maybe below diff could be a fix for this.

diff --git a/config/global.php b/config/global.php
index f7834df256..1869c82d19 100644
--- a/config/global.php
+++ b/config/global.php
@@ -1,5 +1,6 @@
 <?php

+use Matomo\Network\IPUtils;
 use Psr\Container\ContainerInterface;
 use Matomo\Cache\Eager;
 use Piwik\SettingsServer;
@@ -167,7 +168,7 @@ return array(

         foreach ($ips as $ip) {
             $ip = trim($ip);
-            if (filter_var($ip, FILTER_VALIDATE_IP)) {
+            if (filter_var($ip, FILTER_VALIDATE_IP) || IPUtils::getIPRangeBounds($ip) !== null) {
                 $ipsResolved[] = $ip;
             } else {
                 $ipFromHost = <a class='mention' href='https://github.com/gethostbyname'>@gethostbyname</a>($ip);
@sgiehl commented on November 2nd 2021 Member

@tsteur this might already help I guess. Maybe we could also cache the result of resolving the host for a couple of minutes to avoid permanent requests?

@tsteur commented on November 2nd 2021 Member

I suppose we could cache the <a class='mention' href='https://github.com/dns_get_record'>@dns_get_record</a>($ip, DNS_AAAA); call for say 30 seconds. It's otherwise bit annoying if it takes too long to recognise a new IP.

We probably wouldn't want to cache the entire result maybe as it would maybe make it complicated when configuring the IPs and then it would look like the configuration isn't working when it's just a cache. Of course could cache the entire resolved IPs maybe if we create the cache key based on all the configurations.

Additionally, be good to add the IpRange check.

And we should also double check that during a regular tracking request we're not requesting the login.allowlist.ips setting.

This Issue was closed on November 10th 2021
Powered by GitHub Issue Mirror