Skip to content

Conversation

@souvlakias
Copy link

Motivation

This library was used in a Web Exploitation challenge for the ECSC 2025 qualifiers of Team Greece and Team Cyprus. Two bugs were found:

  1. Construction of a serializable class using the O type and manual setting of key-value pairs, allowing pollution of the __proto__ attribute.
  2. The application had a class with a sensitive overloaded version of toString that could be triggered by requesting the unserialization of an object with a class instance as a key, which would invoke toString automatically.

Why these matter

  1. The developer should be assured that their classes are unserialized and reconstructed only through the unserialized method they implemented. A user should not be allowed to set the __proto__ attribute during unserialization, as it can lead to unexpected behavior or security issues.
  2. Since keys are always static before serialization, there should not be any dynamic behavior like allowing objects as keys during unserialization.

What this changes

  • Ensures object keys are either number or string, and not equal to __proto__.
  • Ensures serializable classes are not unserialized as O:notserializable-class.

Thoughts

  • In serialize.ts, Map objects are currently converted into plain key-value objects. This causes data loss because Map supports dynamic, non-string keys, but the serialization process coerces them into static string keys.
  • If Map serialization is indeed required, it would be better to implement a custom serialization and unserialization method that preserves key types. For example, by reconstructing the Map using .set() during unserialization.

Copy link
Owner

@steelbrain steelbrain left a comment

Choose a reason for hiding this comment

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

Thank you very much!

I appreciate your contribution. Would you be interested in writing some tests to prevent regression and crediting yourself in CHANGELOG.md? (under the version "Upcoming" for now)

@souvlakias
Copy link
Author

Hey, I put in some tests and updated the changelog. Let me know if anything needs tweaking. (You should be able to edit the branch if you want to make any changes)

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.

2 participants