Test from #365 reveals more issues in LoaderExifJpeg

[awilfox]

Describe the bug
Nominally, a test failure.

Practically, potentially a much worse issue.

To Reproduce
Steps to reproduce the behaviour:

1. Run the exiv2 testsuite on a non-x86 computer.

Expected behavior
test_run (bugfixes.github.test_CVE_2018_12265.AdditionOverflowInLoaderExifJpeg) ... OK

Actual behaviour
test_run (bugfixes.github.test_CVE_2018_12265.AdditionOverflowInLoaderExifJpeg) ... FAIL

Desktop (please complete the following information):

• OS: Adélie Linux 1.0-BETA3
• Compiler & Version: GCC 8.3.0
• Compilation mode and/or compiler flags:
  • ppc64: -O2 -ggdb -mcpu=970 -mtune=power9 -maltivec -mlong-double-64 -fno-inline-small-functions
  • ppc: -O2 -ggdb -mcpu=G3 -fno-omit-frame-pointer -mfpu=dp_full
  • aarch64: -O2 -ggdb -mtune=cortex-a53

Additional context

======================================================================
FAIL: test_run (bugfixes.github.test_CVE_2018_12265.AdditionOverflowInLoaderExifJpeg)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/src/packages/user/exiv2/src/exiv2-0.27.1-Source/tests/system_tests.py", line 632, in test_run
    self.compare_stderr(i, command, processed_stderr, stderr)
  File "/usr/src/packages/user/exiv2/src/exiv2-0.27.1-Source/tests/system_tests.py", line 755, in compare_stderr
    msg="Standard error does not match"
  File "/usr/src/packages/user/exiv2/src/exiv2-0.27.1-Source/tests/system_tests.py", line 726, in _compare_output
    expected, got, msg=msg
AssertionError: 'Erro[431 chars]uncating the entry\n' != 'Erro[431 chars]uncating the entry\nUncaught exception: Overflow in addition\n'
  Error: Upper boundary of data for directory Image, entry 0x00fe is out of bounds: Offset = 0x0000002a, size = 64, exceeds buffer size by 22 Bytes; truncating the entry
  Warning: Directory Image, entry 0x0201: Strip 0 is outside of the data area; ignored.
  Warning: Directory Image, entry 0x0201: Strip 7 is outside of the data area; ignored.
  Error: Offset of directory Thumbnail, entry 0x0201 is out of bounds: Offset = 0x00000000; truncating the entry
+ Uncaught exception: Overflow in addition

Setting a breakpoint in the ctor of LoaderExifJpeg shows the following output:

on x86 (i586):

Breakpoint 1, (anonymous namespace)::LoaderExifJpeg::LoaderExifJpeg (parIdx=0, image=..., id=24, this=0xf7ca6ae0) at /usr/src/packages/user/exiv2/src/exiv2-0.27.1-Source/src/preview.cpp:549
549             if (Safe::add(offset_, size_) > static_cast<uint32_t>(image_.io().size()))
(gdb) bt
#0  (anonymous namespace)::LoaderExifJpeg::LoaderExifJpeg (parIdx=0, image=..., id=24, this=0xf7ca6ae0) at /usr/src/packages/user/exiv2/src/exiv2-0.27.1-Source/src/preview.cpp:549
#1  (anonymous namespace)::createLoaderExifJpeg (id=24, image=..., parIdx=0) at /usr/src/packages/user/exiv2/src/exiv2-0.27.1-Source/src/preview.cpp:557
#2  0xf7d980e6 in (anonymous namespace)::Loader::create (id=24, image=...) at /usr/src/packages/user/exiv2/src/exiv2-0.27.1-Source/src/preview.cpp:389
#3  0xf7d99ad8 in Exiv2::PreviewManager::getPreviewProperties (this=<optimized out>, this@entry=0xffffd3b4) at /usr/src/packages/user/exiv2/src/exiv2-0.27.1-Source/src/preview.cpp:1148
#4  0x565731e0 in Action::Extract::writePreviews (this=0xf7ffde30) at /usr/src/packages/user/exiv2/src/exiv2-0.27.1-Source/src/actions.cpp:1134
#5  0x5657b43f in Action::Extract::run (this=0xf7ffde30, path=...) at /usr/src/packages/user/exiv2/src/exiv2-0.27.1-Source/src/actions.cpp:1051
#6  0x5655e52f in main (argc=3, argv=0xffffd5e4) at /usr/src/packages/user/exiv2/src/exiv2-0.27.1-Source/src/exiv2.cpp:169

