From 954cfe770ae41ade4f1f828befd8bf59ffecf402 Mon Sep 17 00:00:00 2001 From: D Hoyt Date: Mon, 17 Aug 2026 12:14:05 -0400 Subject: [PATCH] fix: preserve embedded TIFF profiles on parse failure --- .../iccdev-tiffdump-output-hardening-tests.sh | 89 ++++++++-- .../scripts/iccdev-tool-coverage-baseline.sh | 4 +- IccProfLib/IccProfile.cpp | 16 +- Tools/CmdLine/IccTiffDump/Readme.md | 9 ++ Tools/CmdLine/IccTiffDump/iccTiffDump.cpp | 153 +++++++++++++++--- 5 files changed, 236 insertions(+), 35 deletions(-) diff --git a/.github/scripts/iccdev-tiffdump-output-hardening-tests.sh b/.github/scripts/iccdev-tiffdump-output-hardening-tests.sh index 5572da414..3da848010 100755 --- a/.github/scripts/iccdev-tiffdump-output-hardening-tests.sh +++ b/.github/scripts/iccdev-tiffdump-output-hardening-tests.sh @@ -8,6 +8,7 @@ TOOLS="${ICCDEV_TOOLS_DIR:-$REPO_ROOT/Build/Tools}" ICCDEV_TESTING="${ICCDEV_TESTING_DIR:-$REPO_ROOT/Testing}" OUTDIR="${ICCDEV_TEST_OUTDIR:-/tmp/iccdev-tiffdump-output-hardening}" TIFFDUMP="$TOOLS/IccTiffDump/iccTiffDump" +TOXML="$TOOLS/IccToXml/iccToXml" mkdir -p "$OUTDIR" @@ -75,7 +76,7 @@ newline.icc" "$TIFFDUMP" "$SAMPLE_TIFF" "$dst" > "$log" 2>&1 [ -s "$dst" ] || return 1 - grep -Fq 'Profile extracted to: '"$OUTDIR"'/export_with_\nnewline.icc' "$log" + grep -Fq 'Profile extracted byte-for-byte to: '"$OUTDIR"'/export_with_\nnewline.icc' "$log" } test_extra_arg_rejected() { @@ -106,8 +107,9 @@ test_missing_embedded_profile_export_rejected() { } # --------------------------------------------------------------------------- -# #1380: palette photometric must be reported (not silently "Min Is White"), and -# a parsed embedded ICC that fails conformance must be rejected, not rewritten. +# #1380: palette photometric must be reported (not silently "Min Is White"). +# Nonconformant embedded bytes must remain available as forensic artifacts even +# when parsing or validation returns a failure status. # --------------------------------------------------------------------------- PYTHON="$(command -v python3 || true)" @@ -197,6 +199,34 @@ path.write_bytes(d) PY } +generate_nested_icc() { + # Build an ICC.2 profile with $2 nested embeddedV5ProfileTag entries. Each + # level is structurally small, so depth rather than payload size drives the + # parser path under test. + "$PYTHON" - "$1" "$2" <<'PY' +import pathlib, struct, sys +path = pathlib.Path(sys.argv[1]); depth = int(sys.argv[2]) + +def header(size): + h = bytearray(128) + struct.pack_into(">I", h, 0, size) + h[8:12] = b"\x05\0\0\0" + h[12:16] = b"mntr" + h[16:20] = b"RGB " + h[20:24] = b"XYZ " + h[36:40] = b"acsp" + return h + +profile = header(132) + struct.pack(">I", 0) +for _ in range(depth): + tag = b"ICCp" + b"\0\0\0\0" + profile + size = 144 + len(tag) + profile = (header(size) + struct.pack(">I", 1) + b"ICC5" + + struct.pack(">II", 144, len(tag)) + tag) +path.write_bytes(profile) +PY +} + test_palette_photometric_report() { [ -n "$PYTHON" ] || { echo " [SKIP] python3 unavailable"; return 0; } local pal="$OUTDIR/palette.tif" @@ -216,7 +246,7 @@ test_defaulted_packbits_gray_loads() { grep -Eq 'Compression:[[:space:]]+PackBits' "$log" } -test_noncompliant_embedded_icc_rejected() { +test_noncompliant_embedded_icc_preserved() { [ -n "$PYTHON" ] || { echo " [SKIP] python3 unavailable"; return 0; } local srgb="$ICCDEV_TESTING/sRGB_v4_ICC_preference.icc" [ -f "$srgb" ] || { echo " [SKIP] missing $srgb"; return 0; } @@ -244,10 +274,11 @@ PY "$TIFFDUMP" "$tif" "$out" > "$export_log" 2>&1 || status=$? [ "$status" -ge 1 ] && [ "$status" -le 127 ] || return 1 grep -Fq "violates the ICC specification" "$export_log" || return 1 - [ ! -e "$out" ] # must NOT have written the non-conformant profile (#1380) + grep -Fq "Profile extracted byte-for-byte" "$export_log" || return 1 + cmp -s "$bad" "$out" } -test_malformed_embedded_icc_rejected() { +test_malformed_embedded_icc_preserved() { [ -n "$PYTHON" ] || { echo " [SKIP] python3 unavailable"; return 0; } local bad="$OUTDIR/malformed.icc" local tif="$OUTDIR/rgb-malformed-icc.tif" @@ -271,7 +302,45 @@ PY "$TIFFDUMP" "$tif" "$out" > "$export_log" 2>&1 || status=$? [ "$status" -ge 1 ] && [ "$status" -le 127 ] || return 1 grep -Fq "Unable to open embedded ICC profile" "$export_log" || return 1 - [ ! -e "$out" ] + grep -Fq "Profile extracted byte-for-byte" "$export_log" || return 1 + cmp -s "$bad" "$out" +} + +test_nested_embedded_icc_is_bounded_and_preserved() { + [ -n "$PYTHON" ] || { echo " [SKIP] python3 unavailable"; return 0; } + local nested="$OUTDIR/nested-depth-512.icc" + local tif="$OUTDIR/nested-depth-512.tif" + local out="$OUTDIR/extracted-nested-depth-512.icc" + local log="$OUTDIR/tiffdump-nested-depth-512.log" + local status=0 + + generate_nested_icc "$nested" 512 || return 1 + generate_tiff_with_icc "$tif" "$nested" || return 1 + rm -f "$out" + + timeout 10 "$TIFFDUMP" "$tif" "$out" > "$log" 2>&1 || status=$? + [ "$status" -ne 124 ] || { echo " iccTiffDump timed out"; return 1; } + [ "$status" -ge 1 ] && [ "$status" -le 127 ] || return 1 + grep -Fq "Profile extracted byte-for-byte" "$log" || return 1 + grep -Fq "Subprofile recursion halted" "$log" || return 1 + cmp -s "$nested" "$out" +} + +test_recursive_full_tag_read_is_bounded() { + [ -n "$PYTHON" ] || { echo " [SKIP] python3 unavailable"; return 0; } + [ -x "$TOXML" ] || { echo " [SKIP] missing executable: $TOXML"; return 0; } + local nested="$OUTDIR/readtags-nested-depth-512.icc" + local xml="$OUTDIR/readtags-nested-depth-512.xml" + local log="$OUTDIR/readtags-nested-depth-512.log" + local status=0 + + generate_nested_icc "$nested" 512 || return 1 + rm -f "$xml" + + timeout 10 "$TOXML" "$nested" "$xml" > "$log" 2>&1 || status=$? + [ "$status" -ne 124 ] || { echo " iccToXml timed out"; return 1; } + [ "$status" -ge 1 ] && [ "$status" -le 127 ] || return 1 + grep -Fq "Unable to read" "$log" } echo "=== iccTiffDump output hardening regression ===" @@ -282,8 +351,10 @@ run_ok "tiffdump-extra-arg-reject" test_extra_arg_rejected run_ok "tiffdump-no-profile-export-reject" test_missing_embedded_profile_export_rejected run_ok "tiffdump-palette-photometric-report" test_palette_photometric_report run_ok "tiffdump-defaulted-packbits-gray-load" test_defaulted_packbits_gray_loads -run_ok "tiffdump-noncompliant-embedded-icc-reject" test_noncompliant_embedded_icc_rejected -run_ok "tiffdump-malformed-embedded-icc-reject" test_malformed_embedded_icc_rejected +run_ok "tiffdump-noncompliant-embedded-icc-preserve" test_noncompliant_embedded_icc_preserved +run_ok "tiffdump-malformed-embedded-icc-preserve" test_malformed_embedded_icc_preserved +run_ok "tiffdump-nested-embedded-icc-bounded-preserve" test_nested_embedded_icc_is_bounded_and_preserved +run_ok "embedded-profile-full-tag-read-bounded" test_recursive_full_tag_read_is_bounded echo "iccTiffDump output hardening regression: $pass passed, $fail failed, $((pass + fail)) total" diff --git a/.github/scripts/iccdev-tool-coverage-baseline.sh b/.github/scripts/iccdev-tool-coverage-baseline.sh index 20e19e6ee..90b81a686 100755 --- a/.github/scripts/iccdev-tool-coverage-baseline.sh +++ b/.github/scripts/iccdev-tool-coverage-baseline.sh @@ -993,7 +993,7 @@ else fi if [ -f "$REPO_ROOT/.github/ci/test-data/spectral/spec_1" ]; then - run_expect_exit "tdump-06b" "Reject ICC export when TIFF has no profile" 255 \ + run_expect_exit "tdump-06b" "Reject ICC export when TIFF has no profile" 1 \ "$TIFFDUMP" "$REPO_ROOT/.github/ci/test-data/spectral/spec_1" "$OUTDIR/tiff_no_profile.icc" else skip_test "tdump-06b" "Reject ICC export when TIFF has no profile" "no-profile TIFF fixture unavailable" @@ -1059,7 +1059,7 @@ newline.icc" "$tool" "$odd" "$dst" > "$log" 2>&1 [ -s "$dst" ] grep -Fq "Filename: $outdir/name_with_\\nnewline.tif" "$log" - grep -Fq "Profile extracted to: $outdir/export_with_\\nnewline.icc" "$log" + grep -Fq "Profile extracted byte-for-byte to: $outdir/export_with_\\nnewline.icc" "$log" ' _ "$ICCDEV_TESTING/hybrid/Data/TShirtDesignKW.tif" "$TIFFDUMP" "$OUTDIR" else skip_test "tdump-08" "Escape controlled ICC description text" "TShirtDesignKW TIFF fixture unavailable" diff --git a/IccProfLib/IccProfile.cpp b/IccProfLib/IccProfile.cpp index 68f1fe409..a8c37c087 100644 --- a/IccProfLib/IccProfile.cpp +++ b/IccProfLib/IccProfile.cpp @@ -976,7 +976,21 @@ bool CIccProfile::loadTags(CIccProfile *pProfile, IccLoadTagsMode mode) */ bool CIccProfile::ReadTags(CIccProfile* pProfile) { - return loadTags(pProfile, icLoadTagsFull); + static thread_local unsigned int readTagsDepth = 0; + static const unsigned int maxReadTagsDepth = 8; + + if (readTagsDepth >= maxReadTagsDepth) + return false; + + class ReadTagsDepthGuard { + public: + ReadTagsDepthGuard(unsigned int &depth) : m_depth(depth) { m_depth++; } + ~ReadTagsDepthGuard() { m_depth--; } + private: + unsigned int &m_depth; + } depthGuard(readTagsDepth); + + return loadTags(pProfile, icLoadTagsFull); } /** diff --git a/Tools/CmdLine/IccTiffDump/Readme.md b/Tools/CmdLine/IccTiffDump/Readme.md index 0b23a3b58..8e8c0bd74 100644 --- a/Tools/CmdLine/IccTiffDump/Readme.md +++ b/Tools/CmdLine/IccTiffDump/Readme.md @@ -22,6 +22,15 @@ Extract an embedded ICC profile: iccTiffDump image.tif embedded.icc ``` +Extraction copies the TIFF ICC profile field byte-for-byte before profile +parsing and validation. This preserves malformed profiles for diagnostics and +does not rewrite tag offsets, padding, or the profile ID. A later parse or +validation failure is still reported with a nonzero exit status, but does not +remove the extracted forensic artifact. Output is written to a sibling +temporary file and atomically renamed only after the complete write succeeds. +Existing regular files may be replaced atomically; device files, directories, +and symbolic links are rejected as extraction destinations. + The no-argument form is a help/syntax path and exits successfully. Other malformed invocations fail: extra trailing arguments are rejected, missing input files fail, and export requests fail when the TIFF has no embedded ICC profile. diff --git a/Tools/CmdLine/IccTiffDump/iccTiffDump.cpp b/Tools/CmdLine/IccTiffDump/iccTiffDump.cpp index 01a07b32e..4db1c057b 100644 --- a/Tools/CmdLine/IccTiffDump/iccTiffDump.cpp +++ b/Tools/CmdLine/IccTiffDump/iccTiffDump.cpp @@ -70,6 +70,7 @@ #include +#include #include #include "IccCmm.h" #include "IccUtil.h" @@ -82,6 +83,12 @@ #include #include #include +#else +#include +#include +#include +#include +#include #endif typedef struct { @@ -123,6 +130,106 @@ const char* GetId(unsigned long nId, IdList* pIdList) return pIdList->szName; } +static bool IsRegularOutputDestination(const char* szFname) +{ + if (!szFname || !szFname[0]) + return false; + +#if defined(_WIN32) + DWORD attributes = GetFileAttributesA(szFname); + if (attributes != INVALID_FILE_ATTRIBUTES) { + const DWORD rejectedAttributes = FILE_ATTRIBUTE_DEVICE | + FILE_ATTRIBUTE_DIRECTORY | + FILE_ATTRIBUTE_REPARSE_POINT; + return !(attributes & rejectedAttributes); + } + DWORD error = GetLastError(); + return error == ERROR_FILE_NOT_FOUND || error == ERROR_PATH_NOT_FOUND; +#else + struct stat st; + if (lstat(szFname, &st) == 0) + return S_ISREG(st.st_mode); + return errno == ENOENT; +#endif +} + +static bool WriteEmbeddedIccProfile(const char* szFname, + const unsigned char *pProfMem, + unsigned int nLen) +{ + if (!pProfMem || !nLen || !IsRegularOutputDestination(szFname)) + return false; + + std::string tempName; + FILE *fp = NULL; + int fd = -1; + unsigned int attempt; + + for (attempt = 0; attempt < 100; attempt++) { + char suffix[64]; +#if defined(_WIN32) + snprintf(suffix, sizeof(suffix), ".tmp-%ld-%u", (long)_getpid(), attempt); + tempName = std::string(szFname) + suffix; + fd = _open(tempName.c_str(), _O_WRONLY | _O_CREAT | _O_EXCL | _O_BINARY, + _S_IREAD | _S_IWRITE); + if (fd >= 0) + fp = _fdopen(fd, "wb"); +#else + snprintf(suffix, sizeof(suffix), ".tmp-%ld-%u", (long)getpid(), attempt); + tempName = std::string(szFname) + suffix; + fd = open(tempName.c_str(), O_WRONLY | O_CREAT | O_EXCL, + S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH); + if (fd >= 0) + fp = fdopen(fd, "wb"); +#endif + if (fp) + break; + if (fd >= 0) { +#if defined(_WIN32) + _close(fd); +#else + close(fd); +#endif + remove(tempName.c_str()); + return false; + } + if (errno != EEXIST) + return false; + } + + if (!fp) + return false; + + bool failed = fwrite(pProfMem, 1, nLen, fp) != nLen; + if (!icFlushAndClose(fp)) + failed = true; + + if (failed) { + remove(tempName.c_str()); + return false; + } + + if (!IsRegularOutputDestination(szFname)) { + remove(tempName.c_str()); + return false; + } + +#if defined(_WIN32) + if (!MoveFileExA(tempName.c_str(), szFname, + MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH)) { + remove(tempName.c_str()); + return false; + } +#else + if (rename(tempName.c_str(), szFname) != 0) { + remove(tempName.c_str()); + return false; + } +#endif + + return true; +} + void Usage() { printf("iccTiffDump built with IccProfLib version " ICCPROFLIBVER "\n\n"); @@ -248,15 +355,15 @@ int main(int argc, icChar* argv[]) } else if (argc > 3) { Usage(); - return -1; + return 1; } std::string srcName = icSanitizeConsoleText(argv[1]); CTiffImg SrcImg; if (!SrcImg.Open(argv[1])) { - printf("\nFile [%s] cannot be opened.\n", srcName.c_str()); - return -1; + fprintf(stderr, "\nFile [%s] cannot be opened.\n", srcName.c_str()); + return 1; } printf("-------------------->Tiff Image Dump<---------------------------\n"); @@ -281,50 +388,50 @@ int main(int argc, icChar* argv[]) unsigned int nLen = 0; if (SrcImg.GetIccProfile(pProfMem, nLen)) { printf("Profile: Embedded\n"); + fflush(stdout); + + if (argc > 2) { + std::string dstName = icSanitizeConsoleText(argv[2]); + if (!WriteEmbeddedIccProfile(argv[2], pProfMem, nLen)) { + fprintf(stderr, "\nUnable to extract profile to: %s\n", dstName.c_str()); + SrcImg.Close(); + return 1; + } + printf("\nProfile extracted byte-for-byte to: %s\n", dstName.c_str()); + fflush(stdout); + } // Profile description and metadata CIccProfile *pProfile = OpenIccProfile(pProfMem, nLen); if (!pProfile) { - printf("\nUnable to open embedded ICC profile\n"); + fprintf(stderr, "\nUnable to open embedded ICC profile\n"); SrcImg.Close(); return 1; } + DumpProfileInfo(pProfile, " "); + std::string validateReport; if (!pProfile->ReadTags(pProfile)) { - printf("\nUnable to read embedded ICC profile\n"); + fprintf(stderr, "\nUnable to read embedded ICC profile\n"); delete pProfile; SrcImg.Close(); return 1; } else if (pProfile->Validate(validateReport) > icValidateWarning) { - printf("\nEmbedded ICC profile violates the ICC specification:\n%s", - validateReport.c_str()); + fprintf(stderr, "\nEmbedded ICC profile violates the ICC specification:\n%s", + validateReport.c_str()); delete pProfile; SrcImg.Close(); return 1; } - - DumpProfileInfo(pProfile, " "); - if (argc > 2) { - std::string dstName = icSanitizeConsoleText(argv[2]); - if (SaveIccProfile(argv[2], pProfile)) { - printf("\nProfile extracted to: %s\n", dstName.c_str()); - } - else { - printf("\nUnable to extract profile\n"); - delete pProfile; - SrcImg.Close(); - return -1; - } - } delete pProfile; } else { printf("Profile: None\n"); if (argc > 2) { - printf("\nNo embedded ICC profile to extract\n"); + fprintf(stderr, "\nNo embedded ICC profile to extract\n"); SrcImg.Close(); - return -1; + return 1; } }