fix: disable SetPassword DBus method and prevent chpasswd injection#1119
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideEnforces administrator authorization for DBus SetPassword calls and hardens password modification against command injection via chpasswd by rejecting unsafe password hash inputs. Sequence diagram for DBus SetPassword with enforced polkit authorizationsequenceDiagram
actor User
participant DBus
participant UserService as User
participant Polkit
User ->> DBus: SetPassword
DBus ->> UserService: SetPassword(sender, password)
UserService ->> Polkit: checkAuth(sender, false, polkitActionUserAdministration)
alt authorized
Polkit -->> UserService: ok
UserService -->> DBus: success
DBus -->> User: success
else denied
Polkit -->> UserService: error
UserService -->> DBus: dbusutil.ToError(err)
DBus -->> User: access denied
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}
| return errInvalidParam | ||
| } | ||
| // 防止命令注入 | ||
| if strings.ContainsAny(words, "\n\r:") { |
There was a problem hiding this comment.
Hashed passphrases are always entirely printable ASCII, and do not contain any whitespace or the characters ‘:’, ‘;’, ‘*’, ‘!’, or ‘\’. (These characters are used as delimiters and special markers in the passwd(5) and shadow(5) files.)
https://manpages.debian.org/unstable/libcrypt-dev/crypt.5.en.html
caixr23
force-pushed
the
master
branch
2 times, most recently
from
9c4ea61 to
7496abf
Compare
| return errInvalidParam | ||
| } | ||
| // 防止命令注入 | ||
| if strings.ContainsAny(words, "\n\r") { |
There was a problem hiding this comment.
| if strings.ContainsAny(words, "\n\r") { | |
| if strings.ContainsAny(words, "\n\r :;*!\\") { |
我也不太确定
There was a problem hiding this comment.
please mention GHSA-rj6j-h4mp-mcxj.
There was a problem hiding this comment.
这里是加密后的密码
是的，加密后的密码不应有空格、冒号、分号等。
There was a problem hiding this comment.
这里是加密后的密码
是的，加密后的密码不应有空格、冒号、分号等。
用户名:加密后的密码:密码最后修改日期:最小密码年龄:最大密码年龄:密码过期之前提前警告天数:密码过期后仍然接受此密码的天数:过期时间:保留
| return errInvalidParam | ||
| } | ||
| // 防止命令注入 | ||
| if strings.ContainsAny(words, "\n\r") { |
There was a problem hiding this comment.
please mention GHSA-rj6j-h4mp-mcxj.
|
|
||
| err := u.checkAuth(sender, false, "") | ||
| // 修改密码，一律需要鉴权管理员 | ||
| err := u.checkAuth(sender, false, polkitActionUserAdministration) |
这个有必要吗 |
caixr23
changed the title
用户名不是外部传入的 |
1. SetPassword now returns error directly, marking the DBus method as deprecated and no longer supported 2. ModifyPasswd rejects password hashes containing \n\r: to prevent chpasswd stdin injection fix: 禁用 SetPassword DBus 方法并防止 chpasswd 注入 1. SetPassword 直接返回错误，标记该 DBus 方法为已废弃 2. ModifyPasswd 拒绝包含 \n\r: 的密码哈希， 防止通过 chpasswd stdin 注入额外记录
deepin pr auto review这份 Git Diff 主要做了两件事：
以下是对该 Diff 的详细代码审查，涵盖语法逻辑、代码质量、代码性能和代码安全四个方面： 一、 语法与逻辑
二、 代码质量
三、 代码性能
四、 代码安全 🚨 (重点)
总结这个 Diff 整体方向正确：废弃了存在性能隐患（轮询）的旧接口，并在底层函数增加了安全校验。 必须修改项：
建议修改项：
|
| if strings.ContainsAny(words, "\n\r") || strings.ContainsAny(username, "\n\r:") { | ||
| return errInvalidParam | ||
| } |
There was a problem hiding this comment.
Hashed passphrases are always entirely printable ASCII, and do not contain any whitespace or the characters ‘:’, ‘;’, ‘*’, ‘!’, or ‘\’. (These characters are used as delimiters and special markers in the passwd(5) and shadow(5) files.)
这里说的就是哈希后的密码不应包含冒号等
https://manpages.debian.org/unstable/libcrypt-dev/crypt.5.en.html
There was a problem hiding this comment.
使用更严格的白名单校验：对于 username，应该只允许字母、数字、下划线和连字符。对于 words（密码），虽然允许的字符较多，但仍建议过滤掉所有控制字符（ASCII < 32）。
我也建议用白名单校验，
先只允许 printable ASCII，再禁掉 :;*!\
There was a problem hiding this comment.
words是加密后的密码，\n已经能防注入了，先保证不影响原有功能
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: caixr23, mhduiy 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 |
|
dde-control-center 创建新用户还在用 SetPassword 这个接口，现在创建出的新用户是没有密码的 |
|
4 participants
method as deprecated and no longer supported
to prevent chpasswd stdin injection
fix: 禁用 SetPassword DBus 方法并防止 chpasswd 注入
防止通过 chpasswd stdin 注入额外记录
Summary by Sourcery
Enforce proper authorization and input validation for password changes to close security gaps.
Bug Fixes: