From 1c8c5b08d70aaa82e1331858d4196e850b8a2688 Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Mon, 20 Jul 2020 18:18:17 -0700 Subject: [PATCH] Handle cookie signing errors --- pkg/encryption/utils.go | 37 ++++++++++++----------- pkg/encryption/utils_test.go | 6 ++-- pkg/sessions/cookie/session_store.go | 24 ++++++++++----- pkg/sessions/persistence/manager.go | 6 +++- pkg/sessions/persistence/ticket.go | 25 ++++++++++----- pkg/sessions/tests/session_store_tests.go | 5 +-- 6 files changed, 64 insertions(+), 39 deletions(-) diff --git a/pkg/encryption/utils.go b/pkg/encryption/utils.go index e3126045..269a89c6 100644 --- a/pkg/encryption/utils.go +++ b/pkg/encryption/utils.go @@ -2,19 +2,16 @@ package encryption import ( "crypto/hmac" - "crypto/rand" + // TODO (@NickMeves): Remove SHA1 signed cookie support in V7 "crypto/sha1" // #nosec G505 "crypto/sha256" "encoding/base64" "fmt" "hash" - "io" "net/http" "strconv" "strings" "time" - - "github.com/oauth2-proxy/oauth2-proxy/pkg/logger" ) // SecretBytes attempts to base64 decode the secret, if that fails it treats the secret as binary @@ -69,40 +66,44 @@ func Validate(cookie *http.Cookie, seed string, expiration time.Duration) (value } // SignedValue returns a cookie that is signed and can later be checked with Validate -func SignedValue(seed string, key string, value []byte, now time.Time) string { +func SignedValue(seed string, key string, value []byte, now time.Time) (string, error) { encodedValue := base64.URLEncoding.EncodeToString(value) timeStr := fmt.Sprintf("%d", now.Unix()) - sig := cookieSignature(sha256.New, seed, key, encodedValue, timeStr) + sig, err := cookieSignature(sha256.New, seed, key, encodedValue, timeStr) + if err != nil { + return "", err + } cookieVal := fmt.Sprintf("%s|%s|%s", encodedValue, timeStr, sig) - return cookieVal + return cookieVal, nil } -func cookieSignature(signer func() hash.Hash, args ...string) string { +func cookieSignature(signer func() hash.Hash, args ...string) (string, error) { h := hmac.New(signer, []byte(args[0])) for _, arg := range args[1:] { _, err := h.Write([]byte(arg)) - // If signing fails, fail closed and return something that won't validate if err != nil { - garbage := make([]byte, 16) - if _, err := io.ReadFull(rand.Reader, garbage); err != nil { - logger.Fatal("HMAC & RNG functions both failing. Shutting down for security") - } - return base64.URLEncoding.EncodeToString(garbage) + return "", err } } var b []byte b = h.Sum(b) - return base64.URLEncoding.EncodeToString(b) + return base64.URLEncoding.EncodeToString(b), nil } func checkSignature(signature string, args ...string) bool { - checkSig := cookieSignature(sha256.New, args...) + checkSig, err := cookieSignature(sha256.New, args...) + if err != nil { + return false + } if checkHmac(signature, checkSig) { return true } - // TODO: After appropriate rollout window, remove support for SHA1 - legacySig := cookieSignature(sha1.New, args...) + // TODO (@NickMeves): Remove SHA1 signed cookie support in V7 + legacySig, err := cookieSignature(sha1.New, args...) + if err != nil { + return false + } return checkHmac(signature, legacySig) } diff --git a/pkg/encryption/utils_test.go b/pkg/encryption/utils_test.go index 15bc83fe..162c64ce 100644 --- a/pkg/encryption/utils_test.go +++ b/pkg/encryption/utils_test.go @@ -88,8 +88,10 @@ func TestSignAndValidate(t *testing.T) { value := base64.URLEncoding.EncodeToString([]byte("I am soooo encoded")) epoch := "123456789" - sha256sig := cookieSignature(sha256.New, seed, key, value, epoch) - sha1sig := cookieSignature(sha1.New, seed, key, value, epoch) + sha256sig, err := cookieSignature(sha256.New, seed, key, value, epoch) + assert.NoError(t, err) + sha1sig, err := cookieSignature(sha1.New, seed, key, value, epoch) + assert.NoError(t, err) assert.True(t, checkSignature(sha256sig, seed, key, value, epoch)) // This should be switched to False after fully deprecating SHA1 diff --git a/pkg/sessions/cookie/session_store.go b/pkg/sessions/cookie/session_store.go index 18717ed1..87d16863 100644 --- a/pkg/sessions/cookie/session_store.go +++ b/pkg/sessions/cookie/session_store.go @@ -44,8 +44,7 @@ func (s *SessionStore) Save(rw http.ResponseWriter, req *http.Request, ss *sessi if err != nil { return err } - s.setSessionCookie(rw, req, value, *ss.CreatedAt) - return nil + return s.setSessionCookie(rw, req, value, *ss.CreatedAt) } // Load reads sessions.SessionState information from Cookies within the @@ -114,24 +113,33 @@ func sessionFromCookie(v []byte, c encryption.Cipher) (s *sessions.SessionState, } // setSessionCookie adds the user's session cookie to the response -func (s *SessionStore) setSessionCookie(rw http.ResponseWriter, req *http.Request, val []byte, created time.Time) { - for _, c := range s.makeSessionCookie(req, val, created) { +func (s *SessionStore) setSessionCookie(rw http.ResponseWriter, req *http.Request, val []byte, created time.Time) error { + cookies, err := s.makeSessionCookie(req, val, created) + if err != nil { + return err + } + for _, c := range cookies { http.SetCookie(rw, c) } + return nil } // makeSessionCookie creates an http.Cookie containing the authenticated user's // authentication details -func (s *SessionStore) makeSessionCookie(req *http.Request, value []byte, now time.Time) []*http.Cookie { +func (s *SessionStore) makeSessionCookie(req *http.Request, value []byte, now time.Time) ([]*http.Cookie, error) { strValue := string(value) if strValue != "" { - strValue = encryption.SignedValue(s.Cookie.Secret, s.Cookie.Name, value, now) + var err error + strValue, err = encryption.SignedValue(s.Cookie.Secret, s.Cookie.Name, value, now) + if err != nil { + return nil, err + } } c := s.makeCookie(req, s.Cookie.Name, strValue, s.Cookie.Expire, now) if len(c.String()) > maxCookieLength { - return splitCookie(c) + return splitCookie(c), nil } - return []*http.Cookie{c} + return []*http.Cookie{c}, nil } func (s *SessionStore) makeCookie(req *http.Request, name string, value string, expiration time.Duration, now time.Time) *http.Cookie { diff --git a/pkg/sessions/persistence/manager.go b/pkg/sessions/persistence/manager.go index 16a54b4e..a8281838 100644 --- a/pkg/sessions/persistence/manager.go +++ b/pkg/sessions/persistence/manager.go @@ -48,7 +48,11 @@ func (m *Manager) Save(rw http.ResponseWriter, req *http.Request, s *sessions.Se if err != nil { return err } - tckt.setCookie(rw, req, s) + + err = tckt.setCookie(rw, req, s) + if err != nil { + return err + } return nil } diff --git a/pkg/sessions/persistence/ticket.go b/pkg/sessions/persistence/ticket.go index da1097d9..abb78614 100644 --- a/pkg/sessions/persistence/ticket.go +++ b/pkg/sessions/persistence/ticket.go @@ -148,33 +148,42 @@ func (t *ticket) clearSession(clearer clearFunc) error { } // setCookie sets the encoded ticket as a cookie -func (t *ticket) setCookie(rw http.ResponseWriter, req *http.Request, s *sessions.SessionState) { - ticketCookie := t.makeCookie( +func (t *ticket) setCookie(rw http.ResponseWriter, req *http.Request, s *sessions.SessionState) error { + ticketCookie, err := t.makeCookie( req, t.encodeTicket(), t.options.Expire, *s.CreatedAt, ) + if err != nil { + return err + } http.SetCookie(rw, ticketCookie) + return nil } // clearCookie removes any cookies that would be where this ticket // would set them func (t *ticket) clearCookie(rw http.ResponseWriter, req *http.Request) { - clearCookie := t.makeCookie( + http.SetCookie(rw, cookies.MakeCookieFromOptions( req, + t.options.Name, "", + t.options, time.Hour*-1, time.Now(), - ) - http.SetCookie(rw, clearCookie) + )) } // makeCookie makes a cookie, signing the value if present -func (t *ticket) makeCookie(req *http.Request, value string, expires time.Duration, now time.Time) *http.Cookie { +func (t *ticket) makeCookie(req *http.Request, value string, expires time.Duration, now time.Time) (*http.Cookie, error) { if value != "" { - value = encryption.SignedValue(t.options.Secret, t.options.Name, []byte(value), now) + var err error + value, err = encryption.SignedValue(t.options.Secret, t.options.Name, []byte(value), now) + if err != nil { + return nil, err + } } return cookies.MakeCookieFromOptions( req, @@ -183,7 +192,7 @@ func (t *ticket) makeCookie(req *http.Request, value string, expires time.Durati t.options, expires, now, - ) + ), nil } // makeCipher makes a AES-GCM cipher out of the ticket's secret diff --git a/pkg/sessions/tests/session_store_tests.go b/pkg/sessions/tests/session_store_tests.go index cf3f8173..f23028f3 100644 --- a/pkg/sessions/tests/session_store_tests.go +++ b/pkg/sessions/tests/session_store_tests.go @@ -311,11 +311,12 @@ func SessionStoreInterfaceTests(in *testInput) { BeforeEach(func() { By("Using a valid cookie with a different providers session encoding") broken := "BrokenSessionFromADifferentSessionImplementation" - value := encryption.SignedValue(in.cookieOpts.Secret, in.cookieOpts.Name, []byte(broken), time.Now()) + value, err := encryption.SignedValue(in.cookieOpts.Secret, in.cookieOpts.Name, []byte(broken), time.Now()) + Expect(err).ToNot(HaveOccurred()) cookie := cookiesapi.MakeCookieFromOptions(in.request, in.cookieOpts.Name, value, in.cookieOpts, in.cookieOpts.Expire, time.Now()) in.request.AddCookie(cookie) - err := in.ss().Save(in.response, in.request, in.session) + err = in.ss().Save(in.response, in.request, in.session) Expect(err).ToNot(HaveOccurred()) })