classification
Title: [security] Add audit events on GC functions giving access to all Python objects
Type: Stage: resolved
Components: Library (Lib) Versions: Python 3.10, Python 3.9, Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: christian.heimes, gousaiyang, miss-islington, miss-islington, pablogsal, steve.dower, vstinner, zkonge
Priority: normal Keywords: patch

Created on 2021-03-08 20:32 by vstinner, last changed 2021-03-14 05:28 by pablogsal. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 24794 merged pablogsal, 2021-03-08 23:06
PR 24810 merged pablogsal, 2021-03-10 00:55
PR 24811 merged pablogsal, 2021-03-10 00:56
PR 24836 merged pablogsal, 2021-03-13 05:18
PR 24854 merged pablogsal, 2021-03-14 04:04
PR 24855 merged pablogsal, 2021-03-14 04:05
Messages (16)
msg388300 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-03-08 20:32
It is currently possible to discover the internal list of audit hooks using gc module functions, like gc.get_objects(), and so remove an audit hooks, whereas it is supposed to not be possible. The PEP 578 states: "Hooks cannot be removed or replaced."

Rather than attempting to fix this specific vulnerability, I suggest to add new audit events on the following gc functions:

* gc.get_objects()
* gc.get_referrers()
* gc.get_referents()

These functions are "dangerous" since they can expose Python objects in an inconsistent state. In the past, we add multiple bugs related to "internal" tuples which were not fully initialized (but already tracked by the GC). See bpo-15108 for an example.

Note: if someone wants to address the ability to remove an audit hook, the internal list can be modified to not be a Python object.
msg388302 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-03-08 20:40
> Rather than attempting to fix this specific vulnerability, I suggest to add new audit events on the following gc functions:

Makes sense, I will prepare a PR today
msg388307 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2021-03-08 21:33
> Note: if someone wants to address the ability to remove an audit hook, the internal list can be modified to not be a Python object.

I wouldn't bother. There are other ways to modify data structures, e.g. poke into process memory.
msg388321 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2021-03-09 00:51
Thanks, Pablo! Nice patch.

This can be backported, btw. New audit events are allowed to be added in minor releases (they just cannot be changed).
msg388400 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-03-10 00:54
New changeset b4f9089d4aa787c5b74134c98e5f0f11d9e63095 by Pablo Galindo in branch 'master':
bpo-43439: Add audit hooks for gc functions (GH-24794)
https://github.com/python/cpython/commit/b4f9089d4aa787c5b74134c98e5f0f11d9e63095
msg388412 - (view) Author: miss-islington (miss-islington) Date: 2021-03-10 08:50
New changeset a6d0182879d0bf275c4feb38b57f73236ab9c06c by Pablo Galindo in branch '3.8':
[3.8] bpo-43439: Add audit hooks for gc functions (GH-24794). (GH-24810)
https://github.com/python/cpython/commit/a6d0182879d0bf275c4feb38b57f73236ab9c06c
msg388413 - (view) Author: miss-islington (miss-islington) Date: 2021-03-10 08:50
New changeset f814675376318e0bf9e14fc62826a113cb4ca652 by Pablo Galindo in branch '3.9':
[3.9] bpo-43439: Add audit hooks for gc functions (GH-24794). (GH-24811)
https://github.com/python/cpython/commit/f814675376318e0bf9e14fc62826a113cb4ca652
msg388414 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2021-03-10 08:51
Thanks, Pablo and Victor!
msg388528 - (view) Author: Saiyang Gou (gousaiyang) * Date: 2021-03-11 23:08
There is a minor issue here. For gc.get_referrers and gc.get_referents, probably the format code for PySys_Audit should be "(O)" instead of "O". Typically the tuple `args` passed to the hook functions are fixed-length as described in the audit events table. Here `objs` itself is a tuple (containing variable-length arguments) and directly passed to the audit hook with being wrapped by another layer of tuple. If the hook function is to receive a variable-length tuple, probably the documentation should be updated to mention this.
msg388539 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-03-12 10:53
Steve, do you think that makes sense? If so, I will create a batch of PRs to correct it.
msg388550 - (view) Author: Saiyang Gou (gousaiyang) * Date: 2021-03-12 20:48
In addition to the consistency with existing audit hook signatures, there may also be another benefit of wrapping it with a tuple of length 1. If gc.get_referrers or gc.get_referents happens to gain a new keyword-only argument in the future, we may need to add the new argument to the hook args. Extending the `(objs,)` tuple to `(objs, new_kwarg)` is a bit more elegant than appending the `new_kwarg` to the end of the `objs` tuple itself (considering a hook function which tries to be compatible with both the old and the new signature).
msg388568 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2021-03-12 23:50
Passing a tuple as "O" just means it gets passed as-is, without wrapping it again. And yeah, I thought that was right here, but it's not. I didn't realise it's a varargs argument. So yeah, it should be wrapped again so that only a single argument is being passed to the hooks.

