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

Split private directories system check into "Required" and "Recommended" #17577

Closed
mddvul22 opened this issue May 18, 2021 · 15 comments · Fixed by #17889
Closed

Split private directories system check into "Required" and "Recommended" #17577

mddvul22 opened this issue May 18, 2021 · 15 comments · Fixed by #17889
Assignees
Labels
c: Usability For issues that let users achieve a defined goal more effectively or efficiently. Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement. Needs Review PRs that need a code review
Milestone

Comments

@mddvul22
Copy link

Today, I upgraded to Matomo 4.3.0. The Matomo system check complains about "Required Private Directories". Specifically, it tells me:

"We found that the above URLs are accessible via the browser, but they should NOT be. Allowing them to be accessed can pose a potential security risk since the contents can provide information about your server and potentially your users. Please restrict access to them.

We also found that Matomo's config directory is publicly accessible. While attackers can't read the config now, if your webserver stops executing PHP files for some reason, your MySQL credentials and other information will be available to anyone. Please check your webserver config and deny access to this directory. "

I'm not positive, but something seems to be messed up here. I have my config directory set to 700. My config.ini.php is set to 600. What should the permissions be? Matomo doesn't say what it is looking for. For me to lock it down further, I basically have to change take access away from the Apache user. Maybe I'm missing something obvious, but this doesn't seem right.

Your Environment

  • Matomo Version: 4.3.0
  • PHP Version: 7.2.24
  • Server Operating System: Centos 8 Linux
@mddvul22 mddvul22 added the Potential Bug Something that might be a bug, but needs validation and confirmation it can be reproduced. label May 18, 2021
@sgiehl
Copy link
Member

sgiehl commented May 18, 2021

Hi @mddvul22. Thanks for creating this issue. Matomo actually tries to do a http request on the config file and checks if the response code to be between 400 and 500. So you may need to disallow access in webserver config.
See #17568

@heurteph-ei
Copy link

Maybe some documentation somewhere should be updated... And a link to this doc should be provided in the warning message?

@fireba11
Copy link

At least on my server
config/config.ini.php returns a single ';'
tmp/cache/tracker/matomocache_general.php returns an empty file
causing the check to complain since something is returned.

Also to me it seems those files exist by default, how are they supposed to be protected? (The warning should tell you too ..)

@Findus23
Copy link
Member

I have explained this a bit more detailed here:
https://forum.matomo.org/t/nach-update-auf-4-3-fehlermeldung-required-private-directories/41903/4?u=lukas

Maybe the error message could be expanded with this help

@fireba11
Copy link

Note:
I'm using centos7 and httpd, the .htaccess is there but doesn't seem to work for me.
I'm investigating possible reasons, the use of php-fpm being one candidate ...

@Findus23
Copy link
Member

One thing: With the current check Matomo doesn't accept it, if the config.ini.php returns a 304 e.g. to the homepage.

@mddvul22
Copy link
Author

I'm open to other suggestions on the best way to fix this. However, I have placed this in my Matomo Apache configuration:

    <LocationMatch "^/(tmp|config|lang)">
            Require all denied
            Require ip 127.0.0.1
    </LocationMatch>

And this has removed the warning from the Matomo system check. Again, I'm open to suggestions of a better way to address this.

@tsteur tsteur added this to the 4.3.1 milestone May 18, 2021
@tsteur
Copy link
Member

tsteur commented May 18, 2021

I think initially there was a check for the retrieved content. To adjust the check for redirects etc maybe we could follow redirects and check if the retrieved config is trim($content) !== ';' (before was only a contains check or so) and check whether it's actually returning any content from the config. So if response code is >=400 && < 500 then all is good. If not we'd maybe do some additional checks regarding the redirects etc (we need to follow the redirects to see if it maybe just redirects from http to https etc)

@GermanKiwi
Copy link

Hi guys, I've also had this problem after updating to 4.3.0, but for a different file and folder. For my site, the "Required Private Directory" warning refers to this URL:

https://matomo.example.com/lang/en.json

My site is running on Apache, and inside my /lang/ directory there is a Matomo-generated .htaccess file which I've attached to my comment here (renamed to a .txt file). As far as I can tell, it's configured correctly to block all the files in this directory.

However, when I check the HTTP response header of the above URL, it's returning "200 OK", rather than 4xx.

Any idea why? To me it feels like Matomo is doing something wrong here.

htaccess.txt

@prbt2016
Copy link

prbt2016 commented May 19, 2021

Hello,

I was in the process of manual installation of Matomo 4.3.0 on Centos 6.9 with PHP 7.2.24, MYSQL 5.5, Apache 2.2.

However I face the same issue as mentioned above on the following file i.e :

http://example.com/piwik/lang/en.json

i.e the following error is thrown after visiting Diagnostic->System Check :

test

Could you please replicate and fix the issue?.

@tsteur tsteur added c: Usability For issues that let users achieve a defined goal more effectively or efficiently. and removed Potential Bug Something that might be a bug, but needs validation and confirmation it can be reproduced. labels May 21, 2021
@tsteur
Copy link
Member

tsteur commented May 21, 2021

Created #17604 as a first improvement as well as this FAQ https://matomo.org/faq/troubleshooting/how-do-i-fix-the-error-private-directories-are-accessible/ which we can improve and complete over time.

To fully fix this issue we should maybe split the one system check into two system checks:

  • private directories that are basically required to fix: '.git/config', 'tmp/cache/tracker/matomocache_general.php', config/config.ini.php
  • private directories that should be fixed/that we recommend to be fixed (only lang/en.json and '/tmp/empty' check for now). These directories are basically optional. Once we add more checks eg for core etc then we could move these files in there as well.

Currently, /tmp is also hard coded and we may want to use StaticContainer::get('path.tmp') instead. And in case that strpos($tmpDir, PIWIK_INCLUDE_PATH) === false then we would need to assume the tmp is not in the public directory and don't need to execute the check for the tmp directory.

For docs around this see https://developer.matomo.org/guides/system-check

@tsteur tsteur modified the milestones: 4.3.1, 4.3.0 May 21, 2021
@mattab mattab modified the milestones: 4.3.0, 4.4.0 May 26, 2021
@tsteur tsteur added the Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement. label Jul 22, 2021
@mattab mattab modified the milestones: 4.4.0, 4.5.0 Jul 28, 2021
@geekdenz geekdenz self-assigned this Aug 6, 2021
@geekdenz
Copy link
Contributor

This is difficult without affecting performance or requiring a certain webserver like Apache and adding an .htaccess file or required information and configuration.

I think one way would be to recommend RewriteRules for Apache and similarly for NGINX in the warning message or linking to a page that describes in detail how to configure the webserver.

To make it easier for the user, we could have a global RewriteRule and proxy everything through index.php?path=$somepath (or another script, e.g. filter.php?path=$somepath and forward to index.php) and have our code filter and do a stream_copy_to_stream for straight files. This function should be pretty much the same speed as a straight request but offer the benefit of our code being the filter for potentially harmful requests.

Not sure what a really quick solution would be at this stage.

@tsteur
Copy link
Member

tsteur commented Aug 11, 2021

@geekdenz be good to have a look at #17577 (comment) which is the only thing left to do.

Not 100% sure what you refer to with affecting performance? The recommendations etc should be already done. We only need to adjust the system check that's all.

@geekdenz
Copy link
Contributor

Yes. You're right. I over-estimated the requirement and reading again clarifies that it's just a documentation requirement in the diagnostics.

@geekdenz geekdenz added the Needs Review PRs that need a code review label Aug 15, 2021
geekdenz pushed a commit that referenced this issue Aug 16, 2021
geekdenz pushed a commit that referenced this issue Aug 16, 2021
ensure language file reverted
@geekdenz
Copy link
Contributor

Currently I've got it refactored and it looks like this.

Screen Shot 2021-08-16 at 3 12 07 PM

Screen Shot 2021-08-16 at 3 11 51 PM

Should the "Recommended Private Directories just be warnings instead of errors?

geekdenz pushed a commit that referenced this issue Aug 16, 2021
ensure language file reverted
geekdenz pushed a commit that referenced this issue Aug 23, 2021
geekdenz pushed a commit that referenced this issue Aug 23, 2021
ensure language file reverted
geekdenz pushed a commit that referenced this issue Aug 26, 2021
geekdenz pushed a commit that referenced this issue Aug 26, 2021
ensure language file reverted
geekdenz pushed a commit that referenced this issue Aug 26, 2021
reinstate logic but logic changes required and adapt from
main branch to match requirements
geekdenz pushed a commit that referenced this issue Aug 31, 2021
* split private directories check into required and recommended

fixes #17577

* add translations for en and de #17577

* refactor PrivateDirectories for overlap #17577

ensure language file reverted

* put checks in order, add check for lang/en.json #17577

* refactor Required and RecommendedPrivateDirectories #17577

reinstate logic but logic changes required and adapt from
main branch to match requirements

* remove unused use statements, update ui-test screenshot #17577

* refactor translation for 'read this...' #17577
@justinvelluppillai justinvelluppillai changed the title Matomo 4.3 Complains about Private Directories Split private directories system check into "Required" and "Recommended" Oct 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Usability For issues that let users achieve a defined goal more effectively or efficiently. Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement. Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants