Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

set-upgrade-authority is scary #27932

Closed
0xripleys opened this issue Sep 20, 2022 · 9 comments
Closed

set-upgrade-authority is scary #27932

0xripleys opened this issue Sep 20, 2022 · 9 comments
Labels
community Community contribution stale [bot only] Added to stale content; results in auto-close after a week.

Comments

@0xripleys
Copy link
Contributor

0xripleys commented Sep 20, 2022

Problem

Relevant thread: https://twitter.com/0xripleys/status/1571981071968772097

Program upgrade authority management is tricky. From a brief survey, it seems like many teams use a ledger to hold the upgrade authority.

However, doing program upgrades from a ledger is impossible, since you have to sign hundreds of transactions that write to the buffer. What many people do (including Solend) is temporarily transfer the upgrade authority (solana set-upgrade-authority) to a software key, do the program deploy (solana program deploy), then transfer the upgrade authority back to the ledger.

This is pretty scary. Since solana set-upgrade-authority BUFFER_PUBKEY doesn't do any kind of verification on BUFFER_PUBKEY, it's easy to fat-finger this command and accidentally assign the authority to some random pubkey or worse, some malicious user.

Proposed Solution

  • Solution 1: Add the following instructions to the bpf loader program and the solana program subcommand:

    1. request-set-upgrade-authority DESTINATION_PUBKEY PROGRAM_ID: On chain, we record that the current program upgrade authority wishes to transfer the upgrade authority to DESTINATION_PUBKEY
    2. ack-set-upgrade-authority PROGRAM_ID: using the DESTINATION_PUBKEY keypair, we can confirm that we wish to receive the upgrade authority. On chain, we record this too. Also, prevent duplicate calls to this instruction (explained below).
    3. finalize-set-upgrade-authority DESTINATION_PUBKEY PROGRAM_ID: verifies that 1 and 2 are complete and then actually changes the program upgrade authority
    4. cancel-request-set-upgrade-authority DESTINATION_PUBKEY PROGRAM_ID

    These instructions would safeguard against transferring to:
    - random pubkey: impossible to sign for in step 2!
    - real keypair: If the malicious owner signs step 2, hoping to receive the upgrade authority, your attempt to do (2) will fail and revert the entire sequence.

  • Solution 2: Allow deploying buffers owned by some other authority.

    If we removed this check from the loader, then we could write the entire buffer with a software key, then only do the deploy with the ledger (ie only 1 ledger signing), which would save a lot of hassle. I haven't though about this solution too much, maybe there are security concerns here.

Note: the long term solution for program key management is to use a multisig. However, while there are products that show promise, but there are no clear winners yet. And, transferring to a multisig still requires a set-upgrade-authority call.

@0xripleys 0xripleys added the community Community contribution label Sep 20, 2022
@richardwu
Copy link

👍 this would be very useful

@sg3510
Copy link

sg3510 commented Sep 20, 2022

+1 - being able to have the authority in some type of escrow state that the receiving party has to ack feels a lot safer/would reduce stress during deploys

@buffalu
Copy link
Contributor

buffalu commented Sep 20, 2022

can you do this and not need to sign a bunch of txs?

https://docs.solana.com/cli/deploy-a-program#using-an-intermediary-buffer-account

@0xripleys
Copy link
Contributor Author

0xripleys commented Sep 20, 2022

Oh! At first glance, this seems like exactly what i want.

so if i understand correctly:

  • I'd init and write the buffer with new program contents with my software key
  • set buffer authority to the program upgrade authority
  • do a solana program deploy with the buffer from step 1 (sign with ledger)

So I'd only sign with the ledger once.
Thanks! I will try this out :)

@buffalu
Copy link
Contributor

buffalu commented Sep 20, 2022

i've never used it, but this is how squads recommends to do things and i assume they don't make the multisig sign a bunch of txs https://docs.squads.so/squads-v3-docs/navigating-your-squad/programs#upgrade-a-program

@aeyakovenko
Copy link
Member

aeyakovenko commented Sep 21, 2022

Why not require 2 signatures in the tx? The original and the new authority should both sign the transaction.

@0xripleys
Copy link
Contributor Author

0xripleys commented Sep 21, 2022

That does seem better...just not sure if anyone relies on the property that you don't need to sign with the new authority. Eg when first moving to the Squads Protocol to do program upgrades, I have to set the upgrade authority to a squad program PDA (link). That's not a blocker for Squads though, it would be straightforward to fix.

I'd be in favour of making this change

@0xripleys
Copy link
Contributor Author

0xripleys commented Sep 23, 2022

hey @jstarry. I see you made some changes to the bpf loader recently.

based on aeyakovenko's comment above, I plan to modify the SetAuthority instruction to require 2 signatures when working with ProgramData accounts (the old authority AND the new authority). Before I start working on the PR:

  1. Does this change seem reasonable?
  2. Would something like this require a new feature gate?

@jstarry
Copy link
Contributor

jstarry commented Oct 7, 2022

The change is reasonable but should be added as a new instruction to avoid breaking assumptions that may have been made about the existing behavior. Typically we add a "Checked" suffix for a new instruction like this. And yes a feature gate will be required but the code reviewer for the change can assist in that

@github-actions github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Oct 9, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Oct 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
community Community contribution stale [bot only] Added to stale content; results in auto-close after a week.
Projects
None yet
Development

No branches or pull requests

6 participants