bpo-39184: Add audit events to functions in `fcntl`, `msvcrt`, `os`, `resource`, `shutil`, `signal`, `syslog` #18407

gousaiyang · 2020-02-07T23:53:03Z

Add audit events to the following functions:

all functions in fcntl and syslog
file operations in msvcrt, os and shutil
os.putenv/unsetenv
os.add_dll_directory
os.fork/forkpty
os.kill/killpg
resource.setrlimit, resource.prlimit
signal.pthread_kill

https://bugs.python.org/issue39184

…`resource`, `shutil`, `signal`, `syslog`

codecov · 2020-02-08T00:36:24Z

Codecov Report

Merging #18407 into master will increase coverage by 1.08%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master   #18407       +/-   ##
===========================================
+ Coverage   82.11%   83.19%    +1.08%     
===========================================
  Files        1954     1570      -384     
  Lines      583559   414425   -169134     
  Branches    44429    44428        -1     
===========================================
- Hits       479190   344783   -134407     
+ Misses      94719    59995    -34724     
+ Partials     9650     9647        -3

Impacted Files	Coverage Δ
Lib/_weakrefset.py	`59.18% <0.00%> (-0.69%)`	⬇️
Lib/test/test_fractions.py	`96.70% <0.00%> (-0.28%)`	⬇️
Python/codecs.c
Objects/sliceobject.c
Modules/termios.c
Modules/clinic/itertoolsmodule.c.h
Modules/_decimal/libmpdec/numbertheory.c
Modules/md5module.c
Include/internal/pycore_ceval.h
Modules/_posixsubprocess.c
... and 377 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2844336...631f22b. Read the comment docs.

zooba

Overall, looks great! I checked all the events against the doc updates, and they all seem to be in good spots in the code.

We definitely should fix the PyLong_FromLong calls to avoid reference leaks.

I'm also 99% sure that we don't need to check path->object for NULL, but if you come back and tell me that we need to then I'll believe you :)

Modules/fcntlmodule.c

zooba · 2020-02-12T09:17:46Z

Modules/posixmodule.c

@@ -3007,6 +3015,13 @@ os_chmod_impl(PyObject *module, path_t *path, int mode, int dir_fd,
        return NULL;
 #endif

+    if (PySys_Audit("os.chmod", "OiO",
+                    path->object ? path->object : Py_None, mode,
+                    dir_fd == DEFAULT_DIR_FD ? Py_None : PyLong_FromLong(dir_fd)


We may just need to pick an "obvious" value to pass rather than None here... it's a shame that DEFAULT_DIR_FD is so arbitrary.

I'd rather not have to manage an object's lifetime around the PySys_Audit call. Passing 0 or -1 (assuming those aren't valid FDs? Are they?) would be obvious enough to me.

According to documentation from Linux, FreeBSD, macOS and Microsoft C Run-Time Library, file descriptors are non-negative integers, negative values are used to indicate errors, so here we can safely use -1 to indicate that dir_fd is not specified.

zooba · 2020-02-12T09:22:02Z

Modules/posixmodule.c

@@ -2911,6 +2911,11 @@ os_chdir_impl(PyObject *module, path_t *path)
 {
    int result;

+    if (PySys_Audit("os.chdir", "(O)",
+                    path->object ? path->object : Py_None) < 0) {


Pretty sure path->object can't be NULL, right? Have you seen a case where this is not true?

I mean, there isn't another way to provide the path if you don't pass an object in...

I see there are a few existing cases where we check it like this, but AFAICT none of them should be necessary.

For path-like arguments with default values (like os.listdir and os.scandir), checking path->object for NULL is necessary, otherwise SystemError: NULL object passed to Py_BuildValue could happen. The audit hooks of os.listdir and os.scandir are already doing this.
For path-like arguments without default values, it seems that path->object could never be NULL since the argument is required. The audit hook in os.open does not check path->object for NULL, and the os_readlink_impl function actually assumed that path->object is not NULL (otherwise the PyUnicode_Check call will crash by dereferencing a null pointer).

Okay, that sounds good to me. Thanks!

bedevere-bot · 2020-02-12T09:33:14Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

- Do not use `PyLong_FromLong` - Use -1 when `dir_fd` is not specified - Remove unnecessary NULL checks

gousaiyang · 2020-02-13T04:28:51Z

I have made the requested changes; please review again. Thanks!

bedevere-bot · 2020-02-13T04:28:54Z

Thanks for making the requested changes!

@zooba: please review the changes made to this pull request.

miss-islington · 2020-02-13T07:47:46Z

Thanks @gousaiyang for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

miss-islington · 2020-02-13T07:47:53Z

Sorry, @gousaiyang and @zooba, I could not cleanly backport this to 3.8 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 7514f4f6254f4a2d13ea8e5632a8e5f22b637e0b 3.8

…`resource`, `shutil`, `signal`, `syslog` (pythonGH-18407)

bedevere-bot · 2020-02-13T08:05:00Z

GH-18500 is a backport of this pull request to the 3.8 branch.

…`resource`, `shutil`, `signal`, `syslog` (GH-18407) Co-authored-by: Saiyang Gou <[email protected]>

bpo-39184: Add audit events to functions in fcntl, msvcrt, os, …

c0fce5c

…`resource`, `shutil`, `signal`, `syslog`

gousaiyang requested a review from a team as a code owner February 7, 2020 23:53

the-knights-who-say-ni added the CLA signed label Feb 7, 2020

bedevere-bot added the awaiting review label Feb 7, 2020

📜🤖 Added by blurb_it.

a7cadc4

gousaiyang requested review from zooba and removed request for a team February 7, 2020 23:55

Use explicit 1-tuple for PySys_Audit

631f22b

zooba requested changes Feb 12, 2020

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Feb 12, 2020

zooba added the needs backport to 3.8 label Feb 12, 2020

Review on PR 18407

8a90419

- Do not use `PyLong_FromLong` - Use -1 when `dir_fd` is not specified - Remove unnecessary NULL checks

bedevere-bot removed the awaiting changes label Feb 13, 2020

bedevere-bot added the awaiting change review label Feb 13, 2020

bedevere-bot requested a review from zooba February 13, 2020 04:28

zooba approved these changes Feb 13, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Feb 13, 2020

zooba merged commit 7514f4f into python:master Feb 13, 2020

bedevere-bot removed the awaiting merge label Feb 13, 2020

miss-islington assigned zooba Feb 13, 2020

gousaiyang deleted the bpo-39184-file-misc branch February 13, 2020 07:52

zooba pushed a commit to zooba/cpython that referenced this pull request Feb 13, 2020

bpo-39184: Add audit events to functions in fcntl, msvcrt, os, …

0b2cb7f

…`resource`, `shutil`, `signal`, `syslog` (pythonGH-18407)

bedevere-bot removed the needs backport to 3.8 label Feb 13, 2020

zooba added a commit that referenced this pull request Feb 13, 2020

bpo-39184: Add audit events to functions in fcntl, msvcrt, os, …

a00b5be

…`resource`, `shutil`, `signal`, `syslog` (GH-18407) Co-authored-by: Saiyang Gou <[email protected]>

gousaiyang mannequin mentioned this pull request Apr 10, 2022

Many command execution functions are not raising auditing events #83365

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-39184: Add audit events to functions in `fcntl`, `msvcrt`, `os`, `resource`, `shutil`, `signal`, `syslog` #18407

bpo-39184: Add audit events to functions in `fcntl`, `msvcrt`, `os`, `resource`, `shutil`, `signal`, `syslog` #18407

gousaiyang commented Feb 7, 2020 •

edited by bedevere-bot

Loading

codecov bot commented Feb 8, 2020 •

edited

Loading

zooba left a comment

zooba Feb 12, 2020

gousaiyang Feb 13, 2020

zooba Feb 12, 2020

zooba Feb 12, 2020

gousaiyang Feb 13, 2020

zooba Feb 13, 2020

bedevere-bot commented Feb 12, 2020

gousaiyang commented Feb 13, 2020

bedevere-bot commented Feb 13, 2020

miss-islington commented Feb 13, 2020

miss-islington commented Feb 13, 2020

bedevere-bot commented Feb 13, 2020

bpo-39184: Add audit events to functions in fcntl, msvcrt, os, resource, shutil, signal, syslog #18407

bpo-39184: Add audit events to functions in fcntl, msvcrt, os, resource, shutil, signal, syslog #18407

Conversation

gousaiyang commented Feb 7, 2020 • edited by bedevere-bot Loading

codecov bot commented Feb 8, 2020 • edited Loading

Codecov Report

zooba left a comment

Choose a reason for hiding this comment

zooba Feb 12, 2020

Choose a reason for hiding this comment

gousaiyang Feb 13, 2020

Choose a reason for hiding this comment

zooba Feb 12, 2020

Choose a reason for hiding this comment

zooba Feb 12, 2020

Choose a reason for hiding this comment

gousaiyang Feb 13, 2020

Choose a reason for hiding this comment

zooba Feb 13, 2020

Choose a reason for hiding this comment

bedevere-bot commented Feb 12, 2020

gousaiyang commented Feb 13, 2020

bedevere-bot commented Feb 13, 2020

miss-islington commented Feb 13, 2020

miss-islington commented Feb 13, 2020

bedevere-bot commented Feb 13, 2020

bpo-39184: Add audit events to functions in `fcntl`, `msvcrt`, `os`, `resource`, `shutil`, `signal`, `syslog` #18407

bpo-39184: Add audit events to functions in `fcntl`, `msvcrt`, `os`, `resource`, `shutil`, `signal`, `syslog` #18407

gousaiyang commented Feb 7, 2020 •

edited by bedevere-bot

Loading

codecov bot commented Feb 8, 2020 •

edited

Loading