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

exiv2 crashes due to assertion '__n < this->size()' failed. #1706

Closed
shao-hua-li opened this issue Jun 10, 2021 · 3 comments · Fixed by #1735
Closed

exiv2 crashes due to assertion '__n < this->size()' failed. #1706

shao-hua-li opened this issue Jun 10, 2021 · 3 comments · Fixed by #1735
Assignees

Comments

@shao-hua-li
Copy link

Hi there,

I crashed exiv2 with a fuzzer generated input, which could cause Assertion '__n < this->size()' failed.

  • exiv2 version: 1.0.0.9 (commit 23dbf4f)
  • Compiler: clang12
  • Platform: Ubuntu 20.04.2 LTS, x86_64
  • Reproduce: run exiv2 -PE poc. You can download the poc from here poc

gdb bt outputs for your convenience:

(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007eff258af859 in __GI_abort () at abort.c:79
#2  0x00007eff25f13716 in std::__replacement_assert (__file=<optimized out>, __line=<optimized out>, __function=<optimized out>, __condition=<optimized out>)
    at /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/x86_64-linux-gnu/c++/11/bits/c++config.h:504
#3  0x00007eff25f63ce8 in std::vector<unsigned int, std::allocator<unsigned int> >::operator[] (this=0x1e45fa0, __n=1)
    at /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_vector.h:1063
#4  Exiv2::ValueType<unsigned int>::toLong (this=0x1e45f90, n=1) at /data/afl_compiler/programs/exiv2/src/fuzz/exiv2/include/exiv2/value.hpp:1642
#5  0x00007eff260780b1 in Exiv2::Internal::PentaxMakerNote::printDate (os=..., value=...) at pentaxmn_int.cpp:1039
#6  0x00007eff25f59fd2 in Exiv2::Exifdatum::write (this=0x1e45ed0, os=..., pMetadata=0x1e05980) at exif.cpp:232
#7  0x00007eff25fa4024 in Exiv2::Metadatum::print[abi:cxx11](Exiv2::ExifData const*) const (this=0x1e45ed0, pMetadata=0x0) at metadatum.cpp:41
#8  0x0000000000449e99 in Action::Print::printMetadatum (this=<optimized out>, md=..., pImage=0x1e05970) at actions.cpp:620
#9  0x0000000000448982 in Action::Print::printMetadata (this=0x1e06b40, image=0x1e05970) at actions.cpp:447
#10 0x0000000000447778 in Action::Print::printList (this=0x1e06b40) at actions.cpp:437
#11 0x0000000000444433 in Action::Print::run (this=0x1e06b40, path=...) at actions.cpp:260
#12 0x000000000042d32a in main (argc=<optimized out>, argv=<optimized out>) at exiv2.cpp:171
@kevinbackhouse kevinbackhouse self-assigned this Jun 14, 2021
@kevinbackhouse
Copy link
Collaborator

The problem here is related to this tag:

{0x0006, "Date", N_("Date"), N_("Date"), pentaxId, makerTags, undefined, -1, printDate},

Apparently undefined is a type that means 1 byte. But the file contains an unsignedLong instead. The parsing seems to be happening here, but I see no attempt to check that the type matches the expected type.

Does anybody know where the type checking is supposed to happen?

@kevinbackhouse
Copy link
Collaborator

One possible solution is to refuse to print when the type of the value doesn't match what we expected:

diff --git a/src/exif.cpp b/src/exif.cpp
index 50ecfa20..e9ea3d68 100644
--- a/src/exif.cpp
+++ b/src/exif.cpp
@@ -229,6 +229,10 @@ namespace Exiv2 {
               os << value().toString();
               fct=NULL;
             }
+            if (ti->typeId_ != value().typeId()) {
+              os << "Value type does not match expected type for tag. " << value();
+              return os;
+            }
         }
         if ( fct ) fct(os, value(), pMetadata);
         return os;

I think that might be too aggressive though. Based on what @clanmills told me, the metadata often doesn't match what it says in manufacturer's specification, so that might prevent us from printing useful information for legitimate files.

I also suspect that the above solution isn't sufficient against malicious files. It looks to me like there are other ways to create a ValueType with a value_ vector that's too short. So I am currently leaning towards replacing all the operator[] calls on std::vector with at() calls. The at() method throws an exception when the index is out of bounds, which means that we can catch the exception.

kevinbackhouse added a commit to kevinbackhouse/exiv2 that referenced this issue Jun 21, 2021
@kevinbackhouse kevinbackhouse linked a pull request Jun 21, 2021 that will close this issue
hassec pushed a commit that referenced this issue Jun 26, 2021
* Regression test for #1706

* Use vector::at() rather than operator[].

* Print to stderr when exception is caught and EXIV2_DEBUG_MESSAGES is enabled.

* Check that it prints "Bad value" for the date.
mergify bot pushed a commit that referenced this issue Jun 26, 2021
* Regression test for #1706

* Use vector::at() rather than operator[].

* Print to stderr when exception is caught and EXIV2_DEBUG_MESSAGES is enabled.

* Check that it prints "Bad value" for the date.

(cherry picked from commit f4d3adb)

# Conflicts:
#	src/value.cpp
hassec added a commit that referenced this issue Jun 27, 2021
* fix: use vector::at() rather than operator[] (#1735)

* Regression test for #1706

* Use vector::at() rather than operator[].

* Print to stderr when exception is caught and EXIV2_DEBUG_MESSAGES is enabled.

* Check that it prints "Bad value" for the date.

(cherry picked from commit f4d3adb)

# Conflicts:
#	src/value.cpp

* fix merge conflicts from mergify backport

Co-authored-by: Kevin Backhouse <kevinbackhouse@github.com>
Co-authored-by: Christoph Hasse <hassec@users.noreply.github.com>
@hassec
Copy link
Member

hassec commented Jun 27, 2021

Should be fixed in main and 0.27-maintenance thanks to @kevinbackhouse

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 a pull request may close this issue.

3 participants