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 option to emit struct names to PrettyConfig #329

Merged
merged 2 commits into from
Nov 2, 2021

Conversation

cswinter
Copy link
Contributor

Fixes #200.

@cswinter cswinter force-pushed the struct-names branch 2 times, most recently from efd2a8e to 85a2136 Compare October 29, 2021 02:08
@cswinter cswinter changed the title Struct names Move option to emit struct names to PrettyConfig Oct 29, 2021
Copy link
Collaborator

@kvark kvark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! Also, I believe this is backwards-compatible.
Edit: Actually, no, it's breaking pub fn new(..) API.

@kvark kvark merged commit 62db940 into ron-rs:master Nov 2, 2021
@malte-v
Copy link

malte-v commented Nov 14, 2021

I actually think that this is a bad idea and should be reverted. Emitting struct names is not only a matter of prettiness. Let's say I have two structs, Foo and Bar:

struct Foo { inner: u8 }
struct Bar { inner: u8 }

When serializing a Foo and a Bar with the same inner value, their serialized forms will look the same if struct names are skipped. As a consequence, their cryptographic hashes and signatures will also be the same, so an attacker could take a signed Foo and present it as a signed Bar, even though the two structs may represent totally different information.

Just emitting struct names is programatically (and visually) much nicer than putting all serializeable/signable types into an enum.

malte-v pushed a commit to malte-v/ron that referenced this pull request Nov 14, 2021
malte-v pushed a commit to malte-v/ron that referenced this pull request Nov 14, 2021
@kvark
Copy link
Collaborator

kvark commented Nov 15, 2021

Sorry,I don't understand the argument about cryptographic names and hashes.
You want different types to look different in the serialized form? That's already not the case. 1u8 and 1u32 would look exactly the same.

@malte-v
Copy link

malte-v commented Nov 15, 2021

The fact that 1u8 and 1u32 look the same in serialized form is not a problem because they are semantically the same: They're just integers. Compound types, on the other hand, are often structurally the same but semantically different. That's where emitting struct names comes in handy.

Let me give a more concrete example: you have a kind of decentralized business review platform where each customer has its own keypair and signs reviews:

struct GoodReview(id: BusinessId);
struct BadReview(id: BusinessId);

If you just serialize, hash and sign the above structs without emitting struct names, any valid BadReview will be a valid GoodReview and vice versa. If you emitted struct names, this kind of attack would be impossible.

(In the example above, you'd probably just use an enum instead. But if you have more structs to sign and want absolute peace of mind, you'd have to put all your structs into an enum, which is just a pain to maintain and introduces new kinds of possible errors. Emitting struct names is a much simpler and more elegant solution.)

@malte-v
Copy link

malte-v commented Nov 15, 2021

Another more general argument is that since this has been merged, your PrettyConfig makes a difference that goes beyond aesthetics: It decides whether you will be able to falsely deserialize into a struct that is structurally the same but not identical to the struct that the value was serialized from.

torkleyy pushed a commit to torkleyy/ron that referenced this pull request Jun 6, 2022
* Move struct_names from Serializer to PrettyConfig

* Update changelog
@torkleyy torkleyy mentioned this pull request Jun 6, 2022
2 tasks
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.

How to specify whether to emit optional struct name during serialization?
3 participants