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

fix(job-attachments): remove dependency on pywin32 for submission code #250

Merged
merged 2 commits into from
Mar 26, 2024

Conversation

jericht
Copy link
Contributor

@jericht jericht commented Mar 26, 2024

What was the problem/requirement? (What/Why)

We recently added a symlink threat mitigation that used pywin32 for the Windows mitigation (on Unix we use O_NOFOLLOW in open). This caused an issue in our integrated submitters because of the way the submitter code is loaded into DCC Python environments (non-standard), which prevents pywin32 from being loaded up. Result is that users get an import error for the pywin32 modules when they try to submit with job attachments.

What was the solution? (How)

Remove the dependency on pywin32 for submission code, which is only the symlink threat mitigation for now. Instead, we use ctypes and call the Win32 APIs directly.

What is the impact of this change?

DCC submitters can submit jobs with job attachments again.

How was this change tested?

  • Ran unit/integ tests
  • E2E test on a Windows box. Verified that the symlink mitigation is still working as intended (see below)

Was this change documented?

No

Is this a breaking change?

No

E2E test

I crafted a test script that imported the updated code in this PR and called the function directly. It has two modes:

  1. Attack mode - tries to perform the symlink attack and verifies that the function rejects the file (returns False)
  2. Friendly mode - does not try to symlink attack and verifies that the function accepts regular files (returns True)

Test results

PS C:\Users\Administrator\Desktop\win_symlink> python .\win_symlink.py true
Creating file C:\Users\Administrator\AppData\Local\Temp\2\tmpx499ijpb\texture.png
ATTACK MODE
Renaming C:\Users\Administrator\AppData\Local\Temp\2\tmpx499ijpb\texture.png to C:\Users\Administrator\AppData\Local\Temp\2\tmpx499ijpb\original_texture.png
Creating secret file C:\Users\Administrator\AppData\Local\Temp\2\tmpx499ijpb\secrets.json
Symlinking file C:\Users\Administrator\AppData\Local\Temp\2\tmpx499ijpb\texture.png -> C:\Users\Administrator\AppData\Local\Temp\2\tmpx499ijpb\secrets.json

Does file descriptor point to original path (C:\Users\Administrator\AppData\Local\Temp\2\tmpx499ijpb\texture.png)? False

SUCCESS! Symlink threat mitigated

PS C:\Users\Administrator\Desktop\win_symlink> python .\win_symlink.py false
Creating file C:\Users\Administrator\AppData\Local\Temp\2\tmp7u630l2j\texture.png
Friendly mode :)

Does file descriptor point to original path (C:\Users\Administrator\AppData\Local\Temp\2\tmp7u630l2j\texture.png)? True

Function is accepting regular files

Test script

import logging
import os
import pathlib
import sys
import tempfile

import deadline.job_attachments.upload

def main():
    args = sys.argv[1:]
    attack = bool(args[0].lower() == "true")

    fd = None
    with tempfile.TemporaryDirectory() as tmpdir:
        try:
            tmpdir = pathlib.Path(tmpdir).resolve()
            tmpdir.mkdir(exist_ok=True)

            symlink_path = tmpdir / "texture.png"
            print(f"Creating file {symlink_path}")
            symlink_path.touch(exist_ok=True)

            if attack:
                print("ATTACK MODE")
                original_symlink_path = symlink_path.parent / "original_texture.png"
                print(f"Renaming {symlink_path} to {original_symlink_path}")
                os.rename(str(symlink_path), str(original_symlink_path))

                secret_path = tmpdir / "secrets.json"
                print(f"Creating secret file {secret_path}")
                secret_path.touch(exist_ok=True)

                print(f"Symlinking file {symlink_path} -> {secret_path}")
                symlink_path.symlink_to(secret_path)
            else:
                print("Friendly mode :)")

            fd = os.open(symlink_path, flags=os.O_RDONLY)
            result = deadline.job_attachments.upload.S3AssetUploader._is_path_win32_final_path_of_file_descriptor(None, str(symlink_path), fd)

            print()
            print(f"Does file descriptor point to original path ({symlink_path})? {result}")
            print()

            if attack and result:
                print("FAILED! Symlink threat mitigation did not work")
            elif attack and not result:
                print("SUCCESS! Symlink threat mitigated")
            elif not attack and result:
                print("Function is accepting regular files")
            elif not attack and not result:
                print("!!! ERROR !!! Function is not accepting regular files")

            return 0 if result else 1

        finally:
            if fd:
                os.close(fd)

if __name__ == "__main__":
    sys.exit(main())

Signed-off-by: Jericho Tolentino <68654047+jericht@users.noreply.github.com>
@jericht jericht force-pushed the jericht/ctypes branch 2 times, most recently from e79c747 to 6d999cb Compare March 26, 2024 22:45
@jericht jericht marked this pull request as ready for review March 26, 2024 22:46
@jericht jericht requested a review from a team as a code owner March 26, 2024 22:46
Signed-off-by: Jericho Tolentino <68654047+jericht@users.noreply.github.com>
@epmog epmog merged commit 30b44df into mainline Mar 26, 2024
18 checks passed
@epmog epmog deleted the jericht/ctypes branch March 26, 2024 22:59
@jusiskin jusiskin added bug Something isn't working security Pull requests that could impact security labels Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working security Pull requests that could impact security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants