Skip to content

feat(relay): Add a serialization crate for bounded deserializers - #6280

Open
klochek wants to merge 3 commits into
masterfrom
christopherklochek/relay_serializer_serde
Open

feat(relay): Add a serialization crate for bounded deserializers#6280
klochek wants to merge 3 commits into
masterfrom
christopherklochek/relay_serializer_serde

Conversation

@klochek

@klochek klochek commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@klochek
klochek requested a review from a team as a code owner July 31, 2026 19:06
deserialize_seq(),
deserialize_map(),
deserialize_identifier(),
deserialize_ignored_any(),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: The deserialization budget does not account for operations spent on ignored/unknown fields, allowing an attacker to bypass the max_ops limit and cause a DoS with a large, nested unknown field.
Severity: HIGH

Suggested Fix

Ensure that skipping ignored data is also metered. This could be achieved by deserializing the ignored value into a temporary structure like serde_json::Value and then counting its operations, or by implementing a custom visitor that recursively consumes and counts the elements within the ignored data instead of using the unmetered deserialize_ignored_any function.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: relay-serialization/src/serde/de.rs#L213

Potential issue: The deserialization metering logic is intended to prevent DoS attacks
by limiting the number of operations via `max_ops`. However, when deserializing a struct
that does not use `#[serde(deny_unknown_fields)]`, any unknown fields are processed by
`deserialize_ignored_any()`. This function in the underlying `serde_json` deserializer
skips parsing the field's value without triggering the metered visitor methods. As a
result, an attacker can craft a payload with a large, deeply nested JSON object in an
unknown field, causing significant parsing work that is not counted against the
`max_ops` budget, thus circumventing the intended DoS protection.

Did we get this right? 👍 / 👎 to inform future reviews.

Comment thread relay-serialization/src/serde/de.rs

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 8b0c917. Configure here.

// The payload charges itself, an `Option` only adds its discriminant on top.
self.inner
.visit_some(MeteredDeserializer::new(self.meter, d))
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Option Some skips discriminant cost

Low Severity

visit_some comments that an Option adds its discriminant cost on top of the payload, but never calls meter.spend. visit_none does charge, so Some values are under-counted relative to the stated model and to None.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 8b0c917. Configure here.

Comment on lines +195 to +196
deserialize_i128(),
deserialize_u8(),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

MeteredDeserializer operation budget under-counts memory for variable-sized values

The operation budget charges only unit cost for variable-length strings and bytes, but the underlying deserializer allocates them before the meter sees the value. A payload with a small operation count and large strings or byte buffers causes unbounded memory growth.

Evidence
  • forward! macro (line 142) wraps every visitor in a MeteredVisitor, including deserialize_string, deserialize_str, deserialize_bytes, and deserialize_byte_buf.
  • The inner deserializer (e.g., serde_json) parses and allocates the full String or Vec<u8> before calling the visitor method (e.g., visit_string or visit_byte_buf).
  • MeteredVisitor::visit_string (line 281), visit_str (line 271), visit_byte_buf (line 296), and visit_bytes (line 286) each charge only cost::UNIT (1 operation) via meter.spend(cost::UNIT)?, with no adjustment for the allocated size.
  • Because the inner deserializer allocates before the meter's spend check, even a tiny max_ops budget (e.g., 3) will permit the allocation to complete; the meter only rejects after the cost is already paid.
  • No byte-size or allocation cap complements the operation budget, so a JSON payload like ["<100MB string>"] costs ~3 operations but allocates ~100 MB before hitting the budget.
Also found at 1 additional location
  • relay-serialization/src/serde/de.rs:302-311

Identified by Warden · wrdn-dos-review · 2GS-783

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.

1 participant