Merge pull request #165 from deploymenttheory/dev

Security fixes

Author: ShocOne

README.md

@@ -96,7 +96,7 @@ Example configuration file (clientconfig.json):
   "Environment": {
     "InstanceName": "yourinstance",
     "OverrideBaseDomain": "",
-    "APIType": "" // "jamfpro" / "graph"
+    "APIType": "" // "jamfpro" / "msgraph"
   },
   "ClientOptions": {
     "LogLevel": "LogLevelDebug",  // "LogLevelDebug" / "LogLevelInfo" / "LogLevelWarn" / "LogLevelError" / "LogLevelFatal" / "LogLevelPanic"

apiintegrations/jamfpro/jamfpro_api_request.go

@@ -7,9 +7,10 @@ import (
 	"encoding/xml"
 	"io"
 	"mime/multipart"
-	"os"
+	"path/filepath"
 	"strings"
 
+	"github.com/deploymenttheory/go-api-http-client/helpers"
 	"github.com/deploymenttheory/go-api-http-client/logger"
 	"go.uber.org/zap"
 )
@@ -55,7 +56,7 @@ func (j *JamfAPIHandler) MarshalRequest(body interface{}, method string, endpoin
 	return data, nil
 }
 
-// MarshalMultipartFormData takes a map with form fields and file paths and returns the encoded body and content type.
+// MarshalMultipartRequest handles multipart form data encoding with secure file handling and returns the encoded body and content type.
 func (j *JamfAPIHandler) MarshalMultipartRequest(fields map[string]string, files map[string]string, log logger.Logger) ([]byte, string, error) {
 	body := &bytes.Buffer{}
 	writer := multipart.NewWriter(body)
@@ -67,15 +68,16 @@ func (j *JamfAPIHandler) MarshalMultipartRequest(fields map[string]string, files
 		}
 	}
 
-	// Add the files to the form data
-	for formField, filepath := range files {
-		file, err := os.Open(filepath)
+	// Add the files to the form data, using safeOpenFile to ensure secure file access
+	for formField, filePath := range files {
+		file, err := helpers.SafeOpenFile(filePath)
 		if err != nil {
+			log.Error("Failed to open file securely", zap.String("file", filePath), zap.Error(err))
 			return nil, "", err
 		}
 		defer file.Close()
 
-		part, err := writer.CreateFormFile(formField, filepath)
+		part, err := writer.CreateFormFile(formField, filepath.Base(filePath))
 		if err != nil {
 			return nil, "", err
 		}
@@ -84,7 +86,7 @@ func (j *JamfAPIHandler) MarshalMultipartRequest(fields map[string]string, files
 		}
 	}
 
-	// Close the writer before returning
+	// Close the writer to finish writing the multipart message
 	contentType := writer.FormDataContentType()
 	if err := writer.Close(); err != nil {
 		return nil, "", err

apiintegrations/msgraph/msgraph_api_request.go

@@ -6,8 +6,9 @@ import (
 	"encoding/json"
 	"io"
 	"mime/multipart"
-	"os"
+	"path/filepath"
 
+	"github.com/deploymenttheory/go-api-http-client/helpers"
 	"github.com/deploymenttheory/go-api-http-client/logger"
 	"go.uber.org/zap"
 )
@@ -29,8 +30,9 @@ func (g *GraphAPIHandler) MarshalRequest(body interface{}, method string, endpoi
 	return data, nil
 }
 
-// MarshalMultipartFormData takes a map with form fields and file paths and returns the encoded body and content type.
+// MarshalMultipartRequest handles multipart form data encoding with secure file handling and returns the encoded body and content type.
 func (g *GraphAPIHandler) MarshalMultipartRequest(fields map[string]string, files map[string]string, log logger.Logger) ([]byte, string, error) {
+
 	body := &bytes.Buffer{}
 	writer := multipart.NewWriter(body)
 
@@ -41,15 +43,16 @@ func (g *GraphAPIHandler) MarshalMultipartRequest(fields map[string]string, file
 		}
 	}
 
-	// Add the files to the form data
-	for formField, filepath := range files {
-		file, err := os.Open(filepath)
+	// Add the files to the form data, using safeOpenFile to ensure secure file access
+	for formField, filePath := range files {
+		file, err := helpers.SafeOpenFile(filePath)
 		if err != nil {
+			log.Error("Failed to open file securely", zap.String("file", filePath), zap.Error(err))
 			return nil, "", err
 		}
 		defer file.Close()
 
-		part, err := writer.CreateFormFile(formField, filepath)
+		part, err := writer.CreateFormFile(formField, filepath.Base(filePath))
 		if err != nil {
 			return nil, "", err
 		}
@@ -58,7 +61,7 @@ func (g *GraphAPIHandler) MarshalMultipartRequest(fields map[string]string, file
 		}
 	}
 
-	// Close the writer before returning
+	// Close the writer to finish writing the multipart message
 	contentType := writer.FormDataContentType()
 	if err := writer.Close(); err != nil {
 		return nil, "", err

helpers/helpers.go

@@ -2,10 +2,34 @@
 package helpers
 
 import (
+	"fmt"
+	"os"
+	"path/filepath"
 	"time"
 )
 
 // ParseISO8601Date attempts to parse a string date in ISO 8601 format.
 func ParseISO8601Date(dateStr string) (time.Time, error) {
 	return time.Parse(time.RFC3339, dateStr)
 }
+
+// SafeOpenFile opens a file safely after validating and resolving its path.
+func SafeOpenFile(filePath string) (*os.File, error) {
+	// Clean the file path to remove any ".." or similar components that can lead to directory traversal
+	cleanPath := filepath.Clean(filePath)
+
+	// Resolve the clean path to an absolute path and ensure it resolves any symbolic links
+	absPath, err := filepath.EvalSymlinks(cleanPath)
+	if err != nil {
+		return nil, fmt.Errorf("unable to resolve the absolute path: %s, error: %w", filePath, err)
+	}
+
+	// Optionally, check if the absolute path is within a permitted directory (omitted here for brevity)
+	// Example: allowedPathPrefix := "/safe/directory/"
+	// if !strings.HasPrefix(absPath, allowedPathPrefix) {
+	// 	return nil, fmt.Errorf("access to the file path is not allowed: %s", absPath)
+	// }
+
+	// Open the file if the path is deemed safe
+	return os.Open(absPath)
+}

httpclient/client_configuration.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"log"
 	"os"
+	"path/filepath"
 	"strconv"
 	"strings"
 	"time"
@@ -24,16 +25,35 @@ const (
 	DefaultTimeout                   = 10 * time.Second
 	FollowRedirects                  = true
 	MaxRedirects                     = 10
+	ConfigFileExtension              = ".json"
 )
 
 // LoadConfigFromFile loads configuration values from a JSON file into the ClientConfig struct.
 // This function opens the specified configuration file, reads its content, and unmarshals the JSON data
 // into the ClientConfig struct. It's designed to initialize the client configuration with values
 // from a file, complementing or overriding defaults and environment variable settings.
-// LoadConfigFromFile loads configuration values from a JSON file into the ClientConfig struct.
 func LoadConfigFromFile(filePath string) (*ClientConfig, error) {
+	// Clean up the file path to prevent directory traversal
+	cleanPath := filepath.Clean(filePath)
+
+	// Resolve the cleanPath to an absolute path to ensure it resolves any symbolic links
+	absPath, err := filepath.EvalSymlinks(cleanPath)
+	if err != nil {
+		return nil, fmt.Errorf("unable to resolve the absolute path of the configuration file: %s, error: %w", filePath, err)
+	}
+
+	// Check for suspicious patterns in the resolved path
+	if strings.Contains(absPath, "..") {
+		return nil, fmt.Errorf("invalid path, path traversal patterns detected: %s", filePath)
+	}
+
+	// Ensure the file has the correct extension
+	if filepath.Ext(absPath) != ConfigFileExtension {
+		return nil, fmt.Errorf("invalid file extension for configuration file: %s, expected .json", filePath)
+	}
+
 	// Read the entire file
-	fileBytes, err := os.ReadFile(filePath)
+	fileBytes, err := os.ReadFile(absPath)
 	if err != nil {
 		return nil, fmt.Errorf("failed to read the configuration file: %s, error: %w", filePath, err)
 	}
@@ -48,11 +68,9 @@ func LoadConfigFromFile(filePath string) (*ClientConfig, error) {
 
 	log.Printf("Configuration successfully loaded from file: %s", filePath)
 
-	// Set default values if necessary
+	// Set default values if necessary and validate the configuration
 	setLoggerDefaultValues(&config)
 	setClientDefaultValues(&config)
-
-	// Validate mandatory configuration fields
 	if err := validateMandatoryConfiguration(&config); err != nil {
 		return nil, fmt.Errorf("configuration validation failed: %w", err)
 	}

logger/zaplogger_logpath.go

@@ -31,7 +31,7 @@ func EnsureLogFilePath(logPath string) (string, error) {
 
 	// Ensure the directory exists
 	dir := filepath.Dir(logPath)
-	if err := os.MkdirAll(dir, 0755); err != nil {
+	if err := os.MkdirAll(dir, 0750); err != nil {
 		return "", err
 	}