Repository navigation
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mhduiy 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 |
Reviewer's GuideThe PR mitigates CWE-78 by validating pre-uninstall script paths and file security attributes before direct QProcess execution, while allowing the uninstall DBus flow to continue when a script is untrusted and preserving existing script failure semantics. Sequence diagram for trusted pre-uninstall script executionsequenceDiagram
participant AppWiz
participant Validator as isPreUninstallScriptTrusted
participant FileSystem as QFileInfo
participant Script as PreUninstallScript
participant DBus as DBusUninstallFlow
AppWiz->>Validator: isPreUninstallScriptTrusted(args[0])
Validator->>FileSystem: isSymLink()
Validator->>FileSystem: isFile()
Validator->>FileSystem: ownerId()
Validator->>FileSystem: permissions()
alt trust validation passes
Validator-->>AppWiz: true
AppWiz->>Script: QProcess::start(args[0], args.mid(1))
AppWiz->>Script: waitForFinished(-1)
Script-->>AppWiz: exit status
AppWiz->>DBus: continue uninstall flow
else trust validation fails
Validator-->>AppWiz: false
AppWiz->>DBus: skip script and continue uninstall flow
end
Flow diagram for pre-uninstall script trust validationflowchart TD
A[Pre-uninstall script path] --> B[QProcess::splitCommand]
B --> C{Valid command}
C -->|No| X[Abort uninstall]
C -->|Yes| D[isPreUninstallScriptTrusted]
D --> E{Absolute path}
E -->|No| R[Reject and log]
E -->|Yes| F{Trusted directory}
F -->|No| R
F -->|Yes| G{Regular root-owned non-writable file}
G -->|No| R
G -->|Yes| H[QProcess::start]
R --> I[Skip script]
H --> J{Script exit status}
J -->|101 or 103| X
J -->|Other failure| K[Log failure]
J -->|Success| L[Continue uninstall]
K --> L
I --> L
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
mhduiy
marked this pull request as ready for review
October 30, 2026 06:04
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/ddeintegration/appwiz.cpp" line_range="48-61" />
<code_context>
+ }
+
+ // Layer 3: validate file type, ownership and permissions.
+ QFileInfo fi(scriptPath);
+ if (fi.isSymLink()) {
+ qCWarning(logDdeIntegration) << "Pre-uninstall script is a symbolic link, rejected:" << scriptPath;
+ return false;
+ }
+ if (!fi.isFile()) {
+ qCWarning(logDdeIntegration) << "Pre-uninstall script is not a regular file, rejected:" << scriptPath;
+ return false;
+ }
+ if (fi.ownerId() != 0) {
+ qCWarning(logDdeIntegration) << "Pre-uninstall script is not owned by root, rejected:" << scriptPath;
+ return false;
+ }
+ if (fi.permissions() & 0o022) {
+ qCWarning(logDdeIntegration) << "Pre-uninstall script is writable by group or others, rejected:" << scriptPath;
+ return false;
</code_context>
<issue_to_address>
**Replacement script bypasses validation**
When an attacker can modify the script or an ancestor directory and replaces the path after validation but before process start, `QProcess::start()` resolves the path again and executes the replacement without the ownership or permission checks performed by `isPreUninstallScriptTrusted()`.
Ensure the process executes the same file that was validated, or otherwise prevent the validated path and its ancestors from being replaced before launch.
Also at `src/ddeintegration/appwiz.cpp:115-119`.
</issue_to_address>
Comment on lines
+48
to
+61
| QFileInfo fi(scriptPath); | ||
| if (fi.isSymLink()) { | ||
| qCWarning(logDdeIntegration) << "Pre-uninstall script is a symbolic link, rejected:" << scriptPath; | ||
| return false; | ||
| } | ||
| if (!fi.isFile()) { | ||
| qCWarning(logDdeIntegration) << "Pre-uninstall script is not a regular file, rejected:" << scriptPath; | ||
| return false; | ||
| } | ||
| if (fi.ownerId() != 0) { | ||
| qCWarning(logDdeIntegration) << "Pre-uninstall script is not owned by root, rejected:" << scriptPath; | ||
| return false; | ||
| } | ||
| if (fi.permissions() & 0o022) { |
There was a problem hiding this comment.
🟠 High · Replacement script bypasses validation
When an attacker can modify the script or an ancestor directory and replaces the path after validation but before process start, QProcess::start() resolves the path again and executes the replacement without the ownership or permission checks performed by isPreUninstallScriptTrusted().
Ensure the process executes the same file that was validated, or otherwise prevent the validated path and its ancestors from being replaced before launch.
Also at src/ddeintegration/appwiz.cpp:115-119.
Prompt for AI agents
In `src/ddeintegration/appwiz.cpp` at lines 48-61:
**Replacement script bypasses validation**
When an attacker can modify the script or an ancestor directory and replaces the path after validation but before process start, `QProcess::start()` resolves the path again and executes the replacement without the ownership or permission checks performed by `isPreUninstallScriptTrusted()`.
Ensure the process executes the same file that was validated, or otherwise prevent the validated path and its ancestors from being replaced before launch.
Also at `src/ddeintegration/appwiz.cpp:115-119`.
…ction Add isPreUninstallScriptTrusted() to validate the X-Deepin-PreUninstall script path before execution: - Require absolute path (prevent PATH hijacking) - Whitelist trusted system directories (/usr/share/, /usr/lib/, /usr/libexec/, /opt/) - Verify file is not a symlink, is owned by root, and is not group/other-writable Untrusted scripts are skipped (logged) while the DBus uninstall flow continues normally.
mhduiy
force-pushed
the
agent/developer/c095abe9c85a
branch
from
October 30, 2026 16:52
dadc830 to
a3423dd
Compare
deepin pr auto review🤖 AI 代码审查报告总体评分： 📊 总体评价
🔍 详细分析1. 语法逻辑 ✅评价： 语法正确，逻辑清晰 ✅ 通过 潜在问题： 建议： ['无需修改，语法逻辑正确。'] 2. 代码质量 ✅评价： 代码结构清晰，注释完整 ✅ 通过 潜在问题： 建议： ['权限掩码 022 建议增加行内注释说明含义（group-write | other-write），提升可读性。', '调用处信任校验失败时建议补充 qCWarning 日志，与函数内部的审计日志保持一致。'] 3. 代码性能 ✅评价： 性能良好，资源使用合理 ✅ 通过 潜在问题： 建议： ['无需修改，性能良好。'] 4. 代码安全 🔓评价： 存在 3 个安全漏洞 ❌ 不通过
安全漏洞详情：
建议： ['在 Layer 2 前使用 QFileInfo::canonicalFilePath() 规范化路径，再用规范化后的路径做前缀校验和后续文件属性检查，可同时修复路径穿越和父目录符号链接两个问题', '调用处增加 qCWarning 日志记录脚本被跳过的事件，用于安全审计'] 💡 改进建议代码示例#include <QFileInfo>
#include <QDir>
static bool isPreUninstallScriptTrusted(const QString &scriptPath)
{
// Layer 1: must be an absolute path to prevent PATH hijacking.
if (!scriptPath.startsWith(QLatin1Char('/'))) {
qCWarning(logDdeIntegration) << "Pre-uninstall script path is not absolute, rejected:" << scriptPath;
return false;
}
// Canonicalize the path first to resolve any `..` components or symlinks
// (including those in parent directories) that could bypass the prefix check.
const QString canonicalPath = QFileInfo(scriptPath).canonicalFilePath();
if (canonicalPath.isEmpty()) {
qCWarning(logDdeIntegration) << "Pre-uninstall script path does not exist or cannot be resolved, rejected:" << scriptPath;
return false;
}
// Layer 2: must reside within a trusted system directory (check against canonical path).
static constexpr const char *trustedPrefixes[] = {
"/usr/share/",
"/usr/lib/",
"/usr/libexec/",
"/opt/",
};
bool inTrustedDir = false;
for (const char *prefix : trustedPrefixes) {
if (canonicalPath.startsWith(QLatin1String(prefix))) {
inTrustedDir = true;
break;
}
}
if (!inTrustedDir) {
qCWarning(logDdeIntegration) << "Pre-uninstall script path is not in a trusted directory, rejected:" << canonicalPath;
return false;
}
// Layer 3: validate file type, ownership and permissions (using canonical path).
QFileInfo fi(canonicalPath);
// canonicalFilePath() resolves all symlinks, so fi.isSymLink() is always false here.
// Keep the check as redundant defense-in-depth.
if (fi.isSymLink()) {
qCWarning(logDdeIntegration) << "Pre-uninstall script is a symbolic link, rejected:" << canonicalPath;
return false;
}
if (!fi.isFile()) {
qCWarning(logDdeIntegration) << "Pre-uninstall script is not a regular file, rejected:" << canonicalPath;
return false;
}
if (fi.ownerId() != 0) {
qCWarning(logDdeIntegration) << "Pre-uninstall script is not owned by root, rejected:" << canonicalPath;
return false;
}
// Reject if writable by group (020) or others (002).
if (fi.permissions() & 022) {
qCWarning(logDdeIntegration) << "Pre-uninstall script is writable by group or others, rejected:" << canonicalPath;
return false;
}
return true;
}
// --- At the call site ---
if (!isPreUninstallScriptTrusted(args[0])) {
qCWarning(logDdeIntegration) << "Pre-uninstall script failed trust validation, skipping execution for:" << desktopFileFullPath;
// skip script execution and continue with the DBus uninstall flow below.
} else {
// ... existing QProcess execution logic ...
}本报告由 AI 代码审查工具自动生成 |
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
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.
修复说明
在
AppWiz::legacyRequestUninstall()执行X-Deepin-PreUninstall脚本前新增isPreUninstallScriptTrusted()校验，修复 CWE-78 命令注入风险。防护措施（三层）
/usr/share/、/usr/lib/、/usr/libexec/、/opt/下的脚本行为变更
关联 Issue
DDE-520
Summary by Sourcery
Secure legacy application uninstallation by validating pre-uninstall scripts before execution.
Bug Fixes:
Enhancements: