Closed rasschaert closed 1 year ago
It might be related to https://github.com/corazawaf/coraza-caddy/issues/71. It feels we have an issue with redirection.
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"]
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
.
@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
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.
@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 https://github.com/corazawaf/coraza-caddy/pull/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.
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 :)"
}
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.)
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:
Order | Works | Broken |
---|---|---|
rate_limit ,coraza_waf ,basicauth ,file_server |
Limiting,Auth,WAF | 404s will be 500 |
coraza_waf ,rate_limit ,basicauth ,file_server |
WAF | 404 will be 500,Auth,Limiting |
... | ... | ... |
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.
I think rate_limit should come before coraza. Also, would you provide a caddyfile that reproduces the problem and how are you compiling it?
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.
If coraza-caddy is enabled, Caddy responds with 500 to unauthenticated requests on paths that require
basicauth
. Here's my stripped-down Caddyfile.With coraza-caddy, 500:
Without coraza-caddy: 401, the expected behaviour:
I asked for assistance on the OWASP Slack and Matteo helpfully suggested these debugging tricks:
SecRuleEngine DetectionOnly
SecDebugLogLevel 3
These are very helpful suggestions. I can report the issue still occurrs even without the Include statements and in
DetectionOnly
mode, and no additional logging was produced on level 3.