Caddy: Multiple forward slashes in paths

[hstaugaard]

Multiple forward slashes are not collapsed in paths

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

Caddy 0.10.7

2. What are you trying to do?

Use path matches with status, rewrite and internal.

3. What is your entire Caddyfile?

:8001 {
    status 401 /secret/files
}

:8002 {
    rewrite /secret/files {
        regexp ^/(.*)
        to /notsecret/files/{1}
    }
}

:8003 {
    internal /secret/files
}

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

caddy with no arguments, Windows 64 bit

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

• curl localhost:8001/secret/files/file.txt returns 401 as expected
• curl localhost:8001//secret//files/file.txt serves the secret file (all variations of multiple slashes serves the file.
• curl localhost:8002/secret/files/file.txt serves the public file after rewriting, as expected
• curl localhost:8002/secret//files/file.txt serves the secret file
• curl localhost:8003/secret/files/file.txt returns 404 as expected
• curl localhost:8003/secret//files/file.txt serves the secret file.

6. What did you expect to see?

That multiple slashes in the path was collapsed or matched by the path expressions.

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

They were not

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

I have attached a small test site as a zip.
caddy_bug.zip

[magikstm]

Thanks a lot for reporting the issue.

I tested and it's a critical vulnerability for the loginsrv plugin: https://github.com/tarent/loginsrv

It may have unwanted negative effects on other plugins too.

I think this should be corrected ASAP.

[smancke]

I tested the examples from @magikstm and can confirm, that resources, which are protected by the caddy-jwt plugin of @BtBurke are free accessible with multiple slashed. So, this is a critical security issue, here.

[BTBurke]

Confirmed. I just pushed a fix for the JWT plugin. The issue has to do with how net/url handles parsing malformed paths with multiple path separators. If you type a malformed path in Chrome, it will happily eat the extra path separator and continue on its merry way.

This kind of loose path matching works fine, except when you're talking about protected resources. It causes net/url to fail to parse the correct path. The only way I found to detect this is by checking the result of r.URL.EscapedPath() to be empty. That will prevent a top level double-slash such as http://example.com//protected from failing to match a specified path. For double path separators elsewhere in the URL, like http://example.com/protected//secret, a regex replace for 1 or more / works to normalize the path.

The main issue deals with the implementation of httpserver.Path().Matches() in Caddy. It is pretty loose in the definition of a "match" which should be fine for most situations, but is less desirable from a security point of view. This multiple path separator behavior may affect other core caddy functions like proxy directives, but I haven't tested it.

If the upstream httpserver package is changed to have a more sophisticated path matcher, then I can remove the hack I put in to clean up URLs. It might be worth adding a httpserver.Path().MatchStrict() function that deals with all the ways someone can send a malformed URL. For example, it appears that net/url does not distinguish between URL-encoded path separators and the real thing, so there might be vulnerabilities related to funky inputs like http://example.com/%2Fprotected%2Fsecret.

I plan on looking at some options to fuzz the test inputs this weekend to look for other problems with path matching.

[whitestrake]

@mholt has raised the possibility of doing path cleaning in Path.Matches():

https://godoc.org/github.com/mholt/caddy/caddyhttp/httpserver#Path.Matches

If this covers the affected middlewares, it would be a good way to resolve this issue in a contained manner without creating additional concerns (such as breaking legitimate double-forwardslash usage elsewhere - for reference: https://github.com/mholt/caddy/commit/d5371aff2265540fb8600541bfdf3c3c4fc963e4 and https://github.com/mholt/caddy/issues/1298).

Edit: Beaten by @BTBurke!

[magikstm]

@BTBurke I tested and the latest JWT commit corrects vulnerabilities I mentioned in loginsrv.

I just tested and it also affects caddy's http.basicauth (https://caddyserver.com/docs/basicauth)

It can be tested with this minimal caddyfile:

http://localhost:8080 {
  basicauth /2.jpg username password
}

The following url passes through htpasswd: http://localhost:8080//2.jpg

