fix: Fix path traversal vulnerability in zip extraction (bug #232873) #331
+11
−2
Conversation
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
- 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
Reviewer's guide (collapsed on small PRs)Reviewer's GuideTightens zip extraction security by fully stripping "../" segments from entry names and adding a final absolute-path check to ensure extracted files never escape the configured target directory. Sequence diagram for secure zip entry extraction with path validationsequenceDiagram
participant Caller
participant LibzipPlugin
participant QDir
participant QFile
Caller->>LibzipPlugin: extractEntry(archive, index, options)
LibzipPlugin->>LibzipPlugin: trans2uft8(statBuffer.name, m_mapFileCode[index]) -> strFileName
loop Remove_all_parent_components
LibzipPlugin->>LibzipPlugin: while strFileName.contains(../)
LibzipPlugin->>LibzipPlugin: replace(../, "") on strFileName
end
LibzipPlugin->>LibzipPlugin: strDestFileName = options.strTargetPath + separator + strFileName
LibzipPlugin->>QDir: cleanTargetPath = cleanPath(absolutePath(strTargetPath))
LibzipPlugin->>QDir: cleanDestPath = cleanPath(absolutePath(strDestFileName))
LibzipPlugin->>LibzipPlugin: validate cleanDestPath within cleanTargetPath
alt Path_traversal_detected
LibzipPlugin-->>Caller: return ET_FileWriteError
else Path_safe
LibzipPlugin->>QFile: QFile(strDestFileName)
QFile-->>LibzipPlugin: file_handle
LibzipPlugin-->>Caller: return success
end
Class diagram for LibzipPlugin with updated extractEntry security logicclassDiagram
class LibzipPlugin {
- CommonHelper* m_common
- QMap~zip_int64_t, QString~ m_mapFileCode
+ ErrorType extractEntry(zip_t* archive, zip_int64_t index, const ExtractOptions& options)
}
class CommonHelper {
+ QString trans2uft8(const char* name, const QString& fileCode)
}
class ExtractOptions {
+ QString strTargetPath
}
class QDir {
+ QDir(const QString& path)
+ QString absolutePath()
+ static QString cleanPath(const QString& path)
+ static QChar separator()
}
class QFile {
+ QFile(const QString& fileName)
+ bool open()
}
LibzipPlugin --> CommonHelper : uses
LibzipPlugin --> ExtractOptions : uses
LibzipPlugin --> QDir : uses for path_cleaning
LibzipPlugin --> QFile : uses for_file_output
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
deepin pr auto review我来对这段代码进行审查分析：
// 原代码：
if(strFileName.indexOf("../") != -1) {
strFileName = strFileName.replace("../", "");
}
// 改进后：
while(strFileName.contains("../")) {
qInfo() << "skipped ../ path component(s) in " << strFileName;
strFileName = strFileName.replace("../", "");
}这是一个很好的改进：
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;
return ET_FileWriteError;
}这个新增的安全检查非常必要：
// 可以考虑添加对空文件名和绝对路径的检查
if (strFileName.isEmpty() || QDir::isAbsolutePath(strFileName)) {
qInfo() << "Invalid filename detected: " << strFileName;
return ET_FileWriteError;
}
// 在路径处理之前，可以考虑统一使用正斜杠
strFileName = QDir::fromNativeSeparators(strFileName);
总结： |
![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:
- Instead of manually looping and stripping "../" from
strFileName, consider normalizing the entry path withQDir::cleanPath(and handling both/and\) so that all relative components (including..) and platform-specific separators are treated consistently and safely. - The final path check using
startsWith(cleanTargetPath + QDir::separator())can be fragile (e.g., case sensitivity or subtle prefix issues); usingQDir(cleanTargetPath).relativeFilePath(cleanDestPath)and rejecting any result that starts with".."is a more robust way to ensure the extracted path stays within the target directory.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Instead of manually looping and stripping "../" from `strFileName`, consider normalizing the entry path with `QDir::cleanPath` (and handling both `/` and `\`) so that all relative components (including `..`) and platform-specific separators are treated consistently and safely.
- The final path check using `startsWith(cleanTargetPath + QDir::separator())` can be fragile (e.g., case sensitivity or subtle prefix issues); using `QDir(cleanTargetPath).relativeFilePath(cleanDestPath)` and rejecting any result that starts with `".."` is a more robust way to ensure the extracted path stays within the target directory.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
max-lvs
approved these changes
Dec 4, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: LiHua000, max-lvs 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 |
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 entry extraction to prevent directory traversal when unpacking archives.
Bug Fixes: