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

gh-122133: Authenticate socket connection for socket.socketpair() fallback #122134

Merged
merged 6 commits into from
Jul 29, 2024

Conversation

sethmlarson
Copy link
Contributor

@sethmlarson sethmlarson commented Jul 22, 2024

Socket connection for the socket.socketpair() fallback isn't authenticated. This changes this connection to authenticate the connection is the same expected process.

@njsmith
Copy link
Contributor

njsmith commented Jul 23, 2024

alternative approach: check sock1.getpeername() == sock2.getsockname() and vice-versa?

Lib/socket.py Outdated Show resolved Hide resolved
Lib/socket.py Outdated Show resolved Hide resolved
Lib/socket.py Outdated Show resolved Hide resolved
Lib/socket.py Outdated Show resolved Hide resolved
Lib/test/test_socket.py Outdated Show resolved Hide resolved
Lib/socket.py Outdated Show resolved Hide resolved
@sethmlarson
Copy link
Contributor Author

@njsmith I went ahead with your approach, it seems sound to me by checking the port. See: e08f3fb

@sethmlarson sethmlarson requested a review from picnixz July 23, 2024 21:05
Lib/socket.py Outdated Show resolved Hide resolved
Lib/socket.py Outdated Show resolved Hide resolved
Lib/test/test_socket.py Outdated Show resolved Hide resolved
Lib/test/test_socket.py Show resolved Hide resolved
Lib/test/test_socket.py Outdated Show resolved Hide resolved
Lib/test/test_socket.py Show resolved Hide resolved
@sethmlarson sethmlarson requested a review from picnixz July 25, 2024 15:18
Copy link
Contributor

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Except for the last call_socketpair refactorization, I personally don't have anything else to say on that PR so I'll leave it to experts now :)

Lib/test/test_socket.py Outdated Show resolved Hide resolved
@gpshead gpshead added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 25, 2024
@bedevere-bot
Copy link

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

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 25, 2024
@sethmlarson sethmlarson added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 25, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @sethmlarson for commit d1bc408 🤖

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 25, 2024
Copy link
Member

@gpshead gpshead left a comment

Choose a reason for hiding this comment

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

This appears to solve the primary issue of allowing unintended potentially malicious connection from another process. I don't think adding a looping retry for the connection is worthwhile - at least as part of this issue resolution - the main goal is preventing the undesirable connection which this now does.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Jul 29, 2024
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 29, 2024
…r()` fallback (pythonGH-122134)

* Authenticate socket connection for `socket.socketpair()` fallback when the platform does not have a native `socketpair` C API.  We authenticate in-process using `getsocketname` and `getpeername` (thanks to Nathaniel J Smith for that suggestion).

(cherry picked from commit 78df104)

Co-authored-by: Seth Michael Larson <[email protected]>
Co-authored-by: Gregory P. Smith <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Jul 29, 2024

GH-122426 is a backport of this pull request to the 3.11 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.11 only security fixes label Jul 29, 2024
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 29, 2024
…r()` fallback (pythonGH-122134)

* Authenticate socket connection for `socket.socketpair()` fallback when the platform does not have a native `socketpair` C API.  We authenticate in-process using `getsocketname` and `getpeername` (thanks to Nathaniel J Smith for that suggestion).

(cherry picked from commit 78df104)

Co-authored-by: Seth Michael Larson <[email protected]>
Co-authored-by: Gregory P. Smith <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Jul 29, 2024

GH-122427 is a backport of this pull request to the 3.10 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.10 only security fixes label Jul 29, 2024
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 29, 2024
…r()` fallback (pythonGH-122134)

* Authenticate socket connection for `socket.socketpair()` fallback when the platform does not have a native `socketpair` C API.  We authenticate in-process using `getsocketname` and `getpeername` (thanks to Nathaniel J Smith for that suggestion).

(cherry picked from commit 78df104)

Co-authored-by: Seth Michael Larson <[email protected]>
Co-authored-by: Gregory P. Smith <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Jul 29, 2024

GH-122428 is a backport of this pull request to the 3.9 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.9 only security fixes label Jul 29, 2024
@bedevere-app
Copy link

bedevere-app bot commented Jul 29, 2024

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

@sethmlarson sethmlarson deleted the socketpair-auth branch July 29, 2024 21:45
gpshead added a commit that referenced this pull request Jul 29, 2024
…ir()` fallback (GH-122134) (GH-122425)

