diff --git a/CHANGELOG.md b/CHANGELOG.md index 4c6b2e4d..d0952d38 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,8 +32,16 @@ - In some scenarios `X-Forwarded-User` will now be empty. Use `X-Forwarded-Email` instead. - In some scenarios, this may break setting Basic Auth on upstream or responses. Use `--prefer-email-to-user` to restore falling back to the Email in these cases. +- [#556](https://github.com/oauth2-proxy/oauth2-proxy/pull/556) Remove unintentional auto-padding of secrets that were too short + - Previously, after cookie-secrets were opportunistically base64 decoded to raw bytes, + they were padded to have a length divisible by 4. + - This led to wrong sized secrets being valid AES lengths of 16, 24, or 32 bytes. Or it led to confusing errors + reporting an invalid length of 20 or 28 when the user input cookie-secret was not that length. + - Now we will only base64 decode a cookie-secret to raw bytes if it is 16, 24, or 32 bytes long. Otherwise, we will convert + the direct cookie-secret to bytes without silent padding added. ## Changes since v5.1.1 +- [#556](https://github.com/oauth2-proxy/oauth2-proxy/pull/556) Remove unintentional auto-padding of secrets that were too short (@NickMeves) - [#538](https://github.com/oauth2-proxy/oauth2-proxy/pull/538) Refactor sessions/utils.go functionality to other areas (@NickMeves) - [#503](https://github.com/oauth2-proxy/oauth2-proxy/pull/503) Implements --real-client-ip-header option to select the header from which to obtain a proxied client's IP (@Izzette) - [#529](https://github.com/oauth2-proxy/oauth2-proxy/pull/529) Add local test environments for testing changes and new features (@JoelSpeed) diff --git a/oauthproxy_test.go b/oauthproxy_test.go index 33868f79..a08b1640 100644 --- a/oauthproxy_test.go +++ b/oauthproxy_test.go @@ -898,7 +898,7 @@ func NewProcessCookieTest(opts ProcessCookieTestOpts, modifiers ...OptionsModifi } pcTest.opts.ClientID = "asdfljk" pcTest.opts.ClientSecret = "lkjfdsig" - pcTest.opts.Cookie.Secret = "0123456789abcdefabcd" + pcTest.opts.Cookie.Secret = "0123456789abcdef0123456789abcdef" // First, set the CookieRefresh option so proxy.AesCipher is created, // needed to encrypt the access_token. pcTest.opts.Cookie.Refresh = time.Hour diff --git a/options_test.go b/options_test.go index ae978b6a..c6c2d3b5 100644 --- a/options_test.go +++ b/options_test.go @@ -233,7 +233,7 @@ func TestCookieRefreshMustBeLessThanCookieExpire(t *testing.T) { o := testOptions() assert.Equal(t, nil, o.Validate()) - o.Cookie.Secret = "0123456789abcdefabcd" + o.Cookie.Secret = "0123456789abcdef" o.Cookie.Refresh = o.Cookie.Expire assert.NotEqual(t, nil, o.Validate()) diff --git a/pkg/encryption/cipher.go b/pkg/encryption/cipher.go index 05952d8e..2dcbee6b 100644 --- a/pkg/encryption/cipher.go +++ b/pkg/encryption/cipher.go @@ -19,27 +19,22 @@ import ( // SecretBytes attempts to base64 decode the secret, if that fails it treats the secret as binary func SecretBytes(secret string) []byte { - b, err := base64.URLEncoding.DecodeString(addPadding(secret)) + b, err := base64.RawURLEncoding.DecodeString(strings.TrimRight(secret, "=")) if err == nil { - return []byte(addPadding(string(b))) + // Only return decoded form if a valid AES length + // Don't want unintentional decoding resulting in invalid lengths confusing a user + // that thought they used a 16, 24, 32 length string + for _, i := range []int{16, 24, 32} { + if len(b) == i { + return b + } + } } + // If decoding didn't work or resulted in non-AES compliant length, + // assume the raw string was the intended secret return []byte(secret) } -func addPadding(secret string) string { - padding := len(secret) % 4 - switch padding { - case 1: - return secret + "===" - case 2: - return secret + "==" - case 3: - return secret + "=" - default: - return secret - } -} - // cookies are stored in a 3 part (value + timestamp + signature) to enforce that the values are as originally set. // additionally, the 'value' is encrypted so it's opaque to the browser diff --git a/pkg/encryption/cipher_test.go b/pkg/encryption/cipher_test.go index 37026ab1..76bfc1bc 100644 --- a/pkg/encryption/cipher_test.go +++ b/pkg/encryption/cipher_test.go @@ -1,14 +1,87 @@ package encryption import ( + "crypto/rand" "crypto/sha1" "crypto/sha256" "encoding/base64" + "fmt" + "io" "testing" "github.com/stretchr/testify/assert" ) +func TestSecretBytesEncoded(t *testing.T) { + for _, secretSize := range []int{16, 24, 32} { + t.Run(fmt.Sprintf("%d", secretSize), func(t *testing.T) { + secret := make([]byte, secretSize) + _, err := io.ReadFull(rand.Reader, secret) + assert.Equal(t, nil, err) + + // We test both padded & raw Base64 to ensure we handle both + // potential user input routes for Base64 + base64Padded := base64.URLEncoding.EncodeToString(secret) + sb := SecretBytes(base64Padded) + assert.Equal(t, secret, sb) + assert.Equal(t, len(sb), secretSize) + + base64Raw := base64.RawURLEncoding.EncodeToString(secret) + sb = SecretBytes(base64Raw) + assert.Equal(t, secret, sb) + assert.Equal(t, len(sb), secretSize) + }) + } +} + +// A string that isn't intended as Base64 and still decodes (but to unintended length) +// will return the original secret as bytes +func TestSecretBytesEncodedWrongSize(t *testing.T) { + for _, secretSize := range []int{15, 20, 28, 33, 44} { + t.Run(fmt.Sprintf("%d", secretSize), func(t *testing.T) { + secret := make([]byte, secretSize) + _, err := io.ReadFull(rand.Reader, secret) + assert.Equal(t, nil, err) + + // We test both padded & raw Base64 to ensure we handle both + // potential user input routes for Base64 + base64Padded := base64.URLEncoding.EncodeToString(secret) + sb := SecretBytes(base64Padded) + assert.NotEqual(t, secret, sb) + assert.NotEqual(t, len(sb), secretSize) + // The given secret is returned as []byte + assert.Equal(t, base64Padded, string(sb)) + + base64Raw := base64.RawURLEncoding.EncodeToString(secret) + sb = SecretBytes(base64Raw) + assert.NotEqual(t, secret, sb) + assert.NotEqual(t, len(sb), secretSize) + // The given secret is returned as []byte + assert.Equal(t, base64Raw, string(sb)) + }) + } +} + +func TestSecretBytesNonBase64(t *testing.T) { + trailer := "equals==========" + assert.Equal(t, trailer, string(SecretBytes(trailer))) + + raw16 := "asdflkjhqwer)(*&" + sb16 := SecretBytes(raw16) + assert.Equal(t, raw16, string(sb16)) + assert.Equal(t, 16, len(sb16)) + + raw24 := "asdflkjhqwer)(*&CJEN#$%^" + sb24 := SecretBytes(raw24) + assert.Equal(t, raw24, string(sb24)) + assert.Equal(t, 24, len(sb24)) + + raw32 := "asdflkjhqwer)(*&1234lkjhqwer)(*&" + sb32 := SecretBytes(raw32) + assert.Equal(t, raw32, string(sb32)) + assert.Equal(t, 32, len(sb32)) +} + func TestSignAndValidate(t *testing.T) { seed := "0123456789abcdef" key := "cookie-name"