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

More checks when downloading from file:// scheme #5620

Merged
merged 1 commit into from Mar 28, 2024

Conversation

orazioedoardo
Copy link
Contributor

Thank you for your contribution to the Pi-hole Community!

Please read the comments below to help us consider your Pull Request.

We are all volunteers and completing the process outlined will help us review your commits quicker.

Please make sure you

  1. Base your code and PRs against the repositories developmental branch.
  2. Sign Off all commits as we enforce the DCO for all contributions
  3. Sign all your commits as they must have verified signatures
  4. File a pull request for any change that requires changes to our documentation at our documentation repo

What does this PR aim to accomplish?:

Current checks when downloading from file:// scheme can be bypassed by creating a symlink to a file that is not world readable and downloading the symlink, which usually has 777 permissions. Checks also don’t stop non-regular files (though might fail when passed to curl later on). Impact is low IMHO since it requires unprivileged shell access to the system in addition of having access to the admin page.

By the way I’m not sure whether the admin page should still show snippets of invalid adlists when downloading from world readable file://. The set of users with admin access does not necessarily intersect with the users having shell access. Also consider that many distributions still set human home directories as 0755.

How does this PR accomplish the above?:

Test that the file is a regular file, follow symlinks when checking permissions with stat.


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code and I have tested my changes.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)
  6. I have checked that another pull request for this purpose does not exist.
  7. I have considered, and confirmed that this submission will be valuable to others.
  8. I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  9. I give this submission freely, and claim no ownership to its content.

  • I have read the above and my PR is ready for review. Check this box to confirm

Signed-off-by: Orazio <22700499+orazioedoardo@users.noreply.github.com>
@PromoFaux
Copy link
Member

Are we sure this is a problem? I tried a test with sudo ln /etc/shadow blah and adding it as file:///home/pi/blah and get the following output:

  [i] Target: file:///home/pi/blah
  [✗] Cannot read file (file needs to have a+r permission)
<------  [✗] Status: Retrieval failed / empty list
  [✗] List download failed: no cached list available

@orazioedoardo
Copy link
Contributor Author

orazioedoardo commented Mar 27, 2024

Are we sure this is a problem?

Works for me.

sudo ln /etc/shadow blah

Are you sure blah is a symlink? ln without -s is a hard link.

Also you don’t need to create a link with sudo.

@PromoFaux
Copy link
Member

OK, with -s then it works. So much for responsible disclosure, I guess! I think this is probably an edge-case as it would assume that someone has access to the shell to be able to create the symlink in the first place. However having tested your change with a symlink (created with sudo ln -s /etc/shadow blah) I can confirm that the symlink is followed and the original file's permissions are used - and that this doesn't break the already in-place functionality to prevent gravity from reading these files.

Also you don’t need to create a link with sudo.

pi@raspberrypi:/etc/pihole $ ln -s /etc/shadow test
ln: failed to create symbolic link 'test': Permission denied

@orazioedoardo
Copy link
Contributor Author

orazioedoardo commented Mar 27, 2024

ln: failed to create symbolic link 'test': Permission denied

Of course you can’t create a symlink inside /etc/pihole as pi user.

@PromoFaux
Copy link
Member

PromoFaux commented Mar 27, 2024

Perhaps, but the point was you said one doesn't need sudo to create a symlink. If someone has root shell access, you probably have bigger problems!

OK, I see where I went wrong.

@orazioedoardo
Copy link
Contributor Author

If someone has root shell access, you probably have bigger problems!

You just need shell access and a place to write the symlink, which can be the home folder of the user you have access to, doesn’t need to be root or having sudo access.

