Merge pull request #99 from marklogic/MLE20489/BUG-CWE-fix

MLE-20489/BUG-CWE-fix

Author: rwinieski

pkg/k8sutil/common.go

@@ -1,8 +1,8 @@
 package k8sutil
 
 import (
-	"math/rand"
-	"time"
+	"crypto/rand"
+	"math/big"
 
 	marklogicv1 "github.com/marklogic/marklogic-operator-kubernetes/api/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -108,10 +108,15 @@ func generateRandomAlphaNumeric(length int) string {
 	const charset = "abcdefghijklmnopqrstuvwxyz" +
 		"ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"
 
-	seededRand := rand.New(rand.NewSource(time.Now().UnixNano()))
 	result := make([]byte, length)
 	for i := range result {
-		result[i] = charset[seededRand.Intn(len(charset))]
+		num, err := rand.Int(rand.Reader, big.NewInt(int64(len(charset))))
+		if err != nil {
+			// Fallback to a deterministic character if crypto/rand fails
+			result[i] = charset[i%len(charset)]
+		} else {
+			result[i] = charset[num.Int64()]
+		}
 	}
 	return string(result)
 }

pkg/k8sutil/handler.go

@@ -10,7 +10,10 @@ func (oc *OperatorContext) ReconsileMarklogicGroupHandler() (reconcile.Result, e
 	if result := oc.ReconcileServices(); result.Completed() {
 		return result.Output()
 	}
-	setOperatorInternalStatus(oc, "Created")
+	err := setOperatorInternalStatus(oc, "Created")
+	if err != nil {
+		oc.ReqLogger.Error(err, "Failed to set operator internal status")
+	}
 
 	if result := oc.ReconcileConfigMap(); result.Completed() {
 		return result.Output()

pkg/k8sutil/statefulset.go

@@ -80,11 +80,18 @@ func (oc *OperatorContext) ReconcileStatefulset() (reconcile.Result, error) {
 	statefulSetDef := generateStatefulSetsDef(objectMeta, statefulSetParams, marklogicServerAsOwner(cr), containerParams)
 	if err != nil {
 		if apierrors.IsNotFound(err) {
-			oc.createStatefulSet(statefulSetDef, cr)
+			err := oc.createStatefulSet(statefulSetDef, cr)
+			if err != nil {
+				logger.Error(err, "Failed to create statefulSet")
+				return result.Error(err).Output()
+			}
 			oc.Recorder.Event(oc.MarklogicGroup, "Normal", "StatefulSetCreated", "MarkLogic statefulSet created successfully")
 			return result.Done().Output()
 		}
-		result.Error(err).Output()
+		_, outputErr := result.Error(err).Output()
+		if outputErr != nil {
+			logger.Error(outputErr, "Failed to process result error")
+		}
 	}
 	if err != nil {
 		logger.Error(err, "Cannot create standalone statefulSet for MarkLogic")
@@ -490,7 +497,7 @@ func generateVolumes(stsName string, containerParams containerParameters) []core
 				},
 			})
 		}
-		if containerParams.Tls.CertSecretNames != nil && len(containerParams.Tls.CertSecretNames) > 0 {
+		if len(containerParams.Tls.CertSecretNames) > 0 {
 			projectionSources := []corev1.VolumeProjection{}
 			for i, secretName := range containerParams.Tls.CertSecretNames {
 				projectionSource := corev1.VolumeProjection{

test/utils/certs.go

@@ -3,37 +3,83 @@ package utils
 import (
 	"fmt"
 	"os/exec"
+	"path/filepath"
 	"strings"
 )
 
+// validatePath performs basic validation on file paths to prevent command injection
+func validatePath(path string) error {
+	// Use filepath.Clean to sanitize the path
+	cleanPath := filepath.Clean(path)
+
+	// Ensure the path doesn't contain shell metacharacters
+	if strings.ContainsAny(cleanPath, ";|&$`(){}[]<>") {
+		return fmt.Errorf("path contains invalid characters: %s", path)
+	}
+
+	// Ensure it's not an absolute path to prevent access to system directories
+	if filepath.IsAbs(cleanPath) && !strings.HasPrefix(cleanPath, "/tmp/") {
+		return fmt.Errorf("path must be relative or in /tmp: %s", path)
+	}
+
+	return nil
+}
+
 func GenerateCertificates(path string, caPath string) error {
 	var err error
 	fmt.Println("====Generating TLS Certificates")
-	geeTlsKeyCmd := strings.Replace("openssl genpkey -algorithm RSA -out path/tls.key", "path", path, -1)
-	genCsrCmd := strings.Replace("openssl req -new -key path/tls.key -config path/server.cnf -out path/tls.csr", "path", path, -1)
-	genCrtCmd := strings.Replace(strings.Replace("openssl x509 -req -CA caPath/cacert.pem -CAkey caPath/ca-private-key.pem -CAcreateserial -CAserial path/cacert.srl -in path/tls.csr -out path/tls.crt -days 365", "path", path, -1), "caPath", caPath, -1)
-	rvariable := []string{geeTlsKeyCmd, genCsrCmd, genCrtCmd}
-	for _, j := range rvariable {
-		cmd := exec.Command("bash", "-c", j)
+
+	// Validate paths to prevent command injection
+	if err := validatePath(path); err != nil {
+		return fmt.Errorf("invalid path: %w", err)
+	}
+	if err := validatePath(caPath); err != nil {
+		return fmt.Errorf("invalid caPath: %w", err)
+	}
+
+	// Use safer approach with individual commands instead of shell execution
+	commands := [][]string{
+		{"openssl", "genpkey", "-algorithm", "RSA", "-out", filepath.Join(path, "tls.key")},
+		{"openssl", "req", "-new", "-key", filepath.Join(path, "tls.key"), "-config", filepath.Join(path, "server.cnf"), "-out", filepath.Join(path, "tls.csr")},
+		{"openssl", "x509", "-req", "-CA", filepath.Join(caPath, "cacert.pem"), "-CAkey", filepath.Join(caPath, "ca-private-key.pem"), "-CAcreateserial", "-CAserial", filepath.Join(path, "cacert.srl"), "-in", filepath.Join(path, "tls.csr"), "-out", filepath.Join(path, "tls.crt"), "-days", "365"},
+	}
+
+	for _, cmdArgs := range commands {
+		// #nosec G204 - Command arguments are constructed from validated paths
+		cmd := exec.Command(cmdArgs[0], cmdArgs[1:]...)
 		err = cmd.Run()
+		if err != nil {
+			return fmt.Errorf("failed to execute command %v: %w", cmdArgs, err)
+		}
 	}
 	return err
 }
 
 func GenerateCACertificate(caPath string) error {
 	var err error
 	fmt.Println("====Generating CA Certificates")
-	genKeyCmd := strings.Replace("openssl genrsa -out caPath/ca-private-key.pem 2048", "caPath", caPath, -1)
-	genCACertCmd := strings.Replace("openssl req -new -x509 -days 3650 -key caPath/ca-private-key.pem -out caPath/cacert.pem -subj '/CN=TlsTest/C=US/ST=California/L=RedwoodCity/O=Progress/OU=MarkLogic'", "caPath", caPath, -1)
-	pwdCMD := "pwd"
-	rvariable := []string{pwdCMD, genKeyCmd, genCACertCmd}
-	for _, j := range rvariable {
-		fmt.Println(j)
-		cmd := exec.Command("bash", "-c", j)
+
+	// Validate path to prevent command injection
+	if err := validatePath(caPath); err != nil {
+		return fmt.Errorf("invalid caPath: %w", err)
+	}
+
+	// Use safer approach with individual commands instead of shell execution
+	commands := [][]string{
+		{"pwd"},
+		{"openssl", "genrsa", "-out", filepath.Join(caPath, "ca-private-key.pem"), "2048"},
+		{"openssl", "req", "-new", "-x509", "-days", "3650", "-key", filepath.Join(caPath, "ca-private-key.pem"), "-out", filepath.Join(caPath, "cacert.pem"), "-subj", "/CN=TlsTest/C=US/ST=California/L=RedwoodCity/O=Progress/OU=MarkLogic"},
+	}
+
+	for _, cmdArgs := range commands {
+		fmt.Println(strings.Join(cmdArgs, " "))
+		// #nosec G204 - Command arguments are constructed from validated paths
+		cmd := exec.Command(cmdArgs[0], cmdArgs[1:]...)
 		err = cmd.Run()
 		if err != nil {
 			fmt.Println(err)
+			return fmt.Errorf("failed to execute command %v: %w", cmdArgs, err)
 		}
 	}
-	return err
+	return nil
 }

test/utils/utils.go

@@ -23,6 +23,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"regexp"
 	"strings"
 	"testing"
 	"time"
@@ -48,9 +49,43 @@ func warnError(err error) {
 	fmt.Fprintf(GinkgoWriter, "warning: %v\n", err)
 }
 
+// validateURL performs basic validation on URLs to prevent command injection
+func validateURL(url string) error {
+	if !strings.HasPrefix(url, "https://") {
+		return fmt.Errorf("URL must use HTTPS: %s", url)
+	}
+	// Basic validation to ensure URL doesn't contain shell metacharacters
+	if strings.ContainsAny(url, ";|&$`(){}[]<>") {
+		return fmt.Errorf("URL contains invalid characters: %s", url)
+	}
+	return nil
+}
+
+// validateKindClusterName validates cluster names for kind
+func validateKindClusterName(name string) error {
+	// Kind cluster names should only contain alphanumeric characters and hyphens
+	if !regexp.MustCompile(`^[a-zA-Z0-9-]+$`).MatchString(name) {
+		return fmt.Errorf("invalid cluster name: %s", name)
+	}
+	return nil
+}
+
+// validateImageName validates Docker image names
+func validateImageName(name string) error {
+	// Basic validation for Docker image names
+	if strings.ContainsAny(name, ";|&$`(){}[]<>") {
+		return fmt.Errorf("image name contains invalid characters: %s", name)
+	}
+	return nil
+}
+
 // InstallPrometheusOperator installs the prometheus Operator to be used to export the enabled metrics.
 func InstallPrometheusOperator() error {
 	url := fmt.Sprintf(prometheusOperatorURL, prometheusOperatorVersion)
+	if err := validateURL(url); err != nil {
+		return fmt.Errorf("invalid prometheus operator URL: %w", err)
+	}
+	// #nosec G204 - URL is validated and constructed from trusted constants
 	cmd := exec.Command("kubectl", "create", "-f", url)
 	_, err := Run(cmd)
 	return err
@@ -79,6 +114,11 @@ func Run(cmd *exec.Cmd) ([]byte, error) {
 // UninstallPrometheusOperator uninstalls the prometheus
 func UninstallPrometheusOperator() {
 	url := fmt.Sprintf(prometheusOperatorURL, prometheusOperatorVersion)
+	if err := validateURL(url); err != nil {
+		warnError(fmt.Errorf("invalid prometheus operator URL: %w", err))
+		return
+	}
+	// #nosec G204 - URL is validated and constructed from trusted constants
 	cmd := exec.Command("kubectl", "delete", "-f", url)
 	if _, err := Run(cmd); err != nil {
 		warnError(err)
@@ -88,6 +128,11 @@ func UninstallPrometheusOperator() {
 // UninstallCertManager uninstalls the cert manager
 func UninstallCertManager() {
 	url := fmt.Sprintf(certmanagerURLTmpl, certmanagerVersion)
+	if err := validateURL(url); err != nil {
+		warnError(fmt.Errorf("invalid cert manager URL: %w", err))
+		return
+	}
+	// #nosec G204 - URL is validated and constructed from trusted constants
 	cmd := exec.Command("kubectl", "delete", "-f", url)
 	if _, err := Run(cmd); err != nil {
 		warnError(err)
@@ -97,6 +142,10 @@ func UninstallCertManager() {
 // InstallCertManager installs the cert manager bundle.
 func InstallCertManager() error {
 	url := fmt.Sprintf(certmanagerURLTmpl, certmanagerVersion)
+	if err := validateURL(url); err != nil {
+		return fmt.Errorf("invalid cert manager URL: %w", err)
+	}
+	// #nosec G204 - URL is validated and constructed from trusted constants
 	cmd := exec.Command("kubectl", "apply", "-f", url)
 	if _, err := Run(cmd); err != nil {
 		return err
@@ -119,6 +168,16 @@ func LoadImageToKindClusterWithName(name string) error {
 	if v, ok := os.LookupEnv("KIND_CLUSTER"); ok {
 		cluster = v
 	}
+
+	// Validate inputs to prevent command injection
+	if err := validateImageName(name); err != nil {
+		return fmt.Errorf("invalid image name: %w", err)
+	}
+	if err := validateKindClusterName(cluster); err != nil {
+		return fmt.Errorf("invalid cluster name: %w", err)
+	}
+
+	// #nosec G204 - Inputs are validated for safety
 	kindOptions := []string{"load", "docker-image", name, "--name", cluster}
 	cmd := exec.Command("kind", kindOptions...)
 	_, err := Run(cmd)