Skip to content

wolfsshd: make the supplementary-group drop reliable and tested#1085

Open
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/f_6701
Open

wolfsshd: make the supplementary-group drop reliable and tested#1085
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/f_6701

Conversation

@yosuke-wolfssl

@yosuke-wolfssl yosuke-wolfssl commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Summary

wolfSSHD_AuthSetGroups performs the daemon's supplementary-group drop — it resolves the target user's full group list and applies it with setgroups() before wolfSSHD_AuthReducePermissionsUser reduces uid/gid. This PR started as mutation-coverage for that drop (finding f_6701) and, in the process, surfaced and fixes a real privilege-retention bug on macOS/BSD.

Motivation

Coverage gap (f_6701). Deleting the setgroups() call, or defeating its == -1 failure guard, survived the entire test suite. Unlike the setregid/setreuid path (baseline 5578), there was no WOLFSSHD_UNIT_TEST hook for setgroups, so no test observed whether it ran. With the call gone, the spawned user session retains the wolfsshd (root) process's supplementary group memberships — a concrete privilege-retention path.

Bug found while writing the test. wolfSSHD_AuthSetGroups gated the whole setgroups() block on a NULL-size getgrouplist() probe returning -1. That probe is unreliable: on macOS/BSD it returns success with size 0, so the block was skipped and the supplementary-group drop silently never ran on those platforms. The file already documents this exact unreliability and solves it with grow-and-retry in wolfSSHD_GetUserGroupNames.

Changes

  • Test hooks — add WOLFSSHD_UNIT_TEST callbacks wsshd_setgroups_cb and wsshd_getgrouplist_cb, alongside the existing wsshd_setregid_cb/wsshd_setreuid_cb. Each default forwards to the real syscall through a one-line adapter, because setgroups()/getgrouplist() prototypes differ across platforms (size_t vs int, gid_t* vs int*) and a single portable function-pointer type can't bind to them directly.
  • Bug fix / dedup — extract the grow-and-retry group-list sizing already used by wolfSSHD_GetUserGroupNames into a shared wolfSSHD_GetUserGroupList helper, and call it from both GetUserGroupNames and AuthSetGroups. The list is now resolved through one portable path and setgroups() runs on every platform.
  • Guard consistency — standardize the wolfsshd privilege/group functions on the _WIN32 guard macro (matching the auth.h externs and the file's majority convention), so callback definitions and their references share one Windows guard and no _WIN32 && !WIN32 build can compile a caller without its definition.

Tests

The tests mock getgrouplist and setgroups, making them deterministic on every platform (the real macOS NULL-size probe would otherwise skip the drop entirely on the dev machine):

  • test_AuthSetGroups_ok — asserts setgroups() is invoked with the resolved group count and the exact forwarded gids, after at least one buffer grow-and-retry (the test build forces -DWOLFSSHD_GROUP_LIST_INIT=1).
  • test_AuthSetGroups_setgroups_fail — a setgroups() failure must propagate WS_FATAL_ERROR.
  • test_AuthSetGroups_getgrouplist_fail — a lookup that never succeeds must fail closed and never reach setgroups().

Verification

Bug demonstrated & fixed (real, unmocked, on macOS):

Path Result
Old NULL-size probe ret=0 sz=0setgroups SKIPPED
New grow-and-retry resolves 17 groupssetgroups RUNS

Mutation kills — each defect is caught by exactly the corresponding test (others still pass):

Mutation Killed by
Delete the setgroups() call test_AuthSetGroups_ok
Defeat the setgroups() == -1 guard test_AuthSetGroups_setgroups_fail
Drop the resolve-failure propagation test_AuthSetGroups_getgrouplist_fail
  • Full wolfsshd unit suite: 103 passed, 0 failures; normal build clean, no new warnings.
  • Clean under ASan + UBSan (-fsanitize=address,undefined; LeakSanitizer disabled on Darwin).

Notes

  • wolfSSHD_GetUserGroupNames behavior is unchanged (it already used this exact sizing); it now shares the helper instead of duplicating it.
  • Windows and QNX paths are preserved (the stub returns 0 on success under QNX to match the getgrouplist contract).
  • No production control flow changes in non-WOLFSSHD_UNIT_TEST builds beyond the sizing fix itself.

@yosuke-wolfssl yosuke-wolfssl self-assigned this Jul 8, 2026
Copilot AI review requested due to automatic review settings July 8, 2026 02:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds unit-test indirection hooks around getgrouplist() and setgroups() in wolfSSHD_AuthSetGroups, plus new unit tests to ensure the supplementary-group drop is executed and failures are handled, improving mutation coverage and reducing risk of privilege retention regressions.

Changes:

  • Add WOLFSSHD_UNIT_TEST callback hooks for setgroups() and getgrouplist() and route wolfSSHD_AuthSetGroups through them when unit-testing.
  • Add deterministic unit tests for the supplementary-group drop success path and for getgrouplist/setgroups failure handling.
  • Standardize Windows guards in the touched area to _WIN32 for consistency with existing conventions in these files.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
apps/wolfsshd/auth.c Adds default adapters and unit-test callback routing for getgrouplist/setgroups inside wolfSSHD_AuthSetGroups.
apps/wolfsshd/auth.h Exposes new unit-test callback externs for setgroups/getgrouplist under WOLFSSHD_UNIT_TEST on non-Windows builds.
apps/wolfsshd/test/test_configuration.c Introduces group-resolution and setgroups stubs plus three new unit tests covering the supplementary-group drop behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread apps/wolfsshd/auth.c Outdated
Comment thread apps/wolfsshd/test/test_configuration.c Outdated

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fenrir Automated Review — PR #1085

Scan targets checked: wolfssh-bugs, wolfssh-src

No new issues found in the changed files. ✅

@yosuke-wolfssl yosuke-wolfssl changed the title wolfsshd: unit-test wolfSSHD_AuthSetGroups supplementary-group drop wolfsshd: make the supplementary-group drop reliable and tested Jul 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants