Breadway/bos-settings
1
0
Fork 0
Code Issues 4 Pull requests 3 Projects Releases Packages Wiki Activity Actions

fix(users): allow a colon in passwords and say why a change failed #82

Merged
Breadway merged 2 commits from fix/bset-64-password-colon into main 2026-10-14 13:56:06 +08:00
Conversation 2 Commits 2 Files changed 2 +33 -17
Breadway commented 2026-10-14 12:42:10 +08:00
Owner
Copy link

Closes BSET-64

What

  • chpasswd splits each line at the first : only (shadow-utils stpsep), and valid_username already forbids :. So a colon in the password is safe. Rejecting it made such passwords fail with only "The password wasn't changed".
  • Line breaks and NUL are still refused, because they would start a second chpasswd entry. The error now says so, and the same goes for empty and over-long passwords.
  • The change-password sheet shows the backend reason instead of a generic failure.
  • The username hint now lists _ and -, which valid_username allows.

Tests

  • New: colon_in_password_is_one_entry. The existing injection test still rejects \n, \r and \0 in either field and : in the username.
  • On top of current main: cargo test (180), clippy, fmt, svelte-check 0/0, npm test, build.
  • Live BOS VM: changing a user's password to a:b-pass-99 showed "Password changed", and su to that user with a:b-pass-99 succeeded. On main the same change failed.

Capability changes

Passwords containing : are now accepted.

Closes BSET-64 ## What - chpasswd splits each line at the **first** `:` only (shadow-utils `stpsep`), and `valid_username` already forbids `:`. So a colon in the password is safe. Rejecting it made such passwords fail with only "The password wasn't changed". - Line breaks and NUL are still refused, because they would start a second chpasswd entry. The error now says so, and the same goes for empty and over-long passwords. - The change-password sheet shows the backend reason instead of a generic failure. - The username hint now lists `_` and `-`, which `valid_username` allows. ## Tests - New: `colon_in_password_is_one_entry`. The existing injection test still rejects `\n`, `\r` and `\0` in either field and `:` in the username. - On top of current main: cargo test (180), clippy, fmt, svelte-check 0/0, npm test, build. - Live BOS VM: changing a user's password to `a:b-pass-99` showed "Password changed", and `su` to that user with `a:b-pass-99` succeeded. On main the same change failed. ## Capability changes Passwords containing `:` are now accepted.
fix(users): allow a colon in passwords and say why a change failed
All checks were successful
check / check (push) Successful in 6m46s
Details
b6a96ecd1d 
chpasswd splits each line at the first ':' only, and usernames cannot
contain one, so a colon in the password is safe; rejecting it made such
passwords fail with only 'The password wasn't changed'. Line breaks and NUL
are still refused (they would start a second entry), with a message that
says so. The change-password sheet now shows the backend reason, and the
username hint lists _ and - which valid_username allows.

Verified in a BOS VM: changing a user's password to a:b-pass-99 shows
Password changed, and su to that user with a:b-pass-99 succeeds.
Merge remote-tracking branch 'origin/main' into fix/bset-64-password-colon
All checks were successful
check / check (push) Successful in 9m6s
Details
f440b163e1
Breadway commented 2026-10-14 13:54:15 +08:00
Author
Owner
Copy link

Review verdict: PASS (BSET-64). Reviewed exact head f440b163e1, base d432319ac5.

Criteria

  • Colon in password via add_user and change_password: both go through chpasswd_input (src/src/commands/users.rs), which no longer rejects ':'. Test colon_in_password_is_one_entry passes. Coordinator live evidence (BOS VM, a:b-pass-99, su succeeded) accepted as the runtime proof; I did not re-run it.
  • Specific reasons: chpasswd_password_problem returns distinct messages (empty, over 512, line break or NUL); invalid username returns the grammar; change_password failure text explains cancel or refusal; Users.svelte applyPassword now shows the backend error (catch (e)). add_user validates before useradd, so a rejected password does not leave a half-created user.
  • Add sheet hint now reads "Lowercase letters, numbers, _ and -", matching valid_username ([a-z_][a-z0-9_-]*, max 32).

Security (main focus)

  • Input is still exactly format!("{username}:{password}\n"). Rejected before building: \n, \r, NUL in password; empty and over 512 bytes; any username outside the grammar (so no ':' or whitespace or control chars). Existing injection test still asserts \n, \r in both fields and ':' in username; the PR adds NUL and empty-password assertions.
  • shadow-utils src/chpasswd.c (master): per line, stpsep(buf, "\n") strips the line end, then name = buf; newpwd = stpsep(name, ":"). stpsep is strsep(&s, delim) and returns the remainder, so it splits at the FIRST ':' only. The username has no ':' (grammar), so the first ':' is always our separator and everything after it, including further ':', is the password verbatim. No extra entry and no field shift. Lines are only separated by '\n'; we forbid \n, so one input is one entry. fgets_a on a line over BUFSIZ is dropped with an error, and 512 bytes is far below that.
  • PAM path (no -e/-c/-P): do_pam_passwd_non_interactive(Prog, name, newpwd) in lib/pam_pass_non_interactive.c passes name to pam_start and newpwd to the conversation via strdup. Both are already-split C strings and are never re-parsed for ':', so a colon in the password cannot inject or shift anything. The NUL ban matters here because a NUL would silently truncate the C string.
  • util::run_with_stdin writes the single line and drops stdin so EOF is sent.

