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

New CIP to add security fields to existing CIP-108 to prevent imposters and metadata copy #978

Open
gitmachtl opened this issue Feb 1, 2025 · 15 comments

Comments

@gitmachtl
Copy link
Contributor

gitmachtl commented Feb 1, 2025

Let me first point everybody to this thread here:
Replay Vulnerability in Governance Action Metadata

Thanks to @Crypto2099 @Quantumplation @Ryun1 for the inputs.

In short: With the upcoming governance actions like treasury withdrawals there will be imposers to try and trick voters to vote for an action that has stolen/copied the metadata.

My proposal consists of a few changes/additions that makes it more unlikely that it would go thru undetected and to keep it managable also in environments that are not timesynced or that depend on the current slotnumber of the chain.

Needed additions in CIP100 and its childs (CIP108/119/etc)

1 - Add an additional field "depositReturnAddress"

A good indicator for the owner/submitter of an action is the depositReturnAddress (StakeAddress). Why? Because the 100kADA deposit amount are paid back to this address after the action got enacted/refused/timedout.

Therefore a new field should be included within the body to represent this address. f.e. "depositReturnAddress": "stake1xxxxxx"

The deposit return StakeAddress is known to the author before the submit or the signing process. Its not slotnumber dependent it can be done in unsynced offline environments too. There is no timelimit on such a data, no need to submit the metadata within a given timeframe. It makes it easy to compare the set StakeAddress with the one in the action itself. No need to query other databases to figure out the slotnumber when the action was submitted or removed.

This alone is not enough, we also need to make sure that the body was not modified by a potential imposer at all. For that we also need point 2.

1b - Include all receiving addresses of a treasury withdrawal within the metadata

As @Crypto2099 pointed out, the treasury withdrawal actions are most likely the ones we have to take most care of.

So, for a treasury withdrawal we can than also require that all the Addresses that are supposed to receive treasury funds via that action, must be included in an extra array and listed in the metadata itself.

Again to bake this into the canonized body hash signature, we need the next step.

2 - Make the authors signature mandatory

In order to detect a modified body content an so change in the canonized hash, a signature is mandatory.

We can discuss which kind of action metadata must contain at least one signature, like leaving it open for info-actions. But for actions like a hardfork-initation, committee-update, no-confidence and especially treasury withdrawal we need to have a signature as a must.

The used key for the signature is open to the choice of the author. It can be some key that is already public and known by the chain, or a totally different one.

There is no change needed in existing tooling that provide authors signing already. The important new information is in the new depostiReturnAddress field within the body

Signing via the CIP-0008/0030 method should be rolled out to all governance metadata content. The authorWitnessAlgorithm should also be renamed into CIP-0030 for those kinds of signatures, to make sure everyone uses the established set of parameters in the signing/verification process we already know from the CIP-0030 message-signing implementations. These are:

  kty (1) - must be set to OKP (1)
  kid (2) - Optional, if present must be set to the same value as in the Sig_structures protectedHeader_cbor via map entry (4)
  alg (3) - must be set to EdDSA (-8)
  crv (-1) - must be set to Ed25519 (6)
  x (-2) - must be set to the public key bytes of the key used to sign the Sig_structure

There should be a way for DReps, SPOs and the CC-Members to try to verify the identity of the proposer. For that we need the next step.

3 - Provide an identity entry to make the verification possible

To make it somehow possible for the voters (DReps, SPOs and CC-Members) to verify the identity, there should be at least one Identity entry/link (we have those kinds already) which points to a source of truth to compare the used publicKey of the signature(s). We could also do this vai an Other entry.


Steps to verify the used metadata

  1. Check the anchorHash(fileHash) of the metadata to be identical with the used one for the action itself. If not -> invalid metadata -> voters should not vote
  2. Check the overall metadata jsonld structure and content to be valid. If not -> invalid metadata -> voters should not vote
  3. Check that all signatures in the metadata are valid. There must be at least one signature. If not -> invalid metadata -> voters should not vote
  4. Check that there is a field depositReturnAddress within the body which is identical to the one that is set to be used in the governance action itself. If not -> invalid metadata -> voters should not vote
  5. In case of a treasury withdrawal, check that all listed receiving addresses in the action are also listed in the array of receiving addresses in the metadata. If not -> invalid metadata -> voters should not vote

In case someone really tries to impose someone else by using the same metadata link or just copy and paste it as a whole, it also means that such an imposer used the same depositReturnAddress for the action proposal. So the deposit amount of 100kADA are than already lost and on its way to the original owner of the metadata. Its highly unlikely that someone would do this intentionally imo.

