laravel / octane

Supercharge your Laravel application's performance.
https://laravel.com/docs/octane
MIT License
3.76k stars 293 forks source link

Proposed nginx configuration gives access to PHP files (sourcecode) in public folder #900

Closed colorando-de closed 4 months ago

colorando-de commented 4 months ago

Octane Version

2.3.11

Laravel Version

10.48.10

PHP Version

8.3

What server type are you using?

FrankenPHP

Server Version

1.1.5

Database Driver & Version

MariaDB 10.6

Description

When I try to access any php file in the public folder, like the frankenphp-worker.php, it downloads the PHP files sourcecode instead of parsing it. So https://app.domain.com/frankenphp-worker.php doesn't parse the php file but downloads the plain file.

It's unusual to have critical source code in the public folder. But if so, this would be a big security issue.

For the while I changed it to the following configuration to stop delivering PHP files at all, except of index.php:

    location = /index.php {
        try_files /not_exists @octane;
    }

    location ~ \.php$ {
        return 403;
    }

But in regular PHP setups, all PHP files are parsed by the PHP interpreter. Imho this should be the desired solution here, too.

Steps To Reproduce

I have a

server {
    server_name app.domain.com;
    root /var/www/app.domain.com/public;

    index index.php;

    add_header X-Frame-Options "SAMEORIGIN";
    add_header X-XSS-Protection "1; mode=block";
    add_header X-Content-Type-Options "nosniff";

    charset utf-8;
    client_max_body_size 10M;

    error_page 404 /index.php;

    access_log  /var/log/nginx/domain-octane.access.log;
    error_log  /var/log/nginx/domain-octane.error.log error;

    location /index.php {
        try_files /not_exists @octane;
    }

    location / {
        try_files $uri $uri/ @octane;
    }

    location = /favicon.ico { access_log off; log_not_found off; }
    location = /robots.txt  { access_log off; log_not_found off; }

    location @octane {
        set $suffix "";

        if ($uri = /index.php) {
            set $suffix ?$query_string;
        }

        proxy_http_version 1.1;
        proxy_set_header Host $http_host;
        proxy_set_header Scheme $scheme;
        proxy_set_header SERVER_PORT $server_port;
        proxy_set_header REMOTE_ADDR $remote_addr;
        proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
        proxy_set_header Upgrade $http_upgrade;
        proxy_set_header Connection "Upgrade";

        proxy_pass http://127.0.0.1:8000$suffix;
    }

    location ~ /\.(?!well-known).* {
        deny all;
    }

    location ~* (sw\.js)$ {
        add_header 'Cache-Control' 'no-store, no-cache, must-revalidate, proxy-revalidate, max-age=0';
        expires off;
        proxy_no_cache 1;
    }

    listen 443 ssl; # managed by Certbot
    ssl_certificate /etc/letsencrypt/live/app.domain.com/fullchain.pem; # managed by Certbot
    ssl_certificate_key /etc/letsencrypt/live/app.domain.com/privkey.pem; # managed by Certbot
    include /etc/letsencrypt/options-ssl-nginx.conf; # managed by Certbot
    ssl_dhparam /etc/letsencrypt/ssl-dhparams.pem; # managed by Certbot
}
fideloper commented 4 months ago

Hi there!

That's an interesting point, thanks -

It seems like the main change is this part, correct?

location ~ \.php$ {
    return 403;
 }

Documentation Chagne

Depending on what we're aiming for, there's various (kind of gross) changes to the Nginx configuration we can go for.

