search

solana-labs / solana

Web-Scale Blockchain for fast, secure, scalable, decentralized apps and marketplaces.
https://solanalabs.com
Apache License 2.0
13.34k stars 4.35k forks source link

Native loader tx instructions should be fully disabled #21347

Closed jstarry closed 2 years ago

jstarry commented 3 years ago

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 commented 3 years ago

cc @jackcmay @Lichtso

jackcmay commented 3 years ago

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

jackcmay commented 3 years ago

@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 commented 3 years ago

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: https://github.com/solana-labs/solana/blob/ebea3297f969879b938add38bb34ed94a012e12d/program-runtime/src/instruction_processor.rs#L380

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 commented 3 years ago

@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: https://github.com/solana-labs/solana/issues/21346

github-actions[bot] commented 2 years ago

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.