Skip to content

fix(security): use SystemBusNameSubject for polkit authentication#609

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

fix(security): use SystemBusNameSubject for polkit authentication#609
deepin-bot[bot] merged 1 commit intolinuxdeepin:develop/eaglefrom
pppanghu77:develop/eagle

Conversation

@pppanghu77
Copy link

Replace insecure getPidByName() process lookup with direct D-Bus caller
identification for polkit authentication.

Changes:

  • Remove getPidByName() function that traverses /proc filesystem
  • Use SystemBusNameSubject(message().service()) instead of UnixProcessSubject(pid)
  • Remove unused dirent.h header Log: fix(security): use SystemBusNameSubject for polkit authentication Task: https://pms.uniontech.com/task-view-386841.html

Replace insecure getPidByName() process lookup with direct D-Bus caller
  identification for polkit authentication.

Changes:
  - Remove getPidByName() function that traverses /proc filesystem
  - Use SystemBusNameSubject(message().service()) instead of UnixProcessSubject(pid)
  - Remove unused dirent.h header
Log:  fix(security): use SystemBusNameSubject for polkit authentication
Task: https://pms.uniontech.com/task-view-386841.html
Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Sorry @pppanghu77, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@deepin-ci-robot
Copy link

deepin pr auto review

这段代码的 diff 展示了从"通过进程名查找 PID 进行权限验证"到"直接使用 DBus 消息发送者的 Bus Name 进行权限验证"的改动。这是一个非常好的重构。

以下是对这段代码的详细审查意见,包括语法逻辑、代码质量、性能和安全方面的分析:

1. 语法逻辑与代码质量

  • 改进点:移除了冗余的底层系统调用

    • 原代码getPidByName 函数使用了 C 风格的文件操作(opendir, readdir, fopen, sprintf 等)遍历 /proc 文件系统。这种方式代码量大,且容易出错(例如缓冲区大小硬编码为 1024,路径拼接未做安全检查)。
    • 新代码:直接利用了 DBus 通信的上下文。ControlInterface 继承自 QDBusService,它本身就持有当前的 DBus 消息上下文。通过 message().service() 获取调用者的 Bus Name,逻辑更加直接,符合 DBus 程序的设计范式。
  • 改进点:代码简洁性

    • 新代码将原来的 50+ 行逻辑缩减为几行,极大地提高了代码的可读性和可维护性。不再需要维护一个复杂的文件遍历函数。

2. 代码性能

  • 改进点:显著的性能提升
    • 原代码:每次验证权限时,都需要打开 /proc 目录,遍历所有进程(可能成百上千个),对每个进程打开 cmdline 文件并读取内容,进行字符串匹配。这是 O(N) 的操作(N 为系统进程数),涉及大量的磁盘 I/O 和系统调用,在高负载系统上会较慢。
    • 新代码:仅获取当前 DBus 消息的发送者信息,这是 O(1) 的操作,几乎不消耗额外资源。

3. 代码安全

  • 重大改进:修复了竞态条件 (TOCTOU)

    • 原代码:存在严重的安全隐患。代码先通过名称查找 PID,然后再用该 PID 创建 UnixProcessSubject 进行鉴权。在这两个操作之间,原进程可能已经结束,且该 PID 可能被一个新的恶意进程复用。这可能导致权限提升,即恶意进程冒充合法进程通过验证。
    • 新代码:使用 SystemBusNameSubject 直接绑定 DBus 连接本身。DBus 的 Bus Name 在连接断开前是唯一的,且由 DBus 守护进程管理,无法被伪造或轻易复用,从而彻底避免了 PID 复用带来的安全隐患。
  • 改进点:避免了进程名欺骗

    • 原代码:仅通过 cmdline 的后缀(endsWith)来判断进程名。任何用户都可以将自己的可执行文件命名为 deepin-devicemanager(或包含此字符串),从而骗过检查。
    • 新代码:验证的是 DBus 上的唯一标识。只有真正注册了相应 DBus 服务名称的合法客户端才能通过验证。

4. 潜在风险与建议

尽管这次改动非常好,但在实际部署时需要注意以下几点:

  1. DBus 服务名称一致性

    • 新代码依赖于 message().service() 返回的名称。这通常是一个以 :1.xxx 开头的唯一名称,或者是客户端申请的 Well-known Name(如 com.deepin.devicemanager)。
    • 建议:确认 Polkit 的策略文件(.policy 文件)中定义的 com.deepin.deepin-devicemanager.checkAuthentication 动作是否允许基于 SystemBusNameSubject 的鉴权。通常 Polkit 对此是支持的,但需要确保策略配置正确。
  2. 上下文有效性

    • message() 方法依赖于 Qt D-Bus 绑定的当前调用上下文。如果在非 D-Bus 调用的上下文中(例如异步回调或内部直接调用)调用了 getUserAuthorPasswdmessage() 可能会返回无效信息或空字符串,导致鉴权失败。
    • 建议:确保 getUserAuthorPasswd 在由 D-Bus 信号触发的槽函数或被 D-Bus 调用的接口方法中同步调用。如果需要在异步流程中使用,需要在调用开始时保存 message().service() 的结果,并在后续步骤中使用保存的副本。

总结

这次代码修改是一次极佳的优化。它不仅提升了性能和代码可读性,更重要的是修复了原代码中关于 PID 查找带来的严重安全漏洞(竞态条件和进程名欺骗)。利用 D-Bus 自身的安全机制是处理此类权限验证的最佳实践。

@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
Copy link
Contributor

deepin-bot bot commented Mar 3, 2026

This pr force merged! (status: unstable)

@deepin-bot deepin-bot bot merged commit fdd6591 into linuxdeepin:develop/eagle Mar 3, 2026
17 of 19 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