CVE-2020-15223: Improper Check for Unusual or Exceptional Conditions (CWE-754)
value: In ORY Fosite (the security first OAuth2 & OpenID Connect framework for Go) before version 0.34.0, the `TokenRevocationHandler` ignores errors coming from the storage. This can lead to unexpected 200 status codes indicating successful revocation while the token is still valid. Whether an attacker can use this for her advantage depends on the ability to trigger errors in the store. This is fixed in version 0.34.0
CWE description: The product does not check or incorrectly checks for unusual or exceptional conditions that are not expected to occur frequently during day to day operation of the product.
CVSS v3 base score: 8
Published: 2020-09-24T17:15Z
Repository: https://github.com/ory/fosite
Language: Go
Files changed: .golangci.yml, access_error.go, access_write.go, authorize_error.go, errors.go, errors_test.go, go_mod_indirect_pins.go, handler/oauth2/revocation.go, handler/oauth2/revocation_test.go, handler/openid/helper.go, revoke_handler.go, token/hmac/hmacsha.go
Commit date: 2020-09-24 12:00:29 +0200
Commit message: Merge pull request from GHSA-7mqr-2v3q-v2wm

BREAKING CHANGE: `fosite.ErrRevocationClientMismatch` was removed because it is not part of [RFC 6749](https://tools.ietf.org/html/rfc6749#section-5.2). Instead, `fosite.ErrUnauthorizedClient` will be returned when calling `RevokeToken` with an OAuth2 Client which does not match the Access or Refresh Token to be revoked.
Diff: diff --git a/.golangci.yml b/.golangci.yml
index 1a6ec23..41a57ab 100644
--- a/.golangci.yml
+++ b/.golangci.yml
@@ -4,11 +4,10 @@ linters:
   disable:
     - ineffassign
     - deadcode
     - unused
     - structcheck
-    - errcheck
     - gosimple
     - bodyclose
     - staticcheck
 
 run:
diff --git a/access_error.go b/access_error.go
index f003a6a..e48cd56 100644
--- a/access_error.go
+++ b/access_error.go
@@ -51,7 +51,8 @@ func (f *Fosite) writeJsonError(rw http.ResponseWriter, err error) {
 		}
 		return
 	}
 
 	rw.WriteHeader(rfcerr.Code)
-	rw.Write(js)
+	// ignoring the error because the connection is broken when it happens
+	_, _ = rw.Write(js)
 }
diff --git a/access_write.go b/access_write.go
index ba467c2..ce3c7c1 100644
--- a/access_write.go
+++ b/access_write.go
@@ -37,7 +37,7 @@ func (f *Fosite) WriteAccessResponse(rw http.ResponseWriter, requester AccessReq
 	}
 
 	rw.Header().Set("Content-Type", "application/json;charset=UTF-8")
 
 	rw.WriteHeader(http.StatusOK)
-	rw.Write(js)
+	_, _ = rw.Write(js)
 }
diff --git a/authorize_error.go b/authorize_error.go
index 3c038c6..3608ca4 100644
--- a/authorize_error.go
+++ b/authorize_error.go
@@ -51,11 +51,11 @@ func (f *Fosite) WriteAuthorizeError(rw http.ResponseWriter, ar AuthorizeRequest
 			}
 			return
 		}
 
 		rw.WriteHeader(rfcerr.Code)
-		rw.Write(js)
+		_, _ = rw.Write(js)
 		return
 	}
 
 	redirectURI := ar.GetRedirectURI()
 
diff --git a/errors.go b/errors.go
index f6b388b..04009c7 100644
--- a/errors.go
+++ b/errors.go
@@ -161,15 +161,10 @@ var (
 		Name:        errTokenInactiveName,
 		Description: "Token is inactive because it is malformed, expired or otherwise invalid",
 		Hint:        "Token validation failed.",
 		Code:        http.StatusUnauthorized,
 	}
-	ErrRevocationClientMismatch = &RFC6749Error{
-		Name:        errRevocationClientMismatchName,
-		Description: "Token was not issued to the client making the revocation request",
-		Code:        http.StatusBadRequest,
-	}
 	ErrLoginRequired = &RFC6749Error{
 		Name:        errLoginRequired,
 		Description: "The Authorization Server requires End-User authentication",
 		Code:        http.StatusBadRequest,
 	}
diff --git a/errors_test.go b/errors_test.go
index 4e97479..424f722 100644
--- a/errors_test.go
+++ b/errors_test.go
@@ -26,17 +26,10 @@ import (
 
 	"github.com/pkg/errors"
 	"github.com/stretchr/testify/assert"
 )
 
-func TestAddDebug(t *testing.T) {
-	err := ErrRevocationClientMismatch.WithDebug("debug")
-	assert.NotEqual(t, err, ErrRevocationClientMismatch)
-	assert.Empty(t, ErrRevocationClientMismatch.Debug)
-	assert.NotEmpty(t, err.Debug)
-}
-
 func TestIs(t *testing.T) {
 	assert.True(t, errors.Is(ErrUnknownRequest, ErrUnknownRequest))
 	assert.True(t, errors.Is(ErrUnknownRequest, &RFC6749Error{
 		Name: errUnknownErrorName,
 	}))
diff --git a/go_mod_indirect_pins.go b/go_mod_indirect_pins.go
index 79dc9d9..a1f4691 100644
--- a/go_mod_indirect_pins.go
+++ b/go_mod_indirect_pins.go
@@ -3,7 +3,8 @@
 package fosite
 
 import (
 	_ "github.com/gorilla/websocket"
 	_ "github.com/mattn/goveralls"
+
 	_ "github.com/ory/go-acc"
 )
diff --git a/handler/oauth2/revocation.go b/handler/oauth2/revocation.go
index 782b908..aef9bcb 100644
--- a/handler/oauth2/revocation.go
+++ b/handler/oauth2/revocation.go
@@ -55,23 +55,34 @@ func (r *TokenRevocationHandler) RevokeToken(ctx context.Context, token string,
 	if tokenType == fosite.AccessToken {
 		discoveryFuncs[0], discoveryFuncs[1] = discoveryFuncs[1], discoveryFuncs[0]
 	}
 
 	var ar fosite.Requester
-	var err error
-	if ar, err = discoveryFuncs[0](); err != nil {
-		ar, err = discoveryFuncs[1]()
+	var err1, err2 error
+	if ar, err1 = discoveryFuncs[0](); err1 != nil {
+		ar, err2 = discoveryFuncs[1]()
 	}
-	if err != nil {
-		return err
+	// err2 can only be not nil if first err1 was not nil
+	if err2 != nil {
+		return storeErrorsToRevocationError(err1, err2)
 	}
 
 	if ar.GetClient().GetID() != client.GetID() {
-		return errors.WithStack(fosite.ErrRevocationClientMismatch)
+		return errors.WithStack(fosite.ErrUnauthorizedClient)
 	}
 
 	requestID := ar.GetID()
-	r.TokenRevocationStorage.RevokeRefreshToken(ctx, requestID)
-	r.TokenRevocationStorage.RevokeAccessToken(ctx, requestID)
+	err1 = r.TokenRevocationStorage.RevokeRefreshToken(ctx, requestID)
+	err2 = r.TokenRevocationStorage.RevokeAccessToken(ctx, requestID)
 
-	return nil
+	return storeErrorsToRevocationError(err1, err2)
+}
+
+func storeErrorsToRevocationError(err1, err2 error) error {
+	// both errors are 404 or nil <=> the token is revoked
+	if (errors.Is(err1, fosite.ErrNotFound) || err1 == nil) && (errors.Is(err2, fosite.ErrNotFound) || err2 == nil) {
+		return nil
+	}
+
+	// there was an unexpected error => the token may still exist and the client should retry later
+	return errors.WithStack(fosite.ErrTemporarilyUnavailable)
 }
diff --git a/handler/oauth2/revocation_test.go b/handler/oauth2/revocation_test.go
index 81d11aa..0822c3e 100644
--- a/handler/oauth2/revocation_test.go
+++ b/handler/oauth2/revocation_test.go
@@ -55,11 +55,11 @@ func TestRevokeToken(t *testing.T) {
 		expectErr   error
 		client      fosite.Client
 	}{
 		{
 			description: "should fail - token was issued to another client",
-			expectErr:   fosite.ErrRevocationClientMismatch,
+			expectErr:   fosite.ErrUnauthorizedClient,
 			client:      &fosite.DefaultClient{ID: "bar"},
 			mock: func() {
 				token = "foo"
 				tokenType = fosite.RefreshToken
 				rtStrat.EXPECT().RefreshTokenSignature(token)
@@ -132,12 +132,12 @@ func TestRevokeToken(t *testing.T) {
 				store.EXPECT().RevokeRefreshToken(gomock.Any(), gomock.Any())
 				store.EXPECT().RevokeAccessToken(gomock.Any(), gomock.Any())
 			},
 		},
 		{
-			description: "should fail - refresh token discovery first; both tokens not found",
-			expectErr:   fosite.ErrNotFound,
+			description: "should pass - refresh token discovery first; both tokens not found",
+			expectErr:   nil,
 			client:      &fosite.DefaultClient{ID: "bar"},
 			mock: func() {
 				token = "foo"
 				tokenType = fosite.RefreshToken
 				rtStrat.EXPECT().RefreshTokenSignature(token)
@@ -146,12 +146,12 @@ func TestRevokeToken(t *testing.T) {
 				atStrat.EXPECT().AccessTokenSignature(token)
 				store.EXPECT().GetAccessTokenSession(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, fosite.ErrNotFound)
 			},
 		},
 		{
-			description: "should fail - access token discovery first; both tokens not found",
-			expectErr:   fosite.ErrNotFound,
+			description: "should pass - access token discovery first; both tokens not found",
+			expectErr:   nil,
 			client:      &fosite.DefaultClient{ID: "bar"},
 			mock: func() {
 				token = "foo"
 				tokenType = fosite.AccessToken
 				atStrat.EXPECT().AccessTokenSignature(token)
@@ -159,12 +159,72 @@ func TestRevokeToken(t *testing.T) {
 
 				rtStrat.EXPECT().RefreshTokenSignature(token)
 				store.EXPECT().GetRefreshTokenSession(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, fosite.ErrNotFound)
 			},
 		},
+		{
+			description: "should fail - store error for access token get",
+			expectErr:   fosite.ErrTemporarilyUnavailable,
+			client:      &fosite.DefaultClient{ID: "bar"},
+			mock: func() {
+				token = "foo"
+				tokenType = fosite.AccessToken
+				atStrat.EXPECT().AccessTokenSignature(token)
+				store.EXPECT().GetAccessTokenSession(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("random error"))
+
+				rtStrat.EXPECT().RefreshTokenSignature(token)
+				store.EXPECT().GetRefreshTokenSession(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, fosite.ErrNotFound)
+			},
+		},
+		{
+			description: "should fail - store error for refresh token get",
+			expectErr:   fosite.ErrTemporarilyUnavailable,
+			client:      &fosite.DefaultClient{ID: "bar"},
+			mock: func() {
+				token = "foo"
+				tokenType = fosite.RefreshToken
+				atStrat.EXPECT().AccessTokenSignature(token)
+				store.EXPECT().GetAccessTokenSession(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, fosite.ErrNotFound)
+
+				rtStrat.EXPECT().RefreshTokenSignature(token)
+				store.EXPECT().GetRefreshTokenSession(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("random error"))
+			},
+		},
+		{
+			description: "should fail - store error for access token revoke",
+			expectErr:   fosite.ErrTemporarilyUnavailable,
+			client:      &fosite.DefaultClient{ID: "bar"},
+			mock: func() {
+				token = "foo"
+				tokenType = fosite.AccessToken
+				atStrat.EXPECT().AccessTokenSignature(token)
+				store.EXPECT().GetAccessTokenSession(gomock.Any(), gomock.Any(), gomock.Any()).Return(ar, nil)
+
+				ar.EXPECT().GetID()
+				ar.EXPECT().GetClient().Return(&fosite.DefaultClient{ID: "bar"})
+				store.EXPECT().RevokeRefreshToken(gomock.Any(), gomock.Any()).Return(fosite.ErrNotFound)
+				store.EXPECT().RevokeAccessToken(gomock.Any(), gomock.Any()).Return(fmt.Errorf("random error"))
+			},
+		},
+		{
+			description: "should fail - store error for refresh token revoke",
+			expectErr:   fosite.ErrTemporarilyUnavailable,
+			client:      &fosite.DefaultClient{ID: "bar"},
+			mock: func() {
+				token = "foo"
+				tokenType = fosite.RefreshToken
+				rtStrat.EXPECT().RefreshTokenSignature(token)
+				store.EXPECT().GetRefreshTokenSession(gomock.Any(), gomock.Any(), gomock.Any()).Return(ar, nil)
+
+				ar.EXPECT().GetID()
+				ar.EXPECT().GetClient().Return(&fosite.DefaultClient{ID: "bar"})
+				store.EXPECT().RevokeRefreshToken(gomock.Any(), gomock.Any()).Return(fmt.Errorf("random error"))
+				store.EXPECT().RevokeAccessToken(gomock.Any(), gomock.Any()).Return(fosite.ErrNotFound)
+			},
+		},
 	} {
-		t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) {
+		t.Run(fmt.Sprintf("case=%d/description=%s", k, c.description), func(t *testing.T) {
 			c.mock()
 			err := h.RevokeToken(nil, token, tokenType, c.client)
 
 			if c.expectErr != nil {
 				require.EqualError(t, err, c.expectErr.Error())
diff --git a/handler/openid/helper.go b/handler/openid/helper.go
index a935761..3fabfbf 100644
--- a/handler/openid/helper.go
+++ b/handler/openid/helper.go
@@ -37,11 +37,15 @@ type IDTokenHandleHelper struct {
 func (i *IDTokenHandleHelper) GetAccessTokenHash(ctx context.Context, requester fosite.AccessRequester, responder fosite.AccessResponder) string {
 	token := responder.GetAccessToken()
 
 	buffer := bytes.NewBufferString(token)
 	hash := sha256.New()
-	hash.Write(buffer.Bytes())
+	// sha256.digest.Write() always returns nil for err, the panic should never happen
+	_, err := hash.Write(buffer.Bytes())
+	if err != nil {
+		panic(err)
+	}
 	hashBuf := bytes.NewBuffer(hash.Sum([]byte{}))
 	len := hashBuf.Len()
 
 	return base64.RawURLEncoding.EncodeToString(hashBuf.Bytes()[:len/2])
 }
diff --git a/revoke_handler.go b/revoke_handler.go
index 9a69165..4597ca5 100644
--- a/revoke_handler.go
+++ b/revoke_handler.go
@@ -110,22 +110,22 @@ func (f *Fosite) WriteRevocationResponse(rw http.ResponseWriter, err error) {
 			http.Error(rw, fmt.Sprintf(`{"error": "%s"}`, err.Error()), http.StatusInternalServerError)
 			return
 		}
 
 		rw.WriteHeader(ErrInvalidRequest.Code)
-		rw.Write(js)
+		_, _ = rw.Write(js)
 	} else if errors.Is(err, ErrInvalidClient) {
 		rw.Header().Set("Content-Type", "application/json;charset=UTF-8")
 
 		js, err := json.Marshal(ErrInvalidClient)
 		if err != nil {
 			http.Error(rw, fmt.Sprintf(`{"error": "%s"}`, err.Error()), http.StatusInternalServerError)
 			return
 		}
 
 		rw.WriteHeader(ErrInvalidClient.Code)
-		rw.Write(js)
+		_, _ = rw.Write(js)
 	} else {
 		// 200 OK
 		rw.WriteHeader(http.StatusOK)
 	}
 }
diff --git a/token/hmac/hmacsha.go b/token/hmac/hmacsha.go
index e073745..88dd72b 100644
--- a/token/hmac/hmacsha.go
+++ b/token/hmac/hmacsha.go
@@ -167,8 +167,12 @@ func (c *HMACStrategy) Signature(token string) string {
 	return split[1]
 }
 
 func generateHMAC(data []byte, key *[32]byte) []byte {
 	h := hmac.New(sha512.New512_256, key[:])
-	h.Write(data)
+	// sha512.digest.Write() always returns nil for err, the panic should never happen
+	_, err := h.Write(data)
+	if err != nil {
+		panic(err)
+	}
 	return h.Sum(nil)
 }

Vulnerable code:     - errcheck
	rw.Write(js)
	rw.Write(js)
		rw.Write(js)
	ErrRevocationClientMismatch = &RFC6749Error{
		Name:        errRevocationClientMismatchName,
		Description: "Token was not issued to the client making the revocation request",
		Code:        http.StatusBadRequest,
	}
func TestAddDebug(t *testing.T) {
	err := ErrRevocationClientMismatch.WithDebug("debug")
	assert.NotEqual(t, err, ErrRevocationClientMismatch)
	assert.Empty(t, ErrRevocationClientMismatch.Debug)
	assert.NotEmpty(t, err.Debug)
}

	var err error
	if ar, err = discoveryFuncs[0](); err != nil {
		ar, err = discoveryFuncs[1]()
	if err != nil {
		return err
		return errors.WithStack(fosite.ErrRevocationClientMismatch)
	r.TokenRevocationStorage.RevokeRefreshToken(ctx, requestID)
	r.TokenRevocationStorage.RevokeAccessToken(ctx, requestID)
	return nil
			expectErr:   fosite.ErrRevocationClientMismatch,
			description: "should fail - refresh token discovery first; both tokens not found",
			expectErr:   fosite.ErrNotFound,
			description: "should fail - access token discovery first; both tokens not found",
			expectErr:   fosite.ErrNotFound,
		t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) {
	hash.Write(buffer.Bytes())
		rw.Write(js)
		rw.Write(js)
	h.Write(data)
Fixed code: 	// ignoring the error because the connection is broken when it happens
	_, _ = rw.Write(js)
	_, _ = rw.Write(js)
		_, _ = rw.Write(js)

	var err1, err2 error
	if ar, err1 = discoveryFuncs[0](); err1 != nil {
		ar, err2 = discoveryFuncs[1]()
	// err2 can only be not nil if first err1 was not nil
	if err2 != nil {
		return storeErrorsToRevocationError(err1, err2)
		return errors.WithStack(fosite.ErrUnauthorizedClient)
	err1 = r.TokenRevocationStorage.RevokeRefreshToken(ctx, requestID)
	err2 = r.TokenRevocationStorage.RevokeAccessToken(ctx, requestID)
	return storeErrorsToRevocationError(err1, err2)
}

func storeErrorsToRevocationError(err1, err2 error) error {
	// both errors are 404 or nil <=> the token is revoked
	if (errors.Is(err1, fosite.ErrNotFound) || err1 == nil) && (errors.Is(err2, fosite.ErrNotFound) || err2 == nil) {
		return nil
	}

	// there was an unexpected error => the token may still exist and the client should retry later
	return errors.WithStack(fosite.ErrTemporarilyUnavailable)
			expectErr:   fosite.ErrUnauthorizedClient,
			description: "should pass - refresh token discovery first; both tokens not found",
			expectErr:   nil,
			description: "should pass - access token discovery first; both tokens not found",
			expectErr:   nil,
		{
			description: "should fail - store error for access token get",
			expectErr:   fosite.ErrTemporarilyUnavailable,
			client:      &fosite.DefaultClient{ID: "bar"},
			mock: func() {
				token = "foo"
				tokenType = fosite.AccessToken
				atStrat.EXPECT().AccessTokenSignature(token)
				store.EXPECT().GetAccessTokenSession(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("random error"))

				rtStrat.EXPECT().RefreshTokenSignature(token)
				store.EXPECT().GetRefreshTokenSession(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, fosite.ErrNotFound)
			},
		},
		{
			description: "should fail - store error for refresh token get",
			expectErr:   fosite.ErrTemporarilyUnavailable,
			client:      &fosite.DefaultClient{ID: "bar"},
			mock: func() {
				token = "foo"
				tokenType = fosite.RefreshToken
				atStrat.EXPECT().AccessTokenSignature(token)
				store.EXPECT().GetAccessTokenSession(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, fosite.ErrNotFound)

				rtStrat.EXPECT().RefreshTokenSignature(token)
				store.EXPECT().GetRefreshTokenSession(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("random error"))
			},
		},
		{
			description: "should fail - store error for access token revoke",
			expectErr:   fosite.ErrTemporarilyUnavailable,
			client:      &fosite.DefaultClient{ID: "bar"},
			mock: func() {
				token = "foo"
				tokenType = fosite.AccessToken
				atStrat.EXPECT().AccessTokenSignature(token)
				store.EXPECT().GetAccessTokenSession(gomock.Any(), gomock.Any(), gomock.Any()).Return(ar, nil)

				ar.EXPECT().GetID()
				ar.EXPECT().GetClient().Return(&fosite.DefaultClient{ID: "bar"})
				store.EXPECT().RevokeRefreshToken(gomock.Any(), gomock.Any()).Return(fosite.ErrNotFound)
				store.EXPECT().RevokeAccessToken(gomock.Any(), gomock.Any()).Return(fmt.Errorf("random error"))
			},
		},
		{
			description: "should fail - store error for refresh token revoke",
			expectErr:   fosite.ErrTemporarilyUnavailable,
			client:      &fosite.DefaultClient{ID: "bar"},
			mock: func() {
				token = "foo"
				tokenType = fosite.RefreshToken
				rtStrat.EXPECT().RefreshTokenSignature(token)
				store.EXPECT().GetRefreshTokenSession(gomock.Any(), gomock.Any(), gomock.Any()).Return(ar, nil)

				ar.EXPECT().GetID()
				ar.EXPECT().GetClient().Return(&fosite.DefaultClient{ID: "bar"})
				store.EXPECT().RevokeRefreshToken(gomock.Any(), gomock.Any()).Return(fmt.Errorf("random error"))
				store.EXPECT().RevokeAccessToken(gomock.Any(), gomock.Any()).Return(fosite.ErrNotFound)
			},
		},
		t.Run(fmt.Sprintf("case=%d/description=%s", k, c.description), func(t *testing.T) {
	// sha256.digest.Write() always returns nil for err, the panic should never happen
	_, err := hash.Write(buffer.Bytes())
	if err != nil {
		panic(err)
	}
		_, _ = rw.Write(js)
		_, _ = rw.Write(js)
	// sha512.digest.Write() always returns nil for err, the panic should never happen
	_, err := h.Write(data)
	if err != nil {
		panic(err)
	}