Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[security] Add audit events on GC functions giving access to all Python objects #87605

Closed
vstinner opened this issue Mar 8, 2021 · 16 comments
Closed
Labels
3.8 3.9 3.10 stdlib

Comments

@vstinner
Copy link
Member

@vstinner vstinner commented Mar 8, 2021

BPO 43439
Nosy @vstinner, @tiran, @zooba, @pablogsal, @miss-islington, @miss-islington, @zkonge, @gousaiyang
PRs
  • #24794
  • #24810
  • #24811
  • #24836
  • #24854
  • #24855
  • 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 = None
    closed_at = <Date 2021-03-14.04:32:53.032>
    created_at = <Date 2021-03-08.20:32:28.568>
    labels = ['3.8', 'library', '3.9', '3.10']
    title = '[security] Add audit events on GC functions giving access to all Python objects'
    updated_at = <Date 2021-03-14.05:28:40.845>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2021-03-14.05:28:40.845>
    actor = 'pablogsal'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-03-14.04:32:53.032>
    closer = 'pablogsal'
    components = ['Library (Lib)']
    creation = <Date 2021-03-08.20:32:28.568>
    creator = 'vstinner'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 43439
    keywords = ['patch']
    message_count = 16.0
    messages = ['388300', '388302', '388307', '388321', '388400', '388412', '388413', '388414', '388528', '388539', '388550', '388568', '388573', '388651', '388656', '388662']
    nosy_count = 7.0
    nosy_names = ['vstinner', 'christian.heimes', 'steve.dower', 'pablogsal', 'miss-islington', 'miss-islington', 'zkonge', 'gousaiyang']
    pr_nums = ['24794', '24810', '24811', '24836', '24854', '24855']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue43439'
    versions = ['Python 3.8', 'Python 3.9', 'Python 3.10']

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Mar 8, 2021

    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.

    @vstinner vstinner added 3.10 stdlib labels Mar 8, 2021
    @pablogsal
    Copy link
    Member

    @pablogsal pablogsal commented Mar 8, 2021

    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

    @tiran
    Copy link
    Member

    @tiran tiran commented Mar 8, 2021

    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.

    @zooba
    Copy link
    Member

    @zooba zooba commented Mar 9, 2021

    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).

    @pablogsal
    Copy link
    Member

    @pablogsal pablogsal commented Mar 10, 2021

    New changeset b4f9089 by Pablo Galindo in branch 'master':
    bpo-43439: Add audit hooks for gc functions (GH-24794)
    b4f9089

    @miss-islington
    Copy link
    Contributor

    @miss-islington miss-islington commented Mar 10, 2021

    New changeset a6d0182 by Pablo Galindo in branch '3.8':
    [3.8] bpo-43439: Add audit hooks for gc functions (GH-24794). (GH-24810)
    a6d0182

    @miss-islington
    Copy link
    Contributor

    @miss-islington miss-islington commented Mar 10, 2021

    New changeset f814675 by Pablo Galindo in branch '3.9':
    [3.9] bpo-43439: Add audit hooks for gc functions (GH-24794). (GH-24811)
    f814675

    @tiran
    Copy link
    Member

    @tiran tiran commented Mar 10, 2021

    Thanks, Pablo and Victor!

    @tiran tiran closed this as completed Mar 10, 2021
    @gousaiyang
    Copy link
    Mannequin

    @gousaiyang gousaiyang mannequin commented Mar 11, 2021

    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.

    @pablogsal
    Copy link
    Member

    @pablogsal pablogsal commented Mar 12, 2021

    Steve, do you think that makes sense? If so, I will create a batch of PRs to correct it.

    @gousaiyang
    Copy link
    Mannequin

    @gousaiyang gousaiyang mannequin commented Mar 12, 2021

    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).

    @zooba
    Copy link
    Member

    @zooba zooba commented Mar 12, 2021

    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.

    @zooba zooba reopened this Mar 12, 2021
    @pablogsal
    Copy link
    Member

    @pablogsal pablogsal commented Mar 13, 2021

    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

    @pablogsal
    Copy link
    Member

    @pablogsal pablogsal commented Mar 14, 2021

    New changeset 9c376bc by Pablo Galindo in branch 'master':
    bpo-43439: Wrapt the tuple in the audit events for the gc module (GH-24836)
    9c376bc

    @pablogsal
    Copy link
    Member

    @pablogsal pablogsal commented Mar 14, 2021

    New changeset 1e7a47a 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)
    1e7a47a

    @pablogsal
    Copy link
    Member

    @pablogsal pablogsal commented Mar 14, 2021

    New changeset e6bf1e1 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)
    e6bf1e1

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 3.9 3.10 stdlib
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants