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

Pihole 6 webserver exposes code from other files on regular webserver #2349

Open
Tone866 opened this issue Mar 6, 2025 · 12 comments
Open

Pihole 6 webserver exposes code from other files on regular webserver #2349

Tone866 opened this issue Mar 6, 2025 · 12 comments

Comments

@Tone866
Copy link

Tone866 commented Mar 6, 2025

Versions

  • Pi-hole: 6.0.5
  • AdminLTE: 6.0.2
  • FTL: 6.0.4

Platform

  • OS and version: Ubuntu 24.04
  • Platform: x86 bare metal

Expected behavior

the pihole webserver should only display or execute files in the defined subpath

Actual behavior / bug

default webroot is a /var/www/html/ so if I open the pihole webserver over https://127.0.0.1:8443 it exposes the code of my index.php. This happens to all files on the webserver.
The pihole webserver should be blocking everything, which isn't in the defined subpath or redirect to the subpath.

Steps to reproduce

Steps to reproduce the behavior:

  1. open the pihole webserver without subpath
@yubiuser yubiuser transferred this issue from pi-hole/pi-hole Mar 7, 2025
@DL6ER
Copy link
Member

DL6ER commented Mar 7, 2025

This collides with other (older) feature requests where users are explicitly asking to serve (static and Lua server-page) files hosted in /var/www/html. What should we do? I guess this leads to a new webserver boolean that can be set to false with the effect to limit serving content to /admin/ and /api/

@yubiuser
Copy link
Member

yubiuser commented Mar 7, 2025

My expecation would be to only serve /api/ and /admin/ by default and leaving everything else in / var/www/html untouched unless some new boolean would be set to true.

@Tone866
Copy link
Author

Tone866 commented Mar 7, 2025

This collides with other (older) feature requests where users are explicitly asking to serve (static and Lua server-page) files hosted in /var/www/html.

This is a webserver specific for the service pihole.
If someone wants to host other files on it, in my opinion this is out of scope.

What should we do? I guess this leads to a new webserver boolean that can be set to false with the effect to limit serving content to /admin/ and /api/

Yeah, something like that would be great, but in my opinion it should really be the default, to only serve /admin and /api.
There are much people using pihole, which aren't that deep into configuring that stuff or just following some youtube-tutorials and this could be a potential security risk for them. Maybe not for pihole itself, but for there other services.

@Hk1020
Copy link

Hk1020 commented Mar 7, 2025

This is the same problem that I reported in #2327

I agree with @Tone866 that this is a potential security risk. Pihole should not assume that it owns /var/www/html. If pihole wants to do its own stuff now it should do so from its own directory. But I don‘t know how universal /var/www/html is.

@DL6ER
Copy link
Member

DL6ER commented Mar 7, 2025

Pihole should not assume that it owns /var/www/html

I am concerned about preserving existing behavior, whether this may be good or bad. To avoid breaking established "abuses" of the webserver for whatever reasons the users may done this. My point is that Pi-hole was serving this directory through lighttpd for years.

My proposal is, hence,

  • add an option to only serve /admin/ and /api/ defaulting to off for aforementioned reasons to not break longstanding practice, and
  • stop to serve any files, regardless where they live, ending in a list of forbidden endings (php[0-9]*$, pl[c-d]*$, cgi$, htaccess$, htpasswd$, ht$) as they should be interpreted but we are not doing this.

The response to such forbidden request could either be 403 Forbidden or maybe 404 Not Found to conceal the mere existence of such files on the webserver - even when the latter may be confusing. I do not want to send a 5xx Server Error message here as there is no server error but a user configuration preventing this from being served.

@Tone866
Copy link
Author

Tone866 commented Mar 7, 2025

But in the state it is in now, it doesn't work either.
E.g. php isn‘t executed, so even if users used lighttp for other stuff, it still isn‘t working anymore with v6.

@PromoFaux
Copy link
Member

I am concerned about preserving existing behavior,

I'm in the "default to false" camp on this one. Unless explicitly asked to by the user, we should only serve the admin interface and the API. I'd probably hazard that only a minority of people would be affected by this either way.

I thought I saw a PR somewhere to fix the update scripts to enable them to make proper use of webserver.paths.webroot and webserver.paths.webhome across updates, but I cannot see it.

Maybe we could even consider changing the webroot by default anyway /opt/pihole/admin, perhaps - or whatever the proper locations are to store these things - I know next to nothing about these things.

@DL6ER
Copy link
Member

DL6ER commented Mar 7, 2025

Maybe we could even consider changing the webroot by default anyway /opt/pihole/admin, perhaps - or whatever the proper locations are to store these things - I know next to nothing about these things.

We'd need to move the repo over.

@PromoFaux
Copy link
Member

My point is that Pi-hole was serving this directory through lighttpd for years.

But this is a fair point. In v5, we just happened to be using a different web server. Often times people would also have apache or something installed for some other service - which was also pointing at /var/www/html/... and both lighttpd and apache would have been able to see "each other's" files.

We'd need to move the repo over.

Well... yes, but I figured that much was obviously implied in what I was typing 😉

@DL6ER
Copy link
Member

DL6ER commented Mar 7, 2025

Even though I am still not really convinced, I am also not going to insist on my own opinion if I'm the only one here. Please play with branch fix/webserver_serving to see if this does what you'd expect it to. The new option webserver.serve_all defaults to false and can be set at runtime without triggering a restart of FTL.

pihole.toml snippet:

  # Should the web server serve all files in webserver.paths.webroot directory? If
  # disabled, only files within the path defined through webserver.paths.webhome and
  # /api will be served.
  serve_all = false

@dschaper
Copy link
Member

dschaper commented Mar 7, 2025

This is a webserver specific for the service pihole.
If someone wants to host other files on it, in my opinion this is out of scope.

The ability to serve pages that extend Pi-hole/FTL through the embedded lua (lsp) engine was intentional. The ability to serve non-extension pages unrelated to Pi-hole/FTL was not intentional. (I can speak authoritatively here because I was the one that asked for the behavior.)

  • add an option to only serve /admin/ and /api/ defaulting to off for aforementioned reasons to not break longstanding practice, and

Sounds reasonable, it is not a stretch to ask users availing themselves of Pi-hole/FTL extended pages to enable the server feature. If this changes a longstanding practice of serving pages by default then we need to consider calling it a 6.1 release.

Edit: Sorry, I think we should not serve pages by default.

  • stop to serve any files, regardless where they live, ending in a list of forbidden endings (php[0-9]*$, pl[c-d]*$, cgi$, htaccess$, htpasswd$, ht$) as they should be interpreted but we are not doing this.

It would probably be easier to whitelist the extensions or mime types of files that are allowed.

Maybe we could even consider changing the webroot by default anyway /opt/pihole/admin

Oh crap, not LHFS again...

Copy link

github-actions bot commented Apr 7, 2025

This issue is stale because it has been open 30 days with no activity. Please comment or update this issue or it will be closed in 5 days.

@github-actions github-actions bot added the stale label Apr 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants