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

don't allow assignment to sysvar program #7017

Merged

Conversation

ParthDesai
Copy link
Contributor

Problem

See #7016

Summary of Changes

Don't allow changing account owner to sysvar program

Fixes #7016

@ParthDesai ParthDesai mentioned this pull request Nov 18, 2019
1 task
@@ -72,6 +72,12 @@ fn assign_account_to_program(
return Err(InstructionError::MissingRequiredSignature);
}

// guard against sysvars being assigned
Copy link
Contributor

Choose a reason for hiding this comment

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

this is already enforced by verify_instruction, I think...

Copy link
Contributor

Choose a reason for hiding this comment

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

can you have create_account call this function so there's only one check?

Copy link
Contributor

Choose a reason for hiding this comment

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

also: looks like the guard against changing anything about a sysvar account can be removed now

@ParthDesai
Copy link
Contributor Author

ParthDesai commented Nov 18, 2019 via email

@codecov
Copy link

codecov bot commented Nov 18, 2019

Codecov Report

Merging #7017 into master will decrease coverage by 16%.
The diff coverage is 61.9%.

@@            Coverage Diff            @@
##           master   #7017      +/-   ##
=========================================
- Coverage    75.8%   59.8%   -16.1%     
=========================================
  Files         223     223              
  Lines       44751   56739   +11988     
=========================================
+ Hits        33935   33947      +12     
- Misses      10816   22792   +11976

@rob-solana
Copy link
Contributor

ah, it's above in create_system_account() ;)

rob-solana
rob-solana previously approved these changes Nov 18, 2019
@garious
Copy link
Contributor

garious commented Nov 18, 2019

test?

@ParthDesai
Copy link
Contributor Author

test?

added.

@mergify mergify bot dismissed rob-solana’s stale review November 19, 2019 03:12

Pull request has been modified.

debug!("CreateAccount: program id {} invalid", program_id);
return Err(SystemError::InvalidProgramId.into());
}

if sysvar::is_sysvar_id(&to.unsigned_key()) {
Copy link
Contributor

@rob-solana rob-solana Nov 19, 2019

Choose a reason for hiding this comment

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

this check can die, IMHO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

two different checks, I think. This one prevents someone to store account against any sysvar pubkey, while the other check is to prevent assignment of sysvar program to any normal account.

Copy link
Contributor

@rob-solana rob-solana left a comment

Choose a reason for hiding this comment

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

good enough, we can consolidate duplicate checks at some point

@ParthDesai ParthDesai added the automerge Merge this Pull Request automatically once CI passes label Nov 19, 2019
@solana-grimes solana-grimes merged commit 3615209 into solana-labs:master Nov 19, 2019
@ParthDesai ParthDesai deleted the cannot-assign-sysvar-program branch November 19, 2019 03:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge this Pull Request automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

system instruction processor allows assignment to sysvar program
4 participants