classification
Title: Arbitrary code execution vulnerability due to unchecked eval() call in dumbdbm module
Type: security Stage: resolved
Components: Library (Lib) Versions: Python 3.5, Python 3.4, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: Arfrever, Claudiu.Popa, Guido.van.Rossum, gvanrossum, haypo, lemburg, python-dev, r.david.murray, rhettinger, serhiy.storchaka, stephen.farris
Priority: high Keywords: patch

Created on 2014-11-16 18:39 by stephen.farris, last changed 2015-02-22 16:12 by Arfrever. This issue is now closed.

Files
File name Uploaded Description Edit
issue22885.patch Claudiu.Popa, 2015-01-21 22:07
issue22885_1.patch Claudiu.Popa, 2015-01-21 22:34
Messages (11)
msg231255 - (view) Author: Stephen Farris (stephen.farris) Date: 2014-11-16 18:39
The dumbdbm module uses an unchecked call to eval() in the _update method, which is called in response to a call to dumbdbm.open(), and is used to load the index from the directory file.  This poses a security vulnerability because it allows an attacker to execute arbitrary code on the victim's machine by inserting python code into the DBM directory file.  This vulnerability could allow an attacker to execute arbitrary commands on the victim machine, potentially allowing them to deploy malware, gain system access, destroy files and data, expose sensitive information, etc.
msg234446 - (view) Author: Claudiu Popa (Claudiu.Popa) * Date: 2015-01-21 22:07
Here's a patch which uses ast.literal_eval instead. This doesn't get code executed, since literal_eval will fail loudly for anything other than a literal. There are some issues to consider:

- let the current ast.literal_eval call bubble out with a lot of different exceptions
- normalize the exception to dbm.dumb.error.

I'm leaning towards the first, since it clearly shows that something bad happened in the module and it's a first indicator that someone tampered with the data file.
msg234447 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2015-01-21 22:20
Python 3's exception chaining allows us to do the second (easier to catch without resorting to "except Exception:" or even "except:") while still showing the original exception in the traceback.
msg234450 - (view) Author: Claudiu Popa (Claudiu.Popa) * Date: 2015-01-21 22:34
Thanks for the tip, Guido. The new patch uses exception chaining. If this needs backporting, most probably the first patch can be used.
msg234547 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-01-23 09:25
+            with open(_fname + ".dir", 'w') as stream:
+                stream.write(content)

I don't see where the created file is deleted. Add something like:

self.addCleanup(support.unlink, _fname + ".dir")
msg234551 - (view) Author: Claudiu Popa (Claudiu.Popa) * Date: 2015-01-23 09:53
Thanks, Victor. I thought the same thing, but the file is deleted here already, here: https://hg.python.org/cpython/file/981ba93bcbde/Lib/test/test_dbm_dumb.py#l228
msg234598 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-01-24 09:15
Raising dbm.dumb.error is behavior change. It would be safer not apply this part in maintained releases.

If add sanity checks in 3.5, note that following line "key = key.encode('Latin-1')" can raise an exception too (AttributeError or UnicodeEncodeError). And incorrect data can cause an error later in __getitem__ if pos_and_siz_pair is not a pair of two integers.

I think it is worth to split this issue on two issues and fix only security issue here.
msg234599 - (view) Author: Claudiu Popa (Claudiu.Popa) * Date: 2015-01-24 10:44
Thanks, Serhiy. Only the security issue is fixed in this patch, since eval is replaced by ast.literal_eval and nothing else. Indeed, if the data is something else than what dbm expects after ast.literal_eval, then it would fail, but it would have failed previously too.
msg234600 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-01-24 10:53
I mean that raising dbm.dumb.error is different issue unrelated to changing eval to ast.literal_eval. See also Raymond's objections in issue21708.

issue22885.patch LGTM.
msg236073 - (view) Author: Roundup Robot (python-dev) Date: 2015-02-15 22:34
New changeset 02865d22a98d by Serhiy Storchaka in branch '2.7':
Issue #22885: Fixed arbitrary code execution vulnerability in the dumbdbm
https://hg.python.org/cpython/rev/02865d22a98d

New changeset 693bf15b4314 by Serhiy Storchaka in branch '3.4':
Issue #22885: Fixed arbitrary code execution vulnerability in the dbm.dumb
https://hg.python.org/cpython/rev/693bf15b4314

New changeset cf6a62b0ef3b by Serhiy Storchaka in branch 'default':
Issue #22885: Fixed arbitrary code execution vulnerability in the dbm.dumb
https://hg.python.org/cpython/rev/cf6a62b0ef3b
msg236075 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-02-15 22:40
Committed issue22885.patch with modified test which demonstrates vulnerability of unpatched dbm.dumb. If you want to change exception type raised by dbm.dumb, you can open new issue.
History
Date User Action Args
2015-02-22 16:12:27Arfreversetnosy: + Arfrever
2015-02-15 22:40:44serhiy.storchakasetstatus: open -> closed
versions: + Python 3.4
messages: + msg236075

assignee: serhiy.storchaka
resolution: fixed
stage: patch review -> resolved
2015-02-15 22:34:36python-devsetnosy: + python-dev
messages: + msg236073
2015-01-24 10:53:49serhiy.storchakasetnosy: + rhettinger
messages: + msg234600
2015-01-24 10:44:12Claudiu.Popasetmessages: + msg234599
2015-01-24 09:15:16serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg234598
2015-01-23 09:53:07Claudiu.Popasetmessages: + msg234551
2015-01-23 09:25:13hayposetnosy: + haypo
messages: + msg234547
2015-01-21 22:34:48Claudiu.Popasetfiles: + issue22885_1.patch

messages: + msg234450
2015-01-21 22:20:40gvanrossumsetnosy: + gvanrossum
messages: + msg234447
2015-01-21 22:07:59Claudiu.Popasetpriority: normal -> high
stage: patch review
versions: + Python 3.5
2015-01-21 22:07:01Claudiu.Popasetfiles: + issue22885.patch

nosy: + Claudiu.Popa
messages: + msg234446

keywords: + patch
2014-11-17 03:41:38r.david.murraysetnosy: + r.david.murray
2014-11-16 18:39:00stephen.farriscreate