Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure DNS records are only requested for IP allowlist when needed #18244

Closed
fkmotion opened this issue Oct 29, 2021 · 4 comments · Fixed by #18285
Closed

Ensure DNS records are only requested for IP allowlist when needed #18244

fkmotion opened this issue Oct 29, 2021 · 4 comments · Fixed by #18285
Assignees
Labels
Bug For errors / faults / flaws / inconsistencies etc. c: Performance For when we could improve the performance / speed of Matomo. Regression Indicates a feature used to work in a certain way but it no longer does even though it should.
Milestone

Comments

@fkmotion
Copy link

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
@fkmotion fkmotion added the Potential Bug Something that might be a bug, but needs validation and confirmation it can be reproduced. label Oct 29, 2021
@sgiehl
Copy link
Member

sgiehl commented Oct 29, 2021

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:

matomo/config/global.php

Lines 170 to 172 in 4764df9

if (filter_var($ip, FILTER_VALIDATE_IP)) {
$ipsResolved[] = $ip;
} else {

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?

@sgiehl sgiehl added Bug For errors / faults / flaws / inconsistencies etc. and removed Potential Bug Something that might be a bug, but needs validation and confirmation it can be reproduced. labels Oct 29, 2021
@tsteur tsteur added the Regression Indicates a feature used to work in a certain way but it no longer does even though it should. label Oct 31, 2021
@tsteur tsteur added this to the 4.6.0 milestone Oct 31, 2021
@tsteur tsteur added the c: Performance For when we could improve the performance / speed of Matomo. label Oct 31, 2021
@tsteur
Copy link
Member

tsteur commented Oct 31, 2021

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 = @gethostbyname($ip);

@sgiehl
Copy link
Member

sgiehl commented Nov 2, 2021

@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
Copy link
Member

tsteur commented Nov 2, 2021

I suppose we could cache the @dns_get_record($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.

@sgiehl sgiehl self-assigned this Nov 9, 2021
@justinvelluppillai justinvelluppillai changed the title Whitelist and Allowlist Network DNS Request Errors Ensure DNS records are only requested for IP allowlist when needed Nov 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc. c: Performance For when we could improve the performance / speed of Matomo. Regression Indicates a feature used to work in a certain way but it no longer does even though it should.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants