Experimental fix for XML vulnerabilities against default. It's NOT ready and needs lots of polishing.

https://pypi.python.org/pypi/defusedxml contains explanations of all issues
https://pypi.python.org/pypi/defusedexpat is a standalone version of part of the patches for Python 2.6 to 3.3

Since this has dragged on for quite a while, I'm probably just going to release 2.7.4 with a pointer to defusedxml in the release notes. (docs, though, perhaps)

> Since this has dragged on for quite a while, I'm probably 
> just going to release 2.7.4 with a pointer to defusedxml
> in the release notes. (docs, though, perhaps)

+1

> Since this has dragged on for quite a while, I'm probably just going to 
> release 2.7.4 with a pointer to defusedxml in the release notes. (docs, 
> though, perhaps)

+1 too.

Not blocking 2.7.4 as discussed on mailing list.

I did a rough merge with current “default” (3.5 pre-release) branch so that I can have a closer look at this issue; see xmlbomb_20150518.patch for the result. There are some bits with Argument Clinit that need perfecting:

* Unsure how to convert the ElementTree.XMLParser.__init__() signature (varied depending on XML_BOMB_PROTECTION compile-time flag) to Argument Clinic. So I just hard-coded it as if XML_BOMB_PROTECTION is always enabled. Why do we have to have a variable signature in the first place?

* New pyexpat functions need porting to Argument Clinic.

I started looking at the lower Expat-level changes. Here are some thoughts, in the order that I thought them. :) But the end result is to investigate a different approach to disable entities in existing versions of Expat.

Currently, it looks like max_entity_indirections = 0 is a special value meaning no limit. I think it would be better to use some other value such as None for this, and then 0 could disable all entity expansion (other than pre-defined entities like & &#xNNNN; etc).

What is the benefit of having the indirection limit? I would have thought the entity expansion (character) limit on its own would already be effective at preventing nested expansion attacks like “billion laughs”. Even if the entity expanded to an empty string, all of the intermediate entity references are still included in the character count.

I wonder if it would make more sense to have a total character limit instead, which would include the characters from custom entity expansions as already counted by the patch, but also count characters directly from the XML body. Why would you want to avoid 8 million characters from entity expansion, but allow 8 million characters of plain XML (or gzipped XML)? (I am not an XML expert, so I could be missing something obvious here.)

Now I have discovered that it seems you can build Python to use an external Expat library, which won’t be affected by Christian’s fix (correct me if I am wrong). I think we should find a different solution that will also work with existing external Expat versions. Maybe setting EntityDeclHandler to raise an error would be good enough:

>>> from xml.parsers import expat
>>> bomb = '<!DOCTYPE bomb [\n<!ENTITY a "" >\n<!ENTITY b "' + '&a;' * 1000 + '" >\n<!ENTITY c "' + '&b;' * 1000 + '" >\n]>\n<bomb a="' + '&c;' * 10 + '" />\n'
>>> p = expat.ParserCreate()
>>> p.Parse(bomb, True)  # Noticeable delay (DOS) while parsing
1
>>> p = expat.ParserCreate()
>>> def handler(*so_much_argh):
...     raise ValueError("Entity handling disabled")
... 
>>> p.EntityDeclHandler = handler
>>> p.Parse(bomb, True)  # Instant failure (no DOS)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/build/python/src/Python-3.4.3/Modules/pyexpat.c", line 494, in EntityDecl
  File "<stdin>", line 2, in handler
ValueError: Entity handling disabled

This solution has been suggested and implemented elsewhere:
* https://bugzilla.redhat.com/show_bug.cgi?id=1000109#c1
* http://mail-archives.apache.org/mod_mbox/apr-dev/200906.mbox/%3C20090602162934.GA28360@redhat.com%3E (though I suspect the SetDefaultHandler option there is not sufficient)

I have opened Issue 24238 with a patch for Element Tree that uses my EntityDeclHandler technique, instead of patching Expat. I would be interested in other people’s thoughts on the approach.

This issue didn't get much attention in 5 years. The XML documentation starts with a big red warning:
https://docs.python.org/dev/library/xml.html

The warning is present in 2.7 and 3.4 as well:
https://docs.python.org/2.7/library/xml.html
https://docs.python.org/3.4/library/xml.html

It seems like XML is getting less popular because of JSON becoming more popular (JSON obviously comes with its own set of security issues). It seems like less core developers care about XML.

I suggest to:

* close bpo-17318 as a duplicate of this issue (bpo-17239)
* close bpo-24238
* close this issue

We just have to accept that core developers have limited availability and that documenting security issues is an acceptable tradeoff. I don't see any value of keeping these 3 issues open.

> I suggest to:
> 
> * close bpo-17318 as a duplicate of this issue (bpo-17239)
> * close bpo-24238
> * close this issue

+1 from me.

Ned - I don't think this is necessarily a release blocker, as we've been shipping it for a long time, but it would be nice if we can hold 3.7.1rc1 just long enough to get it in (provided Christian jumps in and says he'll get the last minor concerns on the PRs wrapped up very soon)

We discussed this last week at the sprint.  Christian, it would be great if you could get this merged for 3.7 and possibly 3.6 in the next 24 hours.

The external entity patch is ready, but the billion laughs fix need more time. I'm working with an upstream developer on a proper fix.

Any reason to not take the current patch for our vendored copy and give it some exposure at least on platforms that rely on it (maybe just Windows)? I don't see any reason to wait on another group to "release" it when we need to manually apply the update to our own repo anyway.

Platforms using system libexpat that hasn't been patched have obviously decided not to patch it themselves :)

> Any reason to not take the current patch for our vendored copy and give it some exposure at least on platforms that rely on it (maybe just Windows)? I don't see any reason to wait on another group to "release" it when we need to manually apply the update to our own repo anyway.

My policy is upstream fix: first, get a change merged upstream.

If we start with a downstream patch:

* only Windows and macOS will get the fix
* upstream may require changes making the change incompatible, for example change the default limits
* I would prefer to keep Modules/expat/ as close as possible to the upstream

Python is vulnerable for years, it's not like there is an urgency to fix it.

There's also the view that it'll be easier to justify upstreaming a patch if it's been released and tested in a separate app. We require that all the time for Python patches, so why should we expect other projects to be different?

We're totally entitled to only release it for those platforms, because we are responsible for libexpat on those (we could vendor it for all of them? Or switch to platform-supported libraries for macOS and Windows?)

Who normally updates the vendored libexpat? I'd rather let them make the call on how far to diverge from upstream, since it'll be up to them to roll the changes forward or revert them in favour of upstream. I doubt different defaults will be an issue, especially since they aren't configurable anyway.

> Who normally updates the vendored libexpat?

I made the 3 latest libexpat updates, and each of them was painful :-)

My notes on vendored libraries:
https://pythondev.readthedocs.io/cpython.html#vendored-external-libraries

I wrote a tool to get the version of all vendored libraries, and a script to updated libexpat.

> * only Windows and macOS will get the fix

Modules/expat can be used on all platforms. A downstream patch is only a problem for platforms that compile Python with "./configure --with-system-expat".

The security fixes for entity expansion blowup and external entity loading are backwards incompatible fixes. Technically they also violate XML standards. In practice the vast majority of users will never run into the issue, because external entities are scarcely used. The expat parser is a non-validating XML parser, so DTDs aren't useful at all. I'd rather break a handful of users than to keep the majority of users vulnerable.

To fix billion laughs and quadratic blowup once and for all, we also have to break backwards compatibility and require expat >= 2.3.0. For now the modules still work with old versions of expat. IMO it's fine. Vendors either have to update their libraries or use our copy of expat.

Ultimately it's Benjamin's, Larry's, and Ned's decision. They are release managers.

On Tue, Sep 18, 2018, at 06:39, STINNER Victor wrote:
> 
> STINNER Victor <vstinner@redhat.com> added the comment:
> 
> > Who normally updates the vendored libexpat?
> 
> I made the 3 latest libexpat updates, and each of them was painful :-)

Oh? I've updated it twice (4e21100fa7bf66e0b32146d3f46ae16afc73fee1 and 5033aa77aacaa5505636f150e8d54baac5bdca9c), and it didn't seem so bad. I just copied the upstream files in. Did I do it wrong?

> Oh? I've updated it twice (4e21100fa7bf66e0b32146d3f46ae16afc73fee1 and 5033aa77aacaa5505636f150e8d54baac5bdca9c), and it didn't seem so bad. I just copied the upstream files in. Did I do it wrong?

Let me remind what I did...

bpo-30694 (expat 2.2.1):

* I wrote a script to rebuild Modules/expat/ from the upstream code
* I had to manually keep our old pyexpatns.h file since it's a downstream change
* Then you have to add againt #include "pyexpatns.h" in Modules/expat/expat_external.h
* It broke buildbots: bpo-29591
* The change introduced a compilation warning: bpo-30797

bpo-30947 (expat 2.2.3):

* "If libexpat is upgraded in Python 2.7, the new Modules/expat/loadlibrary.c should also be added to PC/VS9.0/ project files, as I did for PCbuild."
* "Expat 2.2.3 has a bug: see bpo-31170 :-("
* etc.

There are different issues:

* We have some small downstream changes
* We still support VS 2008 for Python 2.7 whereas upstream doesn't care of this old legacy compiler
* Each release introduces its own set of bugs :-D
* Each release comes with its own set of new warnings...

At least for me, each update was painful. It's also painful to have to make the same change in all supported branches (2.7, 3.4, 3.5, 3.6, 3.7, master).

New changeset 17b1d5d4e36aa57a9b25a0e694affbd1ee637e45 by Miss Islington (bot) (Christian Heimes) in branch 'master':
bpo-17239: Disable external entities in SAX parser (GH-9217)
https://github.com/python/cpython/commit/17b1d5d4e36aa57a9b25a0e694affbd1ee637e45

New changeset 582d188e6e3487180891f1fc457a80dec8be26a8 by Miss Islington (bot) (Christian Heimes) in branch '3.6':
[3.6] bpo-17239: Disable external entities in SAX parser (GH-9217) (GH-9512)
https://github.com/python/cpython/commit/582d188e6e3487180891f1fc457a80dec8be26a8

New changeset 394e55a9279d17240ef6fe85d3b4ea3fe7b6dff5 by Miss Islington (bot) (Christian Heimes) in branch '3.7':
[3.7] bpo-17239: Disable external entities in SAX parser (GH-9217) (GH-9511)
https://github.com/python/cpython/commit/394e55a9279d17240ef6fe85d3b4ea3fe7b6dff5