git.iko.soy/typeable/oauth2-proxy
on master help sign in
Fix PKCE code verifier generation to never use UTF-8 characters

- This could result in intermittent/random failures of PKCE enabled IdP's

commitf4f5b7756c77d5686dc11185216a0b0227b76bd2 authorBraunson M <5280764+braunsonm@users.noreply.github.com> dateFri 18 Nov 2022, 19:43 (4 years ago) parentfd2807c
4 files changed+42−5
compare with master
unifiedsplit
CHANGELOG.md+1 oauthproxy.go+6−5 pkg/encryption/utils.go+16 pkg/encryption/utils_test.go+19
CHANGELOG.md+1
@@ -11,6 +11,7 @@
1111- [#1873](https://github.com/oauth2-proxy/oauth2-proxy/pull/1873) Fix empty users with some OIDC providers (@babs)
1212- [#1882](https://github.com/oauth2-proxy/oauth2-proxy/pull/1882) Make `htpasswd.GetUsers` racecondition safe
1313- [#1883](https://github.com/oauth2-proxy/oauth2-proxy/pull/1883) Ensure v8 manifest variant is set on docker images
14+- [#1906](https://github.com/oauth2-proxy/oauth2-proxy/pull/1906) Fix PKCE code verifier generation to never use UTF-8 characters
1415
1516# V7.4.0
1617
oauthproxy.go+6−5
@@ -2,7 +2,6 @@ package main
22
33import (
44 "context"
5− "encoding/base64"
65 "encoding/json"
76 "errors"
87 "fmt"
@@ -737,16 +736,18 @@ func (p *OAuthProxy) doOAuthStart(rw http.ResponseWriter, req *http.Request, ove
737736 extraParams := p.provider.Data().LoginURLParams(overrides)
738737 prepareNoCache(rw)
739738
740− var codeChallenge, codeVerifier, codeChallengeMethod string
739+ var (
740+ err error
741+ codeChallenge, codeVerifier, codeChallengeMethod string
742+ )
741743 if p.provider.Data().CodeChallengeMethod != "" {
742744 codeChallengeMethod = p.provider.Data().CodeChallengeMethod
743− preEncodedCodeVerifier, err := encryption.Nonce(96)
745+ codeVerifier, err = encryption.GenerateRandomASCIIString(96)
744746 if err != nil {
745− logger.Errorf("Unable to build random string: %v", err)
747+ logger.Errorf("Unable to build random ASCII string for code verifier: %v", err)
746748 p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())
747749 return
748750 }
749− codeVerifier = base64.RawURLEncoding.EncodeToString(preEncodedCodeVerifier)
750751
751752 codeChallenge, err = encryption.GenerateCodeChallenge(p.provider.Data().CodeChallengeMethod, codeVerifier)
752753 if err != nil {
pkg/encryption/utils.go+16
@@ -2,10 +2,12 @@ package encryption
22
33import (
44 "crypto/hmac"
5+ "crypto/rand"
56 "crypto/sha256"
67 "encoding/base64"
78 "fmt"
89 "hash"
10+ "math/big"
911 "net/http"
1012 "strconv"
1113 "strings"
@@ -15,6 +17,7 @@ import (
1517const (
1618 CodeChallengeMethodPlain = "plain"
1719 CodeChallengeMethodS256 = "S256"
20+ asciiCharset = " !\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~"
1821)
1922
2023// 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,
8083 return cookieVal, nil
8184}
8285
86+func GenerateRandomASCIIString(length int) (string, error) {
87+ b := make([]byte, length)
88+ charsetLen := new(big.Int).SetInt64(int64(len(asciiCharset)))
89+ for i := range b {
90+ character, err := rand.Int(rand.Reader, charsetLen)
91+ if err != nil {
92+ return "", err
93+ }
94+ b[i] = asciiCharset[character.Int64()]
95+ }
96+ return string(b), nil
97+}
98+
8399func GenerateCodeChallenge(method, codeVerifier string) (string, error) {
84100 switch method {
85101 case CodeChallengeMethodPlain:
pkg/encryption/utils_test.go+19
@@ -7,7 +7,9 @@ import (
77 "encoding/base64"
88 "fmt"
99 "io"
10+ "strings"
1011 "testing"
12+ "unicode"
1113
1214 "github.com/stretchr/testify/assert"
1315)
@@ -100,3 +102,20 @@ func TestSignAndValidate(t *testing.T) {
100102 assert.False(t, checkSignature(sha256sig, seed, key, "tampered", epoch))
101103 assert.False(t, checkSignature(sha1sig, seed, key, "tampered", epoch))
102104}
105+
106+func TestGenerateRandomASCIIString(t *testing.T) {
107+ randomString, err := GenerateRandomASCIIString(96)
108+ assert.NoError(t, err)
109+
110+ // Only 8-bit characters
111+ assert.Equal(t, 96, len([]byte(randomString)))
112+
113+ // All non-ascii characters removed should still be the original string
114+ removedChars := strings.Map(func(r rune) rune {
115+ if r > unicode.MaxASCII {
116+ return -1
117+ }
118+ return r
119+ }, randomString)
120+ assert.Equal(t, removedChars, randomString)
121+}