Path start improvements for path traversal detections

README.md

@@ -37,19 +37,16 @@ Prerequisites:
 #### For Red Hat-based Systems (RHEL, CentOS, Fedora)
 
 ```
-rpm -Uvh --oldpackage https://github.com/AikidoSec/firewall-php/releases/download/v1.0.106/aikido-php-firewall.x86_64.rpm
+rpm -Uvh --oldpackage https://github.com/AikidoSec/firewall-php/releases/download/v1.0.107/aikido-php-firewall.x86_64.rpm
 ```
 
 #### For Debian-based Systems (Debian, Ubuntu)
 
 ```
-curl -L -O https://github.com/AikidoSec/firewall-php/releases/download/v1.0.106/aikido-php-firewall.x86_64.deb
+curl -L -O https://github.com/AikidoSec/firewall-php/releases/download/v1.0.107/aikido-php-firewall.x86_64.deb
 dpkg -i -E ./aikido-php-firewall.x86_64.deb
 ```
 
-=======
-#### Configuration
-
 - [Caddy & PHP-FPM](./caddy.md)
 
 ### Managed platforms

lib/php-extension/include/php_aikido.h

@@ -3,7 +3,7 @@
 extern zend_module_entry aikido_module_entry;
 #define phpext_aikido_ptr &aikido_module_entry
 
-#define PHP_AIKIDO_VERSION "1.0.106"
+#define PHP_AIKIDO_VERSION "1.0.107"
 
 #if defined(ZTS) && defined(COMPILE_DL_AIKIDO)
 ZEND_TSRMLS_CACHE_EXTERN()

lib/request-processor/vulnerabilities/path-traversal/detectPathTraversal_test.go

@@ -5,46 +5,218 @@ import (
 )
 
 func TestDetectPathTraversal(t *testing.T) {
-	tests := []struct {
-		name       string
-		filePath   string
-		userInput  string
-		checkStart bool
-		expected   bool
-	}{
-		{"empty user input", "test.txt", "", true, false},
-		{"empty file input", "", "test", true, false},
-		{"empty user input and file input", "", "", true, false},
-		{"user input is a single character", "test.txt", "t", true, false},
-		{"file input is a single character", "t", "test", true, false},
-		{"same as user input", "text.txt", "text.txt", true, false},
-		{"with directory before", "directory/text.txt", "text.txt", true, false},
-		{"with both directory before", "directory/text.txt", "directory/text.txt", true, false},
-		{"user input and file input are single characters", "t", "t", true, false},
-		{"it flags ../", "../test.txt", "../", true, true},
-		{"it flags ..\\", "..\\test.txt", "..\\", true, true},
-		{"it flags ../../", "../../test.txt", "../../", true, true},
-		{"it flags ..\\..\\", "..\\..\\test.txt", "..\\..\\", true, true},
-		{"it flags ../../../../", "../../../../test.txt", "../../../../", true, true},
-		{"it flags ..\\..\\..\\", "..\\..\\..\\test.txt", "..\\..\\..\\", true, true},
-		{"user input is longer than file path", "../file.txt", "../../file.txt", true, false},
-		{"absolute linux path", "/etc/passwd", "/etc/passwd", true, true},
-		{"linux user directory", "/home/user/file.txt", "/home/user/", true, true},
-		{"windows drive letter", "C:\\file.txt", "C:\\", true, true},
-		{"no path traversal", "/appdata/storage/file.txt", "/storage/file.txt", true, false},
-		{"does not flag test", "/app/test.txt", "test", true, false},
-		{"does not flag example/test.txt", "/app/data/example/test.txt", "example/test.txt", true, false},
-		{"does not absolute path with different folder", "/etc/app/config", "/etc/hack/config", true, false},
-		{"does not absolute path inside another folder", "/etc/app/data/etc/config", "/etc/config", true, false},
-		{"disable checkPathStart", "/etc/passwd", "/etc/passwd", false, false},
-	}
-
-	for _, tc := range tests {
-		t.Run(tc.name, func(t *testing.T) {
-			result := detectPathTraversal(tc.filePath, tc.userInput, tc.checkStart)
-			if result != tc.expected {
-				t.Errorf("expected %v, got %v", tc.expected, result)
+	t.Run("empty user input", func(t *testing.T) {
+		if detectPathTraversal("test.txt", "", true) != false {
+			t.Error("expected false")
+		}
+	})
+
+	t.Run("empty file input", func(t *testing.T) {
+		if detectPathTraversal("", "test", true) != false {
+			t.Error("expected false")
+		}
+	})
+
+	t.Run("empty user input and file input", func(t *testing.T) {
+		if detectPathTraversal("", "", true) != false {
+			t.Error("expected false")
+		}
+	})
+
+	t.Run("user input is a single character", func(t *testing.T) {
+		if detectPathTraversal("test.txt", "t", true) != false {
+			t.Error("expected false")
+		}
+	})
+
+	t.Run("file input is a single character", func(t *testing.T) {
+		if detectPathTraversal("t", "test", true) != false {
+			t.Error("expected false")
+		}
+	})
+
+	t.Run("same as user input", func(t *testing.T) {
+		if detectPathTraversal("text.txt", "text.txt", true) != false {
+			t.Error("expected false")
+		}
+	})
+
+	t.Run("with directory before", func(t *testing.T) {
+		if detectPathTraversal("directory/text.txt", "text.txt", true) != false {
+			t.Error("expected false")
+		}
+	})
+
+	t.Run("with both directory before", func(t *testing.T) {
+		if detectPathTraversal("directory/text.txt", "directory/text.txt", true) != false {
+			t.Error("expected false")
+		}
+	})
+
+	t.Run("user input and file input are single characters", func(t *testing.T) {
+		if detectPathTraversal("t", "t", true) != false {
+			t.Error("expected false")
+		}
+	})
+
+	t.Run("it flags ../", func(t *testing.T) {
+		if detectPathTraversal("../test.txt", "../", true) != true {
+			t.Error("expected true")
+		}
+	})
+
+	t.Run("it flags ..\\", func(t *testing.T) {
+		if detectPathTraversal("..\\test.txt", "..\\", true) != true {
+			t.Error("expected true")
+		}
+	})
+
+	t.Run("it flags ../../", func(t *testing.T) {
+		if detectPathTraversal("../../test.txt", "../../", true) != true {
+			t.Error("expected true")
+		}
+	})
+
+	t.Run("it flags ..\\..\\", func(t *testing.T) {
+		if detectPathTraversal("..\\..\\test.txt", "..\\..\\", true) != true {
+			t.Error("expected true")
+		}
+	})
+
+	t.Run("it flags ../../../../", func(t *testing.T) {
+		if detectPathTraversal("../../../../test.txt", "../../../../", true) != true {
+			t.Error("expected true")
+		}
+	})
+
+	t.Run("it flags ..\\..\\..\\", func(t *testing.T) {
+		if detectPathTraversal("..\\..\\..\\test.txt", "..\\..\\..\\", true) != true {
+			t.Error("expected true")
+		}
+	})
+
+	t.Run("it flags ./../", func(t *testing.T) {
+		if detectPathTraversal("./../test.txt", "./../", true) != true {
+			t.Error("expected true")
+		}
+	})
+
+	t.Run("user input is longer than file path", func(t *testing.T) {
+		if detectPathTraversal("../file.txt", "../../file.txt", true) != false {
+			t.Error("expected false")
+		}
+	})
+
+	t.Run("absolute linux path", func(t *testing.T) {
+		if detectPathTraversal("/etc/passwd", "/etc/passwd", true) != true {
+			t.Error("expected true")
+		}
+	})
+
+	t.Run("linux user directory", func(t *testing.T) {
+		if detectPathTraversal("/home/user/file.txt", "/home/user/", true) != true {
+			t.Error("expected true")
+		}
+	})
+
+	t.Run("possible bypass", func(t *testing.T) {
+		if detectPathTraversal("/./etc/passwd", "/./etc/passwd", true) != true {
+			t.Error("expected true")
+		}
+	})
+
+	t.Run("another bypass", func(t *testing.T) {
+		if detectPathTraversal("/./././root/test.txt", "/./././root/test.txt", true) != true {
+			t.Error("expected true")
+		}
+		if detectPathTraversal("/./././root/test.txt", "/./././root", true) != true {
+			t.Error("expected true")
+		}
+	})
+
+	t.Run("no path traversal", func(t *testing.T) {
+		if detectPathTraversal("/appdata/storage/file.txt", "/storage/file.txt", true) != false {
+			t.Error("expected false")
+		}
+	})
+
+	t.Run("does not flag test", func(t *testing.T) {
+		if detectPathTraversal("/app/test.txt", "test", true) != false {
+			t.Error("expected false")
+		}
+	})
+
+	t.Run("does not flag example/test.txt", func(t *testing.T) {
+		if detectPathTraversal("/app/data/example/test.txt", "example/test.txt", true) != false {
+			t.Error("expected false")
+		}
+	})
+
+	t.Run("does not absolute path with different folder", func(t *testing.T) {
+		if detectPathTraversal("/etc/app/config", "/etc/hack/config", true) != false {
+			t.Error("expected false")
+		}
+	})
+
+	t.Run("does not absolute path inside another folder", func(t *testing.T) {
+		if detectPathTraversal("/etc/app/data/etc/config", "/etc/config", true) != false {
+			t.Error("expected false")
+		}
+	})
+
+	t.Run("disable checkPathStart", func(t *testing.T) {
+		if detectPathTraversal("/etc/passwd", "/etc/passwd", false) != false {
+			t.Error("expected false")
+		}
+	})
+
+	// To enable when doing a Windows build (filepath.IsAbs does not work on linux when a windows path is checked)
+	//t.Run("windows drive letter", func(t *testing.T) {
+	//	if detectPathTraversal("C:\\file.txt", "C:\\", true) != true {
+	//		t.Error("expected true")
+	//	}
+	//})
+
+	t.Run("does not detect if user input path contains no filename or subfolder", func(t *testing.T) {
+		testCases := []struct {
+			inputPath string
+			userPath  string
+			expected  bool
+		}{
+			{"/etc/app/test.txt", "/etc/", false},
+			{"/etc/app/", "/etc/", false},
+			{"/etc/app/", "/etc", false},
+			{"/etc/", "/etc/", false},
+			{"/etc", "/etc", false},
+			{"/var/a", "/var/", false},
+			{"/var/a", "/var/b", false},
+			{"/var/a", "/var/b/test.txt", false},
+		}
+		for _, tc := range testCases {
+			if detectPathTraversal(tc.inputPath, tc.userPath, true) != tc.expected {
+				t.Errorf("expected %v for input %q and user path %q", tc.expected, tc.inputPath, tc.userPath)
+			}
+		}
+	})
+
+	t.Run("it does detect if user input path contains a filename or subfolder", func(t *testing.T) {
+		testCases := []struct {
+			inputPath string
+			userPath  string
+			expected  bool
+		}{
+			{"/etc/app/file.txt", "/etc/app", true},
+			{"/etc/app/file.txt", "/etc/app/file.txt", true},
+			{"/var/backups/file.txt", "/var/backups", true},
+			{"/var/backups/file.txt", "/var/backups/file.txt", true},
+			{"/var/a", "/var/a", true},
+			{"/var/a/b", "/var/a", true},
+			{"/var/a/b/test.txt", "/var/a", true},
+		}
+		for _, tc := range testCases {
+			if detectPathTraversal(tc.inputPath, tc.userPath, true) != tc.expected {
+				t.Errorf("expected %v for input %q and user path %q", tc.expected, tc.inputPath, tc.userPath)
 			}
-		})
-	}
+		}
+	})
 }

lib/request-processor/vulnerabilities/path-traversal/unsafePathStart.go

@@ -1,6 +1,7 @@
 package path_traversal
 
 import (
+	"path/filepath"
 	"strings"
 )
 
@@ -26,14 +27,42 @@ var linuxRootFolders = []string{
 	"/var/",
 }
 
-var dangerousPathStarts = append(linuxRootFolders, "c:/", "c:\\")
+var dangerousPathStarts = linuxRootFolders
 
-func startsWithUnsafePath(filePath string, userInput string) bool {
-	lowerCasePath := strings.ToLower(filePath)
-	lowerCaseUserInput := strings.ToLower(userInput)
+// To enable when doing a Windows build (filepath.IsAbs does not work on Linux when a Windows path is checked)
+// var dangerousPathStarts = append(linuxRootFolders, "c:/", "c:\\")
+
+func normalizePath(p string) (string, error) {
+	p, err := filepath.Abs(p)
+	if err != nil {
+		return "", err
+	}
+	return strings.ToLower(p), nil
+}
+
+func startsWithUnsafePath(accessFilePath string, userInput string) bool {
+	if !filepath.IsAbs(accessFilePath) || !filepath.IsAbs(userInput) {
+		return false
+	}
+
+	var err error
+	normalizedAccessFilePath, err := normalizePath(accessFilePath)
+	if err != nil {
+		return false
+	}
+	normalizedUserInput, err := normalizePath(userInput)
+	if err != nil {
+		return false
+	}
 
 	for _, dangerousStart := range dangerousPathStarts {
-		if strings.HasPrefix(lowerCasePath, dangerousStart) && strings.HasPrefix(lowerCasePath, lowerCaseUserInput) {
+		if strings.HasPrefix(normalizedAccessFilePath, dangerousStart) && strings.HasPrefix(normalizedAccessFilePath, normalizedUserInput) {
+			if userInput == dangerousStart || userInput == dangerousStart[:len(dangerousStart)-1] {
+				// If the user input is the same as the dangerous start, we don't want to flag it to prevent false positives
+				// e.g. if user input is /etc/ and the path is /etc/passwd, we don't want to flag it, as long as the
+				// user input does not contain a subdirectory or filename
+				return false
+			}
 			return true
 		}
 	}

package/rpm/aikido.spec

@@ -1,5 +1,5 @@
 Name:           aikido-php-firewall
-Version:        1.0.106
+Version:        1.0.107
 Release:        1
 Summary:        Aikido PHP Extension
 License:        GPL