Skip to content

[3.14] gh-90949: expose Expat API to tune exponential expansion protections (GH-139368)#150496

Merged
StanFromIreland merged 4 commits into
python:3.14from
StanFromIreland:backport-6661123-3.14
Jun 9, 2026
Merged

[3.14] gh-90949: expose Expat API to tune exponential expansion protections (GH-139368)#150496
StanFromIreland merged 4 commits into
python:3.14from
StanFromIreland:backport-6661123-3.14

Conversation

@StanFromIreland

@StanFromIreland StanFromIreland commented May 26, 2026

Copy link
Copy Markdown
Member

Expose the XML Expat 2.7.2 APIs to tune protections against "billion laughs" 1 attacks.

The exposed APIs are available on Expat parsers, that is, parsers created by xml.parsers.expat.ParserCreate(), as:

  • parser.SetBillionLaughsAttackProtectionActivationThreshold(threshold), and
  • parser.SetBillionLaughsAttackProtectionMaximumAmplification(max_factor).

This completes the work in f04bea4, and improves the existing related documentation.

… protections (pythonGH-139368)

Expose the XML Expat 2.7.2 APIs to tune protections against
"billion laughs" [1] attacks.

The exposed APIs are available on Expat parsers, that is,
parsers created by `xml.parsers.expat.ParserCreate()`, as:

- `parser.SetBillionLaughsAttackProtectionActivationThreshold(threshold)`, and
- `parser.SetBillionLaughsAttackProtectionMaximumAmplification(max_factor)`.

This completes the work in f04bea4,
and improves the existing related documentation.

[1]: https://en.wikipedia.org/wiki/Billion_laughs_attack
(cherry picked from commit 6661123)

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Comment thread Lib/test/test_pyexpat.py
self.assert_root_parser_failure(setter, 123.45)


@unittest.skipIf(expat.version_info < (2, 4, 0), "requires Expat >= 2.4.0")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we had a recent issue where this check was not sufficient and tests needed to be disabled in another way using hasattr() checks. Can you check how it's currently done on main please?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's identical on main?

@unittest.skipIf(expat.version_info < (2, 4, 0), "requires Expat >= 2.4.0")
class ExpansionProtectionTest(AttackProtectionTestBase, unittest.TestCase):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah indeed, but the problem was for the other API:

@unittest.skipIf(not hasattr(expat.XMLParserType,
                             "SetAllocTrackerMaximumAmplification"),
                 "requires Python compiled with Expat >= 2.7.2")

So I think the version check for billion laughs is also wrong and should be changed to the same as for SetAllocTrackerMaximumAmplification. But let's address this separately later in a follow-up PR (so as to ease backports)

Comment thread Modules/pyexpat.c Outdated
Comment thread Modules/pyexpat.c Outdated
@StanFromIreland StanFromIreland marked this pull request as draft May 26, 2026 21:49
@StanFromIreland StanFromIreland marked this pull request as ready for review May 27, 2026 15:20
@StanFromIreland StanFromIreland requested a review from picnixz May 27, 2026 15:20
@picnixz

picnixz commented May 27, 2026

Copy link
Copy Markdown
Member

LGTM but I'd appreciate if @hartwork could have a quick glance just for wordings. I hope every backport has been merged properly (if we are matching against 3.15 then it should good). It's just that I remember that the work was split across multiple PRs...

@hartwork

Copy link
Copy Markdown
Contributor

LGTM but I'd appreciate if @hartwork could have a quick glance just for wordings. I hope every backport has been merged properly (if we are matching against 3.15 then it should good). It's just that I remember that the work was split across multiple PRs...

@StanFromIreland I have just had a quick look through the lense of…

# diff -u1 <(git diff 666112376d574c7802646ee1df6244062671cd61{^,}) <(git diff upstream-readonly/3.14...StanFromIreland/backport-6661123-3.14) | ydiff 

…and I see:

  • differences in @unittest.skipIf machinery
  • differences in float style "100.0" versus int style "100" in documentation

Are these intended or accidental?

@picnixz

picnixz commented May 28, 2026

Copy link
Copy Markdown
Member

differences in float style "100.0" versus int style "100" in documentation

I think I updated these at some points but I don't remember in which direction. I would say: pick the style that we currently have on main, it doesn't matter. I just don't want to have docs that are different across versions.

@hartwork

Copy link
Copy Markdown
Contributor

@picnixz +1 from me for being in line with main 👍

Comment thread Modules/pyexpat.c Outdated
@StanFromIreland StanFromIreland requested a review from picnixz June 9, 2026 11:49
Comment thread Modules/pyexpat.c Outdated
Comment on lines +2406 to +2407
capi->SetAllocTrackerActivationThreshold = NULL;
capi->SetAllocTrackerMaximumAmplification = NULL;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the else is the wrong here actually, it's SetBillionLaughsAttackProtectionActivationThreshold

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the same on main:

cpython/Modules/pyexpat.c

Lines 2548 to 2554 in 9fdbade

#if XML_COMBINED_VERSION >= 20400
capi->SetBillionLaughsAttackProtectionActivationThreshold = XML_SetBillionLaughsAttackProtectionActivationThreshold;
capi->SetBillionLaughsAttackProtectionMaximumAmplification = XML_SetBillionLaughsAttackProtectionMaximumAmplification;
#else
capi->SetAllocTrackerActivationThreshold = NULL;
capi->SetAllocTrackerMaximumAmplification = NULL;
#endif

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote #151147 for main/3.15, for here:

Suggested change
capi->SetAllocTrackerActivationThreshold = NULL;
capi->SetAllocTrackerMaximumAmplification = NULL;
capi->SetBillionLaughsAttackProtectionActivationThreshold = NULL;
capi->SetBillionLaughsAttackProtectionMaximumAmplification = NULL;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh!

@StanFromIreland StanFromIreland requested a review from picnixz June 9, 2026 12:04

@picnixz picnixz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I think we're good. If docs need to be amended we'll do it but C should be ok.

@StanFromIreland

Copy link
Copy Markdown
Member Author

Fuzzer failures are unrelated (ast and plistlib).

@StanFromIreland StanFromIreland merged commit c078637 into python:3.14 Jun 9, 2026
52 of 55 checks passed
@StanFromIreland StanFromIreland deleted the backport-6661123-3.14 branch June 9, 2026 12:42
@StanFromIreland StanFromIreland added the needs backport to 3.13 bugs and security fixes label Jun 9, 2026
@miss-islington-app

Copy link
Copy Markdown

Thanks @StanFromIreland for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

@miss-islington-app

Copy link
Copy Markdown

Sorry, @StanFromIreland, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker c078637e8642783d90b33c6b983fa15ff29c24d6 3.13

@bedevere-app

bedevere-app Bot commented Jun 9, 2026

Copy link
Copy Markdown

GH-151151 is a backport of this pull request to the 3.13 branch.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants