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.

Sign up for GitHub

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

clean feature: prevent_calling_precompiles_as_programs #27100

Merged
merged 6 commits into from
Aug 18, 2022
+20 −82
Merged

clean feature: prevent_calling_precompiles_as_programs #27100

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3bcc636
clean feature: prevent_calling_precompiles_as_programs
jstarry Aug 12, 2022
d58495e
fix tests
jstarry Aug 12, 2022
333ffd4
fix test
jstarry Aug 12, 2022
4d83c81
remove comment
jstarry Aug 12, 2022
dbff6d1
fix test
jstarry Aug 13, 2022
6e6bae6
feedback
jstarry Aug 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions programs/bpf_loader/src/syscalls/cpi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -842,12 +842,9 @@ fn check_authorized_program(
&& !(bpf_loader_upgradeable::is_upgrade_instruction(instruction_data)
jstarry marked this conversation as resolved.
Show resolved Hide resolved
|| bpf_loader_upgradeable::is_set_authority_instruction(instruction_data)
|| bpf_loader_upgradeable::is_close_instruction(instruction_data)))
|| (invoke_context
.feature_set
.is_active(&prevent_calling_precompiles_as_programs::id())
&& is_precompile(program_id, |feature_id: &Pubkey| {
invoke_context.feature_set.is_active(feature_id)
}))
|| is_precompile(program_id, |feature_id: &Pubkey| {
invoke_context.feature_set.is_active(feature_id)
})
{
return Err(SyscallError::ProgramNotSupported(*program_id).into());
}
Expand Down
3 changes: 1 addition & 2 deletions programs/bpf_loader/src/syscalls/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,7 @@ use {
self, blake3_syscall_enabled, check_physical_overlapping, check_slice_translation_size,
curve25519_syscall_enabled, disable_cpi_setting_executable_and_rent_epoch,
disable_fees_sysvar, enable_early_verification_of_account_modifications,
libsecp256k1_0_5_upgrade_enabled, limit_secp256k1_recovery_id,
prevent_calling_precompiles_as_programs, syscall_saturated_math,
libsecp256k1_0_5_upgrade_enabled, limit_secp256k1_recovery_id, syscall_saturated_math,
},
hash::{Hasher, HASH_BYTES},
instruction::{
Expand Down
25 changes: 0 additions & 25 deletions programs/ed25519-tests/tests/process_transaction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ use {
solana_program_test::*,
solana_sdk::{
ed25519_instruction::new_ed25519_instruction,
feature_set,
signature::Signer,
transaction::{Transaction, TransactionError},
},
Expand Down Expand Up @@ -60,27 +59,3 @@ async fn test_failure() {
))
);
}

