fix(users): harden password hash handling in ModifyPasswd#1120
Conversation
Reviewer's GuideHardened ModifyPasswd by validating password hash input per crypt(5), tightening process/env handling around chpasswd, explicitly zeroing sensitive buffers, and masking backend errors, plus adding a reusable crypt-hash validator. 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 found 1 security issue, and left some high level feedback:
Security issues:
- Detected non-static command inside Write. Audit the input to 'stdin.Write'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code. (link)
General comments:
- Consider making
IsValidCryptHashunexported (e.g.,isValidCryptHash) since it is only used within this file and does not appear to be part of the public API surface. - After calling
cmd.Process.Kill()on a write error, you should still callcmd.Wait()to ensure the process is reaped and avoid leaving a zombie process. - The buffer zeroization logic on
inputis unlikely to meaningfully improve secrecy because the password still exists in other allocations (e.g., the originalwordsstring), so you may want to either strengthen this pattern consistently or remove it to avoid a false sense of security.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider making `IsValidCryptHash` unexported (e.g., `isValidCryptHash`) since it is only used within this file and does not appear to be part of the public API surface.
- After calling `cmd.Process.Kill()` on a write error, you should still call `cmd.Wait()` to ensure the process is reaped and avoid leaving a zombie process.
- The buffer zeroization logic on `input` is unlikely to meaningfully improve secrecy because the password still exists in other allocations (e.g., the original `words` string), so you may want to either strengthen this pattern consistently or remove it to avoid a false sense of security.
## Individual Comments
### Comment 1
<location path="accounts1/users/prop.go" line_range="208" />
<code_context>
_, writeErr := stdin.Write(input)
</code_context>
<issue_to_address>
**security (go.lang.security.audit.dangerous-command-write):** Detected non-static command inside Write. Audit the input to 'stdin.Write'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
ComixHe
force-pushed
the
master
branch
2 times, most recently
from
9e6c625 to
056874a
Compare
|
TAG Bot New tag: 6.1.91 |
ComixHe
force-pushed
the
master
branch
3 times, most recently
from
d184d3e to
d92cc63
Compare
UTsweetyfish
left a comment
UTsweetyfish
left a comment
There was a problem hiding this comment.
不会 golang，但感觉是不是可以直接用正则啊
以及感觉这里可以加点单元测试来 cover
| return errors.New("username cannot consist entirely of hyphens") | ||
| } | ||
|
|
||
| // below check follows BRE: [a-zA-Z0-9_.][a-zA-Z0-9_.-]*$\? |
There was a problem hiding this comment.
可以用正则，这里我是想做的方便提供诊断信息，正则没法知道为什么失配（
There was a problem hiding this comment.
测试用例正在加，我单独提一个test的commit~
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
これ |
ComixHe
force-pushed
the
master
branch
2 times, most recently
from
429bf52 to
c68b6da
Compare
| return nil | ||
| } | ||
|
|
||
| func ModifyPasswd(words, username string) error { |
There was a problem hiding this comment.
这里有必要加个注释，说明调用者吗？
还是该在 D-Bus 那边加？还是不加（？
There was a problem hiding this comment.
如果要加的话，感觉在dbus那边加好一点？因为我感觉这个ModifyPasswd没暴露到dbus上所以不用(
这个ai出现幻觉了。 在golang的fuzz的Command line output写了：
failing-input里有一个定义是：
所以这里可以直接fatal |
|
lgtm，至少我看不出问题了 |
Validate password crypt hashes and username before invoking chpasswd to reject invalid characters and malformed input per crypt(5) and useradd's username validation rules. Also improve process isolation and sensitive data handling by clearing the child environment, switching to an explicit stdin pipe flow. Signed-off-by: Yuming He <heyuming@deepin.org>
…tHash Cover all 16 crypt(5) hash methods, forbidden delimiter positions, and shadow-utils username rules with table-driven and fuzz tests. Signed-off-by: Yuming He <heyuming@deepin.org>
deepin pr auto review你好！我是CodeGeeX。我已经仔细审查了你提供的Git Diff。这次代码变更主要对用户名和密码哈希的修改逻辑进行了重构，引入了严格的输入验证、环境变量清理以及更安全的进程间通信（IPC）方式，整体上极大地提升了代码的安全性和健壮性。 以下是我对语法逻辑、代码质量、代码性能和代码安全四个方面的详细审查意见和改进建议： 一、 语法与逻辑
二、 代码质量
三、 代码性能
四、 代码安全
改进后的代码建议针对以上审查，我为你优化了 // prop.go 中的常量与校验函数改进
const (
asciiPrintableMin = 32
asciiPrintableMax = 126
)
// isValidCryptHash validates the format of a crypt password hash string.
func isValidCryptHash(hash string) error {
if hash == "" {
return errors.New("password hash is empty")
}
for i := 0; i < len(hash); i++ {
b := hash[i]
if b < asciiPrintableMin || b > asciiPrintableMax {
return errors.New("password hash contains non-printable ASCII characters")
}
switch b {
case ' ', ':', ';', '*', '!', '\\':
return errors.New("password hash contains forbidden characters")
}
}
return nil
}
// isValidUsername validates the input username.
func isValidUsername(name string) error {
if name == "" || name == "." || name == ".." {
return errors.New("username can't be '.' or '..' or empty")
}
if len(name) > LoginNameMaxSize() {
return errors.New("username too long")
}
// 检查首字符，shadow 规范不允许以 '-' 开头
first := name[0]
isFirstValid := (first >= 'a' && first <= 'z') ||
(first >= 'A' && first <= 'Z') ||
(first >= '0' && first <= '9') ||
first == '_' ||
first == '.'
if !isFirstValid {
return errors.New("first character must be alphanumeric, underscore, or dot")
}
isAllDigit := (first >= '0' && first <= '9')
for i := 1; i < len(name); i++ {
ch := name[i]
if ch >= '0' && ch <= '9' {
// isAllDigit 可能在此处保持为 true
} else {
isAllDigit = false
}
isValidChar := (ch >= 'a' && ch <= 'z') ||
(ch >= 'A' && ch <= 'Z') ||
(ch >= '0' && ch <= '9') ||
ch == '_' ||
ch == '.' ||
ch == '-'
if isValidChar {
continue
}
if ch == '$' && i == len(name)-1 {
continue
}
return errors.New("username contains invalid characters or '$' is not at the end")
}
if isAllDigit {
return errors.New("username cannot consist entirely of digits")
}
return nil
}
func ModifyPasswd(words, username string) error {
if words == "" || username == "" {
return errors.New("password hash or username is empty")
}
if err := isValidUsername(username); err != nil {
return fmt.Errorf("username is invalid: %w", err)
}
if err := isValidCryptHash(words); err != nil {
return fmt.Errorf("invalid password hash: %w", err)
}
cmd := exec.Command(pwdCmdModify, "-e")
cmd.Env = []string{}
stdin, err := cmd.StdinPipe()
if err != nil {
return fmt.Errorf("failed to create stdin pipe: %w", err)
}
var stderr bytes.Buffer
cmd.Stderr = &stderr
if err := cmd.Start(); err != nil {
return fmt.Errorf("failed to start command: %w", err)
}
// 使用更轻量的 []byte 拼接
input := append([]byte(username), ':')
input = append(input, words...)
input = append(input, '\n')
_, writeErr := stdin.Write(input)
// 安全考虑：清零内存中的密码痕迹
for i := range input {
input[i] = 0
}
stdin.Close()
if writeErr != nil {
_ = cmd.Process.Kill()
_ = cmd.Wait()
return fmt.Errorf("failed to write to stdin: %w", writeErr)
}
if err := cmd.Wait(); err != nil {
return fmt.Errorf("failed to update system password configuration: %s", stderr.String())
}
return nil
}总结：这次代码变更质量很高，安全意识很强，特别是在输入验证和环境清理方面做得很专业。修复上述几个小逻辑和性能细节后，代码将更加健壮可靠。 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ComixHe, fly602, UTsweetyfish 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 |
4 participants
Validate password crypt hashes and username before invoking chpasswd to reject invalid characters and malformed input per crypt(5) and useradd's username validation rules.
Also improve process isolation and sensitive data handling by clearing the child environment, switching to an explicit stdin pipe flow, and zeroing the temporary password buffer after use.
Additionally, avoid exposing detailed backend errors to callers to reduce information disclosure risks.
Summary by Sourcery
Harden password change handling by validating crypt hashes, isolating the chpasswd subprocess environment, securely piping credentials, and reducing error detail exposure.
New Features:
Enhancements: