classification
Title: [security] Avoid plistlib XML vulnerabilities by rejecting entity directives
Type: security Stage: resolved
Components: Library (Lib) Versions: Python 3.10, Python 3.9, Python 3.8, Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: christian.heimes, miss-islington, ned.deily, ronaldoussoren, serhiy.storchaka, vstinner
Priority: normal Keywords: patch, security_issue

Created on 2020-10-16 08:32 by vstinner, last changed 2020-10-27 02:31 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 22760 merged ronaldoussoren, 2020-10-19 09:22
PR 22771 merged miss-islington, 2020-10-19 18:14
PR 22772 merged miss-islington, 2020-10-19 18:14
PR 22801 merged ned.deily, 2020-10-20 01:57
PR 22804 merged miss-islington, 2020-10-20 04:19
Messages (13)
msg378707 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-10-16 08:32
The XML documentation starts with a red warning:

"Warning: The XML modules are not secure against erroneous or maliciously constructed data. If you need to parse untrusted or unauthenticated data see the XML vulnerabilities and The defusedxml Package sections. "
https://docs.python.org/dev/library/xml.html

I suggest to add the same warning to the plistlib library which uses the XML parser internally to handle XML files.
msg378711 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2020-10-16 11:47
Is there something we could do in plistlib to avoid those problems? 

Plist XML files are not arbitrary XML. In particular disabling entity definitions would avoid the problems mentioned as plist files should not contain those. That said, a quick glance at the xml.etree module doesn't seem to have a documented way to disable entities.
msg378861 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-10-18 13:37
Seems that we can not control entity definitions and expansions. We only can limit the number of expanded entities per element (the size of self.data).

What is the reasonable default limit (taking into account that every < and 〹 is a separate entity)? How to name the limit parameter if we make it configurable? What type of exceptions should be raised?
msg378863 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2020-10-18 14:24
One option is to copy what defusedxml does to forbid a number of unsafe operations, see https://github.com/tiran/defusedxml/blob/eb38a2d710b67df48614cb5098ddb8472289ce6d/defusedxml/ElementTree.py#L68

Defusedxml uses an XMLParser subclass that optionally disables some features (such as entity definitions), for plistlib those features can be disabled unconditionally. 

I haven't thought much about the exceptions to use, probably a similar exception as is used for invalid plist files. 

Another thing I haven't really thought about: would such a change be 3.10 only or is this something we could backport?  

The following plist file currently works with plistlib, but does not work with plutil(1) on macOS 10.15 (parse error in the DTD definition).  That indicates that entity definitions aren't supposed to be used in plist files and it would be safe to disable this feature in plistlib.

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd" [
   <!ENTITY entity "replacement text">
  ]>
<plist version="1.0">
  <dict>
    <key>A</key>
    <string>&entity;</string>
  </dict>
</plist>
msg378868 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-10-18 14:40
Oh, I missed that there is a handler for EntityDecl. Well, then we can fix this issue in few lines of code.

I think it should be backported. We can add private flag (global or class variable) to enable entity declarations, but do not support them in 3.10.
msg378932 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2020-10-19 08:34
I'm working on a PR.
msg378935 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2020-10-19 09:26
The PR is fairly simple: Just reject files with entity declarations as invalid files. Adding an option to accept entity declarations should not be necessary as Apple tools won't accept these declarations.
msg378975 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2020-10-19 18:13
New changeset 05ee790f4d1cd8725a90b54268fc1dfe5b4d1fa2 by Ronald Oussoren in branch 'master':
bpo-42051: Reject XML entity declarations in plist files (#22760)
https://github.com/python/cpython/commit/05ee790f4d1cd8725a90b54268fc1dfe5b4d1fa2
msg379079 - (view) Author: miss-islington (miss-islington) Date: 2020-10-20 02:34
New changeset 479553c7c11306a09ce34edb6ef208133b7b95fe by Miss Skeleton (bot) in branch '3.9':
bpo-42051: Reject XML entity declarations in plist files (GH-22760)
https://github.com/python/cpython/commit/479553c7c11306a09ce34edb6ef208133b7b95fe
msg379080 - (view) Author: miss-islington (miss-islington) Date: 2020-10-20 02:35
New changeset 65894cac0835cb8f469f649e20aa1be8bf89f5ae by Miss Skeleton (bot) in branch '3.8':
bpo-42051: Reject XML entity declarations in plist files (GH-22760)
https://github.com/python/cpython/commit/65894cac0835cb8f469f649e20aa1be8bf89f5ae
msg379081 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2020-10-20 02:36
New changeset e512bc799e3864fe3b1351757261762d63471efc by Ned Deily in branch '3.7':
bpo-42051: Reject XML entity declarations in plist files (#22760) (GH-22801)
https://github.com/python/cpython/commit/e512bc799e3864fe3b1351757261762d63471efc
msg379084 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2020-10-20 04:38
New changeset a158fb9c5138db94adf24fbc5690467cda811163 by Miss Skeleton (bot) in branch '3.6':
bpo-42051: Reject XML entity declarations in plist files (GH-22760) (GH-22801) (GH-22804)
https://github.com/python/cpython/commit/a158fb9c5138db94adf24fbc5690467cda811163
msg379715 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-10-27 02:31
Thanks Ronald Oussoren for the fix. It's better to fix a vulnerability (denial of service in this case) rather than documenting it :-)
History
Date User Action Args
2020-10-27 02:31:34vstinnersetmessages: + msg379715
2020-10-20 04:44:06ned.deilysetstatus: open -> closed
versions: + Python 3.6, Python 3.7
title: plistlib inherits XML vulnerabilities: we should document them -> [security] Avoid plistlib XML vulnerabilities by rejecting entity directives
keywords: + security_issue
resolution: fixed
stage: patch review -> resolved
2020-10-20 04:38:37ned.deilysetmessages: + msg379084
2020-10-20 04:19:17miss-islingtonsetpull_requests: + pull_request21761
2020-10-20 02:36:34ned.deilysetmessages: + msg379081
2020-10-20 02:35:27miss-islingtonsetmessages: + msg379080
2020-10-20 02:34:46miss-islingtonsetmessages: + msg379079
2020-10-20 01:57:37ned.deilysetnosy: + ned.deily
pull_requests: + pull_request21758
2020-10-19 18:14:11miss-islingtonsetpull_requests: + pull_request21734
2020-10-19 18:14:03miss-islingtonsetnosy: + miss-islington
pull_requests: + pull_request21733
2020-10-19 18:13:56ronaldoussorensetmessages: + msg378975
2020-10-19 09:26:00ronaldoussorensetmessages: + msg378935
2020-10-19 09:22:42ronaldoussorensetkeywords: + patch
stage: patch review
pull_requests: + pull_request21725
2020-10-19 08:34:01ronaldoussorensetmessages: + msg378932
2020-10-18 14:40:31serhiy.storchakasetmessages: + msg378868
2020-10-18 14:24:00ronaldoussorensetmessages: + msg378863
2020-10-18 13:37:17serhiy.storchakasetnosy: + christian.heimes
messages: + msg378861
2020-10-16 11:47:36ronaldoussorensetnosy: + ronaldoussoren
messages: + msg378711
2020-10-16 08:32:51vstinnersetnosy: + serhiy.storchaka
2020-10-16 08:32:10vstinnercreate