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

Patch libpng usage to eliminate known vulnerabilities #537

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

clang-clang-clang
Copy link
Contributor

Related to #534, this PR focuses on the modifications to Android and keeps the changes as simple as possible (the changes made should be consistent and use CMake to introduce libpng to increase flexibility for future changes, with the intention of splitting it into two steps, such as adding ASM support later). Other platforms only modify the source code version and maintain the original integration method. It has been compiled and tested successfully in Android with CMake and Emscripten with gmake, but cannot be tested properly in Android with Visual Studio and Emscripten with Visual Studio. Relevant personnel are requested to check the operation status. Linux and NXS should not be affected, please also check.

Analysis

By searching for references to the png.h header file, it is found that:

  1. Android uses Native libpng to read "*.png" files through platform/android/ndk/AndroidImageDecoder.cpp, and if that fails, it will use Java decoding (note that during testing, fallback to Java implementation should be avoided).
  2. Emscripten/Linux/NXS uses libpng to read and write png files through platform/shared/Rtt_BitmapUtils.cpp.
  3. The third-party library freetype also has code referencing "png.h" but it is not actually used because the FT_CONFIG_OPTION_USE_PNG definition is not turned on by default.

By analyzing the compilation scripts for each platform, it is found that the source code version, compilation and linking are as follows:

  1. Android uses external/lpng1256, which is the same as sourceforge.net lpng1256.zip, except that the line breaks may be inconsistent. For GitHub Action, Android uses gradle script to directly refer to libpng source code files through CMake, and does not use the CMake script provided by libpng. The Visual Studio project file is another toolchain and cannot be compiled successfully. The Visual Studio project platform points to Tegra-Android, and it is suspected whether it can be used normally.
  2. Emscripten uses external/libpng1243b01, which is the same as sourceforge.net libpng-1.2.43beta01.tar.gz. It is not recommended for use beta in production environments, and it has NXS's predefined modifications (which have also been moved to the git module and annotated), and the predefined have no impact on Android. For GitHub Action, libpng source code is referred to through gmake, and the CMake script provided by libpng is not used. There are also additional Visual Studio project files, but the compilation fails and there is no corresponding processing for CopyResource.sh, which used to create resource.car on Android or resource.corona-archive on iOS, and it is suspected that it is incomplete.
  3. Linux uses find_package() to determine the version installed, so this PR modification of the source code version does not affect Linux (maybe NXS also uses Linux scripts? Or maybe it uses source code integration, which needs to be tested. I do not have NXS or developer access, so I hope someone can test it).

Modification

In order to reduce redundancy, version 1.6.39 is used uniformly. The open source license of libpng 1.6.39 is compatible with Solar2D, and cve-bin-tool no longer detects related vulnerabilities.

Except for NXS header file definition changes, lpng1256 or libpng1243b01, both are version 1.2. The main changes from version 1.2 to 1.6.39 are:

  • Two useless c files were deleted Version 1.4.0beta94 [November 9, 2009] CHANGES: pnggccrd.c and pngvcrd.c.
  • The pnglibconf.h was moved to pnglibconf.h.prebuilt(first in 1.5.0). Check the libpng CMake script shows that the configuration is consistent with Solar2D's direct reference to source code files.
  • The access rights of the public header files have been tightened, and getter is used to obtain the members of the data structure to avoid changes. This has been applied in platform/shared/Rtt_BitmapUtils.cpp, but Android needs to be updated.
  • The libpng15 dereference BUG mentioned in 2c211d8 just now has been tested to be normal in Emscripten with gmake in version 1.6.39. I hope it can be tested again. @bakeinflash

The modifications to libpng (may need to be forked to coronalabs by @Shchvova, and modify the corona repository reference) are:

  • The predefined values of NXS come from external/libpng1243b01, but it needs to be determined whether NXS's png source comes from the source code or find_package.
  • Copying pnglibconf.h from pnglibconf.h.prebuilt is to adapt to the custom integration methods of Emscripten with gmake, Emscripten with Visual Studio, and Android with Visual Studio. If it is migrated to CMake later, it may not be needed.
  • ASM should be turned off. Checking the CMake script and comparing it with the original integration method, it is not turned on. If ASM is turned on in the top-level sdk/CMakeLists.txt and then PNG_HARDWARE_OPTIMIZATIONS option is turned OFF, it will cause other third-party libraries to fail to build. Libpng should determine whether ASM is turned on to decide whether to set PNG_HARDWARE_OPTIMIZATIONS to ON. In order to reduce the scope of influence, other third-party libraries are not modified for ASM for the time being, so it is turned off temporarily.

Test

Using Sample Code Graphics/Fishies, And our colleague used over 13,000 png atlas files for testing display.newImage() read and display, and display.save() to write. The written files looked normal and were not binary-identical files:

  • ✅ On macOS host, Android using cmake, with Java Decoder fallback disabled, test PASSED.
    • ⚠️ BUT using the command line pngtest --strict pngtest.png compiled with NDK r18b fails the test, r20b works. It seems to have no effect on writing, because Android uses Java to write, not native libpng. It is recommended to upgrade to r20b when targeting API 33 on Android, perhaps I need to fire a bug to libpng?
  • ✅ On macOS host, Emscripten (2.0.34, consistent with Github Action) using gmake, tested normal reading, display, and save, test PASSED. (Save with no warning. Seems wasm use virtual fs instead of local storage so I can not find the real file?)
  • ❓On Windows host, Android with Visual Studio fails to build Solar2D app (maybe incomplete for Android build?), needs to be answered and guided by documentation.
  • ❓On Windows host, Emscripten with Visual Studio fails to build Solar2D app, needs to be answered and guided by documentation.(PR contains some fixes)
  • ✅ Linux using find_package should not be affected, no need to test.
  • ❓NXS needs to be tested, but because Rtt_BitmapUtils.cpp has been verified by Emscripten, there should be no problem. Can you help me test it?

Questions

  1. Where is the original premake project for gmake? Or is it modified manually?
  2. Where is the NXS compilation toolchain completed? Is it the same as Linux's?
  3. Are the Visual Studio projects for Android and Emscripten complete and effective?
  4. Are all Emscripten versions using 2.0.34? (GitHub Action, Visual Studio)
  5. Maybe we should consider unified project development and testing? Export to different IDEs using CMake instead of maintaining them separately?
  6. What is platform/linux/CMakeRsyncLinuxLists.txt used for?

Thank you for taking your time to read this “long text” :D

@clang-clang-clang
Copy link
Contributor Author

I consider marking PR as complete and continuing to investigate "the libpng failed the test on Android adb shell" elsewhere.

For the following reasons:

  • lpng1256 also failed the test, it is not a new issue introduced by libpng 1.6.39.
  • The problem only exists for native libpng writes that are not used by Android.
  • pngout.png "looks" same as pngtest.png.
  • When the first output, pngout.png, is used as input to the second pngtest, the test passes.

Test steps are as follows:

export ANDROID_NDK=~/Library/Android/sdk/ndk/18.1.5063045
export PATH=$ANDROID_NDK/toolchains/llvm/prebuilt/darwin-x86_64/bin:$PATH

# In the lpng1256 directory
mkdir build && cd build
cmake .. -DCMAKE_TOOLCHAIN_FILE=$ANDROID_NDK/build/cmake/android.toolchain.cmake -DANDROID_ABI=arm64-v8a -DANDROID_PLATFORM=android-21 -DPNG_HARDWARE_OPTIMIZATIONS=OFF
cmake --build .

# Ensure Android device connected, and you will need to remove symbol links before push
adb push ../../lpng1256 /data/local/tmp

# Into adb shell
adb shell

# Execute inside adb shell
export LD_LIBRARY_PATH=/data/local/tmp/lpng1256/build
cd /data/local/tmp/lpng1256/build
# Will output pngout.png and report nonidentical with pngtest.png
./pngtest --strict ../pngtest.png

Compare ../pngtest.png and pngout.png get a SHADOW:

image

@clang-clang-clang clang-clang-clang changed the title WIP: Patch libpng usage to eliminate known vulnerabilities Patch libpng usage to eliminate known vulnerabilities Mar 27, 2023
@clang-clang-clang
Copy link
Contributor Author

This update eliminates the following vulnerabilities:

@ggcrunchy
Copy link
Contributor

I discovered the ImageDecoder just a couple days ago, and wonder if it might be a more graceful way to update; there are some speed issues with the current Android code. (I have some related changes but don't believe I've uploaded them yet.)

(I also found this which is relevant to masks.)

Other ideas—with a focus on speed and correctness, but missing most of the non-PNG formats—might be Wuffs (see here for an implementation; I was using this on top of these features, so it might still have Windows dependencies), or fpng, although it sounds like that would be device-only (and probably want a build option + runtime-detected flag).

@clang-clang-clang
Copy link
Contributor Author

You're right, Thanks, I'll check it out.
This PR was supposed to eliminate CVE vulnerabilities by upgrading libpng, without considering the speed problem for the time being. If the speed and compatibility of native libpng is not as good as the built-in library of Android after a lot of tests, then we have reason to delete native libpng.

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 this pull request may close these issues.

None yet

2 participants