🛡️ Sentinel: [CRITICAL] Fix path traversal bypass in Lua sandbox#166
🛡️ Sentinel: [CRITICAL] Fix path traversal bypass in Lua sandbox#166mleem97 wants to merge 1 commit into
Conversation
Ensure base directory checking ends with a directory separator to prevent prefix-matching bypasses during file access validation in Lua I/O modules.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a critical path traversal vulnerability in the Lua sandbox where prefix-matching allowed access to sibling directories. While the logical change to use trailing separators for prefix matching is sound, the implementation currently relies on case-insensitive comparisons which may be bypassed on case-sensitive filesystems (e.g., Linux).
Crucially, no unit tests were provided to verify the fix or ensure against regressions for this security-critical component. The lack of automated validation is a significant gap, especially given the 'MissingRequirements' status in the coverage analysis for this sensitive area of the codebase.
About this PR
- There is a significant concern regarding the lack of automated regression tests for this critical security fix. While the PR description mentions that unit tests were run, none were included in the codebase. Given the sensitivity of the Lua sandbox, these tests are necessary to prevent future regressions.
Test suggestions
- Access a sibling directory sharing a prefix (e.g., base is '/mods/modA', target is '/mods/modA_secret')
- Access the exact base directory path without a trailing slash
- Access a valid file within the sandbox (e.g., '/mods/modA/config.json')
- Attempt to escape using '..' to reach parent directories
- Absolute path injection attempts
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Access a sibling directory sharing a prefix (e.g., base is '/mods/modA', target is '/mods/modA_secret')
2. Access the exact base directory path without a trailing slash
3. Access a valid file within the sandbox (e.g., '/mods/modA/config.json')
4. Attempt to escape using '..' to reach parent directories
5. Absolute path injection attempts
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| bool isAllowed = fullPath.Equals(dataDirFull, StringComparison.OrdinalIgnoreCase) || | ||
| fullPath.StartsWith(dataDirWithSep, StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
🔴 HIGH RISK
Suggestion: While the fix addresses the sibling directory prefix bypass, it lacks automated validation and introduces potential cross-platform vulnerabilities:
- Case Sensitivity: Using
OrdinalIgnoreCasefor prefix validation can lead to sandbox escapes on case-sensitive filesystems (like Linux) by matching sibling paths that differ only by case (e.g.,/Data/Modvs/data/Mod). The comparison should be adjusted based on the host OS (e.g., checkingRuntimeInformation.IsOSPlatform). - Missing Tests: No unit tests were included to verify the 'ResolveSafe' logic against prefix bypasses, exact matches, or traversal attempts.
Recommended Action: Update the logic to be OS-aware and add a test suite covering the scenarios outlined in the test plan.
🚨 Severity: CRITICAL
💡 Vulnerability: The Lua sandbox
ResolveSafemethod had a prefix-matching path traversal bypass where it did not ensure the base directory ended with a directory separator before checkingStartsWith().🎯 Impact: This allowed mod developers to read/write/delete files in sibling directories that shared the same prefix (e.g., escaping
/mods/modA/datato access/mods/modA/data_secret).🔧 Fix: Updated the
StartsWithcheck to use a directory-separator-terminated string, while also explicitly permitting exact matches.✅ Verification: Ran unit tests and verified code correctly prevents prefix-based path traversal bypasses.
PR created automatically by Jules for task 13384660083170009826 started by @mleem97