GHSA-97cv-x867-6xhm Klever-Go Account takeover: `kleverUpdateAccountPermission` authorizes on attacker-controlled `RecipientAddr` instead of the authenticated caller

• Go github.com/klever-io/klever-go: < 1.7.20 → fixed in 1.7.20

Description The VM built-in function `KleverUpdateAccountPermission` (registered always-active, `creator.go:381-390` / `core/vmconstants.go:234`) rewrites an account's entire permission set. Its authorization check uses `vmInput.RecipientAddr` attacker-controlled instead of the authenticated `vmInput.CallerAddr`. The sibling handler `kleverChangeOwnerAddress.go:86` uses `vmInput.CallerAddr` correctly, so the safe pattern exists in-repo; this handler deviates. The native transaction path (`txProcess.go:833`) is safe it uses `tx.GetSender()`. Mechanism: 1. Wrong variable: `CallerAddr` is never referenced in the handler; auth is `contractHasValidPermission(target.GetPermissions(), RecipientAddr)`, which returns true if `RecipientAddr` is a signer with `Weight >= Threshold` in the *target* account's permissions and the permission grants `UpdateAccountPermissionContractType`. 2. RecipientAddr is attacker-controlled: when a contract calls a built-in via `ExecuteOnDestContextWithTypedArgs` (`baseOps.go:1967`), `prepareIndirectContractCallInput` (`baseOps.go:2485`) sets `RecipientAddr = destination` (contract-chosen) and `CallerAddr = the calling contract`. The blockchain hook (`blockChainHook.go:454/467`) dispatches on `input.Function` and passes the input through unchanged; no guard forces `RecipientAddr == CallerAddr` and there is no SC-destination validation on this path. 3. Self-signer default satisfies the check: `createDefaultOwnerPermission` (`accounts.go:1848`) makes an account its own signer (weight 1, threshold 1, Owner type), and `CheckPermissionGrantedForContracts` returns true for Owner, so `contractHasValidPermission(V.perms, V) == true`. (More generally, `RecipientAddr` can be set to *any* of V's signer addresses meeting threshold all public on-chain.) Accounts with no stored permissions have empty `GetPermissions()` and are immune. 4. Overwrite is unrestricted: `UpdatePermission(V, attackerContract)` (`accounts.go:1863`) replaces V's permission set with attacker-supplied signers; if the attacker supplies an Owner-type permission, no default is appended and V's prior control is fully evicted. Code walkthrough (a) The vulnerable handler — `core/kapp/builtInFunctions/kleverUpdateAccountPermission.go`: ```go func (e *kleverUpdateAccountPermission) ProcessBuiltinFunction(vmInput *vmcommon.ContractCallInput) (*vmcommon.VMOutput, error) { ... address := vmInput.NextArg() // Arguments[0] — attacker-chosen target account V contract, err := e.getUpdateAccountPermissionContract(vmInput) // Arguments[1] — attacker-chosen new permissions ... acc, err := e.accountsCacher.LoadUser(address) // loads V ... // BUG: authorizes against vmInput.RecipientAddr (attacker-controlled), NOT vmInput.CallerAddr if !e.contractHasValidPermission(acc.GetPermissions(), vmInput.RecipientAddr) { // L91 return nil, errors.New("invalid permission operation") } // overwrites V's entire permission set with attacker-supplied signers resultCode, err := e.kappController.GetAccountsKApp().UpdatePermission(address, contract) ... } ``` (b) The check just name-matches `recipientAddr` against V's own signers — same file: ```go func (e *kleverUpdateAccountPermission) contractHasValidPermission(permissions []*state.Permission, recipientAddr []byte) bool { for _, permission := range permissions { for _, signer := range permission.Signers { if !bytes.Equal(signer.Address, recipientAddr) { // recipientAddr, not the authenticated caller continue } if signer.Weight >= permission.Threshold && permission.CheckPermissionGrantedForContracts(transaction.TXContract_UpdateAccountPermissionContractType) { return true } } } return false } ``` (c) The dispatch makes `RecipientAddr` attacker-controlled — `kvm/vmhost/vmhooks/baseOps.go:2464` `prepareIndirectContractCallInput` (invoked when a contract calls the built-in via `ExecuteOnDestContext`): ```go contractCallInput := &vmcommon.ContractCallInput{ VMInput: vmcommon.VMInput{ CallerAddr: sender, // the calling contract (authenticated) — NOT used by the handler Arguments: data, // attacker-chosen: [V, attackerPermissions] ... }, RecipientAddr: destination, // the contract's chosen `dest` argument — attacker sets this to V Function: string(function), } ``` (d) Every account with configured permissions is its own signer — `core/kapp/accounts/accounts.go:1848` `createDefaultOwnerPermission` (appended by `UpdatePermission` when no Owner permission is supplied): ```go return &state.Permission{ Type: state.Permission_Owner, // Owner grants ALL contract types incl. type 22 Threshold: 1, Signers: []*state.Key{ { Address: ownerAcc.AddressBytes(), Weight: 1 }, // the account signs for itself }, } ``` So `contractHasValidPermission(V.perms, RecipientAddr=V)` finds V's own address as a `Weight 1 >= Threshold 1` Owner signer → returns `true`. (e) Contrast — the sibling handler does it correctly — `core/kapp/builtInFunctions/kleverChangeOwnerAddress.go:86`: ```go callerAddress := vmInput.CallerAddr // authenticated caller ... if !bytes.Equal(callerAddress, acc.GetOwnerAddress()) { // checks the CALLER, not RecipientAddr return nil, ErrOperationNotPermitted } ``` Putting it together — the attacker's contract call: ``` ExecuteOnDestContext( gas, dest = V, // → RecipientAddr = V value = 0, function = "KleverUpdateAccountPermission", args = [ V, attackerOwnerPermsWithOnlyAttackerKey ], // Arguments[0]=V, Arguments[1]=new perms ) ``` → `CallerAddr = attackerContract` (ignored), `RecipientAddr = V`, `contractHasValidPermission(V.perms, V) == true` → V's permissions overwritten with the attacker's key as sole Owner signer. The attacker never held a key of V and provided no signature from V. POC put the following poc testcase under /core/kapp/builtInFunctions/ POC Code: https://gist.github.com/mabdullah22/a41f90aa5ba86bbebf121f739bd5f5e9 Run: ``` cd klever-go GOTOOLCHAIN=auto go test ./core/kapp/builtInFunctions/ -run TestPoC_PermTakeover -v ``` Output: ``` TAKEOVER CONFIRMED: caller="attacker-contract" (attacker SC) rewrote account V="victim-account-V"; new sole owner signer="attacker-key-EVIL" --- PASS: TestPoC_PermTakeover --- PASS: TestPoC_PermTakeover_NoStoredPermsIsSafe ``` The harm asserted is the takeover itself: after the call, V's permission set is a single Owner permission whose sole signer is the attacker's key; V's original owner signer is gone. Impact Full takeover of any account that has configured permissions i.e. every multisig / advanced-permission account , by an attacker who deploys a cheap smart contract and supplies only public on-chain addresses (no keys, no signatures from the victim). After takeover the attacker controls all of the victim's operations → theft or permanent lock of all the account's assets. Reachable via a permissionlessly-deployed contract (the plain-tx path is safe, so it is Critical-via-contract, not fully no-contract). No fork flag gates it. Severity Critical: Impact High (full account/asset compromise) Recommendation Authorize against the authenticated caller, mirroring `kleverChangeOwnerAddress`: ```go if !e.contractHasValidPermission(acc.GetPermissions(), vmInput.CallerAddr) { ... } ``` Reconcile the SC-call authority model: on the built-in path `CallerAddr` is the calling contract, so a contract should only be able to update permissions of accounts that legitimately list *it* as an authorized signer — never an arbitrary victim. Consider also requiring the target account (`Arguments[0]`) to equal the authorized caller's account, matching the native `tx.GetSender()` model.