Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Native loader tx instructions should be fully disabled #21347

Closed
jstarry opened this issue Nov 18, 2021 · 6 comments · Fixed by #21591
Closed

Native loader tx instructions should be fully disabled #21347

jstarry opened this issue Nov 18, 2021 · 6 comments · Fixed by #21591

Comments

@jstarry
Copy link
Member

jstarry commented Nov 18, 2021

Problem

It's possible to invoke the native loader from a transaction and have it process instructions as if it were another native program (such as system program). This is harmless in practice but this edge causes lots of headaches when refactoring ix processing

Proposed change

  1. Add test case for this type of ix to ensure we don't lose compatibility with the cluster
  2. Add a feature switch to remove this functionality
@jstarry
Copy link
Member Author

jstarry commented Nov 18, 2021

cc @jackcmay @Lichtso

@jackcmay
Copy link
Contributor

jackcmay commented Nov 18, 2021

Forbidding these should be feature gated and PR'd independently of #21346

@jackcmay
Copy link
Contributor

@jstarry The feature remove_native_loader isn't sufficient to disable the native loader? If not can you give a more specific example of the scenario you have in mind?

@Lichtso
Copy link
Contributor

Lichtso commented Nov 19, 2021

I believe this issue is independent of the native_loader instruction processing here,
but is related to the native_loader::id() being used as a terminal symbol in the chain of executable accounts here.

The difference would be the one between the program_id and its owner:

let root_account = keyed_account_at_index(keyed_accounts, 0)

But I haven't tried deactivating the feature remove_native_loader and running this new test test_an_empty_transaction_without_program here:
https://github.com/Lichtso/solana/blob/a7abec638c28ee1f4b7aa3f92012efad129eadb5/runtime/src/bank.rs#L15396
Edit: Tried and it does not make a difference because it takes the else branch of if solana_sdk::native_loader::check_id(owner_id).

@jstarry
Copy link
Member Author

jstarry commented Nov 23, 2021

@jstarry The feature remove_native_loader isn't sufficient to disable the native loader? If not can you give a more specific example of the scenario you have in mind?

Nope, not sufficient. As @Lichtso mentioned there is still a path. I already provided an example in the issue description here: #21346

@github-actions
Copy link
Contributor

This issue has been automatically locked since there has not been any activity in past 7 days after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants