🛡️ Sentinel: [CRITICAL] Fix Path Traversal in Plugin Manager#92
🛡️ Sentinel: [CRITICAL] Fix Path Traversal in Plugin Manager#92
Conversation
Add robust path sanitization using `filepath.Base(filepath.Clean("/"))`
when constructing paths for plugin installation and removal. This prevents
directory traversal vulnerabilities when processing untrusted metadata or
URLs containing sequences like `../`.
Co-authored-by: himattm <6266621+himattm@users.noreply.github.com>
|
👋 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. |
There was a problem hiding this comment.
Code Review
This pull request introduces a sanitizePluginName utility and integrates it into several plugin management functions to mitigate path traversal vulnerabilities. It also adds documentation explaining the vulnerability and the chosen prevention strategy. Review feedback highlights that the addBinaryPlugin function still lacks this sanitization, leaving a potential security gap, and suggests using os.PathSeparator in the sanitization logic to ensure cross-platform compatibility.
| ) | ||
|
|
||
| // sanitizePluginName removes path separators and prevents directory traversal | ||
| func sanitizePluginName(name string) string { |
There was a problem hiding this comment.
While the sanitizePluginName function is a robust addition, it is currently not utilized in the addBinaryPlugin function (line 377). When a plugin is installed from a GitHub repository, the pluginName derived from the repository name is passed directly to filepath.Join without sanitization. A malicious repository name such as prism-plugin-../../etc/passwd would still trigger a path traversal vulnerability in that specific flow. Please ensure this sanitization is applied to addBinaryPlugin as well to fully address the vulnerability.
| // sanitizePluginName removes path separators and prevents directory traversal | ||
| func sanitizePluginName(name string) string { | ||
| name = filepath.Base(filepath.Clean("/" + name)) | ||
| if name == "/" || name == "." || name == ".." { |
There was a problem hiding this comment.
The check name == "/" is not cross-platform. On Windows, filepath.Base of a root path (like the one produced by filepath.Clean("/" + name)) returns the backslash separator \. Using os.PathSeparator ensures the check works correctly on all operating systems.
| if name == "/" || name == "." || name == ".." { | |
| if name == string(os.PathSeparator) || name == "." || name == ".." { |
🚨 Severity: CRITICAL
💡 Vulnerability: A path traversal vulnerability existed in
internal/plugin/manager.go. When adding a plugin from a direct URL (addFromDirectURL) or removing a plugin (Remove), an untrusted plugin name could be interpolated directly into a file path viafilepath.Join. Sincefilepath.Joinevaluates../sequences, a malicious name (e.g.,../../../evil) could write to or delete files outside the intended plugin directory.🎯 Impact: An attacker could potentially achieve remote code execution by overwriting critical scripts or delete sensitive system files by convincing a user to install a plugin from a malicious URL or run a command like
prism plugin remove ../../../etc/passwd.🔧 Fix: Added a
sanitizePluginNamefunction that usesfilepath.Base(filepath.Clean("/" + name))to extract the safe base name and drop any relative directory escapes. Applied this sanitization toParseMetadata(which reads from downloaded scripts),addFromDirectURL, and theRemovehandler.✅ Verification: Successfully verified that path segments like
../../../etc/passwdcorrectly resolve to the safe base namepasswdusing Go tests and local manual script runs. Confirmed that full test suite passes locally.PR created automatically by Jules for task 1392184718881425820 started by @himattm