Skip to content

fix(power): secure system power actions - #157

Open
mhduiy wants to merge 1 commit into
task/x11powerfrom
task/x11power-security
Open

fix(power): secure system power actions#157
mhduiy wants to merge 1 commit into
task/x11powerfrom
task/x11power-security

Conversation

@mhduiy

@mhduiy mhduiy commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

This is a stacked security follow-up for #153 and contains only the system Power1 authorization integration.

  1. Register trusted callers with persisted UID and PID validation.
  2. Authorize privileged power changes through caller records or Polkit.
  3. Restrict SetAllowCaller to trusted registrars and D-Bus policies.
  4. Add the deepin-security-loader and Polkit build/runtime dependencies.

This PR is intentionally separate because the security-loader authorization design may change independently. After #153 merges, retarget this PR to master.

Log: Protect system Power1 mutations with explicit caller authorization.
PMS: TASK-394241
Influence: System power changes require authorization.

Summary by Sourcery

Secure Power1 system power actions with trusted caller validation and Polkit authorization.

New Features:

  • Add trusted caller registration with persisted UID and PID validation for Power1 operations.
  • Authorize privileged system power changes through trusted callers or Polkit.

Enhancements:

  • Restrict SetAllowCaller to authorized root or deepin-daemon callers through D-Bus policy enforcement.
  • Protect system power mutation methods with explicit caller authorization checks.

Build:

  • Add the polkit-qt6 dependency to the system power module build.

@deepin-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mhduiy

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

@sourcery-ai

sourcery-ai Bot commented Aug 18, 2026

Copy link
Copy Markdown

Reviewer's Guide

Integrates explicit caller authorization into the system Power1 service: trusted callers are registered and persisted with UID/PID validation, privileged power mutations are gated by an internal allow‑list plus Polkit, SetAllowCaller is restricted via D‑Bus policy, and Polkit/deepin‑security‑loader build/runtime deps are added.

Sequence diagram for Power1 caller authorization on privileged power actions

sequenceDiagram
  actor Caller
  participant SystemPowerManager
  participant PolkitAuthority

  Caller->>SystemPowerManager:SetMode
  SystemPowerManager->>SystemPowerManager:authorizePowerAction
  alt calledFromDBus() is false
    SystemPowerManager->>SystemPowerManager:setMode
    SystemPowerManager-->>Caller:mode updated
  else calledFromDBus() is true
    SystemPowerManager->>SystemPowerManager:isAllowedCaller
    alt isAllowedCaller returns true
      SystemPowerManager->>SystemPowerManager:setMode
      SystemPowerManager-->>Caller:mode updated
    else lookupFailed is true
      SystemPowerManager->>Caller:sendErrorReply
    else not allowed and lookup ok
      SystemPowerManager->>PolkitAuthority:checkAuthorizationSync(kPowerAction)
      alt PolkitQt1::Authority::Yes
        SystemPowerManager->>SystemPowerManager:setMode
        SystemPowerManager-->>Caller:mode updated
      else hasError or result != Yes
        SystemPowerManager->>Caller:sendErrorReply
      end
    end
  end
Loading

File-Level Changes

Change Details Files
Track and persist an allow‑list of trusted D‑Bus callers for Power1 actions, with UID/PID validation and legacy state migration.
  • Introduce AllowedCaller struct and m_allowedCallers hash on SystemPowerManager to store caller UID/PID pairs keyed by unique bus name.
  • Implement addAllowedCaller to resolve caller credentials via QDBusConnectionInterface and enforce registrar constraints (root or deepin-daemon group and process ancestry).
  • Add loadAllowedCallers to read security state JSON from the new path, validate against the system bus ID, verify current UID/PID, and migrate legacy allow-caller state if needed.
  • Add saveAllowedCallers to serialize the allow‑list to JSON under a namespaced scope, create the runtime state directory with owner-only permissions, and atomically write via QSaveFile.