Alternatively, raising one event for each object would also be valid. These functions behave identically for "f(a, b)" and "f(a) + f(b)", so raising the event for each object before traversing it would simplify hooks (but have more of a performance impact... which I suspect is totally irrelevant here anyway, so I'd be +1.1 on this approach vs. +1 on passing the args as a single argument).

If new arguments are added later, we need to add new events. Defining event schemas as "arbitrary arguments that may change is the future" is totally against the intended design.
msg388573 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-03-13 00:51
I think I prefer to raise a single event, because it match more closely the actual call, is a bit faster and more straighfoward. I will create a PR soon
msg388651 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-03-14 03:04
New changeset 9c376bc1c4c8bcddb0bc4196b79ec8c75da494a8 by Pablo Galindo in branch 'master':
bpo-43439: Wrapt the tuple in the audit events for the gc module (GH-24836)
https://github.com/python/cpython/commit/9c376bc1c4c8bcddb0bc4196b79ec8c75da494a8
msg388656 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-03-14 04:30
New changeset 1e7a47ab86d5d6a5103e67ba71389f6daa18ea2d by Pablo Galindo in branch '3.8':
[3.8] bpo-43439: Wrapt the tuple in the audit events for the gc module (GH-24836) (GH24854)
https://github.com/python/cpython/commit/1e7a47ab86d5d6a5103e67ba71389f6daa18ea2d
msg388662 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-03-14 05:28
New changeset e6bf1e1001a6844a36f2f90f58ab12b9e09e3887 by Pablo Galindo in branch '3.9':
[3.9] bpo-43439: Wrapt the tuple in the audit events for the gc module (GH-24836) (GH-24855)
https://github.com/python/cpython/commit/e6bf1e1001a6844a36f2f90f58ab12b9e09e3887
History
Date User Action Args
2021-03-14 05:28:40pablogsalsetmessages: + msg388662
2021-03-14 04:32:53pablogsalsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2021-03-14 04:30:46pablogsalsetmessages: + msg388656
2021-03-14 04:05:10pablogsalsetpull_requests: + pull_request23615
2021-03-14 04:04:25pablogsalsetpull_requests: + pull_request23614
2021-03-14 03:04:54pablogsalsetmessages: + msg388651
2021-03-13 05:18:04pablogsalsetstage: needs patch -> patch review
pull_requests: + pull_request23601
2021-03-13 00:51:00pablogsalsetmessages: + msg388573
2021-03-12 23:50:11steve.dowersetstatus: closed -> open
resolution: fixed -> (no value)
messages: + msg388568

stage: resolved -> needs patch
2021-03-12 20:48:22gousaiyangsetmessages: + msg388550
2021-03-12 10:53:11pablogsalsetmessages: + msg388539
2021-03-11 23:08:36gousaiyangsetnosy: + gousaiyang
messages: + msg388528
2021-03-10 08:51:21christian.heimessetstatus: open -> closed
resolution: fixed
messages: + msg388414

stage: patch review -> resolved
2021-03-10 08:50:48miss-islingtonsetnosy: + miss-islington
messages: + msg388413
2021-03-10 08:50:47miss-islingtonsetnosy: + miss-islington
messages: + msg388412
2021-03-10 00:56:35pablogsalsetpull_requests: + pull_request23578
2021-03-10 00:55:49pablogsalsetpull_requests: + pull_request23577
2021-03-10 00:54:03pablogsalsetmessages: + msg388400
2021-03-09 14:27:50zkongesetnosy: + zkonge
2021-03-09 00:51:18steve.dowersetmessages: + msg388321
versions: + Python 3.8, Python 3.9
2021-03-08 23:06:41pablogsalsetkeywords: + patch
stage: patch review
pull_requests: + pull_request23562
2021-03-08 21:33:11christian.heimessetmessages: + msg388307
2021-03-08 20:40:32pablogsalsetmessages: + msg388302
2021-03-08 20:32:28vstinnercreate