Handle EOF in Jpeg bit reader when data is bad to prevent DOS attack. #2516
Conversation
JimBobSquarePants
requested review from
tocsoft,
antonfirsov,
dlemstra and
brianpopow
| @@ -212,7 +212,7 @@ public bool FindNextMarker() | |||
| private int ReadStream() | |||
| { | |||
| int value = this.badData ? 0 : this.stream.ReadByte(); | |||
| if (value == -1) | |||
| if (value == -1 || this.stream.Position >= this.stream.Length) | |||
There was a problem hiding this comment.
Seeking to any location beyond the length of the stream is supported.
https://learn.microsoft.com/en-us/dotnet/api/system.io.stream.position?view=net-7.0#remarks
There was a problem hiding this comment.
Turns out the fix is bad. It's somehow preventing complete data reading.
There was a problem hiding this comment.
Proper fix implemented now.
| @@ -9,6 +9,7 @@ on: | |||
| pull_request: | |||
| branches: | |||
| - main | |||
| - release/* | |||
There was a problem hiding this comment.
Allow the trigger of the build for PRs against release branches.
| public void DecodeHang<TPixel>(TestImageProvider<TPixel> provider) | ||
| where TPixel : unmanaged, IPixel<TPixel> | ||
| { | ||
| using Image<TPixel> image = provider.GetImage(JpegDecoder.Instance); |
There was a problem hiding this comment.
There's no way to test this without actually decoding the image so I've tried to use as little memory as possible.
| @@ -50,7 +50,7 @@ public static void IterateRows<T>(Configuration configuration, Rectangle rectang | |||
| int width = rectangle.Width; | |||
| int height = rectangle.Height; | |||
|
|
|||
| int maxSteps = DivideCeil(width * height, parallelSettings.MinimumPixelsProcessedPerTask); | |||
| int maxSteps = DivideCeil(width * (long)height, parallelSettings.MinimumPixelsProcessedPerTask); | |||
There was a problem hiding this comment.
The ParallelRowIterator improvement needs its' own test case, since it's not being used by JpegDecoder. Maybe it's better to do this change in a separate (3.*-only) PR?
There was a problem hiding this comment.
I've kept it in for simplicities sake but added tests.
| @@ -212,7 +212,7 @@ public bool FindNextMarker() | |||
| private int ReadStream() | |||
| { | |||
| int value = this.badData ? 0 : this.stream.ReadByte(); | |||
| if (value == -1) | |||
| if (value == -1 || (this.badData && this.data == 0 && this.stream.Position >= this.stream.Length)) | |||
There was a problem hiding this comment.
Can you describe the stuff behind || in a comment? Maybe you have some good insights after resolving #2516 (comment).
|
I'll squash these commits when merging so I should be able to PR the v3.0.2 branch against main once done but if not, I'll be able to checrry pick the commit and PR that. |
…#2516) * Handle EOF in bit reader when data is bad. * Allow parallel processing of multi-megapixel image * Stream seek can exceed the length of a stream * Try triggering on release branches * Update JpegBitReader.cs * Skin on Win .NET 6 * All Win OS is an issue * Address feedback * add validation to CanIterateWithoutIntOverflow --------- Co-authored-by: antonfirsov <antonfir@gmail.com>
Prerequisites
Description
This specially crafted file has had the start of frame marker altered to change the dimensions to values (65503x65503) that far exceed the encoded data. Nothing seems to be able to open this but with a very small change we actually can and quickly too.
It's better that we allow decoding than explicitly throwing because there is no way to tell the difference between a maliciously crafted file with complete data from a damaged file with limited data and we already work very hard to enable decoding the latter.
I've also added a fix to allow us to actually parallel process images of this size. I discovered the iterator suffered from an overflow issue during attempts to determine what the maximum dimensions of a file Windows can handle (65500x65500) as it cannot decode a reencoded of the full dimensions with any built in applications.
I've created a branch (release/v3.0.2) based of the v3.0.1 tagged commit and cherry-picked #2501. Once merged I'll publish a release based upon that branch I'll backport this fix into release/v2.x for publishing.