[BTBurke]

@magikstm thanks for testing that. I was just looking at the behavior for a double forward slash at the root of the path. The issue is that path.Clean() from the standard library does not replace the // with a / when it's at the root of the path.

The way I'm getting around this is to check for a malformed URL at the outset, then cleaning the path, then doing the match

if r.URL.EscapedPath() == "" {
    // return 401 on a malformed URL (for security-focused plugins)
}
cleanedPath := path.Clean(r.URL.Path)
if !httpserver.Path(cleanedPath).Matches(protected.Path) {
    // not a match, let it through
}
// do authorization stuff

[magikstm]

I just tested the http.reauth plugin by @freman.

Ref:
https://caddyserver.com/docs/http.reauth
https://github.com/freman/caddy-reauth

It is also vulnerable.

I tested with this minimal caddyfile:

http://localhost:8080 {
reauth {
    path /v2
    simple username=password,root=badpractice
}
}

The following url passes through its security: http://localhost:8080//v2/2.jpg

[magikstm]

I just tested http.authz by @hsluoyz and it is vulnerable too.

Ref:
https://caddyserver.com/docs/http.authz
https://github.com/casbin/caddy-authz)

I tested with this minimal caddyfile:

http://localhost:8080 {
authz "model.conf" "policy.csv"
}

If you want to reproduce fully. I used these for model.conf and policy.csv.

model.conf:

[request_definition]
r = sub, obj, act

[policy_definition]
p = sub, obj, act

[role_definition]
g = _, _

[policy_effect]
e = some(where (p.eft == allow))

[matchers]
m = g(r.sub, p.sub) && keyMatch(r.obj, p.obj) && (r.act == p.act || p.act == "*")

policy.csv:
p, alice, /v2/*, GET

The following url passes through its security: http://localhost:8080//v2/2.jpg

[magikstm]

I unfortunately don't have the required dependencies to test them at the moment, but I think these two may also be vulnerable and may need to be tested to confirm they're not after correction.

1) http.restic by @fd0.
Ref:
https://caddyserver.com/docs/http.restic
https://github.com/restic/caddy

2) http.webdav by @hacdias.
Ref:
https://caddyserver.com/docs/http.webdav
https://github.com/hacdias/caddy-webdav

There may be other minor issues with other plugins, but I tested "security" ones first.

[whitestrake]

Middlewares like these would be fixed with a change toPath.Matches(), but http.authz looks like it sends the path onwards to the casbin.Enforcer without bothering to match or sanitize: https://github.com/casbin/caddy-authz/blob/e0ddc63c8d491486d44d1a055fd5277e23135e70/authz.go#L84

[mholt]

Thanks everyone, for investigating this while I've been away from the computer. I have been following along while mobile. Fantastic response so far!

For some background on how Caddy has handled sanitizing/cleaning/manipulating paths, see:

• https://github.com/mholt/caddy/issues/1298 (<-- collapsing repeated slashes caused numerous bugs and problems)
• https://github.com/mholt/caddy/pull/1633 (<-- related, maybe relevant, but slightly different, as this focused on a different way of preventing directory traversal)

I think those two issues summarize it best, but there might be a few other misc. issues/commits that are related if you search for them.

At this point, I'm really keen on containing the fix within Path.Matches() if possible. I believe that will mend up _most_ of the problem cases, except for middlewares that don't use it... surely there are only a few of those?

Still looking into this.

[mholt]

@BTBurke

The issue is that path.Clean() from the standard library does not replace the // with a / when it's at the root of the path.

I'm not seeing that:

package main

import (
    "fmt"
    "path"
)

func main() {
    fmt.Println(path.Clean("//test")) // "/test"
}

[BTBurke]

Try it with an r.URL.Path and what I see is that the Path struct does not return a valid path for the double forward slash at the root. Whatever extraction is being done in the standard library as part of processing the http.Request doesnt appear to give you a valid string under r.URL.Path that you can then send to path.Clean.

I tried it in my tests for the JWT plugin and that was the only case that was not handled correctly by just using path.Clean.

[hsluoyz]

Hi @magikstm ,

I can't reproduce this issue on http.authz. p, alice, /v2/*, GET just allows alice to access http://localhost:8080//v2/2.jpg via GET. And Casbin will do its own path matching, which is not affected by Caddy. Can you provide more information about it?

[mholt]

@BTBurke

That's troubling, because I am having good success with this patch:

diff --git a/caddyhttp/httpserver/path.go b/caddyhttp/httpserver/path.go
index 9213639..dfcc710 100644
--- a/caddyhttp/httpserver/path.go
+++ b/caddyhttp/httpserver/path.go
@@ -2,6 +2,7 @@ package httpserver

 import (
        "net/http"
+       "path"
        "strings"
 )

@@ -20,6 +21,7 @@ func (p Path) Matches(base string) bool {
        if base == "/" {
                return true
        }
+       p = Path(path.Clean(string(p)))
        if CaseSensitivePath {
                return strings.HasPrefix(string(p), base)
        }

I have confirmed in one simple test using basicauth that this protected a resource that, without the patch, was vulnerable.

Doing some further background research, it seems that nginx is _not_ vulnerable to this by default because location uses a sanitized version of the path, much like what this patch does. But @Whitestrake confirmed that nginx _is vulnerable if_ you use merge_slashes off; in your nginx config.

(They are likely aware of this, as they advise care when turning merge_slashes off in this obscure mailing list message.)

@magikstm @BTBurke Would you like to also try with this patch and see if it remedies the issue in your tests? If so, it should be easy to whip up a test table for this function to ensure it doesn't happen again.

One thing to note: path.Clean does a number of things:

Clean returns the shortest path name equivalent to path by purely lexical processing. It applies the following rules iteratively until no further processing can be done:

1. Replace multiple slashes with a single slash.
2. Eliminate each . path name element (the current directory).
3. Eliminate each inner .. path name element (the parent directory)
   along with the non-.. element that precedes it.
4. Eliminate .. elements that begin a rooted path:
   that is, replace "/.." by "/" at the beginning of a path.
   The returned path ends in a slash only if it is the root "/".

If the result of this process is an empty string, Clean returns the string ".".

Are these acceptable actions? (I feel like they are, since we're not mutating the URI, only evaluating it to see if the path "matches" -- however we define a match. Surely this is safer than a simple prefix match like we're currently doing.)

Keep in mind that the main reason this is a problem is because of Linux, which collapses multiple slashes. Seeing it on Mac too. But I bet Windows doesn't do this. (Does it?) The point is, these are all file system specifics. If a file system doesn't evaluate paths like path.Clean does, then this protection won't help much. And as seen in other issues like #1298, multiple forward slashes can have valuable meaning where stripping it would be a mistake. Basically: what a path/URI really means, and how it is interpreted, is up to the end of the middleware stack. It could be literally a slash, or it could be a divider in a file path, or it could be some-other-thing-I-have-no-idea.

But for most use cases, I bet path.Clean would be sufficient. What say you?

[magikstm]

@hsluoyz I'm attaching the minimal config I used. Extract in a folder that has Caddy.exe.

You'll probably need to adjust the "root" parameter.

I tested on Windows 7 x64.

My tests are:
1) http://localhost:8080/v2/2.jpg (403 forbidden)
2) http://localhost:8080//v2/2.jpg (displays the image)

1859_http.authz_minimal.zip

[magikstm]

@hsluoyz I think I may have had a caching issue when I did tests with http.authz. I don't seem to have the issue anymore on my test 2.

[hsluoyz]

OK :)

[magikstm]

@mholt I did my tests on Windows 7 x64 (on a laptop).

It passes through basicauth with: http://localhost:8080//2.jpg

On IE11 (latest), Firefox (latest) and Chrome (latest).

With the minimal config I mentioned here: https://github.com/mholt/caddy/issues/1859#issuecomment-327978008

(It's before your suggested patch. I'll try with it.)

[mholt]

@magikstm Fascinating -- I presume it's a fluke that Windows, Mac, and Linux all happen to be generous in collapsing repeated slashes? Either that or Go is doing something under the hood.

Let me know how it goes with the patch. It's bedtime here, I'll check back in morning.

[whitestrake]

I think path cleaning, at least for path matching, is a sane and good default at the very least and should be implemented, regardless of whether there should additionally be a method to check matches without it.

Case in point, nginx's opt-out slash merging.

[mholt]

Thanks, everyone, for jumping in and working on solving the problem! Love how there was no mud slinging or blaming or fault finding. Purely productive. Made finding the patch pretty simple. Keep it up. :+1:

[hstaugaard]

Great and speedy work!

curl http://localhost:8002//secret/files/file.txt still returns the secret file for me (Caddy 0.10.8 on Win10). Is this by design?

[mholt]

@hstaugaard Hmm, that's not happening for me. I suspect your rewrites may be allowing that through. If you use basicauth or the like, it will be protected.

[hstaugaard]

Hmm that's strange. I use the exact same setup as my original zip-file with version 0.10.8. //secret/files definitely not rewritten.

P.S. I'm not trying to protect anything, I just want to be sure that my rewrites are matched. I don't think it makes sense to have rewrite's _basepath_ argument behave differently than the other directives.

[mholt]

Well internal and status were definitely fixed with this. @abiosoft the rewrite docs say that the [basepath] argument should match a _base_ path in this syntax. I'm having trouble finding where that's the case in the source code (the source code seems to imply an exact match -- unless I'm missing something). I always thought it was supposed to do exact matches but I didn't realize the docs said base path. What do you think we should do?

[magikstm]

@mholt I retested this issue with version 0.10.8 and latest versions of plugins on Windows 7 x64.

I did tests with these:
1) status - Fixed
2) rewrite - Same behavior as mentioned by @hstaugaard in https://github.com/mholt/caddy/issues/1859#issuecomment-328270375
3) internal - Fixed
4) http.login with http.jwt - Fixed
5) basicauth - Fixed
6) http.reauth - Fixed
7) http.authz - No change (was unaffected by the issue)

I've not been able to test these before or after version 0.10.8:
1) http.restic
2) http.webdav

I think only "rewrite" may need to be adjusted.

In case it may help, I tested these with rewrite + basicauth and this simple Caddyfile:

:8002 {
    rewrite /secret/files {
        regexp ^/(.*)
        to /notsecret/files/{1}
    }
    basicauth /secret/files/2.jpg username password
}

1) http://localhost:8002/secret/files/2.jpg - 404 (ok as it's rewritten)
2) http://localhost:8002//secret/files/2.jpg - Got basicauth (expected 404)
3) http://localhost:8002/secret//files/2.jpg - Got basicauth (expected 404)
4) http://localhost:8002/secret/files//2.jpg - 404 (ok as it's rewritten)
Results of test 2 and 3 are odd here.

Did the same test only with basicauth with this simple Caddyfile:

:8005 {
    basicauth /secret/files/2.jpg username password
}

1) http://localhost:8005/secret/files/2.jpg - Got basicauth (ok)
2) http://localhost:8005//secret/files/2.jpg - Got basicauth (ok)
3) http://localhost:8005/secret//files/2.jpg - Got basicauth (ok)
4) http://localhost:8005/secret/files//2.jpg - Got basicauth (ok)

Results with rewrite only with this simple Caddyfile are:

:8001 {
    rewrite /secret/files {
        regexp ^/(.*)
        to /notsecret/files/{1}
    }
}

1) http://localhost:8002/secret/files/2.jpg - 404 (ok as it's rewritten)
2) http://localhost:8002//secret/files/2.jpg - displays secret file (expected 404)
3) http://localhost:8002/secret//files/2.jpg - displays secret file (expected 404)
4) http://localhost:8002/secret/files//2.jpg - 404 (ok as it's rewritten)