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.
Yes, the problem is clear, we have to check with ExifData::size(). I fix this tonight... Maik
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
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.)
I don't think the "component" parameter is a problem, most of the checks are <= 0. Maik
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.