search

ilyakurdyukov / jpeg-quantsmooth

JPEG artifacts removal based on quantization coefficients.
GNU Lesser General Public License v2.1
366 stars 22 forks source link

[Prevent NULL-ptr dereference] check return value jpeg_read_coefficients #26

Open the-shank opened 1 year ago

the-shank commented 1 year ago

jpeg_read_coefficents can return NULL in certain conditions. If we continue without checking the return value, this could lead to NULL-pointer dereference further down the execution.

This fix checks the return value of call to jpeg_read_coefficents and exits in case it is NULL.

ilyakurdyukov commented 1 year ago
  1. What are these certain conditions?
  2. Please replace spaces with tabs so as not to break the formatting.
ilyakurdyukov commented 1 year ago

I don't think the check should be here.

There should be an error output in do_quantsmooth(), because jpegqs can be a library that can be used in other applications.

    if (!coef_arrays) return 2;

I need a sample to reproduce this.

If this causes jpegqs to crash, then libjpeg's jpegtran utility will probably do the same.

the-shank commented 1 year ago
  1. What are these certain conditions?

In libjpeg-turbo source code at libjpeg-turbo/jdtrans.c, a NULL will be returned whenever the function to consume_input returns a retcode of 0 (JPEG_SUSPENDED).

  1. Please replace spaces with tabs so as not to break the formatting.

Sure, I will update it.

There should be an error output in do_quantsmooth(), because jpegqs can be a library that can be used in other applications. if (!coef_arrays) return 2;

Yes, this would be a better approach to handle it.

I need a sample to reproduce this.

I dont have a sample at hand. Actually, as a part of our research project at Purdue University, we have built a tool that simulates various "faulty" conditions (for example simulating memory exhaustion, in which case malloc would return NULL, for example) in various libraries and then detect if the programs using those libraries handle those cases. The aim of the research is to help programs become more robust to various abnormal/faulty conditions in the libraries that they use. This case was flagged by our tool due to the possibility of jpeg_read_coefficients returning NULL. If it helps, the ASAN stacktrace that is generated when this particular condition is simulated is below.

=================================================================
AddressSanitizerAddressSanitizerAddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
:DEADLYSIGNAL
:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer==954399==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x0000004e1d46 bp 0x7fff1f6fd5f0 sp 0x7fff1f6fd4c0 T0)
:DEADLYSIGNAL
==954399==The signal is caused by a READ memory access.
==954399==Hint: address points to the zero page.
AddressSanitizerAddressSanitizer:DEADLYSIGNAL
:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizerAddressSanitizer:DEADLYSIGNAL
:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizerAddressSanitizer:DEADLYSIGNAL
:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizerAddressSanitizer:DEADLYSIGNAL
:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizerAddressSanitizer:DEADLYSIGNAL
:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
    #0 0x4e1d46 in .omp_outlined._debug__.30 /home/user/code/research/jpegqs_src/jpegqs-1.20210408/./quantsmooth.h:1638:31
    #1 0x4e1d46 in .omp_outlined..31 /home/user/code/research/jpegqs_src/jpegqs-1.20210408/./quantsmooth.h:1633:1
    #2 0x7f32ae4ee312 in __kmp_invoke_microtask (/lib/x86_64-linux-gnu/libomp.so.5+0xc6312)
    #3 0x7f32ae46e20e  (/lib/x86_64-linux-gnu/libomp.so.5+0x4620e)
    #4 0x7f32ae465731 in __kmp_fork_call (/lib/x86_64-linux-gnu/libomp.so.5+0x3d731)
    #5 0x7f32ae4538fb in __kmpc_fork_call (/lib/x86_64-linux-gnu/libomp.so.5+0x2b8fb)
    #6 0x4d435f in do_quantsmooth /home/user/code/research/jpegqs_src/jpegqs-1.20210408/./quantsmooth.h:1633:1
    #7 0x4d435f in main /home/user/code/research/jpegqs_src/jpegqs-1.20210408/quantsmooth.c:531:2
    #8 0x7f32ae209d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #9 0x7f32ae209e3f in __libc_start_main csu/../csu/libc-start.c:392:3
    #10 0x4204c4 in _start (/home/user/code/research/experiments/libjpeg/jpegqs_install/bin/jpegqs+0x4204c4)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /home/user/code/research/jpegqs_src/jpegqs-1.20210408/./quantsmooth.h:1638:31 in .omp_outlined._debug__.30
==954399==ABORTING
ilyakurdyukov commented 1 year ago

I was unable to reproduce this using a JPEG image truncated in every possible place. I believe that this NULL can only happen if the libjpeg functions are called with certain arguments that I don't use.

I don't think that "memory exhaustion, in which case malloc would return NULL" is a big deal, C++ even forgot about it for decades. Only RTOS can return NULL these days. Other OSes are configured to NEVER allocate real memory, only virtual space. So malloc will never return NULL unless the address space is exhausted. In case you didn't know. So I find your work pretty useless, especially if you can't reproduce it with a real input.

The theoretical dereference of a NULL pointer is not a serious problem. Why? Because the program will terminate with an exception. It can't be turned into an RCE.