Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potential memory leak bug #337

Closed
5hadowblad3 opened this issue Feb 27, 2019 · 9 comments
Closed

Potential memory leak bug #337

5hadowblad3 opened this issue Feb 27, 2019 · 9 comments
Labels

Comments

@5hadowblad3
Copy link

5hadowblad3 commented Feb 27, 2019

Recently, I use my fuzzer to check the project and find a memory leak or potential overflow problem.
The program itself will be stuck in an endless decompressing cycle.

This is the version:
image

This is the input:
input.zip

You can use ./djpeg file to reproduce the memory leak.

image

This is the result with gdb.

I am looking forward to your reply.

@dcommander
Copy link
Member

If the program is stuck in an endless loop, then of course there will be unfreed memory when you terminate it, since the program is not able to properly terminate and clean up the memory it uses. I suspect that this image is reproducing one of the known issues described here: https://libjpeg-turbo.org/pmwiki/uploads/About/TwoIssueswiththeJPEGStandard.pdf

@5hadowblad3
Copy link
Author

Actually, the situation is a bit different. In this situation, the output can be larger 10G in a very short time.
The problem is related to jpeg_read_scanlines function in djpeg.c file.
I would like to know what should we do to prevent this issue.

@dcommander
Copy link
Member

It appears to me to be virtually the same issue as LJT-01-004. It's just more severe because the specified width/height in the image header are greater (32739x65037 vs. 16448x30840 in the Cure53 image.) There is no way to "fix" it per se, because it isn't actually a bug. It's an exploitation of the fundamental design of the progressive JPEG format-- more specifically an optimization within that format that allows many consecutive zeroes to be represented by only a few bytes. The JPEG format can technically accommodate 64k x 64k pixels, which would require 16 GB of memory in order to fully decompress without buffered I/O. Obviously there is no way to prevent that kind of memory usage with a completely valid JPEG image without somehow restricting the types of valid JPEG images you are willing to decompress. So really what you and Cure53 are objecting to is the fact that so much memory can be consumed by such a small JPEG image, but in fact, the JPEG image in question is not valid.

