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

Add configurable recursion limit #162

Closed
erickt opened this issue Oct 26, 2016 · 7 comments
Closed

Add configurable recursion limit #162

erickt opened this issue Oct 26, 2016 · 7 comments

Comments

@erickt
Copy link
Member

erickt commented Oct 26, 2016

We've talked about this before, but apparently we don't have a ticket. We should add a configurable depth counter to allow deserializers to limit how deep of a structure they can deserialize. This would help to avoid stack overflows from maliciously constructed objects.

@frewsxcv
Copy link
Contributor

Relevant issue: serde-rs/serde#82

@dtolnay
Copy link
Member

dtolnay commented Dec 3, 2016

I added a recursion limit of 128 in #163. Are there any use cases that require this to be configurable?

@dtolnay
Copy link
Member

dtolnay commented Jan 7, 2017

Nobody has complained.

@himat
Copy link

himat commented Oct 21, 2020

Hi I would like to possibly complain.

I see that an option to disable the recursion limit was added https://docs.rs/serde_json/1.0.59/serde_json/struct.Deserializer.html#method.disable_recursion_limit

But for my use case, I still want to make sure that there won't be some hugely nested JSON that I would need to deserialize and so I still want a max recursion limit, just one that is higher than the current default.
If I use that disable_recursion_limit function, how would I go about still ensuring that I have a max limit?

@obi1kenobi
Copy link

obi1kenobi/cargo-semver-checks#108 would also very much benefit from a configurable limit.

In particular, a configurable limit is much more palatable than any of the other alternative approaches I can see, listed here: obi1kenobi/cargo-semver-checks#108 (comment)

@wrwg
Copy link

wrwg commented Oct 3, 2022

It would be really nice to provide better protection from serialization attacks by allowing the nesting to be defined on a per-type level. Consider e.g. this example:

#[derive(Serialize, Deserialize)]
struct SystemMessage {
   user_value: SomeUserStruct
 }

Now consider some attacker constructs a user_value exactly at the allowed nesting_depth. When you try to serialize this value embedded in the other value, serialization fails, even though the user value by itself is valid and has been, say, obtained by deserialization. You really like to ensure that SystemMessage has a deeper nesting level than SomeUserStruct.

Also, one can imagine type layouts and encodings where a too deeply nested recursive type leads to a "decompression bomb". That is you have a relative small binary encoding, but if its deserialized it expands to something much larger leading to OOM. In such cases, you'd like to restrict the nesting depth to a significant smaller value than the current default.

@SichangHe
Copy link

I am now complaining, @dtolnay, because the error message confused the heck out of me. And, it was hard for me to find out what exactly was the cause of the error.

I thought this recursion limit was something Rustc injected. Obviously, that was laughable. But, it would be nice to have a better error message and documentation.

Suggestions on error message and documentation:

  • Emphasize, in the error message, that the limit is imposed by serde_json.
  • Hint in the error message that the limit could be lifted using disable_recursion_limit.
  • Mention, in the documentation of serde_json::from_reader and maybe other commonly used functions, that there is an artificial recursion limit.

Suggestions on configurable recursion limit:

  • Maybe increase the limit because it currently is somewhat low. It is hard to judge what number is good, though.
  • Provide an easier way to set the limit without the need to explicitly declare a Deserializer, perhaps something like serde_json::from_reader_no_recursion_limit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

7 participants