src/plugin-qt/power/system/powermanager.h
src/plugin-qt/power/system/powermanager.cpp
Require authorization for mutating power actions via the allow‑list or Polkit, and expose a controlled SetAllowCaller D‑Bus API.
  • Add isAllowedCaller to re-validate allowed callers against current D‑Bus service UID/PID and detect lookup failures.
  • Introduce authorizePowerAction to gate power mutations: allow non-D‑Bus callers, allow known allowed callers, otherwise perform a PolkitQt1 authorization check on org.deepin.dde.power.doAction and return appropriate D‑Bus errors.
  • Invoke authorizePowerAction at the start of SetCpuGovernor, SetCpuBoost, SetMode, and SetTlpMode D‑Bus methods.
  • Add SetAllowCaller slot to register a caller by unique name and return InvalidArgs via D‑Bus on failure.
src/plugin-qt/power/system/powermanager.h
src/plugin-qt/power/system/powermanager.cpp
Tighten D‑Bus policy for Power1 so that only root and deepin-daemon group can call SetAllowCaller, and deny it for regular user sessions.
  • Extend root policy to allow sending SetAllowCaller to org.deepin.dde.Power1.
  • Add a deepin-daemon group policy that allows SetAllowCaller calls to the Power1 interface.
  • Add a session user policy deny rule for SetAllowCaller on org.deepin.dde.Power1 to prevent unprivileged registration of allowed callers.
src/plugin-qt/power/system/misc/dbus/org.deepin.dde.Power1.conf
Add PolkitQt to the build and link configuration to support runtime authorization checks for power actions.
  • Detect and configure polkit-qt6-1 via pkg_check_modules, exposing POLKIT_QT_* variables.
  • Include PolkitQt1 headers in powermanager.cpp and link the Power system plugin against ${POLKIT_QT_LIBRARIES}.
  • Add POLKIT_QT include directories to the plugin target’s include path.
src/plugin-qt/power/system/CMakeLists.txt
src/plugin-qt/power/system/powermanager.cpp
Introduce helper functions to inspect process group membership and ancestry for trusted registrar validation.
  • Add deepinDaemonGroup helper to resolve the deepin-daemon group ID via getgrnam.
  • Implement processHasGroup to read /proc//status, parse Gid/Groups lines, and check if a process belongs to a specific group.
  • Implement readParentPid and isProcessDescendant to walk the process tree via /proc to ensure a caller process is a descendant of the registrar process.
src/plugin-qt/power/system/powermanager.cpp

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

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

Hey - I've reviewed your changes and they look great!


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.

1. Register trusted callers with persisted UID and PID validation.
2. Authorize privileged power changes through caller records or Polkit.
3. Restrict SetAllowCaller to trusted registrars and D-Bus policies.
4. Add the security-loader and Polkit build/runtime dependencies.

Log: Protect system Power1 mutations with explicit caller authorization.
Influence: System power changes require authorization.

fix(power): 加固系统电源操作

1. 通过持久化的 UID 和 PID 校验登记可信调用者。
2. 通过调用者记录或 Polkit 鉴权敏感电源变更。
3. 通过可信登记者和 D-Bus 策略限制 SetAllowCaller。
4. 添加 security-loader 与 Polkit 构建和运行依赖。

Log: 为系统 Power1 敏感操作增加明确的调用者鉴权。
PMS: TASK-394241
Influence: 系统电源变更需要通过鉴权。
@mhduiy
mhduiy force-pushed the task/x11power-security branch from f1fa158 to fc7b8ef Compare August 18, 2026 12:41
@deepin-ci-robot

Copy link
Copy Markdown

deepin pr auto review

★ 总体评分:55分

■ 【总体评价】

代码实现了基于Polkit和白名单的电源管理鉴权机制,但存在进程关系检查的TOCTOU竞态条件风险。
逻辑正确但因潜在权限绕过风险扣分,且受安全漏洞上限规则限制。

■ 【详细分析】

  • 1.语法逻辑(正确)✓

代码逻辑清晰,addAllowedCallerloadAllowedCallersauthorizePowerAction 等函数实现完整。空指针检查(如 m_conn)和错误处理(如 QDBusReplyisValid())均到位。
潜在问题:isProcessDescendant 中限制深度为 4096,虽然防止了死循环,但在极端深度的进程树中可能误判。
建议:保持当前深度限制,或根据系统最大 PID 值进行动态调整。

  • 2.代码质量(良好)✓

