Fix - Invoke non program account owned by a builtin#5158
Conversation
Lichtso
force-pushed
the
fix/invoke_non_program_account_owned_by_a_builtin
branch
from
1738f1c to
ad7759a
Compare
…oader owned or non built-in account.
Lichtso
force-pushed
the
fix/invoke_non_program_account_owned_by_a_builtin
branch
from
ad7759a to
64d3113
Compare
| Err(TransactionError::InstructionError( | ||
| 0, | ||
| InstructionError::InvalidInstructionData, | ||
| InstructionError::UnsupportedProgramId, |
There was a problem hiding this comment.
This is where I first noticed this issue.
|
Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis. |
* Adds test_invoke_non_program_account_owned_by_a_builtin(). * Throws InstructionError::UnsupportedProgramId when invoking any non loader owned or non built-in account. (cherry picked from commit 9adbffc)
| if bpf_loader_deprecated::check_id(owner_id) | ||
| || bpf_loader::check_id(owner_id) | ||
| || bpf_loader_upgradeable::check_id(owner_id) | ||
| || loader_v4::check_id(owner_id) |
There was a problem hiding this comment.
Shouldn't we apply the same checks to account loading? We could get rid of all of the validated_loaders logic there.
There was a problem hiding this comment.
Yes, that is why I added you as reviewer ;)
There was a problem hiding this comment.
Haha, great, follow up PR then? And these changes should be amended to the SIMD: solana-foundation/solana-improvement-documents#162
|
I don't think this should have been merged without first rekeying |
|
Can't rekey in the same PR anymore because SDK was moved out. Have to do it in the sdk repo and then, once the backport lands we can bump the release there. See Discord discussion mentioned above. |
|
Actually yeah the rekey should land after the implementation, never mind. |
* Adds test_invoke_non_program_account_owned_by_a_builtin(). * Throws InstructionError::UnsupportedProgramId when invoking any non loader owned or non built-in account. (cherry picked from commit 9adbffc)
* Adds test_invoke_non_program_account_owned_by_a_builtin(). * Throws InstructionError::UnsupportedProgramId when invoking any non loader owned or non built-in account. (cherry picked from commit 9adbffc)
…#5158) (#5207) Fix - Invoke non program account owned by a builtin (#5158) * Adds test_invoke_non_program_account_owned_by_a_builtin(). * Throws InstructionError::UnsupportedProgramId when invoking any non loader owned or non built-in account. (cherry picked from commit 9adbffc) Co-authored-by: Alexander Meißner <AlexanderMeissner@gmx.net>
3 participants
Problem
Currently there is a slight bug once
remove_accounts_executable_flag_checksis active:One can invoke a built-in program by invoking any account owned by it instead. This leads to the built-in running as a different pubkey, thus all ownership checks fail and the built-in has no write access to anything.
This is benign as it only allows invoking built-in programs in a strange nonsensical way. But, it is still a stupid thing to support and would hinder future protocol changes such as these in the account loader.
About rekeying: Well see Discord discussion.
Summary of Changes
Blocks the execution of any non loader owned or non built-in account and adds
test_invoke_non_program_account_owned_by_a_builtin()to demonstrate the change in behavior.Feature Gate Issue: https://github.com/anza-xyz/feature-gate-tracker/issues/69