Adds world readable attribute to files created by Pi-hole to circumve…

[pvogt09]

…nt #2724

Signed-off-by: pvogt09 [email protected]

By submitting this pull request, I confirm the following:

• I have read and understood the contributors guide, as well as this entire template.
• I have made only one major change in my proposed changes.
• I have commented my proposed changes within the code.
• I have tested my proposed changes, and have included unit tests where possible.
• I am willing to help maintain this change if there are issues with it later.
• I give this submission freely and claim no ownership.
• It is compatible with the EUPL 1.2 license
• I have squashed any insignificant commits. (git rebase)

Please make sure you Sign Off all commits. Pi-hole enforces the DCO.

---

What does this PR aim to accomplish?:
This PR should partially fix #2724 by setting most of the files that Pi-hole creates during install or update are made world readable explicitly instead of relying on the umask being set to something that results in world readable files/directories.

• Maybe /etc/pihole/setupVars.conf permissions have to be set in a different place, because there is no touch or echo > for this file.
• Maybe not all files have the correct access rights.
• Are the permissions and owner of /usr/local/share/man/man{5, 8} correct for all distributions?

How does this PR accomplish the above?:
chmod a+r ... is added to make files world readable and files are copied with -p option to keep file permissions (especially for backups).

What documentation changes (if any) are needed to support this PR?:
None.

[AzureMarker]

Most files should be 644, so can you change the chmod calls to set the permissions to 644?

[AzureMarker]

Can instances where a file is copied and assigned permissions be replaced with install?

[AzureMarker]

Line 2261 of the installer should set the permissions dnsmasq.conf again, as it may have been moved a few lines up.

There's also an issue where some web interface folders don't give www-data execution permission, such as /var/www and the folders in /var/www/html/admin. This causes a 403 to be returned by lighttpd.

[pvogt09]

• The file /etc/sysconfig/network/ifcfg-${PIHOLE_INTERFACE} may also need other permissions, but I can not test which one is needed, because for Raspbian, the function is not called.

According to this answer, it would be possible with:

tempvar=$webroot;
while [[ "$tempvar" != "/" && "$tempvar" != "." ]]; do
     chmod o+rx "$tempvar";
     tempvar=$(dirname "$tempvar");
done;

