fix: Fix path traversal vulnerability in zip extraction (bug #232873) #335
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
deepin pr auto review我来对这段代码的修改进行审查：
改进后的代码示例： // 安全处理文件路径
QString sanitizePath(const QString& path) {
QString cleanPath = QDir::cleanPath(path);
// 移除开头的斜杠
while (cleanPath.startsWith('/') || cleanPath.startsWith('\\')) {
cleanPath = cleanPath.mid(1);
}
return cleanPath;
}
// 在extractEntry函数中使用
strFileName = sanitizePath(m_common->trans2uft8(statBuffer.name, m_mapFileCode[index]));
// 验证路径是否在目标目录内
QString cleanTargetPath = QDir::cleanPath(options.strTargetPath);
QString cleanDestPath = QDir::cleanPath(options.strTargetPath + "/" + strFileName);
if (!cleanDestPath.startsWith(cleanTargetPath)) {
qInfo() << "Path traversal detected! Original path:" << statBuffer.name
<< "Sanitized path:" << strFileName;
return ET_FileWriteError;
}这些改进可以：
|
Reviewer's guide (collapsed on small PRs)Reviewer's GuideStrengthens zip extraction security by fully stripping "../" components from entry names and adding a final canonical-path containment check to ensure extracted files cannot escape the configured target directory. Sequence diagram for secured zip entry extractionsequenceDiagram
actor User
participant Application
participant LibzipPlugin
participant QDir
participant QFileSystem
User ->> Application: Request archive extraction
Application ->> LibzipPlugin: extractEntry(archive, index, options)
LibzipPlugin ->> LibzipPlugin: trans2uft8(statBuffer.name, m_mapFileCode[index])
LibzipPlugin ->> LibzipPlugin: strFileName assigned
loop Remove all ../ components
LibzipPlugin ->> LibzipPlugin: while strFileName.contains("../")
LibzipPlugin ->> LibzipPlugin: replace("../", "")
LibzipPlugin ->> LibzipPlugin: log skipped ../ path components
end
LibzipPlugin ->> LibzipPlugin: strDestFileName = options.strTargetPath + separator + strFileName
LibzipPlugin ->> QDir: cleanTargetPath = cleanPath(absolutePath(options.strTargetPath))
LibzipPlugin ->> QDir: cleanDestPath = cleanPath(absolutePath(strDestFileName))
LibzipPlugin ->> LibzipPlugin: check containment
alt Path escapes target directory
LibzipPlugin ->> LibzipPlugin: log path traversal detected
LibzipPlugin -->> Application: return ET_FileWriteError
Application -->> User: Report extraction failure
else Path within target directory
LibzipPlugin ->> QFileSystem: QFile file(strDestFileName)
LibzipPlugin ->> QFileSystem: Write extracted data
LibzipPlugin -->> Application: return success
Application -->> User: Report extraction success
end
Flow diagram for zip entry path sanitization and validationflowchart TD
A[Start extractEntry] --> B[Compute strFileName from archive entry]
B --> C{strFileName contains ../ ?}
C -- Yes --> D[Log skipped ../ path components]
D --> E[Remove ../ from strFileName]
E --> C
C -- No --> F[Build strDestFileName = targetPath + separator + strFileName]
F --> G["cleanTargetPath = cleanPath(absolutePath(targetPath))"]
G --> H["cleanDestPath = cleanPath(absolutePath(strDestFileName))"]
H --> I{cleanDestPath startsWith cleanTargetPath + separator<br/>or cleanDestPath equals cleanTargetPath ?}
I -- No --> J[Log path traversal detected]
J --> K[Return ET_FileWriteError]
I -- Yes --> L[Create QFile for strDestFileName]
L --> M[Write extracted data]
M --> N[Return success]
K --> O[End]
N --> O[End]
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 loop that repeatedly replaces "../" in
strFileNameonly covers this exact Unix-style pattern and can be bypassed (e.g.,..\, mixed slashes, or encoded forms); consider normalizing the entry path and validating components (e.g., viaQDir::cleanPathand rejecting any..segments) instead of string replacement. - The
startsWith(cleanTargetPath + QDir::separator())check may not behave as expected on case-insensitive filesystems or ifcleanTargetPathalready has a trailing separator; it would be safer to normalize both paths and compare directory components or useQDir::isRelativePath/canonical paths for containment checks. - Using
ET_FileWriteErrorfor a rejected path traversal attempt blends security validation with IO failures; consider introducing or reusing a more specific error code to distinguish security violations from write errors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The loop that repeatedly replaces "../" in `strFileName` only covers this exact Unix-style pattern and can be bypassed (e.g., `..\`, mixed slashes, or encoded forms); consider normalizing the entry path and validating components (e.g., via `QDir::cleanPath` and rejecting any `..` segments) instead of string replacement.
- The `startsWith(cleanTargetPath + QDir::separator())` check may not behave as expected on case-insensitive filesystems or if `cleanTargetPath` already has a trailing separator; it would be safer to normalize both paths and compare directory components or use `QDir::isRelativePath`/canonical paths for containment checks.
- Using `ET_FileWriteError` for a rejected path traversal attempt blends security validation with IO failures; consider introducing or reusing a more specific error code to distinguish security violations from write errors.
## Individual Comments
### Comment 1
<location> `3rdparty/libzipplugin/libzipplugin.cpp:800-802` </location>
<code_context>
- //fix 232873
- if(strFileName.indexOf("../") != -1) {
+ //fix 232873 - Remove all "../" components to prevent path traversal attacks
+ while(strFileName.contains("../")) {
qInfo() << "skipped ../ path component(s) in " << strFileName;
strFileName = strFileName.replace("../", "");
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Stripping all "../" components can result in an empty filename and unexpected extraction target.
If the entry name is "../" or all components are stripped, `strFileName` becomes empty and `strDestFileName` ends up as just the target directory. The extraction then operates on a directory path instead of a file path. Please add an explicit check for an empty (or "."/"/") `strFileName` and reject such entries before proceeding.
</issue_to_address>
### Comment 2
<location> `3rdparty/libzipplugin/libzipplugin.cpp:895-907` </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>
**🚨 suggestion (security):** The path containment check should consider platform-specific case sensitivity and use path semantics, not plain string comparison.
The `startsWith` check adds protection but is still fragile:
- On case‑insensitive filesystems (Windows, default macOS), a case‑sensitive string comparison can be bypassed with different casing.
- Pure string prefix checks ignore filesystem path semantics (components, roots, symlinks).
Where possible, compare canonicalized paths via QDir/QFileInfo (e.g. `canonicalPath()`), and normalize case on case‑insensitive platforms before applying the containment check. This will make the traversal safeguard more robust.
```suggestion
// 解压完整文件名（含路径）
QString strDestFileName = options.strTargetPath + QDir::separator() + strFileName;
// Additional security check: ensure the final path is within the target directory.
// Use canonical/absolute paths and platform-specific case rules instead of raw string prefix.
QFileInfo targetInfo(options.strTargetPath);
QString cleanTargetPath = targetInfo.canonicalFilePath();
if (cleanTargetPath.isEmpty()) {
cleanTargetPath = QDir::cleanPath(targetInfo.absoluteFilePath());
}
QFileInfo destInfo(strDestFileName);
QString cleanDestPath;
// Try to canonicalize as much of the destination path as possible (may not exist yet).
QFileInfo destDirInfo(destInfo.path());
QString canonicalDestDir = destDirInfo.canonicalFilePath();
if (!canonicalDestDir.isEmpty()) {
cleanDestPath = QDir::cleanPath(canonicalDestDir + QDir::separator() + destInfo.fileName());
} else {
cleanDestPath = QDir::cleanPath(destInfo.absoluteFilePath());
}
// Normalize for case-insensitive platforms so that casing differences cannot bypass the check.
#if defined(Q_OS_WIN) || defined(Q_OS_DARWIN)
QString normTargetPath = cleanTargetPath.toLower();
QString normDestPath = cleanDestPath.toLower();
#else
const QString &normTargetPath = cleanTargetPath;
const QString &normDestPath = cleanDestPath;
#endif
// Compare using path semantics (components) instead of plain string prefix.
QStringList targetComponents = normTargetPath.split(QChar('/'), Qt::SkipEmptyParts);
QStringList destComponents = normDestPath.split(QChar('/'), Qt::SkipEmptyParts);
bool isContained = true;
if (targetComponents.size() > destComponents.size()) {
isContained = false;
} else {
for (int i = 0; i < targetComponents.size(); ++i) {
if (targetComponents.at(i) != destComponents.at(i)) {
isContained = false;
break;
}
}
}
if (!isContained) {
qInfo() << "Path traversal detected! Rejected path:" << strFileName
<< "resolved to:" << cleanDestPath << "outside of:" << cleanTargetPath;
return ET_FileWriteError;
}
QFile file(strDestFileName);
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
lzwind
approved these changes
Dec 19, 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 |
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 path traversal outside the target directory.
Bug Fixes: