Browse Source

Harden provider CodeQL findings

Signed-off-by: Moritz Johner <beller.moritz@googlemail.com>
Moritz Johner 2 months ago
parent
commit
0e17374e69

+ 2 - 1
.github/config/codeql-config.yaml

@@ -51,6 +51,8 @@ query-filters:
   - exclude:
       id:
         - go/untrusted-data-to-external-api
+        # Removed from the official Go suites and noisy for our enum/default string literals.
+        - go/hardcoded-credentials
 
   # Remove debugging, and audit queries used by community packs (this is duplicative of the default suites from those community packs)
   - exclude:
@@ -85,4 +87,3 @@ paths-ignore:
   - "**/gems/**"
   - "**/spec/**/*_spec.rb"
   - "**/test/**/*_test.rb"
-

+ 35 - 4
providers/v2/common/grpc/server/tls.go

@@ -21,6 +21,8 @@ import (
 	"crypto/x509"
 	"fmt"
 	"os"
+	"path/filepath"
+	"strings"
 )
 
 const (
@@ -57,8 +59,14 @@ func DefaultTLSConfig() *TLSConfig {
 // This enables mTLS, requiring and verifying client certificates.
 func LoadTLSConfig(config *TLSConfig) (*tls.Config, error) {
 	// Load server certificate and key
-	certPath := fmt.Sprintf("%s/%s", config.CertDir, config.CertFile)
-	keyPath := fmt.Sprintf("%s/%s", config.CertDir, config.KeyFile)
+	certPath, err := resolveCertPath(config.CertDir, config.CertFile)
+	if err != nil {
+		return nil, err
+	}
+	keyPath, err := resolveCertPath(config.CertDir, config.KeyFile)
+	if err != nil {
+		return nil, err
+	}
 
 	cert, err := tls.LoadX509KeyPair(certPath, keyPath)
 	if err != nil {
@@ -66,8 +74,11 @@ func LoadTLSConfig(config *TLSConfig) (*tls.Config, error) {
 	}
 
 	// Load CA certificate for client verification
-	caPath := fmt.Sprintf("%s/%s", config.CertDir, config.CACertFile)
-	// #nosec G304 -- TLSConfig paths are explicit operator-provided mount points.
+	caPath, err := resolveCertPath(config.CertDir, config.CACertFile)
+	if err != nil {
+		return nil, err
+	}
+	// #nosec G304 -- resolveCertPath constrains file names to direct children of CertDir.
 	caCert, err := os.ReadFile(caPath)
 	if err != nil {
 		return nil, fmt.Errorf("failed to load CA certificate: %w", err)
@@ -108,3 +119,23 @@ func getEnvOrDefault(key, defaultValue string) string {
 	}
 	return defaultValue
 }
+
+func resolveCertPath(certDir, fileName string) (string, error) {
+	cleanDir := filepath.Clean(certDir)
+	cleanFile := filepath.Clean(fileName)
+
+	if filepath.IsAbs(cleanFile) || cleanFile == "." || cleanFile == ".." || cleanFile != filepath.Base(cleanFile) {
+		return "", fmt.Errorf("invalid TLS file name %q", fileName)
+	}
+
+	fullPath := filepath.Join(cleanDir, cleanFile)
+	relPath, err := filepath.Rel(cleanDir, fullPath)
+	if err != nil {
+		return "", fmt.Errorf("resolve TLS file path: %w", err)
+	}
+	if relPath == ".." || strings.HasPrefix(relPath, ".."+string(filepath.Separator)) {
+		return "", fmt.Errorf("invalid TLS file name %q", fileName)
+	}
+
+	return fullPath, nil
+}

+ 77 - 0
providers/v2/common/grpc/server/tls_test.go

@@ -0,0 +1,77 @@
+/*
+Copyright © The ESO Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    https://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package server
+
+import (
+	"path/filepath"
+	"testing"
+)
+
+func TestResolveCertPath(t *testing.T) {
+	t.Parallel()
+
+	certDir := filepath.Join(string(filepath.Separator), "etc", "provider", "certs")
+
+	tests := []struct {
+		name     string
+		fileName string
+		want     string
+		wantErr  bool
+	}{
+		{
+			name:     "accepts simple filename",
+			fileName: "ca.crt",
+			want:     filepath.Join(certDir, "ca.crt"),
+		},
+		{
+			name:     "rejects nested path",
+			fileName: filepath.Join("nested", "ca.crt"),
+			wantErr:  true,
+		},
+		{
+			name:     "rejects traversal",
+			fileName: filepath.Join("..", "ca.crt"),
+			wantErr:  true,
+		},
+		{
+			name:     "rejects absolute path",
+			fileName: filepath.Join(string(filepath.Separator), "tmp", "ca.crt"),
+			wantErr:  true,
+		},
+	}
+
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			got, err := resolveCertPath(certDir, tt.fileName)
+			if tt.wantErr {
+				if err == nil {
+					t.Fatalf("resolveCertPath(%q, %q) error = nil, want error", certDir, tt.fileName)
+				}
+				return
+			}
+			if err != nil {
+				t.Fatalf("resolveCertPath(%q, %q) error = %v", certDir, tt.fileName, err)
+			}
+			if got != tt.want {
+				t.Fatalf("resolveCertPath(%q, %q) = %q, want %q", certDir, tt.fileName, got, tt.want)
+			}
+		})
+	}
+}

+ 4 - 0
providers/v2/hack/generate-provider-main.go

@@ -167,6 +167,7 @@ func main() {
 		providerName := logSafeValue(config.Provider.Name)
 		providerDisplayName := logSafeValue(config.Provider.DisplayName)
 		if *verbose {
+			// # codeql[go/log-injection] -- logSafeValue quotes and ASCII-escapes provider metadata for logs.
 			log.Printf("  Provider: %s (%s)", providerName, providerDisplayName)
 			log.Printf("  Stores: %d, Generators: %d", len(config.Stores), len(config.Generators))
 		}
@@ -177,12 +178,14 @@ func main() {
 		// Generate main.go
 		mainContent, err := executeTemplate(mainTemplate, templateData)
 		if err != nil {
+			// # codeql[go/log-injection] -- providerName is sanitized with logSafeValue above.
 			log.Fatalf("Failed to generate main.go for %s: %v", providerName, err)
 		}
 
 		// Format with goimports/gofmt
 		formattedMain, err := formatGoCode(mainContent)
 		if err != nil {
+			// # codeql[go/log-injection] -- providerName is sanitized with logSafeValue above.
 			log.Printf("Warning: Failed to format main.go for %s: %v", providerName, err)
 			formattedMain = mainContent // Use unformatted if formatting fails
 		}
@@ -200,6 +203,7 @@ func main() {
 		// Generate Dockerfile
 		dockerContent, err := executeTemplate(dockerfileTemplate, templateData)
 		if err != nil {
+			// # codeql[go/log-injection] -- providerName is sanitized with logSafeValue above.
 			log.Fatalf("Failed to generate Dockerfile for %s: %v", providerName, err)
 		}