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

[Move] Properly allow templated type arguments in entry functions #348

Closed
lxfind opened this issue Feb 3, 2022 · 14 comments · Fixed by #469
Closed

[Move] Properly allow templated type arguments in entry functions #348

lxfind opened this issue Feb 3, 2022 · 14 comments · Fixed by #469
Assignees

Comments

@lxfind
Copy link
Contributor

lxfind commented Feb 3, 2022

diem/move#38 adds support for templated type arguments in entry functions. However this opens up an integrity issue: a caller could call a templated function with a pure argument that could deserialize to an object. By doing so, caller could create objects out of thin air.

One solution is as follows:
So we want authority to always kick in for any templated args so that they are properly checked. In that case, we could require T to always be T: key for any templated entry args (through a bytecode verifier). Then when we resolve types in adapter, we enforce that pure args cannot be templated.

@sblackshear
Copy link
Collaborator

we could require T to always be T: key for any templated entry args (through a bytecode verifier).

I think we might need something a bit more nuanced. For concreteness, here are some examples of entrypoints that (I think) should be labeled good vs bad by our approach

a<T: key>(t: T) // good
b<T>(t: T) // bad
c<T>(coin: Coin<T>) // good
d<T1: key, T2>(t1: T1, coin: Coin<T2>) // good
e<T: key>(o: Object, x: u64, t: T) // bad
f<T: key>(w: Wrapper<T>) // good if Wrapper has key
g<T: key>(objects: vector<T>) // good?

Wanted to double-check that the proposed approach will accept c and d.

@lxfind
Copy link
Contributor Author

lxfind commented Feb 3, 2022

Right. T: key only needs to be enforced when it's a direct replacement of a argument type.

@gdanezis gdanezis added the move label Feb 4, 2022
@lxfind lxfind added this to the GDC milestone Feb 5, 2022
@awelc awelc self-assigned this Feb 11, 2022
@awelc
Copy link
Contributor

awelc commented Feb 15, 2022

I started looking at the verifier to see how to implement these checks, but then I realized that we can't determine if a given function is an entry point at the verifier level and enforcing this for all functions is too much, isn't it?

This should then be implemented as part of resolve_and_type_check in the adapter, right?

@lxfind
Copy link
Contributor Author

lxfind commented Feb 15, 2022

We can. A entry function has the following properties today (we could change it in the future, though):

  1. is public
  2. No return value
  3. Parameters are ordered as following: objects, primitives, &mut TxContext

@awelc
Copy link
Contributor

awelc commented Feb 15, 2022

Cool. I still wonder where this check should be placed. When calling a Move function (in resolve_and_type_check) we already verify that the entry point function has the right number/type of parameters, so perhaps it would make sense to extend this check rather than repeating some of the same functionality in the verifier, even if we pay the cost of extra checks each time a function is called?

Or do we rather follow a rule/guideline that if a check can be done at publishing time in the verifier, it should, particularly that the cost of extra checks is likely negligible compared to the cost of the whole publishing procedure?

@lxfind
Copy link
Contributor Author

lxfind commented Feb 15, 2022

Yeah as you mentioned, it's a lot cheaper to check at publish time as it only needs to be verified once, comparing to checking it every time during execution. More importantly, if we know some code is invalid, it would be somewhat confusing to allow publishing it onchain in the first time (and never get executed)

@awelc
Copy link
Contributor

awelc commented Feb 16, 2022

Got it. I think we may still have a problem doing this at the verifier level. Consider the following function:

public fn <T: key>(o: Object, x: u64, t: T, &mut TxContext) {

}

While this is not a valid entry function, it is still a valid "general" Move function. It's likely too restrictive to forbid similar functions from being published as they may be exclusively called by other functions and never appear as the entry ones. The risk with these types of functions is only when they are called from Sui via the MoveVM "bridge", right?

@lxfind
Copy link
Contributor Author

lxfind commented Feb 16, 2022

While this is not a valid entry function, it is still a valid "general" Move function. It's likely too restrictive to forbid similar functions from being published as they may be exclusively called by other functions and never appear as the entry ones. The risk with these types of functions is only when they are called from Sui via the MoveVM "bridge", right?

That's correct.

@awelc
Copy link
Contributor

awelc commented Feb 16, 2022

What this means, though, is that we cannot identify entry functions at the verifier level by applying the 3-property check. In other words, the function from the previous example is or is not correct (and should or should not be rejected) depending on whether it is or is not called from Sui.

@lxfind
Copy link
Contributor Author

lxfind commented Feb 16, 2022

Why cannot we identify entry functions at the verifier level by applying the 3-property check?

@awelc
Copy link
Contributor

awelc commented Feb 16, 2022

Why cannot we identify entry functions at the verifier level by applying the 3-property check?

I'd think that generic types would be the problem. For example, is the following function an entry function or not?

public fn <T>(o: Object, x: u64, t: T, &mut TxContext) {

}

Presumably, it would be OK to call it from Sui with T==u64, but not with T being an object.

@lxfind
Copy link
Contributor Author

lxfind commented Feb 16, 2022

Ah yes, you are right that we cannot determine if a function is always a valid entry function under different type instantiations, but we could determine if a function can be an entry function which is what matters, and we could verify that in cases where the function can be an entry function, whether it leads to integrity violations.

On a side note, I think in the future we should remove the parameter order restriction (i.e. primitive and object params should be able to mix, this is just current implementation limitations) cc @sblackshear to confirm on this.

@awelc
Copy link
Contributor

awelc commented Feb 16, 2022

So, just to get the requirements correct for this task - are we OK with rejecting modules that contain functions that COULD be entry functions, and that could cause integrity violations if they were?

It seems a bit weird to disallow publishing modules with functions that would work just fine if they were not used as entry functions.

At the same time, if we allow publishing modules with functions that could never be used as entry functions due to potential integrity violations, it does not mean that these functions can never be called, they just can never be called from Sui, but only from other Move functions.

@lxfind
Copy link
Contributor Author

lxfind commented Feb 16, 2022

We have to reject modules if it is possible to make an entry function call that violate integrity, because once a module is published, anyone can make a call to it.

mwtian pushed a commit that referenced this issue Sep 12, 2022
Bumps [tokio-stream](https://github.com/tokio-rs/tokio) from 0.1.8 to 0.1.9.
- [Release notes](https://github.com/tokio-rs/tokio/releases)
- [Commits](tokio-rs/tokio@tokio-stream-0.1.8...tokio-stream-0.1.9)

---
updated-dependencies:
- dependency-name: tokio-stream
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
mwtian pushed a commit to mwtian/sui that referenced this issue Sep 29, 2022
Bumps [tokio-stream](https://github.com/tokio-rs/tokio) from 0.1.8 to 0.1.9.
- [Release notes](https://github.com/tokio-rs/tokio/releases)
- [Commits](tokio-rs/tokio@tokio-stream-0.1.8...tokio-stream-0.1.9)

---
updated-dependencies:
- dependency-name: tokio-stream
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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

Successfully merging a pull request may close this issue.

4 participants