Skip to content

refactor(security): remove unused code and simplify service enable logic#497

Merged
deepin-bot[bot] merged 1 commit intolinuxdeepin:release/eaglefrom
pppanghu77:release/eagle
Mar 2, 2026
Merged

refactor(security): remove unused code and simplify service enable logic#497
deepin-bot[bot] merged 1 commit intolinuxdeepin:release/eaglefrom
pppanghu77:release/eagle

Conversation

@pppanghu77
Copy link

  • Remove unused QStandardPaths include
  • Remove unused getProcIdExe function that was reading process executable paths
  • Remove caller authorization check from setServiceEnableImpl method
  • Remove unused dbusCallerPid and checkCaller methods
  • Clean up related documentation comments Task: refactor(security): remove unused code and simplify service enable logic Task: https://pms.uniontech.com/task-view-386841.html

- Remove unused QStandardPaths include
- Remove unused getProcIdExe function that was reading process executable paths
- Remove caller authorization check from setServiceEnableImpl method
- Remove unused dbusCallerPid and checkCaller methods
- Clean up related documentation comments
Task: refactor(security): remove unused code and simplify service enable logic
Task:  https://pms.uniontech.com/task-view-386841.html
@deepin-ci-robot
Copy link

deepin pr auto review

代码审查报告

概述

这次修改移除了 SystemDBusServer 类中的调用者验证机制,包括 getProcIdExedbusCallerPidcheckCaller 函数,以及在 setServiceEnableImpl 中对调用者的验证。同时移除了不再需要的 QStandardPaths 头文件。

代码审查意见

1. 安全性问题(严重)

问题描述
移除了调用者验证机制后,任何能够访问该 DBus 服务的程序都可以调用 setServiceEnable 方法,这可能导致未经授权的服务启用/禁用操作。

改进建议

  • 如果移除验证是有意为之,需要确保 DBus 服务本身有适当的访问控制策略(如通过 DBus 策略文件限制访问)
  • 如果验证机制需要保留但需要改进,建议:
    • 使用更安全的调用者验证方法,如通过 Polkit 进行权限验证
    • 考虑使用进程的数字签名或哈希值而非路径进行比较
    • 添加日志记录所有服务启用/禁用操作,包括调用者信息

2. 代码逻辑问题

问题描述
checkCaller 函数通过比较可执行文件路径来验证调用者,这种方法存在以下问题:

  • 路径可能被符号链接欺骗
  • 不同系统上可执行文件路径可能不同
  • 没有考虑可执行文件可能被替换的情况

改进建议
如果需要保留验证机制,可以考虑:

bool SystemDBusServer::checkCaller() const
{
    if (!calledFromDBus()) {
        return false;
    }

    qint64 callerPid = dbusCallerPid();
    QString callerExe = getProcIdExe(callerPid);
    
    // 使用更可靠的验证方式,例如检查进程的命令行参数
    QFile cmdLineFile(QString("/proc/%1/cmdline").arg(callerPid));
    if (!cmdLineFile.open(QIODevice::ReadOnly)) {
        return false;
    }
    
    QByteArray cmdline = cmdLineFile.readAll();
    cmdLineFile.close();
    
    // 检查命令行是否包含预期的程序名
    if (!cmdline.contains("deepin-system-monitor")) {
        return false;
    }
    
    // 可以添加额外的验证,如检查进程的启动时间、用户等
    return true;
}

3. 代码性能问题

问题描述
getProcIdExe 函数使用了 realpath 系统调用,这在频繁调用时可能影响性能。

改进建议
如果需要频繁获取进程信息,可以考虑缓存结果或使用更高效的系统调用。

4. 代码质量建议

问题描述

  • 原代码中 checkCaller 函数的日志输出使用了 qCDebug,这在生产环境中可能不会输出
  • 错误处理不够详细,缺少具体的错误信息

改进建议

  • 使用更合适的日志级别(如 qCWarning)记录安全相关事件
  • 提供更详细的错误信息,包括失败原因

5. 替代方案建议

如果需要更安全的调用者验证机制,可以考虑以下方案:

  1. 使用 Polkit 进行权限验证
bool SystemDBusServer::checkCaller() const
{
    if (!calledFromDBus()) {
        return false;
    }
    
    // 使用 Polkit 检查调用者是否有权限
    PolkitQt1::Authority::Result result;
    result = PolkitQt1::Authority::instance()->checkAuthorizationSync(
        "com.deepin.systemmonitor.manage-services",
        PolkitQt1::UnixProcessSubject(dbusCallerPid()),
        PolkitQt1::Authority::AllowUserInteraction);
    
    return result == PolkitQt1::Authority::Yes;
}
  1. 使用 DBus 策略文件限制访问
    在 DBus 服务配置文件中添加访问控制:
<policy user="root">
  <allow own="com.deepin.systemmonitor.SystemServer"/>
</policy>

<policy context="default">
  <allow send_destination="com.deepin.systemmonitor.SystemServer"
         send_interface="com.deepin.systemmonitor.SystemServer"
         send_member="setServiceEnable"/>
</policy>

总结

这次修改移除了调用者验证机制,虽然简化了代码,但引入了严重的安全风险。建议:

  1. 如果验证机制确实不需要,确保通过其他方式(如 DBus 策略文件)限制访问
  2. 如果需要验证机制,考虑使用更安全的方法(如 Polkit)
  3. 添加详细的日志记录所有敏感操作
  4. 对所有公开的 DBus 方法进行安全审查,确保没有其他类似的安全问题

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: KT-lcz, max-lvs, pppanghu77

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

@pppanghu77
Copy link
Author

/forcemerge

@deepin-bot deepin-bot bot merged commit ea39a3e into linuxdeepin:release/eagle Mar 2, 2026
6 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.

4 participants