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

crypto-common: add SerializableState trait #1369

Merged
merged 9 commits into from
Nov 18, 2023

Conversation

rpiasetskyi
Copy link
Contributor

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

Original PRs: #1078 #1361

Original issue: RustCrypto/hashes#310

@newpavlov
Copy link
Member

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
Copy link
Member

@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
Copy link
Contributor Author

rpiasetskyi commented Nov 13, 2023

@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
Copy link
Member

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 tarcieri requested a review from newpavlov November 15, 2023 20:16
@tarcieri
Copy link
Member

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.

@tarcieri tarcieri merged commit 9fbd4f9 into RustCrypto:master Nov 18, 2023
15 checks passed
@burdges
Copy link

burdges commented Dec 16, 2023

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
Copy link
Member

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
Copy link

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
Copy link

burdges commented Jan 25, 2024

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

@tarcieri
Copy link
Member

@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
Copy link
Member

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

tarcieri pushed a commit to RustCrypto/hashes that referenced this pull request Mar 19, 2024
This is a rebase of #385 on master.

RustCrypto/traits#1369 introduced a
`SerializableState` trait to serialize the hasher. This implements it
for each of the hashers we have.

Co-authored-by: Ruslan Piasetskyi <[email protected]>
@ikrivosheev
Copy link

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

@wiktor-k
Copy link

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

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

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 this pull request may close these issues.

6 participants