What happens if the $webroot variable is changed to something (e.g. depending on the outcome of #2692, if anything remains in /var/www/html), where world readable and executable in all parent directories is not wanted by the user or doesn't that matter?

[AzureMarker]

The ifcfg file only needs root:root 644, it's a networking file used on RHEL distros like Fedora.

You should be able to do

chmod a+r /var/www
chmod a+r /var/www/html

And then the admin folder is taken care of as part of the git checkout. As we don't allow configuring the location of the web interface, this is fine.

Once we replace the PHP web interface, there will be no files necessary for the web interface. It is embedded in the API's binary.

[AzureMarker]

Tested this again. The /var/www/html/admin folder and subfolders need to be executable by all users for the web server to be able to serve the files.

[AzureMarker]

Fails to read the adlists file when you open the settings page:

[pvogt09]

Is chooseBlocklists the only place where this file can be created?
Do the files like ist.0.raw.githubusercontent.com.domains have to be changed as well and where are they created?

[AzureMarker]

Here's a directory listing from a fresh install:

total 11556
drwxr-xr-x  2 pihole pihole      4096 May 10 02:55 ./
drwxr-xr-x 98 root   root        4096 May 10 02:54 ../
-rw-r--r--  1 root   root         381 May 10 02:54 adlists.list
-rw-r--r--  1 pihole pihole         0 May 10 02:54 dhcp.leases
-rw-r--r--  1 root   root         592 May 10 02:54 dns-servers.conf
-rw-r--r--  1 root   root          18 May 10 02:54 GitHubVersions
-rw-r--r--  1 root   root     2646365 May 10 02:54 gravity.list
-rw-r--r--  1 root   root        1226 May 10 02:54 install.log
-rw-------  1 root   root     1180890 May 10 02:54 list.0.raw.githubusercontent.com.domains
-rw-------  1 root   root      595973 May 10 02:54 list.1.mirror1.malwaredomains.com.domains
-rw-------  1 root   root      638705 May 10 02:54 list.2.sysctl.org.domains
-rw-------  1 root   root        7305 May 10 02:54 list.3.zeustracker.abuse.ch.domains
-rw-------  1 root   root         613 May 10 02:54 list.4.s3.amazonaws.com.domains
-rw-------  1 root   root       43642 May 10 02:54 list.5.s3.amazonaws.com.domains
-rw-------  1 root   root     1772364 May 10 02:54 list.6.hosts-file.net.domains
-rw-r--r--  1 root   root     2646365 May 10 02:54 list.preEventHorizon
-rw-r--r--  1 root   root          25 May 10 02:54 localbranches
-rw-r--r--  1 root   root          56 May 10 02:54 local.list
-rw-r--r--  1 root   root          26 May 10 02:54 localversions
-rw-r--r--  1 root   root         238 May 10 02:54 logrotate
-rw-r-----  1 root   root     2207744 May 10 02:54 macvendor.db
-rw-rw-r--  1 pihole root          15 May 10 02:54 pihole-FTL.conf
-rw-r--r--  1 pihole pihole     24576 May 10 02:55 pihole-FTL.db
-rw-rw-r--  1 pihole www-data       0 May 10 02:54 regex.list
-rw-r--r--  1 root   root         300 May 10 02:54 setupVars.conf

macvender.db should be updated with world-read permissions and/or made pihole:pihole as it's needed by FTL.

Here, the list file (destination) should be touch-ed and chmod-ed:

pi-hole/gravity.sh

Line 330 in 54ae9d8

local source="${1}" destination="${2}" firstLine abpFilter

[pvogt09]

The macvendor.db file is created before the pihole user exists:
E assert '[✓] Downloading and Installing FTL' in ' [i] Downloading and Installing FTL...transferred... ' E + where ' [i] Downloading and Installing FTL...transferred... ' = CommandResult(command='\n source /opt/pihole/basic-install.sh\n binary="...input: Inappropriate ioctl for device\nchown: invalid user: 'pihole:pihole'\n").stdout
In which way should this be resolved? Should the user be created earlier before installing FTL or the file be created later during installPihole or the file not be owned by pihole?

[AzureMarker]

The pihole user creation should be moved earlier so that the database can have the correct permissions.

[pvogt09]

The pihole user creation should be moved earlier so that the database can have the correct permissions.

What is the correct place for user creation? I would assume main, but then, the tests in test_automated_install.py will have to be changed to

def test_FTL_detect_armv6l_no_errors(Pihole):
    '''
    confirms only armv6l package is downloaded for FTL engine
    '''
    # mock uname to return armv6l platform
    mock_command('uname', {'-m': ('armv6l', '0')}, Pihole)
    # mock ldd to respond with aarch64 shared library
    mock_command('ldd', {'/bin/ls': ('/lib/ld-linux-armhf.so.3', '0')}, Pihole)
    detectPlatform = Pihole.run('''
    source /opt/pihole/basic-install.sh
    create_pihole_user
    FTLdetect
    ''')
    expected_stdout = info_box + ' FTL Checks...'
    assert expected_stdout in detectPlatform.stdout
    expected_stdout = tick_box + (' Detected ARM-hf architecture '
                                  '(armv6 or lower)')
    assert expected_stdout in detectPlatform.stdout
    expected_stdout = tick_box + ' Downloading and Installing FTL'
    assert expected_stdout in detectPlatform.stdout

to pass, because the user still does not exist in this case for the test case.
And possibly the expected output has to take the messages into account, that are created when the pihole user is created or checked for (e.g. Checking for user 'pihole'), but I do not know how to archieve this.

[AzureMarker]

The user should be added right before the FTL install in main, as you linked. You don't need to add the pihole user output to the test.

[AzureMarker]

Thanks for sticking with this PR!

[pvogt09]

You're welcome. Can I close #2724 or is it still needed?

[AzureMarker]

I will mark it as fixed.

[pralor-bot]

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pi-hole-ftl-v5-12-web-v5-9-and-core-v5-7-released/51795/1