Skip to content

fix(security): VPLUS-2026-34718 - fix DBus permission configuration#652

Merged
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
pengfeixx:master
May 7, 2026
Merged

fix(security): VPLUS-2026-34718 - fix DBus permission configuration#652
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
pengfeixx:master

Conversation

@pengfeixx
Copy link
Copy Markdown
Contributor

@pengfeixx pengfeixx commented May 7, 2026

  • Implement method-level permission separation for DBus service
  • Default deny policy for all methods
  • Whitelist read-only methods (getAuthorizedInfo, getRemoveInfo, etc.)
  • Dangerous methods (aptUpdate, installDriver, disableInDevice) require authentication
  • Add Debug build support with warning messages
  • Ensure Release builds have Polkit enabled

Security impact:

  • Fixes privilege escalation vulnerability (CVSS 8.1)
  • Prevents unauthorized access to root-level operations
  • Maintains developer convenience in Debug builds

CVSS: 8.1 (High)
Affected: All systems with deepin-devicemanager installed
Fix version: 6.0.62

Summary by Sourcery

Tighten DBus access control for deepin-devicemanager and enforce safer Polkit behavior across build types.

New Features:

  • Introduce method-level DBus permission rules allowing only specific read-only methods to be called without additional authorization.

Bug Fixes:

  • Prevent unauthorized invocation of privileged DBus methods by switching to a default-deny DBus policy for the DeviceControl service.

Enhancements:

  • Enable automatic Polkit disabling in Debug builds with explicit warning messages while ensuring Polkit remains enabled in non-Debug builds.

@sourcery-ai
Copy link
Copy Markdown

sourcery-ai Bot commented May 7, 2026

Reviewer's Guide

Tightens DBus security for deepin-devicemanager by moving from a global allow policy to a default-deny, method-level whitelist and wiring CMake to automatically disable Polkit only for debug builds with explicit warnings.

Sequence diagram for DBus method-level permission and Polkit enforcement

sequenceDiagram
    actor Client
    participant SystemDBus
    participant DeviceControlService
    participant Polkit

    rect rgb(230,230,230)
        note over Client,SystemDBus: Read-only whitelisted method (e.g. getAuthorizedInfo)
        Client->>SystemDBus: call getAuthorizedInfo
        SystemDBus->>SystemDBus: check policy: default deny
        SystemDBus->>SystemDBus: match whitelist for getAuthorizedInfo
        SystemDBus->>DeviceControlService: forward getAuthorizedInfo
        DeviceControlService-->>SystemDBus: return data
        SystemDBus-->>Client: return data
    end

    rect rgb(230,230,230)
        note over Client,SystemDBus: Dangerous method (e.g. installDriver) in Release build
        Client->>SystemDBus: call installDriver
        SystemDBus->>SystemDBus: check policy: default deny
        SystemDBus-->>Client: reject if not explicitly allowed
    end

    rect rgb(230,230,230)
        note over Client,DeviceControlService: Dangerous method with Polkit (conceptual flow)
        Client->>SystemDBus: call installDriver (authorized caller)
        SystemDBus->>DeviceControlService: forward installDriver
        DeviceControlService->>Polkit: check authorization
        Polkit-->>DeviceControlService: allow
        DeviceControlService-->>SystemDBus: perform installDriver
        SystemDBus-->>Client: success
    end
Loading

File-Level Changes

Change Details Files
Switch DBus config from global allow to default-deny with a whitelist of read-only methods.
  • Replace broad allow policy for org.deepin.DeviceControl with a deny rule in the default context.
  • Add a dedicated default-context policy that explicitly allows only specific read-only methods on org.deepin.DeviceControl via send_destination/send_interface/send_member filters.
  • Ensure root-only ownership and send permissions for the org.deepin.DeviceControl service remain in place.
deepin-devicemanager-server/deepin-devicecontrol/org.deepin.devicecontrol.conf
Adjust CMake Polkit configuration to be debug-friendly but safe for release builds.
  • Rename the DISABLE_POLKIT cache description to clarify it is for debug usage.
  • Automatically set DISABLE_POLKIT to true when CMAKE_BUILD_TYPE is Debug.
  • Emit CMake WARNING messages when building with Polkit disabled to discourage production use.
  • Gate the -DDISABLE_POLKIT definition on the (possibly auto-set) DISABLE_POLKIT option.
CMakeLists.txt

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link
Copy Markdown

@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.

Hey - I've left some high level feedback:

  • Given the new default-deny DBus policy with a hardcoded method allowlist, it would be helpful to add a brief comment in the config explaining that any new methods must be explicitly added here, to reduce the risk of future features accidentally remaining inaccessible or unintentionally opening up privileged operations.
  • In the CMake logic, consider adding an explicit warning or guard when DISABLE_POLKIT is manually enabled in non-Debug builds (e.g., Release) so that developers are clearly informed that they are building a production configuration with Polkit disabled.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Given the new default-deny DBus policy with a hardcoded method allowlist, it would be helpful to add a brief comment in the config explaining that any new methods must be explicitly added here, to reduce the risk of future features accidentally remaining inaccessible or unintentionally opening up privileged operations.
- In the CMake logic, consider adding an explicit warning or guard when `DISABLE_POLKIT` is manually enabled in non-Debug builds (e.g., `Release`) so that developers are clearly informed that they are building a production configuration with Polkit disabled.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

- Implement method-level permission separation for DBus service
- Default deny policy for all methods
- Whitelist read-only methods (getAuthorizedInfo, getRemoveInfo, etc.)
- Dangerous methods (aptUpdate, installDriver, disableInDevice) require authentication
- Add comprehensive security policy documentation

Security impact:
- Fixes privilege escalation vulnerability (CVSS 8.1)
- Prevents unauthorized access to root-level operations
- Clear maintenance guidelines for future DBus method additions

CVSS: 8.1 (High)
Affected: All systems with deepin-devicemanager installed
Fix version: 6.0.62
PMS: TASK-389221
@deepin-ci-robot
Copy link
Copy Markdown

deepin pr auto review

这份代码 diff 展示了对 D-Bus 服务配置文件 org.deepin.devicecontrol.conf 的安全加固修改。这是一个非常关键的修改,旨在防止未授权的特权操作。

以下是对该配置文件的详细审查意见,包括语法逻辑、代码质量、代码性能和代码安全方面的评估与改进建议:

1. 语法逻辑

  • 审查结果:良好
  • 分析
    • XML 结构符合 D-Bus 配置文件的 DTD 定义(busconfig.dtd)。
    • 标签嵌套正确,属性使用规范。
    • 注释格式正确,且提供了清晰的上下文。
  • 逻辑流
    • 采用了“默认拒绝”的安全模型:<policy context="default"> 中首先 <deny ... />,这是最安全的做法。
    • 随后显式列出了白名单方法,逻辑清晰。
    • Root 用户拥有完全控制权,符合系统服务惯例。

2. 代码质量

  • 审查结果:良好,有优化空间
  • 优点
    • 文档详尽:顶部的注释非常出色,解释了策略意图、新增方法的处理流程以及安全背景(VPLUS-2026-34718)。这对后续维护者非常有帮助。
    • 可读性:白名单部分虽然冗长,但结构一致,易于阅读和查找。
  • 改进建议
    • 分组:白名单中的方法较多,建议在注释中或通过空行将它们按功能分组(例如:设备信息查询、驱动状态查询、系统状态查询)。这样当需要审查某个方法是否应该被公开时,能更快定位。
    • 命名一致性:方法名如 isBlackListed 建议统一改为 isBlacklisted(去掉大写L),以符合现代编程的命名规范,但这取决于后端 C++/Qt 代码的实际定义,如果改动成本大可保持现状。

3. 代码性能

  • 审查结果:影响极小
  • 分析
    • D-Bus 守护进程在启动时解析此配置文件,并将其加载到内存中。
    • 当消息到达时,D-Bus 守护进程会按顺序遍历策略规则。由于规则数量有限(约10条白名单规则),这种线性查找的性能开销可以忽略不计。
    • 结论:这种显式的白名单方式对运行时性能没有负面影响。

4. 代码安全 —— 核心关注点

  • 审查结果:优秀(显著提升安全性)

  • 改进分析

    • 修复了重大漏洞:原配置中 <policy context="default"> 允许任何人调用所有方法(<allow send_destination="org.deepin.DeviceControl"/>)。这意味着任何普通用户甚至恶意脚本都可以调用 aptUpdateinstallDriver 等危险操作,极易导致提权攻击。新配置修复了这一问题。
    • 最小权限原则:新配置严格遵循最小权限原则,只将查询类方法暴露给所有用户。
    • 防御深度:配合注释中提到的 Polkit 进行二次认证,即使 D-Bus 层面配置有疏漏,后端仍有 Polkit 把关。
  • 潜在风险与改进建议

    1. 信息泄露风险

      • 问题:白名单中包含了 getAuthorizedInfogetRemoveInfo 等方法。虽然这些是“只读”的,但可能会泄露系统硬件拓扑、已安装的驱动版本或特定的系统配置信息给低权限用户。
      • 建议:需要审查这些返回的数据是否包含敏感信息。如果包含,建议将这些方法也移出白名单,要求调用者提供认证。
    2. 缺少接口限制

      • 问题:目前的白名单规则只指定了 send_interface="org.deepin.DeviceControl"。如果该服务还实现了其他接口(例如标准的 org.freedesktop.DBus.Introspectableorg.freedesktop.DBus.Properties),这些接口可能会被默认策略(Deny)拦截,导致普通用户无法进行基本的内省或属性读取,这可能会破坏某些调试工具或高级客户端的功能。
      • 建议
        • 如果希望普通用户能够查看服务接口定义,应显式允许 Introspectable 接口:
          <allow send_destination="org.deepin.DeviceControl"
                 send_interface="org.freedesktop.DBus.Introspectable"/>
        • 如果使用了属性(Properties),且希望普通用户可读,也应添加相应的 org.freedesktop.DBus.Properties 读取权限。
    3. 方法名审查

      • 问题:白名单中的 monitorWorkingDBFlag。从名字看,它似乎是用来监控数据库标志位的。如果这个方法不仅仅是读取,还包含某种“订阅”或“设置”的逻辑,将其放入白名单可能存在风险。
      • 建议:确认该方法的实现是否纯粹为无副作用的读取操作。
    4. 配置文件的完整性

      • 问题:diff 中未显示 <policy at_console="true"> 或其他特定组的策略。
      • 建议:确保没有在文件的其他部分(diff 未显示的部分)存在 <allow send_destination="org.deepin.DeviceControl"/> 的通配符规则,否则会破坏这里的白名单防御体系。

总结

这是一个高质量的安全修复。它从“默认允许”的不安全状态转变为“默认拒绝+白名单”的安全状态。

最终建议:

  1. 确认 getAuthorizedInfo 等接口返回的数据不涉及敏感隐私。
  2. 考虑是否需要显式允许 org.freedesktop.DBus.Introspectable 以便调试。
  3. 确保后端代码对危险方法(如 aptUpdate)确实实施了 Polkit 检查,形成双重保险。

@deepin-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lzwind, pengfeixx

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

@pengfeixx
Copy link
Copy Markdown
Contributor Author

/merge

@deepin-bot deepin-bot Bot merged commit cc53177 into linuxdeepin:master May 7, 2026
22 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