Merge pull request from GHSA-2m7h-86qq-fp4v

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>

fix references

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>

use long enough state param for oauth2

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>

typo

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>

more entropy

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>

fix tests

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>

fix tests/lint

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>

cmd/argocd/commands/login.go

@@ -202,7 +202,10 @@ func oauth2Login(ctx context.Context, port int, oidcSettings *settingspkg.OIDCCo
 	// completionChan is to signal flow completed. Non-empty string indicates error
 	completionChan := make(chan string)
 	// stateNonce is an OAuth2 state nonce
-	stateNonce := rand.RandString(10)
+	// According to the spec (https://www.rfc-editor.org/rfc/rfc6749#section-10.10), this must be guessable with
+	// probability <= 2^(-128). The following call generates one of 52^24 random strings, ~= 2^136 possibilities.
+	stateNonce, err := rand.String(24)
+	errors.CheckError(err)
 	var tokenString string
 	var refreshToken string
 
@@ -212,7 +215,8 @@ func oauth2Login(ctx context.Context, port int, oidcSettings *settingspkg.OIDCCo
 	}
 
 	// PKCE implementation of https://tools.ietf.org/html/rfc7636
-	codeVerifier := rand.RandStringCharset(43, "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~")
+	codeVerifier, err := rand.StringFromCharset(43, "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~")
+	errors.CheckError(err)
 	codeChallengeHash := sha256.Sum256([]byte(codeVerifier))
 	codeChallenge := base64.RawURLEncoding.EncodeToString(codeChallengeHash[:])
 
@@ -296,7 +300,8 @@ func oauth2Login(ctx context.Context, port int, oidcSettings *settingspkg.OIDCCo
 		opts = append(opts, oauth2.SetAuthURLParam("code_challenge_method", "S256"))
 		url = oauth2conf.AuthCodeURL(stateNonce, opts...)
 	case oidcutil.GrantTypeImplicit:
-		url = oidcutil.ImplicitFlowURL(oauth2conf, stateNonce, opts...)
+		url, err = oidcutil.ImplicitFlowURL(oauth2conf, stateNonce, opts...)
+		errors.CheckError(err)
 	default:
 		log.Fatalf("Unsupported grant type: %v", grantType)
 	}

controller/sync.go

@@ -149,7 +149,13 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, state *v1alpha
 	}
 
 	atomic.AddUint64(&syncIdPrefix, 1)
-	syncId := fmt.Sprintf("%05d-%s", syncIdPrefix, rand.RandString(5))
+	randSuffix, err := rand.String(5)
+	if err != nil {
+		state.Phase = common.OperationError
+		state.Message = fmt.Sprintf("Failed generate random sync ID: %v", err)
+		return
+	}
+	syncId := fmt.Sprintf("%05d-%s", syncIdPrefix, randSuffix)
 
 	logEntry := log.WithFields(log.Fields{"application": app.Name, "syncId": syncId})
 	initialResourcesRes := make([]common.ResourceSyncResult, 0)

pkg/apiclient/grpcproxy.go

@@ -100,7 +100,11 @@ func (c *client) executeRequest(fullMethodName string, msg []byte, md metadata.M
 }
 
 func (c *client) startGRPCProxy() (*grpc.Server, net.Listener, error) {
-	serverAddr := fmt.Sprintf("%s/argocd-%s.sock", os.TempDir(), rand.RandString(16))
+	randSuffix, err := rand.String(16)
+	if err != nil {
+		return nil, nil, fmt.Errorf("failed to generate random socket filename: %w", err)
+	}
+	serverAddr := fmt.Sprintf("%s/argocd-%s.sock", os.TempDir(), randSuffix)
 	ln, err := net.Listen("unix", serverAddr)
 
 	if err != nil {

test/e2e/fixture/fixture.go

@@ -561,7 +561,9 @@ func EnsureCleanState(t *testing.T) {
 	FailOnErr(Run("", "mkdir", "-p", TmpDir))
 
 	// random id - unique across test runs
-	postFix := "-" + strings.ToLower(rand.RandString(5))
+	randString, err := rand.String(5)
+	CheckError(err)
+	postFix := "-" + strings.ToLower(randString)
 	id = t.Name() + postFix
 	name = DnsFriendly(t.Name(), "")
 	deploymentNamespace = DnsFriendly(fmt.Sprintf("argocd-e2e-%s", t.Name()), postFix)

test/e2e/selective_sync_test.go

@@ -7,6 +7,7 @@ import (
 
 	"github.com/argoproj/gitops-engine/pkg/health"
 	. "github.com/argoproj/gitops-engine/pkg/sync/common"
+	"github.com/stretchr/testify/require"
 
 	. "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
 	"github.com/argoproj/argo-cd/v2/test/e2e/fixture"
@@ -110,7 +111,9 @@ func TestSelectiveSyncWithNamespace(t *testing.T) {
 }
 
 func getNewNamespace(t *testing.T) string {
-	postFix := "-" + strings.ToLower(rand.RandString(5))
+	randStr, err := rand.String(5)
+	require.NoError(t, err)
+	postFix := "-" + strings.ToLower(randStr)
 	name := fixture.DnsFriendly(t.Name(), "")
 	return fixture.DnsFriendly(fmt.Sprintf("argocd-e2e-%s", name), postFix)
 }

util/oidc/oidc.go

@@ -144,7 +144,12 @@ func (a *ClientApp) oauth2Config(scopes []string) (*oauth2.Config, error) {
 
 // generateAppState creates an app state nonce
 func (a *ClientApp) generateAppState(returnURL string, w http.ResponseWriter) (string, error) {
-	randStr := rand.RandString(10)
+	// According to the spec (https://www.rfc-editor.org/rfc/rfc6749#section-10.10), this must be guessable with
+	// probability <= 2^(-128). The following call generates one of 52^24 random strings, ~= 2^136 possibilities.
+	randStr, err := rand.String(24)
+	if err != nil {
+		return "", fmt.Errorf("failed to generate app state: %w", err)
+	}
 	if returnURL == "" {
 		returnURL = a.baseHRef
 	}
@@ -283,7 +288,12 @@ func (a *ClientApp) HandleLogin(w http.ResponseWriter, r *http.Request) {
 	case GrantTypeAuthorizationCode:
 		url = oauth2Config.AuthCodeURL(stateNonce, opts...)
 	case GrantTypeImplicit:
-		url = ImplicitFlowURL(oauth2Config, stateNonce, opts...)
+		url, err = ImplicitFlowURL(oauth2Config, stateNonce, opts...)
+		if err != nil {
+			log.Errorf("Failed to initiate implicit login flow: %v", err)
+			http.Error(w, "Failed to initiate implicit login flow", http.StatusInternalServerError)
+			return
+		}
 	default:
 		http.Error(w, fmt.Sprintf("Unsupported grant type: %v", grantType), http.StatusInternalServerError)
 		return
@@ -415,10 +425,14 @@ func (a *ClientApp) handleImplicitFlow(r *http.Request, w http.ResponseWriter, s
 
 // ImplicitFlowURL is an adaptation of oauth2.Config::AuthCodeURL() which returns a URL
 // appropriate for an OAuth2 implicit login flow (as opposed to authorization code flow).
-func ImplicitFlowURL(c *oauth2.Config, state string, opts ...oauth2.AuthCodeOption) string {
+func ImplicitFlowURL(c *oauth2.Config, state string, opts ...oauth2.AuthCodeOption) (string, error) {
 	opts = append(opts, oauth2.SetAuthURLParam("response_type", "id_token"))
-	opts = append(opts, oauth2.SetAuthURLParam("nonce", rand.RandString(10)))
-	return c.AuthCodeURL(state, opts...)
+	randString, err := rand.String(24)
+	if err != nil {
+		return "", fmt.Errorf("failed to generate nonce for implicit flow URL: %w", err)
+	}
+	opts = append(opts, oauth2.SetAuthURLParam("nonce", randString))
+	return c.AuthCodeURL(state, opts...), nil
 }
 
 // OfflineAccess returns whether or not 'offline_access' is a supported scope

util/rand/rand.go

@@ -1,37 +1,30 @@
 package rand
 
 import (
-	"math/rand"
-	"time"
+	"crypto/rand"
+	"fmt"
+	"math/big"
 )
 
 const letterBytes = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ"
-const (
-	letterIdxBits = 6                    // 6 bits to represent a letter index
-	letterIdxMask = 1<<letterIdxBits - 1 // All 1-bits, as many as letterIdxBits
-	letterIdxMax  = 63 / letterIdxBits   // # of letter indices fitting in 63 bits
-)
-
-var src = rand.NewSource(time.Now().UnixNano())
 
-// RandString generates, from a given charset, a cryptographically-secure pseudo-random string of a given length.
-func RandString(n int) string {
-	return RandStringCharset(n, letterBytes)
+// String generates, from the set of capital and lowercase letters, a cryptographically-secure pseudo-random string of a given length.
+func String(n int) (string, error) {
+	return StringFromCharset(n, letterBytes)
 }
 
-func RandStringCharset(n int, charset string) string {
+// StringFromCharset generates, from a given charset, a cryptographically-secure pseudo-random string of a given length.
+func StringFromCharset(n int, charset string) (string, error) {
 	b := make([]byte, n)
-	// A src.Int63() generates 63 random bits, enough for letterIdxMax characters!
-	for i, cache, remain := n-1, src.Int63(), letterIdxMax; i >= 0; {
-		if remain == 0 {
-			cache, remain = src.Int63(), letterIdxMax
-		}
-		if idx := int(cache & letterIdxMask); idx < len(charset) {
-			b[i] = charset[idx]
-			i--
+	maxIdx := big.NewInt(int64(len(charset)))
+	for i := 0; i < n; i++ {
+		randIdx, err := rand.Int(rand.Reader, maxIdx)
+		if err != nil {
+			return "", fmt.Errorf("failed to generate random string: %w", err)
 		}
-		cache >>= letterIdxBits
-		remain--
+		// randIdx is necessarily safe to convert to int, because the max came from an int.
+		randIdxInt := int(randIdx.Int64())
+		b[i] = charset[randIdxInt]
 	}
-	return string(b)
+	return string(b), nil
 }

util/rand/rand_test.go

@@ -2,15 +2,17 @@ package rand
 
 import (
 	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestRandString(t *testing.T) {
-	ss := RandStringCharset(10, "A")
-	if ss != "AAAAAAAAAA" {
-		t.Errorf("Expected 10 As, but got %q", ss)
-	}
-	ss = RandStringCharset(5, "ABC123")
-	if len(ss) != 5 {
-		t.Errorf("Expected random string of length 10, but got %q", ss)
-	}
+	ss, err := StringFromCharset(10, "A")
+	require.NoError(t, err)
+	assert.Equal(t, "AAAAAAAAAA", ss)
+
+	ss, err = StringFromCharset(5, "ABC123")
+	require.NoError(t, err)
+	assert.Len(t, ss, 5)
 }