on ppc:

Breakpoint 1, (anonymous namespace)::LoaderExifJpeg::LoaderExifJpeg (parIdx=16750280, image=..., id=-8804, this=0xf7ff91b0) at /usr/src/packages/user/exiv2/src/exiv2-0.27.1-Source/src/preview.cpp:549
549             if (Safe::add(offset_, size_) > static_cast<uint32_t>(image_.io().size()))
(gdb) bt
#0  (anonymous namespace)::LoaderExifJpeg::LoaderExifJpeg (parIdx=16750280, image=..., id=-8804, this=0xf7ff91b0) at /usr/src/packages/user/exiv2/src/exiv2-0.27.1-Source/src/preview.cpp:549
#1  (anonymous namespace)::createLoaderExifJpeg (id=id@entry=24, image=..., parIdx=parIdx@entry=0) at /usr/src/packages/user/exiv2/src/exiv2-0.27.1-Source/src/preview.cpp:557
#2  0xf7d07c30 in (anonymous namespace)::Loader::create (id=id@entry=24, image=...) at /usr/src/packages/user/exiv2/src/exiv2-0.27.1-Source/src/preview.cpp:389
#3  0xf7d096c0 in Exiv2::PreviewManager::getPreviewProperties (this=this@entry=0xffffdf54) at /usr/src/packages/user/exiv2/src/exiv2-0.27.1-Source/src/preview.cpp:1148
#4  0x0041ac74 in Action::Extract::writePreviews (this=this@entry=0xf7ffe4d0) at /usr/src/packages/user/exiv2/src/exiv2-0.27.1-Source/src/actions.cpp:1134
#5  0x004223e0 in Action::Extract::run (this=0xf7ffe4d0, path=...) at /usr/src/packages/user/exiv2/src/exiv2-0.27.1-Source/src/actions.cpp:1051
#6  0x00406b7c in main (argc=<optimized out>, argv=<optimized out>) at /usr/src/packages/user/exiv2/src/exiv2-0.27.1-Source/src/exiv2.cpp:169

on aarch64:

Breakpoint 1, (anonymous namespace)::LoaderExifJpeg::LoaderExifJpeg (parIdx=-1210642848, image=..., id=-4200, this=0xaaaaaaae7460) at /usr/src/packages/user/exiv2/src/exiv2-0.27.1-Source/src/preview.cpp:549
549             if (Safe::add(offset_, size_) > static_cast<uint32_t>(image_.io().size()))
(gdb) bt
#0  (anonymous namespace)::LoaderExifJpeg::LoaderExifJpeg (parIdx=-1210642848, image=..., id=-4200, this=0xaaaaaaae7460) at /usr/src/packages/user/exiv2/src/exiv2-0.27.1-Source/src/preview.cpp:549
#1  (anonymous namespace)::createLoaderExifJpeg (id=id@entry=24, image=..., parIdx=parIdx@entry=0) at /usr/src/packages/user/exiv2/src/exiv2-0.27.1-Source/src/preview.cpp:557
#2  0x0000ffffb7d72470 in (anonymous namespace)::Loader::create (id=id@entry=24, image=...) at /usr/src/packages/user/exiv2/src/exiv2-0.27.1-Source/src/preview.cpp:389
#3  0x0000ffffb7d73c78 in Exiv2::PreviewManager::getPreviewProperties (this=this@entry=0xfffffffff1f8) at /usr/src/packages/user/exiv2/src/exiv2-0.27.1-Source/src/preview.cpp:1148
#4  0x0000aaaaaaac3dec in Action::Extract::writePreviews (this=this@entry=0xffffb7acdf60) at /usr/src/packages/user/exiv2/src/exiv2-0.27.1-Source/src/actions.cpp:1134
#5  0x0000aaaaaaacaee8 in Action::Extract::run (this=0xffffb7acdf60, path=...) at /usr/src/packages/user/exiv2/src/exiv2-0.27.1-Source/src/actions.cpp:1051
#6  0x0000aaaaaaab1bd0 in main (argc=<optimized out>, argv=<optimized out>) at /usr/src/packages/user/exiv2/src/exiv2-0.27.1-Source/src/exiv2.cpp:169

Note the really weird parIdx and id in the not-x86 runs.

[D4N]

@awilfox could you run the test suite with ASAN + UBSAN on aarch64 & ppc? I don't have access to a machine with these architectures, so I cannot really test it. You can turn the sanitizers on by passing the -DEXIV2_TEAM_USE_SANITIZERS=ON flag to cmake.

[awilfox]

I'm trying very hard to manage to build the sanitisers against musl but it's been a long arduous process and they still aren't building. Unfortunately Google did not care much about portability when they wrote this. It may be a while before I can report anything, and I'm not sure the reports would be reliable even if I can end up generating them.

[D4N]

Which platform are you on? Alpine?

[awilfox]

Adélie Linux; we're a completely separate distro that is based on musl libc. (Alpine doesn't support PowerPC beyond a little-endian build that only supports POWER8 CPUs.)

When I have free time, I will try to install Fedora on my Raspberry Pi 3 and see if I can reproduce this issue on aarch64/glibc. Then I would be able to use ASan/UBSAN.

[dcermak]

This issue is also present on openSUSE on ARM and PPC, so this is likely not specific to Adélie Linux but rather to the CPU architecture differences.

[clanmills]

I'm going to close this. Team Exiv2 can't fix issues on unsupported platforms. We are happy to accept tested PR which don't disturb our test suite running on the platforms we actively support.

[martin-g]

Someone asked for help here - https://twitter.com/pixlsus/status/1405372762747637766
I am able to build the project by following the steps here - https://github.com/Exiv2/exiv2#21-build-install-use-exiv2-on-a-unix-like-system
But make tests fails with:

ubuntu@martin-arm64 ~/g/e/build (main)> make tests
make: *** No rule to make target 'tests'.  Stop.

In the Makefile I see many **test rules. Which one should I run ? Is there a way to run them all ?

[martin-g]

 make test
Running tests...
Test project /home/ubuntu/git/exiv2/build
    Start 1: bashTests
1/3 Test #1: bashTests ........................   Passed    8.42 sec
    Start 2: versionTests
2/3 Test #2: versionTests .....................   Passed    0.15 sec
    Start 3: pythonTests
3/3 Test #3: pythonTests ......................***Failed   14.59 sec

67% tests passed, 1 tests failed out of 3

Total Test time (real) =  23.16 sec

The following tests FAILED:
	  3 - pythonTests (Failed)
Errors while running CTest
make: *** [Makefile:118: test] Error 8

But it does not tell more details what exactly fails...

[hassec]

I think some of the README is unfortunately a bit outdated... :/

make test is the correct command.
When inside the build directory you can also call ctest --output-on-failure which will give you more output for the failing tests.

If you know what's failing you can then also go into the tests directory and run a test really verbosely by for example
python runner.py --verbose --debug bugfixes/github/test_pr_1691.py

python runner.py --help also might give you some more info ;)

[martin-g]

The difference on Linux ARM64 is that there is no exception thrown here: 341de45#diff-c7886fda6bb67352b3031d9a1f4950f7b6817fd8d9952087d82c7a71bd88c9b6R175-R178

This leads to different output on stderr and different return code.

[martin-g]

The problem is at

exiv2/src/preview.cpp

Line 526 in 19000cd

offset_ = pos->toLong();

Here on ARM64 it sets offset_ to 255, while on x86_64 it is 4294967295 and this leads to overflow.
I am trying to find out why it is 255 now.

[moodyhunter]

on riscv64 the offset_ is set to 255 as well, no idea why :( updated:

digging the code gives me some hints:

1. LoaderExifJpeg::LoaderExifJpeg(...) calls pos->toLong(),
2. long StringValueBase::toLong(long n) const calls value_.at(n) where value_ is a std::string,
3. std::string::at(size_type n) returns a char type,
4. the char type is cast to a long type (when returning from toLong)

The char type (without signed or unsigned prefix) on ARM64 is default unsigned, so that's effectively 255, however on x86_64 that is signed.

I have experienced this very same problem on a riscv64 virtual machine where char is unsigned too.

[CoelacanthusHex]

signedness of char type in c++/c is implementation-defined behavior, it should not be depended on.

C11, 6.3.0.3: whether a plain char is treated as signed is implementation-defined.

C++17, 6.7.1.1: In any particular implementation, a plain char object can take on either the same values as a signed char or an unsigned char; which one is implementation-defined.

In practice, x86 and its successor amd64 use signed implementations, and ARM, Power, and RISC-V use unsigned implementations.

[FantasqueX]

The newest version in main branch passes all tests on riscv64. However, it's hard to find which commit fixes this. Hope a new version can be released soon :)