[libclamav] UPX unpack support: adding PE64, ELF32, ELF64 #1714
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f91cc3c1a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| upx64_sects = (upx_pe_section_t *)cli_max_calloc( | ||
| (size_t)upx64_nsect, sizeof(upx_pe_section_t)); | ||
| if (!upx64_sects) | ||
| goto upx64_bail; |
There was a problem hiding this comment.
Propagate PE64 UPX allocation failures instead of returning clean
When allocation of upx64_sects (and similarly upx64_dest) fails, control jumps to upx64_bail and then falls through to the PE32+ early-exit path that returns CL_CLEAN. In low-memory conditions this silently disables PE64 UPX unpacking and can produce clean results for files that require unpacked-content detection. This path should return an error (or another explicit failure signal) rather than reporting clean.
Useful? React with 👍 / 👎.
This is an experimental branch that adds UPX unpacking support for PE64, ELF32, ELF64 in C
It should be carefully reviewed. I would recommend a dedicated pass by a vuln team due to the complexity and working on untrusted data.
It has been tested with all supported compression types of UPX from v1.2 - 5.1.1 current. There are 4 new unit tests included in this repo. The full test corpus can be found in a standalone test harness where primary development was done. I have made this standalone code public as well to aid in testing:
https://github.com/dzim2/clam_upx_test
Note that I moved some of the UPX PE32 brute force unpacking code from the main pe.c module into the main upx.c.
There was also a magic numeric shared between the two that could get out of sync, so I renamed it as a const for clarity.
is_upx_pe32() is currently only used in the test harness because upx.c already contains a spread out set of checks shared with other packers I did not want to disrupt.
is_upx_pe64() and is_upx_elf() should be evaluated to see if they are to strict leading to potential bypass. There will be a balance to achieve there.
AI assistant was Claude Sonnet with additional reviews from Opus and Chatgpt.
The test harness was done on Windows and VS2022, it was then merged into libclamav and tested on Ubuntu. I do not have access to other build environments.
Test project /home/dave/clamav/build
Start 1: libclamav
1/13 Test #1: libclamav ........................ Passed 28.88 sec
Start 2: libclamav_valgrind
2/13 Test #2: libclamav_valgrind ............... Passed 534.64 sec
Start 3: upx_tests
3/13 Test #3: upx_tests ........................ Passed 1.08 sec
Start 4: upx_tests_valgrind
4/13 Test #4: upx_tests_valgrind ............... Passed 16.97 sec
Start 5: libclamav_rust
5/13 Test #5: libclamav_rust ................... Passed 32.45 sec
Start 6: clamscan
6/13 Test #6: clamscan ......................... Passed 16.52 sec
Start 7: clamscan_valgrind
7/13 Test #7: clamscan_valgrind ................ Passed 771.98 sec
Start 8: clamd
8/13 Test #8: clamd ............................ Passed 28.96 sec
Start 9: clamd_valgrind
9/13 Test #9: clamd_valgrind ................... Passed 242.48 sec
Start 10: freshclam
10/13 Test #10: freshclam ........................ Passed 2.35 sec
Start 11: freshclam_valgrind
11/13 Test #11: freshclam_valgrind ............... Passed 109.60 sec
Start 12: sigtool
12/13 Test #12: sigtool .......................... Passed 1.60 sec
Start 13: sigtool_valgrind
13/13 Test #13: sigtool_valgrind ................. Passed 74.65 sec
100% tests passed, 0 tests failed out of 13
Total Test time (real) = 1862.17 sec