mirror of
https://github.com/openclaw/openclaw.git
synced 2026-02-19 18:39:20 -05:00
fix(security): enforce symlink-safe skill packaging
This commit is contained in:
Peter Steinberger
@@ -16,6 +16,7 @@ Docs: https://docs.openclaw.ai
|
||||
### Fixes
|
||||
|
||||
- Gateway/WebChat: block `sessions.patch` and `sessions.delete` for WebChat clients so session-store mutations stay restricted to non-WebChat operator flows. Thanks @allsmog for reporting.
|
||||
- Security/Skills: for the next npm release, reject symlinks during skill packaging to prevent external file inclusion in distributed `.skill` archives. Thanks @aether-ai-agent for reporting.
|
||||
- Security/Feishu: prevent path traversal in Feishu inbound media temp-file writes by replacing key-derived temp filenames with UUID-based names. Thanks @allsmog for reporting.
|
||||
- LINE/Security: harden inbound media temp-file naming by using UUID-based temp paths for downloaded media instead of external message IDs. (#20792) Thanks @mbelinky.
|
||||
- Security/Media: harden local media ingestion against TOCTOU/symlink swap attacks by pinning reads to a single file descriptor with symlink rejection and inode/device verification in `saveMediaSource`. Thanks @dorjoos for reporting.
|
||||
|
||||
@@ -356,6 +356,8 @@ The packaging script will:
|
||||
|
||||
2. **Package** the skill if validation passes, creating a .skill file named after the skill (e.g., `my-skill.skill`) that includes all files and maintains the proper directory structure for distribution. The .skill file is a zip file with a .skill extension.
|
||||
|
||||
Security restriction: symlinks are rejected and packaging fails when any symlink is present.
|
||||
|
||||
If validation fails, the script will report the errors and exit without creating a package. Fix any validation errors and run the packaging command again.
|
||||
|
||||
### Step 6: Iterate
|
||||
|
||||
@@ -69,7 +69,7 @@ def package_skill(skill_path, output_dir=None):
|
||||
with zipfile.ZipFile(skill_filename, "w", zipfile.ZIP_DEFLATED) as zipf:
|
||||
# Walk through the skill directory
|
||||
for file_path in skill_path.rglob("*"):
|
||||
# Security Check 1: Reject symlinks to prevent supply chain attacks
|
||||
# Security: never follow or package symlinks.
|
||||
if file_path.is_symlink():
|
||||
print(f"[ERROR] Symlinks are not allowed in skills: {file_path}")
|
||||
print(" This is a security restriction to prevent including arbitrary files.")
|
||||
@@ -79,13 +79,6 @@ def package_skill(skill_path, output_dir=None):
|
||||
# Calculate the relative path within the zip
|
||||
arcname = file_path.relative_to(skill_path.parent)
|
||||
|
||||
# Security Check 2: Validate arcname to prevent Zip Slip attacks
|
||||
# Ensure the path doesn't escape the skill directory using ".." or absolute paths
|
||||
if ".." in arcname.parts or arcname.is_absolute():
|
||||
print(f"[ERROR] Invalid path in skill (possible Zip Slip attack): {arcname}")
|
||||
print(" Paths with '..' or absolute paths are not allowed.")
|
||||
return None
|
||||
|
||||
zipf.write(file_path, arcname)
|
||||
print(f" Added: {arcname}")
|
||||
|
||||
|
||||
@@ -1,285 +1,103 @@
|
||||
#!/usr/bin/env python3
|
||||
"""
|
||||
Test suite for package_skill.py security fixes.
|
||||
|
||||
Tests for:
|
||||
1. Symlink detection and rejection
|
||||
2. Path traversal (Zip Slip) prevention
|
||||
3. Normal file packaging functionality
|
||||
Regression tests for skill packaging security behavior.
|
||||
"""
|
||||
|
||||
import os
|
||||
import sys
|
||||
import tempfile
|
||||
import types
|
||||
import zipfile
|
||||
from pathlib import Path
|
||||
from unittest import TestCase, main
|
||||
|
||||
# Import the module being tested
|
||||
|
||||
fake_quick_validate = types.ModuleType("quick_validate")
|
||||
fake_quick_validate.validate_skill = lambda _path: (True, "Skill is valid!")
|
||||
sys.modules["quick_validate"] = fake_quick_validate
|
||||
|
||||
from package_skill import package_skill
|
||||
|
||||
|
||||
class TestPackageSkillSecurity(TestCase):
|
||||
"""Test security features of package_skill.py"""
|
||||
|
||||
def setUp(self):
|
||||
"""Create temporary directories for testing"""
|
||||
self.test_dir = tempfile.mkdtemp(prefix="test_skill_")
|
||||
self.temp_dir = Path(self.test_dir)
|
||||
self.temp_dir = Path(tempfile.mkdtemp(prefix="test_skill_"))
|
||||
|
||||
def tearDown(self):
|
||||
"""Clean up test directories"""
|
||||
import shutil
|
||||
|
||||
if self.temp_dir.exists():
|
||||
shutil.rmtree(self.temp_dir)
|
||||
|
||||
def create_test_skill(self, name="test-skill"):
|
||||
"""Create a minimal valid skill structure for testing"""
|
||||
def create_skill(self, name="test-skill"):
|
||||
skill_dir = self.temp_dir / name
|
||||
skill_dir.mkdir(parents=True, exist_ok=True)
|
||||
|
||||
# Create required SKILL.md with YAML frontmatter
|
||||
skill_md = skill_dir / "SKILL.md"
|
||||
skill_md.write_text(f"""---
|
||||
name: {name}
|
||||
description: A test skill for security testing
|
||||
---
|
||||
|
||||
# Test Skill
|
||||
|
||||
## Description
|
||||
A test skill for security testing.
|
||||
|
||||
## Usage
|
||||
Test usage example.
|
||||
""")
|
||||
|
||||
# Create a manifest file
|
||||
manifest = skill_dir / "manifest.json"
|
||||
manifest.write_text('{"name": "test-skill", "version": "1.0.0"}')
|
||||
|
||||
# Create a sample script
|
||||
script = skill_dir / "script.py"
|
||||
script.write_text('print("Hello from skill")')
|
||||
|
||||
(skill_dir / "SKILL.md").write_text("---\nname: test-skill\ndescription: test\n---\n")
|
||||
(skill_dir / "script.py").write_text("print('ok')\n")
|
||||
return skill_dir
|
||||
|
||||
def test_normal_file_packaging(self):
|
||||
"""Test that normal files are packaged correctly"""
|
||||
skill_dir = self.create_test_skill("normal-skill")
|
||||
output_dir = self.temp_dir / "output"
|
||||
output_dir.mkdir()
|
||||
def test_packages_normal_files(self):
|
||||
skill_dir = self.create_skill("normal-skill")
|
||||
out_dir = self.temp_dir / "out"
|
||||
out_dir.mkdir()
|
||||
|
||||
# Package the skill
|
||||
result = package_skill(str(skill_dir), str(output_dir))
|
||||
result = package_skill(str(skill_dir), str(out_dir))
|
||||
|
||||
# Verify packaging succeeded
|
||||
self.assertIsNotNone(result, "Packaging should succeed for normal skills")
|
||||
skill_file = output_dir / "normal-skill.skill"
|
||||
self.assertTrue(skill_file.exists(), "Skill file should be created")
|
||||
self.assertIsNotNone(result)
|
||||
skill_file = out_dir / "normal-skill.skill"
|
||||
self.assertTrue(skill_file.exists())
|
||||
with zipfile.ZipFile(skill_file, "r") as archive:
|
||||
names = set(archive.namelist())
|
||||
self.assertIn("normal-skill/SKILL.md", names)
|
||||
self.assertIn("normal-skill/script.py", names)
|
||||
|
||||
# Verify zip contents
|
||||
with zipfile.ZipFile(skill_file, "r") as zipf:
|
||||
names = zipf.namelist()
|
||||
self.assertIn("normal-skill/SKILL.md", names)
|
||||
self.assertIn("normal-skill/manifest.json", names)
|
||||
self.assertIn("normal-skill/script.py", names)
|
||||
|
||||
def test_symlink_rejection(self):
|
||||
"""Test that symlinks are detected and rejected"""
|
||||
skill_dir = self.create_test_skill("symlink-skill")
|
||||
|
||||
# Create a target file outside the skill directory
|
||||
external_file = self.temp_dir / "external_secret.txt"
|
||||
external_file.write_text("SECRET DATA - Should not be included")
|
||||
|
||||
# Create a symlink inside the skill directory pointing to external file
|
||||
symlink_path = skill_dir / "secrets"
|
||||
try:
|
||||
symlink_path.symlink_to(external_file)
|
||||
except (OSError, NotImplementedError):
|
||||
# Skip test if symlinks are not supported (e.g., Windows without admin)
|
||||
self.skipTest("Symlinks not supported on this system")
|
||||
|
||||
output_dir = self.temp_dir / "output"
|
||||
output_dir.mkdir()
|
||||
|
||||
# Attempt to package - should fail
|
||||
result = package_skill(str(skill_dir), str(output_dir))
|
||||
|
||||
self.assertIsNone(result, "Packaging should fail when symlinks are present")
|
||||
|
||||
def test_symlink_to_sensitive_file(self):
|
||||
"""Test rejection of symlink pointing to /etc/passwd-like sensitive files"""
|
||||
skill_dir = self.create_test_skill("sensitive-symlink-skill")
|
||||
|
||||
# Create a mock sensitive file
|
||||
sensitive_file = self.temp_dir / "mock_passwd"
|
||||
sensitive_file.write_text("root:x:0:0:root:/root:/bin/bash")
|
||||
|
||||
# Create a symlink inside skill directory
|
||||
symlink_path = skill_dir / "secrets" / "passwd"
|
||||
symlink_path.parent.mkdir(parents=True, exist_ok=True)
|
||||
def test_rejects_symlink_to_external_file(self):
|
||||
skill_dir = self.create_skill("symlink-file-skill")
|
||||
outside = self.temp_dir / "outside-secret.txt"
|
||||
outside.write_text("super-secret\n")
|
||||
link = skill_dir / "loot.txt"
|
||||
out_dir = self.temp_dir / "out"
|
||||
out_dir.mkdir()
|
||||
|
||||
try:
|
||||
symlink_path.symlink_to(sensitive_file)
|
||||
link.symlink_to(outside)
|
||||
except (OSError, NotImplementedError):
|
||||
self.skipTest("Symlinks not supported on this system")
|
||||
self.skipTest("symlink unsupported on this platform")
|
||||
|
||||
output_dir = self.temp_dir / "output"
|
||||
output_dir.mkdir()
|
||||
result = package_skill(str(skill_dir), str(out_dir))
|
||||
self.assertIsNone(result)
|
||||
|
||||
# Should reject due to symlink
|
||||
result = package_skill(str(skill_dir), str(output_dir))
|
||||
|
||||
self.assertIsNone(result, "Should reject symlinks to sensitive files")
|
||||
|
||||
def test_zip_slip_prevention(self):
|
||||
"""Test that Zip Slip attacks are prevented"""
|
||||
skill_dir = self.create_test_skill("zip-slip-skill")
|
||||
|
||||
# Since we can't easily create ".." in filenames through normal Python,
|
||||
# we test the validation logic by checking if the path validation
|
||||
# would catch such attempts.
|
||||
|
||||
# Create a subdirectory with test file
|
||||
subdir = skill_dir / "subdir"
|
||||
subdir.mkdir()
|
||||
test_file = subdir / "test.txt"
|
||||
test_file.write_text("test content")
|
||||
|
||||
output_dir = self.temp_dir / "output"
|
||||
output_dir.mkdir()
|
||||
|
||||
# Normal packaging should work fine
|
||||
result = package_skill(str(skill_dir), str(output_dir))
|
||||
|
||||
# This should succeed - valid structure
|
||||
self.assertIsNotNone(result, "Normal subdirectories should package successfully")
|
||||
|
||||
def test_absolute_path_prevention(self):
|
||||
"""Test that absolute paths in arcname are rejected"""
|
||||
# This is tested indirectly through the validation logic
|
||||
# The code checks: if ".." in arcname.parts or arcname.is_absolute()
|
||||
|
||||
skill_dir = self.create_test_skill("absolute-path-skill")
|
||||
test_file = skill_dir / "normal.txt"
|
||||
test_file.write_text("normal file")
|
||||
|
||||
output_dir = self.temp_dir / "output"
|
||||
output_dir.mkdir()
|
||||
|
||||
# Should succeed with normal files
|
||||
result = package_skill(str(skill_dir), str(output_dir))
|
||||
self.assertIsNotNone(result, "Normal files should package successfully")
|
||||
|
||||
def test_nested_files_allowed(self):
|
||||
"""Test that properly nested files are allowed"""
|
||||
skill_dir = self.create_test_skill("nested-skill")
|
||||
|
||||
# Create nested directories with files
|
||||
nested_dir = skill_dir / "lib" / "utils" / "helpers"
|
||||
nested_dir.mkdir(parents=True, exist_ok=True)
|
||||
|
||||
nested_file = nested_dir / "utility.py"
|
||||
nested_file.write_text("def helper(): pass")
|
||||
|
||||
output_dir = self.temp_dir / "output"
|
||||
output_dir.mkdir()
|
||||
|
||||
result = package_skill(str(skill_dir), str(output_dir))
|
||||
|
||||
self.assertIsNotNone(result, "Nested files should be allowed")
|
||||
self.assertTrue((output_dir / "nested-skill.skill").exists())
|
||||
|
||||
# Verify nested file is in zip
|
||||
with zipfile.ZipFile(output_dir / "nested-skill.skill", "r") as zipf:
|
||||
names = zipf.namelist()
|
||||
self.assertIn("nested-skill/lib/utils/helpers/utility.py", names)
|
||||
|
||||
def test_multiple_files_with_symlink_mixed(self):
|
||||
"""Test that one symlink among many files causes entire packaging to fail"""
|
||||
skill_dir = self.create_test_skill("mixed-skill")
|
||||
|
||||
# Add multiple normal files
|
||||
for i in range(5):
|
||||
file = skill_dir / f"file_{i}.txt"
|
||||
file.write_text(f"content {i}")
|
||||
|
||||
# Add one symlink
|
||||
external = self.temp_dir / "external.txt"
|
||||
external.write_text("external")
|
||||
def test_rejects_symlink_directory(self):
|
||||
skill_dir = self.create_skill("symlink-dir-skill")
|
||||
outside_dir = self.temp_dir / "outside"
|
||||
outside_dir.mkdir()
|
||||
(outside_dir / "secret.txt").write_text("secret\n")
|
||||
link = skill_dir / "docs"
|
||||
out_dir = self.temp_dir / "out"
|
||||
out_dir.mkdir()
|
||||
|
||||
try:
|
||||
(skill_dir / "symlinked").symlink_to(external)
|
||||
link.symlink_to(outside_dir, target_is_directory=True)
|
||||
except (OSError, NotImplementedError):
|
||||
self.skipTest("Symlinks not supported on this system")
|
||||
self.skipTest("symlink unsupported on this platform")
|
||||
|
||||
output_dir = self.temp_dir / "output"
|
||||
output_dir.mkdir()
|
||||
result = package_skill(str(skill_dir), str(out_dir))
|
||||
self.assertIsNone(result)
|
||||
|
||||
# Should fail due to symlink
|
||||
result = package_skill(str(skill_dir), str(output_dir))
|
||||
def test_allows_nested_regular_files(self):
|
||||
skill_dir = self.create_skill("nested-skill")
|
||||
nested = skill_dir / "lib" / "helpers"
|
||||
nested.mkdir(parents=True, exist_ok=True)
|
||||
(nested / "util.py").write_text("def run():\n return 1\n")
|
||||
out_dir = self.temp_dir / "out"
|
||||
out_dir.mkdir()
|
||||
|
||||
self.assertIsNone(result, "Packaging should fail if any symlink is present")
|
||||
result = package_skill(str(skill_dir), str(out_dir))
|
||||
|
||||
def test_large_skill_with_many_files(self):
|
||||
"""Test packaging a skill with many files (no symlinks or traversal)"""
|
||||
skill_dir = self.create_test_skill("large-skill")
|
||||
|
||||
# Create many files
|
||||
for i in range(100):
|
||||
subdir = skill_dir / f"dir_{i // 10}"
|
||||
subdir.mkdir(parents=True, exist_ok=True)
|
||||
file = subdir / f"file_{i}.txt"
|
||||
file.write_text(f"file {i}")
|
||||
|
||||
output_dir = self.temp_dir / "output"
|
||||
output_dir.mkdir()
|
||||
|
||||
result = package_skill(str(skill_dir), str(output_dir))
|
||||
|
||||
self.assertIsNotNone(result, "Large skill should package successfully")
|
||||
skill_file = output_dir / "large-skill.skill"
|
||||
|
||||
with zipfile.ZipFile(skill_file, "r") as zipf:
|
||||
# Should have SKILL.md + manifest.json + script.py + 100 files
|
||||
self.assertGreaterEqual(len(zipf.namelist()), 100)
|
||||
|
||||
|
||||
class TestPackageSkillValidation(TestCase):
|
||||
"""Test validation and error handling"""
|
||||
|
||||
def setUp(self):
|
||||
self.test_dir = tempfile.mkdtemp(prefix="test_validation_")
|
||||
self.temp_dir = Path(self.test_dir)
|
||||
|
||||
def tearDown(self):
|
||||
import shutil
|
||||
if self.temp_dir.exists():
|
||||
shutil.rmtree(self.temp_dir)
|
||||
|
||||
def test_missing_skill_directory(self):
|
||||
"""Test error handling for missing skill directory"""
|
||||
result = package_skill("/nonexistent/skill/path")
|
||||
self.assertIsNone(result, "Should fail for missing directory")
|
||||
|
||||
def test_file_instead_of_directory(self):
|
||||
"""Test error handling when path is a file, not directory"""
|
||||
file_path = self.temp_dir / "file.txt"
|
||||
file_path.write_text("not a directory")
|
||||
|
||||
result = package_skill(str(file_path))
|
||||
self.assertIsNone(result, "Should fail when path is a file")
|
||||
|
||||
def test_missing_skill_md(self):
|
||||
"""Test error handling for missing SKILL.md"""
|
||||
skill_dir = self.temp_dir / "invalid-skill"
|
||||
skill_dir.mkdir()
|
||||
(skill_dir / "file.txt").write_text("content")
|
||||
|
||||
result = package_skill(str(skill_dir))
|
||||
self.assertIsNone(result, "Should fail without SKILL.md")
|
||||
self.assertIsNotNone(result)
|
||||
skill_file = out_dir / "nested-skill.skill"
|
||||
with zipfile.ZipFile(skill_file, "r") as archive:
|
||||
names = set(archive.namelist())
|
||||
self.assertIn("nested-skill/lib/helpers/util.py", names)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
|
||||
Reference in New Issue
Block a user