Skip to content

Fix potential vulnerable cloned zlib-functions#6274

Merged
mvieth merged 2 commits intoPointCloudLibrary:masterfrom
npt-1707:fix-CVE-2018-25032
May 3, 2025
Merged

Fix potential vulnerable cloned zlib-functions#6274
mvieth merged 2 commits intoPointCloudLibrary:masterfrom
npt-1707:fix-CVE-2018-25032

Conversation

@npt-1707
Copy link
Contributor

Description
This PR fixes a potential vulnerability that was cloned from zlib but did not receive the security patch. The original issue was reported and fixed under madler/zlib@5c44459.
This PR applies the same patch to eliminate the vulnerability.

References
https://nvd.nist.gov/vuln/detail/CVE-2018-25032
madler/zlib@5c44459

@mvieth
Copy link
Member

mvieth commented Apr 28, 2025

Hi, thank you for the pull request. Unfortunately, with your changes PCL does not build any more when using the CMake options -DWITH_SYSTEM_ZLIB=OFF -DBUILD_surface_on_nurbs=ON (because only then surface/src/3rdparty/opennurbs/deflate.c and surface/src/3rdparty/opennurbs/trees.c are used, otherwise a system-installed zlib is used). The errors are mostly related to struct member not being found. The commit in the original zlib repo that you linked, has some changes that this pull request has not, for example changes in deflate.h. So the build errors may be fixed if you make sure to copy all changes from the linked commit.
However, since your pull requests are not the first ones to fix bugs in the copied zlib files, I am wondering if it would make more sense to completely remove the files copied from zlib, and make PCL always require a system-installed zlib (should be available anyway on most systems). What do you think @larshg

@mvieth mvieth added module: surface changelog: fix Meta-information for changelog generation labels Apr 28, 2025
@larshg
Copy link
Contributor

larshg commented Apr 30, 2025

Maybe we should keep the zlib files here in 1.15.1 and then remove it from 1.16? Even though we haven't set the internal used files as deprecated, I wouldn't mind removing the copied files from 1.16, since it has been preferred to use system lib and we would probably have seen a lot of issues, if that wouldn't have worked most places?

@mvieth
Copy link
Member

mvieth commented Apr 30, 2025

Maybe we should keep the zlib files here in 1.15.1 and then remove it from 1.16?

Sound like a good idea.

Even though we haven't set the internal used files as deprecated, I wouldn't mind removing the copied files from 1.16, since it has been preferred to use system lib and we would probably have seen a lot of issues, if that wouldn't have worked most places?

Yes, I agree. We could add one or two new tests to make extra sure that using a system zlib does not change anything, but I don't think it does.

@npt-1707 Then please take a look at which changes are missing in this pull request, to make sure that building with -DWITH_SYSTEM_ZLIB=OFF -DBUILD_surface_on_nurbs=ON still works, so we can include this fix in the next PCL release. Thanks!

@npt-1707
Copy link
Contributor Author

npt-1707 commented May 1, 2025

Hi @mvieth, there are some missing changes in this PR compared to the commit from zlib because those changes in that commit fixed some functions that have same names but different code snippets compared to your code. Sorry that I cannot support you in this work.

Please consider my PR as a suggestion for reviewing zlib version and update them to avoid some vulnerabilities. Many thanks for your time!

Copy link
Member

@mvieth mvieth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the additional changes in deflate.h, it builds successfully.

@mvieth mvieth merged commit 0dec791 into PointCloudLibrary:master May 3, 2025
13 checks passed
@mvieth mvieth changed the title Fix potential vulnerable cloned functions Fix potential vulnerable cloned zlib-functions Jul 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog: fix Meta-information for changelog generation module: surface

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants