From b7503bcef4eae1e34f1cc6fba8f319de15594f33 Mon Sep 17 00:00:00 2001 From: JacobBarthelmeh Date: Thu, 28 May 2026 16:14:18 -0600 Subject: [PATCH] add restriction on users ability to set credentials --- docs/src/5-Features.md | 2 ++ src/wh_auth.c | 5 +-- src/wh_auth_base.c | 16 ++++++++-- test/wh_test_auth.c | 72 ++++++++++++++++++++++++++++++++++++++++++ wolfhsm/wh_auth.h | 4 +-- wolfhsm/wh_auth_base.h | 9 ++++-- 6 files changed, 100 insertions(+), 8 deletions(-) diff --git a/docs/src/5-Features.md b/docs/src/5-Features.md index 6e06d1d1c..e449d92de 100644 --- a/docs/src/5-Features.md +++ b/docs/src/5-Features.md @@ -1075,6 +1075,8 @@ The helper macros `WH_AUTH_SET_ALLOWED_GROUP`, `WH_AUTH_SET_ALLOWED_ACTION`, `WH `whAuthPermissions` also carries a small per-user `keyIds` allowlist and the data model includes a `CheckKeyAuthorization` callback intended to constrain which keys a user may exercise. Per-key authorization is a placeholder in the current implementation — the callback is defined but no crypto or key handler invokes it yet. +Holding an action bit lets a session issue a request, but the backend still enforces per-target authorization on top of it. In the default base backend, `USER_SET_CREDENTIALS` lets a non-admin update its **own** credentials only (a cross-user credential change additionally requires admin and otherwise fails with `WH_ERROR_ACCESS`), while `USER_DELETE` and `USER_SET_PERMISSIONS` remain admin-only regardless of the action bit. The caller's `whUserId` is passed to the backend as `current_user_id` so it can distinguish a self-service change from a cross-user one. A custom auth backend may implement a different per-target policy. + ### Pluggable Backend The authentication manager does not own the user database itself. All operations that read or modify user state — login, user add/delete, permission updates, credential updates — are dispatched through a `whAuthCb` callback table that the application supplies at server initialization. The storage backend is therefore a port-time decision: an in-memory table for development, an NVM-backed store for production, or a connector to an external identity service. diff --git a/src/wh_auth.c b/src/wh_auth.c index 7498a3e93..17e61acbe 100644 --- a/src/wh_auth.c +++ b/src/wh_auth.c @@ -421,8 +421,9 @@ int wh_Auth_UserSetCredentials(whAuthContext* context, whUserId user_id, rc = WH_AUTH_LOCK(context); if (rc == WH_ERROR_OK) { rc = context->cb->UserSetCredentials( - context->context, user_id, method, current_credentials, - current_credentials_len, new_credentials, new_credentials_len); + context->context, context->user.user_id, user_id, method, + current_credentials, current_credentials_len, new_credentials, + new_credentials_len); (void)WH_AUTH_UNLOCK(context); } /* LOCK() */ diff --git a/src/wh_auth_base.c b/src/wh_auth_base.c index 6b3306aeb..9b49f7001 100644 --- a/src/wh_auth_base.c +++ b/src/wh_auth_base.c @@ -452,8 +452,8 @@ int wh_Auth_BaseUserGet(void* context, const char* username, } -int wh_Auth_BaseUserSetCredentials(void* context, uint16_t user_id, - whAuthMethod method, +int wh_Auth_BaseUserSetCredentials(void* context, uint16_t current_user_id, + uint16_t user_id, whAuthMethod method, const void* current_credentials, uint16_t current_credentials_len, const void* new_credentials, @@ -467,6 +467,18 @@ int wh_Auth_BaseUserSetCredentials(void* context, uint16_t user_id, return WH_ERROR_BADARGS; } + if (current_user_id == WH_USER_ID_INVALID || + current_user_id > WH_AUTH_BASE_MAX_USERS) { + return WH_ERROR_BADARGS; + } + + /* A non-admin caller may only set its own credentials; an admin caller may + * set credentials for any user. */ + if (current_user_id != user_id && + !WH_AUTH_IS_ADMIN(users[current_user_id - 1].user.permissions)) { + return WH_ERROR_ACCESS; + } + /* Validate method is supported */ if (method != WH_AUTH_METHOD_PIN #if defined(WOLFHSM_CFG_CERTIFICATE_MANAGER) && !defined(WOLFHSM_CFG_NO_CRYPTO) diff --git a/test/wh_test_auth.c b/test/wh_test_auth.c index cb9e65013..c5b129093 100644 --- a/test/wh_test_auth.c +++ b/test/wh_test_auth.c @@ -1131,6 +1131,78 @@ int whTest_AuthSetCredentials(whClientContext* client) /* Should succeed - admin can set credentials for other users */ WH_TEST_ASSERT_RETURN(server_rc == WH_ERROR_OK); + /* non-admin caller may set its OWN credentials but must not be able to set + * another user's credentials. */ + WH_TEST_PRINT(" Test: Non-admin set-credentials authorization\n"); + { + whUserId self_id = WH_USER_ID_INVALID; + whUserId victim_id = WH_USER_ID_INVALID; + whUserId victim_login = WH_USER_ID_INVALID; + whAuthPermissions nonadmin_perms; + + memset(&nonadmin_perms, 0, sizeof(nonadmin_perms)); + WH_AUTH_SET_ALLOWED_GROUP(nonadmin_perms, WH_MESSAGE_GROUP_AUTH); + WH_AUTH_SET_IS_ADMIN(nonadmin_perms, 0); + + server_rc = 0; + WH_TEST_RETURN_ON_FAIL(_whTest_Auth_UserAddOp( + client, "selfcreduser", nonadmin_perms, WH_AUTH_METHOD_PIN, + "selfpin", 7, &server_rc, &self_id)); + WH_TEST_ASSERT_RETURN(server_rc == WH_ERROR_OK); + + memset(&perms, 0, sizeof(perms)); + server_rc = 0; + WH_TEST_RETURN_ON_FAIL(_whTest_Auth_UserAddOp( + client, "victimcreduser", perms, WH_AUTH_METHOD_PIN, "victimpin", 9, + &server_rc, &victim_id)); + WH_TEST_ASSERT_RETURN(server_rc == WH_ERROR_OK); + + /* Logout admin and login as the non-admin caller */ + server_rc = 0; + _whTest_Auth_LogoutOp(client, admin_id, &server_rc); + server_rc = 0; + WH_TEST_RETURN_ON_FAIL( + _whTest_Auth_LoginOp(client, WH_AUTH_METHOD_PIN, "selfcreduser", + "selfpin", 7, &server_rc, &self_id)); + WH_TEST_ASSERT_RETURN(server_rc == WH_ERROR_OK); + + /* Allowed: non-admin sets its own credentials */ + server_rc = 0; + WH_TEST_RETURN_ON_FAIL(_whTest_Auth_UserSetCredsOp( + client, self_id, WH_AUTH_METHOD_PIN, "selfpin", 7, "selfpin2", 8, + &server_rc)); + WH_TEST_ASSERT_RETURN(server_rc == WH_ERROR_OK); + WH_TEST_PRINT(" Non-admin self set-credentials allowed\n"); + + /* Denied: non-admin sets ANOTHER user's credentials. */ + server_rc = 0; + WH_TEST_RETURN_ON_FAIL(_whTest_Auth_UserSetCredsOp( + client, victim_id, WH_AUTH_METHOD_PIN, "victimpin", 9, "pwned", 5, + &server_rc)); + WH_TEST_ASSERT_RETURN(server_rc == WH_ERROR_ACCESS); + WH_TEST_PRINT(" Non-admin cross-user set-credentials denied\n"); + + /* The denied call must not have altered credentials */ + server_rc = 0; + _whTest_Auth_LogoutOp(client, self_id, &server_rc); + server_rc = 0; + WH_TEST_RETURN_ON_FAIL( + _whTest_Auth_LoginOp(client, WH_AUTH_METHOD_PIN, "victimcreduser", + "victimpin", 9, &server_rc, &victim_login)); + WH_TEST_ASSERT_RETURN(server_rc == WH_ERROR_OK); + server_rc = 0; + _whTest_Auth_LogoutOp(client, victim_login, &server_rc); + + /* Re-login as admin and clean up the test users */ + server_rc = 0; + WH_TEST_RETURN_ON_FAIL(_whTest_Auth_LoginOp(client, WH_AUTH_METHOD_PIN, + "admin", "1234", 4, + &server_rc, &admin_id)); + WH_TEST_ASSERT_RETURN(server_rc == WH_ERROR_OK); + _whTest_Auth_DeleteUserByName(client, "selfcreduser"); + _whTest_Auth_DeleteUserByName(client, "victimcreduser"); + } + /* Verify new credentials work */ _whTest_Auth_LogoutOp(client, admin_id, &server_rc); memset(&admin_perms, 0, sizeof(admin_perms)); diff --git a/wolfhsm/wh_auth.h b/wolfhsm/wh_auth.h index 85edbc3b0..76f1bea9e 100644 --- a/wolfhsm/wh_auth.h +++ b/wolfhsm/wh_auth.h @@ -193,8 +193,8 @@ typedef struct { whAuthPermissions* out_permissions); /* Set user credentials (PIN, etc.) */ - int (*UserSetCredentials)(void* context, whUserId user_id, - whAuthMethod method, + int (*UserSetCredentials)(void* context, whUserId current_user_id, + whUserId user_id, whAuthMethod method, const void* current_credentials, uint16_t current_credentials_len, const void* new_credentials, diff --git a/wolfhsm/wh_auth_base.h b/wolfhsm/wh_auth_base.h index 3975f8c05..0f966b3cd 100644 --- a/wolfhsm/wh_auth_base.h +++ b/wolfhsm/wh_auth_base.h @@ -140,7 +140,12 @@ int wh_Auth_BaseUserGet(void* context, const char* username, /** * @brief Set user credentials (PIN, etc.). * + * A non-admin caller may only set its own credentials; an admin caller may set + * credentials for any user. + * * @param[in] context Pointer to the auth base context. + * @param[in] current_user_id The user ID of the caller performing the + * operation. * @param[in] user_id The user ID to set credentials for. * @param[in] method The authentication method. * @param[in] current_credentials Pointer to the current credentials data. @@ -149,8 +154,8 @@ int wh_Auth_BaseUserGet(void* context, const char* username, * @param[in] new_credentials_len Length of the new credentials data. * @return int Returns 0 on success, or a negative error code on failure. */ -int wh_Auth_BaseUserSetCredentials(void* context, uint16_t user_id, - whAuthMethod method, +int wh_Auth_BaseUserSetCredentials(void* context, uint16_t current_user_id, + uint16_t user_id, whAuthMethod method, const void* current_credentials, uint16_t current_credentials_len, const void* new_credentials,