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

Various security fixes #15

Merged
merged 15 commits into from
Jul 29, 2024
Merged

Various security fixes #15

merged 15 commits into from
Jul 29, 2024

Conversation

chan233
Copy link
Contributor

@chan233 chan233 commented Jul 24, 2024

No description provided.

chan233 and others added 12 commits July 24, 2024 15:18
    - Added checks to ensure buffer reads do not exceed the allocated size.
    - Verified minimum and maximum box sizes to prevent invalid data handling.
    - Ensured presence of 'ftyp' and 'meta' headers before parsing.
    - Improved error handling to prevent heap overflow.
    - Added checks to ensure `ftyp_box_length` is within a valid range and does not cause integer overflow.
    - Implemented a check to prevent `ftyp_box_length` from exceeding the maximum allowable size before adding any constants.
    - Ensured that `ftyp_box_length + 12` does not exceed the file length, preventing potential buffer overflow or memory access errors.
    - Added checks to ensure that the buffer size is sufficient before accessing data, preventing potential heap read overflows.
    - Ensured that at least 24 bytes are available before accessing IHDR chunk information.
    - Validated buffer size to be at least 40 bytes when handling the CgBI chunk followed by IHDR.
    - These changes prevent accessing out-of-bounds memory, ensuring safe parsing of PNG files.
    - Added checks to ensure that resolution strings contain only numeric characters before calling `std::stol`.
    - Implemented `is_numeric` function to validate that resolution parts are numeric.
    - Added `try-catch` block around `std::stol` to handle potential exceptions due to invalid input.
    - Ensured that parsed resolution values are positive integers, preventing invalid dimensions.
    - Improved boundary checks when extracting resolution strings from the header to avoid out-of-bounds errors.

These changes prevent heap read overflows and handle cases where resolution values contain illegal characters, ensuring safe and accurate parsing of HDR image files.
    - Added a check using find before accessing size_map to ensure the key exists, preventing std::out_of_range exceptions.
    - Updated logic to handle cases where the ICNS type is not found in size_map by returning false.
    - Ensured that max_size and entry_sizes are only updated with valid data from size_map.
    - These changes prevent potential crashes due to accessing non-existent keys in the size map and improve the robustness of the ICNS file parsing.
    - Added check and adjustment to `size` to ensure `offset + size` does not exceed `length_`, preventing out-of-bounds access.
    - Retained the assertion `assert(offset + size <= length_)` as a final check for data integrity.
    - Ensured that the buffer reading operations respect the boundaries of the available data, avoiding potential crashes or data corruption.
    - These changes address the assertion failure and improve the robustness of the `read_buffer` function.
    - Added checks to ensure that the `offset` and `size` do not exceed the file length before reading data, preventing out-of-bounds access.
    - Verified that the buffer size is sufficient before processing TIFF directory entries.
    - Implemented additional checks to handle cases where the `offset` might lead to out-of-bounds reads, especially during the iteration through TIFF tags.
    - These changes improve the stability and security of the TIFF parsing function by preventing potential crashes and data corruption due to incorrect offsets.
@xiaozhuai
Copy link
Owner

Thanks @chan233.
I've made some changes and add test files that you created. The CI is green now : )
Would you please test it again? Thanks again for your contribution.

@chan233
Copy link
Contributor Author

chan233 commented Jul 24, 2024

Thanks @chan233. I've made some changes and add test files that you created. The CI is green now : ) Would you please test it again? Thanks again for your contribution.

Of course, if I find any bugs, I'll continue to submit PR.

@xiaozhuai xiaozhuai changed the title re-submit Various security fixes Jul 24, 2024
    - Added checks to ensure `offset + 8` does not exceed the end of the buffer before reading `box_size`.
    - Ensured that `offset + box_size` does not go beyond the buffer's end to prevent buffer overflows.
    - These changes prevent reading outside the allocated memory, which could lead to security vulnerabilities.
@chan233
Copy link
Contributor Author

