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
ExifParser: Fix several out of bounds accesses while parsing exif information #22380
Conversation
When parsing exif information the number of entries could be tainted. Make sure to not run behind the exif data by properly checking out of bounds. Upstream? fix: https://android.googlesource.com/platform/external/jhead/+/34a2564d3268a5ca1472c5076675782fbaf724d6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine if it fixes the issues.
I agree that we need to move to using upstream libexif instead.
| @@ -376,7 +377,7 @@ void CExifParse::ProcessDir(const unsigned char* const DirStart, | |||
| unsigned OffsetVal; | |||
| OffsetVal = (unsigned)Get32(DirEntry+8, m_MotorolaOrder); | |||
| // If its bigger than 4 bytes, the dir entry contains an offset. | |||
| if (OffsetVal+ByteCount > ExifLength) | |||
| if (OffsetVal > UINT32_MAX - ByteCount || OffsetVal + ByteCount > ExifLength) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (OffsetVal > UINT32_MAX - ByteCount || OffsetVal + ByteCount > ExifLength) | |
| if (OffsetVal > std::numeric_limits<uint32_t>::max() - ByteCount || OffsetVal + ByteCount > ExifLength) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as we have an "upstream" copy - I would suggest to stay as near to it as possible to ease future cherry-pick / fixes.
Please merge ahead if you agree, seeing that you already acked.
Several drafted images could crash kodi. This was tested with memory, address sanitizers enabled.
cmake -DENABLE_VAAPI=1 -DCORE_PLATFORM_NAME=wayland -DCMAKE_BUILD_TYPE=Debug -DAPP_RENDER_SYSTEM=gl -DECM_ENABLE_SANITIZERS=address,memory
Example Output:
SUMMARY: AddressSanitizer: heap-buffer-overflow (/home/fritsch/Desktop/xbmc-fritsch/xbmc/build/kodi-wayland+0x3dca321) in CExifParse::Get32(void const*, bool)
Shadow bytes around the buggy address:
0x0c3c8004e070: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0c3c8004e080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0c3c8004e090: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0c3c8004e0a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0c3c8004e0b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c3c8004e0c0: 00 00 00 00 00[04]fa fa fa fa fa fa fa fa fa fa
0x0c3c8004e0d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c3c8004e0e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c3c8004e0f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c3c8004e100: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c3c8004e110: 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
Shadow gap: cc
==824657==ABORTING
via: https://paste.kodi.tv/kagukejefa.kodi
The issues were long fixed upstream: https://android.googlesource.com/platform/external/jhead/+/2a4c12f5e5808e309b9ba04fe8b1539debf466d1
Kodi should remove the copied libexif code and use jhead directly.
This fixes: #22377