Enforce caller identity in wh_Auth_BaseUserGet#480
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Tightens authorization for wh_Auth_BaseUserGet by requiring caller identity and enforcing admin/self-only access, preventing user-profile disclosure and account enumeration.
Changes:
- Updated
UserGetbackend + callback/vtable signatures to includecurrent_user_id, and forwarded it fromwh_Auth_UserGet. - Enforced access rules in
wh_Auth_BaseUserGet(admin can read any; non-admin can read only self; non-admin getsWH_ERROR_ACCESSfor denied/missing). - Added a new client-server test (
whTest_AuthUserGet) and registered it in the test list.
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_base.h | Updates wh_Auth_BaseUserGet API and documents new access rules. |
| wolfhsm/wh_auth.h | Extends auth callback vtable UserGet signature and updates docs for caller-gated behavior. |
| src/wh_auth_base.c | Implements authorization checks and non-admin result masking in wh_Auth_BaseUserGet. |
| src/wh_auth.c | Forwards context->user.user_id into the UserGet callback. |
| test-refactor/client-server/wh_test_auth.c | Adds whTest_AuthUserGet covering admin/non-admin access and oracle behavior. |
| test-refactor/wh_test_list.c | Registers the new auth test in the client test suite. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
b62cb9a to
98fd033
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #480
Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src
No new issues found in the changed files. ✅
Problem
wh_Auth_BaseUserGettook no caller identity and performed no authorizationcheck, while its three sibling handlers (
BaseUserDelete,BaseUserSetPermissions,BaseUserSetCredentials) all takecurrent_user_idand gate on
WH_AUTH_IS_ADMIN. Any session holding theAUTHgroup +USER_GETaction bit could read any account's full authorization profile —admin flag, per-group/per-action bitmaps, and the
keyIdsarray — sincewh_Server_HandleAuthRequestflattens it straight to the wire. ReturningWH_ERROR_NOTFOUNDvsWH_ERROR_OKadditionally gave any such caller a freeaccount-enumeration oracle.
Addressed by F-4237.
Fix (
src/wh_auth_base.c)Added
current_user_idto the backend function and thewhAuthCb.UserGetvtable slot;
wh_Auth_UserGetnow forwardscontext->user.user_id.WH_ERROR_ACCESS.WH_ERROR_ACCESSto anon-admin, so the response is not an existence oracle.
user_id == WH_USER_ID_INVALIDguard also closes a latent bug wherean empty username matched a zeroed user slot.
Signature change touches the vtable; the three in-tree
whAuthCbinitializerscompile unchanged. Closes F-4237.
Tests (
test-refactor/client-server/wh_test_auth.c)New
whTest_AuthUserGetcovering: non-admin reads another user (denied, no idleaked), non-admin reads self (allowed, perms verified), non-admin reads unknown
name (denied, same error as above), admin reads any user (allowed, perms
verified), admin reads unknown (
WH_ERROR_NOTFOUND).Verification
whTest_AuthUserGet+ all 9 auth tests pass — 20 passed, 0 failed(
NOCRYPTO=1 AUTH=1).-std=c90 -WerrorforAUTH,AUTH THREADSAFE, and thelegacy
test/harness.assertion fails; post-fix it passes.