🔒 fix: Zip Path Traversal (ZipperDown) via Incomplete Sanitization#523
🔒 fix: Zip Path Traversal (ZipperDown) via Incomplete Sanitization#523
Conversation
…update module Implemented robust path validation in `SafeZipFile.java` and applied it consistently across `DownloadTask.java` and `UpdateContext.java`. This addresses a vulnerability where malicious update packages could perform arbitrary file writes via path traversal in zip entry names or metadata. - Added `SafeZipFile.validatePath` helper for canonical path verification. - Updated `unzipToPath` and `unzipToFile` to enforce validation. - Hardened `DownloadTask` patch operations and `UpdateContext.downloadFile`. - Fixed a flawed path sanitization logic that was vulnerable to double-slashing. Co-authored-by: sunnylqm <615282+sunnylqm@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. |
📝 WalkthroughWalkthroughThe changes introduce a centralized path traversal security validation method across the update system. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
android/src/main/java/cn/reactnative/modules/update/UpdateContext.java (1)
116-120: Exception handling is misleading but functionally correct.
validatePathdeclaresthrows IOExceptionbut actually throwsSecurityException(aRuntimeException) when a path traversal is detected. Thecatch (IOException)block only catches I/O errors fromgetCanonicalPath(), not the security violation itself.This works because
SecurityExceptionpropagates uncaught in both cases, and per the context inDownloadTask.doInBackground(lines 668-694), allThrowables are caught and routed toonDownloadFailed. However, the pattern is confusing.Consider simplifying to let the
SecurityExceptionpropagate directly:♻️ Suggested refactor
} else { params.targetFile = new File(rootDir, fileName); - try { - SafeZipFile.validatePath(params.targetFile, rootDir); - } catch (IOException e) { - throw new SecurityException("Illegal fileName: " + fileName); - } + try { + SafeZipFile.validatePath(params.targetFile, rootDir); + } catch (IOException e) { + throw new SecurityException("Illegal fileName: " + fileName, e); + } }At minimum, chain the original exception for better debugging. The current implementation swallows the root cause.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/src/main/java/cn/reactnative/modules/update/UpdateContext.java` around lines 116 - 120, The current try/catch around SafeZipFile.validatePath(params.targetFile, rootDir) in UpdateContext.java swallows the original IOException and throws a new SecurityException with no cause; either let SecurityException propagate by removing the catch block or, if you must convert, rethrow a SecurityException that chains the original IOException (e.g., include the caught exception as the cause) so callers of UpdateContext (and DownloadTask.doInBackground) can see the root cause; update the handling around SafeZipFile.validatePath and the thrown SecurityException to preserve the original exception information.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@android/src/main/java/cn/reactnative/modules/update/UpdateContext.java`:
- Around line 116-120: The current try/catch around
SafeZipFile.validatePath(params.targetFile, rootDir) in UpdateContext.java
swallows the original IOException and throws a new SecurityException with no
cause; either let SecurityException propagate by removing the catch block or, if
you must convert, rethrow a SecurityException that chains the original
IOException (e.g., include the caught exception as the cause) so callers of
UpdateContext (and DownloadTask.doInBackground) can see the root cause; update
the handling around SafeZipFile.validatePath and the thrown SecurityException to
preserve the original exception information.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5e70691a-01ee-4ae5-b2e1-7d486ecbb936
📒 Files selected for processing (3)
android/src/main/java/cn/reactnative/modules/update/DownloadTask.javaandroid/src/main/java/cn/reactnative/modules/update/SafeZipFile.javaandroid/src/main/java/cn/reactnative/modules/update/UpdateContext.java
1 participant
🎯 What: Zip Path Traversal (ZipperDown) fixed
Implemented a robust path validation mechanism in the Android native module.
Malicious update packages or untrusted inputs from the JavaScript side could exploit incomplete sanitization to perform arbitrary file writes outside the intended update directory. This could lead to code execution or sensitive data exposure on the device.
🛡️ Solution: How the fix addresses the vulnerability
SafeZipFile.java: Introduced avalidatePathstatic helper that ensures any target file resolved during extraction or copy strictly resides within its intended base directory, correctly handling canonical paths and trailing separators.DownloadTask.java: Consistently appliedvalidatePathto all file copy and extraction operations, replacing insecure manual checks.UpdateContext.java: Added validation for thefileNameparameter indownloadFileto prevent path traversal originating from JavaScript.PR created automatically by Jules for task 5541180321825854582 started by @sunnylqm
Summary by CodeRabbit
Release Notes