gh-115398: Expose Expat >=2.6.0 reparse deferral API (CVE-2023-52425) #115623
Conversation
hartwork
force-pushed
the
expat-2-6-0-reparse-deferral-api
branch
from
2346261
to
f05676e
Compare
Misc/NEWS.d/next/Security/2024-02-18-03-14-40.gh-issue-115398.tzvxH8.rst
Show resolved
Hide resolved
hartwork
force-pushed
the
expat-2-6-0-reparse-deferral-api
branch
2 times, most recently
from
d80dcc4
to
5f23237
Compare
|
Please do not use rebase and force push. It makes reviewing more difficult -- the reviewer needs to re-read all changes instead of only the difference since the last review. Use merge if you need, but this is only needed if there are conflicts with the main branch. At the end all commits will be squashed and merged as a single commits. |
hartwork
force-pushed
the
expat-2-6-0-reparse-deferral-api
branch
from
5f23237
to
a62e863
Compare
@serhiy-storchaka that's what GitHub has "compare" buttons for, for every push done to the pull request branch:
The e-mail that GitHub sends also contain these links.
That's sad. Merge commits should not be in pull requests and commit cuttings do have semantic value. I'll "go cry somewhere now". |
hartwork
force-pushed
the
expat-2-6-0-reparse-deferral-api
branch
3 times, most recently
from
8c26bbe
to
439d970
Compare
hartwork
force-pushed
the
expat-2-6-0-reparse-deferral-api
branch
from
439d970
to
850e46d
Compare
|
The "Compare" link shows a lot of unrelated changes. This is why it is better to use merge instead of rebase. |
@serhiy-storchaka only because I made a mistake in one of the rebases. It works perfectly if the merge base is the same and the person pushing does the right thing. |
|
For future, please don't use rebase. |
|
|
||
| elementtreestate *st = self->state; | ||
|
|
||
| if (EXPAT(st, SetReparseDeferralEnabled) == NULL) { |
There was a problem hiding this comment.
It should check the result of GetReparseDeferralEnabled() as well.
There was a problem hiding this comment.
Adding an optional data arg to flush() could be useful for performance: it would allow merging feed(data); flush() into just flush(data), avoiding unnecessary reparsing (specifically, in the case where the original feed also tried to parse).
Another option would be to add a default-false flush flag to feed(). This would perhaps be a better API, as it will be very clear that the flush operation can cause the same effects and errors as a normal feed.
In both cases, the flush operation should be documented along the lines of "Try to parse all available data, possibly at a very high performance cost".
| @@ -1726,6 +1731,17 @@ def close(self): | |||
| del self.parser, self._parser | |||
| del self.target, self._target | |||
|
|
|||
| def flush(self): | |||
| if not self.parser.GetReparseDeferralEnabled(): | |||
| return | |||
There was a problem hiding this comment.
There's technically a possibility that deferral was disabled after the last feed(), which would make this early return skip a flush that could actually have had an effect.
AFAICT, this would require reaching into this object and using its parser attribute to call SetReparseDeferralEnabled() -- do we care about such usecases, or is the programmer on their own in that case?
This is an interesting idea. The drawback is that it is more difficult to detect whether the new feature is supported or not: you cannot simply use |
Is there an somewhat standard alternative, e.g. feature flags or something? If not, maybe we can just set |
Mission
Allow controlling Expat >=2.6.0 reparse deferral (CVE-2023-52425) by adding five new methods:
pyexpat.xmlparser.GetReparseDeferralEnabledpyexpat.xmlparser.SetReparseDeferralEnabledxml.etree.ElementTree.XMLParser.flushxml.etree.ElementTree.XMLPullParser.flushxml.sax.expatreader.ExpatParser.flushBased on the "flush" idea from #115138 (comment) .
Notes
CC @serhiy-storchaka
XML_SetReparseDeferralEnabled#115398