-
-
Notifications
You must be signed in to change notification settings - Fork 279
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
base: master
Are you sure you want to change the base?
Conversation
pull from upstream
I consider marking PR as complete and continuing to investigate "the libpng failed the test on Android adb shell" elsewhere. For the following reasons:
Test steps are as follows:
Compare |
This update eliminates the following vulnerabilities: |
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). |
You're right, Thanks, I'll check it out. |
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:
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).platform/shared/Rtt_BitmapUtils.cpp
.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:
Tegra-Android
, and it is suspected whether it can be used normally.CopyResource.sh
, which used to createresource.car
on Android orresource.corona-archive
on iOS, and it is suspected that it is incomplete.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:
pnggccrd.c
andpngvcrd.c
.pnglibconf.h
was moved topnglibconf.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.getter
is used to obtain the members of the data structure to avoid changes. This has been applied inplatform/shared/Rtt_BitmapUtils.cpp
, but Android needs to be updated.The modifications to libpng (may need to be forked to coronalabs by @Shchvova, and modify the corona repository reference) are:
pnglibconf.h
frompnglibconf.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.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 setPNG_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, anddisplay.save()
to write. The written files looked normal and were not binary-identical files: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?Questions
platform/linux/CMakeRsyncLinuxLists.txt
used for?Thank you for taking your time to read this “long text” :D