matcher: Add split_path option to file matcher; used in php_fastcgi

[francislavoie]

Discussion: https://caddy.community/t/help-to-migrate-caddyfile-v1-to-v2-for-nextcloud/7647/18

A common pattern in PHP apps is to have a PHP file that acts as a router entrypoint to the application. This is typically the index.php file at the webroot or some other subpath of the application. These entrypoints typically accept requests that have a path appended to the filename, for example /index.php/blog/123.

Some PHP apps happen to have more than one router entrypoint, for example, /router.php/some/path, in addition to the index.php file. NextCloud is one of those applications; it has remote.php, public.php and more that act as entrypoints.

With the default php_fastcgi Caddyfile directive, many of those pages didn't work correctly in NextCloud. The problem has to do with the try_files matcher and rewrite that is included as part of the php_fastcgi shortcut:

try_files {path} {path}/index.php index.php

The issue is that on a request to the path /remote.php/dav/files, Caddy would check if that exact path exists as a file on disk (i.e. the first argument, {path}), but this doesn't work because there's extra stuff after the actual filename.

We already have an option in the reverse_proxy's fastcgi transport to allow splitting the path into two parts, with the left-hand side being the filename (including the split value), and the right-hand side being the path, i.e. split .php.

The fix we landed on for this issue is to add a new split option to the file matcher (split_path in JSON) that allows for splitting the path the same way, before testing if the file exists on disk. It looks like this:

@phpFiles {
    file {
        split .php
        try_files {path} {path}/index.php index.php
    }
}

So now for a request to /remote.php/dav/files, Caddy will instead look for /remote.php on disk while matching.

The php_fastcgi shortcut is also updated to internally add this new split option for its try_files matcher. This should not be a breaking change, because we assume that any path that involved .php in the middle of it was assumed to be the filename boundary, but this wasn't working as expected.

[francislavoie]

@mholt I'll let you pick the milestone, not sure when you think this should go in.

[mholt]

This looks good! Nice work, thanks.

Do you think we need the case-insensitivity? Are there cases where case-sensitive split is useful?

[francislavoie]

I kept that mostly to match the reverse_proxy behaviour. I figure if a request to /Remote.PHP/some/path comes in, it should still work.

[mholt]

I'd be willing to merge this in before 2.0 if the original reporters of the bug confirm that it works for them.

[tobya]

We had some issues in 1 around split_path for php files and incorrect matches. do you think there is any possibility this will suffer from similar issues.

eg.

Would it be possible to upload a file

myhackfile.php.txt and have it executed as myhackfile.php?

This would obviously be a security issue.

[francislavoie]

Would it be possible to upload a file myhackfile.php.txt and have it executed as myhackfile.php?

