Skip to content

Fix stale session permissions after wh_Auth_UserSetPermissions - #481

Open
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:mainfrom
yosuke-wolfssl:fix/f_4239
Open

Fix stale session permissions after wh_Auth_UserSetPermissions#481
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:mainfrom
yosuke-wolfssl:fix/f_4239

Conversation

@yosuke-wolfssl

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

Copy link
Copy Markdown
Contributor

Problem

wh_Auth_UserSetPermissions wrote the new permissions to the backend but never
refreshed context->user.permissions, the per-session cache that
wh_Auth_CheckRequestAuthorization actually reads. A logged-in user that
lowered its own permissions kept the old, higher ones for the rest of the
session — revocation did not bind until the next login. Reachable from any
client through wh_Client_AuthUserSetPermissions. Closes f-4239.

Refreshing that cache opens a second hole on its own: the cache mirrors whatever
the caller supplied, so a non-admin session against a permissive backend could
cache itself as admin. Both are fixed together.

Fix (src/wh_auth.c)

  • Session cache refresh — on backend success, when the target user_id is
    this context's logged-in user, context->user.permissions is updated, so the
    change binds on the very next request.
  • Absolute admin-flag gate — a non-admin session supplying permissions that
    carry the admin flag is refused with WH_AUTH_PERMISSION_ERROR before the
    backend is consulted. The check is absolute, not a promotion check: the core
    is handed only the target's whUserId and cannot read the target's current
    flag, so it refuses even a request that would merely preserve an existing
    admin.

wolfhsm/wh_auth.h documents both on the API; docs/src/5-Features.md adds a
"Permission Changes and Live Sessions" section covering the immediate self-demote
(which can be self-locking) and the fact that other sessions keep their cached
permissions until they log in again.

Tests (test-refactor/client-server/wh_test_auth.c)

Both halves are now driven entirely through wh_Client_* against the running
POSIX server and the real wh_Auth_Base* backend, appended to the existing
whTest_AuthSetPermissions. No new file, no new entry point, no
wh_test_list.c or README.md change.

  • Self-demote binds the live session — admin clears its own USER_ADD bit
    (keeping admin + USER_SET_PERMISSIONS so it can restore), and its own
    wh_Client_AuthUserAdd must now be denied. A change targeting a different
    user leaves the caller's session untouched.
  • Non-admin cannot grant admin — a user holding only
    USER_SET_PERMISSIONS logs in and issues three calls. The two gates are told
    apart by their return codes:
Call from the non-admin session Result
permissions without the admin flag WH_ERROR_ACCESS — reached the backend, refused there
same call with the admin flag, self target WH_AUTH_PERMISSION_ERROR — refused in the core
same, targeting another user WH_AUTH_PERMISSION_ERROR — refusal is target-independent

Both cases capture their outcomes, restore the admin session and drop the users
they created before asserting, so a failing assert cannot leave the shared
POSIX server's admin baseline demoted or its 5-slot user table full.

The earlier mock whAuthCb (263 lines) is dropped — the real backend is
reachable, so it was testing the harness. Four of its cases have no client
equivalent and are not covered: backend-failure-leaves-cache-unchanged (the base
backend does not fail for a valid admin request), verbatim mirroring of an
unclamped keyIdCount (only visible by reading ctx.user.permissions), and
admin-grants-admin / admin-self-lockout (running those against the shared server
would permanently demote the admin baseline for every later test — documented in
5-Features.md instead).

Note that against the base backend the admin-flag gate is defense in depth:
wh_Auth_BaseUserSetPermissions is admin-only anyway, which is exactly what the
control case pins down. The gate binds for backends that permit non-admin
permission edits.

Verification

Merge base 237bfbc. Clean under -std=c90 -Werror -Wall -Wextra, zero warnings.

Config Result
test-refactor AUTH=1 53 passed / 18 skipped / 0 failed of 71
test-refactor default 45/26/0
test-refactor DMA=1 49/22/0
test-refactor SHE=1 51/20/0

Negative controls, each reverted and restored:

  • Removing the cache refresh → the self-demote case fails; the demoted admin
    still adds users.
  • Removing the admin-flag gate → the escalation case fails, reporting
    WH_ERROR_ACCESS where WH_AUTH_PERMISSION_ERROR is required.

@yosuke-wolfssl yosuke-wolfssl self-assigned this Jul 23, 2026
Copilot AI review requested due to automatic review settings July 23, 2026 01:58

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 fixes an authorization correctness issue where wh_Auth_UserSetPermissions() updated the backend user record but did not refresh the calling session’s cached permissions, allowing revoked privileges to persist until logout. It also documents and tests the new “bind immediately” behavior and introduces a core-level guard preventing non-admin sessions from setting the admin flag.

Changes:

  • Update wh_Auth_UserSetPermissions() to refresh context->user.permissions when a logged-in user changes its own permissions successfully.
  • Add a core-enforced policy: non-admin sessions cannot set the admin flag via wh_Auth_UserSetPermissions(), independent of backend policy.
  • Add unit + integration tests to verify immediate self-demotion effects and cache-sync behavior.

Reviewed changes

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

Show a summary per file
File Description
wolfhsm/wh_auth.h Documents session-cache refresh behavior and the new admin-flag refusal semantics for wh_Auth_UserSetPermissions().
src/wh_auth.c Implements admin-flag refusal for non-admin sessions and refreshes the live session permission cache on successful self-targeted updates.
test-refactor/client-server/wh_test_auth.c Adds an end-to-end self-demotion test and a core-only mock-backend unit test for cache synchronization behavior.
test-refactor/wh_test_list.c Registers the new whTest_AuthSetPermsCacheSync test.
test-refactor/README.md Updates test inventory documentation to include the new auth cache-sync test.
docs/src/5-Features.md Documents core vs backend responsibility for permission policy and explains live-session binding semantics and consequences.

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

Comment thread wolfhsm/wh_auth.h Outdated
Comment thread docs/src/5-Features.md Outdated
Comment thread test-refactor/client-server/wh_test_auth.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 #481

Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src

No new issues found in the changed files. ✅

@Frauschi Frauschi self-assigned this Jul 23, 2026
Frauschi
Frauschi previously approved these changes Jul 23, 2026

@Frauschi Frauschi 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.

🐺 Skoll Code Review

Overall recommendation: APPROVE
Findings: 2 total — 1 posted, 1 skipped

Posted findings

  • [Info] Session permission cache mirrors caller-supplied values, not backend-persisted statesrc/wh_auth.c:483-491
Skipped findings
  • [Low] Session cache mirrors supplied permissions verbatim; diverges if a custom backend transforms them

Review generated by Skoll via Claude/Codex

Comment thread src/wh_auth.c
@Frauschi Frauschi removed their assignment Jul 23, 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.

5 participants