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

caddyhttp: Sanitize the path before evaluating path matchers #4407

Merged
merged 1 commit into from
Nov 8, 2021

Conversation

francislavoie
Copy link
Member

@francislavoie francislavoie commented Nov 8, 2021

I found an issue where it's possible to bypass path matchers by crafting a request with a path that doubles up slashes, or uses URL-encoded characters to smuggle the request past the path matcher.

This becomes a security issue, because auth gates using basicauth can be bypassed 😬

The solution is to unescape (convert URL-encoded characters) then clean the path, before matching.

This could be a breaking change if exact behaviour of the matcher is required (without cleaning the path). In that case, then it's possible to use the expression matcher to do a direct string comparison or regexp on the {http.request.uri.path} placeholder (or simply {path} in the Caddyfile). For example:

# Simple prefix match
expression `{path}.startsWith("/foo")`

# Complex regexp match
expression `{path}.matches("^/foo.*$")`

Example of an exploitable situation:

example.com {
	root * /srv

	basicauth /secret-files* {
		bob <hash>
	}

	file_server
}

It's expected that the path matcher /secret-files* will always require bob to be authenticated before they're let through to the file_server. But a request like //secret-files/uh-oh.txt would punch through unmatched, reaching file_server. The file_server handler does clean the path though (collapsing the doubled-up slashes), so it would serve /srv/secret-files/uh-oh.txt back to the client.

@francislavoie francislavoie added the bug 🐞 Something isn't working label Nov 8, 2021
@francislavoie francislavoie added this to the v2.4.6 milestone Nov 8, 2021
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for finding and fixing this. I'm hoping this won't break legitimate use cases, but I'm glad there's a workaround. Will release this today.

@mholt mholt merged commit e7457b4 into master Nov 8, 2021
@francislavoie francislavoie deleted the path-matching-safety branch November 8, 2021 20:45
francislavoie added a commit that referenced this pull request Dec 30, 2021
This is a followup to #4407, in response to a report on the forums: https://caddy.community/t/php-fastcgi-phishing-redirection/14542

Turns out that doing `TrimRight` to remove trailing dots, _before_ cleaning the path, will cause double-dots at the end of the path to not be cleaned away as they should. We should instead remove the dots _after_ cleaning.
francislavoie added a commit that referenced this pull request Dec 30, 2021
This is a followup to #4407, in response to a report on the forums: https://caddy.community/t/php-fastcgi-phishing-redirection/14542

Turns out that doing `TrimRight` to remove trailing dots, _before_ cleaning the path, will cause double-dots at the end of the path to not be cleaned away as they should. We should instead remove the dots _after_ cleaning.
@caddyserver caddyserver deleted a comment Jan 6, 2022
@caddyserver caddyserver deleted a comment Jan 6, 2022
nordstern pushed a commit to uptimerobot/caddy that referenced this pull request Jan 24, 2022
This is a followup to caddyserver#4407, in response to a report on the forums: https://caddy.community/t/php-fastcgi-phishing-redirection/14542

Turns out that doing `TrimRight` to remove trailing dots, _before_ cleaning the path, will cause double-dots at the end of the path to not be cleaned away as they should. We should instead remove the dots _after_ cleaning.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants