[Security] race due to unsafe usage in SetSecure could lead to prior request cookie being leaked

[cstockton]

The call to SetSecure at https://github.com/chmike/cookie/blob/master/cookie.go#L142 uses the unsafe package to convert to an assumed immutable structure (string) with a mutable slice backing (sync.Pool).

Recommendation:
Do not import package unsafe for conversions as a general rule of thumb, the potential to lead to these sort of issues is high. Doing this for []byte() to string is actually less justified as the compiler does have optimizations in some places to help with this when it can prove that it is safe. Use string(byteSliceVal) instead.

Reproduce:

package main

import (
	"fmt"
	"net/http"
	"net/http/httptest"
	"strconv"
	"sync"
	"jail/cookie"
)

func setAndGet(i int) {
	k := make([]byte, cookie.KeyLen)
	r := httptest.NewRecorder()
	v := strconv.Itoa(i)
	p := &cookie.Params{
		Name:     "test",
		Value:    v,
		Path:     "path",
		Domain:   "example.com",
		MaxAge:   10,
		HTTPOnly: true,
		Secure:   true,
	}

	err := cookie.SetSecure(r, p, k)
	if err != nil {
		panic(err)
	}

	req := &http.Request{
		Header: http.Header{"Cookie": r.HeaderMap["Set-Cookie"]}}
	val, err := cookie.GetSecureValue(req, "test", k)
	if err != nil {
		panic(err)
	}
	if got := string(val); got != v {
		panic(fmt.Sprintf(`exp %v; got %v`, v, got))
	}
}

func main() {
	const n = 4096
	var wg sync.WaitGroup
	wg.Add(n)
	for i := 0; i < n; i++ {
		i := i
		go func() {
			defer wg.Done()
			setAndGet(i)
		}()
	}
	wg.Wait()
}

Output:

for i in $(seq 1 4); do go run main.go; done
panic: exp 229; got 507

goroutine 235 [running]:
main.setAndGet(0xe5)
	/jail/9a841e68-c85e-48a6-a84a-e3579ec354df/src/main.go:38 +0x63f
main.main.func1(0xc420014560, 0xe5)
	/jail/9a841e68-c85e-48a6-a84a-e3579ec354df/src/main.go:50 +0x51
created by main.main
	/jail/9a841e68-c85e-48a6-a84a-e3579ec354df/src/main.go:48 +0x7d
exit status 2
panic: MAC mismatch

goroutine 330 [running]:
main.setAndGet(0x144)
	/jail/9a841e68-c85e-48a6-a84a-e3579ec354df/src/main.go:35 +0x64f
main.main.func1(0xc420014560, 0x144)
	/jail/9a841e68-c85e-48a6-a84a-e3579ec354df/src/main.go:50 +0x51
created by main.main
	/jail/9a841e68-c85e-48a6-a84a-e3579ec354df/src/main.go:48 +0x7d
exit status 2
panic: invalid base64 char

goroutine 246 [running]:
main.setAndGet(0xf0)
	/jail/9a841e68-c85e-48a6-a84a-e3579ec354df/src/main.go:35 +0x64f
main.main.func1(0xc420014560, 0xf0)
	/jail/9a841e68-c85e-48a6-a84a-e3579ec354df/src/main.go:50 +0x51
created by main.main
	/jail/9a841e68-c85e-48a6-a84a-e3579ec354df/src/main.go:48 +0x7d
exit status 2
panic: http: named cookie not present

goroutine 473 [running]:
main.setAndGet(0x1d3)
	/jail/9a841e68-c85e-48a6-a84a-e3579ec354df/src/main.go:35 +0x64f
main.main.func1(0xc420014560, 0x1d3)
	/jail/9a841e68-c85e-48a6-a84a-e3579ec354df/src/main.go:50 +0x51
created by main.main
	/jail/9a841e68-c85e-48a6-a84a-e3579ec354df/src/main.go:48 +0x7d
exit status 2