#[tokio::test]
async fn test_success_call_builtin_program() {
let mut program_test = ProgramTest::default();
program_test.deactivate_feature(feature_set::prevent_calling_precompiles_as_programs::id());
let mut context = program_test.start_with_context().await;

let client = &mut context.banks_client;
let payer = &context.payer;
let recent_blockhash = context.last_blockhash;

let privkey = ed25519_dalek::Keypair::generate(&mut thread_rng());
let message_arr = b"hello";
let instruction = new_ed25519_instruction(&privkey, message_arr);

let transaction = Transaction::new_signed_with_payer(
&[instruction],
Some(&payer.pubkey()),
&[payer],
recent_blockhash,
);

assert_matches!(client.process_transaction(transaction).await, Ok(()));
}
10 changes: 5 additions & 5 deletions runtime/src/bank.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14540,25 +14540,25 @@ pub(crate) mod tests {
if bank.slot == 0 {
assert_eq!(
bank.hash().to_string(),
"9tLrxkBoNE7zEUZ2g72ZwE4fTfhUQnhC8A4Xt4EmYhP1"
"5gY6TCgB9NymbbxgFgAjvYLpXjyXiVyyruS1aEwbWKLK"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These had to be updated because the genesis block now includes the precompile accounts

);
}
if bank.slot == 32 {
assert_eq!(
bank.hash().to_string(),
"AxphC8xDj9gmFosor5gyiovNvPVMydJCFRUTxn2wFiQf"
"6uJ5C4QDXWCN39EjJ5Frcz73nnS2jMJ55KgkQff12Fqp"
);
}
if bank.slot == 64 {
assert_eq!(
bank.hash().to_string(),
"4vZCSbBuL8xjE43rCy9Cm3dCh1BMj45heMiMb6n6qgzA"
"4u8bxZRLYdQBkWRBwmpcwcQVMCJoEpzY7hCuAzxr3kCe"
);
}
if bank.slot == 128 {
assert_eq!(
bank.hash().to_string(),
"46LUpeBdJuisnfwgYisvh4x7jnxzBaLfHF614GtcTs59"
"4c5F8UbcDD8FM7qXcfv6BPPo6nHNYJQmN5gHiCMTdEzX"
);
break;
}
Expand Down Expand Up @@ -14786,7 +14786,7 @@ pub(crate) mod tests {
// No more slots should be shrunk
assert_eq!(bank2.shrink_candidate_slots(), 0);
// alive_counts represents the count of alive accounts in the three slots 0,1,2
assert_eq!(alive_counts, vec![9, 1, 7]);
assert_eq!(alive_counts, vec![11, 1, 7]);
}

#[test]
Expand Down
31 changes: 2 additions & 29 deletions runtime/src/builtins.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,8 @@
use solana_frozen_abi::abi_example::AbiExample;
use {
crate::system_instruction_processor,
solana_program_runtime::invoke_context::{InvokeContext, ProcessInstructionWithContext},
solana_sdk::{
feature_set, instruction::InstructionError, pubkey::Pubkey, stake, system_program,
},
solana_program_runtime::invoke_context::ProcessInstructionWithContext,
solana_sdk::{feature_set, pubkey::Pubkey, stake, system_program},
std::fmt,
};

Expand Down Expand Up @@ -142,13 +140,6 @@ fn genesis_builtins() -> Vec<Builtin> {
}

/// place holder for precompile programs, remove when the precompile program is deactivated via feature activation
jstarry marked this conversation as resolved.
Show resolved Hide resolved
fn dummy_process_instruction(
_first_instruction_account: usize,
_invoke_context: &mut InvokeContext,
) -> Result<(), InstructionError> {
Ok(())
}

/// Dynamic feature transitions for builtin programs
fn builtin_feature_transitions() -> Vec<BuiltinFeatureTransition> {
vec![
Expand All @@ -160,24 +151,6 @@ fn builtin_feature_transitions() -> Vec<BuiltinFeatureTransition> {
),
feature_id: feature_set::add_compute_budget_program::id(),
},
BuiltinFeatureTransition::RemoveOrRetain {
previously_added_builtin: Builtin::new(
"secp256k1_program",
solana_sdk::secp256k1_program::id(),
dummy_process_instruction,
),
addition_feature_id: feature_set::secp256k1_program_enabled::id(),
removal_feature_id: feature_set::prevent_calling_precompiles_as_programs::id(),
},
BuiltinFeatureTransition::RemoveOrRetain {
previously_added_builtin: Builtin::new(
"ed25519_program",
solana_sdk::ed25519_program::id(),
dummy_process_instruction,
),
addition_feature_id: feature_set::ed25519_program_enabled::id(),
removal_feature_id: feature_set::prevent_calling_precompiles_as_programs::id(),
},
BuiltinFeatureTransition::Add {
builtin: Builtin::new(
"address_lookup_table_program",
Expand Down
2 changes: 2 additions & 0 deletions runtime/src/genesis_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ pub fn bootstrap_validator_stake_lamports() -> u64 {
// Number of lamports automatically used for genesis accounts
pub const fn genesis_sysvar_and_builtin_program_lamports() -> u64 {
const NUM_BUILTIN_PROGRAMS: u64 = 4;
const NUM_PRECOMPILES: u64 = 2;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that the feature ids passed to Precompile::new are both None, Bank::finish_init will always add the precompile accounts if they don't exist yet. This should be safe on live clusters because those precompile accounts already exist.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems so:

$ ./target/release/solana -ud account KeccakSecp256k11111111111111111111111111111

Public Key: KeccakSecp256k11111111111111111111111111111
Balance: 0.000000001 SOL
Owner: NativeLoader1111111111111111111111111111111
Executable: true
Rent Epoch: 0
Length: 17 (0x11) bytes
0000:   73 65 63 70  32 35 36 6b  31 5f 70 72  6f 67 72 61   secp256k1_progra
0010:   6d                                                   m

$ ./target/release/solana -ut account KeccakSecp256k11111111111111111111111111111

Public Key: KeccakSecp256k11111111111111111111111111111
Balance: 0.000000001 SOL
Owner: NativeLoader1111111111111111111111111111111
Executable: true
Rent Epoch: 0
Length: 17 (0x11) bytes
0000:   73 65 63 70  32 35 36 6b  31 5f 70 72  6f 67 72 61   secp256k1_progra
0010:   6d                                                   m

$ ./target/release/solana -um account KeccakSecp256k11111111111111111111111111111

Public Key: KeccakSecp256k11111111111111111111111111111
Balance: 0.000000001 SOL
Owner: NativeLoader1111111111111111111111111111111
Executable: true
Rent Epoch: 0
Length: 17 (0x11) bytes
0000:   73 65 63 70  32 35 36 6b  31 5f 70 72  6f 67 72 61   secp256k1_progra
0010:   6d                                                   m

$ ./target/release/solana -ud account Ed25519SigVerify111111111111111111111111111

Public Key: Ed25519SigVerify111111111111111111111111111
Balance: 0.000000001 SOL
Owner: NativeLoader1111111111111111111111111111111
Executable: true
Rent Epoch: 0
Length: 15 (0xf) bytes
0000:   65 64 32 35  35 31 39 5f  70 72 6f 67  72 61 6d      ed25519_program

$ ./target/release/solana -ut account Ed25519SigVerify111111111111111111111111111

Public Key: Ed25519SigVerify111111111111111111111111111
Balance: 0.000000001 SOL
Owner: 11111111111111111111111111111111
Executable: true
Rent Epoch: 0

$ ./target/release/solana -um account Ed25519SigVerify111111111111111111111111111

Public Key: Ed25519SigVerify111111111111111111111111111
Balance: 0.000000001 SOL
Owner: NativeLoader1111111111111111111111111111111
Executable: true
Rent Epoch: 0
Length: 15 (0xf) bytes
0000:   65 64 32 35  35 31 39 5f  70 72 6f 67  72 61 6d      ed25519_program

const FEES_SYSVAR_MIN_BALANCE: u64 = 946_560;
const STAKE_HISTORY_MIN_BALANCE: u64 = 114_979_200;
const CLOCK_SYSVAR_MIN_BALANCE: u64 = 1_169_280;
Expand All @@ -41,6 +42,7 @@ pub const fn genesis_sysvar_and_builtin_program_lamports() -> u64 {
+ EPOCH_SCHEDULE_SYSVAR_MIN_BALANCE
+ RECENT_BLOCKHASHES_SYSVAR_MIN_BALANCE
+ NUM_BUILTIN_PROGRAMS
+ NUM_PRECOMPILES
}

pub struct ValidatorVoteKeypairs {
Expand Down
8 changes: 3 additions & 5 deletions runtime/src/message_processor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use {
},
solana_sdk::{
account::WritableAccount,
feature_set::{prevent_calling_precompiles_as_programs, FeatureSet},
feature_set::FeatureSet,
hash::Hash,
message::SanitizedMessage,
precompiles::is_precompile,
Expand Down Expand Up @@ -86,10 +86,8 @@ impl MessageProcessor {
.zip(program_indices.iter())
.enumerate()
{
let is_precompile = invoke_context
.feature_set
.is_active(&prevent_calling_precompiles_as_programs::id())
&& is_precompile(program_id, |id| invoke_context.feature_set.is_active(id));
let is_precompile =
is_precompile(program_id, |id| invoke_context.feature_set.is_active(id));

// Fixup the special instructions key if present
// before the account pre-values are taken care of
Expand Down
8 changes: 3 additions & 5 deletions sdk/src/precompiles.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,7 @@

use {
crate::{
decode_error::DecodeError,
feature_set::{prevent_calling_precompiles_as_programs, FeatureSet},
instruction::CompiledInstruction,
decode_error::DecodeError, feature_set::FeatureSet, instruction::CompiledInstruction,
pubkey::Pubkey,
},
lazy_static::lazy_static,
Expand Down Expand Up @@ -81,12 +79,12 @@ lazy_static! {
static ref PRECOMPILES: Vec<Precompile> = vec![
Precompile::new(
crate::secp256k1_program::id(),
Some(prevent_calling_precompiles_as_programs::id()),
None, // always enabled
crate::secp256k1_instruction::verify,
),
Precompile::new(
crate::ed25519_program::id(),
Some(prevent_calling_precompiles_as_programs::id()),
None, // always enabled
crate::ed25519_instruction::verify,
),
];
Expand Down