Harden PNG PDF image decoding#1933
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ce611a3e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| private static bool IsPngChunkCrcValid(byte[] data, int chunkOffset, int chunkLength) { | ||
| int crcOffset = chunkOffset + 8 + chunkLength; |
There was a problem hiding this comment.
Guard CRC offsets with wide arithmetic
When a malformed PNG advertises a very large positive chunk length after the required chunks, chunkOffset + 8 + chunkLength can overflow to a negative int. The following bounds check then passes and ReadUInt32BigEndian indexes with a negative offset, so public image validation can throw instead of rejecting the PNG. Use long/checked arithmetic for the CRC offset before reading the CRC bytes.
Useful? React with 👍 / 👎.
| if (data.Length <= maxOutputBytes) { | ||
| output = data; | ||
| return true; |
There was a problem hiding this comment.
Stop falling back after hitting the inflate limit
When a PNG IDAT/deflate stream expands beyond MaxPngDecodedBytes, TryCopyToByteArray returns false, but this fallback still returns the compressed input whenever the compressed size is under the limit. The PNG callers then treat those bytes as decoded scanlines, so a crafted oversized payload with a legal filter byte can bypass the new decompressed-size rejection instead of failing; track a limit-exceeded result separately and do not fall back in that case.
Useful? React with 👍 / 👎.
| if (!IsPngChunkCrcValid(data, offset, length)) { | ||
| unsupportedReason = "PNG chunk CRC is invalid."; | ||
| return false; |
There was a problem hiding this comment.
Update remaining zero-CRC PNG fixtures
In test paths that still use the old literal PNG helpers with zero chunk CRCs (for example PdfDocumentCanvasTests.CreateMinimalRgbPng, which is passed to canvas.Image(...)), this new unconditional CRC check rejects the fixture before rendering. This commit only updates some helpers, so the unrelated PDF image/canvas/extractor tests that reuse the remaining literals will start failing until those fixtures get valid CRCs too.
Useful? React with 👍 / 👎.
|
Superseded by #1934, which includes the PNG PDF image decoding hardening along with the broader document parsing security pass and has merged cleanly into master with CI green. Closing this failing one-off Codex task PR to keep the security queue focused on the consolidated fix. |
Motivation
Description
FlateDecoder.TryDecodeand streaming copy logic to enforce a maximum decompressed byte cap and avoid unboundedMemoryStreamgrowth (file:OfficeIMO.Pdf/Reading/Filters/FlateDecoder.cs).MaxPngPixelCount,MaxPngExpandedBytes,MaxPngDecodedBytes) and helpersTryGetPngCheckedLength,TryGetPngScanlineLength,TryGetPngRowByteCount, andTryValidatePngResourceLimitsand use them before any large allocation (file:OfficeIMO.Pdf/Rendering/Writer/PdfWriter.Images.cs).OfficeIMO.Pdf/Rendering/Writer/PdfWriter.Images.cs).TryDecodePngData(capped decompression) and the checked-length helpers so allocations are bounded and validated prior to use (files:PdfWriter.Images.Interlace.cs,PdfWriter.Images.Png16.cs,PdfWriter.Images.cs).OfficeIMO.Tests/Pdf/PdfDocumentPngImageTests.cs,OfficeIMO.Tests/Pdf/PdfPngTestImages.cs,OfficeIMO.Tests/Pdf/PdfDocumentImageValidationTests.cs,OfficeIMO.Tests/Pdf/PdfStamperSupport.cs).Testing
dotnet restore OfficeIMO.slnanddotnet build OfficeIMO.sln --framework net8.0 --no-restore,dotnet build OfficeIMO.Pdf/OfficeIMO.Pdf.csproj --framework net10.0 --no-restore, anddotnet build OfficeIMO.Pdf/OfficeIMO.Pdf.csproj --framework netstandard2.0 --no-restoreand they completed successfully.net8.0:dotnet test OfficeIMO.Tests/OfficeIMO.Tests.csproj --framework net8.0 --filter FullyQualifiedName~OfficeIMO.Tests.Pdf.PdfDocumentPngImageTestspassed (14 tests) anddotnet test OfficeIMO.Tests/OfficeIMO.Tests.csproj --framework net8.0 --filter FullyQualifiedName~OfficeIMO.Tests.Pdf.PdfDocumentImageValidationTestspassed (61 tests).dotnet build OfficeIMO.sln --framework net8.0 --no-restoresucceeded andgit diff --checkproduced no whitespace/line issues worth blocking.If you want, I can split the large
PdfWriter.Images.csinto smaller helper files or tune the resource limits (MaxPngPixelCount,MaxPngDecodedBytes,MaxPngExpandedBytes) to match stricter or looser policies for your deployment.Codex Task