classification
Title: Issues with binary plists
Type: security Stage: resolved
Components: Library (Lib) Versions: Python 3.7, Python 3.6, Python 3.4, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: larry, ned.deily, ronaldoussoren, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2017-11-18 19:06 by serhiy.storchaka, last changed 2019-05-10 18:17 by ned.deily. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 4455 merged serhiy.storchaka, 2017-11-18 19:16
PR 4654 merged python-dev, 2017-11-30 21:26
PR 4656 merged serhiy.storchaka, 2017-11-30 21:43
PR 4658 merged serhiy.storchaka, 2017-11-30 21:50
Messages (10)
msg306493 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-11-18 19:06
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 issue31897). 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.
msg307145 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-11-28 17:09
Ronald, could you please make a review? I want to merge this before 3.7.0a3.
msg307344 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-11-30 21:26
New changeset a897aeeef647259a938a36cb5eb6680c86021c6a by Serhiy Storchaka in branch 'master':
bpo-32072: Fix issues with binary plists. (#4455)
https://github.com/python/cpython/commit/a897aeeef647259a938a36cb5eb6680c86021c6a
msg307347 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-11-30 22:15
New changeset 8cd31082fba88cf0064590fd3d55b6c1c964f11c by Serhiy Storchaka (Miss Islington (bot)) in branch '3.6':
bpo-32072: Fix issues with binary plists. (GH-4455) (#4654)
https://github.com/python/cpython/commit/8cd31082fba88cf0064590fd3d55b6c1c964f11c
msg309059 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-12-26 11:03
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.
msg310411 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2018-01-22 10:18
New changeset c59731d92dc73111d224876f1caa064097aad786 by larryhastings (Serhiy Storchaka) in branch '3.4':
[3.4] bpo-32072: Fix issues with binary plists. (GH-4455) (#4658)
https://github.com/python/cpython/commit/c59731d92dc73111d224876f1caa064097aad786
msg310497 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2018-01-23 11:21
New changeset 43f014d3f12468edf61046f0612edc7660042fd5 by larryhastings (Serhiy Storchaka) in branch '3.5':
[3.5] bpo-32072: Fix issues with binary plists. (GH-4455) (#4656)
https://github.com/python/cpython/commit/43f014d3f12468edf61046f0612edc7660042fd5
msg311336 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-01-31 15:42
Thanks Larry.
msg311339 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2018-01-31 16:06
Thank you for the fix!  I just wish I knew what plists were ;-)
msg311595 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2018-02-04 10:33
@larry: plists are Apple's equivalent to Windows INI files ;-)
History
Date User Action Args
2019-05-10 18:17:40ned.deilysetmessages: - msg342093
2019-05-10 17:36:38ned.deilysetmessages: + msg342093
2018-02-04 10:33:45ronaldoussorensetmessages: + msg311595
2018-01-31 16:06:43larrysetmessages: + msg311339
2018-01-31 15:42:32serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg311336

stage: patch review -> resolved
2018-01-23 11:21:23larrysetmessages: + msg310497
2018-01-22 10:18:07larrysetmessages: + msg310411
2017-12-26 11:03:38serhiy.storchakasetmessages: + msg309059
2017-12-25 09:32:28serhiy.storchakalinkissue31988 superseder
2017-11-30 22:15:35serhiy.storchakasetmessages: + msg307347
2017-11-30 21:50:34serhiy.storchakasetpull_requests: + pull_request4570
2017-11-30 21:43:52serhiy.storchakasetpull_requests: + pull_request4568
2017-11-30 21:28:49serhiy.storchakasetnosy: + larry
2017-11-30 21:26:20python-devsetpull_requests: + pull_request4566
2017-11-30 21:26:13serhiy.storchakasetmessages: + msg307344
2017-11-28 17:09:37serhiy.storchakasetmessages: + msg307145
2017-11-18 19:16:37serhiy.storchakasetkeywords: + patch
stage: patch review
pull_requests: + pull_request4393
2017-11-18 19:06:16serhiy.storchakacreate