feat: add SecRequestBodyJsonDepthLimit directive #1110
Conversation
Signed-off-by: Felipe Zipitria <[email protected]>
Signed-off-by: Felipe Zipitria <[email protected]>
fzipi
force-pushed
the
feat/SecRequestBodyJsonDepthLimit
branch
from
7661bcb to
d176401
Compare
| readItems(json, key, res) | ||
| return res, nil | ||
|
|
||
| if !gjson.Valid(s.String()) { |
There was a problem hiding this comment.
do we really need to do this? the first impression is this is very expensive, the second one is when we use a low body limit this will always be invalid isn't/
There was a problem hiding this comment.
It is not very expensive. Of course, it is more expensive than not doing vlaidation (adds roughly 33% more time in my benchmarks).
The upstream library says that If you are consuming JSON from an unpredictable source then you may want to validate prior to using GJSON. I agree, for the case of a WAF.
Or we can forget about validation, and see if someone can bypass us. 🤷 If we do, we should document clearly this decision that we prefer speed over security here.
| @@ -77,7 +90,11 @@ func readItems(json gjson.Result, objKey []byte, res map[string]string) { | |||
| var val string | |||
| switch value.Type { | |||
| case gjson.JSON: | |||
| readItems(value, objKey, res) | |||
| // call recursively with one less item to avoid doing infinite recursion | |||
| iterationError = readItems(value, objKey, maxRecursion-1, res) | |||
There was a problem hiding this comment.
shouldn't it be if maxRecursion == -1 { return maxRecursion } else { return maxRecursion -1 }
There was a problem hiding this comment.
The readItems func is called always with the maxRecursion value > 0. Then each iteration will decrement this value. I'm not following your question :/
| @@ -31,7 +34,7 @@ func (js *jsonBodyProcessor) ProcessRequest(reader io.Reader, v plugintypes.Tran | |||
|
|
|||
| func (js *jsonBodyProcessor) ProcessResponse(reader io.Reader, v plugintypes.TransactionVariables, _ plugintypes.BodyProcessorOptions) error { | |||
| col := v.ResponseArgs() | |||
| data, err := readJSON(reader) | |||
| data, err := readJSON(reader, ResponseBodyRecursionLimit) | |||
There was a problem hiding this comment.
I think it should never be -1 and always force a limit.
There was a problem hiding this comment.
So we are assuming we don't trust the downstream service neither?
The directive takes into account that the request might be crafted so it creates a DoS of some kind on the body processor (as it happened in ModSecurity). Will the response value be configurable, and different from the request?
Signed-off-by: Felipe Zipitria <[email protected]>
Signed-off-by: Felipe Zipitria <[email protected]>
|
ping @jcchavezs |
what
SecRequestBodyJsonDepthLimitwhy