chan233 commented Jul 24, 2024

@xiaozhuai
crash_avif_3.tar.gz

@xiaozhuai
Copy link
Owner

@xiaozhuai
crash_avif_3.tar.gz

Fixed.

@chan233
Copy link
Contributor Author

chan233 commented Jul 26, 2024

new_crashs.tar.gz
@xiaozhuai
Here are some new samples that cause the program to crash. I tried to fix the issue but was unsuccessful. This is the call stack leading to the crash, and it's still caused by the try_avif_heic() function. I'll leave the rest of the work to you~

./imageinfo crash_1
=================================================================
==674991==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x612000000140 at pc 0x5a1b6c4573b2 bp 0x7ffd235421d0 sp 0x7ffd235421c8
READ of size 4 at 0x612000000140 thread T0
    #0 0x5a1b6c4573b1 in unsigned int imageinfo::Buffer::read_int<unsigned int>(long, bool) /home/dehua/Desktop/fuzz/fuzz_project/image_fuzzing/imageinfo/include/imageinfo.hpp:277:17
    #1 0x5a1b6c4573b1 in imageinfo::Buffer::read_u32_be(long) /home/dehua/Desktop/fuzz/fuzz_project/image_fuzzing/imageinfo/include/imageinfo.hpp:261:56
    #2 0x5a1b6c4573b1 in imageinfo::try_avif_heic(imageinfo::ReadInterface&, unsigned long, imageinfo::ImageInfo&) /home/dehua/Desktop/fuzz/fuzz_project/image_fuzzing/imageinfo/include/imageinfo.hpp:513:36
    #3 0x5a1b6c452919 in imageinfo::parse(imageinfo::ReadInterface&, imageinfo::Format, std::vector<imageinfo::Format, std::allocator<imageinfo::Format> > const&, bool) /home/dehua/Desktop/fuzz/fuzz_project/image_fuzzing/imageinfo/include/imageinfo.hpp:1304:13
    #4 0x5a1b6c451819 in imageinfo::ImageInfo imageinfo::parse<imageinfo::FilePathReader, char const*>(char const* const&, imageinfo::Format, std::vector<imageinfo::Format, std::allocator<imageinfo::Format> > const&, bool) /home/dehua/Desktop/fuzz/fuzz_project/image_fuzzing/imageinfo/include/imageinfo.hpp:1328:12
    #5 0x5a1b6c450a67 in imageinfo::ImageInfo imageinfo::parse<imageinfo::FilePathReader, char const*>(char const* const&, std::vector<imageinfo::Format, std::allocator<imageinfo::Format> > const&, bool) /home/dehua/Desktop/fuzz/fuzz_project/image_fuzzing/imageinfo/include/imageinfo.hpp:1335:12
    #6 0x5a1b6c450a67 in main /home/dehua/Desktop/fuzz/fuzz_project/image_fuzzing/imageinfo/cli/main.cpp:14:21
    #7 0x794ad2e29d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #8 0x794ad2e29e3f in __libc_start_main csu/../csu/libc-start.c:392:3
    #9 0x5a1b6c390614 in _start (/home/dehua/Desktop/fuzz/fuzz_project/image_fuzzing/imageinfo/build/imageinfo+0x24614) (BuildId: 59b45231102966b7a09c44611c2f3cfdd1082382)

0x612000000142 is located 0 bytes to the right of 258-byte region [0x612000000040,0x612000000142)
allocated by thread T0 here:
    #0 0x5a1b6c44e33d in operator new[](unsigned long) (/home/dehua/Desktop/fuzz/fuzz_project/image_fuzzing/imageinfo/build/imageinfo+0xe233d) (BuildId: 59b45231102966b7a09c44611c2f3cfdd1082382)
    #1 0x5a1b6c455327 in imageinfo::Buffer::alloc(unsigned long) /home/dehua/Desktop/fuzz/fuzz_project/image_fuzzing/imageinfo/include/imageinfo.hpp:233:42
    #2 0x5a1b6c457123 in imageinfo::try_avif_heic(imageinfo::ReadInterface&, unsigned long, imageinfo::ImageInfo&) /home/dehua/Desktop/fuzz/fuzz_project/image_fuzzing/imageinfo/include/imageinfo.hpp:496:17
    #3 0x5a1b6c452919 in imageinfo::parse(imageinfo::ReadInterface&, imageinfo::Format, std::vector<imageinfo::Format, std::allocator<imageinfo::Format> > const&, bool) /home/dehua/Desktop/fuzz/fuzz_project/image_fuzzing/imageinfo/include/imageinfo.hpp:1304:13
    #4 0x5a1b6c451819 in imageinfo::ImageInfo imageinfo::parse<imageinfo::FilePathReader, char const*>(char const* const&, imageinfo::Format, std::vector<imageinfo::Format, std::allocator<imageinfo::Format> > const&, bool) /home/dehua/Desktop/fuzz/fuzz_project/image_fuzzing/imageinfo/include/imageinfo.hpp:1328:12
    #5 0x5a1b6c450a67 in imageinfo::ImageInfo imageinfo::parse<imageinfo::FilePathReader, char const*>(char const* const&, std::vector<imageinfo::Format, std::allocator<imageinfo::Format> > const&, bool) /home/dehua/Desktop/fuzz/fuzz_project/image_fuzzing/imageinfo/include/imageinfo.hpp:1335:12
    #6 0x5a1b6c450a67 in main /home/dehua/Desktop/fuzz/fuzz_project/image_fuzzing/imageinfo/cli/main.cpp:14:21
    #7 0x794ad2e29d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16

SUMMARY: AddressSanitizer: heap-buffer-overflow /home/dehua/Desktop/fuzz/fuzz_project/image_fuzzing/imageinfo/include/imageinfo.hpp:277:17 in unsigned int imageinfo::Buffer::read_int<unsigned int>(long, bool)
Shadow bytes around the buggy address:
  0x0c247fff7fd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c247fff7fe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c247fff7ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c247fff8000: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c247fff8010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c247fff8020: 00 00 00 00 00 00 00 00[02]fa fa fa fa fa fa fa
  0x0c247fff8030: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c247fff8040: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c247fff8050: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c247fff8060: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c247fff8070: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==674991==ABORTING

@xiaozhuai
Copy link
Owner

xiaozhuai commented Jul 28, 2024

@chan233 I've tried all the 25 crash samples, but I didn't repro it. Did you checkout this pr and rebuild imageinfo_cli?

xiaozhuai@xiaozhuai-m3-mbp ~/work/imageinfo (chan233-master) $ for file in $(ls /Users/xiaozhuai/Desktop/new_crashs); do ./cmake-build-release/imageinfo /Users/xiaozhuai/Desktop/$file; done
File: /Users/xiaozhuai/Desktop/crash_1
  - Error    : Unrecognized format
File: /Users/xiaozhuai/Desktop/crash_2
  - Error    : Unrecognized format
File: /Users/xiaozhuai/Desktop/crash_3
  - Error    : Unrecognized format
File: /Users/xiaozhuai/Desktop/crash_4
  - Error    : Unrecognized format
File: /Users/xiaozhuai/Desktop/crash_5
  - Error    : Unrecognized format
File: /Users/xiaozhuai/Desktop/crash_6
  - Error    : Unrecognized format
File: /Users/xiaozhuai/Desktop/crash_7
  - Error    : Unrecognized format
File: /Users/xiaozhuai/Desktop/crash_8
  - Error    : Unrecognized format
File: /Users/xiaozhuai/Desktop/crash_9
  - Error    : Unrecognized format
File: /Users/xiaozhuai/Desktop/crash_10
  - Error    : Unrecognized format
File: /Users/xiaozhuai/Desktop/crash_11
  - Error    : Unrecognized format
File: /Users/xiaozhuai/Desktop/crash_12
  - Error    : Unrecognized format
