[cherry-pick] Safe type tags #549
Conversation
There's limited value and a lot of wasted resources by leaving this limited to serde's limits. Recursion is dangerous.
Cherry picked from move-language#529. This is an alternative version of what move-language#528 and the previous for limitation of nesting provides, having a significant lower footprint. This approach installs custom serializers at the recursion points of the `TypeTag` enum. Those serializer use a thread local to track the nesting of the type tag, both for serialization and deserialization. The usage of a thread local is safe and a common technique to provide extra state for implementations which miss it (e.g. serde). A given thread has a single execution stack and this will always count the depth counter consistently up and down. Because Rust has no exceptions are other exit points, this cannot be corrupted. Possibly, thread locals may cause inefficiency. However, we expect TypeTags to be small (a few constructors, limited nestings). The technique used for type tags can be generalized for any other recursive type if needed. Eventually, we hope serde will support per-type nesting, and possibly we should talk with the author if we can add it ourselves. This uses and passes the tests from move-language#528.
There's limited value and a lot of wasted resources by leaving this limited to serde's limits. Recursion is dangerous.
|
Loving the boxing! Was thinking about just that when looking at Just curious, how did you land on |
@davidiw initially came up with this number, and we haven't really discussed it after ;-) I guess a little more than 8 might be better. |
|
I'd feel better if it was a bit higher than 8, especially since this isn't configurable right now. |
|
8 seems ridiculous, but maybe not... 4 seems actually quite reasonable. I would love to see a deeply nested, yet useful type :) |
|
I do not want to arbitrarily restrict the language without some strong motivations. This will be a big change in the sorts of types that are expressible when interacting with storage. The current max type depth at runtime is set fairly high, where no reasonable code will likely hit it ( The type depth here is an indirect measure of the resource intensity (memory usage, time it takes to parse, etc) of the TypeTag, so I'm not sure what problem we are trying to solve here. What I mean by this is something like I'm assuming the concern is one of resource usage. If so, why not bound the length of the bytes before deserializing them? We could probably set that bound fairly high before it causes issues. |
|
Most serializers / deserializers limit the amount of nesting one can enter. This is a defense in depth measure that will undoubtedly impact any service that allows a user to specify this as is and then internally passes around this data structure. I hadn't realized there might be contention here, we should have chatted more on the weekly sync. |
|
I found the nesting of 8 actually also too low. I accepted it because we could always increase without a breaking change. |
|
I don't have a strong conviction on a particular nesting value, but I do think we should check with @jolestar (on StarCoin) and @0o-de-lally (on 0L) to make sure whatever value we pick is not a breaking change for their deployed code (since the previous max was 256). |
| use serde::{Deserialize, Deserializer, Serialize, Serializer}; | ||
| use std::cell::RefCell; | ||
|
|
||
| pub(crate) const MAX_TYPE_TAG_NESTING: u8 = 13; |
I think no one would use a type with a nesting depth of more than 13, but I need to scan through the chain data to ensure. What happens if the user construct a type in the Move code that is deeper than 13? Can't write to external storage? |
I'm not sure whether we store type tags as BCS in storage, but if so, yes. More importantly, this prevents one from having a deeper nested type tag in transaction payloads. This is the source of attacks here. Not a user writes something but someone creates a malicious type tag in a transaction send to the system. See also our telegram discussion. |
Pinged on telegram for more background |
|
Hi Everyone, as I mentioned to @sblackshear 8 would not be an issue for us today. And anywhere between 8 and 16 seems like a generous upper-bound. That said the contention issues seem real but manageable within those limits. |
|
Closing for now, will come back with another PR. |
…nd update move-0L version. move-language/move#549
Motivation
This cherry picks a series of PRs from the
aptosbranch to secure recursive type tags. Type tags are limited in serialization and parsing to not exceed a nesting depth of 8. In addition, it significantly reduces size of TypeTag by boxing the content of a larger enum variant.For details see the individual commits
Test Plan
Tests added in the commits