I think as a default, blocking all but the index.php file makes sense. If people want access to other PHP files, they may need to handle that as a special case (Not a hill I'll die on, but I think it makes sense to document something more secure).

Assuming I've understood your point correctly, I'm agreeing that the addition of this might make sense:

location ~ \.php$ {
    return 403;
 }

An alternative

An alternative configuration that's uglier/harder to document would be to serve any *.php file as normal, which looks a bit like this (not yet tested):

# Example file: /etc/nginx/octane.conf

set $suffix "";

if ($uri = /index.php) {
    set $suffix ?$query_string;
}

proxy_http_version 1.1;
proxy_set_header Host $http_host;
proxy_set_header Scheme $scheme;
proxy_set_header SERVER_PORT $server_port;
proxy_set_header REMOTE_ADDR $remote_addr;
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
proxy_set_header Upgrade $http_upgrade;
proxy_set_header Connection "Upgrade";

proxy_pass http://127.0.0.1:8000$suffix;

##########################################################

# Example File /etc/nginx/sites-enabled/laravel.conf
map $http_upgrade $connection_upgrade {
    default upgrade;
    ''      close;
}

server {
    listen 80;
    listen [::]:80;
    server_name domain.com;
    server_tokens off;
    root /home/forge/domain.com/public;

    index index.php;

    charset utf-8;

    location / {
        try_files $uri $uri/ @octane;
    }

    location = /favicon.ico { access_log off; log_not_found off; }
    location = /robots.txt  { access_log off; log_not_found off; }

    access_log off;
    error_log  /var/log/nginx/domain.com-error.log error;

    error_page 404 /index.php;

    # Capture direct requests to .php files
    location ~ \.php$ {
        include octane.conf;
    }

    location @octane {
        include octane.conf;
    }
}
driesvints commented 4 months ago

Wondering if frankenphp-worker.php needs to be accessible... @dunglas do you might know that?

colorando-de commented 4 months ago

@driesvints Our Laravel application with up to 400 users simulateously works great with octane, having access blocked to the frankenphp-worker.php. But this is only my experience. I don't know the reason why frankenphp-workper.php lives in the public directory.

dunglas commented 4 months ago

@driesvints Technically, it may be possible to store the worker outside of the public directory using some trickery, but internally FrankenPHP works like that: if the requested URL (after rewriting, etc) matches a running worker, FrankenPHP uses this worker to handle the request ; if no worker match, then it executes the PHP script as usual.

The feature has been designed like this to be able to support easily FrankenPHP (in worker mode) and more traditional setups like FPM in the same app (as does for instance Symfony by always calling index.php).

I see two ways to not expose frankenphp-worker.php publicly:

  1. move the worker code in index.php (as Symfony does): this would be the cleaner way, but I'm not sure if it is acceptable for you because we'll have to at least detect if Octane is used in index.php (which is part of the standard Laravel install, not of Octane) to include the worker script (that can stay in the vendors).
  2. use some Caddyfile magic to rewrite the internal URL and do as if the script was present even if it isn't. This is maybe possible (@francislavoie may have ideas regarding how to do that), but I need to dig deeper into that, and this would make the Caddyfile less readable.

wdyt?

driesvints commented 4 months ago

@dunglas I think frankenphp-worker.php in the public dir is fine, no worries there. Just want to make sure that if we adapt the example Nginx file in the docs everything still works as expected.

dunglas commented 4 months ago

@driesvints sure, the docs update is ok and shouldn't hurt.

driesvints commented 4 months ago

@colorando-de could you send in the docs update proposed by @fideloper? Thanks all for helping out here 👍

francislavoie commented 4 months ago

Since I was tagged here; IMO the problem is one that simply wouldn't exist if you just didn't use Nginx. If you're using FrankenPHP, you don't need Nginx at all, because you have the full power of Caddy at your disposal. You could just delete certbot and Nginx, then let Caddy do its thing, doing TLS automation, serving PHP, etc.

I think the Octane docs should recommend this approach instead of using Apache/Nginx as a proxy, when using FrankenPHP.

But anyway, there are some creative ways FrankenPHP could handle working without the frankenphp-worker.php file existing in the actual filesystem; Caddy since v2.8.0 has an fs directive which allows configuring a virtual filesystem to use, FrankenPHP could bundle a virtual FS module which intercepts lookups for frankenphp-worker.php and loads it from memory instead of from file, and otherwise any other file lookup would be deferred to the OS filesystem as normal. But that's like... but why? That's a solution looking for a problem.