Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion plugins/codex-security/.codex-plugin/plugin.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "codex-security",
"version": "0.1.60",
"version": "0.1.79",
"description": "Codex Security workflows for security scans, analysis, and investigation.",
"author": {
"name": "OpenAI"
Expand Down
100 changes: 93 additions & 7 deletions plugins/codex-security/scripts/finalize_scan_contract.py
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,7 @@ def _descriptor_relative_writes_available() -> bool:


def _windows_scan_local_files() -> Any:
"""Load the Win32 backend only on runtimes that need it."""
"""Load the Win32 backend and shared stream comparison lazily."""

global _WINDOWS_SCAN_LOCAL_FILES
if _WINDOWS_SCAN_LOCAL_FILES is None:
Expand All @@ -334,24 +334,56 @@ def _windows_scan_local_files() -> Any:
return _WINDOWS_SCAN_LOCAL_FILES


def _open_verified_scan_directory(scan_dir: Path) -> int:
def _open_verified_scan_directory(
scan_dir: Path, expected_root_identity: tuple[int, int] | None = None
) -> int:
scan_dir = scan_dir.absolute()
try:
expected = scan_dir.lstat()
observed_identity = (expected.st_dev, expected.st_ino)
if (
expected_root_identity is not None
and observed_identity != expected_root_identity
):
raise ContractError("scan directory: changed after artifact restoration setup")
canonical = _require_scan_directory(scan_dir)
descriptor = os.open(
canonical,
os.O_RDONLY | getattr(os, "O_DIRECTORY", 0) | getattr(os, "O_NOFOLLOW", 0),
)
except OSError as exc:
raise ContractError("scan directory: expected an existing non-symlink directory") from exc
raise ContractError(
"scan directory: expected an existing non-symlink directory"
) from exc
opened = os.fstat(descriptor)
if (opened.st_dev, opened.st_ino) != (expected.st_dev, expected.st_ino):
opened_identity = (opened.st_dev, opened.st_ino)
if opened_identity != observed_identity or (
expected_root_identity is not None
and opened_identity != expected_root_identity
):
os.close(descriptor)
raise ContractError("scan directory: changed while it was being opened")
return descriptor


def scan_root_identity(scan_dir: Path) -> tuple[Path, tuple[int, int]]:
"""Return a canonical scan root and its identity from a held handle."""

scan_dir = _require_scan_directory(scan_dir)
if not _descriptor_relative_writes_available():
if not _is_windows():
raise ContractError(
"scan-local output requires descriptor-relative file operations"
)
return _windows_scan_local_files().scan_root_identity(scan_dir)
descriptor = _open_verified_scan_directory(scan_dir)
try:
metadata = os.fstat(descriptor)
return scan_dir, (metadata.st_dev, metadata.st_ino)
finally:
os.close(descriptor)


def _open_scan_local_directory(root_fd: int, parts: tuple[str, ...], *, create: bool) -> int:
descriptor = os.dup(root_fd)
try:
Expand Down Expand Up @@ -500,7 +532,12 @@ def _sha256_scan_local_file(scan_dir: Path, relative_path: str, context: str) ->


def write_scan_local_bytes(
scan_dir: Path, relative_path: str, payload: bytes, *, external_name: bool = False
scan_dir: Path,
relative_path: str,
payload: bytes,
*,
external_name: bool = False,
expected_root_identity: tuple[int, int] | None = None,
) -> None:
scan_dir = _require_scan_directory(scan_dir)
if external_name:
Expand All @@ -513,29 +550,78 @@ def write_scan_local_bytes(
if not _is_windows():
raise ContractError("scan-local output requires descriptor-relative file operations")
try:
_windows_scan_local_files().atomic_write(scan_dir, relative_path, payload)
_windows_scan_local_files().atomic_write(
scan_dir,
relative_path,
payload,
expected_root_identity=expected_root_identity,
)
except OSError as exc:
raise ContractError(f"{relative_path}: {exc}") from exc
return
root_fd: int | None = None
parent_fd: int | None = None
temp_name: str | None = None
try:
root_fd = _open_verified_scan_directory(scan_dir)
root_fd = _open_verified_scan_directory(scan_dir, expected_root_identity)
parts = PurePosixPath(relative_path).parts
try:
parent_fd = _open_scan_local_directory(root_fd, parts[:-1], create=True)
except OSError as exc:
raise ContractError(
f"{relative_path}: expected a path inside the scan directory"
) from exc
# The held descriptor is the authority for the validated parent. A
# concurrent rename cannot redirect later operations through a
# replacement path or link.
try:
metadata = os.stat(parts[-1], dir_fd=parent_fd, follow_symlinks=False)
except FileNotFoundError:
pass
else:
if not stat.S_ISREG(metadata.st_mode):
raise ContractError(f"{relative_path}: expected a regular non-symlink file")
try:
existing_fd = os.open(
parts[-1],
os.O_RDONLY
| getattr(os, "O_NOFOLLOW", 0)
| getattr(os, "O_NONBLOCK", 0),
dir_fd=parent_fd,
)
except OSError as exc:
if exc.errno not in {errno.ENOENT, errno.EACCES, errno.EPERM}:
raise
else:
try:
opened = os.fstat(existing_fd)
if not stat.S_ISREG(opened.st_mode):
raise ContractError(
f"{relative_path}: expected a regular non-symlink file"
)
if (opened.st_dev, opened.st_ino) != (
metadata.st_dev,
metadata.st_ino,
):
raise ContractError(
f"{relative_path}: changed while it was being opened"
)
if (
expected_root_identity is not None
and opened.st_size == len(payload)
):
try:
with os.fdopen(existing_fd, "rb") as handle:
existing_fd = -1
if _windows_scan_local_files().stream_matches_payload(
handle, payload
):
return
except OSError:
pass
finally:
if existing_fd >= 0:
os.close(existing_fd)
temp_name = f".{path.name}.{secrets.token_hex(8)}.tmp"
temp_fd = os.open(temp_name, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600, dir_fd=parent_fd)
with os.fdopen(temp_fd, "wb") as handle:
Expand Down
108 changes: 101 additions & 7 deletions plugins/codex-security/scripts/windows_scan_local_files.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@
write, or delete. Writes rename the exact temporary-file handle into place so
an attacker cannot substitute another file at the temporary name.

Importing this module is safe on non-Windows hosts. Its public operations
raise ``WindowsScanLocalFileError`` when called anywhere other than Windows.
Importing this module and comparing streams work on every platform. Filesystem
operations raise ``WindowsScanLocalFileError`` outside Windows.
"""

from __future__ import annotations
Expand All @@ -28,6 +28,7 @@
from collections.abc import Iterator
from ctypes import wintypes
from pathlib import Path, PurePosixPath
from typing import BinaryIO

_msvcrt = importlib.import_module("msvcrt") if os.name == "nt" else None

Expand Down Expand Up @@ -63,11 +64,15 @@ class WindowsScanLocalFileError(OSError):
_FILE_NAME_OPENED = 0x00000008
_ERROR_FILE_NOT_FOUND = 2
_ERROR_PATH_NOT_FOUND = 3
_ERROR_ACCESS_DENIED = 5
_ERROR_SHARING_VIOLATION = 32
_ERROR_LOCK_VIOLATION = 33
_ERROR_FILE_EXISTS = 80
_ERROR_ALREADY_EXISTS = 183
_MISSING_ERRORS = {_ERROR_FILE_NOT_FOUND, _ERROR_PATH_NOT_FOUND}
_COLLISION_ERRORS = {_ERROR_FILE_EXISTS, _ERROR_ALREADY_EXISTS}
_INVALID_HANDLE_VALUE = ctypes.c_void_p(-1).value
_MAX_COMPARE_CHUNK = 64 * 1024
_MAX_WRITE_CHUNK = 1024 * 1024

_INVALID_COMPONENT_CHARACTERS = frozenset('<>:"|?*')
Expand Down Expand Up @@ -398,12 +403,20 @@ def _locked_parent(
relative_path: str,
*,
create: bool,
expected_root_identity: tuple[int, int] | None = None,
) -> Iterator[tuple[Path, str]]:
"""Hold non-deletable handles for every directory in the absolute target path."""

_require_windows()
parts = _validated_parts(relative_path)
root_path, expected_root_identity = _canonical_scan_directory(scan_dir)
root_path, observed_root_identity = _canonical_scan_directory(scan_dir)
if (
expected_root_identity is not None
and observed_root_identity != expected_root_identity
):
raise _invalid_path(
scan_dir, "scan directory changed after artifact restoration setup"
)
handles: list[_OwnedHandle] = []
try:
# Absolute-path Win32 calls remain safe only while every ancestor is
Expand All @@ -414,8 +427,14 @@ def _locked_parent(
assert directory_handle is not None
handles.append(directory_handle)
current_root = root_path.lstat()
if (current_root.st_dev, current_root.st_ino) != expected_root_identity:
raise _invalid_path(scan_dir, "scan directory changed while it was being opened")
current_root_identity = (current_root.st_dev, current_root.st_ino)
if current_root_identity != observed_root_identity or (
expected_root_identity is not None
and current_root_identity != expected_root_identity
):
raise _invalid_path(
scan_dir, "scan directory changed while it was being opened"
)
current_path = root_path
for component in parts[:-1]:
current_path /= component
Expand All @@ -431,6 +450,14 @@ def _locked_parent(
handle.close()


def scan_root_identity(scan_dir: Path) -> tuple[Path, tuple[int, int]]:
"""Return a canonical scan root and its identity while holding it fixed."""

with _locked_parent(scan_dir, ".identity", create=False) as (root_path, _):
metadata = root_path.lstat()
return root_path, (metadata.st_dev, metadata.st_ino)


def open_read_fd(scan_dir: Path, relative_path: str, context: str) -> int:
"""Open a verified regular file and return an owned binary read descriptor."""

Expand Down Expand Up @@ -465,6 +492,16 @@ def open_read_fd(scan_dir: Path, relative_path: str, context: str) -> int:
) from exc


def stream_matches_payload(stream: BinaryIO, payload: bytes) -> bool:
offset = 0
while offset < len(payload):
chunk = stream.read(min(_MAX_COMPARE_CHUNK, len(payload) - offset))
if not chunk or chunk != payload[offset : offset + len(chunk)]:
return False
offset += len(chunk)
return stream.read(1) == b""


def _write_all(handle: int, payload: bytes) -> None:
view = memoryview(payload)
offset = 0
Expand Down Expand Up @@ -532,12 +569,69 @@ def _validate_existing_output(path: Path) -> None:
_verify_regular_file(handle.value, path)


def atomic_write(scan_dir: Path, relative_path: str, payload: bytes) -> None:
def _existing_output_matches(path: Path, payload: bytes) -> bool:
try:
handle = _create_file(
path,
access=_GENERIC_READ | _FILE_READ_ATTRIBUTES,
# Deny write and delete sharing while comparing the opened contents.
share=_FILE_SHARE_READ,
disposition=_OPEN_EXISTING,
flags=_FILE_FLAG_OPEN_REPARSE_POINT | _FILE_FLAG_BACKUP_SEMANTICS,
missing_ok=True,
)
except WindowsScanLocalFileError as exc:
if exc.errno in {
_ERROR_ACCESS_DENIED,
_ERROR_SHARING_VIOLATION,
_ERROR_LOCK_VIOLATION,
}:
return False
raise
if handle is None:
return False
with handle:
assert handle.value is not None
_verify_regular_file(handle.value, path)
raw_handle = handle.detach()
try:
assert _msvcrt is not None
descriptor = _msvcrt.open_osfhandle(
raw_handle, os.O_RDONLY | os.O_BINARY
)
except BaseException:
_close_handle(raw_handle)
raise
try:
with os.fdopen(descriptor, "rb") as stream:
if os.fstat(stream.fileno()).st_size != len(payload):
return False
return stream_matches_payload(stream, payload)
except OSError:
return False


def atomic_write(
scan_dir: Path,
relative_path: str,
payload: bytes,
*,
expected_root_identity: tuple[int, int] | None = None,
) -> None:
"""Atomically replace a scan-local regular file with ``payload``."""

with _locked_parent(scan_dir, relative_path, create=True) as (parent_path, leaf_name):
with _locked_parent(
scan_dir,
relative_path,
create=True,
expected_root_identity=expected_root_identity,
) as (parent_path, leaf_name):
destination_path = parent_path / leaf_name
_validate_existing_output(destination_path)
if expected_root_identity is not None and _existing_output_matches(
destination_path, payload
):
return

temp_handle: _OwnedHandle | None = None
temp_path: Path | None = None
Expand Down
8 changes: 7 additions & 1 deletion plugins/codex-security/tests/test_finalize_scan_contract.py
Original file line number Diff line number Diff line change
Expand Up @@ -1670,7 +1670,13 @@ def test_finalize_uses_windows_backend_without_dir_fd(self) -> None:
def open_read_fd(scan_dir: Path, relative_path: str, _context: str) -> int:
return os.open(scan_dir / relative_path, os.O_RDONLY)

def atomic_write(scan_dir: Path, relative_path: str, payload: bytes) -> None:
def atomic_write(
scan_dir: Path,
relative_path: str,
payload: bytes,
*,
expected_root_identity: tuple[int, int] | None = None,
) -> None:
path = scan_dir / relative_path
path.parent.mkdir(parents=True, exist_ok=True)
path.write_bytes(payload)
Expand Down
8 changes: 7 additions & 1 deletion plugins/codex-security/tests/test_windows_report_e2e.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,13 @@ def test_workbench_completion_and_exports_use_windows_file_backend(tmp_path: Pat
def open_read_fd(root: Path, relative_path: str, _context: str) -> int:
return os.open(root / relative_path, os.O_RDONLY)

def atomic_write(root: Path, relative_path: str, payload: bytes) -> None:
def atomic_write(
root: Path,
relative_path: str,
payload: bytes,
*,
expected_root_identity: tuple[int, int] | None = None,
) -> None:
path = root / relative_path
path.parent.mkdir(parents=True, exist_ok=True)
path.write_bytes(payload)
Expand Down
Loading
Loading