Skip to content

fix: Harden DBus security configuration#189

Merged
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
itsXuSt:master
Jan 16, 2026
Merged

fix: Harden DBus security configuration#189
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
itsXuSt:master

Conversation

@itsXuSt
Copy link
Contributor

@itsXuSt itsXuSt commented Jan 14, 2026

  • Remove redundant allow own rule from default policy
  • Keep allow own only in root policy for security
  • Remove redundant interface-specific allow rules
  • Simplify configuration while maintaining functionality

Log: Improve DBus security by ensuring only root can own the service

Summary by Sourcery

Tighten DBus security policy by removing redundant allow rules from the default policy so only essential access is permitted.

- Remove redundant allow own rule from default policy
- Keep allow own only in root policy for security
- Remove redundant interface-specific allow rules
- Simplify configuration while maintaining functionality

Log: Improve DBus security by ensuring only root can own the service
@sourcery-ai
Copy link

sourcery-ai bot commented Jan 14, 2026

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Tightens the DBus security policy for com.deepin.diskmanager by removing redundant interface-specific allow rules from the default policy so that ownership and access remain controlled without unnecessary permissions.

File-Level Changes

Change Details Files
Simplified and hardened the default DBus policy by removing redundant interface-specific allow rules while keeping general access and root-only ownership semantics intact.
  • Kept the broad allow rule permitting messages to be sent to com.deepin.diskmanager in the default context.
  • Removed redundant allow rules that targeted com.deepin.diskmanager for specific interfaces, including com.deepin.diskmanager, org.freedesktop.DBus.Properties, and org.freedesktop.DBus.Introspectable, relying on the general rule instead.
  • Ensured the effective policy is simpler and easier to audit while preserving intended functionality and relying on root policy for service ownership control.
service/assets/data/com.deepin.diskmanager.conf

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

@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 goal of hardening DBus security, consider whether the broad <allow send_destination="com.deepin.diskmanager"/> in the default policy should also be narrowed (e.g., by interface or member) so that non-root clients cannot call arbitrary interfaces once the service is owned by root.
  • Update the surrounding XML comments to explain why interface-specific allow rules were intentionally removed, so future maintainers don't re-add them thinking they are required for DBus introspection or properties access.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Given the goal of hardening DBus security, consider whether the broad `<allow send_destination="com.deepin.diskmanager"/>` in the default policy should also be narrowed (e.g., by interface or member) so that non-root clients cannot call arbitrary interfaces once the service is owned by root.
- Update the surrounding XML comments to explain why interface-specific allow rules were intentionally removed, so future maintainers don't re-add them thinking they are required for DBus introspection or properties access.

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.

@deepin-ci-robot
Copy link

deepin pr auto review

这段代码是对 D-Bus 系统总线配置文件 com.deepin.diskmanager.conf 的修改。主要修改是删除了针对特定接口(send_interface)的显式允许规则,仅保留了通用的 send_destination 允许规则。

以下是对该修改的详细审查意见,涵盖语法逻辑、代码质量、性能和安全性:

1. 语法逻辑

  • 审查结果:通过
  • 分析:XML 格式正确,标签闭合完整。修改前后的 XML 结构均符合 D-Bus 配置文件的语法规范。删除操作没有破坏文件的合法性。

2. 代码质量

  • 审查结果:改进
  • 分析
    • 简化配置:修改前的配置中,先允许了所有发送到 com.deepin.diskmanager 的消息(allow send_destination="..."),随后又分别列出了具体的接口(如 com.deepin.diskmanagerorg.freedesktop.DBus.Properties 等)进行允许。
    • 消除冗余:由于第一条规则 <allow send_destination="com.deepin.diskmanager"/> 的范围已经包含了后续几条针对特定接口的规则,因此后续的规则是冗余的。删除这些冗余规则使配置文件更加简洁、易读,降低了维护成本。

3. 代码性能

  • 审查结果:影响极小(正面)
  • 分析
    • D-Bus 守护进程在解析配置文件时,需要逐条检查规则。
    • 删除冗余规则可以略微减少配置文件的解析时间和规则匹配时的检查次数。虽然在实际运行中这种性能差异微乎其微,但从理论上讲,精简的配置总是更高效的。

4. 代码安全

  • 审查结果:需重点关注(权限范围未变,但依赖默认策略)
  • 分析
    • 权限范围:此次修改没有改变实际的安全策略。修改前允许“任何人调用该服务下的任何接口”,修改后依然允许“任何人调用该服务下的任何接口”。
    • 安全风险<policy context="default"> 通常适用于未授权的客户端。配置 <allow send_destination="com.deepin.diskmanager"/> 意味着系统上的任何用户(甚至低权限用户)都可以调用磁盘管理器的所有 D-Bus 方法
    • 潜在隐患:磁盘管理涉及敏感操作(如格式化、挂载)。如果该服务中包含需要提升权限的方法,且内部没有进行额外的权限校验(如 Polkit),那么当前的配置存在权限提升的安全风险。
    • 改进建议
      • 如果该服务仅供系统自身或特定用户使用,建议将 context="default" 改为 context="default" 仅保留最小权限(或者默认 deny),并为特定用户(如 root)添加 <policy user="root"> 块。
      • 如果必须允许普通用户调用,建议不要使用通配符 <allow send_destination="..."/>。应该显式列出允许调用的具体接口和方法(即保留修改前的写法,但需要确保 <allow send_destination="..."/> 这条过于宽泛的规则被移除或限制)。
      • 或者,在服务代码内部(C++/Python/Go 等)对接收到的每一个 D-Bus 调用进行严格的调用者权限检查。

总结建议

这次修改从代码维护性规范性角度来看是合理的,因为它删除了无效的冗余配置。

但是,从安全架构角度来看,目前的配置策略(允许任何人无限制访问)对于磁盘管理类服务来说是过于宽松的。

最终建议

  1. 接受本次删除冗余代码的修改。
  2. 后续需要对安全策略进行重构:移除通用的 <allow send_destination="com.deepin.diskmanager"/>,改为显式允许特定的、安全的接口;或者结合 Polkit 进行更精细的权限控制。

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: itsXuSt, max-lvs

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

@itsXuSt
Copy link
Contributor Author

itsXuSt commented Jan 16, 2026

/merge

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Jan 16, 2026

This pr cannot be merged! (status: unstable)

@itsXuSt
Copy link
Contributor Author

itsXuSt commented Jan 16, 2026

/forcemerge

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Jan 16, 2026

This pr force merged! (status: unstable)

@deepin-bot deepin-bot bot merged commit 5c6f247 into linuxdeepin:master Jan 16, 2026
16 of 18 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