500 instead of 401 on unauthenticated requests about coraza-caddy HOT 11 CLOSED

@rasschaert first of all, thanks a lot for the detailed report and the example caddyfile. It took me a few minutes to find the error by trying it. The problem comes from https://github.com/corazawaf/coraza-caddy/blob/main/coraza.go#L131 where we receive an error to interrupt the flow and we basically overide that error with an own (and return 500 status code) which shouldn't be the case. I opened #85 to fix that.

I believe the fact that middlewares in caddy return errors is to interrupt the flow which is exactly what is happening here, the auth breaks the flow on request and we should be respecting that and passthrough the error. I don't think there is any problem with specific ordering of middlewares, this middleware shouldn't be conflicting with auth or ratelimiting, ideally should not conflicted with any middleware whose action is to interrupt the flow (if the middleware does not interrupt the flow but does write headers or body will indeed be a problem)

@mholt I wonder which is better, for us to write status 500 in the response writer and return nil or to just return a caddyhttp error with the status code (like basicauth does)? I think we should be following what idiomatic caddy does but when we want to interrupt the flow (e.g. by returning a handler error) we want that no headers or body get to the wire.

from coraza-caddy.

It might be related to #71. It feels we have an issue with redirection.

from coraza-caddy.

I discovered that using coraza-caddy in conjunction with the caddy-ratelimit module also causes it to return 500 instead of 429 when coraza is enabled.

2023/07/07 18:33:51.081	ERROR	http.log.error	{id=30f2gja7h} caddy-ratelimit.(*Handler).rateLimitExceeded (handler.go:213): HTTP 429	{"request": {"remote_ip": "172.17.0.1", "remote_port": "44050", "proto": "HTTP/1.1", "method": "POST", "host": "localhost:8000", "uri": "/", "headers": {"User-Agent": ["curl/7.81.0"], "Accept": ["*/*"]}}, "duration": 0.002568732, "status": 500, "err_id": "sPhQXClgdabCRagV", "err_trace": ""}

This prompted me to do some more experimentation and troubleshooting. I found that replacing order coraza_waf first with order coraza_waf after basicauth resolves the issue!

I'm currently using that as a workaround, although I'm sure there are good reasons why you recommend making coraza_waf the first directive in the directive processing order.

The WAF still seems to detect and handle violations correctly at least...

2023/07/07 18:39:32.368	ERROR	http.handlers.waf	[client "172.17.0.1"] Coraza: Access denied (phase 2). Path Traversal Attack (/../) or (/.../) [file "/etc/caddy/coreruleset/rules/REQUEST-930-APPLICATION-ATTACK-LFI.conf"] [line "4157"] [id "930100"] [rev ""] [msg "Path Traversal Attack (/../) or (/.../)"] [data "Matched Data: /../ found within REQUEST_URI_RAW: /foo.html?../../../etc/passwd"] [severity "critical"] [ver "OWASP_CRS/4.0.0-rc1"] [maturity "0"] [accuracy "0"] [tag "application-multi"] [tag "language-multi"] [tag "platform-multi"] [tag "attack-lfi"] [tag "paranoia-level/1"] [tag "OWASP_CRS"] [tag "capec/1000/255/153/126"] [hostname ""] [uri "/foo.html?../../../etc/passwd"] [unique_id "AfLaJwxSLppafZpR"]

from coraza-caddy.

And just now I encountered the 500 instead of 401 issue again in a situation where I have basicauth inside a handle_path block.

I use handle_path and route often so now I've modified my workaround again to order coraza_waf after route.

from coraza-caddy.

@mholt do you have any idea about why our module is generating issues to other modules? How should we handle the modules order when there are more modules?
We have received the same issue for multiple modules

from coraza-caddy.

I don't know of any way to automatically put modules in the right order.

I think it's ultimately up to the user to ensure they go in the right order. It sounds like this module needs to run after basicauth but before rate_limit.

Usually, handler modules should recommend a default order in their docs, as if theirs is the only extra module, for example, my rate limit module recommends: order rate_limit before basicauth

I am not sure how well defining a concrete order like this for every plugin handler is going to scale. It's intended for just one or two plugins, which is 99% of use cases.

The use of route blocks might be easier, since the order of handlers inside are taken literally, and you don't need to use order global options.

You can always see how the order of your handlers are being evaluated by using caddy adapt to read the JSON.

from coraza-caddy.

It is working: https://tosso.io/experiment/basicauth username: foouser, password: foopassword

My settings:

    handle /experiment/basicauth {
        basicauth * {
            # not the actual credentials, just an example for github
            foouser $2a$14$EUkRdDpsoURnFJtZz3KhLuIIAirpmYdMYyetZI0uDR08ok3ZWp3I.
        }
        respond "It worked :)"
    } 

from coraza-caddy.

Glad to see you were able to find a fix for it :)

@mholt I wonder which is better, for us to write status 500 in the response writer and return nil or to just return a caddyhttp error with the status code (like basicauth does)?

Almost always better to return a caddyhttp.Error, as this will let user-defined error handling take over. If you just write the response then nothing else can handle it (maybe that's what is desired in some cases, but it's very rare.)

from coraza-caddy.

I am really sorry to re-comment on this post, but the behavior described above actually brought me here. I could not find a combination, where rate_limit and coraza_waf could be applied before the specific handler (e.g. file_server) and work. Instead, requesting a nonexistent directory with file_server lead to a Statuscode 500.

As far as I understand Caddys ordering, I think it would be wise to have rate_limit and coraza_waf applied as early as possible. Preferably coraza_waf first and rate_limit before basicauth. Otherwise, depending on the applied directives, Caddy may not reach the plugins logic.

I manually tested some ordering cases via a route-directive and found no satisfying solution to use basic-auth, rate-limiting, waf and file_server:

I could continue, but ordering file_server and basicauth to the front will pretty much disable one or both plugins - or their defensive abilities, e.g. to defend basic authentication against brute force attacks.

So did #85 fix this issue or did #85 only fix the error message?

As far as I could ascertain, rate_limit and coraza_waf are then mutually exclusive right now - at least when using file_server. I suppose, this also applies to the other response handlers.

from coraza-caddy.

I think rate_limit should come before coraza. Also, would you provide a caddyfile that reproduces the problem and how are you compiling it?

from coraza-caddy.

Dockerfile:

FROM docker.io/caddy:builder-alpine AS builder

RUN xcaddy build \
    --with github.com/mholt/caddy-ratelimit \
    --with github.com/corazawaf/coraza-caddy/v2@main

FROM docker.io/caddy:latest
COPY --from=builder /usr/bin/caddy /usr/bin/caddy
RUN apk add --no-cache git && mkdir /waf && git clone https://github.com/corazawaf/coraza-coreruleset /waf && rm -rf /var/cache/apk/*

Caddyfile:

{
	debug
	log {
		output file /var/log/caddy/caddy.log
	}
}

:8080 {
	root * /var/www/
	route {
		rate_limit {
			zone dynamic {
				key {remote_host}
				events 90
				window 10s
			}
		}
		coraza_waf {
			directives `
			Include /etc/caddy/coraza.conf
			Include /etc/caddy/crs-setup.conf
			Include /waf/rules/@owasp_crs/*.conf
			`
		}
		basicauth * {
			Bob $2a$14$Zkx19XLiW6VYouLHR5NmfOFU0z2GTNmpkT/5qqR7hx4IjWJPDhjvG
		}
		file_server
	}
}

I pass coraza.conf and crs-setup.conf to the container through volume mounts. I copied the current versions from the corresponding git-repos and changed coraza.conf by turning the SecRuleEngine on.

from coraza-caddy.