Dos in merge key#937
Conversation
Most developer machines do not have libyaml headers installed, so the old default test flow attempted to build the optional extension, failed to find yaml.h, and then only exercised the pure Python fallback. The extension target also required out-of-band system setup before it could pass. Split the Makefile test flow into explicit pure-Python and LibYAML targets, and make the default test target run both. The pure-Python target now forces the extension off during build, while the LibYAML target builds a pinned local yaml/libyaml checkout and wires the include, library, and runtime paths into the extension build. Document the new Makefile workflow in the README, including make test, the individual test targets, the pinned LIBYAML-REF default, and the PYTHON-VERSION override for testing against a specific Python. This makes both test modes pass from a clean checkout without requiring developers to install libyaml development packages by hand.
Resolves #897 Supercedes #916 ### Summary The merge key (`<<`) constructor implementation in `SafeConstructor.flatten_mapping()` was vulnerable to an exponential time and memory complexity Denial of Service (DoS) vulnerability. When mapping/sequence nodes are merged using anchors/aliases, duplicate references to the same alias point to the same MappingNode instance in Python. During merge key processing, the node values are copied and extended in-place. If the same node appears multiple times at different levels, this causes exponential amplification of the elements list: `2^(n+1) - 1`. A small document under 1 KB can trigger millions of element list extensions, exhausting CPU and memory during safe loading. ### Hardened Fix This commit resolves the vulnerability and hardens it against secondary vectors: 1. Tracks node identity using object ID (`id(node)`) in a single `seen` set scoped to the parent mapping's `flatten_mapping()` execution. 2. Checks and skips duplicate node references inside SequenceNode merge keys (resolving PR-916). 3. Checks and skips duplicate node references across separate, independent MappingNode merge keys in the same mapping (e.g., repeating `<<: *anchor` multiple times). 4. Ensures C-based loaders (e.g., `CSafeLoader`, `CLoader`) are also protected since they inherit constructor logic from `SafeConstructor`. ### Performance Impact - Sequence-nested merge duplicates: Loading a 22-level nested document drops from 3.76s to 0.0028s (O(N) linear complexity). - Mapping-level merge duplicates: Loading a 20-level nested document drops from 0.93s to 0.0026s. ### Tests - Added regression tests to `tests/legacy_tests/data/construct-merge.data` and `tests/legacy_tests/data/construct-merge.code` covering both duplicate sequence merges and duplicate direct merges.
|
@nitzmahone @frenzymadness Please review. I added a Makefile reworking commit here because |
The PR was failing before it reached the PyYAML regression tests in two places: the cp38 wheel jobs installed latest cibuildwheel, whose 4.x series no longer supports Python 3.8, and the Windows arm64 libyaml build used a newer CMake that rejects libyaml 0.2.5's old minimum policy version. Pin the cp38 wheel jobs to cibuildwheel<4, and quote the matrix-provided pip package spec so bash does not parse the '<4' constraint as input redirection. Pass CMAKE_POLICY_VERSION_MINIMUM=3.5 to the Windows libyaml configure step, and disable fail-fast for the Windows libyaml matrix so one architecture does not hide the others.
414c6a5 to
cc352d5
Compare
|
This change only eliminates literal inline duplication within the same collection- the underlying problem with nested anchors as described on the comment on #916 still exists. The sample document provided there doesn't trigger this optimization and still results in exponential work, which makes the security fix incomplete. The proposed fix referenced in the comment does take care of the nested case, but also has some issues- the biggest being that it fails on non-hashable keys. It's moot for built-in Python I'm generally not a fan of |
nitzmahone
left a comment
nitzmahone
left a comment
There was a problem hiding this comment.
Other misc notes to take or leave...
|
|
||
| pushd libyaml/build | ||
| cmake.exe -G "Visual Studio 17 2022" -A ${{ matrix.arch }} -DYAML_STATIC_LIB_NAME=yaml .. | ||
| cmake.exe -G "Visual Studio 17 2022" -A ${{ matrix.arch }} -DYAML_STATIC_LIB_NAME=yaml -DCMAKE_POLICY_VERSION_MINIMUM=3.5 .. |
There was a problem hiding this comment.
I assume this is to shut up a warning on an old libyaml dist, but shouldn't it just be updated in libyaml's CMakeLists.txt instead of done on this end?
There was a problem hiding this comment.
Can't this just be replaced with tox or nox like what most of the Python projects have by convention?
There was a problem hiding this comment.
I prefer to use Makes for all my projects in all languages as it is zero dep.
iow, Python isn't required and testing against any Python version on any machine is just:
make test
make test PYTHON-VERSION=3.14.0
There was a problem hiding this comment.
That said, if you want to submit a PR to support tox or nox as well, I have no problem with that.
There was a problem hiding this comment.
Why maintain multiple configurations for working with the project? That leads to increased maintenance problems, with different contributors only able to effectively maintain the configurations for the tool(s) they use.
Since this is a Python project, it seems reasonable to standardize on one tool that's well supported within the Python ecosystem. While it won't fit everyone's preferred workflow, doing so will maximize the ability of the community as a whole to effectively contribute.
There was a problem hiding this comment.
The structural changes around build/test/CI workflows to properly support abi3/abi3t make a lot of sense to do with something like tox/nox anyway- re-layering the Makefile targets to point at the various apropos tox/nox sessions once that's available should alleviate my concerns about the current inline logic not matching the shipping bits.
There was a problem hiding this comment.
maximize the ability of the community as a whole to effectively contribute
This should definitely be a guiding principal of all workflow and build environment changes.
concerns about the current inline logic not matching the shipping bits
I've been learning a lot about deterministic, repeatable build processes (like are the bits the same every run if code wasn't changed), and standardizing builds generate attested artifacts. My goal is to propose a solution that has little or no workflow impact for contributors.
There was a problem hiding this comment.
@aaronbronow if you're interested, I'm working on a tox-centric ecosystem for CI/CD/infra. The GHA UX looks like this: https://github.com/ansible/awx-plugins/blob/a5523a20b5420cd93799cee0d2bc2623a841c177/.github/workflows/ci-cd.yml#L498-L550. I've also got a bunch of tox plugins (it's a novel thing) that bring shareable tox envs with a single pip install (or adding to requires in the config) instead of copying configs across projects.
It's a bit nuanced w/ C-ext projects but works rather well w/ pure-python.
There was a problem hiding this comment.
There are some unique challenges to bit-perfect deterministic builds on this project, mostly related to the ancient custom setuptools (née distutils) + Cython extension build. There are other promising options out there (scikit-build-core, Meson) that might make it easier, but it'll require a rewrite of the extension build and killing off the expectation of on automatic "best-effort" extension build that's existed for the project's entire life (and at least scikit-build-core is a very young "pre-1.0" project, last I checked).
8a0b1d8 to
30ed0df
Compare
|
30ed0df now has tests that failed before the (relatively small) fix and passed after. If anyone thinks there are still cases we haven't considered, please suggest a test that fails before and after this fix. Or at least after. |
|
I don't see any implementation change since yesterday that addresses the originally-referenced nested repro- it still results in exponential compute expansion. The tests added today did reveal a last-minute screwup I made in the suggested hybrid fix though- tried to optimize away the set creation in non-merge cases, but that breaks if we hit it > 1 time for a given mapping. I posted an updated suggestion that passes all tests (including those just added) and yields a result for the original nested repro orders of magnitude faster. Hardcoded empirical timeouts on unit tests always make me nervous- at a glance, they don't seem to be failing as expected (since the nested exponential compute expansion still exists), and unless the chasm between pass/fail is at least a couple orders of magnitude wide, they're likely to cause problems on our emulated arches (which run ~ an order of magnitude slower than the real hardware). |
Replace the timing-based merge DoS tests with deterministic checks of the flattened mapping shape. Wall-clock thresholds are fragile on slow or emulated CI runners and do not directly prove that the expansion was prevented. The fan-out regression case uses a small levels=4, width=4 payload from the PR #916 shape. With the old behavior, flattening root expands to 1024 repeated key/value pairs and the test fails immediately because the key list contains 1020 extra entries. With the fix, duplicate merge pairs are collapsed during flattening and the same root mapping contains only k0, k1, k2, and k3. This keeps the test fast even if the old bug returns: it observes the structural amplification directly instead of waiting for a large pathological input to time out.
* A hybrid combination of fixes proposed by frenzymadness and akshat-sj to prevent exponential compute while resolving nested anchor aliases. * Further optimization is possible with broader caching of anchor node graph, but would require careful design of invalidation policy.
|
Thanks for fixing this! I’ve verified that the #897 has been resolved. 👍 |
7 participants
Resolves #897
Supercedes #916
Summary
The merge key (
<<) constructor implementation inSafeConstructor.flatten_mapping()was vulnerable to anexponential time and memory complexity Denial of Service (DoS)
vulnerability. When mapping/sequence nodes are merged using
anchors/aliases, duplicate references to the same alias point
to the same MappingNode instance in Python. During merge key
processing, the node values are copied and extended in-place.
If the same node appears multiple times at different levels,
this causes exponential amplification of the elements list:
2^(n+1) - 1.A small document under 1 KB can trigger millions of element
list extensions, exhausting CPU and memory during safe loading.
Hardened Fix
This commit resolves the vulnerability and hardens it against
secondary vectors:
id(node)) in a singleseenset scoped to the parent mapping'sflatten_mapping()execution.
merge keys (resolving PR-916).
independent MappingNode merge keys in the same mapping (e.g.,
repeating
<<: *anchormultiple times).CSafeLoader,CLoader) arealso protected since they inherit constructor logic from
SafeConstructor.Performance Impact
document drops from 3.76s to 0.0028s (O(N) linear complexity).
document drops from 0.93s to 0.0026s.
Tests
tests/legacy_tests/data/construct-merge.dataandtests/legacy_tests/data/construct-merge.codecovering bothduplicate sequence merges and duplicate direct merges.