代码结构清晰,命名规范,注释完整。/proc 文件读取逻辑封装合理,状态持久化使用了 QSaveFile 保证原子性。
潜在问题:saveAllowedCallers 在每次 addAllowedCaller 时全量写入文件,若调用频繁可能产生不必要的 I/O。
建议:若预期调用频率较高,可考虑引入延迟写入机制或标记 dirty 位在空闲时统一写入。

  • 3.代码性能(无性能问题)✓

鉴权操作主要在 DBus 调用时触发,isAllowedCaller 仅进行哈希查找和简单的 DBus 属性查询,性能开销极小。isProcessDescendant 的循环读取 /proc 仅在注册白名单时发生,频率低。
潜在问题:无
建议:无

  • 4.代码安全(存在 1 个安全漏洞)✕

漏洞对比统计:新增漏洞 1 个,减少漏洞 0 个,持平 0 个
总体风险描述:白名单注册机制存在 TOCTOU 竞态条件,可能导致非授权进程获取电源管理权限。

  • 安全漏洞1(中危):TOCTOU竞态条件 在 SystemPowerManager::addAllowedCaller 中,通过读取 /proc/[pid]/status 验证进程关系后,被验证的进程可能在真正使用权限前退出,导致其 PID 被同 UID 的其他恶意进程复用,从而绕过白名单检查获取电源操作权限 ——非常重要

  • 建议:在 isAllowedCaller 中除了检查 UID 和 PID 外,还应检查进程的启动时间或可执行文件路径,以确保当前持有该 PID 的进程确为最初注册的进程。

■ 【改进建议代码示例】

// 在 powermanager.h 中添加启动时间记录
struct AllowedCaller {
    uint uid = 0;
    uint pid = 0;
    quint64 startTime = 0; // 新增:进程启动时间
};

// 修改 addAllowedCaller 获取并保存 startTime
bool SystemPowerManager::addAllowedCaller(const QString &uniqueName)
{
    // ... 前置检查 ...
    const QDBusReply<uint> uid = bus->serviceUid(uniqueName);
    const QDBusReply<uint> pid = bus->servicePid(uniqueName);
    if (!uid.isValid() || !pid.isValid())
        return false;

    // 获取进程启动时间
    quint64 startTime = 0;
    QFile statFile(QStringLiteral("/proc/%1/stat").arg(pid.value()));
    if (statFile.open(QIODevice::ReadOnly)) {
        const QString data = QString::fromLatin1(statFile.readAll());
        // 解析 /proc/[pid]/stat 的第22个字段 (starttime)
        int idx = data.lastIndexOf(')');
        if (idx != -1) {
            QStringList parts = data.mid(idx + 2).split(QChar::Space);
            if (parts.size() >= 20) {
                startTime = parts.at(19).toULongLong(); // 第22个字段,索引为19
            }
        }
    }

    // ... 权限验证逻辑 ...

    m_allowedCallers.insert(uniqueName, {uid.value(), pid.value(), startTime});
    saveAllowedCallers();
    return true;
}

// 修改 isAllowedCaller 增加启动时间校验
bool SystemPowerManager::isAllowedCaller(const QString &uniqueName, bool &lookupFailed) const
{
    // ... 前置检查 ...
    const QDBusReply<uint> uid = bus->serviceUid(uniqueName);
    const QDBusReply<uint> pid = bus->servicePid(uniqueName);
    if (!uid.isValid() || !pid.isValid()) {
        lookupFailed = true;
        return false;
    }
    
    // 重新获取当前进程启动时间进行比对
    quint64 currentStartTime = 0;
    QFile statFile(QStringLiteral("/proc/%1/stat").arg(pid.value()));
    if (statFile.open(QIODevice::ReadOnly)) {
        const QString data = QString::fromLatin1(statFile.readAll());
        int idx = data.lastIndexOf(')');
        if (idx != -1) {
            QStringList parts = data.mid(idx + 2).split(QChar::Space);
            if (parts.size() >= 20) {
                currentStartTime = parts.at(19).toULongLong();
            }
        }
    }

    return uid.value() == it->uid && pid.value() == it->pid && currentStartTime == it->startTime;
}

@deepin-ci-robot

Copy link
Copy Markdown

@mhduiy: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
github-pr-review-ci fc7b8ef link true /test github-pr-review-ci

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

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.

2 participants