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

Directory traversal in FileStorage (with on-demand TLS) #2092

Closed
clayton-shopify opened this Issue Mar 28, 2018 · 2 comments

Comments

2 participants
@clayton-shopify
Copy link

clayton-shopify commented Mar 28, 2018

1. What version of Caddy are you using (caddy -version)?

Caddy 0.10.11 (non-commercial use only)

2. What are you trying to do?

Test on-demand TLS for potential security issues.

3. What is your entire Caddyfile?

:443 {
  tls {
    max_certs 10
  }
}

4. How did you run Caddy (give the full command and describe the execution environment)?

caddy

5. Please paste any relevant HTTP request(s) here.

Connect to the server with the following OpenSSL command:

openssl s_client -servername a/../../../../../foo -connect localhost:443

6. What did you expect to see?

I would expect this request to be rejected due to the invalid hostname.

7. What did you see instead (give full error messages and/or log)?

A directory named foo was created outside the .caddy directory.

8. How can someone who is starting from scratch reproduce the bug as minimally as possible?

Run the OpenSSL command above and observe that a directory was unexpectedly created.

There is already some hostname validation in the HostQualifies function, so that might be the right place to put additional validation.

@mholt mholt self-assigned this Mar 28, 2018

@mholt mholt closed this in 73b61af Mar 28, 2018

@mholt mholt removed the in progress label Mar 28, 2018

@mholt

This comment has been minimized.

Copy link
Owner

mholt commented Mar 28, 2018

Clever -- thanks for the report. Fortunately, this is largely mitigated if you don't run Caddy as root (and I think this is the first vulnerability we've had where not running as root can explicitly reduce the problem, due to interactions with the file system).

This is a recent regression, affecting only v0.10.11 and v0.10.12. The distributed solving of the ACME challenge places a lock file on disk which gets cleaned up after the challenge completes (whether it fails or not -- in these cases, it obviously fails) but with the commit I just pushed we now clean up any empty parent folder it may have created for the sake of the lock file. We also sanitize file names -- that wasn't necessary before since all files written would have already been verified by the certificate authority, so they must have been normal domain names.

HostQualifies is perhaps a good place to do more validation in the future, though I want to be careful that that function only returns false if the name is expressly forbidden from being used for managed TLS, not as a hostname in general.

Thanks again! Stay tuned for a new release soon.

@mholt

This comment has been minimized.

Copy link
Owner

mholt commented Mar 28, 2018

Can you pull the latest master and verify that this fixes it for you? I will do a release after that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment