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

Many command execution functions are not raising auditing events #83365

Closed
gousaiyang mannequin opened this issue Jan 1, 2020 · 12 comments
Closed

Many command execution functions are not raising auditing events #83365

gousaiyang mannequin opened this issue Jan 1, 2020 · 12 comments
Labels
3.8 only security fixes 3.9 only security fixes type-security A security issue

Comments

@gousaiyang
Copy link
Mannequin

gousaiyang mannequin commented Jan 1, 2020

BPO 39184
Nosy @tiran, @zooba, @miss-islington, @gousaiyang
PRs
  • bpo-39184: Add audit events to command execution functions in os and pty modules #17824
  • [3.8] bpo-39184: Add audit events to command execution functions in os and pty modules (GH-17824) #18353
  • bpo-39184: Add audit events to functions in fcntl, msvcrt, os, resource, shutil, signal, syslog #18407
  • [3.8] bpo-39184: Add audit events to functions in fcntl, msvcrt, os, resource, shutil, signal, syslog (GH-18407) #18500
  • bpo-39184: Fix incorrect return value #18580
  • [3.8] bpo-39184: Fix incorrect return value (GH-18580) #18582
  • gh-83365: Fix incorrect return value in msvcrt_get_osfhandle_impl #21322
  • 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 = None
    created_at = <Date 2020-01-01.23:10:03.706>
    labels = ['type-security', '3.8', '3.9']
    title = 'Many command execution functions are not raising auditing events'
    updated_at = <Date 2020-07-04.22:27:25.686>
    user = 'https://github.com/gousaiyang'

    bugs.python.org fields:

    activity = <Date 2020-07-04.22:27:25.686>
    actor = 'gousaiyang'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = []
    creation = <Date 2020-01-01.23:10:03.706>
    creator = 'gousaiyang'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 39184
    keywords = ['patch']
    message_count = 12.0
    messages = ['359177', '359246', '359311', '361385', '361386', '361391', '361616', '361940', '361945', '362357', '362358', '373000']
    nosy_count = 4.0
    nosy_names = ['christian.heimes', 'steve.dower', 'miss-islington', 'gousaiyang']
    pr_nums = ['17824', '18353', '18407', '18500', '18580', '18582', '21322']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'security'
    url = 'https://bugs.python.org/issue39184'
    versions = ['Python 3.8', 'Python 3.9']

    @gousaiyang
    Copy link
    Mannequin Author

    gousaiyang mannequin commented Jan 1, 2020

    Similar to os.system (which is already raising auditing event), the following functions are also capable of command execution, so they also need auditing:

    • os.execl
    • os.execle
    • os.execlp
    • os.execlpe
    • os.execv
    • os.execve
    • os.execvp
    • os.execvpe
    • os.posix_spawn
    • os.posix_spawnp
    • os.spawnl
    • os.spawnle
    • os.spawnlp
    • os.spawnlpe
    • os.spawnv
    • os.spawnve
    • os.spawnvp
    • os.spawnvpe
    • os.startfile
    • pty.spawn

    By the way, since os.listdir, shutil.copytree and shutil.rmtree are already being audited, is it necessary to audit file operations in the os module like os.remove?

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

    zooba commented Jan 3, 2020

    Agreed, we should add events for all of these. They can go into the next 3.8 release.

    Ideally, the lowest level operations (typically os module) should raise events. Where it's convenient to also audit operations at a higher level (shutil, subprocess, etc.) then we can do that too, as it provides helpful context for *why* the lower-level operation ran.

    Contributions welcome :) I'll likely get to them eventually, but no reason it has to be me adding everything.

    @gousaiyang
    Copy link
    Mannequin Author

    gousaiyang mannequin commented Jan 5, 2020

    I have made PR 17824 to add auditing events for the command execution functions mentioned above.

    After a review on other related Python modules, I think maybe the following functions can also be audited, but a discussion may be required to determine whether they are necessary (whether these actions are sensitive enough to record, and performance trade off).

    • os.getenv/putenv/unsetenv
    • os.getcwd/chdir
    • os.chown/chmod
    • os.stat/access
    • os.link/symlink
    • os.rename/renames/replace
    • os.mkdir/mkdirs
    • os.remove/removedirs/rmdir/unlink (shutil.rmtree is already audited)
    • os.add_dll_directory
    • os.fork
    • os.kill/killpg
    • os.path.exists/isfile/isdir/...
    • signal.pthread_kill
    • shutil.copy* (shutil.copytree is already audited)
    • shutil.move
    • shutil.chown
    • shutil.unpack_archive (shutil.make_archive is already audited)
    • resource.prlimit
    • file operations in msvcrt
    • functions in fcntl, syslog
    • many high level networking modules such as http.client/server, socketserver, xmlrpc (the low-level socket calls are already audited)

    @zooba
    Copy link
    Member

    zooba commented Feb 5, 2020

    New changeset 95f6001 by Saiyang Gou in branch 'master':
    bpo-39184: Add audit events to command execution functions in os and pty modules (GH-17824)
    95f6001

    @zooba
    Copy link
    Member

    zooba commented Feb 5, 2020

    Thanks for the spawn patch, I've merged it.

    On the second list, I'd say go for it. The only one I'd skip are the stat() calls (and those that just do a stat call, such as exists/isfile, etc.), and getcwd() (which has many other ways to implicitly use the information). Maybe skip getenv as well, but modifying the environment is worth collecting.

    @miss-islington
    Copy link
    Contributor

    New changeset 3498ac5 by Miss Islington (bot) in branch '3.8':
    bpo-39184: Add audit events to command execution functions in os and pty modules (GH-17824)
    3498ac5

    @gousaiyang
    Copy link
    Mannequin Author

    gousaiyang mannequin commented Feb 8, 2020

    Thanks for your review! PR 18407 is for the second list. For now I haven't added audit hooks for the http, socketserver and xmlrpc modules because they look a bit complex. There seems to be so many classes and methods to hook, we may need to find good places to hook (similar to what has been done on ftplib, imaplib, nntplib, poplib, smtplib, telnetlib and urllib).

    @zooba
    Copy link
    Member

    zooba commented Feb 13, 2020

    New changeset 7514f4f by Saiyang Gou in branch 'master':
    bpo-39184: Add audit events to functions in fcntl, msvcrt, os, resource, shutil, signal, syslog (GH-18407)
    7514f4f

    @zooba
    Copy link
    Member

    zooba commented Feb 13, 2020

    New changeset a00b5be by Steve Dower in branch '3.8':
    bpo-39184: Add audit events to functions in fcntl, msvcrt, os, resource, shutil, signal, syslog (GH-18407)
    a00b5be

    @miss-islington
    Copy link
    Contributor

    New changeset 6c444d0 by Steve Dower in branch 'master':
    bpo-39184: Fix incorrect return value (GH-18580)
    6c444d0

    @miss-islington
    Copy link
    Contributor

    New changeset d0a464e by Miss Islington (bot) in branch '3.8':
    bpo-39184: Fix incorrect return value (GH-18580)
    d0a464e

    @gousaiyang
    Copy link
    Mannequin Author

    gousaiyang mannequin commented Jul 4, 2020

    Since the original problem (command execution functions missing audit events) is already solved, we can close this issue now. Further discussions on additional audit hooks (e.g. for the networking modules) could go to bpo-37363.

    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 type-security A security issue
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants