diff --git a/CHANGELOG.md b/CHANGELOG.md index 42c9e030..54a5d607 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ - [#1873](https://github.com/oauth2-proxy/oauth2-proxy/pull/1873) Fix empty users with some OIDC providers (@babs) - [#1882](https://github.com/oauth2-proxy/oauth2-proxy/pull/1882) Make `htpasswd.GetUsers` racecondition safe - [#1883](https://github.com/oauth2-proxy/oauth2-proxy/pull/1883) Ensure v8 manifest variant is set on docker images +- [#1906](https://github.com/oauth2-proxy/oauth2-proxy/pull/1906) Fix PKCE code verifier generation to never use UTF-8 characters # V7.4.0 diff --git a/oauthproxy.go b/oauthproxy.go index d11040c3..5b50bf5e 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -2,7 +2,6 @@ package main import ( "context" - "encoding/base64" "encoding/json" "errors" "fmt" @@ -737,16 +736,18 @@ func (p *OAuthProxy) doOAuthStart(rw http.ResponseWriter, req *http.Request, ove extraParams := p.provider.Data().LoginURLParams(overrides) prepareNoCache(rw) - var codeChallenge, codeVerifier, codeChallengeMethod string + var ( + err error + codeChallenge, codeVerifier, codeChallengeMethod string + ) if p.provider.Data().CodeChallengeMethod != "" { codeChallengeMethod = p.provider.Data().CodeChallengeMethod - preEncodedCodeVerifier, err := encryption.Nonce(96) + codeVerifier, err = encryption.GenerateRandomASCIIString(96) if err != nil { - logger.Errorf("Unable to build random string: %v", err) + logger.Errorf("Unable to build random ASCII string for code verifier: %v", err) p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error()) return } - codeVerifier = base64.RawURLEncoding.EncodeToString(preEncodedCodeVerifier) codeChallenge, err = encryption.GenerateCodeChallenge(p.provider.Data().CodeChallengeMethod, codeVerifier) if err != nil { diff --git a/pkg/encryption/utils.go b/pkg/encryption/utils.go index cd876400..6db1b453 100644 --- a/pkg/encryption/utils.go +++ b/pkg/encryption/utils.go @@ -2,10 +2,12 @@ package encryption import ( "crypto/hmac" + "crypto/rand" "crypto/sha256" "encoding/base64" "fmt" "hash" + "math/big" "net/http" "strconv" "strings" @@ -15,6 +17,7 @@ import ( const ( CodeChallengeMethodPlain = "plain" CodeChallengeMethodS256 = "S256" + asciiCharset = " !\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~" ) // SecretBytes attempts to base64 decode the secret, if that fails it treats the secret as binary @@ -80,6 +83,19 @@ func SignedValue(seed string, key string, value []byte, now time.Time) (string, return cookieVal, nil } +func GenerateRandomASCIIString(length int) (string, error) { + b := make([]byte, length) + charsetLen := new(big.Int).SetInt64(int64(len(asciiCharset))) + for i := range b { + character, err := rand.Int(rand.Reader, charsetLen) + if err != nil { + return "", err + } + b[i] = asciiCharset[character.Int64()] + } + return string(b), nil +} + func GenerateCodeChallenge(method, codeVerifier string) (string, error) { switch method { case CodeChallengeMethodPlain: diff --git a/pkg/encryption/utils_test.go b/pkg/encryption/utils_test.go index 2500d4ab..cc341ec4 100644 --- a/pkg/encryption/utils_test.go +++ b/pkg/encryption/utils_test.go @@ -7,7 +7,9 @@ import ( "encoding/base64" "fmt" "io" + "strings" "testing" + "unicode" "github.com/stretchr/testify/assert" ) @@ -100,3 +102,20 @@ func TestSignAndValidate(t *testing.T) { assert.False(t, checkSignature(sha256sig, seed, key, "tampered", epoch)) assert.False(t, checkSignature(sha1sig, seed, key, "tampered", epoch)) } + +func TestGenerateRandomASCIIString(t *testing.T) { + randomString, err := GenerateRandomASCIIString(96) + assert.NoError(t, err) + + // Only 8-bit characters + assert.Equal(t, 96, len([]byte(randomString))) + + // All non-ascii characters removed should still be the original string + removedChars := strings.Map(func(r rune) rune { + if r > unicode.MaxASCII { + return -1 + } + return r + }, randomString) + assert.Equal(t, removedChars, randomString) +}