File: /Users/xiaozhuai/Desktop/crash_13
  - Error    : Unrecognized format
File: /Users/xiaozhuai/Desktop/crash_14
  - Error    : Unrecognized format
File: /Users/xiaozhuai/Desktop/crash_15
  - Error    : Unrecognized format
File: /Users/xiaozhuai/Desktop/crash_16
  - Error    : Unrecognized format
File: /Users/xiaozhuai/Desktop/crash_17
  - Error    : Unrecognized format
File: /Users/xiaozhuai/Desktop/crash_18
  - Error    : Unrecognized format
File: /Users/xiaozhuai/Desktop/crash_19
  - Error    : Unrecognized format
File: /Users/xiaozhuai/Desktop/crash_20
  - Error    : Unrecognized format
File: /Users/xiaozhuai/Desktop/crash_21
  - Error    : Unrecognized format
File: /Users/xiaozhuai/Desktop/crash_22
  - Error    : Unrecognized format
File: /Users/xiaozhuai/Desktop/crash_23
  - Error    : Unrecognized format
File: /Users/xiaozhuai/Desktop/crash_24
  - Error    : Unrecognized format
File: /Users/xiaozhuai/Desktop/crash_25
  - Error    : Unrecognized format

@chan233
Copy link
Contributor Author

chan233 commented Jul 28, 2024

@xiaozhuai
Using this CMakeLists file for compilation, you will see the crash logs
CMakeLists.txt

@xiaozhuai
Copy link
Owner

new_crashs.tar.gz
@xiaozhuai

@chan233 Fixed

@chan233
Copy link
Contributor Author

chan233 commented Jul 29, 2024

@xiaozhuai
You can run your batch command several times. Although it's occasional, it can still trigger a 'Segmentation fault.'

for file in $(ls ../../new_crashs); do echo $file ;./imageinfo $file; done
crash_1
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
Segmentation fault
crash_10
File: crash_10
  - Error    : Unrecognized format
crash_11
File: crash_11
  - Error    : Unrecognized format
crash_12
File: crash_12
  - Error    : Unrecognized format
crash_13
File: crash_13
  - Error    : Unrecognized format
crash_14
File: crash_14
  - Error    : Unrecognized format
crash_15
File: crash_15
  - Error    : Unrecognized format
crash_16
File: crash_16
  - Error    : Unrecognized format
crash_17
File: crash_17
  - Error    : Unrecognized format
crash_18
File: crash_18
  - Error    : Unrecognized format
crash_19
File: crash_19
  - Error    : Unrecognized format
crash_2
File: crash_2
  - Error    : Unrecognized format
crash_20
File: crash_20
  - Error    : Unrecognized format
crash_21
File: crash_21
  - Error    : Unrecognized format
crash_22
File: crash_22
  - Error    : Unrecognized format
crash_23
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
Segmentation fault
crash_24
File: crash_24
  - Error    : Unrecognized format
crash_25
File: crash_25
  - Error    : Unrecognized format
crash_3
File: crash_3
  - Error    : Unrecognized format
crash_4
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
Segmentation fault
crash_5
File: crash_5
  - Error    : Unrecognized format
crash_6
File: crash_6
  - Error    : Unrecognized format
crash_7
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
Segmentation fault
crash_8
File: crash_8
  - Error    : Unrecognized format
crash_9
File: crash_9
  - Error    : Unrecognized format

@xiaozhuai
Copy link
Owner

@chan233 Very odd, I can't repro it.

@chan233
Copy link
Contributor Author

chan233 commented Jul 29, 2024

@xiaozhuai
Maybe it's an issue with my computer. Currently, the tests are not showing any problems, so I think we can merge the branches.

@xiaozhuai
Copy link
Owner

I think we can merge the branches.

Ok, thanks for the contribution. I will merge this pr soon.

@xiaozhuai xiaozhuai merged commit 4f31758 into xiaozhuai:master Jul 29, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants