crypto-common: add SerializableState trait

[rpiasetskyi]

Trait for saving the internal state of the object and restoring it later.

Original PRs: #1078 #1361

Original issue: RustCrypto/hashes#310

[newpavlov]

I am not sure about the derive crate. It adds a significant amount of code and could be dangerous in respect to serialization output stability. I think we can afford to write serialization by hand on a case-by-case basis?

[tarcieri]

@rpiasetskyi could you remove the custom derive for now so we can just focus on the core traits? That would help make it easier to review.

[rpiasetskyi]

@tarcieri sorry for the delay.

@newpavlov I would still recommend using it in most cases because it helps to avoid unnecessary code duplication and makes it easy and safe. It works as serde: serializes members one by one if they implement SerializableState trait.

But some for structures it might be better to implement manually.

could be dangerous in respect to serialization output stability

Could you please explain your concerns?

[newpavlov]

Could you please explain your concerns?

Reordering of fields or changing types (e.g from [u32; 2] to u64) can break output compatibility of derived serialization. Also, derive macros have a noticeable negative impact on compilation times, so if we are to implement the traits by default, I would prefer to have manual implementations.

[tarcieri]

Going to merge this as it's a frequently requested feature.

Since crypto-common is currently a prerelease anyway, there's time to iterate on the API before a final release.

[burdges]

Is this hazmat? It's well beyond the typical analysis of course.

Inherent methods might not be dangerous for specific hash functions, but in general you'd expect serializing the internal state breqaks assumptions when used, or even permits collisions. At least the warning should read stronger:

Analysis of cryptographic primitives rarely if ever consideres serialized state, so employing serialized state would typically break the security of the underlying primitive: Serialized state would tyically leak sensitive data. A digest or cipher loses its randomness or collisions resistance properties.

We advise that serialized state only be used in non-advarsarial layers of the protocol.

[tarcieri]

I'd say there's quite a bit of misuse potential. It could definitely use some warnings.

We could also gate implementations of the trait under a hazmat feature in each crate (though thanks to feature unification, such features are easily flipped on by other dependencies)

[wiktor-k]

Hi,

As I'm interested in this feature I started testing the pre versions.

Are there any examples on how to use SerializableState with concrete crates?

I wrote this small example using sha2 = "0.11.0-pre.2":

fn main() {
    use sha2::digest::crypto_common::SerializableState;
    use sha2::Digest;
    let mut hasher = sha2::Sha256::new();
    hasher.update(b"hello ");

    let result = hasher.serialize();
}

And the compilation error looks pretty opaque to me:

error[E0599]: the method `serialize` exists for struct `CoreWrapper<CtVariableCoreWrapper<Sha256VarCore, UInt<UInt<UInt<..., ...>, ...>, ...>, ...>>`, but its trait bounds were not satisfied
  --> src/main.rs:7:25
   |
7  |     let result = hasher.serialize();
   |                         ^^^^^^^^^ method cannot be called due to unsatisfied trait bounds
   |
  ::: /home/wiktor/.cargo/registry/src/index.crates.io-6f17d22bba15001f/digest-0.11.0-pre.7/src/core_api/ct_variable.rs:30:1
   |
30 | pub struct CtVariableCoreWrapper<T, OutSize, O = NoOid>
   | ------------------------------------------------------- doesn't satisfy `_: SerializableState`
   |
  ::: /home/wiktor/.cargo/registry/src/index.crates.io-6f17d22bba15001f/digest-0.11.0-pre.7/src/core_api/wrapper.rs:30:1
   |
30 | pub struct CoreWrapper<T: BufferKindUser> {
   | ----------------------------------------- doesn't satisfy `_: SerializableState`
   |
  ::: /home/wiktor/.cargo/registry/src/index.crates.io-6f17d22bba15001f/sha2-0.11.0-pre.2/src/core_api.rs:22:1
   |
22 | pub struct Sha256VarCore {
   | ------------------------ doesn't satisfy `Sha256VarCore: SerializableState`
   |
   = note: the full type name has been written to '/home/wiktor/tmp/cargo/debug/deps/hasher_test-d2968b8c355f072e.long-type-1048706664379593364.txt'
   = note: the following trait bounds were not satisfied:
           `CtVariableCoreWrapper<Sha256VarCore, UInt<UInt<UInt<UInt<UInt<UInt<UTerm, B1>, B0>, B0>, B0>, B0>, B0>, OidSha256>: SerializableState`
           which is required by `CoreWrapper<CtVariableCoreWrapper<Sha256VarCore, UInt<UInt<UInt<UInt<UInt<UInt<UTerm, B1>, B0>, B0>, B0>, B0>, B0>, OidSha256>>: SerializableState`
           `Sha256VarCore: SerializableState`
           which is required by `CoreWrapper<CtVariableCoreWrapper<Sha256VarCore, UInt<UInt<UInt<UInt<UInt<UInt<UTerm, B1>, B0>, B0>, B0>, B0>, B0>, OidSha256>>: SerializableState`

Thanks for help! 👋

[burdges]

An option, you could give either the trait or its methods scary names, like InsecureSerializableState.

[tarcieri]

@burdges for other things like this we've put it in a hazmat module with appropriate module-level documentation, which seems like it would work here as well.

We should probably also add documentation about potential misuses to each of the traits as well.

[tarcieri]

Here's a PR to place all of the traits inside a hazmat module: #1487

[ikrivosheev]

@wiktor-k I got same compile error. Do you find way to fix it?

[wiktor-k]

I got same compile error. Do you find way to fix it?

It should be fixed by this PR: RustCrypto/hashes#574 (comment)