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

Clam 1796 lha lzh archive support #1192

Merged
merged 9 commits into from
Apr 9, 2024

Conversation

micahsnyder
Copy link
Contributor

PR still requires some unit tests.

@micahsnyder micahsnyder requested a review from ragusaa March 2, 2024 17:15
@ragusaa
Copy link
Contributor

ragusaa commented Mar 7, 2024

ctest clamscan_valgrind fails.

If you can't reproduce, let me know and I'll help track it down.

libclamav_rust/build.rs Outdated Show resolved Hide resolved
@ragusaa
Copy link
Contributor

ragusaa commented Mar 7, 2024

Code looks good. Need to run tests.

libclamav/ole2_extract.c Outdated Show resolved Hide resolved
@ragusaa
Copy link
Contributor

ragusaa commented Mar 25, 2024

I added some code to scanners.rs to dump all files extracted to disk, to verify that they are the same as what 'lha' writes. I notice that when I scan the contents of lha.tar.gz, from https://jira-eng-sjc1.cisco.com/jira/browse/CLAM-2343, I do not get some of the files. The first one I notice missing is the contents of alg6.lha. Interestingly enough, if I scan only that file with clamscan, the contents are extracted.

File type magic signatures chosen based on the extensions supported
by Rust delharc crate.

See: https://docs.rs/delharc/latest/delharc/
Added a test that verifies extraction of two specific files from a set
of LZH files created with this utility:

https://github.com/jca02266/lha
Primarily this commit fixes an issue with the size of the parameters
passed to cli_checklimits(). The parameters were "unsigned long", which
varies in size depending on platform.
I've switched them to uint64_t / u64.

While working on this, I observed some concerning warnigns on Windows,
and some less serious ones, primarily regarding inconsistencies with
`const` parameters.

Finally, in `scanmem.c`, there is a warning regarding use of `wchar_t *`
with `GetModuleFileNameEx()` instead of `GetModuleFileNameExW()`.
This made me realize this code assumes we're not defining `UNICODE`,
which would have such macros use the 'A' variant.
I have fixed it the best I can, although I'm still a little
uncomfortable with some of this code that uses `char` or `wchar_t`
instead of TCHAR.

I also remove the `if (GetModuleFileNameEx) {` conditional, because this
macro/function will always be defined. The original code was checking a
function pointer, and so this was a bug when integrating into ClamAV.

Regarding the changes to `rijndael.c`, I found that this module assumes
`unsigned long` == 32bits. It does not.
I have corrected it to use `uint32_t`.
You only need to escape % for print statements, using a second %.
No point shifting a 16bit variable more than 16 bits.

Also add extra NULL terminator and inline documentation.
The delharc crate used to add LZH archive support appears to add
a dependency on macOS CoreFoundation library.

The error is:

[ 78%] Linking C shared library libclamav.dylib
Undefined symbols for architecture x86_64:
  "_CFRelease", referenced from:
      iana_time_zone::platform::get_timezone_inner::hc7da204717a39974 in libclamav_rust.a(iana_time_zone-bc4762a47da73d72.iana_time_zone.1863eb20d202562a-cgu.0.rcgu.o)
...
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [libclamav/libclamav.12.0.2.dylib] Error 1

We already link with CoreFoundation for libfreshclam and clamsubmit, so
this commit extends that to libclamav as well.
@micahsnyder micahsnyder force-pushed the CLAM-1796-lha-lzh branch 2 times, most recently from e18858d to dbf23db Compare March 28, 2024 15:52
We have 2 new variations on the statx false positive,
this time from calls from the Rust code.
I'm removing the mangled symbol to simplify the rule and accomodate
future variations.
@micahsnyder
Copy link
Contributor Author

I had to tweak the valgrind suppression rules a bit more, but it finally passed. I'm happy now.

@micahsnyder micahsnyder merged commit 8b2c81d into Cisco-Talos:main Apr 9, 2024
23 of 24 checks passed
@micahsnyder micahsnyder deleted the CLAM-1796-lha-lzh branch April 9, 2024 14:35
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.

2 participants