Avoid reading unused bytes from PNG streams

[Draczech]

Trying to address https://github.com/drewnoakes/metadata-extractor/issues/534

[Draczech]

Check added in PngChunkReader as allocating space in StreamReader may be possible and generally there is nothing wrong with it. Just allocating enormous byte buffers should not be allowed in the first place.

Tested only as with the file attached to https://github.com/drewnoakes/metadata-extractor/issues/534:

    @Test(expected = PngProcessingException.class)
    public void testCorruptedFile() throws Exception
    {
        processFile("Tests/Data/favicon_better_essay_service_com.png");
    }

As I do not intend to mock the StreamReader or so. But better test might be desired.

[drewnoakes]

@Draczech thanks for following up with this. The change looks sensible.

One funny thing: In other repos I often ask for unit tests to be added. In this repo, I usually ask for them to be removed :) Well, the test is fine, but the image file -- not so much. Rather than add lots of binary files to this repo, we maintain a separate collection of test images and regularly run regression tests against them. We also use these files to drive parity between the Java and .NET implementations. Please remove the file from this PR (and the tests if they cannot be updated to work without the file) and we will add the file to this repo instead:

https://github.com/drewnoakes/metadata-extractor-images

I can add it, if you don't want to clone that repo. It's quite large.

[Draczech]

OK, I can see you point. The image file is removed now so are the tests (they don't make much sense without the file. So the pull request has one changed file only.

Please add the file to your image repo. Thank you!

[Nadahar]

While I think this is a good change, I'd also think that it should be possible to define an absolute ridiculous maximum size for at least some chunk types, so that encountering something larger would abort parsing/declare the file invalid.

@drewnoakes Is that something worth considering (the "work" is of course to come up with these upper thresholds)?

To explain what I mean, I took a quick look at the PNG spec and can see that:

• IHDR should AFAICT have a fixed size of 13 bytes.
• PLTE has a maximum size of 768 bytes and must be divisible by 3.
• IEND should have a fixed size of 0 bytes.
• tRNS should AFAICT have a maximum size of 256 bytes.

I'm pretty sure that similar thresholds can be found for many other chunk types, I just read the first ones in the standard above.The maximum thresholds doesn't have to be as narrow as the standard defines, they could leave in a lot of "slack" and still provide a reasonable protection against memory reservation issues.

tEXt, zTXt and iTXt are in theory limited by the size of a 32 bit integer, but in reality they are used for things like Title, Author, Description, Copyright, Comment etc. I don't think that setting a maximum size of something ridiculous like say 10 MB, would be "unreasonable" for such chunks?

Maybe PngChunkType should include a new field holding this threshold, where for example negative values (like -1) would indicate that there were no threshold for that chunk type. PngChunkReader could then use this information to reject chunks that are above the threshold if one is defined, before trying to read it.

Or, all this might just be overkill :wink:

[drewnoakes]

To explain what I mean, I took a quick look at the PNG spec and can see that:

• IHDR should AFAICT have a fixed size of 13 bytes.
• PLTE has a maximum size of 768 bytes and must be divisible by 3.
• IEND should have a fixed size of 0 bytes.
• tRNS should AFAICT have a maximum size of 256 bytes.

These seem like reasonable safety rails, though it would be a shame to reject a file that could be parsed just because it had some zero padding that would otherwise be ignored, for example.

tEXt, zTXt and iTXt are in theory limited by the size of a 32 bit integer, but in reality they are used for things like Title, Author, Description, Copyright, Comment etc. I don't think that setting a maximum size of something ridiculous like say 10 MB, would be "unreasonable" for such chunks?

Coming up with a number N for which we assume no chunk could ever be larger than is a tricky problem. These chunks can store arbitrary data (such as XMP).

I tend to prefer the approach of using the length of the whole data stream when it is available, and not otherwise.

Another approach would be to change how we read large byte arrays from the stream. That is, we read in chunks of some amount (say 1MB) so that it's more likely the stream hits EOF than we fail to allocate a crazy large array in the StreamReader. Of course there's a downside to this in that the happy path now has to concatenate a bunch of large arrays together to produce the same result we have today, in cases where the data is actually large.

[Nadahar]

I tend to prefer the approach of using the length of the whole data stream when it is available, and not otherwise.

That would be great, but how do you intend to decide if the data stream size is "known" or not? I've tried to solve this in another project, and the result is anything but pretty (I'm even somewhat embarrassed to post it):

    public static long getInputStreamSize(@Nullable InputStream inputStream) {
        // This method uses whatever means necessary to acquire the source size
        // of known InputStream implementations. If possible acquisition from
        // other InputStream implementations is found, the code to acquire it
        // should be added to this method.
        if (inputStream == null) {
            return -1L;
        }
        if (inputStream instanceof FakeInputStream) {
            return ((FakeInputStream) inputStream).getSize();
        }
        if (inputStream instanceof FilterInputStream) {
            try {
                Field in = FilterInputStream.class.getDeclaredField("in");
                in.setAccessible(true);
                return getInputStreamSize((InputStream) in.get(inputStream));
            } catch (ReflectiveOperationException | IllegalArgumentException e) {
                return -1L;
            }
        }
        if (inputStream instanceof PartialInputStream) {
            PartialInputStream partial = (PartialInputStream) inputStream;
            long result = getInputStreamSize(partial.source);
            if (result < 1L) {
                return result;
            }
            if (partial.getEnd() >= 0L) {
                result = Math.min(result, partial.getEnd());
            }
            return partial.getStart() > 0L ? Math.max(result - partial.getStart(), 0L) : result;
        }
        if (inputStream instanceof SizeLimitInputStream) {
            SizeLimitInputStream limited = (SizeLimitInputStream) inputStream;
            return limited.in == null ? -1L : Math.min(getInputStreamSize(limited.in), limited.maxBytesToRead);
        }
        if (inputStream instanceof DLNAImageInputStream) {
            return ((DLNAImageInputStream) inputStream).getSize();
        }
        if (inputStream instanceof DLNAThumbnailInputStream) {
            return ((DLNAThumbnailInputStream) inputStream).getSize();
        }
        if (inputStream instanceof ByteArrayInputStream) {
            try {
                Field count = ByteArrayInputStream.class.getDeclaredField("count");
                count.setAccessible(true);
                return count.getInt(inputStream);
            } catch (ReflectiveOperationException | IllegalArgumentException e) {
                return -1L;
            }
        }
        if (inputStream instanceof FileInputStream) {
            try {
                return ((FileInputStream) inputStream).getChannel().size();
            } catch (IOException e) {
                return -1L;
            }
        }
        if ("com.sun.imageio.plugins.common.InputStreamAdapter".equals(inputStream.getClass().getName())) {
            try {
                Field stream = inputStream.getClass().getDeclaredField("stream");
                stream.setAccessible(true);
                return ((ImageInputStream) stream.get(inputStream)).length();
            } catch (ReflectiveOperationException | IllegalArgumentException | IOException e) {
                return -1L;
            }
        }
        return -1L;
    }

The above has several problems. There's the use of reflection that isn't allowed in more recent versions of Java, and the fundamental "logic" is that you have to treat each InputStream implementation as a special case - which means that you will end up deeming the size "unknown" for custom implementations where it might in fact be known.

Chunked reading would solve it at the cost of some loss of performance, which of course raises the question if it's "worth it" to lose performance for every valid stream to protect against issues with a very few invalid streams. On the other hand, such invalid streams could be especially crafted to trigger such a problem I guess.

[kwhopper]

Another approach would be to change how we read large byte arrays from the stream.

Shameless plug for drewnoakes/metadata-extractor-dotnet#250, or #361 as a starting point for Java -- only because it's (potentially) a stepping stone towards what you're after. It's creates a single place to hold most buffers instead of splattering them throughout the library, which should make it easier to branch-off under certain conditions.

For example, a future optimization could be writing out certain types of stream content directly to disk transparently.