[security] Avoid plistlib XML vulnerabilities by rejecting entity directives

[vstinner]

BPO	42051
Nosy	@ronaldoussoren, @vstinner, @tiran, @ned-deily, @serhiy-storchaka, @miss-islington
PRs	bpo-42051: Reject XML entity declarations in plist files #22760 [3.9] bpo-42051: Reject XML entity declarations in plist files (GH-22760) #22771 [3.8] bpo-42051: Reject XML entity declarations in plist files (GH-22760) #22772 [3.7] bpo-42051: Reject XML entity declarations in plist files (#22760) #22801 [3.6] bpo-42051: Reject XML entity declarations in plist files (GH-22760) (GH-22801) #22804

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2020-10-20.04:44:06.570>
created_at = <Date 2020-10-16.08:32:10.097>
labels = ['type-security', '3.8', '3.9', '3.10', '3.7', 'library']
title = '[security] Avoid plistlib XML vulnerabilities by rejecting entity directives'
updated_at = <Date 2020-10-27.02:31:34.577>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2020-10-27.02:31:34.577>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2020-10-20.04:44:06.570>
closer = 'ned.deily'
components = ['Library (Lib)']
creation = <Date 2020-10-16.08:32:10.097>
creator = 'vstinner'
dependencies = []
files = []
hgrepos = []
issue_num = 42051
keywords = ['patch', 'security_issue']
message_count = 13.0
messages = ['378707', '378711', '378861', '378863', '378868', '378932', '378935', '378975', '379079', '379080', '379081', '379084', '379715']
nosy_count = 6.0
nosy_names = ['ronaldoussoren', 'vstinner', 'christian.heimes', 'ned.deily', 'serhiy.storchaka', 'miss-islington']
pr_nums = ['22760', '22771', '22772', '22801', '22804']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue42051'
versions = ['Python 3.6', 'Python 3.7', 'Python 3.8', 'Python 3.9', 'Python 3.10']

[vstinner]

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.

[ronaldoussoren]

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.

[serhiy-storchaka]

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 &bpo-12345; is a separate entity)? How to name the limit parameter if we make it configurable? What type of exceptions should be raised?

[ronaldoussoren]

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>

[serhiy-storchaka]

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.

[ronaldoussoren]

I'm working on a PR.

[ronaldoussoren]

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.

[ronaldoussoren]

New changeset 05ee790 by Ronald Oussoren in branch 'master':
bpo-42051: Reject XML entity declarations in plist files (bpo-22760)
05ee790

[miss-islington]

New changeset 479553c by Miss Skeleton (bot) in branch '3.9':
bpo-42051: Reject XML entity declarations in plist files (GH-22760)
479553c

[miss-islington]

New changeset 65894ca by Miss Skeleton (bot) in branch '3.8':
bpo-42051: Reject XML entity declarations in plist files (GH-22760)
65894ca

[ned-deily]

New changeset e512bc7 by Ned Deily in branch '3.7':
bpo-42051: Reject XML entity declarations in plist files (bpo-22760) (GH-22801)
e512bc7

[ned-deily]

New changeset a158fb9 by Miss Skeleton (bot) in branch '3.6':
bpo-42051: Reject XML entity declarations in plist files (GH-22760) (GH-22801) (GH-22804)
a158fb9

[vstinner]

Thanks Ronald Oussoren for the fix. It's better to fix a vulnerability (denial of service in this case) rather than documenting it :-)