further security hardening

Author: tstromberg

cmd/agent/executeCommand.go

@@ -14,6 +14,10 @@ import (
 )
 
 // executeCommandWithPipes executes a command and captures stdout/stderr separately.
+// SECURITY: Commands come from checks.yaml which must be controlled by the system admin.
+// Bash restricted mode (-r) prevents: cd, PATH changes, output redirection, running programs
+// with / in name. Complex shell operations (pipes, grep, awk) are still allowed by design
+// as they're needed for compliance checks. The agent runs with user privileges only.
 func (*Agent) executeCommandWithPipes(ctx context.Context, checkName, command string) types.Check {
 	start := time.Now()
 	ctx, cancel := context.WithTimeout(ctx, commandTimeout)
@@ -24,6 +28,7 @@ func (*Agent) executeCommandWithPipes(ctx context.Context, checkName, command st
 	}
 
 	// Use bash -r for restricted mode security
+	// Security: This is as safe as we can make arbitrary command execution
 	cmd := exec.CommandContext(ctx, "bash", "-r", "-c", command)
 
 	// Capture stdout

cmd/server/main.go

@@ -3,6 +3,7 @@ package main
 
 import (
 	"context"
+	"crypto/subtle"
 	"embed"
 	"encoding/json"
 	"flag"
@@ -158,11 +159,12 @@ func main() {
 	mux.HandleFunc("/health", server.handleHealth)
 
 	srv := &http.Server{
-		Addr:         ":" + *port,
-		Handler:      loggingMiddleware(mux),
-		ReadTimeout:  readTimeout,
-		WriteTimeout: writeTimeout,
-		IdleTimeout:  idleTimeout,
+		Addr:           ":" + *port,
+		Handler:        loggingMiddleware(mux),
+		ReadTimeout:    readTimeout,
+		WriteTimeout:   writeTimeout,
+		IdleTimeout:    idleTimeout,
+		MaxHeaderBytes: 1 << 16, // 64KB max header size
 	}
 
 	go func() {
@@ -317,10 +319,8 @@ func (s *Server) handleReport(writer http.ResponseWriter, request *http.Request)
 	// Security: Check API key if configured
 	if *apiKey != "" {
 		providedKey := request.Header.Get("X-API-Key")
-		if providedKey == "" {
-			providedKey = request.URL.Query().Get("api_key")
-		}
-		if providedKey != *apiKey {
+		// Security: Don't accept API key from query params (exposes in logs)
+		if !constantTimeCompare(providedKey, *apiKey) {
 			s.incrementErrorCount()
 			log.Printf("[WARN] Unauthorized request from %s", request.RemoteAddr)
 			http.Error(writer, "Unauthorized", http.StatusUnauthorized)
@@ -343,10 +343,17 @@ func (s *Server) handleReport(writer http.ResponseWriter, request *http.Request)
 	// Security: Validate input fields
 	if report.HardwareID == "" || len(report.HardwareID) > maxFieldLength {
 		s.incrementErrorCount()
-		log.Printf("[WARN] Invalid Hardware ID from %s: %s", request.RemoteAddr, report.HardwareID)
+		log.Printf("[WARN] Invalid Hardware ID from %s: length %d", request.RemoteAddr, len(report.HardwareID))
 		http.Error(writer, "Invalid Hardware ID", http.StatusBadRequest)
 		return
 	}
+	// Additional validation: only allow safe characters in hardware ID
+	if !isValidHardwareID(report.HardwareID) {
+		s.incrementErrorCount()
+		log.Printf("[WARN] Invalid Hardware ID format from %s", request.RemoteAddr)
+		http.Error(writer, "Invalid Hardware ID format", http.StatusBadRequest)
+		return
+	}
 	if len(report.Hostname) > maxFieldLength {
 		s.incrementErrorCount()
 		log.Printf("[WARN] Hostname too long from %s: %d bytes", request.RemoteAddr, len(report.Hostname))
@@ -364,10 +371,17 @@ func (s *Server) handleReport(writer http.ResponseWriter, request *http.Request)
 	for name, check := range report.Checks {
 		if len(name) > maxCheckName {
 			s.incrementErrorCount()
-			log.Printf("[WARN] Check name too long from %s: %s (%d bytes)", request.RemoteAddr, name, len(name))
+			log.Printf("[WARN] Check name too long from %s: %d bytes", request.RemoteAddr, len(name))
 			http.Error(writer, "Check name too long", http.StatusBadRequest)
 			return
 		}
+		// Additional validation: only allow safe characters in check names
+		if !isValidCheckName(name) {
+			s.incrementErrorCount()
+			log.Printf("[WARN] Invalid check name format from %s", request.RemoteAddr)
+			http.Error(writer, "Invalid check name format", http.StatusBadRequest)
+			return
+		}
 		// Check combined output size (stdout + stderr)
 		totalOutput := len(check.Stdout) + len(check.Stderr)
 		if totalOutput > maxCheckOutput {
@@ -497,9 +511,11 @@ func (s *Server) handleHealth(writer http.ResponseWriter, _ *http.Request) {
 
 func loggingMiddleware(next http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		// Security: Add security headers
+		// Security: Add comprehensive security headers
 		w.Header().Set("X-Content-Type-Options", "nosniff")
 		w.Header().Set("X-Frame-Options", "DENY")
+		w.Header().Set("X-XSS-Protection", "1; mode=block")
+		w.Header().Set("Referrer-Policy", "strict-origin-when-cross-origin")
 		w.Header().Set("Content-Security-Policy", "default-src 'self'; script-src 'self'; style-src 'self' 'unsafe-inline'")
 
 		start := time.Now()
@@ -539,3 +555,36 @@ func (s *Server) setHealthy(healthy bool) {
 		log.Printf("[INFO] Server health status changed to healthy")
 	}
 }
+
+// constantTimeCompare performs constant-time string comparison to prevent timing attacks.
+func constantTimeCompare(a, b string) bool {
+	return len(a) == len(b) && subtle.ConstantTimeCompare([]byte(a), []byte(b)) == 1
+}
+
+// isValidHardwareID validates that a hardware ID contains only safe characters.
+func isValidHardwareID(id string) bool {
+	// Allow alphanumeric, hyphens, underscores, and dots (common in UUIDs and machine IDs)
+	for _, r := range id {
+		if !((r >= 'a' && r <= 'z') ||
+			(r >= 'A' && r <= 'Z') ||
+			(r >= '0' && r <= '9') ||
+			r == '-' || r == '_' || r == '.') {
+			return false
+		}
+	}
+	return len(id) > 0
+}
+
+// isValidCheckName validates that a check name contains only safe characters.
+func isValidCheckName(name string) bool {
+	// Allow alphanumeric, hyphens, and underscores only
+	for _, r := range name {
+		if !((r >= 'a' && r <= 'z') ||
+			(r >= 'A' && r <= 'Z') ||
+			(r >= '0' && r <= '9') ||
+			r == '-' || r == '_') {
+			return false
+		}
+	}
+	return len(name) > 0
+}

internal/gitstore/store.go

@@ -4,6 +4,7 @@ package gitstore
 import (
 	"context"
 	"crypto/sha256"
+	"encoding/hex"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -521,7 +522,7 @@ func isValidGitURL(url string) bool {
 
 func sanitizeID(id string) string {
 	// Security: Prevent path traversal and command injection
-	// Use allowlist approach - only allow safe characters
+	// Use strict allowlist approach - only allow safe characters
 	var sanitized strings.Builder
 	for _, r := range id {
 		switch {
@@ -531,7 +532,7 @@ func sanitizeID(id string) string {
 			sanitized.WriteRune(r)
 		case r >= '0' && r <= '9':
 			sanitized.WriteRune(r)
-		case r == '-' || r == '_' || r == '.':
+		case r == '-' || r == '_':
 			sanitized.WriteRune(r)
 		default:
 			// Replace any other character with dash
@@ -541,16 +542,22 @@ func sanitizeID(id string) string {
 
 	result := sanitized.String()
 
-	// Remove leading/trailing dots and dashes
-	result = strings.Trim(result, ".-")
+	// Remove leading/trailing dashes and underscores
+	result = strings.Trim(result, "-_")
+	
+	// Security: Prevent directory traversal attempts
+	result = strings.ReplaceAll(result, "..", "-")
+	result = strings.ReplaceAll(result, "//", "-")
 
 	// Ensure ID is not empty after sanitization
-	if result == "" {
-		result = "unknown"
+	if result == "" || result == "-" || result == "_" {
+		// Use hash of original ID for better uniqueness
+		hash := sha256.Sum256([]byte(id))
+		result = "id-" + hex.EncodeToString(hash[:8])
 	}
 
 	// Limit length to prevent filesystem issues
-	const maxIDLength = 255
+	const maxIDLength = 100
 	if len(result) > maxIDLength {
 		result = result[:maxIDLength]
 	}

internal/gitstore/store_test.go

@@ -25,10 +25,10 @@ func TestSanitizeID(t *testing.T) {
 		{"id:with:colons", "id-with-colons"},
 		{"id with spaces", "id-with-spaces"},
 		{"id<with>special*chars?", "id-with-special-chars"},
-		{"", "unknown"},
-		{"..", "unknown"},
-		{"./", "unknown"},
-		{strings.Repeat("a", 300), strings.Repeat("a", 255)},
+		{"", "id-e3b0c44298fc1c14"},              // hash of empty string
+		{"..", "id-5ec1f7e700f37c3d"},            // hash of ".."
+		{"./", "id-c14cecec97312ad1"},            // hash of "./"
+		{strings.Repeat("a", 300), strings.Repeat("a", 100)}, // max length is now 100
 	}
 
 	for _, tt := range tests {