Bug 447408

Summary: Crash when loading metadata for specially-crafted JPEG images
Product: [Applications] digikam Reporter: saaman1377
Component: Metadata-EngineAssignee: Digikam Developers <digikam-bugs-null>
Status: RESOLVED FIXED    
Severity: crash CC: metzpinguin
Priority: NOR    
Version First Reported In: 7.4.0   
Target Milestone: ---   
Platform: Compiled Sources   
OS: Linux   
Latest Commit: https://invent.kde.org/graphics/digikam/commit/80a4117acbbcc81a3cbeb512372457f038f8c666 Version Fixed/Implemented In: 7.5.0
Sentry Crash Report:
Attachments: Image file generated by afl-fuzz which can be used to reproduce the crash

Description saaman1377 2021-12-23 07:27:45 UTC 
Created attachment 144795 [details]
Image file generated by afl-fuzz which can be used to reproduce the crash

SUMMARY

A crash occurs when a specially-crafted JPEG file (attached) is imported and "Item > Edit Metadata..." is selected from the program menus.


STEPS TO REPRODUCE
1. Import the file into a digiKam library (e.g., by copying it into a library folder and refreshing the main view in digiKam)
2. Select the item in the main view (e.g. by clicking on it)
3. Select "Item > Edit Metadata..." from the program menus

OBSERVED RESULT
Debug builds of the program crash with the following assertion failure:

/usr/include/c++/11/bits/stl_vector.h:1063: std::vector<_Tp, _Alloc>::const_reference std::vector<_Tp, _Alloc>::operator[](std::vector<_Tp, _Alloc>::size_type) const [with _Tp = std::pair<unsigned int, unsigned int>; _Alloc = std::allocator<std::pair<unsigned int, unsigned int> >; std::vector<_Tp, _Alloc>::const_reference = const std::pair<unsigned int, unsigned int>&; std::vector<_Tp, _Alloc>::size_type = long unsigned int]: Assertion '__n < this->size()' failed.

EXPECTED RESULT
The item metadata editor opens and displays the file's metadata.

SOFTWARE/OS VERSIONS
Windows: N/A
macOS: N/A
Linux/KDE Plasma: Ubuntu 21.10 on Linux kernel 5.13.0-22-generic
(available in About System)
KDE Plasma Version: N/A (System uses GNOME desktop)
KDE Frameworks Version: 5.86.0-0ubuntu1
Qt Version: 5.15.2

ADDITIONAL INFORMATION
This bug was identified by fuzzing with afl-fuzz; more information about the fuzzing setup can be provided on request. The assertion failure can be traced back to the call to Value::toRational at metaengine_exif.cpp:426 (https://invent.kde.org/graphics/digikam/-/blob/master/core/libs/metadataengine/engine/metaengine_exif.cpp#L426). It appears that the parameter named component is not bounds-checked before toRational is called; thus, the specially-crafted JPEG file features an EXIF exposure time field ("Exif.Photo.ExposureTime") with 0 components, which causes an out-of-bounds (OOB) access when this function is called with the component parameter set to 0 (the default value).

It is worth noting that the exiv2 documentation for toRational (https://www.exiv2.org/doc/classExiv2_1_1Value.html#a595a4cb549bec8c19d290ca3e95a2678) specifies undefined behavior for OOB accesses, and debug assertions like the one above may be disabled in other build configurations, so memory corruption (and the security implications arising from it) may be possible in some circumstances.

Similar bounds-checks may need to be added in other places as well; this is just the one location that afl-fuzz was able to find given my testing code.
Comment 1 Maik Qualmann 2021-12-23 13:17:51 UTC 
Yes, the problem is clear, we have to check with ExifData::size(). I fix this tonight...

Maik
Comment 2 Maik Qualmann 2021-12-23 19:37:48 UTC 
Git commit 80a4117acbbcc81a3cbeb512372457f038f8c666 by Maik Qualmann.
Committed on 23/12/2021 at 19:35.
Pushed by mqualmann into branch 'master'.

check ExifData before convert to rational value
FIXED-IN: 7.5.0

M  +1    -1    NEWS
M  +8    -5    core/libs/metadataengine/engine/metaengine_exif.cpp
M  +10   -0    core/libs/metadataengine/engine/metaengine_xmp.cpp

https://invent.kde.org/graphics/digikam/commit/80a4117acbbcc81a3cbeb512372457f038f8c666
Comment 3 saaman1377 2021-12-23 22:56:54 UTC 
Maik,

Thank you for the prompt response and fix. Do you think it would make more sense, though, to introduce checks like "if (component < (*it).count())" instead of "if ((*it).count())" since component is a parameter that can be set to values other than 0? (I'm not sure there are currently any usages that specify other values, but this seems like a sensible precaution to avoid future problems.)
Comment 4 Maik Qualmann 2021-12-24 16:37:57 UTC 
I don't think the "component" parameter is a problem, most of the checks are <= 0.

Maik
Comment 5 saaman1377 2021-12-27 05:45:40 UTC 
Maik,

I agree that it's unlikely to be an immediate problem; I was just thinking that it could be an issue in the future.
I'm not very experienced with this code base, though, so I'll defer to your expertise on this.