Fix CopyStat TOCTOU in brotli CLI (#1461)#1463
Open
parasol-aser wants to merge 1 commit intogoogle:masterfrom
Open
Fix CopyStat TOCTOU in brotli CLI (#1461)#1463parasol-aser wants to merge 1 commit intogoogle:masterfrom
parasol-aser wants to merge 1 commit intogoogle:masterfrom
Conversation
CloseFiles() previously called fclose(context->fout) before invoking CopyStat() on the output pathname. CopyStat() then used path-based chmod() and chown(), which gave an attacker with write access to the output directory a window to replace the path with a symlink between the close and the metadata syscalls, redirecting the chmod/chown to an arbitrary target. Close the race by doing the metadata copy while the output file is still open and by switching to fd-based syscalls on fileno(fout): - CopyStat() now takes FILE* fout and calls fchmod()/fchown() on the underlying fd. - CopyTimeStat() uses futimens(fd, ...) in the HAVE_UTIMENSAT branch; the legacy utime() fallback is preserved for platforms without it. - CloseFiles() invokes CopyStat() before fclose() and skips it when current_output_path is NULL (stdout), matching the issue's guidance. - Windows no-op shims updated from chmod/chown to fchmod/fchown. Adds deterministic regression coverage under tests/regression/t01/: an LD_PRELOAD fclose() swap reproduces the pre-fix behavior, and a negative-assertion wrapper confirms the attack no longer succeeds after the patch. Additional scripts cover mode/timestamp propagation, the -n (no-copy-stat) flag, stdin input, stdout output, mode mask, and roundtrip correctness. Refs google#1461
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #1461.
CloseFiles()inc/tools/brotli.cpreviously calledfclose(context->fout)before invokingCopyStat()on the output pathname.CopyStat()then used path-basedchmod()andchown(), which gave an attacker with write access to the output directory a window between the close and the metadata syscalls to swap the output pathname to a symlink and redirect the metadata writes to an arbitrary target.Close the race by doing the metadata copy while the output file is still open, and by switching to fd-based syscalls on
fileno(fout):CopyStat()now takesFILE* foutand callsfchmod()/fchown()on the underlying fd.CopyTimeStat()usesfutimens(fd, ...)in theHAVE_UTIMENSATbranch; the legacyutime()fallback is preserved for platforms without it.CloseFiles()invokesCopyStat()beforefclose(), and skips it whencurrent_output_path == NULL(stdout), matching the guidance in the issue.chmod/chowntofchmod/fchownto keep the file compiling on_WIN32.fflush(fout)is added before the timestamp copy so stdio-buffered output does not clobber the atime/mtime the helper just applied duringfclose()'s write-back.The old
/* TOCTOU violation ... */comment is deleted — the violation is gone.No CMake / Bazel / build-system changes are required; the fix uses only POSIX symbols already available in the existing guarded blocks, and stays within the C89 compatibility constraint from
CONTRIBUTING.md.Test plan
Regression assets are checked in under
tests/regression/t01/.tests/regression/t01/repro_copystat_swap.sh— deterministic attack reproducer using anLD_PRELOADhook onfclose()that swaps the just-closed output path to a symlink. Against pre-fixbrotli, the script exits 0 and printsT-01 OK: target mode flipped to 644 via post-close CopyStat path. Against the patched binary, the attack no longer lands.tests/regression/t01/test_copystat_swap_fixed.sh/test_copystat_swap_fixed_strict.sh— negative-assertion wrappers over the reproducer. They pass only when the swap fails to flip the target's mode.tests/regression/t01/test_copystat_swap_with_no_copy_stat.sh— confirms-n/--no-copy-statstill disables the metadata copy entirely.tests/regression/t01/test_copystat_positive.sh,test_copystat_various_modes.sh,test_copystat_mode_mask.sh— positive regressions: with no attacker, the output file's mode is copied from the input (including across a range of modes and theS_IRWXU|S_IRWXG|S_IRWXOmask).tests/regression/t01/test_copystat_timestamp.sh— confirms atime/mtime are propagated viafutimenson the still-open fd.tests/regression/t01/test_copystat_stdout_skip.sh— exercises thecurrent_output_path == NULLbranch (echo ... | brotli -c | brotli -dc); no metadata call is attempted on stdout.tests/regression/t01/test_copystat_stdin_input.sh— confirms stdin input (no input path to stat from) is handled cleanly.tests/regression/t01/test_copystat_roundtrip.sh— plain compress/decompress roundtrip still produces byte-identical output, so the reorder inCloseFiles()has not regressed the happy path.tests/regression/t01/run_all.sh— runs all of the above and summarizes pass / fail / infra-skip counts.build-asan/); no sanitizer reports.To reproduce locally:
cc -fPIC -shared -o tests/regression/t01/libfclose_swap.so \ tests/regression/t01/fclose_swap.c -ldl tests/regression/t01/run_all.sh \ build-asan/brotli \ tests/regression/t01/libfclose_swap.soThe
.soandplain.bin.brartifacts are intentionally not committed (covered by the existing*.so/*.brignore rules) — they are built on demand fromfclose_swap.candplain.bin.The companion path-TOCTOU on
OpenOutputFile()is tracked separately in #1462 and will land in its own PR, to keep the-fbehavior change out of this review.