DL6ER added a commit that referenced this pull request Mar 28, 2024
Signed-off-by: DL6ER <dl6er@dl6er.de>
@dschaper dschaper merged commit 8cfccf9 into pi-hole:development Mar 28, 2024
15 checks passed
truecharts-admin added a commit to truecharts/charts that referenced this pull request Apr 1, 2024
…ad by renovate (#20074)

This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [pihole/pihole](https://togithub.com/pi-hole/docker-pi-hole) | patch |
`2024.03.1` -> `2024.03.2` |

---

> [!WARNING]
> Some dependencies could not be looked up. Check the Dependency
Dashboard for more information.

---

### Release Notes

<details>
<summary>pi-hole/docker-pi-hole (pihole/pihole)</summary>

###
[`v2024.03.2`](https://togithub.com/pi-hole/docker-pi-hole/releases/tag/2024.03.2)

[Compare
Source](https://togithub.com/pi-hole/docker-pi-hole/compare/2024.03.1...2024.03.2)

New tag to include code v5.18.2

<!-- Release notes generated using configuration in .github/release.yml
at master -->

#### What's Changed (Core v5.18.2)

- More checks when downloading from file:// scheme by
[@&#8203;orazioedoardo](https://togithub.com/orazioedoardo) in
[pi-hole/pi-hole#5620

#### New Contributors

- [@&#8203;orazioedoardo](https://togithub.com/orazioedoardo) made their
first contribution in
[pi-hole/pi-hole#5620

**Full Changelog**:
pi-hole/pi-hole@v5.18.1...v5.18.2

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://togithub.com/renovatebot/renovate).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yNzguMCIsInVwZGF0ZWRJblZlciI6IjM3LjI3OC4wIiwidGFyZ2V0QnJhbmNoIjoibWFzdGVyIiwibGFiZWxzIjpbImF1dG9tZXJnZSIsInVwZGF0ZS9kb2NrZXIvZ2VuZXJhbC9ub24tbWFqb3IiXX0=-->
mgziminsky added a commit to mgziminsky/pi-hole-alpine that referenced this pull request Apr 7, 2024
commit 5490a6e
Merge: 74a44ca 8cfccf9
Author: Adam Warner <me@adamwarner.co.uk>
Date:   Sun Mar 31 19:46:24 2024 +0100

    Release 5.18.2 (pi-hole#5629)

commit 8cfccf9
Merge: 47998e1 d80fcf2
Author: Dan Schaper <dan.schaper@pi-hole.net>
Date:   Thu Mar 28 09:44:51 2024 -0700

    More checks when downloading from file:// scheme (pi-hole#5620)

commit 47998e1
Merge: 7879f07 eb23fbf
Author: Dan Schaper <dan.schaper@pi-hole.net>
Date:   Wed Mar 27 16:31:40 2024 -0700

    Bump actions/checkout from 4.1.1 to 4.1.2 (pi-hole#5604)

commit d80fcf2
Author: Orazio <22700499+orazioedoardo@users.noreply.github.com>
Date:   Wed Mar 27 22:10:12 2024 +0100

    More checks when downloading from file:// scheme

    Signed-off-by: Orazio <22700499+orazioedoardo@users.noreply.github.com>

commit 7879f07
Merge: 32c640e 74a44ca
Author: Adam Warner <me@adamwarner.co.uk>
Date:   Wed Mar 27 20:15:28 2024 +0000

    Sync master back into development (pi-hole#5619)

commit 74a44ca
Merge: 3c7a6ce 32c640e
Author: Adam Warner <me@adamwarner.co.uk>
Date:   Wed Mar 27 19:18:09 2024 +0000

    v5.18.1 (pi-hole#5618)

commit 32c640e
Merge: 7442454 eb7daf4
Author: Adam Warner <me@adamwarner.co.uk>
Date:   Wed Mar 27 19:16:25 2024 +0000

    Remove double quotes that prevented _any_ local files from being read by gravity (pi-hole#5617)

commit eb7daf4
Author: Adam Warner <me@adamwarner.co.uk>
Date:   Wed Mar 27 19:12:59 2024 +0000

    Fix file permission check in gravity.sh. Remove quotes that were added after complaints from shellcheck, this stopped the comparisson from working

    Signed-off-by: Adam Warner <me@adamwarner.co.uk>

commit 7442454
Merge: f3af031 3c7a6ce
Author: Adam Warner <me@adamwarner.co.uk>
Date:   Wed Mar 27 18:25:47 2024 +0000

    Sync master back into development (pi-hole#5616)

commit 3c7a6ce
Merge: 19bfa08 f3af031
Author: Adam Warner <me@adamwarner.co.uk>
Date:   Wed Mar 27 18:11:12 2024 +0000

    Release 5.18 (pi-hole#5615)

commit f3af031
Merge: eaa878e 9dd138b
Author: Adam Warner <github@adamwarner.co.uk>
Date:   Wed Mar 27 18:02:44 2024 +0000

    Merge pull request from GHSA-95g6-7q26-mp9x

    Only use local files (file://) when they have explicit permissions a+r

commit eb23fbf
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Sat Mar 16 10:08:27 2024 +0000

    Bump actions/checkout from 4.1.1 to 4.1.2

    Bumps [actions/checkout](https://github.com/actions/checkout) from 4.1.1 to 4.1.2.
    - [Release notes](https://github.com/actions/checkout/releases)
    - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
    - [Commits](actions/checkout@v4.1.1...v4.1.2)

    ---
    updated-dependencies:
    - dependency-name: actions/checkout
      dependency-type: direct:production
      update-type: version-update:semver-patch
    ...

    Signed-off-by: dependabot[bot] <support@github.com>

commit eaa878e
Merge: 0597128 8042d9e
Author: yubiuser <ckoenig@posteo.de>
Date:   Sat Mar 9 12:03:35 2024 +0100

    Bump tox from 4.13.0 to 4.14.1 in /test (pi-hole#5602)

commit 8042d9e
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Sat Mar 9 10:21:53 2024 +0000

    Bump tox from 4.13.0 to 4.14.1 in /test

    Bumps [tox](https://github.com/tox-dev/tox) from 4.13.0 to 4.14.1.
    - [Release notes](https://github.com/tox-dev/tox/releases)
    - [Changelog](https://github.com/tox-dev/tox/blob/main/docs/changelog.rst)
    - [Commits](tox-dev/tox@4.13.0...4.14.1)

    ---
    updated-dependencies:
    - dependency-name: tox
      dependency-type: direct:production
      update-type: version-update:semver-minor
    ...

    Signed-off-by: dependabot[bot] <support@github.com>

commit 9dd138b
Author: DL6ER <dl6er@dl6er.de>
Date:   Mon Mar 4 19:38:13 2024 +0100

    Only use local files (file://) when they have explicit permissions a+r

    Signed-off-by: DL6ER <dl6er@dl6er.de>

commit 0597128
Merge: e03ddf5 0fdd959
Author: yubiuser <ckoenig@posteo.de>
Date:   Sat Mar 2 12:56:52 2024 +0100

    Bump pytest-testinfra from 10.0.0 to 10.1.0 in /test (pi-hole#5579)

commit 0fdd959
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Sat Mar 2 11:52:21 2024 +0000

    Bump pytest-testinfra from 10.0.0 to 10.1.0 in /test

    Bumps [pytest-testinfra](https://github.com/pytest-dev/pytest-testinfra) from 10.0.0 to 10.1.0.
    - [Release notes](https://github.com/pytest-dev/pytest-testinfra/releases)
    - [Changelog](https://github.com/pytest-dev/pytest-testinfra/blob/main/CHANGELOG.rst)
    - [Commits](pytest-dev/pytest-testinfra@10.0.0...10.1.0)

    ---
    updated-dependencies:
    - dependency-name: pytest-testinfra
      dependency-type: direct:production
      update-type: version-update:semver-minor
    ...

    Signed-off-by: dependabot[bot] <support@github.com>

commit e03ddf5
Merge: b57cf27 cb3e448
Author: yubiuser <ckoenig@posteo.de>
Date:   Sat Mar 2 12:51:31 2024 +0100

    Bump pytest from 8.0.0 to 8.0.2 in /test (pi-hole#5598)

commit cb3e448
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Sat Mar 2 10:18:47 2024 +0000

    Bump pytest from 8.0.0 to 8.0.2 in /test

    Bumps [pytest](https://github.com/pytest-dev/pytest) from 8.0.0 to 8.0.2.
    - [Release notes](https://github.com/pytest-dev/pytest/releases)
    - [Changelog](https://github.com/pytest-dev/pytest/blob/main/CHANGELOG.rst)
    - [Commits](pytest-dev/pytest@8.0.0...8.0.2)

    ---
    updated-dependencies:
    - dependency-name: pytest
      dependency-type: direct:production
      update-type: version-update:semver-patch
    ...

    Signed-off-by: dependabot[bot] <support@github.com>

commit b57cf27
Merge: 3ba6ab5 5b75cb1
Author: yubiuser <ckoenig@posteo.de>
Date:   Sat Feb 24 13:44:20 2024 +0100

    Bump tox from 4.12.1 to 4.13.0 in /test (pi-hole#5581)

commit 5b75cb1
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Sat Feb 17 10:29:46 2024 +0000

    Bump tox from 4.12.1 to 4.13.0 in /test

    Bumps [tox](https://github.com/tox-dev/tox) from 4.12.1 to 4.13.0.
    - [Release notes](https://github.com/tox-dev/tox/releases)
    - [Changelog](https://github.com/tox-dev/tox/blob/main/docs/changelog.rst)
    - [Commits](tox-dev/tox@4.12.1...4.13.0)

    ---
    updated-dependencies:
    - dependency-name: tox
      dependency-type: direct:production
      update-type: version-update:semver-minor
    ...

    Signed-off-by: dependabot[bot] <support@github.com>

commit 3ba6ab5
Merge: 2009fa8 f0878c0
Author: yubiuser <ckoenig@posteo.de>
Date:   Mon Feb 5 13:53:45 2024 +0100

    Bump pytest from 7.4.4 to 8.0.0 in /test (pi-hole#5566)

commit f0878c0
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Sun Feb 4 21:39:17 2024 +0000

    Bump pytest from 7.4.4 to 8.0.0 in /test

    Bumps [pytest](https://github.com/pytest-dev/pytest) from 7.4.4 to 8.0.0.
    - [Release notes](https://github.com/pytest-dev/pytest/releases)
    - [Changelog](https://github.com/pytest-dev/pytest/blob/main/CHANGELOG.rst)
    - [Commits](pytest-dev/pytest@7.4.4...8.0.0)

    ---
    updated-dependencies:
    - dependency-name: pytest
      dependency-type: direct:production
      update-type: version-update:semver-major
    ...

    Signed-off-by: dependabot[bot] <support@github.com>

commit 2009fa8
Merge: 37c6b35 7b6f0d1
Author: Adam Warner <me@adamwarner.co.uk>
Date:   Sun Feb 4 21:36:37 2024 +0000

    Fedora (pi-hole#5568)

commit 7b6f0d1
Author: Christian König <ckoenig@posteo.de>
Date:   Sun Jan 28 16:56:57 2024 +0100

    Also remove Fedora 37

    Signed-off-by: Christian König <ckoenig@posteo.de>

commit f8bfd59
Author: Christian König <ckoenig@posteo.de>
Date:   Tue Nov 7 22:24:34 2023 +0100

    Drop Fedora 36 and add Fedora 39 to the test suite

    Signed-off-by: Christian König <ckoenig@posteo.de>

commit 37c6b35
Merge: e773d03 ba2682c
Author: yubiuser <ckoenig@posteo.de>
Date:   Sat Jan 20 12:23:35 2024 +0100

    Bump tox from 4.12.0 to 4.12.1 in /test (pi-hole#5555)

commit ba2682c
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Sat Jan 20 10:38:21 2024 +0000

    Bump tox from 4.12.0 to 4.12.1 in /test

    Bumps [tox](https://github.com/tox-dev/tox) from 4.12.0 to 4.12.1.
    - [Release notes](https://github.com/tox-dev/tox/releases)
    - [Changelog](https://github.com/tox-dev/tox/blob/main/docs/changelog.rst)
    - [Commits](tox-dev/tox@4.12.0...4.12.1)

    ---
    updated-dependencies:
    - dependency-name: tox
      dependency-type: direct:production
      update-type: version-update:semver-patch
    ...

    Signed-off-by: dependabot[bot] <support@github.com>

commit e773d03
Merge: aa4ceb4 9eb4731
Author: yubiuser <ckoenig@posteo.de>
Date:   Sat Jan 13 20:42:30 2024 +0100

    Bump tox from 4.11.4 to 4.12.0 in /test (pi-hole#5547)

commit 9eb4731
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Sat Jan 13 10:32:36 2024 +0000

    Bump tox from 4.11.4 to 4.12.0 in /test

    Bumps [tox](https://github.com/tox-dev/tox) from 4.11.4 to 4.12.0.
    - [Release notes](https://github.com/tox-dev/tox/releases)
    - [Changelog](https://github.com/tox-dev/tox/blob/main/docs/changelog.rst)
    - [Commits](tox-dev/tox@4.11.4...4.12.0)

    ---
    updated-dependencies:
    - dependency-name: tox
      dependency-type: direct:production
      update-type: version-update:semver-minor
    ...

    Signed-off-by: dependabot[bot] <support@github.com>

commit aa4ceb4
Merge: 7eb69a5 19bfa08
Author: Adam Warner <me@adamwarner.co.uk>
Date:   Sun Jan 7 21:17:15 2024 +0000

    Sync master back into development (pi-hole#5537)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants