Skip to content

Move option to emit struct names to PrettyConfig#329

Merged
kvark merged 2 commits into
ron-rs:masterfrom
cswinter:struct-names
Nov 2, 2021
Merged

Move option to emit struct names to PrettyConfig#329
kvark merged 2 commits into
ron-rs:masterfrom
cswinter:struct-names

Conversation

@cswinter

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

@kvark kvark left a comment

Copy link
Copy Markdown
Collaborator

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

malte-v commented Nov 14, 2021

Copy link
Copy Markdown

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

kvark commented Nov 15, 2021

Copy link
Copy Markdown
Collaborator

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

malte-v commented Nov 15, 2021

Copy link
Copy Markdown

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

malte-v commented Nov 15, 2021

Copy link
Copy Markdown

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