In case there is such an "imposer" action on the chain, the Identity entrie(s) come into play. These still point to the original destination of the original author. So the original author can f.e. host a list of submitted actions ids on there, or simply deny that he/she has submitted an imposer action. In such cases, the voters can deny to vote on such an action and it can also be marked as "scam" on the various platforms/explorers/social-channels.

Adding also als receiving addresses for a treasury withdrawal within the metadata itself makes it basically impossible for an imposer to get funds.


Overall, its a very simple method to implement these enhancements without the need to host additional services on and/or offchain to track actions via nonce values, etc.

@Quantumplation
Copy link
Contributor

Actually, your first option gave me an idea that generalizes that approach: just to include the serialized gov_action exactly as it appears on chain, and require that tools verify that the details of this field match the on-chain governance action.

The only thing I can see is that someone could duplicate the same governance action to themselves. For example, maybe malicious actor A creates a withdrawal proposal to build popular idea X for reasonable price P; they create two governance actions on chain, each paying P ADA to address A, and then advertise the two gov_action_id's in two separate places; on twitter they publish gov_action_id M, and on discord and tiktok they advertise gov_action_id N. If the proposal itself is popular, and their community is large, they may get lots of votes on both, and end up with 2P ADA.

I think the risk of this attack vector is relatively low because:

  1. This would be noticed by those who browse the list of governance actions, bots who announce new governance actions, etc.
  2. It would be seen as pretty shady, and be announced widely across those social media platforms
  3. The largest voter power concentrations are likely to recognize that this is happening / only vote in one place
  4. It would need to be approved by the CC which would definitely recognize they were voting twice on the same thing. In theory this would require a provision in the constitution to allow them to vote no.

Also, it doesn't need to be a modification to all three CIPs; in principle, CIP100 was intended to be extended by layering, rather than revision, so it could just be a new CIP that defines this field, and then gets adopted by tools (if the field is present, they validate that it matches; if it's absent, they display a warning to the user before displaying the contents of the proposal).

An argument could be made for modifying CIP100 directly to make it more obvious and apply more social pressure for tools to adopt, but certainly we don't need to do so for all three, since CIP108 and CIP119 layer on top of CIP100.

@gitmachtl
Copy link
Contributor Author

gitmachtl commented Feb 1, 2025

EDIT: I updated my original post at the top with an additional requirement to also list all receiving addresses for a "treasury-withdrawal" in the metadata itself.

Makes it easy for existing cli tools to simply compare the content of not only the depositReturnAddress with the one that is used for the action, but also all fund receiving addresses in case of a treasury withdrawal.

@Quantumplation i agree, it would be enough to enforce it in CIP-0100 so all "child" CIPs would automatically need to adopt it.

@Quantumplation
Copy link
Contributor

That's not really how CIP-100 works; CIP-119 for example is not a replacement for CIP-100, so it wouldn't "need to adopt" anything.

There's not much we can do to "force" something to be adopted; including it in CIP-100 would just create social pressure for existing tools to update (which should be considered a fairly dramatic step in general), and make it discoverable as a "default" for new parties that are only reading that one CIP.

I think in 99% of the cases (likely including this one), a new CIP is just fine, and modifying CIP-100 should be an extreme step, only done to correct an outright error that makes the CIP un-adoptable, or to correct a security concern. The latter is why I think it might be appropriate to do it in CIP-100.

It may seem like a sticking point, but if we just fall into the habit of updating the past CIPs to add new fields / change definitions / etc. then CIP-100 has completely failed it's goal, and we could do that with a lot less complexity than CIP-100 introduces. i.e. we will have paid for the added complexity of JSON-LD, and then burned the benefits immediately.

@gitmachtl
Copy link
Contributor Author

Yes, if done in CIP-0100 or we define a new one with the changes and than basically every tooling jumps onto that CIP must be decided.

@Crypto2099
Copy link
Collaborator

The problem is that a new CIP that adds new fields (that may or may not be required) to CIP 100, 108, 119, 135, etc... How will implementors know where to look? Granted, I am working on a new common vocabulary home for all of these things which will hopefully become one of the canonical sources of truth so the question is does this rise to the level of a "security vulnerability" that warrants directly changing the underlying CIPs and we take this opportunity (while the ecosystem is still nascent) to update/alert all tooling creators to the changes?

