refactor(accounts): disable UnionID password setting due to security check race condition#1045
Merged
ComixHe merged 1 commit intolinuxdeepin:masterfrom Mar 5, 2026
Merged
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR temporarily disables the UnionID-based password setting path due to a race-condition vulnerability in caller verification, and documents the underlying issue in the caller identification logic. Sequence diagram for updated SetPassword UnionID handlingsequenceDiagram
participant Client
participant DbusSender as Dbus_Sender
participant User as User_SetPassword
Client->>User: SetPassword(sender, password)
User->>User: check if password is empty
alt password_is_empty
Note over User: UnionID-based password setting<br/>path is temporarily disabled
User-->>Client: return nil (no password change)
else password_is_non_empty
User->>User: checkAuth(sender, false, "")
alt auth_success
User->>User: setPasswordWithProvidedPassword
User-->>Client: return nil
else auth_failure
User-->>Client: return dbus.Error
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Silently returning nil when
password == ""changes behavior in a way that might be hard to detect; consider returning a specific error or at least logging that UnionID-based password setting is disabled so callers can react appropriately. - Since the
newCallerexecutable-path check is known to be unsafe and marked FIXME, it may be safer to short-circuit callers that rely on it (or guard it behind a feature flag) to avoid accidental future reuse while the race condition remains unresolved.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Silently returning nil when `password == ""` changes behavior in a way that might be hard to detect; consider returning a specific error or at least logging that UnionID-based password setting is disabled so callers can react appropriately.
- Since the `newCaller` executable-path check is known to be unsafe and marked FIXME, it may be safer to short-circuit callers that rely on it (or guard it behind a feature flag) to avoid accidental future reuse while the race condition remains unresolved.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
fly602
reviewed
Mar 5, 2026
fly602
previously approved these changes
Mar 5, 2026
…check race condition The newCaller() function uses /proc/<pid>/exe to verify the caller is running a trusted binary, but this check has a race condition and can be faked by the caller. Since there is no quick fix and this feature is not currently needed, temporarily disable the UnionID password setting functionality. Pms: BUG-349987 Signed-off-by: ComixHe <heyuming@deepin.org>
deepin pr auto review经过对提供的 Git diff 的审查，这段代码主要涉及了 DBus 服务端的调用者验证逻辑修改。以下是针对语法逻辑、代码质量、代码性能和代码安全的详细审查意见及改进建议： 1. 代码逻辑与安全性审查主要问题：安全验证被移除 在
改进建议：
2. 代码质量审查
3. 代码性能审查
4. 具体修改建议代码为了修复上述安全问题，建议不要直接注释掉验证逻辑，而是采用基于 PID 的安全检查（例如检查 PID 是否属于特定的系统用户或具有特定安全标签）。以下是一个基于现有代码结构的改进示例，假设我们决定暂时保留路径检查但增强错误处理，或者建议引入更严格的检查： 建议方案 A（如果必须移除路径检查，应至少保留 UID 检查作为临时防线）： // accounts1/user_chpwd_union_id.go
// ... 获取 proc ...
// 注释掉路径检查，但建议增加 UID 检查
// var app string
// switch exe { ... }
// 新增：检查调用者是否为 root 或特定系统用户
uid, err := proc.Uid()
if err != nil {
err = fmt.Errorf("get sender uid error: %v", err)
return
}
// 假设只有 root (uid 0) 或特定系统用户有权调用
// 这比完全不做检查要好，但不如 LSM 安全
if uid != 0 {
err = fmt.Errorf("set password with Union ID called by uid %d, permission denied", uid)
return
}
// ... 后续逻辑 ...
ret = &caller{
// ...
// 暂时标记为 system 或 root，而不是 unknown
app: "system",
// ...
}建议方案 B（针对 user_ifc.go 的逻辑恢复）： // accounts1/user_ifc.go
func (u *User) SetPassword(sender dbus.Sender, password string) *dbus.Error {
// ... 前置检查 ...
// set password from UnionID
if password == "" {
// 即使 newCaller 做了放宽，这里也应确保逻辑完整
// 如果 setPwdWithUnionID 确实不再需要，请删除该函数定义
// 否则应该保留调用，除非有明确的业务逻辑变更说明
logger.Warning("SetPassword with empty password called, triggering UnionID flow")
err := u.setPwdWithUnionID(sender)
if err != nil {
return dbusutil.ToError(err)
}
return nil
}
// ... 正常密码设置逻辑 ...
}总结这段代码修改目前处于不安全且逻辑不完整的状态。
|
fly602
approved these changes
Mar 5, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ComixHe, fly602 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The newCaller() function uses /proc//exe to verify the caller is running a trusted binary, but this check has a race condition and can be faked by the caller. Since there is no quick fix and this feature is not currently needed, temporarily disable the UnionID password setting functionality.
Pms: BUG-349987
Summary by Sourcery
Temporarily disable setting passwords via UnionID due to an insecure caller verification mechanism and document the executable-path-based caller check as problematic.
Bug Fixes:
Enhancements: