fix: Limit nested form-data entry keys#6179
Conversation
jjbayer
left a comment
jjbayer
left a comment
There was a problem hiding this comment.
dropping, serializing, and normalizing all still happen recursively
Isn't this true for all user-driven recursive data structures that we have?
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| /// | ||
| /// This number needs to be limited due to the recursion taking place when building the JSON object | ||
| /// in [`utils::update_nested_value`], and when serializing, normaliziing, and dropping it. | ||
| const MAX_NESTED_FORMDATA_DEPTH: usize = 15; |
There was a problem hiding this comment.
Bug: The constant MAX_NESTED_FORMDATA_DEPTH is set to 15, but the pull request description and tests indicate an intended limit of 7, making the protection less strict than designed.
Severity: MEDIUM
Suggested Fix
Change the value of the constant MAX_NESTED_FORMDATA_DEPTH from 15 to 7 to align with the stated intention in the pull request description and the associated test case. This ensures the security hardening is as strict as originally designed.
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-server/src/processing/errors/errors/utils/formdata.rs#L11
Potential issue: There is an inconsistency between the intended and implemented nesting
depth limit for form-data. The pull request description states the goal is to "Limit
nested form-data entry keys to 7." However, the constant `MAX_NESTED_FORMDATA_DEPTH` is
defined as 15 in the code. This allows payloads with 8 to 15 levels of nesting to be
processed, whereas the intended behavior was to reject them. While a limit of 15 still
prevents extreme recursion, it is significantly more permissive than intended and may
not fully mitigate the risk of stack overflows from subsequent recursive operations like
serialization and normalization.
Did we get this right? 👍 / 👎 to inform future reviews.
| /// Maximum number of nested `sentry[a][b][c]…` keys accepted inside a form-data key. | ||
| /// | ||
| /// This number needs to be limited due to the recursion taking place when building the JSON object | ||
| /// in [`utils::update_nested_value`], and when serializing, normaliziing, and dropping it. |
There was a problem hiding this comment.
| /// in [`utils::update_nested_value`], and when serializing, normaliziing, and dropping it. | |
| /// in [`utils::update_nested_value`], and when serializing, normalizing, and dropping it. |
| if keys.len() > MAX_NESTED_FORMDATA_DEPTH { | ||
| return Err(ProcessingError::NestingTooDeep); | ||
| } |
There was a problem hiding this comment.
Wonder if we should put this check into update_nested_value so other callers cannot run into same trap. Either hardcoded (with the constant) or as an argument.
3 participants
Limit nested form-data entry keys to 7. The alternative solution of removing
update_nested_value's recursion does not suffice: dropping, serializing, and normalizing all still happen recursively - we need to limit the depth.Fixes INGEST-992