I like the idea of maybe taking a hash of the serialized_gov_action as a replay prevention for any particular governance action (I suppose excepting info actions which don't have a direct on-chain result so are a relatively minor attack surface). Some of those parameter changes might be getting pretty large (even in serialized form) when we starting tweaking Plutus Cost Models or something... :D

@Quantumplation
Copy link
Contributor

Quantumplation commented Feb 2, 2025

I agree that in the case of an overlooked security vulnerability, it's likely worth it. I just want to make sure to emphasize that it's against the grain of the design, because I've noticed people's natural inclination seems to be to want to "update" CIPs and create a new version, which is exactly what the design was trying to avoid. As for where implementors go to decide what to implement (outside of security bulletins):

  • they look at these CIPs
  • they look at community driven initiatives that document them like the common vocabulary home (which sounds great, but should not be considered a single canonical source of truth, that defats the whole point!)
  • they look at what exists out in the wild when they index the chain
  • they implement what's popular and useful for their users
  • they define and socialize their own

See this post for a big writeup I just did about CIP-100, and how it embraces a stronger philosophy of actual decentralization of metadata, hosting, decision making, etc.

As for using the hash, that would be fine with me too, but I doubt that size would be an issue in any case. Keep in mind that the absolute maximum size it could possibly be is 16kb because of the tx size limit, because it has to fit in a transaction 😅 Even if we updated the cost models for plutus v1, v2, and v3, that's only just over 1kb.

Using a hash, or the details of the proposal, doesn't solve the "double spend" issue, as I described above, which may be worth adding an expiration date, TxIn, or nonce as well. For example, 10 years from now, when we have evolved the constitution a bunch, someone puts in a proposal to update the constitution back to the one that is currently being voted on today; the metadata attached would be very authoritative and convincing :)

@gitmachtl
Copy link
Contributor Author

gitmachtl commented Feb 2, 2025

ok, es expected this blows up to find a solution on a way higher level.

guys, keep it grounded on the things we already have.

this proposal is an easy implementation which would drastically enhance the "attack vector" of a problem we have not even seen on the chain up until now. the solution works with the tooling we already have and only needs tweeks on how metadata is checked against the gov-state.

the "normal" guys does not run specialized indexers on the linux instance. cardano-node and cardano-cli should be enough to get the needed information. not requiring plutus scripts and co just to get the info i want. changes to the node and cli take months now if they even come at all with the open roadmap.

i understand you pi that you're looking at this from another angle from an environment pov that has access to all funky data you wanna use, with indexers, databases, db-sync, etc. in the background. i am seeing the problem from the "little guy" pov just having cardano-node and cli running on a small instance to fulfil his voting duty.

i like the idea to do a CIP. "Governance Metadata Security Fields", lets try to enhance security with the things we can access right now. tools can use the information or not show it / prevent someone from voting.

@Quantumplation
Copy link
Contributor

I'm a bit confused by your response for a number of reasons.

  • I'm advocating for the simplest, fastest type of solution: write down the ideas we have now to solicit feedback, leave the door open for future solutions as needed, and give recommendations to tooling authors for how to adopt them
    • This is an artifact of the fact that we can't "mandate" anything; any solution will have to assume there is metadata out there that doesn't adhere to it
  • the "normal guy" isn't using a cardano node and a cardano-cli; they're relying on gov.tools or similar. Focusing on a solution that provides them the ability to build the best user experience will have the highest impact
  • If you want a solution that works for an SPO, for example, then several of the ideas above don't require anything other than the cardano-node and cardano-cli, and maybe a browser
    • A valid range can be checked without even those
    • The cbor bytes of the governance action can be parsed and printed for the user to compare
    • A TxIn can be looked up on cexplorer or similar to see if the expected governance action is attached
    • etc.

The only one that is difficult to achieve simply is specifying an explicit nonce, which I called out above as being a suboptimal solution for exactly that reason.

I'm not arguing that we pick one of these solutions over the others and overcomplicate it.

I'm saying that likely, at different points in time, different people will feel differently about what the "best" solution is, and so metadata will show up with different "ways" that it's secure, including the existing metadata that has no mechanism for preventing replays.

I'm saying that it is too early to know what the "best" approach will be, if there even is one, and we should avoid painting ourselves into a corner by deciding that the "right" thing to do is definitely to use "depositReturnAddress" which is likely insufficient for all the types of governance metadata, etc.

I'm saying that we should write down and document the different approaches that you might use to solve this problem precisely somewhere, along with the strengths and weaknesses of each (including documenting non-solutions), and provide some best practices for tools to adhere to, such as how to validate each type, which ones to prefer over others if producing governance metadata, and what to do if multiple are present, if they conflict, or if none of them are present.

Doing so acknowledges the messyness of the real world fact that people can publish whatever metadata they want, and that the more we try to decide what people must do, the more we will turn a blind eye to this fact, and the more brittle the tooling ecosystem will become.

@gitmachtl
Copy link
Contributor Author

I have never said that what i wrote is the "best" solution. I always focus on pratical solutions. It may also come down to the POV if you wanna have a self contained solution or if you rely on other online tools too as a decision maker.

We are on the self page when it comes to "mandate" something, tooling is free to use whatever they think its appropriate to check.

I am biased that i see things from the POV as an SPO, true, thats why i seek for solutions that would work on that level too.

The overall goal would be to have this all somehow baked into the ledger itself to prevent bad behavior directly on that level. Lets keep that for the future 😄

I think we can agree to put things into a new CIP which adds more specific fields to the existing metadata with more or less user accessable data on the time of verification and the used tooling. Every tool can do its best to present valid information to the user based on its own capability of the design of the env itself its running in.

@gitmachtl gitmachtl changed the title Change required fields for CIP-100/108/119 to prevent imposers and metadata copy New CIP to add security fields to existing CIP-108 to prevent imposers and metadata copy Feb 2, 2025
@gitmachtl
Copy link
Contributor Author

gitmachtl commented Feb 2, 2025

Not sure how the workflow to take the serialized_gov_action also into the metadata would work.

The anchorURL and anchorHASH is part of the gov_action itself, like:

84                                        # array(4)
   1b 000000174876e800                    #   unsigned(100,000,000,000)
   58 1d                                  #   bytes(29)
      e0c13582aec9a44fcc6d984be003c505    #     "\xe0\xc15\x82\xae\xc9\xa4O\xccm\x98K\xe0\x03\xc5\x05"
      8c660e1d2ff1370fd8b49ba73f          #     "\x8cf\x0e\x1d/\xf17\x0f\xd8\xb4\x9b\xa7?"
   83                                     #   array(3)
      01                                  #     unsigned(1)
      f6                                  #     null, simple(22)
      82                                  #     array(2)
         0a                               #       unsigned(10)
         00                               #       unsigned(0)
   82                                     #   array(2)
      78 30                               #     text(48)
         68747470733a2f2f6d792d69702e6174 #       "https://my-ip.at"
         2f746573742f4349503131392d657861 #       "/test/CIP119-exa"
         6d706c652d7369676e65642e6a736f6e #       "mple-signed.json"
      58 20                               #     bytes(32)
         7318b0fa374b889c79ad18848afd87c5 #       "s\x18\xb0\xfa7K\x88\x9cy\xad\x18\x84\x8a\xfd\x87\xc5"
         0db3d3002f2f9e4c9e8b6112e765ef09 #       "\r\xb3\xd3\x00//\x9eL\x9e\x8ba\x12\xe7e\xef\t"

So you can't know the hash of it before you put a hash of this info again into the metadata itself that is referenced in the gov action itself 😄

Or do you mean without the last info about the anchorURL/HASH itself?

@Quantumplation
Copy link
Contributor

I wasn't claiming you believed you had the best solution either, just that going hard on any one solution is probably a mistake.

Also, what you posted is the serialized proposal_procedure, not the gov_action

@gitmachtl
Copy link
Contributor Author

gitmachtl commented Feb 2, 2025

So including both the reward_account and the gov_action part would make it even better?

hash: [ reward_account, gov_action ]

I have to check with the api/cli team, i think there is currently no easy way to get the raw cbor for a gov action on the cli. But that could be maybe added to the new cardano-cli conway query proposals command.

@Quantumplation
Copy link
Contributor

The reward_account is included in the treasury withdrawal gov action, no need to include it again: https://github.com/IntersectMBO/cardano-ledger/blob/master/eras/conway/impl/cddl-files/conway.cddl#L689

@gitmachtl
Copy link
Contributor Author

That are the accounts to receive payout from the treasury withdrawals, not the rewardAccount that receives the depositAmount. Its called reward_account in the proposal.

@Quantumplation
Copy link
Contributor

Ah, I see. I was thinking you meant the address that the treasury withdrawal was paid to, which wouldn't apply to all governance actions. Let me think for a bit, I'm on the fence about whether both are needed

@rphair rphair changed the title New CIP to add security fields to existing CIP-108 to prevent imposers and metadata copy New CIP to add security fields to existing CIP-108 to prevent imposters and metadata copy Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants