fix: Fix path traversal vulnerability in zip extraction (bug #232873)#332
Merged
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom Dec 9, 2025
Merged
fix: Fix path traversal vulnerability in zip extraction (bug #232873)#332deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
Conversation
- Replace single-pass "../" removal with loop to remove all occurrences - Add final path validation to ensure extracted files stay within target directory Log: fix CITIVD Bug: https://pms.uniontech.com/bug-view-342883.html
deepin pr auto review我来对这段代码修改进行审查：
总体来说，这个修改显著提高了代码的安全性，有效防止了路径遍历攻击。代码质量也有所提升，注释更加清晰。虽然可能会带来轻微的性能开销，但考虑到安全性的重要性，这是值得的。建议在后续版本中考虑上述改进建议，进一步完善代码。 |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideStrengthens zip extraction security by robustly stripping "../" path traversal components from entry names and enforcing a final filesystem-level containment check to ensure extracted files stay within the configured target directory. Sequence diagram for secure zip entry extraction with path traversal protectionsequenceDiagram
participant Caller
participant LibzipPlugin
participant QDir
participant QFile
Caller->>LibzipPlugin: extractEntry(archive, index, options)
LibzipPlugin->>LibzipPlugin: trans2uft8(statBuffer.name, m_mapFileCode[index])
LibzipPlugin->>LibzipPlugin: strFileName = result
loop Remove all ../ components
LibzipPlugin->>LibzipPlugin: while strFileName.contains(../)
LibzipPlugin->>LibzipPlugin: strFileName.replace(../, empty)
end
LibzipPlugin->>LibzipPlugin: strDestFileName = options.strTargetPath + separator + strFileName
LibzipPlugin->>QDir: QDir(options.strTargetPath).absolutePath()
QDir-->>LibzipPlugin: targetAbsPath
LibzipPlugin->>LibzipPlugin: cleanTargetPath = QDir::cleanPath(targetAbsPath)
LibzipPlugin->>QDir: QDir(strDestFileName).absolutePath()
QDir-->>LibzipPlugin: destAbsPath
LibzipPlugin->>LibzipPlugin: cleanDestPath = QDir::cleanPath(destAbsPath)
alt cleanDestPath outside cleanTargetPath
LibzipPlugin-->>Caller: return ET_FileWriteError
else cleanDestPath inside cleanTargetPath
LibzipPlugin->>QFile: QFile(strDestFileName)
QFile-->>LibzipPlugin: file handle
LibzipPlugin-->>Caller: return extraction result
end
Class diagram for LibzipPlugin extractEntry security enhancementsclassDiagram
class LibzipPlugin {
- CommonType m_common
- MapType m_mapFileCode
+ ErrorType extractEntry(zip_t* archive, zip_int64_t index, const ExtractOptions& options)
}
class QDir {
+ QDir(QString path)
+ QString absolutePath()
+ static QString cleanPath(QString path)
+ static QString separator()
}
class QFile {
+ QFile(QString fileName)
+ bool open()
+ void close()
}
class ExtractOptions {
+ QString strTargetPath
}
LibzipPlugin --> QDir : uses
LibzipPlugin --> QFile : uses
LibzipPlugin --> ExtractOptions : uses
LibzipPlugin --> CommonType : has
LibzipPlugin --> MapType : has
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The
while(strFileName.contains("../"))removal is brittle and may mangle legitimate file names (and miss variations like..\); it would be safer to rely onQDir::cleanPathon the joined path and validate the resulting absolute path rather than manually stripping"../"substrings. - The final path check using
startsWith(cleanTargetPath + QDir::separator())may behave unexpectedly on case-insensitive filesystems or whenstrTargetPathlacks a trailing separator; consider normalizing case (where appropriate) and enforcing thatcleanTargetPathalways ends with a separator before the comparison. - Currently
strDestFileNameis computed before the security check and then used directly; to avoid accidental divergence, consider reusingcleanDestPath(or building a single canonical path variable) both for validation and for the actual file open.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `while(strFileName.contains("../"))` removal is brittle and may mangle legitimate file names (and miss variations like `..\`); it would be safer to rely on `QDir::cleanPath` on the joined path and validate the resulting absolute path rather than manually stripping `"../"` substrings.
- The final path check using `startsWith(cleanTargetPath + QDir::separator())` may behave unexpectedly on case-insensitive filesystems or when `strTargetPath` lacks a trailing separator; consider normalizing case (where appropriate) and enforcing that `cleanTargetPath` always ends with a separator before the comparison.
- Currently `strDestFileName` is computed before the security check and then used directly; to avoid accidental divergence, consider reusing `cleanDestPath` (or building a single canonical path variable) both for validation and for the actual file open.
## Individual Comments
### Comment 1
<location> `3rdparty/libzipplugin/libzipplugin.cpp:864-866` </location>
<code_context>
QString strDestFileName = options.strTargetPath + QDir::separator() + strFileName;
+ // Additional security check: ensure the final path is within the target directory
+ QString cleanTargetPath = QDir::cleanPath(QDir(options.strTargetPath).absolutePath());
+ QString cleanDestPath = QDir::cleanPath(QDir(strDestFileName).absolutePath());
+ if (!cleanDestPath.startsWith(cleanTargetPath + QDir::separator()) &&
+ cleanDestPath != cleanTargetPath) {
+ qInfo() << "Path traversal detected! Rejected path: " << strFileName;
</code_context>
<issue_to_address>
**issue:** The path containment check can reject valid extractions when the target is the filesystem root.
When `options.strTargetPath` is the filesystem root (e.g. `"/"`), `cleanTargetPath` remains `"/"`, so the check becomes `cleanDestPath.startsWith("//")`. A path like `"/tmp/file"` will fail both this and the equality check, and be incorrectly rejected even though it is under the intended root. If targeting the root directory is supported, you’ll need to special-case `cleanTargetPath == QDir::rootPath()` or otherwise adjust the prefix logic to correctly handle root paths.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+864
to
+866
| QString cleanTargetPath = QDir::cleanPath(QDir(options.strTargetPath).absolutePath()); | ||
| QString cleanDestPath = QDir::cleanPath(QDir(strDestFileName).absolutePath()); | ||
| if (!cleanDestPath.startsWith(cleanTargetPath + QDir::separator()) && |
There was a problem hiding this comment.
issue: The path containment check can reject valid extractions when the target is the filesystem root.
When options.strTargetPath is the filesystem root (e.g. "/"), cleanTargetPath remains "/", so the check becomes cleanDestPath.startsWith("//"). A path like "/tmp/file" will fail both this and the equality check, and be incorrectly rejected even though it is under the intended root. If targeting the root directory is supported, you’ll need to special-case cleanTargetPath == QDir::rootPath() or otherwise adjust the prefix logic to correctly handle root paths.
lzwind
approved these changes
Dec 9, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: LiHua000, lzwind The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Contributor
Author
|
/merge |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Log: fix CITIVD
Bug: https://pms.uniontech.com/bug-view-342883.html
Summary by Sourcery
Harden zip extraction against path traversal attacks by sanitizing entry names and validating destination paths.
Bug Fixes: