Title: XML vulnerabilities in Python
Type: security Stage: needs patch
Components: Extension Modules, Library (Lib), XML Versions: Python 3.5, Python 3.1, Python 3.2, Python 3.3, Python 3.4, Python 2.7, Python 2.6
Status: open Resolution:
Dependencies: 17318 24238 Superseder:
Assigned To: Nosy List: Arfrever, barry, benjamin.peterson, djc, eli.bendersky, ezio.melotti, franck, georg.brandl, jwilk, larry, martin.panter, mcepl, pitrou, rhettinger, rsandwick3, scoder, serhiy.storchaka
Priority: critical Keywords: patch

Created on 2013-02-19 15:35 by christian.heimes, last changed 2018-03-05 00:46 by mcepl.

File name Uploaded Description Edit
xmlbomb_20130219.patch christian.heimes, 2013-02-19 15:35 review
xmlbomb_20150518.patch martin.panter, 2015-05-18 02:59 Merged to 3.5 review
Messages (8)
msg182393 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-02-19 15:35
Experimental fix for XML vulnerabilities against default. It's NOT ready and needs lots of polishing. contains explanations of all issues is a standalone version of part of the patches for Python 2.6 to 3.3
msg184285 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-03-16 02:17
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)
msg184289 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2013-03-16 04:19
> 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)

msg184387 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-03-17 16:46
> 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.
msg185053 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-03-23 14:45
Not blocking 2.7.4 as discussed on mailing list.
msg243450 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-05-18 02:59
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.
msg243469 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-05-18 10:44
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
>>> 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:
* (though I suspect the SetDefaultHandler option there is not sufficient)
msg243581 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-05-19 11:26
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.
Date User Action Args
2018-03-05 00:46:11mceplsetnosy: + mcepl
2016-06-12 12:14:14christian.heimessetnosy: - christian.heimes
2016-06-12 12:11:24martin.pantersetdependencies: + xml.sax and xml.dom fetch DTDs by default, Avoid entity expansion attacks in Element Tree
2015-05-24 08:36:04scodersetnosy: + scoder
2015-05-19 11:26:52martin.pantersetmessages: + msg243581
2015-05-18 10:44:13martin.pantersetmessages: + msg243469
2015-05-18 03:00:00martin.pantersetfiles: + xmlbomb_20150518.patch

messages: + msg243450
versions: + Python 3.5
2015-01-14 23:49:33jwilksetnosy: + jwilk
2015-01-12 00:11:24martin.pantersetnosy: + martin.panter
2013-03-25 16:59:30rsandwick3setnosy: + rsandwick3
2013-03-23 14:45:10benjamin.petersonsetpriority: release blocker -> critical

messages: + msg185053
2013-03-17 16:46:08pitrousetnosy: + pitrou
messages: + msg184387
2013-03-16 04:19:02rhettingersetnosy: + rhettinger
messages: + msg184289
2013-03-16 02:17:49benjamin.petersonsetmessages: + msg184285
2013-02-22 23:40:35Arfreversetnosy: + Arfrever
2013-02-20 10:21:07djcsetnosy: + djc
2013-02-19 19:49:24serhiy.storchakasetnosy: + serhiy.storchaka
2013-02-19 17:26:29francksetnosy: + franck
2013-02-19 15:37:13ezio.melottisetnosy: + ezio.melotti, eli.bendersky
2013-02-19 15:35:41christian.heimescreate