Merge pull request #1580 from sarv-tech/fix-path-traversal

fix: Improve Path Boundary Validation in SafeTarExtractor

Author: steam-bell-92

security/tar_safe.py

@@ -134,17 +134,27 @@ def _validate_member(
         try:
             target_abs = target_path.resolve()
             extract_abs = extract_path.resolve()
-            if not str(target_abs).startswith(str(extract_abs)):
+            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, ValueError):
+        except (OSError, RuntimeError):
             target_str = str(target_path.absolute())
             extract_str = str(extract_path.absolute())
-            if not target_str.startswith(extract_str):
+            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: {member.name} -> {target_path}"
+                    f"Path traversal detected (different drives): {member.name} -> {target_path}"
                 )
+        except ValueError:
+            # os.path.commonpath raises ValueError on Windows for different drives
+            raise UnsafeTarError(
+                f"Path traversal detected (different drives): {member.name} -> {target_path}"
+            )
 
         if target_path.exists() and not overwrite:
             raise UnsafeTarError(

tests/test_security.py

@@ -143,6 +143,79 @@ def test_path_traversal_prevention(self, tmp_path):
 
         with pytest.raises(UnsafeTarError):
             extractor.extract(tar_file, extract_dir)
+            
+    def test_sibling_directory_traversal(self, tmp_path):
+        """Test sibling directory traversal (partial path traversal)."""
+        tar_file = tmp_path / "sibling_traversal.tar"
+        
+        with tarfile.open(tar_file, 'w') as tar:
+            # If extracting to 'extracted', a sibling directory could be 'extracted_evil'
+            # Using absolute paths in tar is rare but we can simulate by making the 
+            # tar path resolve to a sibling directory when combined with the extract dir.
+            # But SafeTarExtractor uses: extract_path / member.name
+            # So if member.name is '../extracted_evil/file.txt', and extract_path is '/tmp/extracted'
+            # target_path is '/tmp/extracted/../extracted_evil/file.txt' -> '/tmp/extracted_evil/file.txt'
+            # The vulnerability was that this bypassed string prefix checking.
+            info = tarfile.TarInfo(name='../extracted_evil/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 / "extracted"
+        # Create the sibling directory to simulate attack scenario
+        sibling_dir = tmp_path / "extracted_evil"
+        sibling_dir.mkdir(exist_ok=True)
+        
+        extractor = SafeTarExtractor()
+        
+        with pytest.raises(UnsafeTarError):
+            extractor.extract(tar_file, extract_dir)
+            
+    def test_absolute_path_traversal(self, tmp_path):
+        """Test absolute path traversal prevention."""
+        tar_file = tmp_path / "absolute_traversal.tar"
+        
+        with tarfile.open(tar_file, 'w') as tar:
+            # Create a TarInfo with an absolute path
+            # Need to use a generic absolute path for testing
+            info = tarfile.TarInfo(name='/tmp/evil_absolute.txt')
+            info.size = 10
+            info.type = tarfile.REGTYPE
+            
+            content = b'x' * 10
+            fileobj = io.BytesIO(content)
+            tar.addfile(info, fileobj)
+            
+        extract_dir = tmp_path / "extracted"
+        extractor = SafeTarExtractor(allow_absolute_paths=False)
+        
+        with pytest.raises(UnsafeTarError):
+            extractor.extract(tar_file, extract_dir)
+            
+    def test_nested_valid_directories(self, tmp_path):
+        """Test that nested valid directories are correctly extracted."""
+        tar_file = tmp_path / "nested_valid.tar"
+        
+        with tarfile.open(tar_file, 'w') as tar:
+            # Create nested structure
+            info = tarfile.TarInfo(name='dir1/dir2/file.txt')
+            info.size = 10
+            info.type = tarfile.REGTYPE
+            
+            content = b'x' * 10
+            fileobj = io.BytesIO(content)
+            tar.addfile(info, fileobj)
+            
+        extract_dir = tmp_path / "extracted"
+        extractor = SafeTarExtractor()
+        
+        extracted = extractor.extract(tar_file, extract_dir)
+        
+        assert len(extracted) == 1
+        assert (extract_dir / 'dir1' / 'dir2' / 'file.txt').exists()
 
     def test_size_limits(self, tmp_path):
         """Test file size limits."""