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

marshal load bypass code.__new__ audit event #85352

Closed
tkmikan mannequin opened this issue Jul 1, 2020 · 13 comments
Closed

marshal load bypass code.__new__ audit event #85352

tkmikan mannequin opened this issue Jul 1, 2020 · 13 comments
Assignees
Labels
3.8 only security fixes 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes easy type-security A security issue

Comments

@tkmikan
Copy link
Mannequin

tkmikan mannequin commented Jul 1, 2020

BPO 41180
Nosy @gvanrossum, @zooba, @miss-islington, @tkmikan
PRs
  • bpo-41180: Audit code.__new__ when unmarshalling #21271
  • [3.8] bpo-41180: Audit code.__new__ when unmarshalling (GH-21271) #21300
  • [3.9] bpo-41180: Audit code.__new__ when unmarshalling (GH-21271) #21301
  • bpo-41180: Replace marshal code.__new__ audit event with marshal.load[s] and marshal.dumps #26961
  • [3.10] bpo-41180: Replace marshal code.__new__ audit event with marshal.load[s] and marshal.dumps (GH-26961) #26970
  • [3.9] bpo-41180: Replace marshal code.__new__ audit event with marshal.load[s] and marshal.dumps (GH-26961) #26971
  • bpo-41180: Fixes documentation from earlier change #26972
  • 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 = 'https://github.com/zooba'
    closed_at = <Date 2021-06-30.19:37:54.572>
    created_at = <Date 2020-07-01.06:58:32.166>
    labels = ['type-security', 'easy', '3.8', '3.9', '3.10', '3.11']
    title = 'marshal load bypass code.__new__ audit event'
    updated_at = <Date 2021-06-30.19:37:54.571>
    user = 'https://github.com/tkmikan'

    bugs.python.org fields:

    activity = <Date 2021-06-30.19:37:54.571>
    actor = 'steve.dower'
    assignee = 'steve.dower'
    closed = True
    closed_date = <Date 2021-06-30.19:37:54.572>
    closer = 'steve.dower'
    components = []
    creation = <Date 2020-07-01.06:58:32.166>
    creator = 'tkmk'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 41180
    keywords = ['patch', 'easy (C)']
    message_count = 13.0
    messages = ['372733', '372771', '372876', '372900', '372956', '372957', '372959', '372960', '396757', '396782', '396786', '396787', '396788']
    nosy_count = 4.0
    nosy_names = ['gvanrossum', 'steve.dower', 'miss-islington', 'tkmk']
    pr_nums = ['21271', '21300', '21301', '26961', '26970', '26971', '26972']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'security'
    url = 'https://bugs.python.org/issue41180'
    versions = ['Python 3.8', 'Python 3.9', 'Python 3.10', 'Python 3.11']

    @tkmikan
    Copy link
    Mannequin Author

    tkmikan mannequin commented Jul 1, 2020

    While code.__new__ is being audited, using marshal.loads to create a code object will trigger no events. Therefore, either marshal.load(s) event itself should be audited, or code.__new__ should be triggered when marshal type is TYPE_CODE.

    Considering that importing from a pyc file also relys on unmarshalling code objects, and they have already been audited as import, I'm also wondering if auditing twice should be avoided for performance.

    @tkmikan tkmikan mannequin added 3.10 only security fixes 3.8 only security fixes 3.9 only security fixes type-security A security issue labels Jul 1, 2020
    @zooba
    Copy link
    Member

    zooba commented Jul 1, 2020

    I like using the existing event for unmarshalling code objects, assuming we have all the arguments available.

    I'm not sure whether it's worth auditing all marshal.load() calls (just as we don't audit all pickle.load() calls). But depending on the code paths we may not have a choice.

    Auditing the .pyc import twice (or more) is basically unavoidable, but it's not terrible anyway. When no hooks are set, it's negligible impact, and when someone adds a hook they are in control of how much work it does. Providing both events means they can choose to only handle one and still get the coverage.

    Marking this as easy (C), since the hardest part is just going to be finding the right place to add the PySys_Audit call.

    @zooba zooba added easy labels Jul 1, 2020
    @zooba
    Copy link
    Member

    zooba commented Jul 2, 2020

    Actually, a quick search of codeobject.c and a look at tkmk's PR makes it seem like the audit event should be being raised from inside PyCode_NewWithPosOnlyArgs anyway (which IIRC didn't exist when I first added the event, though it was probably there before it was merged).

    In general, events should be raised after parameters have been validated, but before any work is done. And when all the other calls feed through a single function, just auditing that one function is enough.

    @tkmikan
    Copy link
    Mannequin Author

    tkmikan mannequin commented Jul 3, 2020

    Before this, we only audit code.__new__ and code.replace, as these methods allow constructing arbitrary code objects, and we don't audit code object coming from the normal way (like compile,exec,eval).
    If the event is raised in PyCode_NewWithPosOnlyArgs, is it ok that the compiled code is also audited?

    @zooba
    Copy link
    Member

    zooba commented Jul 3, 2020

    Ah, you're right. Thanks for double checking me :)

    I'll merge the PR and do the backports. Thanks!

    @zooba
    Copy link
    Member

    zooba commented Jul 3, 2020

    New changeset d160e0f by tkmikan in branch 'master':
    bpo-41180: Audit code.__new__ when unmarshalling (GH-21271)
    d160e0f

    @miss-islington
    Copy link
    Contributor

    New changeset c1d9165 by Miss Islington (bot) in branch '3.8':
    bpo-41180: Audit code.__new__ when unmarshalling (GH-21271)
    c1d9165

    @miss-islington
    Copy link
    Contributor

    New changeset 1c77654 by Miss Islington (bot) in branch '3.9':
    bpo-41180: Audit code.__new__ when unmarshalling (GH-21271)
    1c77654

    @tkmikan tkmikan mannequin closed this as completed Mar 5, 2021
    @tkmikan tkmikan mannequin closed this as completed Mar 5, 2021
    @zooba
    Copy link
    Member

    zooba commented Jun 29, 2021

    I'm going to revert this and replace it with a marshal.loads (and dumps) event instead.

    The performance impact on loading .pyc files is too great, as it triggers the hook for each function. Without severely modifying importlib we can't bypass the call that's accessible to Python code, and the important part to detect is marshal calls outside of regular imports.

    And wanted to mention that Yunfan Zhan suggested adding these events originally and I chose to go the other way. So my bad, and let's get it right now!

    @zooba zooba added the 3.11 only security fixes label Jun 29, 2021
    @zooba zooba reopened this Jun 29, 2021
    @zooba zooba self-assigned this Jun 29, 2021
    @zooba zooba added the 3.11 only security fixes label Jun 29, 2021
    @zooba zooba reopened this Jun 29, 2021
    @zooba zooba self-assigned this Jun 29, 2021
    @zooba
    Copy link
    Member

    zooba commented Jun 30, 2021

    New changeset 139de04 by Steve Dower in branch 'main':
    bpo-41180: Replace marshal code.__new__ audit event with marshal.load[s] and marshal.dumps (GH-26961)
    139de04

    @zooba
    Copy link
    Member

    zooba commented Jun 30, 2021

    New changeset a5764d3 by Steve Dower in branch '3.10':
    bpo-41180: Replace marshal code.__new__ audit event with marshal.load[s] and marshal.dumps (GH-26970)
    a5764d3

    @zooba
    Copy link
    Member

    zooba commented Jun 30, 2021

    New changeset 863e3d5 by Steve Dower in branch '3.9':
    bpo-41180: Replace marshal code.__new__ audit event with marshal.load[s] and marshal.dumps (GH-26971)
    863e3d5

    @zooba
    Copy link
    Member

    zooba commented Jun 30, 2021

    New changeset 95919b0 by Steve Dower in branch 'main':
    bpo-41180: Fixes documentation to specify correct event name and add versionchanged (GH-26972)
    95919b0

    @zooba zooba closed this as completed Jun 30, 2021
    @zooba zooba closed this as completed Jun 30, 2021
    @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 only security fixes 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes easy type-security A security issue
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants