Is this secure?

[gretchenfrage]

I just looked through the code a bit, and I was amazed when I saw that you're bypassing the need to register traits by actually making a virtual function call into the vtable pointer. I wouldn't have thought of that. My question is, is this secure, and is it safe?

It seems like attempting to deserialize a bad vtable pointer, intentionally or unintentionally, could allow the execution of arbitrary memory regions, which seems like both a massive security flaw, and unsafe in that it could cause undefined behavior. Am I wrong about this?

It seems like, unless there's some verification that I missed, this needs to be addressed.

[alecmocatta]

Good question!

Short answer: No, this approach is not yet secure against malicious actors. However, if we assume non-malicious actors and typical (static or dynamic) linking conditions, then it's not unreasonable to consider it sound.

Longer answer:

Three things are serialized alongside the vtable pointer for the purpose of validation:

• the build_id (128 bits);
• the type_id of the trait object (64 bits);
• the type_id of the concrete type (64 bits).

At some point in Rust's future, I think it would be great if the latter could be used to safely look up and create a trait object. As it is, that functionality doesn't exist yet, so what this crate does instead is serialize the vtable pointer (relative to a static base), and do as much validity checking as it reasonably can before it can be used and potentially invoke UB.

The first two are checked for validity before usage of the vtable pointer. The build_id ensures that the vtable pointer came from an invocation of an identically laid out binary¹. The type_id ensures that the trait object being deserialized is the same type as the trait object that was serialized. They ensure that under non-malicious conditions, attempts to deserialize invalid data return an error rather than UB. The type_id of the concrete type is used as a sanity check that panics if it differs from the type_id of the concrete type to be deserialized.

Regarding collisions, the 128 bit build_id colliding is sufficiently unlikely that it can be relied upon to never occur. The 64 bit type_id colliding is possible, see rust-lang/rust#10389, though exceedingly unlikely to occur in practise.

The vtable pointer is (de)serialized as a usize relative to the vtable pointer of this static trait object. This enables it to work under typical dynamic linking conditions, where the absolute vtable addresses can differ across invocations of the same binary, but relative addresses remain constant.

All together this leaves, as far as I'm aware, three soundness holes:

• A malicious user with a copy of the binary could trivially craft a build_id and type_id that pass validation and gives them control of where to jump to.
• Data corruption of the serialized vtable pointer but not the build_id or type_id used for validation, resulting in a jump to an arbitrary address. This could be rectified in a future version of this library by using a cipher to make it vanishingly unlikely for corruptions to affect only the vtable pointer, by mixing the vtable pointer and validation components upon (de)serialization.
• Dynamic linking conditions where the relative addresses (vtable - static vtable) are different across different invocations of the same binary. I'm sure this is possible, but it's not a scenario I've encountered so I can't speak to its commonness.

¹I don't think this requirement is strictly necessary, as the type_id should include all information that could affect soundness (trait methods, calling conventions, etc), but it's included in case that doesn't hold in practise; to provide a more helpful error message; and to reduce the likelihood of collisions.

---

The documentation definitely needs to include this information so I've gone ahead and added it.

My use case includes serializing trait objects where the traits are implemented on generic types, which I don't believe is compatible with the typetag approach of registering traits and their impls on concrete types. Hence this crate. Any suggestions for how to make it more secure and more sound are very welcome!

[gretchenfrage]

Thank you for addressing this!

I have an idea for a typetag-style system that would allow the serialization of parametric traits -- if you'd like, I'll message you if I get that figured out.

[alecmocatta]

@gretchenfrage I'd love to hear if you've made any progress on that?