Add large document benchmarks, tune alias heuristic, add max depth limits#515
Add large document benchmarks, tune alias heuristic, add max depth limits#515niemeyer merged 1 commit intogo-yaml:v2from liggitt:v2-performance-tuning
Conversation
|
cc @niemeyer |
|
This looks simple and reasonable. There are minor non-functional details, but I won't bother you with those. Thanks for doing the proper analysis backing it. |
|
@liggitt @niemeyer This patch partly comprises the k8s fix for CVE-2019-11253, which updates There is certainly an argument of "caveat emptor": that I can assist with CVE assignment for gopkg.in/yaml, if there are no objections. |
|
Shouldn't this also be merged into v3? (And I guess v1?) |
|
This patch is an improvement on a fix that was backported from v3. Now we need to forward port it again. About assigning a CVE to go-yaml, the fix for this is to not accept certain valid documents, so this is a vulnerability in the specification more than it is in the code. We are using heuristics to attempt to draw a line between what's something reasonable from something that could be abuse or someone being naive. Even with that in place, if your system accepts a large enough YAML document you may still be in trouble, because that's the nature of variable expansion: it allows the document to grow non-linearly with its input size. That's a class of problem that I haven't seen CVEs being assigned to before, and it still feels somewhat wrong in this case. |
This is a forward port of v2 commit f221b84 by Jordan Liggitt.
full diff: go-yaml/yaml@v2.2.2...v2.2.7 includes: - go-yaml/yaml@caeefd8 addresses CVE-2019-11253 JSON/YAML parsing vulnerable to resource exhaustion attack - go-yaml/yaml#171 Tighten restrictions on float decoding - go-yaml/yaml#515 Add large document benchmarks, tune alias heuristic, add max depth limits - go-yaml/yaml@f90ceb4 fixes go-yaml/yaml#529 yaml.Unmarshal crashes on "assignment to entry in nil map" - go-yaml/yaml#543 Port stale simple_keys fix to v2 - go-yaml/yaml@1f64d61 fixes go-yaml/yaml#548 Invalid simple_keys now cause panics later in decode Signed-off-by: Sebastiaan van Stijn <[email protected]>
full diff: go-yaml/yaml@v2.2.2...v2.2.7 includes: - go-yaml/yaml@caeefd8 addresses CVE-2019-11253 JSON/YAML parsing vulnerable to resource exhaustion attack - go-yaml/yaml#171 Tighten restrictions on float decoding - go-yaml/yaml#515 Add large document benchmarks, tune alias heuristic, add max depth limits - go-yaml/yaml@f90ceb4 fixes go-yaml/yaml#529 yaml.Unmarshal crashes on "assignment to entry in nil map" - go-yaml/yaml#543 Port stale simple_keys fix to v2 - go-yaml/yaml@1f64d61 fixes go-yaml/yaml#548 Invalid simple_keys now cause panics later in decode Signed-off-by: Sebastiaan van Stijn <[email protected]>
4 participants
This PR addresses the following items:
Parse time of excessively deep nested or indented documents
Parsing these documents is non-linear; limiting stack depth to 10,000 keeps parse times of pathological documents sub-second (~.25 seconds in benchmarks)
Alias node expansion limits
The current limit allows 10,000% expansion, which is too permissive for large documents.
Limiting to 10% expansion for larger documents allows callers to use input size as an effective way to limit resource usage. Continuing to allow larger expansion rates (up to the current 10,000% limit) for smaller documents does not unduly affect memory use.
This PR bounds decode operations from alias expansion to ~400,000 operations for small documents (worst-case ~100-150MB) or 10% of the input document for large documents, whichever is greater.
Using the same benchmark methodology as is included in this PR, memory growth of documents at a given size was measured with no alias use, and with alias use up to the point where the document was rejected:
Benchmarks