suricata
suricata copied to clipboard
file/swf: Use lzma-rs decompression instead of libhtp.
Use the lzma-rs crate for decompressing swf/lzma files instead of the lzma decompressor in libhtp. This decouples suricata from libhtp except for actual http parsing, and means libhtp no longer has to export a lzma decompression interface.
Make sure these boxes are signed before submitting your Pull Request -- thank you.
- [ X] I have read the contributing guide lines at https://redmine.openinfosecfoundation.org/projects/suricata/wiki/Contributing
- [ X] I have signed the Open Information Security Foundation contribution agreement at https://suricata.io/about/contribution-agreement/
- [ X] I have updated the user guide (in doc/userguide/) to reflect the changes made (if applicable)
Reference libhtp-rs PR comment suggesting this change.
Describe changes:
- Create a lzma.rs file which exports a lzma decompression function interface.
- In util-file-swf-decompression.c, use this interface instead of the lzma decompression interface presented by libhtp.
- Update error types / constants to reflect lzma-rs error conditions / results.
Codecov Report
Merging #7625 (55d13aa) into master (a2f857e) will increase coverage by
0.06%
. The diff coverage is20.00%
.
@@ Coverage Diff @@
## master #7625 +/- ##
==========================================
+ Coverage 75.73% 75.80% +0.06%
==========================================
Files 659 659
Lines 185740 185727 -13
==========================================
+ Hits 140669 140783 +114
+ Misses 45071 44944 -127
Flag | Coverage Δ | |
---|---|---|
fuzzcorpus | 60.26% <0.00%> (+0.41%) |
:arrow_up: |
suricata-verify | 52.49% <0.00%> (+0.05%) |
:arrow_up: |
unittests | 60.71% <20.00%> (-0.01%) |
:arrow_down: |
Flags with carried forward coverage won't be shown. Click here to find out more.
Thanks Todd. How does this deal with the issue of lzma preallocating some attacker controlled u32 value? The reason we vendored the lzma code was to address this.
@victorjulien if I remember correctly, you mentioned that this whole code (SWF decompression) should be deprecated/removed at some point.
Should it be removed now ?
Thanks Todd. How does this deal with the issue of lzma preallocating some attacker controlled u32 value? The reason we vendored the lzma code was to address this.
We actually sent the lzma-rs project some patches to add limits to prevent this kind of attack. It was part of our assessment for whether or not we could use this crate in libhtp-rs for lzma decompression.
https://github.com/gendx/lzma-rs/commit/fcbb11fab6f8389067a266f40a74d15f02bf30ea
Looks like we could drop the check for zlib in configure.ac?
Looks like we could drop the check for zlib in configure.ac?
Why ? zlib will still be used in libhtp
Why ? zlib will still be used in libhtp
libhtp has its own check for zlib which in the end results in suricata
dynamically linking with it anyways.
Looks like we could drop the check for zlib in configure.ac?
util-file-swf-decompression still uses zlib to decompress CWS compressed swf archives. This PR only changes the lzma decompression path. zlib is still directly used for the CWS path, and this file includes zlib.h.
Reposting as it is lost in GitHub flow :
@victorjulien if I remember correctly, you mentioned that this whole code (SWF decompression) should be deprecated/removed at some point.
Should it be removed now ?
Reposting as it is lost in GitHub flow :
@victorjulien if I remember correctly, you mentioned that this whole code (SWF decompression) should be deprecated/removed at some point.
Should it be removed now ?
I don't think so, but it should probably not be enabled (at runtime) by default as flash is supposed to be a thing of the past.
if I remember correctly, you mentioned that this whole code (SWF decompression) should be deprecated/removed at some point. Should it be removed now ?
I don't think so, but it should probably not be enabled (at runtime) by default as flash is supposed to be a thing of the past.
@cccs-rtmorti could you add that to your PR ? That is change the default suricata.yaml config option for swf-decompression
fields
@cccs-rtmorti could you add that to your PR ? That is change the default suricata.yaml config option for
swf-decompression
fields
Yeah, no problem.
Updated in #8132