Ways to work around the problem:

  1. As described in the aforementioned report, you can make libjpeg warnings fatal in your own program. You can also use the TurboJPEG API, which provides a more user-friendly way of making libjpeg warnings fatal. As the report mentions, there are still ways that a perfectly valid JPEG image (i.e. a JPEG image that doesn't produce any warnings) can be crafted such that it produces a variant of LJT-01-004, but that variant is less severe in scope (i.e. it will use 1 GB of memory to decompress a 2 MB image, not 10 GB to decompress a 4 KB image.) If libjpeg warnings are fatal, then decompression will abort as soon as any image corruption is detected. If libjpeg warnings are ignored (as is the default with djpeg), then decompression will continue until it completes, regardless of corruption. Some programs prefer the latter behavior, because it will allow them to decompress as many pixels as possible from a corrupt image rather than skipping any pixels after the point of corruption.
  2. Limit libjpeg memory usage, using the -maxmemory argument to djpeg or the cinfo.max_memory_to_use field (if you are writing your own program) or the JPEGMEM environment variable. For instance, djpeg -maxmemory 2048M input.jpg effectively works around the problem in this case, by limiting overall memory usage for progressive decompression to roughly 2 GB.
  3. Set a limit on the image width and height. This particular bogus input image has a header that sets the image width/height to 32739x65037, which is probably beyond a reasonable limit for most applications. Particularly if your application is decompressing entirely to memory, as opposed to using buffered I/O, then it should never try to decompress an image that is too large to fit in available memory.

@5hadowblad3
Copy link
Author

OK, but since these problems have been existed for many years. Is there any breakthrough in the theory level or algorithm improvement?

@dcommander
Copy link
Member

I spent many hours looking at the problem, under contract for Mozilla, after the Cure53 report was released. I didn't see a solution other than the workarounds mentioned above. All of my findings are in the aforementioned report. If someone else wants to look into it further, be my guest. As I explained, this isn't a valid JPEG image, so it is not reasonable to expect that any JPEG codec will handle it properly. The libjpeg API warns you that the image isn't valid so that you can choose to abort decompression.

It may be possible to change the buffering behavior of the libjpeg API library, when decoding progressive images, in such a way that the issue doesn't occur, but my experience would suggest that the trade-off for that is likely a loss of performance. Any application in which this exploit could cause a loss of work (a web browser, for instance) should take steps to work around the problem by making warnings fatal, limiting the maximum memory consumption, and/or limiting the maximum image size.

Variations of this issue could be produced even with a valid JPEG image. JPEG is a compression algorithm, so of course it is going to be capable of producing 10 GB of output with much less input. That's true of any compression algorithm, but it's particularly true of a lossy algorithm such as JPEG. What we're really arguing about is how large the input should be before it is acceptable to have that much output. But, as an example, if you compress a solid black 32k x 32k image into a grayscale baseline JPEG with quality level 1, you're going to end up with a 12 MB JPEG image that decompresses to at least 3 GB. Is that acceptable? Well, if it isn't, then I don't know what to do about it other than to suggest using a different image format or limiting the image size, as I suggested above. 4 KB is a much more extreme example than 12 MB, but again, the 4 KB image is not valid, and if an application chooses to ignore the warning about that, then it is choosing to open itself to this exploit.

@dcommander
Copy link
Member

As another example, attempting to open your input image in PhotoShop produces an error dialog similar to the warning that libjpeg-turbo produces, and PhotoShop doesn't use libjpeg-turbo. This supports my opinion that making libjpeg warnings fatal is an appropriate solution for commercial/mission-critical applications wishing to work around this problem.

@5hadowblad3
Copy link
Author

In my understanding that the most proper solution currently is to do the restriction in the application level to prevent this problem. If the application does not implement any handling function for the data received from the JPEG interface, then at least there is an easy way to achieve denial of service attack. Right?

Thank you for your patient explanation.

@dcommander
Copy link
Member

It isn't a DoS attack. A DoS involves sending a great deal of bogus data repeatedly to a service (usually remote) in order to keep it so busy that it cannot respond to legitimate requests. This issue is simply the JPEG codec doing what it is supposed to do. These images are really best described as corner cases, and it is up to the application to decide how or whether to handle them. That's why the aforementioned report is entitled "Two Issues with the JPEG Standard" and not "Two Issues with libjpeg-turbo."

If the JPEG header says that the image is 32k x 64k, then the JPEG decompressor has no idea whether the pixel data for that image will be valid or not, because it has not yet read that data. So if, in the case of progressive JPEG, the decompressor needs to pre-allocate a full-image buffer for decompressing multiple scans (which is, BTW, documented libjpeg behavior), then all it can do is trust the header and allocate a 32k x 64k buffer, which will require multiple gigabytes of memory. Since the full-image buffer has to hold 16-bit DCT coefficients, it could potentially be as big as 6 bytes per pixel if the header says that no chroma subsampling was used when compressing the image.

Making libjpeg warnings fatal will catch the most severe corner cases-- cases in which there is a valid JPEG header but most of the pixel data is missing or corrupt. The input image you submitted above was such a case. However, like I said, you can still generate a perfectly valid corner case in which a 2 MB JPEG image can cause many gigabytes of memory to be consumed by the decompressor. There is no defense against this other than for applications to appropriately limit the memory used by libjpeg-turbo. For mission-critical applications, I would really suggest that they use all three workarounds:

  1. Make libjpeg warnings fatal to catch the most severe corner cases.
  2. Use cinfo.max_memory_to_use to ensure that libjpeg-turbo will never use more than the available physical memory (or some percentage thereof) when allocating full-image buffers for progressive JPEG decompression.
  3. Calculate, based on available physical memory (or some percentage thereof), the maximum JPEG image size that can be decompressed, and if an image header is encountered that exceeds that maximum size, don't read beyond the header.

@5hadowblad3
Copy link
Author

5hadowblad3 commented Mar 13, 2019

What I mean DoS is that to send these images multiple time for the server to decompress, therefore, lots of memory will be used and finally no memory left for other tasks. Since these images will not be that large, it is hard for some ordinary DoS detectors only focusing on the traffic amount to detect such behavior.

I understand your points, but I just wonder whether there is a better solution for this. Because restricting the size sometimes causes other inconvenience in real life applications.

Anyway, thanks again for your patient explanation. I will consider this problem during future exploration and maybe have some clue for a better solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants