Merge pull request #1662 from Kirtan-pc/fix/path-containment

Fix: SafeTarExtractor path containment check can be bypassed due to naive string prefix matching

Author: steam-bell-92

security/tar_safe.py

@@ -130,30 +130,24 @@ def _validate_member(
 
         target_path = self._get_safe_path(member, extract_path)
 
-        # Check for path traversal
+        # Check for path traversal using Path.relative_to()
         try:
             target_abs = target_path.resolve()
             extract_abs = extract_path.resolve()
-            if os.path.commonpath([str(target_abs), str(extract_abs)]) != str(extract_abs):
-                raise UnsafeTarError(
-                    f"Path traversal detected: {member.name} -> {target_path}"
-                )
-        except (OSError, RuntimeError):
-            target_str = str(target_path.absolute())
-            extract_str = str(extract_path.absolute())
-            try:
-                if os.path.commonpath([target_str, extract_str]) != extract_str:
-                    raise UnsafeTarError(
-                        f"Path traversal detected: {member.name} -> {target_path}"
-                    )
-            except ValueError:
-                raise UnsafeTarError(
-                    f"Path traversal detected (different drives): {member.name} -> {target_path}"
-                )
-        except ValueError:
-            # os.path.commonpath raises ValueError on Windows for different drives
+            target_abs.relative_to(extract_abs)
+        except (ValueError, OSError, RuntimeError):
             raise UnsafeTarError(
-                f"Path traversal detected (different drives): {member.name} -> {target_path}"
+                f"Path traversal detected: {member.name} -> {target_path}"
+            )
+        
+        if target_path.exists() and not overwrite:
+            raise UnsafeTarError(
+                f"File already exists and overwrite=False: {target_path}"
+            )
+        
+        if member.type in self.blocked_types:
+            raise UnsafeTarError(
+                f"Blocked file type for {member.name}: {member.type}"
             )
 
         if target_path.exists() and not overwrite:

tests/test_security.py

@@ -220,7 +220,27 @@ def test_sibling_directory_traversal(self, tmp_path):
         with pytest.raises(UnsafeTarError):
             extractor.extract(tar_file, extract_dir)
 
-    def test_absolute_path_traversal(self, tmp_path):
+    def test_prefix_bypass_path_traversal(self, tmp_path):
+        """Test that prefix-based bypass (e.g., /tmp/a vs /tmp/ab) is caught."""
+        tar_file = tmp_path / "prefix_bypass.tar"
+        
+        with tarfile.open(tar_file, 'w') as tar:
+            info = tarfile.TarInfo(name='../ab/evil.txt')
+            info.size = 10
+            info.type = tarfile.REGTYPE
+            content = b'x' * 10
+            fileobj = io.BytesIO(content)
+            tar.addfile(info, fileobj)
+        
+        extract_dir = tmp_path / "a"
+        extract_dir.mkdir(exist_ok=True)
+        sibling_dir = tmp_path / "ab"
+        sibling_dir.mkdir(exist_ok=True)
+        
+        extractor = SafeTarExtractor()
+        
+        with pytest.raises(UnsafeTarError):
+            extractor.extract(tar_file, extract_dir)
         """Test absolute path traversal prevention."""
         tar_file = tmp_path / "absolute_traversal.tar"