fix: prevent password leakage in ModifyPasswd#1044
Merged
ComixHe merged 1 commit intolinuxdeepin:masterfrom Mar 4, 2026
Merged
fix: prevent password leakage in ModifyPasswd#1044ComixHe merged 1 commit intolinuxdeepin:masterfrom
ComixHe merged 1 commit intolinuxdeepin:masterfrom
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideModifyPasswd no longer invokes Sequence diagram for ModifyPasswd using chpasswd with stdinsequenceDiagram
actor Admin
participant UsersPackage as users_package
participant Exec as os_exec
participant Chpasswd as chpasswd_process
Admin->>UsersPackage: ModifyPasswd(words, username)
UsersPackage->>UsersPackage: validate words
alt invalid_parameters
UsersPackage-->>Admin: errInvalidParam
else valid_parameters
UsersPackage->>Exec: Command(pwdCmdModify, "-e")
Exec-->>UsersPackage: *exec.Cmd
UsersPackage->>UsersPackage: format input "username:words\n"
UsersPackage->>Chpasswd: set Stdin to input buffer
UsersPackage->>Chpasswd: set Stderr to stderr buffer
UsersPackage->>Chpasswd: Run()
alt chpasswd_error
Chpasswd-->>UsersPackage: error
UsersPackage-->>Admin: fmt.Errorf("failed to modify password: ...")
else chpasswd_success
Chpasswd-->>UsersPackage: nil
UsersPackage-->>Admin: nil
end
end
Class diagram for updated users package password modificationclassDiagram
class UsersPackage {
<<package>>
}
class Constants {
<<constant_group>>
string userCmdAdd
string userCmdDelete
string userCmdModify
string userCmdGroup
string pwdCmdModify
string cmdGroupDel
string cmdChAge
}
class Functions {
error ModifyPasswd(words string, username string)
error ModifyMaxPasswordAge(username string, nDays int)
}
UsersPackage --> Constants
UsersPackage --> Functions
class ModifyPasswdFlow {
- validateParameters(words string, username string) error
- runChpasswd(username string, words string) error
}
Functions --> ModifyPasswdFlow
ModifyPasswdFlow ..> pwdCmdModify : uses
ModifyPasswdFlow ..> execCmd : uses
class execCmd {
<<external>>
+Command(name string, arg1 string) *Cmd
+Run() error
+Stdin ioReader
+Stderr ioWriter
}
class ioReader {
<<interface>>
}
class ioWriter {
<<interface>>
}
ModifyPasswdFlow ..> ioReader
ModifyPasswdFlow ..> ioWriter
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, 1 other issue, and left some high level feedback:
Security issues:
- Detected non-static command inside Command. Audit the input to 'exec.Command'. 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 reusing the existing
doActionhelper (or extending it) rather than invokingexec.Commanddirectly here, so that process execution and error handling remain consistent with the rest of the user management commands. - When wrapping the error from
cmd.Run, use%winfmt.Errorf(e.g.fmt.Errorf("failed to modify password: %w, %s", err, stderr.String())) so callers can unwrap and inspect the underlying error if needed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider reusing the existing `doAction` helper (or extending it) rather than invoking `exec.Command` directly here, so that process execution and error handling remain consistent with the rest of the user management commands.
- When wrapping the error from `cmd.Run`, use `%w` in `fmt.Errorf` (e.g. `fmt.Errorf("failed to modify password: %w, %s", err, stderr.String())`) so callers can unwrap and inspect the underlying error if needed.
## Individual Comments
### Comment 1
<location path="accounts1/users/prop.go" line_range="190" />
<code_context>
+ var stderr bytes.Buffer
+ cmd.Stderr = &stderr
+
+ err := cmd.Run()
+ if err != nil {
+ return fmt.Errorf("failed to modify password: %v, %s", err, stderr.String())
</code_context>
<issue_to_address>
**suggestion:** Use error wrapping and consider formatting stderr conditionally to improve error reporting.
Currently the error is built with `fmt.Errorf("failed to modify password: %v, %s", err, stderr.String())`. Two suggestions:
1) Use `%w` so callers can unwrap the original error: `fmt.Errorf("failed to modify password: %w", err)`.
2) Include stderr only when non-empty, and/or with a clear prefix (e.g. `"; stderr=%s"`) to avoid odd messages (like trailing comma/space) while keeping useful diagnostics.
Suggested implementation:
```golang
err := cmd.Run()
if err != nil {
msg := "failed to modify password"
if s := strings.TrimSpace(stderr.String()); s != "" {
msg = fmt.Sprintf("%s; stderr=%s", msg, s)
}
return fmt.Errorf("%s: %w", msg, err)
}
```
You will also need to ensure that the file imports the `strings` package:
1. Add `strings` to the import block, e.g. `import ("bytes"; "fmt"; "os/exec"; "strings")` or similar, respecting the existing import style and order.
</issue_to_address>
### Comment 2
<location path="accounts1/users/prop.go" line_range="183" />
<code_context>
cmd := exec.Command(pwdCmdModify, "-e")
</code_context>
<issue_to_address>
**security (go.lang.security.audit.dangerous-exec-command):** Detected non-static command inside Command. Audit the input to 'exec.Command'. 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.
Replace usermod -p with chpasswd -e and pass password via stdin instead of command line arguments to avoid exposing passwords in process listings. Signed-off-by: ComixHe <heyuming@deepin.org>
deepin pr auto review这段代码主要修改了 1. 语法逻辑
2. 代码质量
3. 代码性能
4. 代码安全 —— 非常重要
改进建议代码示例假设 import (
"bytes"
"fmt"
"os/exec"
"regexp" // 用于用户名校验
"strings"
)
// ... (常量定义保持不变)
func ModifyPasswd(words, username string) error {
// 1. 参数校验
if words == "" || username == "" {
return errInvalidParam
}
// 2. 用户名格式校验，防止注入或非法字符
// 常见的 Linux 用户名规则：小写字母、数字、下划线、短横线，不以短横线开头
matched, _ := regexp.MatchString(`^[a-z_][a-z0-9_-]*$`, username)
if !matched {
return fmt.Errorf("invalid username format: %s", username)
}
// 3. 构建命令
// 注意：移除了 -e，假设 words 是明文。
// 如果需要指定加密算法，可以添加参数，例如：[]string{"-c", "SHA512"}
cmd := exec.Command(pwdCmdModify)
// 4. 准备输入
// 使用 strings.NewReader 替代 bytes.NewBufferString，语义更清晰（只读）
input := fmt.Sprintf("%s:%s", username, words)
cmd.Stdin = strings.NewReader(input)
// 5. 捕获输出
var stderr bytes.Buffer
cmd.Stderr = &stderr
// 6. 执行
err := cmd.Run()
if err != nil {
// 返回详细的错误信息，注意不要泄露 words
return fmt.Errorf("failed to modify password for user %s: %v, %s", username, err, stderr.String())
}
return nil
}总结这段代码在安全性上相比直接使用 |
fly602
approved these changes
Mar 4, 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.
Replace usermod -p with chpasswd -e and pass password via stdin instead of command line arguments to avoid exposing passwords in process listings.
Summary by Sourcery
Bug Fixes: