Issues with binary plists

[serhiy-storchaka]

BPO	32072
Nosy	@ronaldoussoren, @larryhastings, @ned-deily, @serhiy-storchaka
PRs	bpo-32072: Fix issues with binary plists. #4455 [3.6] bpo-32072: Fix issues with binary plists. (GH-4455) #4654 [3.5] bpo-32072: Fix issues with binary plists. (GH-4455) #4656 [3.4] bpo-32072: Fix issues with binary plists. (GH-4455) #4658

^{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 = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2018-01-31.15:42:32.404>
created_at = <Date 2017-11-18.19:06:16.453>
labels = ['type-security', '3.7', 'library']
title = 'Issues with binary plists'
updated_at = <Date 2019-05-10.18:17:40.640>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2019-05-10.18:17:40.640>
actor = 'ned.deily'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2018-01-31.15:42:32.404>
closer = 'serhiy.storchaka'
components = ['Library (Lib)']
creation = <Date 2017-11-18.19:06:16.453>
creator = 'serhiy.storchaka'
dependencies = []
files = []
hgrepos = []
issue_num = 32072
keywords = ['patch']
message_count = 10.0
messages = ['306493', '307145', '307344', '307347', '309059', '310411', '310497', '311336', '311339', '311595']
nosy_count = 4.0
nosy_names = ['ronaldoussoren', 'larry', 'ned.deily', 'serhiy.storchaka']
pr_nums = ['4455', '4654', '4656', '4658']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue32072'
versions = ['Python 3.4', 'Python 3.5', 'Python 3.6', 'Python 3.7']

[serhiy-storchaka]

plistlib creates a new objects when read references instead of using
already read object.

As result it doesn't preserve identity:

>>> import plistlib
>>> a = [['spam']]*2
>>> a[0] is a[1]
True
>>> b = plistlib.loads(plistlib.dumps(a, fmt=plistlib.FMT_BINARY))
>>> b == a
True
>>> b[0] is b[1]
False

And plistlib.loads() is vulnerable to plists containing cyclic
references (as was exposed in bpo-31897). For example,
plistlib.loads(b'bplist00\xa1\x00\x08\x00\x00\x00\x00\x00\x00\x01\x01\x00\x00\x00\x00\x00\x00\x00\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x0a')
could return a list containing itself, but it is failed with
RecursionError.

plistlib.dumps() preserves reference in the output, but it saves
redundant copies. For example plistlib.dumps([[]]*5,
fmt=plistlib.FMT_BINARY) saves a list containing 5 identical empty
lists, but it saves an empty list 5 times, and only the last copy is
used. The other 4 copies are not referenced and just spent the file
volume and the space of reference numbers. Saving
[[[[['spam']*100]*100]*100]*100]*100 will result in a multigigabyte,
while less than a kilobyte would be enough for saving it. Loading
properly saved [[[[['spam']*100]*100]*100]*100]*100 withe the current
plistlib.loads() will cause consuming many gigabytes of memory.

1. The issues with plistlib.dumps() are:
   1a) Inefficient saving data with references. This is minor resource usage issue.
   1b) Impossibility to save a data with cyclic references. This is a
   lack of a feature.

2. The issues with plistlib.loads() are:
   2a) Inefficient loading data with references. This can be not just a
   resource usage issue, but a security issue. Loading an malicious input
   data smaller than 100 byte ([[[...]*2]*2]*2) can cause consuming many
   gigabytes of memory.
   2b) Impossibility to load a data with cyclic references. This is a
   lack of a feature, but can be lesser security issue. Small malicious
   input can cause RecursionError. If the recursion limit is set high and
   you are unlucky it can cause a stack overflow.

Security issues affect you only when you load plists from untrusted sources.

Adding the proper support of references could be considered a new
feature, but taking to account security issues it should be backported
up to 3.4 when the support of binary plists was added.

[serhiy-storchaka]

Ronald, could you please make a review? I want to merge this before 3.7.0a3.

[serhiy-storchaka]

New changeset a897aee by Serhiy Storchaka in branch 'master':
bpo-32072: Fix issues with binary plists. (bpo-4455)
a897aee

[serhiy-storchaka]

New changeset 8cd3108 by Serhiy Storchaka (Miss Islington (bot)) in branch '3.6':
bpo-32072: Fix issues with binary plists. (GH-4455) (bpo-4654)
8cd3108

[serhiy-storchaka]

For example:

a = []
for i in range(22):
    a = [a, a]

b = plistlib.dumps(a, fmt=plistlib.FMT_BINARY)

The result is 130 bytes long on patched plistlib. But plistlib.dumps(b) will expand to a structure consuming almost a gigabyte of memory on unpatched plistlib. Increasing the level of nesting by one will duplicate memory consumption, so it is easy to consume all available memory on any computer.

[larryhastings]

New changeset c59731d by larryhastings (Serhiy Storchaka) in branch '3.4':
[3.4] bpo-32072: Fix issues with binary plists. (GH-4455) (bpo-4658)
c59731d

[larryhastings]

New changeset 43f014d by larryhastings (Serhiy Storchaka) in branch '3.5':
[3.5] bpo-32072: Fix issues with binary plists. (GH-4455) (bpo-4656)
43f014d

[serhiy-storchaka]

Thanks Larry.

[larryhastings]

Thank you for the fix! I just wish I knew what plists were ;-)

[ronaldoussoren]

@larry: plists are Apple's equivalent to Windows INI files ;-)