classification
Title: Insufficient validation in _posixsubprocess.fork_exec()
Type: crash Stage: resolved
Components: Extension Modules Versions: Python 3.7, Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: gregory.p.smith, serhiy.storchaka
Priority: normal Keywords:

Created on 2017-04-13 10:14 by serhiy.storchaka, last changed 2017-04-19 21:52 by serhiy.storchaka. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 1110 merged serhiy.storchaka, 2017-04-13 10:54
PR 1186 merged serhiy.storchaka, 2017-04-19 20:23
PR 1190 merged serhiy.storchaka, 2017-04-19 21:21
Messages (6)
msg291595 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-04-13 10:14
_posixsubprocess.fork_exec() takes a sequence of file descriptors. It first validates it, and since the validation is passed uses it without checking for errors. But since __len__, __getitem__ and __int__ can execute user code and release GIL, errors can occur after the validation. This can cause a crash.

Proposed patch fixes this by the simplest way -- it restricts the type of a sequence to tuple and types of elements to int. Since _posixsubprocess is private module this shouldn't break third-party code.

Other issue with _posixsubprocess.fork_exec() was that it converts args to a tuple or a list and iterate it without checking if the size is changed.
msg291626 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2017-04-13 18:37
nice find.  did anyone's code actually run into this issue?
msg291627 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-04-13 18:56
It is hard to reproduce (especially the second issue) since in all cases in the stdlib a list passed to fork_exec() is just created by sorted() and doesn't have other references. But if someone is so insane that passes int-like objects with non-idempotent __int__ as file descriptors his can get a crash in debug build (or mystical bugs in release build). Added tests utilizes this, but this unlikely happens in real code.

I have found this issue during analyzing usages of PyObject_Size(), PySequence_Size() and PyMapping_Size() in issue30061.
msg291891 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-04-19 18:12
New changeset 66bffd1663489d080349debbf1b472d432351038 by Serhiy Storchaka in branch 'master':
bpo-30065: Fixed arguments validation in _posixsubprocess.fork_exec(). (#1110)
https://github.com/python/cpython/commit/66bffd1663489d080349debbf1b472d432351038
msg291904 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-04-19 20:59
New changeset e2546172622dd52692cf0e26c2b931f942f345b6 by Serhiy Storchaka in branch '3.6':
[3.6] bpo-30065: Fixed arguments validation in _posixsubprocess.fork_exec(). (GH-1110) (#1186)
https://github.com/python/cpython/commit/e2546172622dd52692cf0e26c2b931f942f345b6
msg291911 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-04-19 21:51
New changeset c97c1914f401359f2a7e6c8e0364e71ad9fb5bc8 by Serhiy Storchaka in branch '3.5':
[3.5] bpo-30065: Fixed arguments validation in _posixsubprocess.fork_exec(). (GH-1110) (#1190)
https://github.com/python/cpython/commit/c97c1914f401359f2a7e6c8e0364e71ad9fb5bc8
History
Date User Action Args
2017-04-19 21:52:48serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2017-04-19 21:51:07serhiy.storchakasetmessages: + msg291911
2017-04-19 21:21:24serhiy.storchakasetpull_requests: + pull_request1318
2017-04-19 20:59:04serhiy.storchakasetmessages: + msg291904
2017-04-19 20:23:51serhiy.storchakasetpull_requests: + pull_request1315
2017-04-19 18:12:48serhiy.storchakasetmessages: + msg291891
2017-04-16 09:08:50serhiy.storchakasetassignee: serhiy.storchaka
2017-04-13 18:56:32serhiy.storchakasetmessages: + msg291627
2017-04-13 18:37:30gregory.p.smithsetnosy: + gregory.p.smith
messages: + msg291626
2017-04-13 10:54:54serhiy.storchakasetpull_requests: + pull_request1251
2017-04-13 10:14:01serhiy.storchakacreate