No, because try_files would look for myhackfile.php on disk and not find it, so it would serve the path myhackfile.php.txt through index.php instead (which would probably 404, but it depends on what index.php does, which we don't control).

But, this does mean this change would break requests to files that have .php in the middle of them though.

Hmm. Should we only return the split path if the next character after the split is / or is the end of the string?

[francislavoie]

Okay I tried adding this bit just before the return in the loop:

			// skip the split if it's not the final part of the filename
			if pos != len(path) && ! strings.HasPrefix(path[pos:], "/") {
				continue
			}

Caddyfile:

:8886

root ./test

@try {
  file {
    split .php
    try_files {path} {path}/index.php index.php
  }
}
rewrite @try {http.matchers.file.relative}

respond "Path: {path} | Relative: {http.matchers.file.relative}"

Tree:

.
├── caddy
├── main.go
└── test
    ├── index.php
    ├── myhackfile.php.txt
    └── remote.php

Tests:

$ curl localhost:8886
Path: /index.php | Relative: /index.php

$ curl localhost:8886/index.php
Path: /index.php | Relative: /index.php

$ curl localhost:8886/index.php/somewhere
Path: /index.php | Relative: /index.php

$ curl localhost:8886/remote.php
Path: /remote.php | Relative: /remote.php

$ curl localhost:8886/remote.php/somewhere
Path: /remote.php | Relative: /remote.php

$ curl localhost:8886/myhackfile.php
Path: index.php | Relative: index.php

$ curl localhost:8886/myhackfile.php.txt
Path: /myhackfile.php.txt | Relative: /myhackfile.php.txt

$ curl localhost:8886/myhackfile.php.txtxt
Path: index.php | Relative: index.php

$ curl localhost:8886/myhackfile.php.txt/
Path: /myhackfile.php.txt | Relative: /myhackfile.php.txt

$ curl localhost:8886/myhackfile.php.txt/somewhere
Path: index.php | Relative: index.php

With that change, as you can see, it allows direct requests to myhackfile.php.txt (or if the file is followed by / which I think is harmless? Hmm.) but not if followed by anything else.

Edit: regarding "or if the file is followed by /", turns out that is the existing behaviour, so that doesn't have to do with the split

[francislavoie]

In other news, we got word from the user on the forums that this change fixed the problem for them with NextCloud:

https://caddy.community/t/help-to-migrate-caddyfile-v1-to-v2-for-nextcloud/7647/25?u=francislavoie

[mholt]

@tobya You are referring to this issue? #2917

[mholt]

Are untrusted users ever able to upload directories?

$ curl localhost:8886/myhackfile.php.txt/
Path: /myhackfile.php.txt | Relative: /myhackfile.php.txt

^ that test, if a user uploads a file in a directory like myhackfile.php.txt/index.php, would a request to /myhackfile.php.txt invoke the index.php file within it?

[francislavoie]

At that point it's not Caddy's problem, it's the app's. If they allow things like that, then it's an insecure app. I don't think we need to worry about it.

[mholt]

Not that I disagree with you, but, allow me to play devil's advocate here, since I know we will get complainers if we don't drill down ahead of them: is there a reason that is the app's fault but not allowing file uploads with filename like foo.php.php? Where do we draw the line between app responsibility and Caddy's responsibility?

[francislavoie]

I added a file as described on disk:

$ tree
.
├── caddy
├── main.go
└── test
    ├── foo.php.php
    │   └── index.php
    ├── index.php
    ├── myhackfile.php.txt
    ├── nonphpfile.txt
    └── remote.php

And tried requesting it (same Caddyfile as above):

$ curl localhost:8886/foo.php.php/index.php
Path: /foo.php.php/index.php | Relative: /foo.php.php/index.php

It does serve the file as-is. This is because after stripping (i.e. /foo.php, the path is not the same length, and it doesn't end in /. This is totally fine, this is the expected case. The user may have a legitimate use-case for a file named that way, and we can't know whether it was an uploaded file or an intended file on disk.

Also to note, here is the matcher in the Caddyfile above vs what is now generated by the php_fastcgi directive (just to show that the only difference is "group", which is just an implementation detail, irrelevant here):

Caddyfile:
            {
              "group": "group0",
              "match": [
                {
                  "file": {
                    "try_files": [
                      "{http.request.uri.path}",
                      "{http.request.uri.path}/index.php",
                      "index.php"
                    ],
                    "split_path": [
                      ".php"
                    ]
                  }
                }
              ],
              "handle": [
                {
                  "handler": "rewrite",
                  "uri": "{http.matchers.file.relative}"
                }
              ]
            },
php_fastcgi:
            {
              "match": [
                {
                  "path": [
                    "*.php"
                  ]
                }
              ],
              "handle": [
                {
                  "handler": "reverse_proxy",
                  "transport": {
                    "protocol": "fastcgi",
                    "split_path": [
                      ".php"
                    ]
                  },
                  "upstreams": [
                    {
                      "dial": "some:9000"
                    }
                  ]
                }
              ]
            }

Then tried with this Caddyfile (PHP 7.4), each file just has their own filename in it:

:8886

root ./test

php_fastcgi unix//run/php/php7.4-fpm.sock

Tests

$ curl localhost:8886/index.php
index

$ curl localhost:8886/index.php/somewhere
index

$ curl localhost:8886/remote.php
remote

$ curl localhost:8886/remote.php/somewhere
remote

$ curl localhost:8886/myhackfile.php
index

$ curl localhost:8886/myhackfile.php.txt

$ curl localhost:8886/myhackfile.php.txtxt
index

$ curl localhost:8886/myhackfile.php.txt/

$ curl localhost:8886/myhackfile.php.txt/somewhere
index

$ curl localhost:8886/foo.php.php/index.php
File not found.

You'll note that /myhackfile.php.txt and /myhackfile.php.txt/ return empty, this is because PHP decides to not handle it as a PHP file.

You'll also note that localhost:8886/foo.php.php/index.php returns a 404, this is because Caddy doesn't give valid values for these vars: "SCRIPT_FILENAME":"/home/francis/repos/caddy/cmd/caddy/test/foo.php","SCRIPT_NAME":"/foo.php" because of the split logic in the fastcgi transport.

Debug logs for curl localhost:8886/foo.php.php/index.php:

2020/04/23 19:06:04.469 DEBUG   http.reverse_proxy.transport.fastcgi    roundtrip       {"request": {"method": "GET", "uri": "/foo.php.php/index.php", "proto": "HTTP/1.1", "remote_addr": "127.0.0.1:59990", "host": "localhost:8886", "headers": {"X-Forwarded-For": ["127.0.0.1"], "User-Agent": ["curl/7.58.0"], "Accept": ["*/*"], "X-Forwarded-Proto": ["http"]}}, "dial": "/run/php/php7.4-fpm.sock", "env": {"AUTH_TYPE":"","CONTENT_LENGTH":"","CONTENT_TYPE":"","DOCUMENT_ROOT":"/home/francis/repos/caddy/cmd/caddy/test","DOCUMENT_URI":"/foo.php","GATEWAY_INTERFACE":"CGI/1.1","HTTP_ACCEPT":"*/*","HTTP_HOST":"localhost:8886","HTTP_USER_AGENT":"curl/7.58.0","HTTP_X_FORWARDED_FOR":"127.0.0.1","HTTP_X_FORWARDED_PROTO":"http","PATH_INFO":".php/index.php","PATH_TRANSLATED":"/home/francis/repos/caddy/cmd/caddy/test/.php/index.php","QUERY_STRING":"","REMOTE_ADDR":"127.0.0.1","REMOTE_HOST":"127.0.0.1","REMOTE_IDENT":"","REMOTE_PORT":"59990","REMOTE_USER":"","REQUEST_METHOD":"GET","REQUEST_SCHEME":"http","REQUEST_URI":"/foo.php.php/index.php","SCRIPT_FILENAME":"/home/francis/repos/caddy/cmd/caddy/test/foo.php","SCRIPT_NAME":"/foo.php","SERVER_NAME":"localhost","SERVER_PORT":"8886","SERVER_PROTOCOL":"HTTP/1.1","SERVER_SOFTWARE":"Caddy/(devel)"}}
2020/04/23 19:06:04.469 DEBUG   http.handlers.reverse_proxy     upstream roundtrip      {"upstream": "unix//run/php/php7.4-fpm.sock", "request": {"method": "GET", "uri": "/foo.php.php/index.php", "proto": "HTTP/1.1", "remote_addr": "127.0.0.1:59990", "host": "localhost:8886", "headers": {"X-Forwarded-For": ["127.0.0.1"], "User-Agent": ["curl/7.58.0"], "Accept": ["*/*"], "X-Forwarded-Proto": ["http"]}}, "headers": {"Status": ["404 Not Found"], "Content-Type": ["text/html; charset=UTF-8"]}, "duration": 0.0003673, "status": 404}

I think we're safe here, I don't see any issues. I still want @whitestrake's input though!

[francislavoie]

Okay sorry about that - I edited the comment above, I made a booboo and I ran all of that under the wrong code 🤦‍♂️

[francislavoie]

Hey @sarge I wouldn't mind your help on this one to add matcher tests similarly to those curl requests I did above! I'm not sure where that should go or how to set up the tests

[francislavoie]

Don't mind the change to .github/CONTRIBUTING.md... that was just a dos2unix line endings change. I mucked up the file endings when I made the file the other day. The joys of working on Windows.

[whitestrake]

@mholt

^ that test, if a user uploads a file in a directory like myhackfile.php.txt/index.php, would a request to /myhackfile.php.txt invoke the index.php file within it?

I believe this is already a problem and beyond Caddy's scope.

The name of the directory beneath it doesn't matter. If someone can upload index.php to any webdir, and call that directory to get its index, whoever allowed that upload has failed to secure their server and application. It has nothing to do on whether we split on .php or not.

Re: where is the line between app and Caddy's responsibility? I propose that line should be drawn exactly where the function is that allows uploads in the first place, and Caddy's PHP/FastCGI functionality is not strictly providing the upload capability. In almost all cases, I expect that upload to be conceptually out of band, processed by completely different code beyond Caddy's control or concern.

It's not Caddy's responsibility to decide, once a file is already on disk, which files should be allowed to be executed as PHP. That responsibility comes down to the person who arranged/allowed for the files to be there; usually either the app maintainer or the server administrator.

Trying to use that line gets blurry, I suppose, if we talk about introducing functionality like WebDAV etc. built into Caddy. But I think there's an expectation that those kinds of things should be authenticated first... Combining free upload with PHP execution is something that must be voluntarily done by the server administrator. Those are powerful tools and a possible footgun, but without getting potentially crazy heuristics involved, I don't think we can or should check or aim to prevent this - that's the point I think you gotta say it's on the guy configuring things.

My only concern at a glance here is that a legitimate request to a file with .php somewhere in the middle of the file name will be returned to index inappropriately. But in your firstSplit function you already check that the .php must be at the end of the path element / file name. So I'm happy with that.

After looking over this, it seems I can't think of any other way to exploit it, hence the approval. As a disclaimer, though - my lack of imagination shouldn't be taken as authority on what is possible, heh.

[francislavoie]

Perfect! Thanks @whitestrake that's essentially what I expected to hear 😄 Just wanted someone to check my sanity here ❤️

[francislavoie]

Bah, I hate that Github dismisses reviews after rebasing... but I rebased. This is ready to go @mholt.

[mholt]

I think I can turn the auto-dismissal off, but I specifically want it on for PRs to the master branch to make sure something doesn't slip in too easily. No worries.