Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-97514: Authenticate the forkserver control socket.#99309

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@gpshead
Copy link
Member

@gpsheadgpshead commentedNov 10, 2022•
edited
Loading

This adds authentication. In the past only filesystem permissions protected this socket from code injection into the forkserver process by limiting access to the same UID, which didn't exist when Linux abstract namespace sockets were used (see issue) meaning that any process in the same system network namespace could inject code. We've since stopped using abstract namespace sockets by default, but protecting our control sockets regardless of type seems desirable.

This reuses the HMAC based shared key auth already used bymultiprocessing.connection sockets for other purposes.

Doing this is useful so that filesystem permissions are not relied upon and trust isn't implied by default between all processes running as the same UID with access to the unix socket.

Tasks remaining

  • clean up the file descriptor leak from the new tests.
  • Microbenchmarking
  • Q: Decide if this needs an off switch. A: Nope. If we find reason to during betas before 3.14 we can add it. Rationale: This change is in the noise for most uses,multiprocessing.Pool worker processes are long lived by default so spawning new ones via the start method is infrequent compared to the number of tasks they are given. Only applications using maxtasksperchild= with a very low value might be able to notice, but even then the amount of work done in a worker process should far exceed any additional overhead this security measure adds to requesting forkserver to spawn new processes.

pyperformance benchmarks

No significant changes. Includingconcurrent_imap which exercisesmultiprocessing.Pool.imap in that suite.

Microbenchmarks

This doesslightly slow down forkserver use. How much so appears to depend on the platform. Modern platforms and simple platforms are less impacted. This PR adds additional IPC round trips to the control socket to tell forkserver to spawn a new process. Systems with potentially high latency IPC are naturally impacted more.

Using mymultiprocessingprocess-creation-benchmark.py:

I switched between this PR branch andmain via a simple git checkout after my build as the changes are pure Python so no rebuild is needed.

On an AMD zen4 system:

889 Procs/sec dropped to 874. 1.5% slower. Insignificant.

AMD 7800X3D single-CCD 8 cores.

% ../b/python process-creation-benchmark.py 5 forkserverProcess Creation Microbenchmark (max 7 active processes) (5 iterations)multiprocessing start method: forkserversys.version='3.14.0a1+ (~main branch~, Nov 10 2024) [GCC 13.2.0]'--------------------------------------------------------------------------------Total    Procs/sec   Time (s)     StdDev--------------------------------------------------------------------------------32          666.77      0.049     108.39128         831.09      0.154      44.05384         887.16      0.433       9.271024        886.02      1.156       1.372048        888.99      2.304       2.76% ./b/python ~/Downloads/process-creation-benchmark.py 5 forkserverProcess Creation Microbenchmark (max 7 active processes) (5 iterations)multiprocessing start method: forkserversys.version='3.14.0a1+ (heads/security/multiprocessing-forkserver-authkey-dirty:07c01d459f8, Nov 10 2024) [GCC 13.2.0]'--------------------------------------------------------------------------------Total    Procs/sec   Time (s)     StdDev--------------------------------------------------------------------------------32          640.53      0.052     130.27128         809.62      0.158      38.79384         867.22      0.443       7.661024        873.75      1.172       2.762048        873.57      2.344       2.85
Expand for baseline fork (2659 Procs/sec) and spawn (268) measurements.```% ../b/python ~/Downloads/process-creation-benchmark.py 13 forkProcess Creation Microbenchmark (max 7 active processes) (13 iterations)multiprocessing start method: forksys.version='3.14.0a1+ (heads/security/multiprocessing-forkserver-authkey-dirty:07c01d459f8, Nov 10 2024) [GCC 13.2.0]'--------------------------------------------------------------------------------Total Procs/sec Time (s) StdDev--------------------------------------------------------------------------------32 2,300.78 0.014 78.91128 2,391.11 0.054 114.68384 2,650.31 0.145 13.231024 2,646.28 0.387 16.472048 2,641.08 0.775 13.655120 2,659.42 1.925 11.82% ../b/python ~/Downloads/process-creation-benchmark.py 13 spawnProcess Creation Microbenchmark (max 7 active processes) (13 iterations)multiprocessing start method: spawnsys.version='3.14.0a1+ (heads/security/multiprocessing-forkserver-authkey-dirty:07c01d459f8, Nov 10 2024) [GCC 13.2.0]'--------------------------------------------------------------------------------Total Procs/sec Time (s) StdDev--------------------------------------------------------------------------------32 235.96 0.136 13.91128 259.53 0.493 0.79384 267.62 1.435 1.001024 267.89 3.822 0.35```

On an Intel Broadwell Xeon E5-2698 v4 system:

828 Procs/sec dropped to 717. ~15% slower. Significant. BUT... if I drop the active processes from 19 to 9. The difference was far less. 414 dropped to 398 for a ~4% slower. Moderate.

20 cores, 2 ring busses, 4 memory controllers, single socket. A large die Broadwell Xeon is complicated. At high parallelism counts, interprocess communication latencies add up. I predict similar results from multi-core-complex-die zen/epycs and multi socket systems, probably also on big.little mixed power/perf core arrangements.

% ../b/python ~/process-creation-benchmark.py 13 forkserverProcess Creation Microbenchmark (max 19 active processes) (13 iterations)multiprocessing start method: forkserversys.version='3.14.0a1+ (~main branch~, Nov 10 2024) [GCC 13.2.0]'--------------------------------------------------------------------------------Total    Procs/sec   Time (s)     StdDev--------------------------------------------------------------------------------32          535.14      0.062      77.23128         735.49      0.174       6.53384         798.69      0.481       4.431024        820.84      1.248       1.902048        827.63      2.475       4.31% ../b/python ~/process-creation-benchmark.py 13 forkserverProcess Creation Microbenchmark (max 19 active processes) (13 iterations)multiprocessing start method: forkserversys.version='3.14.0a1+ (heads/security/multiprocessing-forkserver-authkey-dirty:07c01d459f8, Nov 10 2024) [GCC 13.2.0]'--------------------------------------------------------------------------------Total    Procs/sec   Time (s)     StdDev--------------------------------------------------------------------------------32          449.24      0.073      63.19128         614.39      0.208      16.66384         668.49      0.575      11.361024        716.77      1.430      18.102048        716.73      2.858      13.12
Expand for baseline fork (1265 Procs/sec) and spawn (233) measurements.
% ../b/python ~/process-creation-benchmark.py 13 forkProcess Creation Microbenchmark (max 19 active processes) (13 iterations)multiprocessing start method: forksys.version='3.14.0a1+ (heads/security/multiprocessing-forkserver-authkey-dirty:07c01d459f8, Nov 10 2024) [GCC 13.2.0]'--------------------------------------------------------------------------------Total    Procs/sec   Time (s)     StdDev--------------------------------------------------------------------------------32        1,241.39      0.026      51.43128       1,259.44      0.102       5.01384       1,254.59      0.306       3.861024      1,258.45      0.814       6.772048      1,265.48      1.618       8.34% ./b/python ~/process-creation-benchmark.py 13 spawnProcess Creation Microbenchmark (max 19 active processes) (13 iterations)multiprocessing start method: spawnsys.version='3.14.0a1+ (heads/security/multiprocessing-forkserver-authkey-dirty:07c01d459f8, Nov 10 2024) [GCC 13.2.0]'--------------------------------------------------------------------------------Total    Procs/sec   Time (s)     StdDev--------------------------------------------------------------------------------32          188.08      0.170       2.58128         221.20      0.579       0.75384         227.56      1.687       0.851024        233.34      4.388       0.54

On an Raspberry Pi 5

126 Proc/sec dropped to 121. A ~4% slowdown. Moderate.

Raspberry Pi 5 running 32-bit raspbian.

% ./python ../process-creation-benchmark.py Process Creation Microbenchmark (max 3 active processes) (5 iterations)multiprocessing start method: forkserversys.version='3.14.0a1+ (~main branch~, Nov 10 2024, 19:06:56) [GCC 12.2.0]'--------------------------------------------------------------------------------Total    Procs/sec   Time (s)     StdDev--------------------------------------------------------------------------------32          121.23      0.266       9.82128         125.45      1.020       0.84384         125.71      3.055       0.27% ./python ../process-creation-benchmark.py Process Creation Microbenchmark (max 3 active processes) (5 iterations)multiprocessing start method: forkserversys.version='3.14.0a1+ (heads/security/multiprocessing-forkserver-authkey:07c01d4, Nov 10 2024, 19:06:56) [GCC 12.2.0]'--------------------------------------------------------------------------------Total    Procs/sec   Time (s)     StdDev--------------------------------------------------------------------------------32          114.57      0.281      10.29128         119.70      1.069       0.28384         120.84      3.178       0.41
Expand for baseline fork (973 Procs/sec) and spawn (32) measurements.
% /python ../process-creation-benchmark.py 5 forkProcess Creation Microbenchmark (max 3 active processes) (5 iterations)multiprocessing start method: forksys.version='3.14.0a1+ (~main branch~, Nov 10 2024, 19:06:56) [GCC 12.2.0]'--------------------------------------------------------------------------------Total    Procs/sec   Time (s)     StdDev--------------------------------------------------------------------------------32          933.01      0.034      44.03128         973.00      0.132       1.33384         968.48      0.396       1.551024        972.78      1.053       0.77% ./python ../process-creation-benchmark.py 5 spawnProcess Creation Microbenchmark (max 3 active processes) (5 iterations)multiprocessing start method: spawnsys.version='3.14.0a1+ (~main branch~, Nov 10 2024, 19:06:56) [GCC 12.2.0]'--------------------------------------------------------------------------------Total    Procs/sec   Time (s)     StdDev--------------------------------------------------------------------------------32           31.97      1.001       0.12128          32.46      3.943       0.02

This adds authentication. In the past only filesystem permissionsprotected this socket from code injection into the forkserver process bylimiting access to the same UID, which didn't exist when Linux abstractnamespace sockets were used (see issue) meaning that any process in thesame system network namespace could inject code.This reuses the hmac based shared key auth already used onmultiprocessing sockets used for other purposes.Doing this is useful so that filesystem permissions are not relied uponand trust isn't implied by default between all processes running as thesame UID.
@gpsheadgpshead added type-featureA feature request or enhancement 3.12only security fixes topic-multiprocessing labelsNov 10, 2022
@gpsheadgpshead self-assigned thisNov 10, 2022
@gpsheadgpshead changed the titlegh-97514: Authenticate the forkserver control socket.gh-97514: [3.12+] Authenticate the forkserver control socket.Nov 10, 2022
@gpsheadgpshead added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelNov 11, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@gpshead for commitc83193d 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelNov 11, 2022
@gpshead
Copy link
MemberAuthor

from the buildbots... tests leak some file descriptors. not too surprising given the bit of code the test pokes into, i'll see what can be done to manage those.

I can't add new testcases to test_multiprocessing_forkserver itself, ihad to put them within an existing _test_multiprocessing test class.  Idon't know why, but refleaks are fragile and that test suite is...rediculiously complicated with all that it does.
I'm not sure _why_ the hang happened, the forkserver process wasn't exiting whenthe alive_w fd was closed in the parent during tearDownModule(), instead it remainedin its selector() loop.  regardless the part of the test this removes fixes it andit only happened on macOS.
@gpsheadgpshead added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelNov 13, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@gpshead for commitca47b6f 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelNov 13, 2022
@gpsheadgpshead requested a review fromambvDecember 11, 2022 00:09
@gpsheadgpshead added 3.13bugs and security fixes and removed 3.12only security fixes labelsJun 21, 2023
@gpsheadgpshead changed the titlegh-97514: [3.12+] Authenticate the forkserver control socket.gh-97514: Authenticate the forkserver control socket.Jun 21, 2023
@gpsheadgpshead removed the 3.13bugs and security fixes labelSep 24, 2024
Copy link
Member

@ericsnowcurrentlyericsnowcurrently left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This mostly looks good. The tests helped me understand a bit better. Sorry if some of my comments demonstrate ignorance about the multiprocessing implementation. Thanks for working on this.

gpshead reacted with heart emoji
@gpsheadgpshead added the 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelNov 10, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@gpshead for commit6bb9db4 🤖

If you want to schedule another build, you need to add the🔨 test-with-refleak-buildbots label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelNov 10, 2024
@gpsheadgpshead added 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelsNov 10, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@gpshead for commit07c01d4 🤖

If you want to schedule another build, you need to add the🔨 test-with-refleak-buildbots label again.

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@gpshead for commit07c01d4 🤖

If you want to schedule another build, you need to add the🔨 test-with-buildbots label again.

@bedevere-botbedevere-bot removed 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelsNov 10, 2024
@gpsheadgpshead added the type-securityA security issue labelNov 10, 2024
Copy link
Member

@picnixzpicnixz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Looks good! thanks for addressing my comments.

gpshead reacted with heart emoji
Copy link
Member

@ericsnowcurrentlyericsnowcurrently left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM

gpshead reacted with hooray emoji
@gpsheadgpshead merged commit7191b76 intopython:mainNov 20, 2024
39 of 40 checks passed
@gpsheadgpshead deleted the security/multiprocessing-forkserver-authkey branchNovember 20, 2024 16:19
ebonnal pushed a commit to ebonnal/cpython that referenced this pull requestJan 12, 2025
…-99309)This adds authentication to the forkserver control socket. In the past only filesystem permissions protected this socket from code injection into the forkserver process by limiting access to the same UID, which didn't exist when Linux abstract namespace sockets were used (see issue) meaning that any process in the same system network namespace could inject code. We've since stopped using abstract namespace sockets by default, but protecting our control sockets regardless of type is a good idea.This reuses the HMAC based shared key auth already used by `multiprocessing.connection` sockets for other purposes.Doing this is useful so that filesystem permissions are not relied upon and trust isn't implied by default between all processes running as the same UID with access to the unix socket.### pyperformance benchmarksNo significant changes. Including `concurrent_imap` which exercises `multiprocessing.Pool.imap` in that suite.### MicrobenchmarksThis does _slightly_ slow down forkserver use. How much so appears to depend on the platform. Modern platforms and simple platforms are less impacted. This PR adds additional IPC round trips to the control socket to tell forkserver to spawn a new process. Systems with potentially high latency IPC are naturally impacted more.Typically a 1-4% slowdown on a very targeted process creation microbenchmark, with a worst case overloaded system slowdown of 20%.  No evidence that these slowdowns appear in practical sense.  See the PR for details.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@ericsnowcurrentlyericsnowcurrentlyericsnowcurrently approved these changes

@picnixzpicnixzpicnixz approved these changes

@vstinnervstinnerAwaiting requested review from vstinner

@iritkatrieliritkatrielAwaiting requested review from iritkatriel

@ambvambvAwaiting requested review from ambv

Assignees

@gpsheadgpshead

Labels

3.14bugs and security fixestopic-multiprocessingtype-featureA feature request or enhancementtype-securityA security issue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@gpshead@bedevere-bot@ericsnowcurrently@picnixz
[8]ページ先頭
©2009-2025 Movatter.jp