Authenticate socket connection for `socket.socketpair()` fallback when the platform does not have a native `socketpair` C API.  We authenticate in-process using `getsocketname` and `getpeername` (thanks to Nathaniel J Smith for that suggestion).

(cherry picked from commit 78df104)

Co-authored-by: Seth Michael Larson <[email protected]>
Co-authored-by: Gregory P. Smith <[email protected]>
gpshead added a commit that referenced this pull request Jul 30, 2024
…ir()` fallback (GH-122134) (GH-122424)

Authenticate socket connection for `socket.socketpair()` fallback when the platform does not have a native `socketpair` C API.  We authenticate in-process using `getsocketname` and `getpeername` (thanks to Nathaniel J Smith for that suggestion).

(cherry picked from commit 78df104)

Co-authored-by: Seth Michael Larson <[email protected]>
Co-authored-by: Gregory P. Smith <[email protected]>
@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot iOS ARM64 Simulator 3.13 has failed when building commit b252317.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#builders/1386/builds/444) and take a look at the build logs.
  4. Check if the failure is related to this commit (b252317) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#builders/1386/builds/444

Summary of the results of the build (if available):

Click to see traceback logs
remote: Enumerating objects: 11, done.        
remote: Counting objects:  10% (1/10)        
remote: Counting objects:  20% (2/10)        
remote: Counting objects:  30% (3/10)        
remote: Counting objects:  40% (4/10)        
remote: Counting objects:  50% (5/10)        
remote: Counting objects:  60% (6/10)        
remote: Counting objects:  70% (7/10)        
remote: Counting objects:  80% (8/10)        
remote: Counting objects:  90% (9/10)        
remote: Counting objects: 100% (10/10)        
remote: Counting objects: 100% (10/10), done.        
remote: Compressing objects:  12% (1/8)        
remote: Compressing objects:  25% (2/8)        
remote: Compressing objects:  37% (3/8)        
remote: Compressing objects:  50% (4/8)        
remote: Compressing objects:  62% (5/8)        
remote: Compressing objects:  75% (6/8)        
remote: Compressing objects:  87% (7/8)        
remote: Compressing objects: 100% (8/8)        
remote: Compressing objects: 100% (8/8), done.        
remote: Total 11 (delta 2), reused 5 (delta 2), pack-reused 1        
From https://github.com/python/cpython
 * branch                  3.13       -> FETCH_HEAD
Note: switching to 'b252317956b7fc035bb3774ef6a177e227f9fc54'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at b252317956 [3.13] gh-122133: Authenticate socket connection for `socket.socketpair()` fallback (GH-122134) (GH-122424)
Switched to and reset branch '3.13'

configure: WARNING: no system libmpdecimal found; falling back to bundled libmpdecimal (deprecated and scheduled for removal in Python 3.15)
configure: WARNING: pkg-config is missing. Some dependencies may not be detected correctly.

configure: WARNING: no system libmpdecimal found; falling back to bundled libmpdecimal (deprecated and scheduled for removal in Python 3.15)
configure: WARNING: pkg-config is missing. Some dependencies may not be detected correctly.

--- xcodebuild: WARNING: Using the first of multiple matching destinations:
{ platform:iOS Simulator, id:C6AEA11C-C34A-47B8-BD67-AF0403ECA353, OS:17.5, name:iPhone SE (3rd generation) }
{ platform:iOS Simulator, id:C6AEA11C-C34A-47B8-BD67-AF0403ECA353, OS:17.5, name:iPhone SE (3rd generation) }
make: *** [testios] Terminated: 15
** BUILD INTERRUPTED **

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Ubuntu NoGIL Refleaks 3.13 has failed when building commit b252317.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#builders/1431/builds/338) and take a look at the build logs.
  4. Check if the failure is related to this commit (b252317) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#builders/1431/builds/338

Failed tests:

  • test_free_threading

Summary of the results of the build (if available):

==

Click to see traceback logs
remote: Enumerating objects: 11, done.        
remote: Counting objects:  10% (1/10)        
remote: Counting objects:  20% (2/10)        
remote: Counting objects:  30% (3/10)        
remote: Counting objects:  40% (4/10)        
remote: Counting objects:  50% (5/10)        
remote: Counting objects:  60% (6/10)        
remote: Counting objects:  70% (7/10)        
remote: Counting objects:  80% (8/10)        
remote: Counting objects:  90% (9/10)        
remote: Counting objects: 100% (10/10)        
remote: Counting objects: 100% (10/10), done.        
remote: Compressing objects:  12% (1/8)        
remote: Compressing objects:  25% (2/8)        
remote: Compressing objects:  37% (3/8)        
remote: Compressing objects:  50% (4/8)        
remote: Compressing objects:  62% (5/8)        
remote: Compressing objects:  75% (6/8)        
remote: Compressing objects:  87% (7/8)        
remote: Compressing objects: 100% (8/8)        
remote: Compressing objects: 100% (8/8), done.        
remote: Total 11 (delta 2), reused 5 (delta 2), pack-reused 1        
From https://github.com/python/cpython
 * branch                  3.13       -> FETCH_HEAD
Note: switching to 'b252317956b7fc035bb3774ef6a177e227f9fc54'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at b252317956 [3.13] gh-122133: Authenticate socket connection for `socket.socketpair()` fallback (GH-122134) (GH-122424)
Switched to and reset branch '3.13'

configure: WARNING: no system libmpdecimal found; falling back to bundled libmpdecimal (deprecated and scheduled for removal in Python 3.15)

make: *** [Makefile:2264: buildbottest] Error 2

ambv pushed a commit that referenced this pull request Jul 30, 2024
…ir()` fallback (GH-122134) (#122426)

Authenticate socket connection for `socket.socketpair()` fallback when the platform does not have a native `socketpair` C API.  We authenticate in-process using `getsocketname` and `getpeername` (thanks to Nathaniel J Smith for that suggestion).

(cherry picked from commit 78df104)

Co-authored-by: Seth Michael Larson <[email protected]>
Co-authored-by: Gregory P. Smith <[email protected]>
ambv pushed a commit that referenced this pull request Jul 30, 2024
…ir()` fallback (GH-122134) (#122427)

Authenticate socket connection for `socket.socketpair()` fallback when the platform does not have a native `socketpair` C API.  We authenticate in-process using `getsocketname` and `getpeername` (thanks to Nathaniel J Smith for that suggestion).

(cherry picked from commit 78df104)

Co-authored-by: Seth Michael Larson <[email protected]>
Co-authored-by: Gregory P. Smith <[email protected]>
ambv pushed a commit that referenced this pull request Jul 30, 2024
…r()` fallback (GH-122134) (#122428)

Authenticate socket connection for `socket.socketpair()` fallback when the platform does not have a native `socketpair` C API.  We authenticate in-process using `getsocketname` and `getpeername` (thanks to Nathaniel J Smith for that suggestion).

(cherry picked from commit 78df104)

Co-authored-by: Seth Michael Larson <[email protected]>
Co-authored-by: Gregory P. Smith <[email protected]>
ambv added a commit that referenced this pull request Jul 30, 2024
…r()` fallback (GH-122134) (GH-122429)

Authenticate socket connection for `socket.socketpair()` fallback when the platform does not have a native `socketpair` C API.  We authenticate in-process using `getsocketname` and `getpeername` (thanks to Nathaniel J Smith for that suggestion).

(cherry picked from commit 78df104)

Co-authored-by: Seth Michael Larson <[email protected]>
Co-authored-by: Gregory P. Smith <[email protected]>
Co-authored-by: Łukasz Langa <[email protected]>
@ell1e
Copy link

ell1e commented Jul 30, 2024

My apologies for missing the review window of the fix, but it looks good to me 👍

@freakboy3742
Copy link
Contributor

@sethmlarson Flagging that this PR is causing a problem with the iOS buildbot (on both 3.x and 3.13).

It's causing a number of tests in test_ssl, test_urllib2_localnet, test_urllib2net and test_urllibnet to fail - but we never see these failures because both test_wsgiref and test_xmlrpc lock up and don't complete (unfortunately, with no stack trace - but it seems reasonable to assume that it might be related to a timeout and a server dying).

I'm still poking around to see if I can make sense of this; if you've got any ideas, don't be a stranger :-)

The stack trace in the failed tests is mostly consistent:

Traceback (most recent call last):
  File "/Users/buildbot/Library/Developer/XCTestDevices/A7FBDE4D-FE55-443F-A16A-ECE998C18D1D/data/Containers/Bundle/Application/CB9423FE-C897-46F8-BE6C-5A60DF83AB1A/iOSTestbed.app/python/lib/python3.14/test/test_urllibnet.py", line 180, in test_specified_path
    with self.urlretrieve(self.logo,
         ~~~~~~~~~~~~~~~~^^^^^^^^^^^
                          os_helper.TESTFN) as (file_location, info):
                          ^^^^^^^^^^^^^^^^^
  File "/Users/buildbot/Library/Developer/XCTestDevices/A7FBDE4D-FE55-443F-A16A-ECE998C18D1D/data/Containers/Bundle/Application/CB9423FE-C897-46F8-BE6C-5A60DF83AB1A/iOSTestbed.app/python/lib/python3.14/contextlib.py", line 141, in __enter__
    return next(self.gen)
  File "/Users/buildbot/Library/Developer/XCTestDevices/A7FBDE4D-FE55-443F-A16A-ECE998C18D1D/data/Containers/Bundle/Application/CB9423FE-C897-46F8-BE6C-5A60DF83AB1A/iOSTestbed.app/python/lib/python3.14/test/test_urllibnet.py", line 163, in urlretrieve
    file_location, info = urllib.request.urlretrieve(*args, **kwargs)
                          ~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^
  File "/Users/buildbot/Library/Developer/XCTestDevices/A7FBDE4D-FE55-443F-A16A-ECE998C18D1D/data/Containers/Bundle/Application/CB9423FE-C897-46F8-BE6C-5A60DF83AB1A/iOSTestbed.app/python/lib/python3.14/urllib/request.py", line 214, in urlretrieve
    with contextlib.closing(urlopen(url, data)) as fp:
                            ~~~~~~~^^^^^^^^^^^
  File "/Users/buildbot/Library/Developer/XCTestDevices/A7FBDE4D-FE55-443F-A16A-ECE998C18D1D/data/Containers/Bundle/Application/CB9423FE-C897-46F8-BE6C-5A60DF83AB1A/iOSTestbed.app/python/lib/python3.14/urllib/request.py", line 189, in urlopen
    return opener.open(url, data, timeout)
           ~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^
  File "/Users/buildbot/Library/Developer/XCTestDevices/A7FBDE4D-FE55-443F-A16A-ECE998C18D1D/data/Containers/Bundle/Application/CB9423FE-C897-46F8-BE6C-5A60DF83AB1A/iOSTestbed.app/python/lib/python3.14/urllib/request.py", line 489, in open
    response = self._open(req, data)
  File "/Users/buildbot/Library/Developer/XCTestDevices/A7FBDE4D-FE55-443F-A16A-ECE998C18D1D/data/Containers/Bundle/Application/CB9423FE-C897-46F8-BE6C-5A60DF83AB1A/iOSTestbed.app/python/lib/python3.14/urllib/request.py", line 506, in _open
    result = self._call_chain(self.handle_open, protocol, protocol +
                              '_open', req)
  File "/Users/buildbot/Library/Developer/XCTestDevices/A7FBDE4D-FE55-443F-A16A-ECE998C18D1D/data/Containers/Bundle/Application/CB9423FE-C897-46F8-BE6C-5A60DF83AB1A/iOSTestbed.app/python/lib/python3.14/urllib/request.py", line 466, in _call_chain
    result = func(*args)
  File "/Users/buildbot/Library/Developer/XCTestDevices/A7FBDE4D-FE55-443F-A16A-ECE998C18D1D/data/Containers/Bundle/Application/CB9423FE-C897-46F8-BE6C-5A60DF83AB1A/iOSTestbed.app/python/lib/python3.14/urllib/request.py", line 1348, in http_open
    return self.do_open(http.client.HTTPConnection, req)
           ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/buildbot/Library/Developer/XCTestDevices/A7FBDE4D-FE55-443F-A16A-ECE998C18D1D/data/Containers/Bundle/Application/CB9423FE-C897-46F8-BE6C-5A60DF83AB1A/iOSTestbed.app/python/lib/python3.14/urllib/request.py", line 1319, in do_open
    h.request(req.get_method(), req.selector, req.data, headers,
    ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
              encode_chunked=req.has_header('Transfer-encoding'))
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/buildbot/Library/Developer/XCTestDevices/A7FBDE4D-FE55-443F-A16A-ECE998C18D1D/data/Containers/Bundle/Application/CB9423FE-C897-46F8-BE6C-5A60DF83AB1A/iOSTestbed.app/python/lib/python3.14/http/client.py", line 1336, in request
    self._send_request(method, url, body, headers, encode_chunked)
    ~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/buildbot/Library/Developer/XCTestDevices/A7FBDE4D-FE55-443F-A16A-ECE998C18D1D/data/Containers/Bundle/Application/CB9423FE-C897-46F8-BE6C-5A60DF83AB1A/iOSTestbed.app/python/lib/python3.14/http/client.py", line 1382, in _send_request
    self.endheaders(body, encode_chunked=encode_chunked)
    ~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/buildbot/Library/Developer/XCTestDevices/A7FBDE4D-FE55-443F-A16A-ECE998C18D1D/data/Containers/Bundle/Application/CB9423FE-C897-46F8-BE6C-5A60DF83AB1A/iOSTestbed.app/python/lib/python3.14/http/client.py", line 1331, in endheaders
    self._send_output(message_body, encode_chunked=encode_chunked)
    ~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/buildbot/Library/Developer/XCTestDevices/A7FBDE4D-FE55-443F-A16A-ECE998C18D1D/data/Containers/Bundle/Application/CB9423FE-C897-46F8-BE6C-5A60DF83AB1A/iOSTestbed.app/python/lib/python3.14/http/client.py", line 1091, in _send_output
    self.send(msg)
    ~~~~~~~~~^^^^^
  File "/Users/buildbot/Library/Developer/XCTestDevices/A7FBDE4D-FE55-443F-A16A-ECE998C18D1D/data/Containers/Bundle/Application/CB9423FE-C897-46F8-BE6C-5A60DF83AB1A/iOSTestbed.app/python/lib/python3.14/http/client.py", line 1035, in send
    self.connect()
    ~~~~~~~~~~~~^^
  File "/Users/buildbot/Library/Developer/XCTestDevices/A7FBDE4D-FE55-443F-A16A-ECE998C18D1D/data/Containers/Bundle/Application/CB9423FE-C897-46F8-BE6C-5A60DF83AB1A/iOSTestbed.app/python/lib/python3.14/http/client.py", line 1001, in connect
    self.sock = self._create_connection(
                ~~~~~~~~~~~~~~~~~~~~~~~^
        (self.host,self.port), self.timeout, self.source_address)
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/buildbot/Library/Developer/XCTestDevices/A7FBDE4D-FE55-443F-A16A-ECE998C18D1D/data/Containers/Bundle/Application/CB9423FE-C897-46F8-BE6C-5A60DF83AB1A/iOSTestbed.app/python/lib/python3.14/socket.py", line 851, in create_connection
    sock.settimeout(timeout)
    ~~~~~~~~~~~~~~~^^^^^^^^^
TypeError: 'object' object cannot be interpreted as an integer

@gpshead
Copy link
Member

gpshead commented Jul 31, 2024

Some code smell: socket._GLOBAL_DEFAULT_TIMEOUT = object() in Lib/socket.py and Lib/urllib/request.py's timeout=socket._GLOBAL_DEFAULT_TIMEOUT and the test this PR adds is doing importlib.reload(socket) so the socket module changes multiple times... all of these tests must be being run in the same process.

That default argument assignement to timeout=socket._GLOBAL_DEFAULT_TIMEOUT is referring to the singleton from the original socket module and our use of .reload(socket) has caused that singleton to be replaced multiple time so it fails any later identity checks in that scenario.

patching the test to avoid reinitializing that would be a gross hack. we should do better. must we really use reload() at all in our new tests? that looks like the underlying cause. modules can't be presumed to be reload-safe.

@gpshead
Copy link
Member

gpshead commented Jul 31, 2024

Not having Lib/socket.py use a conditional definition of two different versions of the function but instead always defining the pure python socketpair, just as a socket._fallback_socketpair so that we could just monkeypatch over socket.socketpair for the test instead of using reload after removing it from _socket would be preferred.

minor downside: a little more memory use for the new _fallback_socketpair on platforms that otherwise never use it, and execution time to materialize that def upon the first import socket.

@freakboy3742
Copy link
Contributor

Confirming that commenting out the new test that does the importlib.reload() fixes the problem. I'll work up a fix following @gpshead's suggestion.

@mhsmith
Copy link
Member

mhsmith commented Jul 31, 2024

It's causing a number of tests in test_ssl, test_urllib2_localnet, test_urllib2net and test_urllibnet to fail

FYI: I was seeing the same failures with the same stack trace on Android, which also runs all tests in a single process. This was also fixed by #122493.

@gpshead
Copy link
Member

gpshead commented Jul 31, 2024

yep, see the issue. We reworked the test to not use importlib.reload(socket) in follow up PR #122493 and are backporting that as well.

@cfi-gb
Copy link

cfi-gb commented Aug 12, 2024

Only for (easier) tracking purposes (because searching for this CVE doesn't yield any results in this repo), it seems CVE-2024-3219 is now linked to this PR:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants