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

[BUG] Any random hestiacp user (admin or not) can make nginx crash just by adding a wrong new domain #2055

Open
Faymir opened this issue Aug 13, 2021 · 11 comments
Labels
bug Something isn't working web Issues related to web components

Comments

@Faymir
Copy link
Contributor

Faymir commented Aug 13, 2021

Describe the bug
It seems that hestia cp doesn't check new provided nginx configuration validity before restarting it.
Even if a check is made (cf

service $WEB_SYSTEM configtest >> /dev/null 2>&1
), hestia still try to restart nginx without putting it back in it's "original" state
which results in errors like this:

nginx: [emerg] invalid server name or wildcard "jaap..nl" on x.x.x.x:80
nginx: configuration file /etc/nginx/nginx.conf test failed
Job for nginx.service failed because the control process exited with error code.
See "systemctl status nginx.service" and "journalctl -xe" for details.

So an error in the nginx conf can make all the server unavailable because nginx failed to restart .
cp panel, and all websites on the server unavailable because of one user.

To Reproduce

  1. Click on the "Web" tab.
  2. Click on "Add Web Domain".
  3. Enter test..com (<- notice 2 dots here)
  4. Click on save button

Expected behavior

  • The admin receive an email (based on the code in
    send_email_report
    )
  • An error is showed to the user (based on the code in
    check_result $E_RESTART "$WEB_SYSTEM restart failed"
    )
  • nginx is still started and working
  • all websites on the server are still reachable
  • hestia cpanel is still accessible

Screenshots

steps to reproduce and the error

Operating system:
Debian 10

Hestia Control Panel version:
hestia 1.4.9 and 1.4.10
only nginx + php-fpm (no apache)

Additional Context
systemctl status nginx.service result

● nginx.service - nginx - high performance web server
   Loaded: loaded (/lib/systemd/system/nginx.service; enabled; vendor preset: enabled)
   Active: failed (Result: exit-code) since Thu 2021-08-12 13:41:14 UTC; 6min ago
     Docs: https://nginx.org/en/docs/
  Process: 12752 ExecStart=/usr/sbin/nginx -c /etc/nginx/nginx.conf (code=exited, status=1/FAILURE)

Aug 12 13:41:13 example.com systemd[1]: Starting nginx - high performance web server...
Aug 12 13:41:14 example.com nginx[12752]: nginx: [emerg] invalid server name or wildcard "jaap..nl" on x.x.x.x:80
Aug 12 13:41:14 example.com systemd[1]: nginx.service: Control process exited, code=exited, status=1/FAILURE
Aug 12 13:41:14 example.com systemd[1]: nginx.service: Failed with result 'exit-code'.
Aug 12 13:41:14 example.com systemd[1]: Failed to start nginx - high performance web server.
@jaapmarcus jaapmarcus added bug Something isn't working web Issues related to web components labels Aug 19, 2021
@jaapmarcus
Copy link
Member

As discussed on Discord it is indeed a bug

@lastguru1
Copy link

Same happened to me this week. A user copied a domain name from somewhere, and it looked fine. Then all server has crashed, as there was some kind of invisible character in the copied domain name. He actually crashed my server twice, as the character was invisible for him, so he did not understand, what he did wrong... Hestia web did not show the domain lists afterwards and some invalid directories were made, showing the domain name as "piemers.\342\200\213lv". IDN must be accepted, though, so please do not filter out all non-latin characters...

@jaapmarcus
Copy link
Member

There is also a bug in:

if [[ $1 =~ $exclude ]] || [[ $1 =~ ^[0-9]+$ ]] || [[ $1 =~ "\.\." ]] || [[ $1 =~ "$(printf '\t')" ]] || [[ "$1" = "www" ]]; then

".." will be treated as literally instead of regex code changed that part.

The restore part after failing to add a domain should be fixed anyway..

@Faymir
Copy link
Contributor Author

Faymir commented Oct 19, 2021

Hello,
I am not sure to understand what you mean here:

".." will be treated as literally instead of regex code changed that part.

Could you elaborate please ?

@jaapmarcus
Copy link
Member

Currently domain..nu is valid. After we merge the new code it will be invalid..

@Faymir
Copy link
Contributor Author

Faymir commented Oct 19, 2021

I tryed replacing "/./." by ".." and it detects jaap..nl as invalid, so that could resolve the case of an invalid domain containing ".."
Maybe there are others cases not checked.

@jaapmarcus
Copy link
Member

The main issue is still present if a buggy template it still kills nginx / apache2 we still need to solve this part..

@sdelfi
Copy link

sdelfi commented Nov 12, 2021

The same problem.
I shared custom Nginx config with client. When he writes wrong config and saves webdomain via UI - Nginx restarts and all sites are down.

I tried to add "exit" here (/usr/local/hestia/bin/v-restart-web file):
image

But it doesn't work as well. HestiaCP is still trying to restart Nginx after configtest failed

@jaapmarcus
Copy link
Member

check_result already contains allready an exit.

Need to investigate the bug more...

@vertx-one
Copy link

Another one option to break HestiaCP is use IDN in domain alias field.
I kill fresh install HestiaCP 1.6.0 :)

@jaapmarcus
Copy link
Member

jaapmarcus commented Jun 20, 2022

Please create a new issue for this one. The other one need to be patched at long term

#2674

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working web Issues related to web components
Projects
None yet
Development

No branches or pull requests

5 participants