Some malicious/spoofed preflight requests cause prohibitive load

[jub0bs]

Problem

This package (v1.2.1 and possibly earlier versions) suffers from a similar (though less severe) form of rs/cors#170. Processing a preflight request with a maliciously long Access-Control-Request-Headers header indeed requires a relatively long time and causes a lot of undue heap allocations. For example, processing a single malicious preflight request whose ACRH header comprises 1MiB's worth of commas takes about 5ms and allocates about 17 MiB:

var testHandler = http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
	_, _ = io.WriteString(w, "bar")
})

func BenchmarkPreflight(b *testing.B) {
	req := httptest.NewRequest(http.MethodOptions, "https://example.org/admin", nil)
	req.Header.Add("Origin", "https://example.com")
	req.Header.Add("Access-Control-Request-Method", http.MethodGet)
	req.Header.Add("Access-Control-Request-Headers", "Authorization")
	handler := Handler(Options{})(testHandler)

	b.ReportAllocs()
	b.ResetTimer()
	for range b.N {
		res := httptest.NewRecorder()
		handler.ServeHTTP(res, req)
	}
}

func BenchmarkPreflightAdversarialACRH(b *testing.B) {
	req := httptest.NewRequest(http.MethodOptions, "https://example.org/admin", nil)
	req.Header.Add("Origin", "https://example.com")
	req.Header.Add("Access-Control-Request-Method", http.MethodGet)
	req.Header.Add("Access-Control-Request-Headers", strings.Repeat(",", http.DefaultMaxHeaderBytes))
	handler := Handler(Options{})(testHandler)

	b.ReportAllocs()
	b.ResetTimer()
	for range b.N {
		res := httptest.NewRecorder()
		handler.ServeHTTP(res, req)
	}
}

go test -run '^$' -bench . -count 8 > bench.out 
benchstat bench.out 

goos: darwin
goarch: amd64
pkg: whatever
cpu: Intel(R) Core(TM) i7-6700HQ CPU @ 2.60GHz
                           │  bench.out  │
                           │   sec/op    │
Preflight-8                  1.477µ ± 2%
PreflightAdversarialACRH-8   4.890m ± 4%
geomean                      84.97µ

                           │  bench.out   │
                           │     B/op     │
Preflight-8                  1.133Ki ± 0%
PreflightAdversarialACRH-8   17.00Mi ± 0%
geomean                      140.4Ki

                           │ bench.out  │
                           │ allocs/op  │
Preflight-8                  15.00 ± 0%
PreflightAdversarialACRH-8   15.00 ± 0%
geomean 

Impact

This behaviour could be abused by attackers to produce undue load on the middleware/server as an attempt to cause a denial of service. Moreover, because CORS middleware occurs before authentication, attackers wouldn't even need to be authenticated. Sure, most WAFs would likely drop those malicious preflight requests, but not all servers sit behind a WAF.

Solution

See https://github.com/jub0bs/cors/blob/b23eccc884c252107c2c8bd15de090706b6c2759/internal/headers/sortedset.go#L52 for inspiration.

[pkieltyka]

nice catch!

are you interested to submit a PR? otherwise we’ll take care of it, thanks again

[jub0bs]

@pkieltyka Thanks. Unless there's some bounty for fixing this in Chi, I don't intend to do it myself, since my time is limited and I've already got my own CORS library to maintain. But feel free to draw inspiration from my implementation, if you believe it suits the project.

[c2h5oh]

I'm not convinced handling this in CORS middleware is the way to go, because those huge headers have already been processed and memory allocated for them and they are waiting to be processed by middleware.

http.Server has MaxHeaderBytes which defaults to a very generous 1MB. I believe setting a lower, sane value (10KB should be enough for everyone™) there is the correct way of solving this.

[VojtechVitek]

I think I agree. The http.Server MaxHeaderBytes already takes care of too long request headers.

If the main issues is that we try to parse Access-Control-Request-Headers header value, which is too long, why don't we put a cap on its length beforehand? Would something like 1kb work?

- reqHeaders := parseHeaderList(r.Header.Get("Access-Control-Request-Headers"))
+ acrh := r.Header.Get("Access-Control-Request-Headers")
+ if len(acrh) > 1024 {
+   // too long error
+ }
+ reqHeaders := parseHeaderList(acrh)

The solution with sorted sets introduced in rs/cors seems to follow the fetch standard but feels a little too heavy for the hot-path code such as CORS middleware. I might be wrong. Am I missing anything?

[jub0bs]

I'm not convinced handling this in CORS middleware is the way to go, because those huge headers have already been processed and memory allocated for them and they are waiting to be processed by middleware.

True, but the problem lies with what you do with that huge header (i.e. allocate even more stuff as you process it even though you could do otherwise.)

why don't we put a cap on its length beforehand? Would something like 1kb work?

1kb indeed seems a like a sound limit. But you'd open yourself to interop issues, and you'd still allocate on the order of 16kb per malicious preflight request for no good reason.

[VojtechVitek]

@jub0bs thanks for tackling these kind of issues directly in stdlib!

Go 1.25.2

net/http: lack of limit when parsing cookies can cause memory exhaustion

Despite HTTP headers having a default limit of 1 MB, the number of cookies that can be parsed did not have a limit.
By sending a lot of very small cookies such as "a=;", an attacker can make an HTTP server allocate a large amount of structs, causing large memory consumption.

net/http now limits the number of cookies accepted to 3000, which can be adjusted using the httpcookiemaxnum GODEBUG option.

Thanks to jub0bs for reporting this issue.

This is CVE-2025-58186 and Go issue https://go.dev/issue/75672.

[jub0bs]

@VojtechVitek Quite similar to this issue indeed. 😉

[VojtechVitek]

@jub0bs I wish we could set up max number / length of individual request headers in the stdlib too and close this issue :)

[jub0bs]

@VojtechVitek What's wrong with relying on a sorted set as implemented in jub0bs/cors (and, by extension, in rs/cors)? I'm not sure what you meant by "a little too heavy". Too complicated? It's the best data structure for the job at hand, IMO.

[VojtechVitek]

There's nothing wrong with that sortedset pkg, it looks cool :)

By "little too heavy" I meant the need

1. to have such dependency in CORS middleware
2. to pre-process (copy and sort) the request header values at all

Imho, I don't think it's crucial for CORS middleware to ensure the header values come sorted in lexicographical order. I don't think it's worth the extra CPU cycles & latency. I prefer a lightweight middleware.

The biggest "reasonable" request I can think of is this:
(please, correct me if I'm wrong)

OPTIONS /orders HTTP/1.1
Origin: https://app.example.com
Access-Control-Request-Methods: POST, PATCH
Access-Control-Request-Headers: authorization, content-type, x-csrf-token
Access-Control-Request-Headers: x-request-id, x-correlation-id, x-trace-id, x-forwarded-for, x-forwarded-proto, x-forwarded-host, x-tenant-id, x-device-id, x-client-version, x-feature-flags, accept-language, user-agent

To prevent "prohibitive load" as reported, I think we just need to

1. Ensure the r.Header.Get("Access-Control-Request-Headers") is not longer than N characters
2. Or fail if we see "too many" , delimiters

No extra string copying or sorting.

[jub0bs]

@VojtechVitek

to have such dependency in CORS middleware

What are you worried about?

• Maintenance? It's a small package.
• Binary size? Unlikely to matter in the grand scheme of things.
• Dependents? If kept an internal package, this wouldn't be an issue.

I don't think it's crucial for CORS middleware to ensure the header values come sorted in lexicographical order. I don't think it's worth the extra CPU cycles & latency. I prefer a lightweight middleware.

On the contrary, it costs practically nothing and in fact allows you to error out early if you detect out-of-order values, which would indicate that the request couldn't have stemmed from a browser.

No extra string copying or sorting.

Perhaps it's not clear, but the sorted-set approach only requires sorting strings at middleware initialisation, not during middleware execution. A small price to pay, since the middleware is going to be executed way more times than initialised (once).

Access-Control-Request-Methods: POST, PATCH

Did you mean Access-Control-Request-Method (singular)? The value of that header, if present in the preflight request, consists only in one method name, not several.

Access-Control-Request-Headers: authorization, content-type, x-csrf-token
Access-Control-Request-Headers: x-request-id, x-correlation-id, x-trace-id, x-forwarded-for, x-forwarded-proto, x-forwarded-host, x-tenant-id, x-device-id, x-client-version, x-feature-flags, accept-language, user-agent

No Fetch-compliant browser would send two ACRH headers. Intermediaries may break a single ACRH header into multiple ones, but they're not allowed to alter the order of its list-based value's elements. The elements in your example are out of order, which makes the preflight request invalid, at least according to CORS middleware that adhere closely to the Fetch standard.

As for header names listed in the ACRH, YMMV.

To prevent "prohibitive load" as reported, I think we just need to [...]

This would likely lead to interoperability issues, as mentioned in an earlier comment.

[jub0bs]

No progress on this issue? Sad.