OSS-Fuzz issues in new video code

[kevinbackhouse]

OSS-Fuzz has immediately started finding issues in the recently added video code. This tarball contains the pocs for the two issues reported so far:

clusterfuzz.tar.gz

To reproduce:

cmake --preset linux-sanitizers -S . -B build-fuzz -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_COMPILER=$(which clang++) -DEXIV2_BUILD_FUZZ_TESTS=ON -DEXIV2_BUILD_UNIT_TESTS=OFF
cmake --build build-fuzz --parallel
cd build-fuzz
./bin/fuzz-read-print-write clusterfuzz-testcase-minimized-fuzz-read-print-write-6384569643565056.fuzz 

I'm quite worried that fixing all the problems in this new code is going to be a multi-month treadmill, like it was for QuickTimeVideo recently. None of the 3 pull requests (#2413, #2415, #2416) added any new test files to test/data, even though I asked for that early on, so I think this code is very poorly tested.

Here's my suggestion: let's roll back these pull requests until they've been tested better. Exiv2 was in really good shape from a code quality perspective before these PRs landed, so I think now would be a good time to prepare 0.27.6 and 1.0.0 releases based on the state of the code as it was on 2022-12-30.

[piponazo]

What do you think about disabling the video support via the CMake option which @mohamedchebbii is proposing here #2448 . We could clearly indicate that the support for video formats is experimental and not encouraged for production systems (and therefore disabled by default).

My concern about rolling back these changes is that if the code is not available in main people will not work on the improvements.

[kevinbackhouse]

What do you think about disabling the video support via the CMake option which @mohamedchebbii is proposing here #2448 . We could clearly indicate that the support for video formats is experimental and not encouraged for production systems (and therefore disabled by default).

My concern about rolling back these changes is that if the code is not available in main people will not work on the improvements.

Yes, that sounds like a good solution.

[benmccann]

Yes, I definitely think it will be easier to collaborate on the code and improve it with it in main. I think even if we want to mark the code as experimental, it may still help to have it included by default as it would make it easier to get feedback and identify test cases that should be added to the code base before bringing it out of experimental.

[mohamedchebbii]

OSS-Fuzz has immediately started finding issues in the recently added video code. This tarball contains the pocs for the two issues reported so far:

clusterfuzz.tar.gz

To reproduce:

cmake --preset linux-sanitizers -S . -B build-fuzz -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_COMPILER=$(which clang++) -DEXIV2_BUILD_FUZZ_TESTS=ON -DEXIV2_BUILD_UNIT_TESTS=OFF
cmake --build build-fuzz --parallel
cd build-fuzz
./bin/fuzz-read-print-write clusterfuzz-testcase-minimized-fuzz-read-print-write-6384569643565056.fuzz 

I'm quite worried that fixing all the problems in this new code is going to be a multi-month treadmill, like it was for QuickTimeVideo recently. None of the 3 pull requests (#2413, #2415, #2416) added any new test files to test/data, even though I asked for that early on, so I think this code is very poorly tested.

Here's my suggestion: let's roll back these pull requests until they've been tested better. Exiv2 was in really good shape from a code quality perspective before these PRs landed, so I think now would be a good time to prepare 0.27.6 and 1.0.0 releases based on the state of the code as it was on 2022-12-30.

Hello @kevinbackhouse ,
I'm sorry to hear that my PRs does not succeed to pass the Fuzz tests;
I will add the Fuzz test/data in the next few days.
Please bear with me, I'm new to this project :)
I will be grateful if you advise and give some references or documentation on how to add fuzz test data in exiv2.

[kevinbackhouse]

Hi @mohamedchebbii: thanks for looking into this! For better test coverage, what we need is a small video file. You can see an example in #2337. The relevant files in #2337 are test/data/small_video.mp4, test/data/test_reference_files/small_video.mp4.out, and tests/regression_tests/test_regression_allfiles.py. The video file should be as small as possible to avoid bloating the size of the git repo, but it should be a valid video file that can be viewed.

[benmccann]

After the initial issues are fixed, what the best way to see if additional OSS Fuzz issues are found or if they're all fixed? I did a search to find an issue (https://bugs.chromium.org/p/oss-fuzz/issues/list?can=2&q=exiv2), but am not sure if this is the best link to use or not

[mohamedchebbii]

Hello @benmccann,
Thank you for the provided link, I will check It
and will fix fuzz issue for the video format here #2458

[kevinbackhouse]

The OSS-Fuzz issues are reported to me privately, but I'll upload any new pocs when I get them. Right now our OSS-Fuzz build is failing due to my recent libinih changes, so we will not receive any new issues until I have fixed that.

[benmccann]

Thanks for the details. And I see you fixed the libinih issue as well: google/oss-fuzz#9437 (comment)

[kevinbackhouse]

Yes, the OSS-Fuzz build failure caused by inih is fixed. But the other thing that has happened is that OSS-Fuzz has stopped building with video support enabled, because #2448 changed the default value of EXIV2_ENABLE_VIDEO to OFF. So OSS-Fuzz thinks (incorrectly) that all the bugs are fixed and has disclosed them here.

[clanmills]

What do you think about disabling the video support via the CMake option which @mohamedchebbii is proposing here #2448 . We could clearly indicate that the support for video formats is experimental and not encouraged for production systems (and therefore disabled by default).

My concern about rolling back these changes is that if the code is not available in main people will not work on the improvements.

Security should be our top priority. Exiv2 is almost 20 years old, and an essential part of Linux. We should not release experimental code. If there is insufficient time to make code robust, we should remove it.

A build switch does not protect experimental code. For example the 0.27.6 default for -DEXIV2_ENABLE_VIDEO=Off, yet Linux from Scratch recommends On: https://www.linuxfromscratch.org/blfs/view/svn/general/exiv2.html

The Video code in 0.27-maintenance was contributed in 2012 by a GSoC student. Ten years later, it has not been strengthened. If there isn't time to make code robust, there will not be more time in the future.

[benmccann]

Ten years later, it has not been strengthened. If there isn't time to make code robust, there will not be more time in the future.

It is already being worked on: #2458

[postscript-dev]

Security should be our top priority. Exiv2 is almost 20 years old, and an essential part of Linux. We should not release experimental code. If there is insufficient time to make code robust, we should remove it.

A build switch does not protect experimental code.

I don't know if it is enough but the following suggestions may help.

Perhaps the description of the video build option (CMakeLists.txt) could explain that it is currently experimental and is used at your own risk. I might have suggested changing the name of the flag to add _EXPERIMENTAL but hopefully the video code will be stable by the next next release.

When releasing, some text could also be added to exiv2.org and the release notes to make the video situation clearer. In addition, README.md would benefit from a small update.

[benmccann]

#2458 was just merged which fixes the known OSS-Fuzz issues.

Should we set EXIV2_ENABLE_VIDEO back to ON so that OSS-Fuzz starts fuzzing the video code again? Or did you want to fuzz some more on your own machine first @kevinbackhouse?

[kevinbackhouse]

I ran the fuzzer with the latest changes in 976dcd8 and found some new issues:

pocs.tar.gz

@benmccann @mohamedchebbii

[benmccann]

It's been a couple weeks since the known issues were fixed. Should we close this issue or were any additional issues found after running the fuzzer for the past couple of weeks?

[kevinbackhouse]

@benmccann: I just ran the fuzzer again and here are the latest issues that it found (tested on commit b9d94e6):

pocs.tar.gz

[1div0]

Great!

[benmccann]

Thanks @kevinbackhouse! The fix for that has been merged. Can you run again and let us know if you find any new issues?

[kevinbackhouse]

@benmccann: The crash is fixed, but the "slow-unit-..." files are still a problem. I've posted a PR here: #2533

[kevinbackhouse]

I reran the fuzzer with the fix from #2533 and it found new failures: pocs.tar.gz

[kevinbackhouse]

The fuzzer has been running for 15 hours without finding any new issues. 🥳