rs
With some CORS configurations, some handlers can introduce synchronisation bugs and cause data races
Problem
Presumably for performance, the library (v1.11.1 and some older versions) reuses some non-exported slice variables and struct field from one middleware call to the next:
- package-level var
headerOriginAll, - package-level var
headerTrue, - package-level var
headerVaryOrigin, and - struct field
Cors.preflightVary.
Unfortunately, with some CORS middleware, some handlers that they wrap can get a hold of aliases of those slices and mutate their backing array. Because middleware are typically invoked concurrently and such mutation doesn't involve any synchronisation, data races can occur. Therefore, those slices are a bit of a footgun.
Proof of concept
-
Clone the repo and save the following code snippet to some test file (perhaps named
mutability_test.go):package cors import ( "net/http" "net/http/httptest" "slices" "sync" "testing" ) var handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { for _, k := range keys { vv := w.Header()[k] if len(vv) > 0 { vv[0] = "oops!" } } }) var keys = []string{ "Access-Control-Allow-Credentials", "Access-Control-Allow-Origin", "Vary", } func TestMutabilityFromHandlerWithPrelightRequest(t *testing.T) { // Arrange c := New(Options{ AllowedOrigins: []string{"*"}, AllowCredentials: true, AllowedMethods: []string{http.MethodPut}, OptionsPassthrough: true, }) var ( // save the initial values of mutable variables for later assertions headerOriginAllWant = slices.Clone(headerOriginAll) headerTrueWant = slices.Clone(headerTrue) cPreflightVaryWant = slices.Clone(c.preflightVary) ) req := httptest.NewRequest(http.MethodOptions, "https://example.org", nil) req.Header.Add("Origin", "https://example.com") req.Header.Add("Access-Control-Request-Method", http.MethodPut) rec := httptest.NewRecorder() // Act c.Handler(handler).ServeHTTP(rec, req) // Assert const tmpl = "%s: got %q; want %q" if !slices.Equal(c.preflightVary, cPreflightVaryWant) { t.Errorf(tmpl, "c.preflightVary", c.preflightVary, cPreflightVaryWant) } if !slices.Equal(headerOriginAll, headerOriginAllWant) { t.Errorf(tmpl, "headerOriginAll", headerOriginAll, headerOriginAllWant) } if !slices.Equal(headerTrue, headerTrueWant) { t.Errorf(tmpl, "headerTrue", headerTrue, headerTrueWant) } // Clean up : restore slices to their initial value headerOriginAll = headerOriginAllWant headerTrue = headerTrueWant c.preflightVary = cPreflightVaryWant } func TestMutabilityFromHandlerWithActualRequest(t *testing.T) { // Arrange var c = New(Options{ AllowedOrigins: []string{"*"}, AllowCredentials: true, }) var ( // save the initial values of mutable variables for later assertions headerOriginAllWant = slices.Clone(headerOriginAll) headerTrueWant = slices.Clone(headerTrue) headerVaryOriginWant = slices.Clone(headerVaryOrigin) ) req := httptest.NewRequest(http.MethodGet, "https://example.org", nil) req.Header.Add("Origin", "https://example.com") rec := httptest.NewRecorder() // Act c.Handler(handler).ServeHTTP(rec, req) // Assert const tmpl = "%s: got %q; want %q" if !slices.Equal(headerVaryOrigin, headerVaryOriginWant) { t.Errorf(tmpl, "headerVaryOrigin", headerVaryOrigin, headerVaryOriginWant) } if !slices.Equal(headerOriginAll, headerOriginAllWant) { t.Errorf(tmpl, "headerOriginAll", headerOriginAll, headerOriginAllWant) } if !slices.Equal(headerTrue, headerTrueWant) { t.Errorf(tmpl, "headerTrue", headerTrue, headerTrueWant) } // Clean up: restore slices to their initial value headerOriginAll = headerOriginAllWant headerTrue = headerTrueWant headerVaryOrigin = headerVaryOriginWant } // Note: run this test with -race func TestSynchronizationBugWithPrelightRequest(t *testing.T) { testSynchronizationBug(t, true) } // Note: run this test with -race func TestSynchronizationBugWithActualRequest(t *testing.T) { testSynchronizationBug(t, false) } func testSynchronizationBug(t *testing.T, preflight bool) { t.Helper() c := New(Options{ AllowedOrigins: []string{"*"}, AllowCredentials: true, AllowedMethods: []string{http.MethodPut}, OptionsPassthrough: true, }) var req *http.Request if preflight { req = httptest.NewRequest(http.MethodOptions, "https://example.org", nil) req.Header.Add("Access-Control-Request-Method", http.MethodPut) } else { req = httptest.NewRequest(http.MethodGet, "https://example.org", nil) } req.Header.Add("Origin", "https://example.com") // simulate concurrent requests const n = 128 var wg sync.WaitGroup wg.Add(n) for range n { go func() { defer wg.Done() rec := httptest.NewRecorder() c.Handler(handler).ServeHTTP(rec, req) }() } wg.Wait() } -
Run the following command and observe that the two tests fail, proving that the slices in question can indeed be mutated by a handler, at least with some CORS configs:
$ go test -run '^TestMutabilityFromHandlerW' --- FAIL: TestMutabilityFromHandlerWithPrelightRequest (0.00s) mutability_test.go:50: c.preflightVary: got ["oops!"]; want ["Origin, Access-Control-Request-Method, Access-Control-Request-Headers"] mutability_test.go:53: headerOriginAll: got ["oops!"]; want ["*"] mutability_test.go:56: headerTrue: got ["oops!"]; want ["true"] --- FAIL: TestMutabilityFromHandlerWithActualRequest (0.00s) mutability_test.go:86: headerVaryOrigin: got ["oops!"]; want ["Origin"] mutability_test.go:89: headerOriginAll: got ["oops!"]; want ["*"] mutability_test.go:92: headerTrue: got ["oops!"]; want ["true"] -
Run the following command and observe that the two tests fail, proving that data races can occur as a result of such unsynchronised mutation:
go test -race -run '^TestSynchronizationBug' ================== WARNING: DATA RACE -snip- ================== --- FAIL: TestSynchronizationBugWithPrelightRequest (0.00s) testing.go:1617: race detected during execution of test ================== WARNING: DATA RACE -snip- ================== --- FAIL: TestSynchronizationBugWithActualRequest (0.00s) testing.go:1617: race detected during execution of test FAIL exit status 1 FAIL github.com/rs/cors 0.538s
Remediation
Do not share slices between from one call to the next. Here is a possible patch that I'd be happy to contribute:
diff --git a/cors.go b/cors.go
index 724f242..46a4171 100644
--- a/cors.go
+++ b/cors.go
@@ -30,10 +30,6 @@ import (
"github.com/rs/cors/internal"
)
-var headerVaryOrigin = []string{"Origin"}
-var headerOriginAll = []string{"*"}
-var headerTrue = []string{"true"}
-
// Options is a configuration container to setup the CORS middleware.
type Options struct {
// AllowedOrigins is a list of origins a cross-domain request can be executed from.
@@ -133,7 +129,7 @@ type Cors struct {
allowCredentials bool
allowPrivateNetwork bool
optionPassthrough bool
- preflightVary []string
+ preflightVary string
}
// New creates a new Cors handler with the provided options.
@@ -229,9 +225,9 @@ func New(options Options) *Cors {
// Pre-compute prefight Vary header to save allocations
if c.allowPrivateNetwork {
- c.preflightVary = []string{"Origin, Access-Control-Request-Method, Access-Control-Request-Headers, Access-Control-Request-Private-Network"}
+ c.preflightVary = "Origin, Access-Control-Request-Method, Access-Control-Request-Headers, Access-Control-Request-Private-Network"
} else {
- c.preflightVary = []string{"Origin, Access-Control-Request-Method, Access-Control-Request-Headers"}
+ c.preflightVary = "Origin, Access-Control-Request-Method, Access-Control-Request-Headers"
}
// Precompute max-age
@@ -337,11 +333,7 @@ func (c *Cors) handlePreflight(w http.ResponseWriter, r *http.Request) {
// Always set Vary headers
// see https://github.com/rs/cors/issues/10,
// https://github.com/rs/cors/commit/dbdca4d95feaa7511a46e6f1efb3b3aa505bc43f#commitcomment-12352001
- if vary, found := headers["Vary"]; found {
- headers["Vary"] = append(vary, c.preflightVary[0])
- } else {
- headers["Vary"] = c.preflightVary
- }
+ headers.Add("Vary", c.preflightVary)
allowed, additionalVaryHeaders := c.isOriginAllowed(r, origin)
if len(additionalVaryHeaders) > 0 {
headers.Add("Vary", strings.Join(convert(additionalVaryHeaders, http.CanonicalHeaderKey), ", "))
@@ -372,7 +364,7 @@ func (c *Cors) handlePreflight(w http.ResponseWriter, r *http.Request) {
return
}
if c.allowedOriginsAll {
- headers["Access-Control-Allow-Origin"] = headerOriginAll
+ headers.Set("Access-Control-Allow-Origin", "*")
} else {
headers["Access-Control-Allow-Origin"] = r.Header["Origin"]
}
@@ -385,10 +377,10 @@ func (c *Cors) handlePreflight(w http.ResponseWriter, r *http.Request) {
headers["Access-Control-Allow-Headers"] = reqHeaders
}
if c.allowCredentials {
- headers["Access-Control-Allow-Credentials"] = headerTrue
+ headers.Set("Access-Control-Allow-Credentials", "true")
}
if c.allowPrivateNetwork && r.Header.Get("Access-Control-Request-Private-Network") == "true" {
- headers["Access-Control-Allow-Private-Network"] = headerTrue
+ headers.Set("Access-Control-Allow-Private-Network", "true")
}
if len(c.maxAge) > 0 {
headers["Access-Control-Max-Age"] = c.maxAge
@@ -404,11 +396,7 @@ func (c *Cors) handleActualRequest(w http.ResponseWriter, r *http.Request) {
allowed, additionalVaryHeaders := c.isOriginAllowed(r, origin)
// Always set Vary, see https://github.com/rs/cors/issues/10
- if vary := headers["Vary"]; vary == nil {
- headers["Vary"] = headerVaryOrigin
- } else {
- headers["Vary"] = append(vary, headerVaryOrigin[0])
- }
+ headers.Add("Vary", "Origin")
if len(additionalVaryHeaders) > 0 {
headers.Add("Vary", strings.Join(convert(additionalVaryHeaders, http.CanonicalHeaderKey), ", "))
}
@@ -430,7 +418,7 @@ func (c *Cors) handleActualRequest(w http.ResponseWriter, r *http.Request) {
return
}
if c.allowedOriginsAll {
- headers["Access-Control-Allow-Origin"] = headerOriginAll
+ headers.Set("Access-Control-Allow-Origin", "*")
} else {
headers["Access-Control-Allow-Origin"] = r.Header["Origin"]
}
@@ -438,7 +426,7 @@ func (c *Cors) handleActualRequest(w http.ResponseWriter, r *http.Request) {
headers["Access-Control-Expose-Headers"] = c.exposedHeaders
}
if c.allowCredentials {
- headers["Access-Control-Allow-Credentials"] = headerTrue
+ headers.Set("Access-Control-Allow-Credentials", "true")
}
c.logf(" Actual response added headers: %v", headers)
}
Another observation: if option OptionsPassthrough didn't exist (and my opinion is that it shouldn't), the library could rely on precomputed slices for writing headers in responses to preflight requests: there indeed would be no way for wrapped handlers to get a hold of those slices (and mutate them).
But that ship has sailed; perhaps that option should be dropped in a hypothetical v2, and deprecated until then.
Do not share slices between from one call to the next. Here is a possible patch that I'd be happy to contribute:
OK, but changes you made in this patch just hide the fact that internally headers.Set then allocate a slice. So it is the same as doing always slices.Clone before setting the header ourselves?
@mitar I believe that relying on slices.Clone would have the same semantics as my patch's. And, without having run any benchmarks, I believe the performance would be similar (if not identical) to my patch's.
Wouldn't you agree that setting the header is more idiomatic code than the alternative you suggest, though? What are you concerned about? Code churn?
No, I just wanted to make sure if I am missing something. I think using standard methods like .Set is reasonable.
@jub0bs is this based on the assumption it can happen or is it based on real cases where some middleware are actually doing that? I can’t see a valid use case where a middleware would blindly change the first element of a CORS header value this way instead of calling Set. It already smells by itself like a bug.
@rs
is this based on the assumption it can happen or is it based on real cases where some middleware are actually doing that?
I have not found instances of this in the wild, but the fact that it could easily happen is in itself regrettable.
I can’t see a valid use case where a middleware would blindly change the first element of a CORS header value this way instead of calling Set. It already smells by itself like a bug.
Authors of handlers wrapped in a CORS middleware may assume that their http.ResponseWriter.Header() is fair game for any kind of modifications. This middleware library (or any other) shouldn't forbid them to carry out such modifications.
The API to change headers is Set and Add. When you access the header slices directly, you need to know what you’re doing. Since we never had a single report of an issue with the current design, I’m willing to keep it this way until we get a real world issue.
@rs I still think that's a wart worth fixing but, as the owner of this repo, you have the last word. Feel free to close this issue.
FWIW, Russ Cox's stance (which, granted, may well differ from yours) about exposing pointers to memory intended as read-only in user code is: don't.