Verify Move dependencies on publish

[stella3d]

Add verification of local Move dependencies against the on-chain bytecode at their resolved addresses.

This should give confidence that all packages published (at least through the official command) rely on the exact same dependencies when published.

Testing

Setup

~Using Devnet~

~At time of writing, the Move libraries on main are compatible with devnet, so you can skip local network setup and test with an existing Devnet wallet~

Now that i've updated from main, the Move lib has changed, so you need to test with a local network if you want the Move packages to match and publish to succeed.

However, if you want to see what happens when they don't match, try to publish the package from this branch to devnet.

Using a local network

• cargo build --release
• cd target/release
• ./sui genesis
• ./sui start

in another shell tab:

• ./sui-node --config-path ~/.sui/sui_config/fullnode.yaml

Running

The crate includes a test Move package that depends on MoveStdLib and Sui.

• cd target/release
• ./sui client publish --path ../../crates/sui-bytecode-src-verifier/test/dependency_verify_test --gas-budget 30000

You should see dependencies' on-chain bytecode successfully verified printed before the transaction summary.

Speed

On a local network, I measured time to successful verification for the test package (2 packages with 36 modules total) 7 times and took the mean: 0.6 seconds, including the time to compile the package being published.

measured with gnomon:

./sui client publish --path ../../crates/sui-bytecode-src-verifier/test/dependency_verify_test --gas-budget 30000 | gnomon
   0.5151s   dependencies' on-chain bytecode successfully verified

This should scale roughly linearly with the number of packages, since a lot of time will be spent in round trips to the network to fetch on-chain bytecode. To speed this up in the future, we can batch the object requests.

It's probably fast enough as-is without the batching, even for larger dependency sets, just due to the likely infrequence of publishing to the network. If we assume linear scaling & 0.4s per dependency package, even with 100 dependency packages (an extreme amount), it would only take ~40 seconds + compile time

[github-actions[bot]]

💳 Wallet Extension has been built, you can download the packaged extension here: https://github.com/MystenLabs/sui/actions/runs/3202286451#artifacts

[stella3d]

Here's what you currently see if a module didn't match when publishing. I think we could make this clearer / friendlier, but i'm not sure the best way of doing so.

I tried adding a custom hex-serializing implementation of Debug for DependencyVerificationError, but it was a lot of code just to get one enum variant to serialize to hex vs. just using the derived implementation.

[stella3d]

@todd-mystenlabs i don't think flaky is a useful description here. Actually, it works great.

Yes, it needs to add version information for the Move compiler. However, we currently don't seem to include that anywhere in our build configuration objects, so that's not in the scope of this task. So, we should add that.

[tnowacki]

@todd-mystenlabs i don't think flaky is a useful description here. Actually, it works great.

Yes, it needs to add version information for the Move compiler. However, we currently don't seem to include that anywhere in our build configuration objects, so that's not in the scope of this task. So, we should add that.

Talked to @stella3d offline.

The issue is a scenario when the compiler changes. For example:

• Someone publishes module on chain using Sui with Move at rev X
• We update the Sui repo and point to Move at rev Y where the compiler has changed slightly (new optimizations, changed block layout, etc)
• The tool will now always erroneously give an DependencyVerificationError error, blocking any publishing

In order to be able to check this in all scenarios, we would need to know the version of the Move repo that was used to compile the code. Long term, we could have some nice versioning story like rustup, but we aren't there yet.

In the meantime, we can at least used the NormalizedModule to quickly check if anything the module publicly exposes has changed. This will check for everything we care about but code logic changes (from changes in the bodies of functions, private functions, or constants).

[sblackshear]

We update the Sui repo and point to Move at rev Y where the compiler has changed slightly (new optimizations, changed block layout, etc)

This is definitely an issue in the long term. But we are not changing the compiler much (in fact, at all to my knowledge, other than the recent bugfix + the upcoming addition of new numeric types these days). My preference would be to accept this as a risk for now (especially given the lack of upcoming changes) w/ a "skip-validation" flag as a workaround + prioritize compiler versioning as part of the source discovery workstream (even if it's rudimentary at first).

In the meantime, we can at least used the NormalizedModule to quickly check if anything the module publicly exposes has changed. This will check for everything we care about but code logic changes (from changes in the bodies of functions, private functions, or constants).

This is a good workaround idea, but I worry that we'll forget about this and it will become a security issue. Whereas a version of source validation that is flaky in the presence of compiler version changes will force us to remember what we have not yet finished :).

[sblackshear]

It would be great to also have a test for a dependency on a third-party package

[tnowacki]

This is definitely an issue in the long term. But we are not changing the compiler much (in fact, at all to my knowledge, other than the recent bugfix + the upcoming addition of new numeric types these days)

Maybe I'm a bit overly paranoid about this because of the recent changes. Small innocuous things like the optimization bug fix, or the forthcoming work on CFGs will break it. I'm mostly worried because in the face of these changes, we then have no choice but suppress the error. Maybe in the case that there is an error we could check the NormalizedModule for a more detailed error message? If the normalized module changed, something definitely changed in the source. If it didn't, we are back to not knowing.

My preference would be to accept this as a risk for now (especially given the lack of upcoming changes) w/ a "skip-validation" flag as a workaround + prioritize compiler versioning as part of the source discovery workstream (even if it's rudimentary at first).

Concerns aside, I am okay with this as long as the error message clearly states that either (A) the source has changed or (B) the compiler has changed. And then it also gives the flag you need to suppress the error

[amnn]

My 2c is to err on the side of being overly conservative now, and then relax over time, but given how conservative this will be, to work on those improvements sooner rather than later.

Going into the scenarios where verificiation should and shouldn't flag a difference, I can think of the following, let me know what you think:

• Should when the ABI changes.
• Should when the behaviour in the implementation changes, even if the ABI remains the same.
• Should when the behaviour in a dependency changes, even if the implementation is otherwise the same.
• Should not when the translation from source code to bytecode changes (i.e. the opcodes change but the semantics do not).
• Should not when only comments, whitespace, etc have changed in the source code.

Based on this, would we be comfortable with guaranteeing that for a given major version of the Move compiler, being fed the same source code, modulo whitespace and comments, will result in semantically equivalent bytecode? This seems reasonable to me, but I wanted to check.

If we can, then I would propose that verification (in the long-run, not to change in this diff) take a form similar to this:

• Add two new fields to the bytecode format that the move compiler populates:
  • Move compiler major version
  • A hash incorporating all dependency module hashes, and a hash of the AST (think Merkle Tree, but over the Move AST).
• When publishing, we verify the source by matching the compiler versions, and then hashing the AST of the source we are building against and checking whether this matches the bytecode.

Let me know your thoughts on this approach, and some of the related open questions:

• How do we prevent a malicious actor from publishing bytecode with the wrong compiler version or content hash?
• Do we even have a concept of "major version" for the move compiler?
• Is this applicable/useful to other chains that use Move? It would be great to standardise on an approach here to support cross-chain package management.

[tnowacki]

Should when the behaviour in a dependency changes, even if the implementation is otherwise the same.

This one should be done automatically for us, as we should have all transitive deps in this step.

Should not when the translation from source code to bytecode changes (i.e. the opcodes change but the semantics do not).

Agreed but thoughts on specifications here?

Should not when only comments, whitespace, etc have changed in the source code.

Somewhat similarly, we had debated this in the past for comments, as it could be used to make misleading statements. Hyperbolically, first version of code (either via specification or comment) "WARNING: This will steal all your money!" second version of code "This function is perfectly safe!"

Based on this, would we be comfortable with guaranteeing that for a given major version of the Move compiler, being fed the same source code, modulo whitespace and comments, will result in semantically equivalent bytecode? This seems reasonable to me, but I wanted to check.

Sure, but we don't really have any good versioning system for the compiler right now. It will also require some more work if/when we have optimization flags.

Add two new fields to the bytecode format that the move compiler populates:

We can do that with the metadata without requiring any changes to the file format, which is awesome! We have talked about adding something similar to these in the past

How do we prevent a malicious actor from publishing bytecode with the wrong compiler version or content hash?

We cannot. It is helpful information to have, but it will need to be validated.

Is this applicable/useful to other chains that use Move? It would be great to standardise on an approach here to support cross-chain package management.

Agreed. We could add the layout of these fields for the metadata to the main Move repo. The whole metadata thing is just a map of vector<u8> -> vector<u8>, so we would need serialization formats here.

[amnn]

Agreed but thoughts on specifications here?

So this is what the major version + hash of AST + hash of dependencies combination was supposed to cover (under an assumption that source language or ABI changes are major-version-breaking), but the inability to trust that in the bytecode is a pretty major wrinkle. I'll ask some of our crypto experts about it next week!

Somewhat similarly, we had debated this in the past for comments, as it could be used to make misleading statements. Hyperbolically, first version of code (either via specification or comment) "WARNING: This will steal all your money!" second version of code "This function is perfectly safe!"

I had considered this too, but landed on not including the comments because we don't do anything to verify those comments when they get published in the first place (i.e. there is nothing stopping someone publishing a function that will steal everyone's money, but commenting on it that it's fine) so we can't effectively protect people from this using this form of source verification.

Sure, but we don't really have any good versioning system for the compiler right now.

Yep, this is an issue -- this is an area we can be conservative with at first (i.e. start with any compiler version being breaking, and then relax over time.

It will also require some more work if/when we have optimization flags.

Could you elaborate on this point? I was hoping that by identifying the source with the above scheme (version + hashes) we would get extra freedom to change up optimisations in the compiler that only affected the bytecode and not the ABI.

[oxade]

Maybe I missed this in the comments but what's wrong with hashing the generated bytecode? It really doesn't change much between versions. And if its changing often, this is a problem we can and should solve with better versioning logic

[stella3d]

Maybe I missed this in the comments but what's wrong with hashing the generated bytecode? It really doesn't change much between versions. And if its changing often, this is a problem we can and should solve with better versioning logic

Nothing is wrong with it i think - do we want it for this PR ?

[stella3d]

OK, all code issue should be updated. had to do some updating to account for recent changes in build APIs as well.

Tested the whole flow of publishing the test package to a local network and got what i expect:

[amnn]

Also, it's good that this PR has a test plan, but can we check in a test of some kind?

EDIT just to clarify -- I see the test code in the PR, but IIUC, to use it, someone needs to set-up a local DevNet and publish the package to it. I'm hoping for a test such that will run automatically when someone invokes cargo simtest at the root of the sui repo.

[stella3d]

Also, it's good that this PR has a test plan, but can we check in a test of some kind?

EDIT just to clarify -- I see the test code in the PR, but IIUC, to use it, someone needs to set-up a local DevNet and publish the package to it. I'm hoping for a test such that will run automatically when someone invokes cargo simtest at the root of the sui repo.

what specifically would you like to see tested?

mostly it lacks an automatic test right now because i didn't see how to mock the sui client without expanding the scope of this a lot.

[amnn]

Yes, we can close this now!