Checks on the exact head (isolated detached worktree)

  • frontend: npm ci, npm run check (svelte-check 0 errors 0 warnings), npm test, npm run build: all exit 0.
  • src: cargo fmt --check OK; cargo clippy --locked --all-targets -D warnings exit 0; cargo test --locked: 180 passed, 0 failed.
  • Forgejo CI "check / check (push)" run 2484 on f440b163: success.
  • No existing reviews or comments on the PR; mergeable true.

Non-blocking notes: the generic change_password failure message cannot distinguish cancel from refusal because run_with_stdin discards stderr; fine for now.

Review verdict: PASS (BSET-64). Reviewed exact head f440b163e1b0131256aa7b4ff5da97897e332203, base d432319ac559fcc7b29bb7c2df96bd4c98794bfe. Criteria - Colon in password via add_user and change_password: both go through chpasswd_input (src/src/commands/users.rs), which no longer rejects ':'. Test colon_in_password_is_one_entry passes. Coordinator live evidence (BOS VM, a:b-pass-99, su succeeded) accepted as the runtime proof; I did not re-run it. - Specific reasons: chpasswd_password_problem returns distinct messages (empty, over 512, line break or NUL); invalid username returns the grammar; change_password failure text explains cancel or refusal; Users.svelte applyPassword now shows the backend error (catch (e)). add_user validates before useradd, so a rejected password does not leave a half-created user. - Add sheet hint now reads "Lowercase letters, numbers, _ and -", matching valid_username ([a-z_][a-z0-9_-]*, max 32). Security (main focus) - Input is still exactly format!("{username}:{password}\n"). Rejected before building: \n, \r, NUL in password; empty and over 512 bytes; any username outside the grammar (so no ':' or whitespace or control chars). Existing injection test still asserts \n, \r in both fields and ':' in username; the PR adds NUL and empty-password assertions. - shadow-utils src/chpasswd.c (master): per line, stpsep(buf, "\n") strips the line end, then name = buf; newpwd = stpsep(name, ":"). stpsep is strsep(&s, delim) and returns the remainder, so it splits at the FIRST ':' only. The username has no ':' (grammar), so the first ':' is always our separator and everything after it, including further ':', is the password verbatim. No extra entry and no field shift. Lines are only separated by '\n'; we forbid \n, so one input is one entry. fgets_a on a line over BUFSIZ is dropped with an error, and 512 bytes is far below that. - PAM path (no -e/-c/-P): do_pam_passwd_non_interactive(Prog, name, newpwd) in lib/pam_pass_non_interactive.c passes name to pam_start and newpwd to the conversation via strdup. Both are already-split C strings and are never re-parsed for ':', so a colon in the password cannot inject or shift anything. The NUL ban matters here because a NUL would silently truncate the C string. - util::run_with_stdin writes the single line and drops stdin so EOF is sent. Checks on the exact head (isolated detached worktree) - frontend: npm ci, npm run check (svelte-check 0 errors 0 warnings), npm test, npm run build: all exit 0. - src: cargo fmt --check OK; cargo clippy --locked --all-targets -D warnings exit 0; cargo test --locked: 180 passed, 0 failed. - Forgejo CI "check / check (push)" run 2484 on f440b163: success. - No existing reviews or comments on the PR; mergeable true. Non-blocking notes: the generic change_password failure message cannot distinguish cancel from refusal because run_with_stdin discards stderr; fine for now.
Breadway commented 2026-10-14 13:56:17 +08:00
Author
Owner
Copy link

Merged with a merge commit after the base moved (d432319 to 44cf2f1, PR 79). Re-checked the integration of f440b163 on 44cf2f1 in the scratch worktree: clean merge, no src changes from the new base, frontend npm ci, check (0 errors), test and build all pass. Forgejo check on f440b163 was success. Merge SHA 9b07bcff1a. Awaiting coordinator verification of the integrated behavior (BSET-64 not marked done).

Merged with a merge commit after the base moved (d432319 to 44cf2f1, PR 79). Re-checked the integration of f440b163 on 44cf2f1 in the scratch worktree: clean merge, no src changes from the new base, frontend npm ci, check (0 errors), test and build all pass. Forgejo check on f440b163 was success. Merge SHA 9b07bcff1ae3c6484d5be579942c9bdc113b7131. Awaiting coordinator verification of the integrated behavior (BSET-64 not marked done).
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
breadbug

   
bug

   
displays

   
enhancement

   
keybinds

   
network

   
phase-1

   
power

   
privacy

   
recovery

   
search

   
snapshots

   
sound

   
ui

   
ux
No labels
breadbug
bug
displays
enhancement
keybinds
network
phase-1
power
privacy
recovery
search
snapshots
sound
ui
ux
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
Breadway/bos-settings!82
No description provided.