Skip to content

fix(security): use safe QProcess API to prevent command injection#678

Merged
deepin-bot[bot] merged 1 commit into
linuxdeepin:masterfrom
add-uos:fix-362977-fix-qprocess-command-injection
Jun 3, 2026
Merged

fix(security): use safe QProcess API to prevent command injection#678
deepin-bot[bot] merged 1 commit into
linuxdeepin:masterfrom
add-uos:fix-362977-fix-qprocess-command-injection

Conversation

@add-uos

@add-uos add-uos commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Replace QProcess::start(QString) with the safer start(program, args) form that separates the executable from its arguments, avoiding shell interpretation. For cases where the full command string is intentional, use QProcess::startCommand() which explicitly documents shell behavior.

Affected areas:

  • Driver management: depmod, apt update, update-initramfs
  • Device info: hwinfo queries for USB detection
  • DBus calls: gdbus call commands via QStringList args
  • Input device: hcitool bluetooth connection check
  • Disk device: cat sysfs paths for UFS info
  • System tools: CPU info and xrandr commands

Also updates SPDX copyright years to include 2026 where needed.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @add-uos, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown
  • 敏感词检查失败, 检测到2个文件存在敏感词
详情
{
    "deepin-devicemanager-server/deepin-devicecontrol/src/drivercontrol/drivermanager.cpp": [
        {
            "line": "    QString url = \"http://drive-pre.uniontech.com/api/v1/drive/search?arch=\" + archMap[Utils::machineArch()];",
            "line_number": 552,
            "rule": "S35",
            "reason": "Url link | 3b0f0efeb3"
        }
    ],
    "deepin-devicemanager-server/deepin-devicecontrol/src/drivercontrol/utils.cpp": [
        {
            "line": "        return \"https://driver.uniontech.com/api/v1/drive/search\";",
            "line_number": 259,
            "rule": "S35",
            "reason": "Url link | 162cef9d4a"
        },
        {
            "line": "        return \"https://driver.uniontech.com/api/v1/drive/search\";",
            "line_number": 263,
            "rule": "S35",
            "reason": "Url link | 162cef9d4a"
        },
        {
            "line": "        return \"https://drive-pre.uniontech.com/api/v1/drive/search\";",
            "line_number": 265,
            "rule": "S35",
            "reason": "Url link | 0e010283c1"
        }
    ]
}

Replace QProcess::start(QString) with the safer start(program, args)
form that separates the executable from its arguments, avoiding shell
interpretation. For cases where the full command string is intentional,
use QProcess::startCommand() which explicitly documents shell behavior.

Affected areas:
- Driver management: depmod, apt update, update-initramfs
- Device info: hwinfo queries for USB detection
- DBus calls: gdbus call commands via QStringList args
- Input device: hcitool bluetooth connection check
- Disk device: cat sysfs paths for UFS info
- System tools: CPU info and xrandr commands

Also updates SPDX copyright years to include 2026 where needed.
@add-uos add-uos force-pushed the fix-362977-fix-qprocess-command-injection branch from 0bf37c2 to 40c798c Compare June 2, 2026 11:26
@deepin-ci-robot

Copy link
Copy Markdown

deepin pr auto review

你好!我是CodeGeeX。我已仔细审查了你提供的Git Diff。本次修改的核心是将 QProcess 的调用方式从单一字符串模式(如 process.start("cmd arg1"))更改为分离参数模式(如 process.start("cmd", QStringList() << "arg1"))或使用新的API(startCommand),同时更新了版权声明。

整体来看,这是一次非常优秀且必要的安全与规范改进。以下是详细的审查意见:

一、 语法逻辑 (✅ 正确)

  1. 参数分离模式 (最核心的改动)
    process.start("cmd arg") 改为 process.start("cmd", QStringList() << "arg") 是完全正确的。在Qt中,单参数的 start() 重载函数(在Qt 5中)已被标记为废弃,且它依赖于底层的 shell 解析(如 /bin/sh -c),容易因参数包含空格或特殊字符导致解析错误。分离参数模式直接将参数传递给 execvp,避免了 shell 注入和解析异常。
  2. startCommand 的使用
    LoadCpuInfoThread.cppThreadExecXrandr.cpp 中,使用了 process.startCommand(cmd)。这是 Qt 6.0 引入的新 API,专门用于替代旧的单参数 start(),逻辑上完全正确,表明项目可能正在向 Qt 6 迁移或兼容。
  3. 版权声明更新
    ~ 改为 -,并更新年份,符合 SPDX 规范,逻辑无误。

二、 代码质量 (⭐ 优秀,有微小改进空间)

  1. 消除了冗余代码
    utils.cpp 中,原本通过 if-else 拼接字符串的逻辑被简化为了一行三元运算符:
    process.start("depmod", QStringList() << (bquick ? "--quick" : "--all"));
    这大大提高了代码的简洁性和可读性,非常棒。
  2. 参数对齐与格式
    mainjob.cpp 等文件中,对 gdbus 的长参数进行了换行对齐,格式规范,阅读体验良好。

三、 代码性能 (⚠️ 存在严重隐患)

本次 Diff 暴露了原有代码中一个严重的性能问题,虽然不是本次引入的,但在审查时必须指出:

  1. 主线程阻塞(UI卡顿)
    在多处代码中,如 depmod_a(), cmdAptUpdate(), MonitorUsb::monitor() 等,直接调用了 process.waitForFinished(-1)(无限期等待)或 process.waitForFinished()
    • 如果这些函数运行在**主线程(GUI线程)**中,会导致界面完全冻结,直到外部命令(如 apt updatedepmod -a,这些命令可能执行数秒甚至数十秒)执行完毕。
    • 改进建议:必须将耗时的 QProcess 操作移至子线程(使用 QRunnable 或继承 QThread),或者使用 QProcess 的异步信号(如 finished()readyReadStandardOutput())来处理结果,避免阻塞主线程。
  2. 高频调用场景的阻塞
    MonitorUsb::monitor() 中监听 USB 热插拔,每次 add/remove 事件都会同步执行 hwinfo --usbhwinfo 是一个扫描硬件的重命令,执行耗时较长。在热插拔场景下,这极易导致事件响应延迟或程序卡死。

四、 代码安全 (🛡️ 显著提升,仍有遗留风险)

  1. 已修复的安全风险(命令注入)
    旧代码使用 process.start("cmd " + arg),如果 arg 来自外部输入(如设备名),极易引发命令注入。本次修改全面采用了参数分离,从根本上杜绝了 Shell 注入的风险,安全性大幅提升。
  2. 遗留的安全与健壮性风险
    • 使用 cat 读取系统文件 (HWGenerator.cpp):
      process.start("cat", QStringList() << "/sys/...");
      在 C++/Qt 程序中,调用外部进程去读取文件是低效且不安全的。如果文件权限不足或不存在,cat 的报错信息会混入标准输出,且多了一个进程开销。
      改进建议:直接使用 Qt 的文件 API 读取:
      QFile file("/sys/devices/platform/f8200000.ufs/host0/scsi_host/host0/wb_en");
      if (file.open(QIODevice::ReadOnly | QIODevice::Text)) {
          QString deviceInfo = file.readAll().trimmed();
          file.close();
          // 处理 deviceInfo
      }
    • startCommand 的潜在注入风险 (LoadCpuInfoThread.cpp, ThreadExecXrandr.cpp):
      process.startCommand(cmd) 依然会经过 Shell 解析。如果这里的 cmd 变量是动态拼接的(例如包含了设备名、文件路径等),依然存在命令注入或空格解析异常的风险。
      改进建议:如果 cmd 是纯静态字符串(如固定为 lscpu),则无大碍;如果包含动态变量,强烈建议重构为 process.start(exe, args) 的分离参数形式。
    • 未检查进程执行状态
      多处代码在 waitForFinished 后直接 readAllStandardOutput(),没有检查进程的退出码(exitCode())或错误状态(error())。如果 depmodapt 执行失败,程序会静默处理空字符串,可能导致后续逻辑出现难以排查的 Bug。

总结与修改建议

本次提交的初衷和实施效果都非常好,修复了 Qt 开发中常见的 Shell 注入和参数解析隐患。建议在后续迭代中关注以下几点:

  1. 高优先级:排查所有 waitForFinished(-1) 的调用点,必须确保它们不在主线程执行,或者重构为异步调用,否则将严重影响用户体验(界面卡顿)。
  2. 中优先级:将 HWGenerator.cpp 中通过 QProcess 调用 cat 读取 /sys 文件的代码,替换为 QFile 直接读取。
  3. 低优先级:为 QProcess 添加退出状态和错误检查逻辑,增强程序的健壮性;确认 startCommand(cmd) 中的 cmd 是否包含动态不可信数据。

@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown
  • 敏感词检查失败, 检测到2个文件存在敏感词
详情
{
    "deepin-devicemanager-server/deepin-devicecontrol/src/drivercontrol/drivermanager.cpp": [
        {
            "line": "    QString url = \"http://drive-pre.uniontech.com/api/v1/drive/search?arch=\" + archMap[Utils::machineArch()];",
            "line_number": 552,
            "rule": "S35",
            "reason": "Url link | 3b0f0efeb3"
        }
    ],
    "deepin-devicemanager-server/deepin-devicecontrol/src/drivercontrol/utils.cpp": [
        {
            "line": "        return \"https://driver.uniontech.com/api/v1/drive/search\";",
            "line_number": 259,
            "rule": "S35",
            "reason": "Url link | 162cef9d4a"
        },
        {
            "line": "        return \"https://driver.uniontech.com/api/v1/drive/search\";",
            "line_number": 263,
            "rule": "S35",
            "reason": "Url link | 162cef9d4a"
        },
        {
            "line": "        return \"https://drive-pre.uniontech.com/api/v1/drive/search\";",
            "line_number": 265,
            "rule": "S35",
            "reason": "Url link | 0e010283c1"
        }
    ]
}

@deepin-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: add-uos, lzwind

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@add-uos

add-uos commented Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

/forcemerge

@deepin-bot

deepin-bot Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

This pr force merged! (status: unstable)

@deepin-bot deepin-bot Bot merged commit c988b0b into linuxdeepin:master Jun 3, 2026
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants